diff --git a/build.gradle.kts b/build.gradle.kts index b84c229..adf0cba 100644 --- a/build.gradle.kts +++ b/build.gradle.kts @@ -10,6 +10,7 @@ val rewriteVersion = rewriteRecipe.rewriteVersion.get() recipeDependencies { parserClasspath("org.slf4j:slf4j-api:2.+") + parserClasspath("org.slf4j:slf4j-api:1.7.+") parserClasspath("log4j:log4j:1.+") parserClasspath("org.apache.logging.log4j:log4j-core:2.+") parserClasspath("org.apache.logging.log4j:log4j-api:2.+") diff --git a/src/main/java/org/openrewrite/java/logging/slf4j/WrapExpensiveLogStatementsInConditionals.java b/src/main/java/org/openrewrite/java/logging/slf4j/WrapExpensiveLogStatementsInConditionals.java index 182da78..5f26447 100644 --- a/src/main/java/org/openrewrite/java/logging/slf4j/WrapExpensiveLogStatementsInConditionals.java +++ b/src/main/java/org/openrewrite/java/logging/slf4j/WrapExpensiveLogStatementsInConditionals.java @@ -46,41 +46,67 @@ public class WrapExpensiveLogStatementsInConditionals extends Recipe { @Override public String getDisplayName() { - return "Wrap expensive log statements in conditionals"; + return "Optimize log statements"; } @Override public String getDescription() { return "When trace, debug and info log statements use methods for constructing log messages, " + "those methods are called regardless of whether the log level is enabled. " + - "This recipe encapsulates those log statements in an `if` statement that checks the log level before calling the log method. " + - "It then bundles surrounding log statements with the same log level into the `if` statement to improve readability of the resulting code."; + "This recipe optimizes these statements by either wrapping them in if-statements (SLF4J 1.x) " + + "or converting them to fluent API calls (SLF4J 2.0+) to ensure expensive methods are only called when necessary."; } @Override public TreeVisitor getVisitor() { return Preconditions.check( or(new UsesMethod<>(infoMatcher), new UsesMethod<>(debugMatcher), new UsesMethod<>(traceMatcher)), - new AddIfEnabledVisitor()); + new OptimizeLogStatementsVisitor()); } - private static class AddIfEnabledVisitor extends JavaVisitor { + +private static class OptimizeLogStatementsVisitor extends JavaVisitor { final Set visitedBlocks = new HashSet<>(); + private boolean supportsFluentApi(J.MethodInvocation logMethod) { + // Check if the logger type supports fluent API by looking for atInfo/atDebug/atTrace methods + if (logMethod.getSelect() == null || logMethod.getMethodType() == null) { + return false; + } + + JavaType.FullyQualified loggerType = TypeUtils.asFullyQualified(logMethod.getMethodType().getDeclaringType()); + if (loggerType == null) { + return false; + } + + // Check if the logger type has the fluent API methods (introduced in SLF4J 2.0) + return loggerType.getMethods().stream() + .anyMatch(m -> "atInfo".equals(m.getName()) || + "atDebug".equals(m.getName()) || + "atTrace".equals(m.getName())); + } + @Override public J visitMethodInvocation(J.MethodInvocation method, ExecutionContext ctx) { J.MethodInvocation m = (J.MethodInvocation) super.visitMethodInvocation(method, ctx); if (m.getSelect() != null && (infoMatcher.matches(m) || debugMatcher.matches(m) || traceMatcher.matches(m)) && !isInIfStatementWithLogLevelCheck(getCursor(), m) && + !isAlreadyUsingFluentApi(getCursor()) && isAnyArgumentExpensive(m)) { + + // Check if we should use fluent API (SLF4J 2.0+) or if-statements (SLF4J 1.x) + if (supportsFluentApi(m)) { + return convertToFluentApi(m, ctx); + } + // Use the traditional if-statement approach for SLF4J 1.x J container = getCursor().getParentTreeCursor().getValue(); if (container instanceof J.Block) { UUID id = container.getId(); J.If if_ = ((J.If) JavaTemplate .builder("if(#{logger:any(org.slf4j.Logger)}.is#{}Enabled()) {}") - .javaParser(JavaParser.fromJavaVersion().classpathFromResources(ctx, "slf4j-api-2.+")) + .javaParser(JavaParser.fromJavaVersion().classpathFromResources(ctx, "slf4j-api-1.+")) .build() .apply(getCursor(), m.getCoordinates().replace(), m.getSelect(), StringUtils.capitalize(m.getSimpleName()))) @@ -93,6 +119,98 @@ public J visitMethodInvocation(J.MethodInvocation method, ExecutionContext ctx) return m; } + private J.MethodInvocation convertToFluentApi(J.MethodInvocation m, ExecutionContext ctx) { + String logLevel = m.getSimpleName(); + String fluentLevel = "at" + StringUtils.capitalize(logLevel); + + List args = m.getArguments(); + if (!args.isEmpty()) { + if (args.size() > 1) { + // First argument is the message template + Expression messageTemplate = args.get(0); + + // Build fluent API with addArgument() calls for each parameter + StringBuilder templateStr = new StringBuilder(); + templateStr.append("#{logger:any(org.slf4j.Logger)}.").append(fluentLevel).append("()"); + + // Add each parameter as an argument + // Use lambda for expensive operations, direct value for cheap ones + List templateArgs = new ArrayList<>(); + //noinspection DataFlowIssue + templateArgs.add(m.getSelect()); + + for (int i = 1; i < args.size(); i++) { + Expression arg = args.get(i); + if (isExpensiveArgument(arg)) { + // Use supplier lambda for expensive operations + templateStr.append(".addArgument(() -> #{any()})"); + } else { + // Use direct value for cheap operations + templateStr.append(".addArgument(#{any()})"); + } + templateArgs.add(arg); + } + templateStr.append(".log(#{any()})"); + templateArgs.add(messageTemplate); + + JavaTemplate template = JavaTemplate + .builder(templateStr.toString()) + .javaParser(JavaParser.fromJavaVersion().classpathFromResources(ctx, "slf4j-api-2.+")) + .build(); + + return template.apply(getCursor(), m.getCoordinates().replace(), templateArgs.toArray()); + } else { + // Simple case with just a message + Expression arg = args.get(0); + if (isExpensiveArgument(arg)) { + // Use supplier lambda for expensive message + JavaTemplate template = JavaTemplate + .builder("#{logger:any(org.slf4j.Logger)}.#{}().log(() -> #{any()})") + .javaParser(JavaParser.fromJavaVersion().classpathFromResources(ctx, "slf4j-api-2.+")) + .build(); + + //noinspection DataFlowIssue + return template.apply(getCursor(), m.getCoordinates().replace(), + m.getSelect(), fluentLevel, arg); + } else { + // Use direct value for cheap message + JavaTemplate template = JavaTemplate + .builder("#{logger:any(org.slf4j.Logger)}.#{}().log(#{any()})") + .javaParser(JavaParser.fromJavaVersion().classpathFromResources(ctx, "slf4j-api-2.+")) + .build(); + + //noinspection DataFlowIssue + return template.apply(getCursor(), m.getCoordinates().replace(), + m.getSelect(), fluentLevel, arg); + } + } + } + return m; + } + + private boolean isExpensiveArgument(Expression arg) { + return !(arg instanceof J.MethodInvocation && isSimpleGetter((J.MethodInvocation) arg) || + arg instanceof J.Literal || + arg instanceof J.Identifier || + arg instanceof J.FieldAccess || + arg instanceof J.Binary && isOnlyLiterals((J.Binary) arg)); + } + + private boolean isAlreadyUsingFluentApi(Cursor cursor) { + // Check if we're already in a fluent API chain + J.MethodInvocation parent = cursor.firstEnclosing(J.MethodInvocation.class); + if (parent != null && parent.getSimpleName().equals("log")) { + Expression select = parent.getSelect(); + if (select instanceof J.MethodInvocation) { + J.MethodInvocation selectMethod = (J.MethodInvocation) select; + return selectMethod.getSimpleName().equals("addArgument") || + selectMethod.getSimpleName().equals("addParameter") || + selectMethod.getSimpleName().startsWith("at"); + } + } + return false; + } + @Override public J visitCompilationUnit(J.CompilationUnit cu, ExecutionContext ctx) { J j = super.visitCompilationUnit(cu, ctx); @@ -127,6 +245,7 @@ arg instanceof J.Binary && isOnlyLiterals((J.Binary) arg)) } private static boolean isSimpleGetter(J.MethodInvocation mi) { + // Consider it a simple getter if it follows getter naming convention and has no parameters return ((mi.getSimpleName().startsWith("get") && mi.getSimpleName().length() > 3) || (mi.getSimpleName().startsWith("is") && mi.getSimpleName().length() > 2)) && mi.getMethodType() != null && diff --git a/src/main/resources/META-INF/rewrite/classpath.tsv.gz b/src/main/resources/META-INF/rewrite/classpath.tsv.gz index 65262bf..323195c 100644 Binary files a/src/main/resources/META-INF/rewrite/classpath.tsv.gz and b/src/main/resources/META-INF/rewrite/classpath.tsv.gz differ diff --git a/src/test/java/org/openrewrite/java/logging/slf4j/WrapExpensiveLogStatementsInConditionalsSlf4j2Test.java b/src/test/java/org/openrewrite/java/logging/slf4j/WrapExpensiveLogStatementsInConditionalsSlf4j2Test.java new file mode 100644 index 0000000..da97935 --- /dev/null +++ b/src/test/java/org/openrewrite/java/logging/slf4j/WrapExpensiveLogStatementsInConditionalsSlf4j2Test.java @@ -0,0 +1,239 @@ +/* + * Copyright 2025 the original author or authors. + *

+ * Licensed under the Moderne Source Available License (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + *

+ * https://docs.moderne.io/licensing/moderne-source-available-license + *

+ * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.openrewrite.java.logging.slf4j; + +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.CsvSource; +import org.openrewrite.DocumentExample; +import org.openrewrite.InMemoryExecutionContext; +import org.openrewrite.java.JavaParser; +import org.openrewrite.test.RecipeSpec; +import org.openrewrite.test.RewriteTest; + +import static org.openrewrite.java.Assertions.java; + +class WrapExpensiveLogStatementsInConditionalsSlf4j2Test implements RewriteTest { + + @Override + public void defaults(RecipeSpec spec) { + spec.recipe(new WrapExpensiveLogStatementsInConditionals()) + .parser(JavaParser.fromJavaVersion() + .classpathFromResources(new InMemoryExecutionContext(), "slf4j-api-2.1.+")); + } + + @DocumentExample + @Test + void convertToFluentApiWithExpensiveArgument() { + rewriteRun( + //language=java + java( + """ + import org.slf4j.Logger; + + class A { + void method(Logger logger) { + logger.info("Result: {}", calculateResult()); + logger.info("This was {} and {}", doExpensiveOperation(), "bar"); + } + + String calculateResult() { + return "result"; + } + + String doExpensiveOperation() { + return "foo"; + } + } + """, + """ + import org.slf4j.Logger; + + class A { + void method(Logger logger) { + logger.atInfo().addArgument(() -> calculateResult()).log("Result: {}"); + logger.atInfo().addArgument(() -> doExpensiveOperation()).addArgument("bar").log("This was {} and {}"); + } + + String calculateResult() { + return "result"; + } + + String doExpensiveOperation() { + return "foo"; + } + } + """ + ) + ); + } + + @CsvSource(textBlock = """ + info, Info + debug, Debug + trace, Trace + """) + @ParameterizedTest + void allLogMethodsWithFluentApi(String method, String level) { + rewriteRun( + java( + """ + import org.slf4j.Logger; + + class A { + void method(Logger logger) { + logger.%s("SomeString {}, {}", "some param", expensiveOp()); + } + + String expensiveOp() { + return "expensive"; + } + } + """.formatted(method), + """ + import org.slf4j.Logger; + + class A { + void method(Logger logger) { + logger.at%s().addArgument("some param").addArgument(() -> expensiveOp()).log("SomeString {}, {}"); + } + + String expensiveOp() { + return "expensive"; + } + } + """.formatted(level) + ) + ); + } + + @Test + void doNotConvertWithoutExpensiveArguments() { + rewriteRun( + java( + """ + import org.slf4j.Logger; + + class A { + String simpleField = "field"; + + void method(Logger logger) { + logger.info("Simple message"); + logger.info("Message with {}", "literal"); + logger.info("Message with {}", simpleField); + } + } + """ + ) + ); + } + + @Test + void doNotConvertAlreadyFluentApi() { + rewriteRun( + java( + """ + import org.slf4j.Logger; + + class A { + void method(Logger logger) { + logger.atInfo().log("Some message"); + logger.atDebug().addArgument("arg").log("With {}"); + } + } + """ + ) + ); + } + + @Test + void useMethodReferencesForSimpleMethodCalls() { + rewriteRun( + java( + """ + import org.slf4j.Logger; + + class A { + void method(Logger logger) { + logger.info("Value: {}", computeValue()); + logger.debug("Complex: {}", computeValue() + " suffix"); + } + + String computeValue() { + return "value"; + } + } + """, + """ + import org.slf4j.Logger; + + class A { + void method(Logger logger) { + logger.atInfo().addArgument(() -> computeValue()).log("Value: {}"); + logger.atDebug().addArgument(() -> computeValue() + " suffix").log("Complex: {}"); + } + + String computeValue() { + return "value"; + } + } + """ + ) + ); + } + + @Test + void handleBlocksWithExpensiveOperationsUsingFluentApi() { + rewriteRun( + java( + """ + import org.slf4j.Logger; + + class A { + void method(Logger logger) { + System.out.println("an unrelated statement"); + logger.info(expensiveOp()); + logger.info("SomeString {}", "some param"); + logger.info("SomeString {}", expensiveOp()); + System.out.println("another unrelated statement"); + } + + String expensiveOp() { + return "expensive"; + } + } + """, + """ + import org.slf4j.Logger; + + class A { + void method(Logger logger) { + System.out.println("an unrelated statement"); + logger.atInfo().log(() -> expensiveOp()); + logger.info("SomeString {}", "some param"); + logger.atInfo().addArgument(() -> expensiveOp()).log("SomeString {}"); + System.out.println("another unrelated statement"); + } + + String expensiveOp() { + return "expensive"; + } + } + """ + ) + ); + } +} diff --git a/src/test/java/org/openrewrite/java/logging/slf4j/WrapExpensiveLogStatementsInConditionalsTest.java b/src/test/java/org/openrewrite/java/logging/slf4j/WrapExpensiveLogStatementsInConditionalsTest.java index 0bad33a..2e08270 100644 --- a/src/test/java/org/openrewrite/java/logging/slf4j/WrapExpensiveLogStatementsInConditionalsTest.java +++ b/src/test/java/org/openrewrite/java/logging/slf4j/WrapExpensiveLogStatementsInConditionalsTest.java @@ -33,7 +33,7 @@ class WrapExpensiveLogStatementsInConditionalsTest implements RewriteTest { public void defaults(RecipeSpec spec) { spec.recipe(new WrapExpensiveLogStatementsInConditionals()) .parser(JavaParser.fromJavaVersion() - .classpathFromResources(new InMemoryExecutionContext(), "slf4j-api-2.1.+")); + .classpathFromResources(new InMemoryExecutionContext(), "slf4j-api-1.7.+")); } @DocumentExample diff --git a/src/test/resources/META-INF/rewrite/classpath.tsv.gz b/src/test/resources/META-INF/rewrite/classpath.tsv.gz new file mode 100644 index 0000000..9487e80 Binary files /dev/null and b/src/test/resources/META-INF/rewrite/classpath.tsv.gz differ