jborgers / pmd-jpinpoint-rules Goto Github PK

separate test-code cases: get, getString, harmless; add testcase with fully qualified classname (no import)

First 4 rules:

• AvoidApacheCommonsFileItemNonStreaming
• AvoidCalendarDateCreation
• AvoidConcatInAppend and
• AvoidConcatInLoop

Use unit like milliseconds or seconds in the name of time quantities like timeout, expirytime, timeToLive, to avoid confusion and errors about units.
For variable names and @value members

example:
bad:
@Autowired
public RetrieveCache(final @value("${cache.expiryTime}") long timeToLive) {
super(timeToLive);
}
good:
@Autowired
public RetrieveCache(final @value("${cache.expiryTimeSeconds}") long timeToLiveSeconds) {
super(timeToLiveSeconds);
}

Similar to NotProperlySynchronizingOnThisWhileUsingGuardedBy, see #43
Now for the field case.
Examples:

   import net.jcip.annotations.GuardedBy;
   class Bad1 {
        private final Object LOCK = new Object();
        @GuardedBy("LOCK")
        private String txt;

        public String getText() { return txt; } // bad
        public synchronized void setText(String t) { txt = t; } // bad
    }

    import net.jcip.annotations.GuardedBy;
    class Good1 {
        private final Object LOCK = new Object();
        @GuardedBy("LOCK")
        private String txt;

        public String getText() {
            synchronized(LOCK) { return txt; }
        }
        public void setText(String t) {
            synchronized(LOCK) { txt = t; }
        }
    }

Juan of the PMD-team writes:

The ruleset schema can be as lean as:

< ruleset name="Best Practices"
xmlns="http://pmd.sourceforge.net/ruleset/2.0.0"
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:schemaLocation="http://pmd.sourceforge.net/ruleset/2.0.0 https://pmd.sourceforge.io/ruleset_2_0_0.xsd" >
(notice we use https and changed the domain that serves the xsd). You actually don’t even need to define xmlsn:fn.

AvoidImplicitelyRecompilingRegex String.split fires on long regex, literals or by fields.
External fields, from other classes, cannot be checked and currently fire a violation. This can be a false positive. We want to avoid false positives, so external fields should be assumed not to be a problem.
Example:

public class Try {
    private static final String SMALL = "-.";
    private static final String LARGE = "------.-";
    public static void foo(String query) {
        query.split(Constants.SEP); //assumed good, external
        query.split(SMALL); //good
        query.split(LARGE); //bad
    }
}

An XML Factory like DocumentBuilderFactory, TransformerFactory, MessageFactory is used as static field.

Problem: These factory objects are typically not thread-safe and rather expensive to create because of class loading.

Solution: Create the Factories as local variables and use command line arguments to specify the implementation class.

public class Bad1 {
private static final DocumentBuilderFactory DB_FACTORY = DocumentBuilderFactory.newInstance(); // bad
private static SAXParserFactory sp_factory; // bad
static MessageFactory M_FACTORY; // bad
static final TransformerFactory T_FACTORY = TransformerFactory.newInstance(); // bad
protected static final XMLInputFactory XI_FACTORY = XMLInputFactory.newInstance(); //bad
static XMLOutputFactory XO_Factory; //bad
static XMLEventFactory XE_Factory; //bad
static DatatypeFactory DT_Factory; //bad

SchemaFactory S_FACTORY = SchemaFactory.newInstance(); // good
private static CustomerFactory C_FACTORY; // good

public static void build() {
    DocumentBuilderFactory dbFactory = DocumentBuilderFactory.newInstance(); // good
    // use dbFactory
}

}

class CustomerFactory {
}

Can you create a rule to detect a potential memory leak when CDI.current().select is used?:

The following code will have a memory leak when the finally destroy is not used.

See more info: https://blog.akquinet.de/2017/01/04/dont-get-trapped-into-a-memory-leak-using-cdi-instance-injection/

MyClass myObject= CDI.current().select(MyClass.class).get();
try {
....//do stuff
} finally {
CDI.current().destroy(myObject);
}

