From 3a8de9d6fee6e5375bcb9d35cf3403ba4d9a9aa4 Mon Sep 17 00:00:00 2001 From: Pierre Delagrave Date: Mon, 21 Jul 2025 14:05:16 -0400 Subject: [PATCH 1/6] support migrating `JUL.log("{0}", arrayIdentifier)` to slf4j --- .../slf4j/JulParameterizedArguments.java | 84 ++++++++++++------- .../slf4j/JulParameterizedArgumentsTest.java | 30 +++++++ 2 files changed, 84 insertions(+), 30 deletions(-) diff --git a/src/main/java/org/openrewrite/java/logging/slf4j/JulParameterizedArguments.java b/src/main/java/org/openrewrite/java/logging/slf4j/JulParameterizedArguments.java index 41b0ca6f..fb187fe1 100644 --- a/src/main/java/org/openrewrite/java/logging/slf4j/JulParameterizedArguments.java +++ b/src/main/java/org/openrewrite/java/logging/slf4j/JulParameterizedArguments.java @@ -88,6 +88,57 @@ private static J.Literal buildStringLiteral(String string) { return new J.Literal(randomId(), Space.EMPTY, Markers.EMPTY, string, String.format("\"%s\"", string), null, JavaType.Primitive.String); } + /** + * Create a list of expression representing each element of an array + * + * @param arrayExpression Either a J.NewArray or a J.Identifier of an array + * @param indiceCount size of the array + * @return A List of expression representing each item of the array. If the passed arrayExpression isn't of type J.NewArray or J.Identifier, the arrayExpression is returned alone in a list. + */ + private static List splitArrayToExpressions(Expression arrayExpression, int indiceCount) { + if (arrayExpression instanceof J.NewArray) { + final List initializer = ((J.NewArray) arrayExpression).getInitializer(); + if (initializer == null || initializer.isEmpty()) { + return Collections.emptyList(); + } + return initializer; + } + if (arrayExpression instanceof J.Identifier && arrayExpression.getType() instanceof JavaType.Array) { + List arrayAccessExpr = new ArrayList<>(indiceCount); + for (int i = 0; i < indiceCount; i++) { + arrayAccessExpr.add( + new J.ArrayAccess(randomId(), Space.EMPTY, Markers.EMPTY, arrayExpression.withPrefix(Space.EMPTY), + new J.ArrayDimension(randomId(), Space.EMPTY, Markers.EMPTY, + new JRightPadded<>( + new J.Literal( + randomId(), Space.EMPTY, Markers.EMPTY, i, String.valueOf(i), null, JavaType.Primitive.Int + ), Space.EMPTY, Markers.EMPTY + ) + ), arrayExpression.getType() + ) + ); + } + return arrayAccessExpr; + } + return Collections.singletonList(arrayExpression); + } + + private static String createTemplateString(String newName, List targetArguments) { + List targetArgumentsStrings = new ArrayList<>(); + targetArguments.forEach(targetArgument -> targetArgumentsStrings.add("#{any()}")); + return newName + '(' + String.join(",", targetArgumentsStrings) + ')'; + } + + private static List originalLoggedArgumentIndices(String strFormat) { + // A string format like "Hello {0} {1} {1}" should be transformed to 0, 1, 1 + Matcher matcher = Pattern.compile("\\{(\\d+)}").matcher(strFormat); + List loggedArgumentIndices = new ArrayList<>(2); + while (matcher.find()) { + loggedArgumentIndices.add(Integer.valueOf(matcher.group(1))); + } + return loggedArgumentIndices; + } + @Override public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, ExecutionContext ctx) { if (METHOD_MATCHER_ARRAY.matches(method) || METHOD_MATCHER_PARAM.matches(method)) { @@ -101,18 +152,18 @@ public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, Execu return method; } String newName = getMethodIdentifier(levelArgument); - if(newName == null) { + if (newName == null) { return method; } maybeRemoveImport("java.util.logging.Level"); String originalFormatString = Objects.requireNonNull((String) ((J.Literal) messageArgument).getValue()); List originalIndices = originalLoggedArgumentIndices(originalFormatString); - List originalParameters = originalParameters(originalArguments.get(2)); + List stringFormatArguments = splitArrayToExpressions(originalArguments.get(2), originalIndices.size()); List targetArguments = new ArrayList<>(2); targetArguments.add(buildStringLiteral(originalFormatString.replaceAll("\\{\\d*}", "{}"))); - originalIndices.forEach(i -> targetArguments.add(originalParameters.get(i))); + originalIndices.forEach(i -> targetArguments.add(stringFormatArguments.get(i))); return JavaTemplate.builder(createTemplateString(newName, targetArguments)) .contextSensitive() .javaParser(JavaParser.fromJavaVersion() @@ -122,32 +173,5 @@ public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, Execu } return super.visitMethodInvocation(method, ctx); } - - private List originalLoggedArgumentIndices(String strFormat) { - // A string format like "Hello {0} {1} {1}" should be transformed to 0, 1, 1 - Matcher matcher = Pattern.compile("\\{(\\d+)}").matcher(strFormat); - List loggedArgumentIndices = new ArrayList<>(2); - while (matcher.find()) { - loggedArgumentIndices.add(Integer.valueOf(matcher.group(1))); - } - return loggedArgumentIndices; - } - - private static List originalParameters(Expression logParameters) { - if (logParameters instanceof J.NewArray) { - final List initializer = ((J.NewArray) logParameters).getInitializer(); - if (initializer == null || initializer.isEmpty()) { - return Collections.emptyList(); - } - return initializer; - } - return Collections.singletonList(logParameters); - } - - private static String createTemplateString(String newName, List targetArguments) { - List targetArgumentsStrings = new ArrayList<>(); - targetArguments.forEach(targetArgument -> targetArgumentsStrings.add("#{any()}")); - return newName + '(' + String.join(",", targetArgumentsStrings) + ')'; - } } } diff --git a/src/test/java/org/openrewrite/java/logging/slf4j/JulParameterizedArgumentsTest.java b/src/test/java/org/openrewrite/java/logging/slf4j/JulParameterizedArgumentsTest.java index e7388066..d7c0d350 100644 --- a/src/test/java/org/openrewrite/java/logging/slf4j/JulParameterizedArgumentsTest.java +++ b/src/test/java/org/openrewrite/java/logging/slf4j/JulParameterizedArgumentsTest.java @@ -116,6 +116,36 @@ void method(Logger logger, String param1, String param2) { ); } + @Test + void arrayIdentifierArgument() { + rewriteRun( + // language=java + java( + """ + import java.util.logging.Level; + import java.util.logging.Logger; + + class Test { + void method(Logger logger, String[] params) { + logger.log(Level.INFO, "INFO Log entry, param2: {1}, param1: {0}, etc", params); + logger.log(Level.INFO, "INFO Log entry, param1: {0}, param2: {1}, etc", params); + } + } + """, + """ + import org.slf4j.Logger; + + class Test { + void method(Logger logger, String[] params) { + logger.info("INFO Log entry, param2: {}, param1: {}, etc", params[1], params[0]); + logger.info("INFO Log entry, param1: {}, param2: {}, etc", params[0], params[1]); + } + } + """ + ) + ); + } + @Test void repeatLoggedArgumentAsNeeded() { rewriteRun( From 6ff6128394a0b49aafc5930fd60b0af360d43da5 Mon Sep 17 00:00:00 2001 From: Pierre Delagrave Date: Wed, 30 Jul 2025 12:25:43 -0400 Subject: [PATCH 2/6] applied multiple review suggestions --- .../slf4j/JulParameterizedArguments.java | 212 ++++++++---------- .../slf4j/JulParameterizedArgumentsTest.java | 28 +++ 2 files changed, 127 insertions(+), 113 deletions(-) diff --git a/src/main/java/org/openrewrite/java/logging/slf4j/JulParameterizedArguments.java b/src/main/java/org/openrewrite/java/logging/slf4j/JulParameterizedArguments.java index fb187fe1..93f5f7a3 100644 --- a/src/main/java/org/openrewrite/java/logging/slf4j/JulParameterizedArguments.java +++ b/src/main/java/org/openrewrite/java/logging/slf4j/JulParameterizedArguments.java @@ -20,6 +20,7 @@ import org.openrewrite.Preconditions; import org.openrewrite.Recipe; import org.openrewrite.TreeVisitor; +import org.openrewrite.internal.StringUtils; import org.openrewrite.java.JavaIsoVisitor; import org.openrewrite.java.JavaParser; import org.openrewrite.java.JavaTemplate; @@ -35,11 +36,12 @@ import java.util.regex.Matcher; import java.util.regex.Pattern; +import static java.util.Collections.emptyList; import static org.openrewrite.Tree.randomId; public class JulParameterizedArguments extends Recipe { - private static final MethodMatcher METHOD_MATCHER_PARAM = new MethodMatcher("java.util.logging.Logger log(java.util.logging.Level, java.lang.String, java.lang.Object)"); - private static final MethodMatcher METHOD_MATCHER_ARRAY = new MethodMatcher("java.util.logging.Logger log(java.util.logging.Level, java.lang.String, java.lang.Object[])"); + private static final MethodMatcher METHOD_MATCHER_PARAM = new MethodMatcher("java.util.logging.Logger log(java.util.logging.Level, String, Object)"); + private static final MethodMatcher METHOD_MATCHER_ARRAY = new MethodMatcher("java.util.logging.Logger log(java.util.logging.Level, String, Object[])"); @Override public String getDisplayName() { @@ -53,125 +55,109 @@ public String getDescription() { @Override public TreeVisitor getVisitor() { - return Preconditions.check(Preconditions.or(new UsesMethod<>(METHOD_MATCHER_PARAM), new UsesMethod<>(METHOD_MATCHER_ARRAY)), new JulParameterizedToSlf4jVisitor()); - } - - private static class JulParameterizedToSlf4jVisitor extends JavaIsoVisitor { - - public static boolean isStringLiteral(Expression expression) { - return expression instanceof J.Literal && TypeUtils.isString(((J.Literal) expression).getType()); - } - - private static @Nullable String getMethodIdentifier(Expression levelArgument) { - String levelSimpleName = levelArgument instanceof J.FieldAccess ? - (((J.FieldAccess) levelArgument).getName().getSimpleName()) : - (((J.Identifier) levelArgument).getSimpleName()); - switch (levelSimpleName) { - case "ALL": - case "FINEST": - case "FINER": - return "trace"; - case "FINE": - return "debug"; - case "CONFIG": - case "INFO": - return "info"; - case "WARNING": - return "warn"; - case "SEVERE": - return "error"; - } - return null; - } - - private static J.Literal buildStringLiteral(String string) { - return new J.Literal(randomId(), Space.EMPTY, Markers.EMPTY, string, String.format("\"%s\"", string), null, JavaType.Primitive.String); - } - - /** - * Create a list of expression representing each element of an array - * - * @param arrayExpression Either a J.NewArray or a J.Identifier of an array - * @param indiceCount size of the array - * @return A List of expression representing each item of the array. If the passed arrayExpression isn't of type J.NewArray or J.Identifier, the arrayExpression is returned alone in a list. - */ - private static List splitArrayToExpressions(Expression arrayExpression, int indiceCount) { - if (arrayExpression instanceof J.NewArray) { - final List initializer = ((J.NewArray) arrayExpression).getInitializer(); - if (initializer == null || initializer.isEmpty()) { - return Collections.emptyList(); + return Preconditions.check(Preconditions.or(new UsesMethod<>(METHOD_MATCHER_PARAM), new UsesMethod<>(METHOD_MATCHER_ARRAY)), new JavaIsoVisitor() { + @Override + public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, ExecutionContext ctx) { + if (METHOD_MATCHER_ARRAY.matches(method) || METHOD_MATCHER_PARAM.matches(method)) { + List originalArguments = method.getArguments(); + + Expression levelArgument = originalArguments.get(0); + Expression messageArgument = originalArguments.get(1); + + if (!(levelArgument instanceof J.FieldAccess || levelArgument instanceof J.Identifier) || !isStringLiteral(messageArgument)) { + return method; + } + String newMethodName = getMethodIdentifier(levelArgument); + if (newMethodName == null) { + return method; + } + maybeRemoveImport("java.util.logging.Level"); + + String originalFormatString = Objects.requireNonNull((String) ((J.Literal) messageArgument).getValue()); + List originalIndices = originalLoggedArgumentIndices(originalFormatString); + List stringFormatArguments = splitArrayToExpressions(originalArguments.get(2), originalIndices.size()); + + List targetArguments = new ArrayList<>(2); + targetArguments.add(buildStringLiteral(originalFormatString.replaceAll("\\{\\d*}", "{}"))); + originalIndices.forEach(i -> targetArguments.add(stringFormatArguments.get(i))); + String newInvocationCode = newMethodName + '(' + StringUtils.repeat(",#{any()}", targetArguments.size()).substring(1) + ')'; + + return JavaTemplate + .builder(newInvocationCode) + .javaParser(JavaParser.fromJavaVersion().classpathFromResources(ctx, "slf4j-api-2.1.+")) + .build() + .apply( + getCursor(), + method.getCoordinates().replaceMethod(), + targetArguments.toArray() + ); } - return initializer; + return super.visitMethodInvocation(method, ctx); } - if (arrayExpression instanceof J.Identifier && arrayExpression.getType() instanceof JavaType.Array) { - List arrayAccessExpr = new ArrayList<>(indiceCount); - for (int i = 0; i < indiceCount; i++) { - arrayAccessExpr.add( - new J.ArrayAccess(randomId(), Space.EMPTY, Markers.EMPTY, arrayExpression.withPrefix(Space.EMPTY), - new J.ArrayDimension(randomId(), Space.EMPTY, Markers.EMPTY, - new JRightPadded<>( - new J.Literal( - randomId(), Space.EMPTY, Markers.EMPTY, i, String.valueOf(i), null, JavaType.Primitive.Int - ), Space.EMPTY, Markers.EMPTY - ) - ), arrayExpression.getType() - ) - ); - } - return arrayAccessExpr; - } - return Collections.singletonList(arrayExpression); - } - - private static String createTemplateString(String newName, List targetArguments) { - List targetArgumentsStrings = new ArrayList<>(); - targetArguments.forEach(targetArgument -> targetArgumentsStrings.add("#{any()}")); - return newName + '(' + String.join(",", targetArgumentsStrings) + ')'; - } - - private static List originalLoggedArgumentIndices(String strFormat) { - // A string format like "Hello {0} {1} {1}" should be transformed to 0, 1, 1 - Matcher matcher = Pattern.compile("\\{(\\d+)}").matcher(strFormat); - List loggedArgumentIndices = new ArrayList<>(2); - while (matcher.find()) { - loggedArgumentIndices.add(Integer.valueOf(matcher.group(1))); + + private boolean isStringLiteral(Expression expression) { + return expression instanceof J.Literal && TypeUtils.isString(((J.Literal) expression).getType()); } - return loggedArgumentIndices; - } - @Override - public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, ExecutionContext ctx) { - if (METHOD_MATCHER_ARRAY.matches(method) || METHOD_MATCHER_PARAM.matches(method)) { - List originalArguments = method.getArguments(); + private @Nullable String getMethodIdentifier(Expression levelArgument) { + String levelSimpleName = levelArgument instanceof J.FieldAccess ? (((J.FieldAccess) levelArgument).getName().getSimpleName()) : (((J.Identifier) levelArgument).getSimpleName()); + switch (levelSimpleName) { + case "ALL": + case "FINEST": + case "FINER": + return "trace"; + case "FINE": + return "debug"; + case "CONFIG": + case "INFO": + return "info"; + case "WARNING": + return "warn"; + case "SEVERE": + return "error"; + } + return null; + } - Expression levelArgument = originalArguments.get(0); - Expression messageArgument = originalArguments.get(1); + private J.Literal buildStringLiteral(String string) { + return new J.Literal(randomId(), Space.EMPTY, Markers.EMPTY, string, String.format("\"%s\"", string), null, JavaType.Primitive.String); + } - if (!(levelArgument instanceof J.FieldAccess || levelArgument instanceof J.Identifier) || - !isStringLiteral(messageArgument)) { - return method; + /** + * Create a list of expression representing each element of an array + * + * @param arrayExpression Either a J.NewArray or a J.Identifier of an array + * @param indiceCount size of the array + * @return A List of expression representing each item of the array. If the passed arrayExpression isn't of type J.NewArray or J.Identifier, the arrayExpression is returned alone in a list. + */ + private List splitArrayToExpressions(Expression arrayExpression, int indiceCount) { + if (arrayExpression instanceof J.NewArray) { + List initializer = ((J.NewArray) arrayExpression).getInitializer(); + return initializer == null ? emptyList() : initializer; + } + if (arrayExpression instanceof J.Identifier && arrayExpression.getType() instanceof JavaType.Array) { + List arrayAccessExpr = new ArrayList<>(indiceCount); + for (int i = 0; i < indiceCount; i++) { + J.Literal literal = new J.Literal(randomId(), Space.EMPTY, Markers.EMPTY, i, String.valueOf(i), null, JavaType.Primitive.Int); + J.ArrayDimension dimension = new J.ArrayDimension(randomId(), Space.EMPTY, Markers.EMPTY, JRightPadded.build(literal)); + J.ArrayAccess arrayAccess = new J.ArrayAccess(randomId(), Space.EMPTY, Markers.EMPTY, arrayExpression.withPrefix(Space.EMPTY), dimension, arrayExpression.getType()); + arrayAccessExpr.add(arrayAccess); + } + return arrayAccessExpr; } - String newName = getMethodIdentifier(levelArgument); - if (newName == null) { - return method; + return Collections.singletonList(arrayExpression); + } + + private List originalLoggedArgumentIndices(String strFormat) { + // A string format like "Hello {0} {1} {1}" should be transformed to 0, 1, 1 + Matcher matcher = Pattern.compile("\\{(\\d+)}").matcher(strFormat); + List loggedArgumentIndices = new ArrayList<>(2); + while (matcher.find()) { + loggedArgumentIndices.add(Integer.valueOf(matcher.group(1))); } - maybeRemoveImport("java.util.logging.Level"); - - String originalFormatString = Objects.requireNonNull((String) ((J.Literal) messageArgument).getValue()); - List originalIndices = originalLoggedArgumentIndices(originalFormatString); - List stringFormatArguments = splitArrayToExpressions(originalArguments.get(2), originalIndices.size()); - - List targetArguments = new ArrayList<>(2); - targetArguments.add(buildStringLiteral(originalFormatString.replaceAll("\\{\\d*}", "{}"))); - originalIndices.forEach(i -> targetArguments.add(stringFormatArguments.get(i))); - return JavaTemplate.builder(createTemplateString(newName, targetArguments)) - .contextSensitive() - .javaParser(JavaParser.fromJavaVersion() - .classpathFromResources(ctx, "slf4j-api-2.1.+")) - .build() - .apply(getCursor(), method.getCoordinates().replaceMethod(), targetArguments.toArray()); + return loggedArgumentIndices; } - return super.visitMethodInvocation(method, ctx); - } + }); } + } diff --git a/src/test/java/org/openrewrite/java/logging/slf4j/JulParameterizedArgumentsTest.java b/src/test/java/org/openrewrite/java/logging/slf4j/JulParameterizedArgumentsTest.java index d7c0d350..e973f686 100644 --- a/src/test/java/org/openrewrite/java/logging/slf4j/JulParameterizedArgumentsTest.java +++ b/src/test/java/org/openrewrite/java/logging/slf4j/JulParameterizedArgumentsTest.java @@ -88,6 +88,34 @@ void method(Logger logger, String param1, String param2) { ); } + @Test + void parameterizedArgumentArrayWithNoInitializer() { + rewriteRun( + // language=java + java( + """ + import java.util.logging.Level; + import java.util.logging.Logger; + + class Test { + void method(Logger logger) { + logger.log(Level.INFO, "INFO Log entry", new String[]{}); + } + } + """, + """ + import org.slf4j.Logger; + + class Test { + void method(Logger logger) { + logger.info("INFO Log entry"); + } + } + """ + ) + ); + } + @Test void retainLoggedArgumentOrder() { rewriteRun( From fdd33bd58f61aa144557fd4cf826e9875f645e42 Mon Sep 17 00:00:00 2001 From: Tim te Beek Date: Thu, 31 Jul 2025 13:15:07 +0200 Subject: [PATCH 3/6] Use static import for Collections --- .../java/logging/slf4j/JulParameterizedArguments.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/main/java/org/openrewrite/java/logging/slf4j/JulParameterizedArguments.java b/src/main/java/org/openrewrite/java/logging/slf4j/JulParameterizedArguments.java index 93f5f7a3..39916a5c 100644 --- a/src/main/java/org/openrewrite/java/logging/slf4j/JulParameterizedArguments.java +++ b/src/main/java/org/openrewrite/java/logging/slf4j/JulParameterizedArguments.java @@ -37,6 +37,7 @@ import java.util.regex.Pattern; import static java.util.Collections.emptyList; +import static java.util.Collections.singletonList; import static org.openrewrite.Tree.randomId; public class JulParameterizedArguments extends Recipe { @@ -145,7 +146,7 @@ private List splitArrayToExpressions(Expression arrayExpression, int } return arrayAccessExpr; } - return Collections.singletonList(arrayExpression); + return singletonList(arrayExpression); } private List originalLoggedArgumentIndices(String strFormat) { From f8f51e32516a4cd6fe399b0de333fcf6d80ec91c Mon Sep 17 00:00:00 2001 From: Tim te Beek Date: Thu, 31 Jul 2025 15:54:07 +0200 Subject: [PATCH 4/6] Apply suggestions from code review --- .../logging/slf4j/JulParameterizedArgumentsTest.java | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/src/test/java/org/openrewrite/java/logging/slf4j/JulParameterizedArgumentsTest.java b/src/test/java/org/openrewrite/java/logging/slf4j/JulParameterizedArgumentsTest.java index e973f686..d3ee584d 100644 --- a/src/test/java/org/openrewrite/java/logging/slf4j/JulParameterizedArgumentsTest.java +++ b/src/test/java/org/openrewrite/java/logging/slf4j/JulParameterizedArgumentsTest.java @@ -159,16 +159,6 @@ void method(Logger logger, String[] params) { logger.log(Level.INFO, "INFO Log entry, param1: {0}, param2: {1}, etc", params); } } - """, - """ - import org.slf4j.Logger; - - class Test { - void method(Logger logger, String[] params) { - logger.info("INFO Log entry, param2: {}, param1: {}, etc", params[1], params[0]); - logger.info("INFO Log entry, param1: {}, param2: {}, etc", params[0], params[1]); - } - } """ ) ); From b82f6185c76429307f96d9a6023426365e0c44df Mon Sep 17 00:00:00 2001 From: Pierre Delagrave Date: Thu, 31 Jul 2025 11:44:47 -0400 Subject: [PATCH 5/6] Revert the array unpacking improvement First time testing for a negative case shows that the recipe isn't producing valid code by itself and that `org.openrewrite.java.logging.slf4j.JulToSlf4j` isn't either. Fixed in the next commit. --- .../slf4j/JulParameterizedArguments.java | 109 ++++++++---------- 1 file changed, 48 insertions(+), 61 deletions(-) diff --git a/src/main/java/org/openrewrite/java/logging/slf4j/JulParameterizedArguments.java b/src/main/java/org/openrewrite/java/logging/slf4j/JulParameterizedArguments.java index 39916a5c..7a0db49a 100644 --- a/src/main/java/org/openrewrite/java/logging/slf4j/JulParameterizedArguments.java +++ b/src/main/java/org/openrewrite/java/logging/slf4j/JulParameterizedArguments.java @@ -30,7 +30,6 @@ import org.openrewrite.marker.Markers; import java.util.ArrayList; -import java.util.Collections; import java.util.List; import java.util.Objects; import java.util.regex.Matcher; @@ -58,42 +57,55 @@ public String getDescription() { public TreeVisitor getVisitor() { return Preconditions.check(Preconditions.or(new UsesMethod<>(METHOD_MATCHER_PARAM), new UsesMethod<>(METHOD_MATCHER_ARRAY)), new JavaIsoVisitor() { @Override - public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, ExecutionContext ctx) { - if (METHOD_MATCHER_ARRAY.matches(method) || METHOD_MATCHER_PARAM.matches(method)) { - List originalArguments = method.getArguments(); - - Expression levelArgument = originalArguments.get(0); - Expression messageArgument = originalArguments.get(1); - - if (!(levelArgument instanceof J.FieldAccess || levelArgument instanceof J.Identifier) || !isStringLiteral(messageArgument)) { - return method; - } - String newMethodName = getMethodIdentifier(levelArgument); - if (newMethodName == null) { - return method; - } - maybeRemoveImport("java.util.logging.Level"); - - String originalFormatString = Objects.requireNonNull((String) ((J.Literal) messageArgument).getValue()); - List originalIndices = originalLoggedArgumentIndices(originalFormatString); - List stringFormatArguments = splitArrayToExpressions(originalArguments.get(2), originalIndices.size()); - - List targetArguments = new ArrayList<>(2); - targetArguments.add(buildStringLiteral(originalFormatString.replaceAll("\\{\\d*}", "{}"))); - originalIndices.forEach(i -> targetArguments.add(stringFormatArguments.get(i))); - String newInvocationCode = newMethodName + '(' + StringUtils.repeat(",#{any()}", targetArguments.size()).substring(1) + ')'; - - return JavaTemplate - .builder(newInvocationCode) - .javaParser(JavaParser.fromJavaVersion().classpathFromResources(ctx, "slf4j-api-2.1.+")) - .build() - .apply( - getCursor(), - method.getCoordinates().replaceMethod(), - targetArguments.toArray() - ); + public J.MethodInvocation visitMethodInvocation(J.MethodInvocation mi, ExecutionContext ctx) { + mi = super.visitMethodInvocation(mi, ctx); + + if (!(METHOD_MATCHER_ARRAY.matches(mi) || METHOD_MATCHER_PARAM.matches(mi))) { + return mi; + } + + List originalArguments = mi.getArguments(); + Expression levelArgument = originalArguments.get(0); + Expression messageArgument = originalArguments.get(1); + Expression stringFormatArgument = originalArguments.get(2); + + if (!(levelArgument instanceof J.FieldAccess || levelArgument instanceof J.Identifier) || !isStringLiteral(messageArgument)) { + return mi; + } + String newMethodName = getMethodIdentifier(levelArgument); + if (newMethodName == null) { + return mi; + } + if (stringFormatArgument instanceof J.Identifier && stringFormatArgument.getType() instanceof JavaType.Array) { + return mi; + } + + maybeRemoveImport("java.util.logging.Level"); + + String originalFormatString = Objects.requireNonNull((String) ((J.Literal) messageArgument).getValue()); + List originalIndices = originalLoggedArgumentIndices(originalFormatString); + List stringFormatArguments; + if (stringFormatArgument instanceof J.NewArray) { + List initializer = ((J.NewArray) stringFormatArgument).getInitializer(); + stringFormatArguments = initializer == null ? emptyList() : initializer; + } else { + stringFormatArguments = singletonList(stringFormatArgument); } - return super.visitMethodInvocation(method, ctx); + + List targetArguments = new ArrayList<>(2); + targetArguments.add(buildStringLiteral(originalFormatString.replaceAll("\\{\\d*}", "{}"))); + originalIndices.forEach(i -> targetArguments.add(stringFormatArguments.get(i))); + String newInvocationCode = newMethodName + '(' + StringUtils.repeat(",#{any()}", targetArguments.size()).substring(1) + ')'; + + return JavaTemplate + .builder(newInvocationCode) + .javaParser(JavaParser.fromJavaVersion().classpathFromResources(ctx, "slf4j-api-2.1.+")) + .build() + .apply( + getCursor(), + mi.getCoordinates().replaceMethod(), + targetArguments.toArray() + ); } private boolean isStringLiteral(Expression expression) { @@ -124,31 +136,6 @@ private J.Literal buildStringLiteral(String string) { return new J.Literal(randomId(), Space.EMPTY, Markers.EMPTY, string, String.format("\"%s\"", string), null, JavaType.Primitive.String); } - /** - * Create a list of expression representing each element of an array - * - * @param arrayExpression Either a J.NewArray or a J.Identifier of an array - * @param indiceCount size of the array - * @return A List of expression representing each item of the array. If the passed arrayExpression isn't of type J.NewArray or J.Identifier, the arrayExpression is returned alone in a list. - */ - private List splitArrayToExpressions(Expression arrayExpression, int indiceCount) { - if (arrayExpression instanceof J.NewArray) { - List initializer = ((J.NewArray) arrayExpression).getInitializer(); - return initializer == null ? emptyList() : initializer; - } - if (arrayExpression instanceof J.Identifier && arrayExpression.getType() instanceof JavaType.Array) { - List arrayAccessExpr = new ArrayList<>(indiceCount); - for (int i = 0; i < indiceCount; i++) { - J.Literal literal = new J.Literal(randomId(), Space.EMPTY, Markers.EMPTY, i, String.valueOf(i), null, JavaType.Primitive.Int); - J.ArrayDimension dimension = new J.ArrayDimension(randomId(), Space.EMPTY, Markers.EMPTY, JRightPadded.build(literal)); - J.ArrayAccess arrayAccess = new J.ArrayAccess(randomId(), Space.EMPTY, Markers.EMPTY, arrayExpression.withPrefix(Space.EMPTY), dimension, arrayExpression.getType()); - arrayAccessExpr.add(arrayAccess); - } - return arrayAccessExpr; - } - return singletonList(arrayExpression); - } - private List originalLoggedArgumentIndices(String strFormat) { // A string format like "Hello {0} {1} {1}" should be transformed to 0, 1, 1 Matcher matcher = Pattern.compile("\\{(\\d+)}").matcher(strFormat); From 03dfd97f9f6126f2fbd321990582e249d0ace9a0 Mon Sep 17 00:00:00 2001 From: Tim te Beek Date: Mon, 4 Aug 2025 17:51:48 +0200 Subject: [PATCH 6/6] Skip `JulParameterizedArguments` to avoid unsafe change there --- .../java/logging/slf4j/JulParameterizedArguments.java | 5 +++++ .../java/logging/slf4j/JulParameterizedArgumentsTest.java | 4 ++++ 2 files changed, 9 insertions(+) diff --git a/src/main/java/org/openrewrite/java/logging/slf4j/JulParameterizedArguments.java b/src/main/java/org/openrewrite/java/logging/slf4j/JulParameterizedArguments.java index 624c2624..1d52d6f2 100644 --- a/src/main/java/org/openrewrite/java/logging/slf4j/JulParameterizedArguments.java +++ b/src/main/java/org/openrewrite/java/logging/slf4j/JulParameterizedArguments.java @@ -92,6 +92,11 @@ public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, Execu Expression messageArgument = originalArguments.get(1); Expression stringFormatArgument = originalArguments.get(2); + if (stringFormatArgument.getType() instanceof JavaType.Array && + !(stringFormatArgument instanceof J.NewArray)) { + return method; + } + if (!(levelArgument instanceof J.FieldAccess || levelArgument instanceof J.Identifier) || !isStringLiteral(messageArgument)) { return method; diff --git a/src/test/java/org/openrewrite/java/logging/slf4j/JulParameterizedArgumentsTest.java b/src/test/java/org/openrewrite/java/logging/slf4j/JulParameterizedArgumentsTest.java index d3ee584d..48cfd34f 100644 --- a/src/test/java/org/openrewrite/java/logging/slf4j/JulParameterizedArgumentsTest.java +++ b/src/test/java/org/openrewrite/java/logging/slf4j/JulParameterizedArgumentsTest.java @@ -15,8 +15,10 @@ */ package org.openrewrite.java.logging.slf4j; +import org.junit.jupiter.api.Disabled; import org.junit.jupiter.api.Test; import org.openrewrite.DocumentExample; +import org.openrewrite.Issue; import org.openrewrite.test.RecipeSpec; import org.openrewrite.test.RewriteTest; @@ -144,6 +146,8 @@ void method(Logger logger, String param1, String param2) { ); } + @Disabled("Skipped by `JulParameterizedArguments`, but incomplete changes seen from JUL -> Log4j -> Slf4j") + @Issue("https://github.com/openrewrite/rewrite-logging-frameworks/pull/244#issuecomment-3140661425") @Test void arrayIdentifierArgument() { rewriteRun(