diff --git a/src/main/java/org/openrewrite/java/logging/ParameterizedLogging.java b/src/main/java/org/openrewrite/java/logging/ParameterizedLogging.java index c6a69390..33afa9d8 100644 --- a/src/main/java/org/openrewrite/java/logging/ParameterizedLogging.java +++ b/src/main/java/org/openrewrite/java/logging/ParameterizedLogging.java @@ -81,12 +81,35 @@ public TreeVisitor getVisitor() { @Override public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, ExecutionContext ctx) { J.MethodInvocation m = super.visitMethodInvocation(method, ctx); - if (matcher.matches(m) && !m.getArguments().isEmpty() && !(m.getArguments().get(0) instanceof J.Empty) && m.getArguments().size() <= 2) { + if (matcher.matches(m) && !m.getArguments().isEmpty() && !(m.getArguments().get(0) instanceof J.Empty)) { final int logMsgIndex = isMarker(m.getArguments().get(0)) ? 1 : 0; + // Only process if we have at most 2 arguments after accounting for marker + if (m.getArguments().size() - logMsgIndex > 2) { + return m; + } Expression logMsg = m.getArguments().get(logMsgIndex); if (logMsg instanceof J.Binary) { StringBuilder messageBuilder = new StringBuilder(); List newArgList = new ArrayList<>(); + List concatenationArgs = new ArrayList<>(); + List regularArgs = new ArrayList<>(); + Expression possibleThrowable = null; + + // First, process all arguments + for (int index = 0; index < m.getArguments().size(); index++) { + Expression arg = m.getArguments().get(index); + if (index == logMsgIndex && arg instanceof J.Binary) { + MessageAndArguments literalAndArgs = concatenationToLiteral(arg, new MessageAndArguments("", new ArrayList<>())); + concatenationArgs.addAll(literalAndArgs.arguments); + } else if (index == m.getArguments().size() - 1 && + TypeUtils.isAssignableTo("java.lang.Throwable", arg.getType())) { + possibleThrowable = arg; + } else { + regularArgs.add(arg); + } + } + + // Build the message template ListUtils.map(m.getArguments(), (index, message) -> { if (index > 0) { messageBuilder.append(", "); @@ -97,13 +120,19 @@ public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, Execu messageBuilder.append(literalAndArgs.message); messageBuilder.append("\""); literalAndArgs.arguments.forEach(arg -> messageBuilder.append(", #{any()}")); - newArgList.addAll(literalAndArgs.arguments); } else { messageBuilder.append("#{any()}"); - newArgList.add(message); } return message; }); + + // Assemble arguments in correct order: regular args, concatenation args, throwable (if any) + newArgList.addAll(regularArgs); + newArgList.addAll(concatenationArgs); + if (possibleThrowable != null) { + newArgList.add(possibleThrowable); + } + m = JavaTemplate.builder(escapeDollarSign(messageBuilder.toString())) .build() .apply(new Cursor(getCursor().getParent(), m), m.getCoordinates().replaceArguments(), newArgList.toArray()); diff --git a/src/test/java/org/openrewrite/java/logging/ParameterizedLoggingTest.java b/src/test/java/org/openrewrite/java/logging/ParameterizedLoggingTest.java index 8f838951..84d2ef73 100644 --- a/src/test/java/org/openrewrite/java/logging/ParameterizedLoggingTest.java +++ b/src/test/java/org/openrewrite/java/logging/ParameterizedLoggingTest.java @@ -864,6 +864,95 @@ static void method(String name) { ); } + @Issue("https://github.com/openrewrite/rewrite-logging-frameworks/issues/236") + @Test + void preserveArgumentOrderWhenMixingConcatenationAndParameters() { + rewriteRun( + spec -> spec.recipe(new ParameterizedLogging("org.slf4j.Logger info(..)", false)), + //language=java + java( + """ + import org.slf4j.Logger; + + class Test { + static void method(Logger logger, String esHost, String[] indexPrefix) { + logger.info("Some logging {} with string concatenation: " + String.join(",", indexPrefix), esHost); + } + } + """, + """ + import org.slf4j.Logger; + + class Test { + static void method(Logger logger, String esHost, String[] indexPrefix) { + logger.info("Some logging {} with string concatenation: {}", esHost, String.join(",", indexPrefix)); + } + } + """ + ) + ); + } + + @Issue("https://github.com/openrewrite/rewrite-logging-frameworks/issues/236") + @Test + void preserveArgumentOrderWithMarkerAndMixedConcatenation() { + rewriteRun( + spec -> spec.recipe(new ParameterizedLogging("org.slf4j.Logger info(..)", false)), + //language=java + java( + """ + import org.slf4j.Logger; + import org.slf4j.Marker; + + class Test { + static void method(Logger logger, Marker marker, String param1, String param2) { + logger.info(marker, "Message with {} and concat: " + param2, param1); + } + } + """, + """ + import org.slf4j.Logger; + import org.slf4j.Marker; + + class Test { + static void method(Logger logger, Marker marker, String param1, String param2) { + logger.info(marker, "Message with {} and concat: {}", param1, param2); + } + } + """ + ) + ); + } + + @Issue("https://github.com/openrewrite/rewrite-logging-frameworks/issues/236") + @Test + void preserveArgumentOrderWithMultipleConcatenations() { + rewriteRun( + spec -> spec.recipe(new ParameterizedLogging("org.slf4j.Logger info(..)", false)), + //language=java + java( + """ + import org.slf4j.Logger; + + class Test { + static void method(Logger logger, String existing, String part1, String part2) { + logger.info("Existing param {} with concat: " + part1 + " and " + part2, existing); + } + } + """, + """ + import org.slf4j.Logger; + + class Test { + static void method(Logger logger, String existing, String part1, String part2) { + logger.info("Existing param {} with concat: {} and {}", existing, part1, part2); + } + } + """ + ) + ); + } + @Test void returnsAnonymousClassThatUsesLoggerInMethodImplementation() { rewriteRun(