Skip to content

Avoid unsafe changes in JulParameterizedArguments when JavaType.Array argument is not a J.NewArray #244

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 9 commits into from
Aug 4, 2025
Merged

Avoid unsafe changes in JulParameterizedArguments when JavaType.Array argument is not a J.NewArray #244

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
3a8de9d
support migrating `JUL.log("{0}", arrayIdentifier)` to slf4j
pdelagrave Jul 21, 2025
6ff6128
applied multiple review suggestions
pdelagrave Jul 30, 2025
3c1c0d1
Merge branch 'main' into julparamsimprovement
pdelagrave Jul 30, 2025
2e9d58c
Merge remote-tracking branch 'origin/main' into julparamsimprovement
timtebeek Jul 31, 2025
fdd33bd
Use static import for Collections
timtebeek Jul 31, 2025
f8f51e3
Apply suggestions from code review
timtebeek Jul 31, 2025
b82f618
Revert the array unpacking improvement
pdelagrave Jul 31, 2025
f533f53
Merge remote-tracking branch 'origin/main' into julparamsimprovement
timtebeek Aug 4, 2025
03dfd97
Skip `JulParameterizedArguments` to avoid unsafe change there
timtebeek Aug 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
186 changes: 98 additions & 88 deletions src/main/java/org/openrewrite/java/logging/slf4j/JulParameterizedArguments.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -29,6 +30,7 @@
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;
Expand All @@ -39,8 +41,8 @@
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() {
Expand All @@ -54,101 +56,109 @@ public String getDescription() {

@Override
public TreeVisitor<?, ExecutionContext> getVisitor() {
return Preconditions.check(Preconditions.or(new UsesMethod<>(METHOD_MATCHER_PARAM), new UsesMethod<>(METHOD_MATCHER_ARRAY)), new JulParameterizedToSlf4jVisitor());
}

private static class JulParameterizedToSlf4jVisitor extends JavaIsoVisitor<ExecutionContext> {

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 Preconditions.check(Preconditions.or(new UsesMethod<>(METHOD_MATCHER_PARAM), new UsesMethod<>(METHOD_MATCHER_ARRAY)), new JavaIsoVisitor<ExecutionContext>() {
@Override
public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, ExecutionContext ctx) {
if (METHOD_MATCHER_ARRAY.matches(method) || METHOD_MATCHER_PARAM.matches(method)) {
List<Expression> 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<Integer> originalIndices = originalLoggedArgumentIndices(originalFormatString);
List<Expression> stringFormatArguments = splitArrayToExpressions(originalArguments.get(2), originalIndices.size());

List<Expression> 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 super.visitMethodInvocation(method, ctx);
}
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);
}
private boolean isStringLiteral(Expression expression) {
return expression instanceof J.Literal && TypeUtils.isString(((J.Literal) expression).getType());
}

@Override
public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, ExecutionContext ctx) {
if (METHOD_MATCHER_ARRAY.matches(method) || METHOD_MATCHER_PARAM.matches(method)) {
List<Expression> 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<Expression> splitArrayToExpressions(Expression arrayExpression, int indiceCount) {
if (arrayExpression instanceof J.NewArray) {
List<Expression> initializer = ((J.NewArray) arrayExpression).getInitializer();
return initializer == null ? emptyList() : initializer;
}
String newName = getMethodIdentifier(levelArgument);
if(newName == null) {
return method;
if (arrayExpression instanceof J.Identifier && arrayExpression.getType() instanceof JavaType.Array) {
List<Expression> 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;
}
maybeRemoveImport("java.util.logging.Level");

String originalFormatString = Objects.requireNonNull((String) ((J.Literal) messageArgument).getValue());
List<Integer> originalIndices = originalLoggedArgumentIndices(originalFormatString);
List<Expression> originalParameters = originalParameters(originalArguments.get(2));

List<Expression> targetArguments = new ArrayList<>(2);
targetArguments.add(buildStringLiteral(originalFormatString.replaceAll("\\{\\d*}", "{}")));
originalIndices.forEach(i -> targetArguments.add(originalParameters.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 super.visitMethodInvocation(method, ctx);
}

private List<Integer> 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<Integer> loggedArgumentIndices = new ArrayList<>(2);
while (matcher.find()) {
loggedArgumentIndices.add(Integer.valueOf(matcher.group(1)));
return singletonList(arrayExpression);
}
return loggedArgumentIndices;
}

private static List<Expression> originalParameters(Expression logParameters) {
if (logParameters instanceof J.NewArray) {
final List<Expression> initializer = ((J.NewArray) logParameters).getInitializer();
if (initializer == null || initializer.isEmpty()) {
return emptyList();

private List<Integer> 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<Integer> loggedArgumentIndices = new ArrayList<>(2);
while (matcher.find()) {
loggedArgumentIndices.add(Integer.valueOf(matcher.group(1)));
}
return initializer;
return loggedArgumentIndices;
}
return singletonList(logParameters);
}

private static String createTemplateString(String newName, List<Expression> targetArguments) {
List<String> targetArgumentsStrings = new ArrayList<>();
targetArguments.forEach(targetArgument -> targetArgumentsStrings.add("#{any()}"));
return newName + '(' + String.join(",", targetArgumentsStrings) + ')';
}
});
}

}
58 changes: 58 additions & 0 deletions src/test/java/org/openrewrite/java/logging/slf4j/JulParameterizedArgumentsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -116,6 +144,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(
Expand Down
Loading