diff --git a/docs/pages/release_notes.md b/docs/pages/release_notes.md index 3974fe8e68..2e9bf24d9b 100644 --- a/docs/pages/release_notes.md +++ b/docs/pages/release_notes.md @@ -49,6 +49,7 @@ not support all features of the latest EcmaScript standard. * java-design * [#3679](https://github.com/pmd/pmd/issues/3679): \[java] Make FinalFieldCouldBeStatic detect constant variable * java-errorprone + * [#3644](https://github.com/pmd/pmd/issues/3644): \[java] InvalidLogMessageFormat: false positives with logstash structured logging * [#3686](https://github.com/pmd/pmd/issues/3686): \[java] ReturnEmptyCollectionRatherThanNull - false negative with conditioned returns * [#3701](https://github.com/pmd/pmd/issues/3701): \[java] MissingStaticMethodInNonInstantiatableClass false positive with method inner classes * java-performance diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/errorprone/InvalidLogMessageFormatRule.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/errorprone/InvalidLogMessageFormatRule.java index 65f187b751..2322134244 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/errorprone/InvalidLogMessageFormatRule.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/errorprone/InvalidLogMessageFormatRule.java @@ -9,6 +9,7 @@ import java.util.Arrays; import java.util.Collections; import java.util.HashMap; import java.util.HashSet; +import java.util.Iterator; import java.util.List; import java.util.Map; import java.util.Set; @@ -48,6 +49,14 @@ public class InvalidLogMessageFormatRule extends AbstractJavaRule { private static final Map> LOGGERS; + /** + * Whitelisted methods of net.logstash.logback.argument.StructuredArguments + */ + private static final Set STRUCTURED_ARGUMENTS_METHODS = Collections.unmodifiableSet(new HashSet<>( + Arrays.asList("a", "array", "defer", "e", + "entries", "f", "fields", "keyValue", + "kv", "r", "raw", "v", "value"))); + static { Map> loggersMap = new HashMap<>(); @@ -116,16 +125,25 @@ public class InvalidLogMessageFormatRule extends AbstractJavaRule { final List argumentList = parentNode.getFirstChildOfType(ASTPrimarySuffix.class) .getFirstDescendantOfType(ASTArgumentList.class).findChildrenOfType(ASTExpression.class); - if (argumentList.get(0).getType() != null && !argumentList.get(0).getType().equals(String.class)) { - if (argumentList.size() == 1) { - // no need to check for message params in case no string and no params found - return data; + // remove any arguments before the string message - these might be method calls for + // logstash markers or structured arguments + // this also removes any non-string value, e.g. a slf4j-Marker + // if the type cannot be determined, it is considered not to be a string... + Iterator iterator = argumentList.iterator(); + while (iterator.hasNext()) { + ASTExpression argument = iterator.next(); + if (!TypeTestUtil.isA(String.class, argument)) { + iterator.remove(); } else { - // ignore the first argument if it is a known non-string value, e.g. a slf4j-Marker - argumentList.remove(0); + break; } } + if (argumentList.isEmpty()) { + // no need to check for message params in case no string message found + return data; + } + // remove the message parameter final ASTExpression messageParam = argumentList.remove(0); @@ -145,6 +163,12 @@ public class InvalidLogMessageFormatRule extends AbstractJavaRule { if (argumentList.size() > expectedArguments) { removeThrowableParam(argumentList); } + + // remove any logstash structured arguments at the end + // but only, if there are not enough placeholders + if (argumentList.size() > expectedArguments) { + removePotentialStructuredArguments(argumentList.size() - expectedArguments, argumentList); + } int providedArguments = argumentList.size(); @@ -319,4 +343,50 @@ public class InvalidLogMessageFormatRule extends AbstractJavaRule { } return stringLiterals; } + + /** + * Removes up to {@code maxArgumentsToRemove} arguments from the end of the {@code argumentList}, + * if the argument is a method call to one of the whitelisted StructuredArguments methods. + * + * @param maxArgumentsToRemove + * @param argumentList + */ + private void removePotentialStructuredArguments(int maxArgumentsToRemove, List argumentList) { + int removed = 0; + while (!argumentList.isEmpty() && removed < maxArgumentsToRemove) { + int lastIndex = argumentList.size() - 1; + ASTExpression argument = argumentList.get(lastIndex); + if (isStructuredArgumentMethodCall(argument)) { + argumentList.remove(lastIndex); + removed++; + } else { + // stop if something else is encountered + break; + } + } + } + + /* + * └─ Expression == argument + * └─ PrimaryExpression + * ├─ PrimaryPrefix + * │ └─ Name: eg. "keyValue" or "StructuredArguments.keyValue" + * └─ PrimarySuffix + * └─ Arguments + */ + private boolean isStructuredArgumentMethodCall(ASTExpression argument) { + if (argument.getNumChildren() == 1 && argument.getChild(0) instanceof ASTPrimaryExpression) { + ASTPrimaryExpression primary = (ASTPrimaryExpression) argument.getChild(0); + if (primary.getNumChildren() == 2 && primary.getChild(1) instanceof ASTPrimarySuffix) { + ASTPrimaryPrefix prefix = (ASTPrimaryPrefix) primary.getChild(0); + ASTPrimarySuffix suffix = (ASTPrimarySuffix) primary.getChild(1); + if (suffix.isArguments() && prefix.getNumChildren() == 1 && prefix.getChild(0) instanceof ASTName) { + ASTName name = (ASTName) prefix.getChild(0); + return name.getImage().startsWith("StructuredArguments.") + || STRUCTURED_ARGUMENTS_METHODS.contains(name.getImage()); + } + } + } + return false; + } } diff --git a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/errorprone/xml/InvalidLogMessageFormat.xml b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/errorprone/xml/InvalidLogMessageFormat.xml index cb5013974a..8649759cb2 100644 --- a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/errorprone/xml/InvalidLogMessageFormat.xml +++ b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/errorprone/xml/InvalidLogMessageFormat.xml @@ -811,8 +811,8 @@ public class InvalidLogMessageFormatTest { ignore slf4j-Markers when detecting the number of arguments #2250 - 2 - 11,17 + 1 + 11 @@ -1039,6 +1043,45 @@ public class Foo { logger.trace(logMessage, param); } } +} + ]]> + + + + [java] InvalidLogMessageFormat: false positives with logstash structured logging #3644 + 2 + 27,29 +