Skip to content

Commit

Permalink
Fix: Slf4jLogShouldBeConstant removes passed exception (#103)
Browse files Browse the repository at this point in the history 
* fixes: #95

- Add remaining arguments at the end of the log method call
- Ignore log calls where specifiers like %s and {} are combined as order might be an issue.

* prefer shorter test samples, but keep consistency

* strip off wildcards from regex

* removed redundant null check
add empty check
use StringUtils isNullOrEmpty

* revert String.valueOf -> .toString()
add requireNonNull to satisfy intellij

* also rewrite with only a single (possibly blank) argument.

* Add whitespace

* remove redundant check. Compiler takes care of this.

---------

Co-authored-by: Peter Streef <peter.streef@twill.net>
Co-authored-by: Tim te Beek <timtebeek@gmail.com>
  • Loading branch information
@pstreef @ps-twill @timtebeek
3 people committed May 6, 2023
1 parent 97b8e64 commit f5ff9e4
Show file tree
Hide file tree
Showing 2 changed files with 149 additions and 50 deletions.
50 changes: 27 additions & 23 deletions src/main/java/org/openrewrite/java/logging/slf4j/Slf4jLogShouldBeConstant.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import org.openrewrite.Recipe;
import org.openrewrite.TreeVisitor;
import org.openrewrite.internal.ListUtils;
import org.openrewrite.internal.StringUtils;
import org.openrewrite.java.JavaTemplate;
import org.openrewrite.java.JavaVisitor;
import org.openrewrite.java.MethodMatcher;
Expand All @@ -29,14 +30,17 @@
import org.openrewrite.java.tree.TypeUtils;

import java.time.Duration;
import java.util.Arrays;
import java.util.HashSet;
import java.util.List;
import java.util.Set;
import java.util.regex.Matcher;
import java.util.*;
import java.util.regex.Pattern;

public class Slf4jLogShouldBeConstant extends Recipe {

private static final String SLF4J_FORMAT_SPECIFIER = "{}";
private static final Pattern SLF4J_FORMAT_SPECIFIER_PATTERN = Pattern.compile("\\{}");
private static final Pattern FORMAT_SPECIFIER_PATTERN = Pattern.compile("%[\\d.]*[dfscbBhHn%]");

// A regular expression that matches index specifiers like '%2$s', '%1$s', etc.
private static final Pattern INDEXED_FORMAT_SPECIFIER_PATTERN = Pattern.compile("%(\\d+\\$)[a-zA-Z]");
private static final MethodMatcher SLF4J_LOG = new MethodMatcher("org.slf4j.Logger *(..)");
private static final MethodMatcher STRING_FORMAT = new MethodMatcher("java.lang.String format(..)");

Expand Down Expand Up @@ -66,6 +70,7 @@ protected TreeVisitor<?, ExecutionContext> getSingleSourceApplicableTest() {
public Set<String> getTags() {
return new HashSet<>(Arrays.asList("logging", "slf4j"));
}

@Override
protected JavaVisitor<ExecutionContext> getVisitor() {
return new JavaVisitor<ExecutionContext>() {
Expand All @@ -78,26 +83,25 @@ public J visitMethodInvocation(J.MethodInvocation method, ExecutionContext execu
if (STRING_FORMAT.matches(args.get(0))) {
J.MethodInvocation stringFormat = (J.MethodInvocation) args.get(0);

if (stringFormat.getArguments() == null ||
stringFormat.getArguments().size() <= 1 ||
!CompleteExceptionLogging.isStringLiteral(stringFormat.getArguments().get(0))
) {
if (!CompleteExceptionLogging.isStringLiteral(stringFormat.getArguments().get(0))) {
return method;
}

String strFormat = ((J.Literal) stringFormat.getArguments().get(0)).getValue().toString();
if (containsIndexFormatSpecifier(strFormat)) {
String strFormat = Objects.requireNonNull(((J.Literal) stringFormat.getArguments().get(0)).getValue()).toString();
if (containsIndexFormatSpecifier(strFormat) || containsCombinedFormatSpecifiers(strFormat)) {
return method;
}
String updatedStrFormat = replaceFormatSpecifier(strFormat, "{}");
return method.withArguments(ListUtils.map(stringFormat.getArguments(), (n, arg) -> {
String updatedStrFormat = replaceFormatSpecifier(strFormat, SLF4J_FORMAT_SPECIFIER);
List<Expression> stringFormatWithArgs = ListUtils.map(stringFormat.getArguments(), (n, arg) -> {
if (n == 0) {
J.Literal str = (J.Literal) arg;
return str.withValue(updatedStrFormat)
.withValueSource("\"" + updatedStrFormat + "\"");
.withValueSource("\"" + updatedStrFormat + "\"");
}
return arg;
}));
});
List<Expression> originalArgsWithoutMessage = args.subList(1, args.size());
return method.withArguments(ListUtils.concatAll(stringFormatWithArgs, originalArgsWithoutMessage));
} else if (STRING_VALUE_OF.matches(args.get(0))) {
Expression valueOf = ((J.MethodInvocation) args.get(0)).getArguments().get(0);
if (TypeUtils.isAssignableTo(JavaType.ShallowClass.build("java.lang.Throwable"), valueOf.getType())) {
Expand All @@ -120,21 +124,21 @@ public J visitMethodInvocation(J.MethodInvocation method, ExecutionContext execu
};
}

private boolean containsCombinedFormatSpecifiers(String str) {
return FORMAT_SPECIFIER_PATTERN.matcher(str).find() && SLF4J_FORMAT_SPECIFIER_PATTERN.matcher(str).find();
}

private static String replaceFormatSpecifier(String str, String replacement) {
if (str == null || str.isEmpty()) {
if (StringUtils.isNullOrEmpty(str)) {
return str;
}
Pattern pattern = Pattern.compile("%[\\d\\.]*[dfscbBhHn%]");
Matcher matcher = pattern.matcher(str);
return matcher.replaceAll(replacement);
return FORMAT_SPECIFIER_PATTERN.matcher(str).replaceAll(replacement);
}

private static boolean containsIndexFormatSpecifier(String str) {
if (str == null || str.isEmpty()) {
if (StringUtils.isNullOrEmpty(str)) {
return false;
}
// A regular expression that matches index specifiers like '%2$s', '%1$s', etc.
String indexSpecifierRegex = ".*%(\\d+\\$)[a-zA-Z].*";
return str.matches(indexSpecifierRegex);
return INDEXED_FORMAT_SPECIFIER_PATTERN.matcher(str).find();
}
}
149 changes: 122 additions & 27 deletions src/test/java/org/openrewrite/java/logging/slf4j/Slf4jLogShouldBeConstantTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,17 +34,15 @@ public void defaults(RecipeSpec spec) {

@Test
void firstParameterIsNotLiteral() {
//language=java
rewriteRun(
java(
"""
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
class A {
private final static Logger LOG = LoggerFactory.getLogger(A.class);
Logger log;
public void inconsistent(String message, Object... args) {
LOG.warn(String.format(message, args));
log.warn(String.format(message, args));
}
}
"""
Expand All @@ -61,22 +59,22 @@ void doNotUseStringFormat() {
"""
import org.slf4j.Logger;
class Test {
Logger logger;
Logger log;
void test() {
Object obj1 = new Object();
Object obj2 = new Object();
logger.info(String.format("Object 1 is %s and Object 2 is %s", obj1, obj2));
log.info(String.format("Object 1 is %s and Object 2 is %s", obj1, obj2));
}
}
""",
"""
import org.slf4j.Logger;
class Test {
Logger logger;
Logger log;
void test() {
Object obj1 = new Object();
Object obj2 = new Object();
logger.info("Object 1 is {} and Object 2 is {}", obj1, obj2);
log.info("Object 1 is {} and Object 2 is {}", obj1, obj2);
}
}
"""
Expand All @@ -93,23 +91,23 @@ void doNotUseValueOfException() {
"""
import org.slf4j.Logger;
class Test {
Logger logger;
Logger log;
void test() {
try {
} catch(Exception e) {
logger.warn(String.valueOf(e));
log.warn(String.valueOf(e));
}
}
}
""",
"""
import org.slf4j.Logger;
class Test {
Logger logger;
Logger log;
void test() {
try {
} catch(Exception e) {
logger.warn("Exception", e);
log.warn("Exception", e);
}
}
}
Expand All @@ -127,20 +125,20 @@ void doNotUseToString() {
"""
import org.slf4j.Logger;
class Test {
Logger logger;
Logger log;
void test() {
Object obj1 = new Object();
logger.info(obj1.toString());
log.info(obj1.toString());
}
}
""",
"""
import org.slf4j.Logger;
class Test {
Logger logger;
Logger log;
void test() {
Object obj1 = new Object();
logger.info("{}", obj1);
log.info("{}", obj1);
}
}
"""
Expand All @@ -155,11 +153,10 @@ void noChangeWithIndexSpecifier() {
java(
"""
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
class A {
private final static Logger LOG = LoggerFactory.getLogger(A.class);
Logger log;
void method() {
LOG.info(String.format("The the second argument is '%2$s', and the first argument is '%1$s'.", "foo", "bar"));
log.info(String.format("The the second argument is '%2$s', and the first argument is '%1$s'.", "foo", "bar"));
}
}
"""
Expand All @@ -174,25 +171,123 @@ void differentFormatSpecifier() {
java(
"""
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
class A {
private final static Logger LOG = LoggerFactory.getLogger(A.class);
Logger log;
void method() {
log.info(String.format("The first argument is '%d', and the second argument is '%.2f'.", 1, 2.3333));
}
}
""",
"""
import org.slf4j.Logger;
class A {
Logger log;
void method() {
log.info("The first argument is '{}', and the second argument is '{}'.", 1, 2.3333);
}
}
"""
)
);
}

@Issue("https://github.com/openrewrite/rewrite-logging-frameworks/issues/95")
@Test
void doNotUseStringFormatAndKeepPassedException() {
//language=java
rewriteRun(
java(
"""
import org.slf4j.Logger;
class A {
Logger log;
void method() {
LOG.info(String.format("The first argument is '%d', and the second argument is '%.2f'.", 1, 2.3333));
Exception ex = new Exception();
log.warn(String.format("Unexpected exception: %s", ex.getClass().getName()), ex);
}
}
""",
"""
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
class A {
Logger log;
void method() {
Exception ex = new Exception();
log.warn("Unexpected exception: {}", ex.getClass().getName(), ex);
}
}
"""
)
);
}

@Issue("https://github.com/openrewrite/rewrite-logging-frameworks/issues/95")
@Test
void noChangeWithCombinedSpecifier() {
//language=java
rewriteRun(
java(
"""
import org.slf4j.Logger;
class A {
Logger log;
void method() {
Exception ex = new Exception();
log.warn(String.format("Message: {} from Unexpected exception: %s", ex.getClass().getName()), ex.getMessage(), ex);
}
}
"""
)
);
}

@Test
void doNotUseStringFormatWithoutArgs() {
//language=java
rewriteRun(
java(
"""
import org.slf4j.Logger;
class A {
Logger log;
void method() {
log.info(String.format("hi"));
}
}
""",
"""
import org.slf4j.Logger;
class A {
private final static Logger LOG = LoggerFactory.getLogger(A.class);
Logger log;
void method() {
log.info("hi");
}
}
"""
)
);
}

@Test
void doNotUseStringFormatForBlankLog() {
//language=java
rewriteRun(
java(
"""
import org.slf4j.Logger;
class A {
Logger log;
void method() {
log.info(String.format(" "));
}
}
""",
"""
import org.slf4j.Logger;
class A {
Logger log;
void method() {
LOG.info("The first argument is '{}', and the second argument is '{}'.", 1, 2.3333);
log.info(" ");
}
}
"""
Expand Down

0 comments on commit f5ff9e4

Please sign in to comment.