phoedos/pmd
1
1
Fork 1
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

[java] Merge GuardDebugLogging and GuardLogStatementJavaUtil into

Browse Source 
the rule GuardLogStatement
This commit is contained in:
Andreas Dangel
2017-11-30 22:38:14 +01:00
parent 345cc50f78
commit 1447ac8a4a
4 changed files with 263 additions and 286 deletions
Download Patch File Download Diff File Expand all files Collapse all files

107
pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/bestpractices/GuardLogStatementRule.java
View File

@ -30,19 +30,52 @@ import net.sourceforge.pmd.properties.StringMultiProperty;
*
*/
public class GuardLogStatementRule extends AbstractJavaRule implements Rule {
/*
* guard methods and log levels:
*
* log4j + apache commons logging (jakarta):
* trace -> isTraceEnabled
* debug -> isDebugEnabled
* info -> isInfoEnabled
* warn -> isWarnEnabled
* error -> isErrorEnabled
*
*
* java util:
* log(Level.FINE) -> isLoggable
* finest -> isLoggable
* finer -> isLoggable
* fine -> isLoggable
* info -> isLoggable
* warning -> isLoggable
* severe -> isLoggable
*/
public static final StringMultiProperty LOG_LEVELS = new StringMultiProperty("logLevels", "LogLevels to guard",
new String[] {}, 1.0f, ',');
new String[] {"trace", "debug", "info", "warn", "error",
"log", "finest", "finer", "fine", "info", "warning", "severe"}, 1.0f, ',');
public static final StringMultiProperty GUARD_METHODS = new StringMultiProperty("guardsMethods",
"method use to guard the log statement", new String[] {}, 2.0f, ',');
"method use to guard the log statement",
new String[] {"isTraceEnabled", "isDebugEnabled", "isInfoEnabled", "isWarnEnabled", "isErrorEnabled",
"isLoggable"}, 2.0f, ',');
protected Map<String, String> guardStmtByLogLevel = new HashMap<>(5);
private static final String XPATH_EXPRESSION = "//PrimaryPrefix[ends-with(Name/@Image, 'LOG_LEVEL')]"
+ "[count(../descendant::AdditiveExpression) > 0]"
+ "[count(ancestor::IfStatement/Expression/descendant::PrimaryExpression["
+ "ends-with(descendant::PrimaryPrefix/Name/@Image,'GUARD')]) = 0]";
private static final String XPATH_EXPRESSION =
// first part deals with log4j / apache commons logging
"//StatementExpression/PrimaryExpression/PrimaryPrefix[ends-with(Name/@Image, 'LOG_LEVEL')]\n"
+ "[..//AdditiveExpression]\n"
+ "[not(ancestor::IfStatement) or\n"
+ " not(ancestor::IfStatement/Expression/PrimaryExpression/PrimaryPrefix/Name[contains('GUARD', substring-after(@Image, '.'))])]\n"
+ "|\n"
// this part deals with java util
+ "//StatementExpression/PrimaryExpression/PrimaryPrefix[ends-with(Name/@Image, 'LOG_LEVEL_UPPERCASE')]\n"
+ "[../../..//AdditiveExpression]\n"
+ "[not(ancestor::IfStatement) or\n"
+ " not(ancestor::IfStatement/Expression/PrimaryExpression\n"
+ " [contains('GUARD', substring-after(PrimaryPrefix/Name/@Image, '.'))]\n"
+ " [ends-with(PrimarySuffix//Name/@Image, 'LOG_LEVEL_UPPERCASE')])\n"
+ "]";
public GuardLogStatementRule() {
definePropertyDescriptor(LOG_LEVELS);
@ -67,7 +100,7 @@ public class GuardLogStatementRule extends AbstractJavaRule implements Rule {
protected void findViolationForEachLogStatement(ASTCompilationUnit unit, Object data, String xpathExpression) {
for (Entry<String, String> entry : guardStmtByLogLevel.entrySet()) {
List<? extends Node> nodes = findViolations(unit, entry.getKey(), entry.getValue(), xpathExpression);
List<Node> nodes = findViolations(unit, entry.getKey(), entry.getValue(), xpathExpression);
for (Node node : nodes) {
super.addViolation(data, node);
}
@ -75,33 +108,18 @@ public class GuardLogStatementRule extends AbstractJavaRule implements Rule {
}
@SuppressWarnings("unchecked")
private List<? extends Node> findViolations(ASTCompilationUnit unit, String logLevel, String guard,
private List<Node> findViolations(ASTCompilationUnit unit, String logLevel, String guard,
String xpathExpression) {
try {
return unit
.findChildNodesWithXPath(xpathExpression.replaceAll("LOG_LEVEL_UPPERCASE", logLevel.toUpperCase())
.replaceAll("LOG_LEVEL", logLevel).replaceAll("GUARD", guard));
String xpath = xpathExpression.replaceAll("LOG_LEVEL_UPPERCASE", logLevel.toUpperCase())
.replaceAll("LOG_LEVEL", logLevel).replaceAll("GUARD", guard);
return unit.findChildNodesWithXPath(xpath);
} catch (JaxenException e) {
e.printStackTrace();
}
return Collections.EMPTY_LIST;
}
private void setPropertiesDefaultValues(List<String> logLevels, List<String> guardMethods) {
logLevels.add("trace");
logLevels.add("debug");
logLevels.add("info");
logLevels.add("warn");
logLevels.add("error");
guardMethods.clear();
guardMethods.add("isTraceEnabled");
guardMethods.add("isDebugEnabled");
guardMethods.add("isInfoEnabled");
guardMethods.add("isWarnEnabled");
guardMethods.add("isErrorEnabled");
}
protected void extractProperties() {
if (guardStmtByLogLevel.isEmpty()) {
@ -109,11 +127,18 @@ public class GuardLogStatementRule extends AbstractJavaRule implements Rule {
List<String> guardMethods = new ArrayList<>(super.getProperty(GUARD_METHODS));
if (guardMethods.isEmpty() && !logLevels.isEmpty()) {
throw new IllegalArgumentException("Can't specify guardMethods without specifiying logLevels.");
throw new IllegalArgumentException("Can't specify logLevels without specifying guardMethods.");
}
if (logLevels.isEmpty()) {
setPropertiesDefaultValues(logLevels, guardMethods);
if (logLevels.size() > guardMethods.size()) {
// reuse the last guardMethod for the remaining log levels
int needed = logLevels.size() - guardMethods.size();
String lastGuard = guardMethods.get(guardMethods.size() - 1);
for (int i = 0; i < needed; i++) {
guardMethods.add(lastGuard);
}
}
if (logLevels.size() != guardMethods.size()) {
throw new IllegalArgumentException("For each logLevel a guardMethod must be specified.");
}
buildGuardStatementMap(logLevels, guardMethods);
@ -121,18 +146,14 @@ public class GuardLogStatementRule extends AbstractJavaRule implements Rule {
}
protected void buildGuardStatementMap(List<String> logLevels, List<String> guardMethods) {
for (String logLevel : logLevels) {
boolean found = false;
for (String guardMethod : guardMethods) {
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:" + logLevel
+ ". Should be something like 'is" + logLevel + "Enabled'.");
for (int i = 0; i < logLevels.size(); i++) {
String logLevel = "." + logLevels.get(i);
if (guardStmtByLogLevel.containsKey(logLevel)) {
String combinedGuard = guardStmtByLogLevel.get(logLevel);
combinedGuard += "|" + guardMethods.get(i);
guardStmtByLogLevel.put(logLevel, combinedGuard);
} else {
guardStmtByLogLevel.put(logLevel, guardMethods.get(i));
}
}
}

134
pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/bestpractices/xml/GuardDebugLogging.xml
View File

@ -3,138 +3,4 @@
xmlns="http://pmd.sourceforge.net/rule-tests"
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:schemaLocation="http://pmd.sourceforge.net/rule-tests http://pmd.sourceforge.net/rule-tests_1_0_0.xsd">
<test-code>
<description><![CDATA[
ok, no error expected
]]></description>
<expected-problems>0</expected-problems>
<code><![CDATA[
public class Test {
private static final Log __log = LogFactory.getLog(Test.class);
public void test() {
// good:
if (__log.isDebugEnabled()) {
__log.debug("bla" + "",e );
}
}
}
]]></code>
</test-code>
<test-code>
<description><![CDATA[
Complex logging without guard
]]></description>
<expected-problems>2</expected-problems>
<code><![CDATA[
public class Test {
private static final Log __log = LogFactory.getLog(Test.class);
public void test() {
// bad:
__log.debug("log something" + " and " + "concat strings");
// bad:
__log.debug("log something" + " and " + "concat strings", e);
// good:
if (__log.isDebugEnabled()) {
__log.debug("bla" + "",e );
}
}
}
]]></code>
</test-code>
<test-code>
<description><![CDATA[
Complex logging wit misplaced guard
]]></description>
<expected-problems>1</expected-problems>
<code><![CDATA[
public class Test {
private static final Log __log = LogFactory.getLog(Test.class);
public void test() {
if (true) {
// bad:
__log.debug("log something" + " and " + "concat strings");
// this is useless:
__log.isDebugEnabled();
}
}
}
]]></code>
</test-code>
<test-code>
<description>ok #1189 GuardLogStatementRule and GuardDebugLoggingRule broken for log4j2</description>
<expected-problems>0</expected-problems>
<code><![CDATA[
public class Test {
public void test() {
final Marker m = MarkerManager.getMarker(mymarker);
if (logger.isDebugEnabled(m)) {
logger.debug(m, message + "");
}
}
}
]]></code>
</test-code>
<test-code>
<description>violation - wrong guard #1189 GuardLogStatementRule and GuardDebugLoggingRule broken for log4j2</description>
<expected-problems>1</expected-problems>
<code><![CDATA[
public class Test {
public void test() {
final Marker m = MarkerManager.getMarker(mymarker);
if (logger.isTraceEnabled(m)) {
logger.debug(m, message + "");
}
}
}
]]></code>
</test-code>
<test-code>
<description>violation - no if #1189 GuardLogStatementRule and GuardDebugLoggingRule broken for log4j2</description>
<expected-problems>1</expected-problems>
<code><![CDATA[
public class Test {
public void test() {
final Marker m = MarkerManager.getMarker(mymarker);
logger.isDebugEnabled(m); // must be within an if
logger.debug(m, message + "");
}
}
]]></code>
</test-code>
<test-code>
<description>#1224 GuardDebugLogging broken in 5.1.1 - missing additive statement check in log statement</description>
<expected-problems>0</expected-problems>
<code><![CDATA[
public class Test {
private static final Log __log = LogFactory.getLog(Test.class);
public void test() {
// okay:
__log.debug("log something");
// okay:
__log.debug("log something with exception", e);
// good:
if (__log.isDebugEnabled()) {
__log.debug("bla" + "",e );
}
}
}
]]></code>
</test-code>
<test-code>
<description>#1341 pmd:GuardDebugLogging violates LOGGER.debug with format "{}"</description>
<expected-problems>0</expected-problems>
<code><![CDATA[
public class GuardDebugFalsePositive {
public void test() {
String tempSelector = "a";
LOGGER.debug("MessageSelector={}" , tempSelector);
}
}
]]></code>
</test-code>
</test-data>

208
pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/bestpractices/xml/GuardLogStatement.xml
View File

@ -4,9 +4,7 @@
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:schemaLocation="http://pmd.sourceforge.net/rule-tests http://pmd.sourceforge.net/rule-tests_1_0_0.xsd">
<test-code>
<description><![CDATA[
OK, guard is here
]]></description>
<description>OK, guard is here - log4j</description>
<expected-problems>0</expected-problems>
<code><![CDATA[
public class Foo {
@ -20,9 +18,36 @@ public class Foo {
]]></code>
</test-code>
<test-code>
<description><![CDATA[
KO, missing guard 1
]]></description>
<description>ok, no error expected - apache commons logging</description>
<expected-problems>0</expected-problems>
<code><![CDATA[
public class Test {
private static final Log __log = LogFactory.getLog(Test.class);
public void test() {
// good:
if (__log.isDebugEnabled()) {
__log.debug("bla" + "",e );
}
}
}
]]></code>
</test-code>
<test-code>
<description>Guarded call - OK - java util</description>
<expected-problems>0</expected-problems>
<code><![CDATA[
public class Foo {
private void foo(Logger logger) {
if ( logger.isLoggable(Level.FINE) ) {
logger.fine("debug message" + "");
}
}
}
]]></code>
</test-code>
<test-code>
<description>KO, missing guard 1 - log4j</description>
<expected-problems>1</expected-problems>
<code><![CDATA[
public class Foo {
@ -35,9 +60,7 @@ public class Foo {
]]></code>
</test-code>
<test-code>
<description><![CDATA[
KO, missing guard 2
]]></description>
<description>KO, missing guard 2 - log4j</description>
<rule-property name="logLevels">debug,trace</rule-property>
<rule-property name="guardsMethods">isDebugEnabled,isTraceEnabled</rule-property>
<expected-problems>1</expected-problems>
@ -56,6 +79,57 @@ public class Foo {
}
]]></code>
</test-code>
<test-code>
<description>Complex logging without guard - apache commons logging</description>
<expected-problems>2</expected-problems>
<code><![CDATA[
public class Test {
private static final Log __log = LogFactory.getLog(Test.class);
public void test() {
// bad:
__log.debug("log something" + " and " + "concat strings");
// bad:
__log.debug("log something" + " and " + "concat strings", e);
// good:
if (__log.isDebugEnabled()) {
__log.debug("bla" + "",e );
}
}
}
]]></code>
</test-code>
<test-code>
<description>Complex logging with misplaced guard - apache commons logging</description>
<expected-problems>1</expected-problems>
<code><![CDATA[
public class Test {
private static final Log __log = LogFactory.getLog(Test.class);
public void test() {
if (true) {
// bad:
__log.debug("log something" + " and " + "concat strings");
// this is useless:
__log.isDebugEnabled();
}
}
}
]]></code>
</test-code>
<test-code>
<description>Unguarded call - KO - java util</description>
<expected-problems>1</expected-problems>
<code><![CDATA[
public class Foo {
private void foo(Logger logger) {
logger.fine("debug message" + "");
}
}
]]></code>
</test-code>
<test-code>
<description>ok #1189 GuardLogStatementRule and GuardDebugLoggingRule broken for log4j2</description>
<expected-problems>0</expected-problems>
@ -90,4 +164,120 @@ public class Test {
}
]]></code>
</test-code>
<test-code>
<description>violation - no if #1189 GuardLogStatementRule and GuardDebugLoggingRule broken for log4j2</description>
<expected-problems>1</expected-problems>
<code><![CDATA[
public class Test {
public void test() {
final Marker m = MarkerManager.getMarker(mymarker);
logger.isDebugEnabled(m); // must be within an if
logger.debug(m, message + "");
}
}
]]></code>
</test-code>
<test-code>
<description>#1224 GuardDebugLogging broken in 5.1.1 - missing additive statement check in log statement - apache commons logging</description>
<expected-problems>0</expected-problems>
<code><![CDATA[
public class Test {
private static final Log __log = LogFactory.getLog(Test.class);
public void test() {
// okay:
__log.debug("log something");
// okay:
__log.debug("log something with exception", e);
// good:
if (__log.isDebugEnabled()) {
__log.debug("bla" + "",e );
}
}
}
]]></code>
</test-code>
<test-code>
<description>#1341 pmd:GuardDebugLogging violates LOGGER.debug with format "{}" - slf4j</description>
<expected-problems>0</expected-problems>
<code><![CDATA[
public class GuardDebugFalsePositive {
public void test() {
String tempSelector = "a";
LOGGER.debug("MessageSelector={}" , tempSelector);
}
}
]]></code>
</test-code>
<test-code>
<description>#1203 GuardLogStatementJavaUtil issues warning for severe level not being specified as property</description>
<rule-property name="logLevels">finest,finer,fine,info</rule-property>
<rule-property name="guardsMethods">isLoggable</rule-property>
<expected-problems>0</expected-problems>
<code><![CDATA[
import java.util.logging.Logger;
import java.util.logging.Level;
public class Foo {
Logger LOGGER;
public void foo() {
LOGGER.severe("This is a severe message" + " and concat");
}
}
]]></code>
</test-code>
<test-code>
<description>#1227 GuardLogStatementJavaUtil doesn't catch log(Level.FINE, "msg" + " msg") calls</description>
<expected-problems>1</expected-problems>
<expected-linenumbers>8</expected-linenumbers>
<code><![CDATA[
import java.util.logging.Logger;
import java.util.logging.Level;
public class Foo {
Logger LOGGER;
public void foo() {
LOGGER.log(Level.FINE, "This is a severe message" + " and concat"); // violation
if (LOGGER.isLoggable(Level.FINE)) {
LOGGER.log(Level.FINE, "This is a severe message" + " and concat"); // no violation
}
}
}
]]></code>
</test-code>
<test-code>
<description>#1347 False positive for GuardLogStatementJavaUtil with slf4j</description>
<expected-problems>0</expected-problems>
<code><![CDATA[
import org.slf4j.Logger;
public class GuardLogTest {
Logger LOG;
public void foo() {
if (LOG.isInfoEnabled()) {
LOG.info("update: After spool map size: " + map.size());
}
}
}
]]></code>
</test-code>
<test-code>
<description>#1398 False positive for GuardLogStatementJavaUtil with Log4j</description>
<expected-problems>0</expected-problems>
<code><![CDATA[
import org.apache.log4j.Logger;
public class GuardLogTest {
Logger LOG;
public void foo() {
if (LOG.isInfoEnabled()) {
LOG.info("update: After spool map size: " + map.size());
}
}
}
]]></code>
</test-code>
</test-data>

100
pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/bestpractices/xml/GuardLogStatementJavaUtil.xml
View File

@ -3,74 +3,6 @@
xmlns="http://pmd.sourceforge.net/rule-tests"
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:schemaLocation="http://pmd.sourceforge.net/rule-tests http://pmd.sourceforge.net/rule-tests_1_0_0.xsd">
<test-code>
<description><![CDATA[
Guarded call - OK
]]></description>
<expected-problems>0</expected-problems>
<code><![CDATA[
public class Foo {
private void foo(Logger logger) {
if ( logger.isLoggable(Level.FINE) ) {
logger.fine("debug message" + "");
}
}
}
]]></code>
</test-code>
<test-code>
<description><![CDATA[
Unguarded call - KO
]]></description>
<expected-problems>1</expected-problems>
<code><![CDATA[
public class Foo {
private void foo(Logger logger) {
logger.fine("debug message" + "");
}
}
]]></code>
</test-code>
<test-code>
<description>#1203 GuardLogStatementJavaUtil issues warning for severe level not being specified as property</description>
<rule-property name="logLevels">finest,finer,fine,info</rule-property>
<rule-property name="guardsMethods">isLoggable</rule-property>
<expected-problems>0</expected-problems>
<code><![CDATA[
import java.util.logging.Logger;
import java.util.logging.Level;
public class Foo {
Logger LOGGER;
public void foo() {
LOGGER.severe("This is a severe message" + " and concat");
}
}
]]></code>
</test-code>
<test-code>
<description>#1227 GuardLogStatementJavaUtil doesn't catch log(Level.FINE, "msg" + " msg") calls</description>
<expected-problems>1</expected-problems>
<expected-linenumbers>8</expected-linenumbers>
<code><![CDATA[
import java.util.logging.Logger;
import java.util.logging.Level;
public class Foo {
Logger LOGGER;
public void foo() {
LOGGER.log(Level.FINE, "This is a severe message" + " and concat"); // violation
if (LOGGER.isLoggable(Level.FINE)) {
LOGGER.log(Level.FINE, "This is a severe message" + " and concat"); // no violation
}
}
}
]]></code>
</test-code>
<test-code>
<description>#1335 GuardLogStatementJavaUtil should not apply to SLF4J Logger</description>
<expected-problems>0</expected-problems>
@ -87,36 +19,4 @@ public class GuardLogTest {
}
]]></code>
</test-code>
<test-code>
<description>#1347 False positive for GuardLogStatementJavaUtil</description>
<expected-problems>0</expected-problems>
<code><![CDATA[
import org.slf4j.Logger;
public class GuardLogTest {
Logger LOG;
public void foo() {
if (LOG.isInfoEnabled()) {
LOG.info("update: After spool map size: " + map.size());
}
}
}
]]></code>
</test-code>
<test-code>
<description>#1398 False positive for GuardLogStatementJavaUtil with Log4j</description>
<expected-problems>0</expected-problems>
<code><![CDATA[
import org.apache.log4j.Logger;
public class GuardLogTest {
Logger LOG;
public void foo() {
if (LOG.isInfoEnabled()) {
LOG.info("update: After spool map size: " + map.size());
}
}
}
]]></code>
</test-code>
</test-data>