DecimalFormat and ChoiceFormat are not thread safe

Static blocks are not yet supported, they should. Tests:

class Formatted {
private static final String[] names_violate;
private static final String[] names_ok;
private static final ThreadLocal[] FORMATTERS;
private static final String[] nonEmptyViolate;
private static final String[] emptyOk;
private static final Map mapOk;
private static final Map mapViolate;
@GuardedBy("lock")
private static final Map mapOkGuarded;
private static final ConcurrentMap mapSafe;

static {
    names_violate = new String[]{"aap", "noot", stringViolate1}; //bad
    names_ok = new String[]{};
    FORMATTERS = new ThreadLocal[NUMBER_OF_FORMATTERS]; //bad
    emptyOk = new String[0];
    nonEmptyViolate = new String[5]; // bad
    mapOk = Collections.emptyMap();
    mapViolate = new HashMap(); // HashMap is known mutable type // bad
    mapOkGuarded = new HashMap();
    mapSafe = new ConcurrentHashMap();
}

}

When using @ConfigurationProperties(prefix="") to inject configuration properties into a Spring-Boot application, Sonar returns the pmd:AvoidUnguardedAssignmentToNonFinalFieldsInSharedObjects warning. Since the setters of this class are only called once, this kind of thread-safety rules are not necessary in these cases and classes with this annotation can be excepted from this rule.
Kind regards,
Stijn Schmeink

When calling method setAttribute on a class which is not an HttpSession, pmd:MinimizeAttributesInSession is still triggered resulting in a false positive. Possible cause is a literal scan for the method setAttribute without checking if it is actually called on an HttpSession.

We have AvoidSqlInExpression which deals with several representations of the IN-clause in Java.
Add coverage of javax.persistence.criteria.Path/Expression.in()

Spring Expression Language (SpEL) expression is used for computing the key dynamically. Problem: evaluating the expression language is expensive, on every call.
Solution: use a custom KeyGenerator: keyGenerator=... instead of key=...

example:
class Bad1 {
@Cacheable(value = "Cache1", key = "#key1") // bad
public String bad1(final String key1) {
return getRemote(key1);
}
}

Make a rule that violates on annotation combinations that make no sense, like @ Data and @ EqualsHashCode (lombok); @ Configuration and @ Component (spring); @ Entity and @ Component (spring).

When analyzing violations of concurrency checks and equals-hashCode checks, it turned out some were false positives because code had wrong annotation combinations. So, these combinations should be correct, hence this rule.

This code gives a false positive, please fix:

@RequestMapping(value = "/info/all", method = RequestMethod.GET)
public ResponseEntity agentInfoAll() {
    String result = getAllServers()
            .stream()
            .filter(server -> !server.startsWith(GROUP_OF_SERVERS_PREFIX))
            .map(server -> {
                CompletableFuture<ResponseEntity<String>> futureResponse =
                        CompletableFuture.supplyAsync(() -> agentClient.pingAgent(server), executor);
                ResponseEntity<String> response = futureResponse
                        .handle((res, ex) -> (res != null) ? res : ResponseEntity.status(HttpStatus.INTERNAL_SERVER_ERROR).body("Error pinging server: " + ex.getMessage()))
                        .join();
                return server + " : " + buildStatus(response) + "\n" + response.getBody() + "\n\n";
            })
            .collect(Collectors.joining());

    return ResponseEntity.ok().contentType(MediaType.TEXT_PLAIN).body(Pattern.compile("\n{2,}").matcher(result).replaceAll("\n\n"));
}

Support Spring 5 @requestScope annotation just like @scope("request")

Now merging into company specific and all rules file are two runs of the tool. Easy to forget one. Merging all rules needs 3 arguments which are typically always the same.

No args should be needed to generate both files in one go.

False positive:
@component
@scope("request")
// Only request is safe
class AComponent4Ok {
private String url; //
public void setUrlOk(final String url) {
this.url = url;
} // no violation: request scope
}

