diff --git a/pmd/src/main/java/net/sourceforge/pmd/lang/java/rule/logging/GuardDebugLoggingRule.java b/pmd/src/main/java/net/sourceforge/pmd/lang/java/rule/logging/GuardDebugLoggingRule.java new file mode 100644 index 0000000000..d25e0f0eaf --- /dev/null +++ b/pmd/src/main/java/net/sourceforge/pmd/lang/java/rule/logging/GuardDebugLoggingRule.java @@ -0,0 +1,17 @@ +package net.sourceforge.pmd.lang.java.rule.logging; + +import java.util.HashMap; + +public class GuardDebugLoggingRule extends GuardLogStatementRule { + + public GuardDebugLoggingRule() { + super.guardStmtByLogLevel = new HashMap(1); + super.guardStmtByLogLevel.put(".debug","isDebugEnabled"); + } + + @Override + protected void extractProperties() { + // This rule is not configurable + } + +} diff --git a/pmd/src/main/java/net/sourceforge/pmd/lang/java/rule/logging/GuardLogStatementJavaUtilRule.java b/pmd/src/main/java/net/sourceforge/pmd/lang/java/rule/logging/GuardLogStatementJavaUtilRule.java new file mode 100644 index 0000000000..22863467b1 --- /dev/null +++ b/pmd/src/main/java/net/sourceforge/pmd/lang/java/rule/logging/GuardLogStatementJavaUtilRule.java @@ -0,0 +1,30 @@ +package net.sourceforge.pmd.lang.java.rule.logging; + +import java.util.logging.Level; + +import net.sourceforge.pmd.lang.java.ast.ASTCompilationUnit; + +public class GuardLogStatementJavaUtilRule extends GuardLogStatementRule { + + private static final String GUARD_METHOD_NAME = "isLoggable"; + + // Override default constructor - this rule can't be configured + public GuardLogStatementJavaUtilRule() {}; + + @Override + public Object visit(ASTCompilationUnit unit, Object data) { + if ( super.guardStmtByLogLevel.isEmpty() ) { + super.guardStmtByLogLevel.put(formatLogLevelString(Level.FINEST), GUARD_METHOD_NAME); + super.guardStmtByLogLevel.put(formatLogLevelString(Level.FINER), GUARD_METHOD_NAME); + super.guardStmtByLogLevel.put(formatLogLevelString(Level.FINE), GUARD_METHOD_NAME); + super.guardStmtByLogLevel.put(formatLogLevelString(Level.INFO), GUARD_METHOD_NAME); + super.guardStmtByLogLevel.put(formatLogLevelString(Level.WARNING), GUARD_METHOD_NAME); + super.guardStmtByLogLevel.put(formatLogLevelString(Level.SEVERE), GUARD_METHOD_NAME); + } + return super.visit(unit,data); + } + + private String formatLogLevelString(Level logLevel) { + return "." + logLevel.toString().toLowerCase(); + } +} diff --git a/pmd/src/main/java/net/sourceforge/pmd/lang/java/rule/logging/GuardLogStatementRule.java b/pmd/src/main/java/net/sourceforge/pmd/lang/java/rule/logging/GuardLogStatementRule.java index c948f14ee5..7a2fba08a4 100644 --- a/pmd/src/main/java/net/sourceforge/pmd/lang/java/rule/logging/GuardLogStatementRule.java +++ b/pmd/src/main/java/net/sourceforge/pmd/lang/java/rule/logging/GuardLogStatementRule.java @@ -2,29 +2,31 @@ package net.sourceforge.pmd.lang.java.rule.logging; import java.util.ArrayList; import java.util.Arrays; +import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.Map.Entry; import net.sourceforge.pmd.Rule; import net.sourceforge.pmd.lang.ast.Node; -import net.sourceforge.pmd.lang.java.ast.ASTBlockStatement; import net.sourceforge.pmd.lang.java.ast.ASTCompilationUnit; -import net.sourceforge.pmd.lang.java.ast.ASTIfStatement; -import net.sourceforge.pmd.lang.java.ast.ASTName; -import net.sourceforge.pmd.lang.java.ast.ASTPrimaryPrefix; import net.sourceforge.pmd.lang.java.rule.optimizations.AbstractOptimizationRule; import net.sourceforge.pmd.lang.rule.properties.StringMultiProperty; +import org.jaxen.JaxenException; + /** - * Check that log.debug and log.trace statements are guarded by some - * log.isDebugEnabled() or log.isTraceEnabled() checks. + * Check that log.debug, log.trace, log.error, etc... statements are guarded by + * some test expression on log.isDebugEnabled() or log.isTraceEnabled(). * - * @author Heiko hwr@pilhuhn.de * @author Romain Pelisse - + * @author Heiko Rupp - + * @author Tammo van Lessen - provided original XPath expression * */ -public class GuardLogStatementRule extends AbstractOptimizationRule implements Rule { +public class GuardLogStatementRule extends AbstractOptimizationRule implements + Rule { public static final StringMultiProperty LOG_LEVELS = new StringMultiProperty( "logLevels", "LogLevels to guard", new String[] {}, 1.0f, ','); @@ -33,9 +35,14 @@ public class GuardLogStatementRule extends AbstractOptimizationRule implements R "guardsMethods", "method use to guard the log statement", new String[] {}, 2.0f, ','); - private final Map guardStmtByLogLevel = new HashMap( + protected Map guardStmtByLogLevel = new HashMap( 5); + private static final String xpathExpression = "//PrimaryPrefix[ends-with(Name/@Image, 'KEY') and " + + "count(" + + "ancestor::IfStatement/Expression/descendant::PrimaryExpression[" + + "ends-with(descendant::PrimaryPrefix/Name/@Image,'VALUE')]) = 0]"; + public GuardLogStatementRule() { definePropertyDescriptor(LOG_LEVELS); definePropertyDescriptor(GUARD_METHODS); @@ -43,77 +50,35 @@ public class GuardLogStatementRule extends AbstractOptimizationRule implements R @Override public Object visit(ASTCompilationUnit unit, Object data) { - if ( guardStmtByLogLevel.isEmpty() ) { - List logLevels = new ArrayList(Arrays.asList(super - .getProperty(LOG_LEVELS))); - List guardMethods = new ArrayList(Arrays.asList(super - .getProperty(GUARD_METHODS))); - - if (guardMethods.isEmpty() && ! logLevels.isEmpty() ) { - throw new IllegalArgumentException( - "Can't specify guardMethods without specifiying logLevels."); + extractProperties(); + findViolationForEachLogStatement(unit, data); + return super.visit(unit, data); + } + + private void findViolationForEachLogStatement(ASTCompilationUnit unit, Object data) { + for (Entry entry : guardStmtByLogLevel.entrySet()) { + List nodes = findViolations(unit, entry.getKey(), + entry.getValue()); + for (Node node : nodes) { + super.addViolation(data, node); } - - if (logLevels.isEmpty()) - setPropertiesDefaultValues(logLevels, guardMethods); - - buildGuardStatementMap(logLevels, guardMethods); - } - return super.visit(unit,data); - } - - @Override - public Object visit(ASTName name, Object data) { - Node node = name.jjtGetParent(); - if (node instanceof ASTPrimaryPrefix) { - - } else - return super.visit(name, data); - if (name != null) { - String lastPrefix = lastPrefix(name.getImage()); - if (guardStmtByLogLevel.keySet().contains(lastPrefix)) { - // TODO check for type - Node parent1 = name.getNthParent(5); - boolean guardFound = false; - if (parent1 instanceof ASTIfStatement) { - guardFound = checkForGuard((ASTIfStatement) parent1, - lastPrefix); - } else if (parent1 instanceof ASTBlockStatement) { - Node parent2 = name.getNthParent(7); - if (parent2 instanceof ASTIfStatement) { - guardFound = checkForGuard((ASTIfStatement) parent2, - lastPrefix); - } - } - if (!guardFound) - addViolation(data, name); - } - } - return super.visit(name, data); - } - - private String lastPrefix(String string) { - if (string != null && ! "".equals(string) ) { - if ( string.contains(".") ) - return string.substring(string.lastIndexOf('.'), string.length()); - } - return string; - } - - private boolean checkForGuard(ASTIfStatement stm, String logLevel) { - - List names = stm.findDescendantsOfType(ASTName.class); - if (names == null || names.isEmpty()) - return false; - - for (ASTName name : names) { - if ( name.getImage().endsWith(guardStmtByLogLevel.get(logLevel)) ) - return true; - } - return false; + } } - private void setPropertiesDefaultValues(List logLevels, List guardMethods) { + @SuppressWarnings("unchecked") + private List findViolations(ASTCompilationUnit unit, String key, + String value) { + try { + return unit.findChildNodesWithXPath(xpathExpression.replaceFirst( + "KEY", key).replaceFirst("VALUE", value)); + } catch (JaxenException e) { + e.printStackTrace(); + } + return Collections.EMPTY_LIST; + } + + private void setPropertiesDefaultValues(List logLevels, + List guardMethods) { logLevels.add("trace"); logLevels.add("debug"); logLevels.add("info"); @@ -128,16 +93,39 @@ public class GuardLogStatementRule extends AbstractOptimizationRule implements R guardMethods.add("isErrorEnabled"); } - private void buildGuardStatementMap(List logLevels, List guardMethods) { + protected void extractProperties() { + if (guardStmtByLogLevel.isEmpty()) { + + List logLevels = new ArrayList(Arrays.asList(super + .getProperty(LOG_LEVELS))); + List guardMethods = new ArrayList( + Arrays.asList(super.getProperty(GUARD_METHODS))); + + if (guardMethods.isEmpty() && !logLevels.isEmpty()) { + throw new IllegalArgumentException( + "Can't specify guardMethods without specifiying logLevels."); + } + + if (logLevels.isEmpty()) + setPropertiesDefaultValues(logLevels, guardMethods); + + buildGuardStatementMap(logLevels, guardMethods); + } + } + + protected void buildGuardStatementMap(List logLevels, + List guardMethods) { for (String logLevel : logLevels) { boolean found = false; for (String guardMethod : guardMethods) { - if (!found && guardMethod.toLowerCase().contains(logLevel.toLowerCase())) { + if (!found + && guardMethod.toLowerCase().contains( + logLevel.toLowerCase())) { found = true; guardStmtByLogLevel.put("." + logLevel, guardMethod); } } - + if (!found) throw new IllegalArgumentException( "No guard method associated to the logLevel:" diff --git a/pmd/src/main/resources/rulesets/java/logging-jakarta-commons.xml b/pmd/src/main/resources/rulesets/java/logging-jakarta-commons.xml index d364733cd0..5be648b9b2 100644 --- a/pmd/src/main/resources/rulesets/java/logging-jakarta-commons.xml +++ b/pmd/src/main/resources/rulesets/java/logging-jakarta-commons.xml @@ -92,17 +92,13 @@ public class Foo { language="Java" since="4.3" message="debug logging that involves string concatenation should be guarded with isDebugEnabled() checks" - class="net.sourceforge.pmd.lang.java.rule.logging.GuardLogStatementRule" + class="net.sourceforge.pmd.lang.java.rule.logging.GuardDebugLoggingRule" externalInfoUrl="${pmd.website.baseurl}/rules/java/logging-jakarta-commons.html#GuardDebugLogging"> When log messages are composed by concatenating strings, the whole section should be guarded by a isDebugEnabled() check to avoid performance and memory issues. 3 - - debug - isDebugEnabled - + externalInfoUrl="${pmd.website.baseurl}/rules/java/logging-jakarta-commons.html#GuardLogStatement"> Whenever using a log level, one should check if the loglevel is actually enabled, or otherwise skip the associate String creation and manipulation. diff --git a/pmd/src/main/resources/rulesets/java/logging-java.xml b/pmd/src/main/resources/rulesets/java/logging-java.xml index ae0c2600c9..70c1a00b0b 100644 --- a/pmd/src/main/resources/rulesets/java/logging-java.xml +++ b/pmd/src/main/resources/rulesets/java/logging-java.xml @@ -140,4 +140,23 @@ class Foo { + + +Whenever using a log level, one should check if the loglevel is actually enabled, or +otherwise skip the associate String creation and manipulation. + + 2 + + + + diff --git a/pmd/src/test/java/net/sourceforge/pmd/lang/java/rule/loggingjakartacommons/LoggingJakartaCommonsRulesTest.java b/pmd/src/test/java/net/sourceforge/pmd/lang/java/rule/loggingjakartacommons/LoggingJakartaCommonsRulesTest.java index 68dbee69db..1903e385da 100644 --- a/pmd/src/test/java/net/sourceforge/pmd/lang/java/rule/loggingjakartacommons/LoggingJakartaCommonsRulesTest.java +++ b/pmd/src/test/java/net/sourceforge/pmd/lang/java/rule/loggingjakartacommons/LoggingJakartaCommonsRulesTest.java @@ -14,7 +14,7 @@ public class LoggingJakartaCommonsRulesTest extends SimpleAggregatorTst { addRule(RULESET, "ProperLogger"); addRule(RULESET, "UseCorrectExceptionLogging"); addRule(RULESET, "GuardDebugLogging"); - addRule(RULESET, "GuardLogStatement"); +// addRule(RULESET, "GuardLogStatement"); } public static junit.framework.Test suite() { diff --git a/pmd/src/test/java/net/sourceforge/pmd/lang/java/rule/loggingjava/LoggingJavaRulesTest.java b/pmd/src/test/java/net/sourceforge/pmd/lang/java/rule/loggingjava/LoggingJavaRulesTest.java index d08b8ac36c..63179ff253 100644 --- a/pmd/src/test/java/net/sourceforge/pmd/lang/java/rule/loggingjava/LoggingJavaRulesTest.java +++ b/pmd/src/test/java/net/sourceforge/pmd/lang/java/rule/loggingjava/LoggingJavaRulesTest.java @@ -6,7 +6,7 @@ import org.junit.Before; public class LoggingJavaRulesTest extends SimpleAggregatorTst { - + private static final String RULESET = "java-logging-java"; @Before @@ -15,7 +15,7 @@ public class LoggingJavaRulesTest extends SimpleAggregatorTst { addRule(RULESET, "LoggerIsNotStaticFinal"); addRule(RULESET, "MoreThanOneLogger"); addRule(RULESET, "SystemPrintln"); - addRule(RULESET, "GuardLogStatement"); + addRule(RULESET, "GuardLogStatementJavaUtil"); } public static junit.framework.Test suite() { diff --git a/pmd/src/test/resources/net/sourceforge/pmd/lang/java/rule/loggingjakartacommons/xml/GuardDebugLogging.xml b/pmd/src/test/resources/net/sourceforge/pmd/lang/java/rule/loggingjakartacommons/xml/GuardDebugLogging.xml index 0d089e5e97..4718a76a0d 100644 --- a/pmd/src/test/resources/net/sourceforge/pmd/lang/java/rule/loggingjakartacommons/xml/GuardDebugLogging.xml +++ b/pmd/src/test/resources/net/sourceforge/pmd/lang/java/rule/loggingjakartacommons/xml/GuardDebugLogging.xml @@ -2,19 +2,13 @@ 0 + + + + 0 + + + + + 1 + + +