We don't want threads to block without timeouts.
In a test without the stub, threads stay being blocked on completionService.take.
We want the completionService.poll(timeout) to be used.

If your @component inherits fields they are shared and should be accessed in a thread-safe way. If they are final that is fine, but otherwise access is not fine. Assignment is a violation and also access methods like put, set, add and clear are a violation. For latter, a new rule with name could be:
AvoidUnguardedAccessToFieldsInSharedObject.

There should be a violation for the case:

private String violatingMatchesOnField() {
if (field == null || !field.matches(PAT_STRING)) { // bad
return "no match";
}
}

The rule violates for Map, List, etc fields which are initialized inline. When initialized in a constructor, they are not violated. Add this.

Most Test classes have test cases code for one rule. Some Test classes contain code for multiple rules. Convert that to one Test class for each rule for clarity. In addition, make sure testClassName = ruleName + "Test"

@SuppressWarnings("pmd:ObjectMapperCreatedForEachMethodCall")
is also matched while not specifically included in the regex of the rule.

Turns out 'all' matches.
Make this regex more restrictive.

With @singleton annotation require @ConcurrencyManagement annotation and if ConcurrencyManagementType.CONTAINER require @lock annotation.
All on class level.

Not being clear about this has lead to issues.

In the Interceptor, "preHandle" method has the mdc.put and in the method "afterCompletion" did mdc.clear().
But PMD doesn't recognize this mdc.clear(). It complains.

Juan from the PMD-team recommended to make unit tests to automatically test the rules on the test case source code. It can be seen in the PMD repo.

A number of improvements here:
Rules using type resolution should declare the attribute typeResolution="true" in the element.
I’d drop the [ancestor::CompilationUnit/ImportDeclaration/Name[starts-with(@image,'org.apache.commons.fileupload')]] check… we have type resolution to match types, and it should work even if using a fully qualified name instead of an import (a test case is missing here)
The first 2 start-with can probably me merged together by searching for ancestor:: MethodDeclaration//Type/…. This part will be simpler with the changes we are introducing to PMD 7, but in the meantime your approach is correct.
I see you added (jpinpoint-rules) to all rule’s description, but without context it will probably be confusing to users trying to understand what the problem is with their code… remember this will appear in both the report and on the online docs for the rule. What do you think?

Often critical rules are suppressed because they are not well understood by developers. We want to detect it when rules which can detect high risks problems are suppressed. False positives should be fixed in the rule. These rules are informative and not to break builds, so be on minor level. In addition we want to detect all suppressed rules. So, two rules:

• UsingSuppressWarnings
• UsingSuppressWarningsHighRisk

Request for new rule: check for synchronizing correctly when using @GuardedBy("this") on field.
Rule: NotProperlySynchronizingOnThisWhileUsingGuardedBy

Example tests:

class Bad1 {
        @GuardedBy("this")
        private String txt;

        public String getText() {
            return txt; // bad
        }
        public void setText(String t) {
            txt = t; // bad
        }
 }

 class Good1 {
        @GuardedBy("this")
        private String txt;

        public synchronized String getText() {
            return txt;
        }
        public synchronized void setText(String t) {
            txt = t;
        }
 }

The cache by default allows multiple threads accessing by the same key.

Problem: if the value of the key is not available from the cache, it will be fetched/computed by multiple threads while only one time is needed.

Solution: Let only the first accessing thread fetch/compute the value and block others until the value is in the cache. Add attribute sync = "true" to achieve this. (Assuming the cache implementation supports it.)

Extend rule to check for @aspect, it should not be combined with one of:
[@component, @service, @configuration, @repository, @controller, @entity]

AvoidFutureGetWithoutTimeout doesn't work in case of more than one field/param/local of one of the Future types. Fix this.

A.o. add examples, standardize attribute usage, correct link.
Investigate:
For the concat rules, they don't seem to consider string literals in a special way… when writing "foo" + "bar", the compiler will do the concat in compile time, so there is no real performance penalty (same happens for final strings initialized upon declaration, but that one is way harder to detect under the current XPath support; and I'm unsure, but probably Java 8+ does the same for effectively final variables).

If the first argument to String replaceAll isn't actually a regex, it still reports a violation.
Checking String.split is fixed for this case. Checking other methods should be fixed as well, where applicable.

Test case from project code:

    public final static String PAYLOADDATA = "PAYLOADDATA";
    private String stringReplaceAllNonRegexOK() {
        String result = "some xml data with [PAYLOADDATA]";
        StringBuilder payloadData = new StringBuilder("some payload");
        result = result.replaceAll(PAYLOADDATA, payloadData.toString());
        return result;
    }

If fields of singletons (@component) are used unguarded, this is thread-unsafe, or may mixup session data.
A special more hidden case is when a @component inherits its fields from an ordinary class (no @component), that fields cannot be checked. At declaration point it is not known to be a @component.
We have covered assignments to inherited fields.
We cannot detect ordinary use of those fields because they may be immutable/unmodifyable.
We can check for mutating methods.

We assume methods starting with:
add, set, put, clear, remove, replace
mutate the object. Detect those for this new rule.

Enhance rule with calling an operation, examples:
bad:

LOG.debug("Complete Soap response: {}", getSoapMsgAsString(context.getMsg())); // bad

good:

if (LOG.isDebugEnabled()) { // good
    LOG.debug("Complete Soap response: {}", getSoapMsgAsString(context.getMsg()));
}

To avoid too many false positives, we require one or more arguments for the operation. Also we exclude simple obj.get(arg) calls, like list.get(0) and map.get(key) which are typically fast.

Rule should be renamed to 'UnconditionalOperationOnLogArgument' ('String' removed.)

See README for conventions

Avoid a SQL IN-Expression, it fails for > 1000 arguments and pollutes the query plan cache / statement cache.
A rule exists. Now we want support for persistence API - criteria API.

We still want to be able to search by jpinpoint-rules in e.g. Sonar, however, we don't want to put it in the general rule description for inclusion as standard PMD rule.

After extensive investigation we found a concurrency bug in vendor platform code of a online store. It lead to a memory leak which caused costly downtime. We want these kinds of bugs to be found with new rules.

We have two rules for use of unguarded fields and assignments to it for shared objects. We can extend these rules for objects using synchronized in one or another way. Synchronized is meant for multi-threaded context, so it is shared by threads and the object itself should be thread-safe. I think this applies for most common/typical cases. So fields should be properly guarded and assignments to it should be guarded.
Names:

• AvoidUnguardedMutableFieldsInObjectsUsingSynchronized
• AvoidUnguardedAssignmentToNonFinalFieldsInObjectsUsingSynchronized

False positive.
Should be no violation for:

public void good(Object arg) {
    try {
        MDC.put("sessionId", arg); // good, cleared in finally
        MDC.put(USER_TYPE_KEY1, arg); // good
    } finally {
        MDC.clear();
    }
}

wrappedLine.append(str, offset, wrapLength + offset); // false positive: + but no string concat

This case is not covered:

@Component
public class Try extends Parent {
    void bad() {
        inherited = new Object(); //bad
    }
    void good(Object objParam, Object inherited) {
        inherited = new Object(); //good, shadowing
        objParam = new Object(); //good
    }
}

The rule ignores inherited fields which are a parameter of local in any method.
Correct is for the current method.

separate tests, add line numbers, possibly add tests

HttpClients should have disabled connection state to reuse TLS connections. In cloud environment sessions are not sticky and connections are stateless. HttpClients should be configured to disable connection state, otherwise TLS connections will not be reused and performance will suffer.

Correct:

return HttpClientBuilder.create()
                .setConnectionManager(conMgr)
                .disableConnectionState()
                .build();

Also support

HttpClients.custom()
HttpAsyncClientBuilder

We use the typeof function in our xpath expression, however, this function has a number of shortcomings, see https://pmd.github.io/2018/05/29/PMD-6.4.0/

We want to utilize the new function typeIs to get cleaner xpath expressions.

Would love to see some rules or insights into Streams and such..