From a7437ba269ac58f5f0854d21cba89947b52e6eee Mon Sep 17 00:00:00 2001 From: Laurens Westerlaken Date: Wed, 12 Nov 2025 18:53:10 +0100 Subject: [PATCH 1/5] Reduce casting to Object to situations that are ambiguous. Add a recipe to remediate the sonar finding in regards to casting as a result of running ParameterizedLogging --- ...ConvertLoggingExceptionCastToToString.java | 136 ++++++++++++ .../java/logging/ParameterizedLogging.java | 8 +- ...ertLoggingExceptionCastToToStringTest.java | 195 ++++++++++++++++++ .../logging/ParameterizedLoggingTest.java | 36 ++++ 4 files changed, 373 insertions(+), 2 deletions(-) create mode 100644 src/main/java/org/openrewrite/java/logging/ConvertLoggingExceptionCastToToString.java create mode 100644 src/test/java/org/openrewrite/java/logging/ConvertLoggingExceptionCastToToStringTest.java diff --git a/src/main/java/org/openrewrite/java/logging/ConvertLoggingExceptionCastToToString.java b/src/main/java/org/openrewrite/java/logging/ConvertLoggingExceptionCastToToString.java new file mode 100644 index 00000000..bad2d1cf --- /dev/null +++ b/src/main/java/org/openrewrite/java/logging/ConvertLoggingExceptionCastToToString.java @@ -0,0 +1,136 @@ +/* + * Copyright 2024 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; + +import lombok.EqualsAndHashCode; +import lombok.Value; +import org.openrewrite.Cursor; +import org.openrewrite.ExecutionContext; +import org.openrewrite.Option; +import org.openrewrite.Recipe; +import org.openrewrite.TreeVisitor; +import org.openrewrite.java.JavaIsoVisitor; +import org.openrewrite.java.JavaTemplate; +import org.openrewrite.java.MethodMatcher; +import org.openrewrite.java.tree.J; +import org.openrewrite.java.tree.TypeUtils; + +import java.util.Arrays; +import java.util.Collections; +import java.util.HashSet; +import java.util.Set; + +@EqualsAndHashCode(callSuper = false) +@Value +public class ConvertLoggingExceptionCastToToString extends Recipe { + + @Option(displayName = "Method pattern", + description = "A method pattern to find matching logging statements to update.", + example = "org.slf4j.Logger debug(..)") + String methodPattern; + + @Override + public String getDisplayName() { + return "Convert Logging exception cast to toString() call"; + } + + @Override + public String getDescription() { + //language=markdown + return "Converts `(Object) exception` casts in logging statements to `exception.toString()` calls. " + + "This is more explicit about the intent to log the string representation of the exception " + + "rather than relying on implicit toString() conversion through Object casting."; + } + + @Override + public Set getTags() { + return new HashSet<>(Arrays.asList("Logging", "RSPEC-S1905")); + } + + @Override + public TreeVisitor getVisitor() { + return new JavaIsoVisitor() { + private final MethodMatcher methodMatcher = new MethodMatcher(methodPattern, true); + + @Override + public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, ExecutionContext ctx) { + J.MethodInvocation m = super.visitMethodInvocation(method, ctx); + + // Check if this matches our target method pattern + if (!methodMatcher.matches(m)) { + return m; + } + + // Check if there are any Object casts of Throwables to replace + boolean hasObjectCastsToReplace = m.getArguments().stream().anyMatch(arg -> { + if (arg instanceof J.TypeCast) { + J.TypeCast cast = (J.TypeCast) arg; + return cast.getType() != null && + TypeUtils.isOfClassType(cast.getType(), "java.lang.Object") && + cast.getExpression() != null && + TypeUtils.isAssignableTo("java.lang.Throwable", cast.getExpression().getType()); + } + return false; + }); + + // If there are no casts to replace, return the method as-is + if (!hasObjectCastsToReplace) { + return m; + } + + // Use a JavaTemplate to properly replace the arguments + JavaTemplate template = JavaTemplate.builder(m.getArguments().stream() + .map(arg -> { + if (arg instanceof J.TypeCast) { + J.TypeCast cast = (J.TypeCast) arg; + if (cast.getType() != null && + TypeUtils.isOfClassType(cast.getType(), "java.lang.Object") && + cast.getExpression() != null && + TypeUtils.isAssignableTo("java.lang.Throwable", cast.getExpression().getType())) { + return "#{any(java.lang.Throwable)}.toString()"; + } + } + return "#{any()}"; + }) + .reduce((a, b) -> a + ", " + b) + .orElse("")) + .build(); + + // Prepare the arguments for the template + Object[] templateArgs = m.getArguments().stream() + .map(arg -> { + if (arg instanceof J.TypeCast) { + J.TypeCast cast = (J.TypeCast) arg; + if (cast.getType() != null && + TypeUtils.isOfClassType(cast.getType(), "java.lang.Object") && + cast.getExpression() != null && + TypeUtils.isAssignableTo("java.lang.Throwable", cast.getExpression().getType())) { + return cast.getExpression(); + } + } + return arg; + }) + .toArray(); + + // Apply the template + return (J.MethodInvocation) template.apply( + new Cursor(getCursor().getParent(), m), + m.getCoordinates().replaceArguments(), + templateArgs); + } + }; + } +} diff --git a/src/main/java/org/openrewrite/java/logging/ParameterizedLogging.java b/src/main/java/org/openrewrite/java/logging/ParameterizedLogging.java index 1a679e28..c03d10f4 100644 --- a/src/main/java/org/openrewrite/java/logging/ParameterizedLogging.java +++ b/src/main/java/org/openrewrite/java/logging/ParameterizedLogging.java @@ -110,6 +110,9 @@ public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, Execu } } + // Determine if we should cast throwable to Object (only when it's the sole argument) + final boolean shouldCastThrowable = regularArgs.isEmpty() && possibleThrowable == null; + // Build the message template ListUtils.map(m.getArguments(), (index, message) -> { if (index > 0) { @@ -120,9 +123,10 @@ public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, Execu MessageAndArguments literalAndArgs = concatenationToLiteral(message, new MessageAndArguments("", new ArrayList<>())); messageBuilder.append(literalAndArgs.message); messageBuilder.append("\""); - // Cast Throwables to Object to preserve toString() behavior + // Cast Throwables to Object to preserve toString() behavior, but only when it's the sole argument literalAndArgs.arguments.forEach(arg -> messageBuilder.append( - TypeUtils.isAssignableTo("java.lang.Throwable", arg.getType()) ? + TypeUtils.isAssignableTo("java.lang.Throwable", arg.getType()) && + literalAndArgs.arguments.size() == 1 && shouldCastThrowable ? ", (Object) #{any()}" : ", #{any()}")); } else { diff --git a/src/test/java/org/openrewrite/java/logging/ConvertLoggingExceptionCastToToStringTest.java b/src/test/java/org/openrewrite/java/logging/ConvertLoggingExceptionCastToToStringTest.java new file mode 100644 index 00000000..0fd72c3b --- /dev/null +++ b/src/test/java/org/openrewrite/java/logging/ConvertLoggingExceptionCastToToStringTest.java @@ -0,0 +1,195 @@ +/* + * Copyright 2024 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; + +import org.junit.jupiter.api.Test; +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 ConvertLoggingExceptionCastToToStringTest implements RewriteTest { + + @Override + public void defaults(RecipeSpec spec) { + spec.parser(JavaParser.fromJavaVersion() + .classpathFromResources(new InMemoryExecutionContext(), "slf4j-api-2.1.+", "log4j-api-2.+", "log4j-core-2.+")); + } + + @DocumentExample + @Test + void convertThrowableCastToToString() { + rewriteRun( + spec -> spec.recipe(new ConvertLoggingExceptionCastToToString("org.slf4j.Logger debug(..)")), + //language=java + java( + """ + import org.slf4j.Logger; + + class Test { + static void asInteger(Logger logger, String numberString) { + try { + Integer i = Integer.valueOf(numberString); + } catch (NumberFormatException ex) { + logger.debug("some big error: {}", (Object) ex); + } + } + } + """, + """ + import org.slf4j.Logger; + + class Test { + static void asInteger(Logger logger, String numberString) { + try { + Integer i = Integer.valueOf(numberString); + } catch (NumberFormatException ex) { + logger.debug("some big error: {}", ex.toString()); + } + } + } + """ + ) + ); + } + + @Test + void convertMultipleThrowableCasts() { + rewriteRun( + spec -> spec.recipe(new ConvertLoggingExceptionCastToToString("org.slf4j.Logger info(..)")), + //language=java + java( + """ + import org.slf4j.Logger; + + class Test { + static void method(Logger logger, Exception e1, RuntimeException e2) { + logger.info("Errors: {} and {}", (Object) e1, (Object) e2); + } + } + """, + """ + import org.slf4j.Logger; + + class Test { + static void method(Logger logger, Exception e1, RuntimeException e2) { + logger.info("Errors: {} and {}", e1.toString(), e2.toString()); + } + } + """ + ) + ); + } + + @Test + void doNotChangeNonThrowableCasts() { + rewriteRun( + spec -> spec.recipe(new ConvertLoggingExceptionCastToToString("org.slf4j.Logger info(..)")), + //language=java + java( + """ + import org.slf4j.Logger; + + class Test { + static void method(Logger logger, String str) { + logger.info("Value: {}", (Object) str); + } + } + """ + ) + ); + } + + @Test + void doNotChangeNonObjectCasts() { + rewriteRun( + spec -> spec.recipe(new ConvertLoggingExceptionCastToToString("org.slf4j.Logger info(..)")), + //language=java + java( + """ + import org.slf4j.Logger; + + class Test { + static void method(Logger logger, Exception ex) { + logger.info("Error: {}", (Throwable) ex); + } + } + """ + ) + ); + } + + @Test + void workWithMarkers() { + rewriteRun( + spec -> spec.recipe(new ConvertLoggingExceptionCastToToString("org.slf4j.Logger info(..)")), + //language=java + java( + """ + import org.slf4j.Logger; + import org.slf4j.Marker; + + class Test { + static void method(Logger logger, Marker marker, Exception ex) { + logger.info(marker, "Error occurred: {}", (Object) ex); + } + } + """, + """ + import org.slf4j.Logger; + import org.slf4j.Marker; + + class Test { + static void method(Logger logger, Marker marker, Exception ex) { + logger.info(marker, "Error occurred: {}", ex.toString()); + } + } + """ + ) + ); + } + + @Test + void workWithLog4j() { + rewriteRun( + spec -> spec.recipe(new ConvertLoggingExceptionCastToToString("org.apache.logging.log4j.Logger error(..)")), + //language=java + java( + """ + import org.apache.logging.log4j.Logger; + + class Test { + static void method(Logger logger, Exception ex) { + logger.error("Failed: {}", (Object) ex); + } + } + """, + """ + import org.apache.logging.log4j.Logger; + + class Test { + static void method(Logger logger, Exception ex) { + logger.error("Failed: {}", ex.toString()); + } + } + """ + ) + ); + } +} diff --git a/src/test/java/org/openrewrite/java/logging/ParameterizedLoggingTest.java b/src/test/java/org/openrewrite/java/logging/ParameterizedLoggingTest.java index 240a755d..3dd65124 100644 --- a/src/test/java/org/openrewrite/java/logging/ParameterizedLoggingTest.java +++ b/src/test/java/org/openrewrite/java/logging/ParameterizedLoggingTest.java @@ -260,6 +260,42 @@ static void asInteger(Logger logger, String numberString) { ); } + @Test + void exceptionArgumentsWithOtherParametersNoCast() { + rewriteRun( + spec -> spec.recipe(new ParameterizedLogging("org.slf4j.Logger debug(..)", false)), + //language=java + java( + """ + import org.slf4j.Logger; + + class Test { + static void asInteger(Logger logger, String numberString, String context) { + try { + Integer i = Integer.valueOf(numberString); + } catch (NumberFormatException ex) { + logger.debug("Error in context " + context + ": " + ex); + } + } + } + """, + """ + import org.slf4j.Logger; + + class Test { + static void asInteger(Logger logger, String numberString, String context) { + try { + Integer i = Integer.valueOf(numberString); + } catch (NumberFormatException ex) { + logger.debug("Error in context {}: {}", context, ex); + } + } + } + """ + ) + ); + } + @Issue("https://github.com/openrewrite/rewrite-logging-frameworks/issues/38") @Test void indexOutOfBoundsExceptionOnParseMethodArguments() { From aff3f1424c6dfacfa585d008dd615aa49e491eb2 Mon Sep 17 00:00:00 2001 From: Laurens Westerlaken Date: Thu, 13 Nov 2025 13:28:40 +0100 Subject: [PATCH 2/5] Polish --- ...ConvertLoggingExceptionCastToToString.java | 80 +++++-------------- ...ertLoggingExceptionCastToToStringTest.java | 2 +- .../logging/ParameterizedLoggingTest.java | 36 +++++++++ 3 files changed, 57 insertions(+), 61 deletions(-) diff --git a/src/main/java/org/openrewrite/java/logging/ConvertLoggingExceptionCastToToString.java b/src/main/java/org/openrewrite/java/logging/ConvertLoggingExceptionCastToToString.java index bad2d1cf..41d5cf53 100644 --- a/src/main/java/org/openrewrite/java/logging/ConvertLoggingExceptionCastToToString.java +++ b/src/main/java/org/openrewrite/java/logging/ConvertLoggingExceptionCastToToString.java @@ -1,5 +1,5 @@ /* - * Copyright 2024 the original author or authors. + * 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. @@ -17,11 +17,8 @@ import lombok.EqualsAndHashCode; import lombok.Value; -import org.openrewrite.Cursor; -import org.openrewrite.ExecutionContext; -import org.openrewrite.Option; -import org.openrewrite.Recipe; -import org.openrewrite.TreeVisitor; +import org.openrewrite.*; +import org.openrewrite.internal.ListUtils; import org.openrewrite.java.JavaIsoVisitor; import org.openrewrite.java.JavaTemplate; import org.openrewrite.java.MethodMatcher; @@ -29,7 +26,6 @@ import org.openrewrite.java.tree.TypeUtils; import java.util.Arrays; -import java.util.Collections; import java.util.HashSet; import java.util.Set; @@ -52,7 +48,8 @@ public String getDescription() { //language=markdown return "Converts `(Object) exception` casts in logging statements to `exception.toString()` calls. " + "This is more explicit about the intent to log the string representation of the exception " + - "rather than relying on implicit toString() conversion through Object casting."; + "rather than relying on implicit toString() conversion through Object casting." + + "Run this after ParameterizedLogging is applied to reduce RSPEC-S1905 findings."; } @Override @@ -74,62 +71,25 @@ public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, Execu return m; } - // Check if there are any Object casts of Throwables to replace - boolean hasObjectCastsToReplace = m.getArguments().stream().anyMatch(arg -> { + JavaTemplate toStringTemplate = JavaTemplate.builder("#{any(java.lang.Throwable)}.toString()") + .build(); + + m = m.withArguments(ListUtils.map(m.getArguments(), arg -> { if (arg instanceof J.TypeCast) { J.TypeCast cast = (J.TypeCast) arg; - return cast.getType() != null && - TypeUtils.isOfClassType(cast.getType(), "java.lang.Object") && - cast.getExpression() != null && - TypeUtils.isAssignableTo("java.lang.Throwable", cast.getExpression().getType()); + if (cast.getType() != null && + TypeUtils.isOfClassType(cast.getType(), "java.lang.Object") && + TypeUtils.isAssignableTo("java.lang.Throwable", cast.getExpression().getType())) { + return toStringTemplate.apply( + new Cursor(getCursor(), arg), + arg.getCoordinates().replace(), + cast.getExpression()); + } } - return false; - }); - - // If there are no casts to replace, return the method as-is - if (!hasObjectCastsToReplace) { - return m; - } - - // Use a JavaTemplate to properly replace the arguments - JavaTemplate template = JavaTemplate.builder(m.getArguments().stream() - .map(arg -> { - if (arg instanceof J.TypeCast) { - J.TypeCast cast = (J.TypeCast) arg; - if (cast.getType() != null && - TypeUtils.isOfClassType(cast.getType(), "java.lang.Object") && - cast.getExpression() != null && - TypeUtils.isAssignableTo("java.lang.Throwable", cast.getExpression().getType())) { - return "#{any(java.lang.Throwable)}.toString()"; - } - } - return "#{any()}"; - }) - .reduce((a, b) -> a + ", " + b) - .orElse("")) - .build(); - - // Prepare the arguments for the template - Object[] templateArgs = m.getArguments().stream() - .map(arg -> { - if (arg instanceof J.TypeCast) { - J.TypeCast cast = (J.TypeCast) arg; - if (cast.getType() != null && - TypeUtils.isOfClassType(cast.getType(), "java.lang.Object") && - cast.getExpression() != null && - TypeUtils.isAssignableTo("java.lang.Throwable", cast.getExpression().getType())) { - return cast.getExpression(); - } - } - return arg; - }) - .toArray(); + return arg; + })); - // Apply the template - return (J.MethodInvocation) template.apply( - new Cursor(getCursor().getParent(), m), - m.getCoordinates().replaceArguments(), - templateArgs); + return m.equals(method) ? method : m; } }; } diff --git a/src/test/java/org/openrewrite/java/logging/ConvertLoggingExceptionCastToToStringTest.java b/src/test/java/org/openrewrite/java/logging/ConvertLoggingExceptionCastToToStringTest.java index 0fd72c3b..c1587898 100644 --- a/src/test/java/org/openrewrite/java/logging/ConvertLoggingExceptionCastToToStringTest.java +++ b/src/test/java/org/openrewrite/java/logging/ConvertLoggingExceptionCastToToStringTest.java @@ -1,5 +1,5 @@ /* - * Copyright 2024 the original author or authors. + * 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. diff --git a/src/test/java/org/openrewrite/java/logging/ParameterizedLoggingTest.java b/src/test/java/org/openrewrite/java/logging/ParameterizedLoggingTest.java index 3dd65124..005b90eb 100644 --- a/src/test/java/org/openrewrite/java/logging/ParameterizedLoggingTest.java +++ b/src/test/java/org/openrewrite/java/logging/ParameterizedLoggingTest.java @@ -260,6 +260,42 @@ static void asInteger(Logger logger, String numberString) { ); } + @Test + void exceptionArgumentsWithMultiCatch() { + rewriteRun( + spec -> spec.recipe(new ParameterizedLogging("org.slf4j.Logger debug(..)", false)), + //language=java + java( + """ + import org.slf4j.Logger; + + class Test { + static void parseValue(Logger logger, String numberString) { + try { + Integer i = Integer.valueOf(numberString); + } catch (NumberFormatException | IllegalArgumentException ex) { + logger.debug("parsing error: " + ex); + } + } + } + """, + """ + import org.slf4j.Logger; + + class Test { + static void parseValue(Logger logger, String numberString) { + try { + Integer i = Integer.valueOf(numberString); + } catch (NumberFormatException | IllegalArgumentException ex) { + logger.debug("parsing error: {}", (Object) ex); + } + } + } + """ + ) + ); + } + @Test void exceptionArgumentsWithOtherParametersNoCast() { rewriteRun( From ce00760e3e9ac4559e33b4ace40895cfb82e0957 Mon Sep 17 00:00:00 2001 From: Laurens Westerlaken Date: Thu, 13 Nov 2025 13:41:52 +0100 Subject: [PATCH 3/5] Polish --- .../java/logging/ConvertLoggingExceptionCastToToString.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/main/java/org/openrewrite/java/logging/ConvertLoggingExceptionCastToToString.java b/src/main/java/org/openrewrite/java/logging/ConvertLoggingExceptionCastToToString.java index 41d5cf53..f81f9756 100644 --- a/src/main/java/org/openrewrite/java/logging/ConvertLoggingExceptionCastToToString.java +++ b/src/main/java/org/openrewrite/java/logging/ConvertLoggingExceptionCastToToString.java @@ -48,7 +48,7 @@ public String getDescription() { //language=markdown return "Converts `(Object) exception` casts in logging statements to `exception.toString()` calls. " + "This is more explicit about the intent to log the string representation of the exception " + - "rather than relying on implicit toString() conversion through Object casting." + + "rather than relying on implicit toString() conversion through Object casting. " + "Run this after ParameterizedLogging is applied to reduce RSPEC-S1905 findings."; } @@ -89,7 +89,7 @@ public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, Execu return arg; })); - return m.equals(method) ? method : m; + return m.getArguments().equals(method.getArguments()) ? method : m; } }; } From 3a9f79161bbc6db473e6a1cbcb4592cef0ed2b6d Mon Sep 17 00:00:00 2001 From: Laurens Westerlaken Date: Tue, 18 Nov 2025 13:18:21 +0100 Subject: [PATCH 4/5] Apply do no harm; when an exception is logged do not change anything as this may cause changes in behavior --- ...ConvertLoggingExceptionCastToToString.java | 96 --------- .../java/logging/ParameterizedLogging.java | 18 +- ...ertLoggingExceptionCastToToStringTest.java | 195 ------------------ .../logging/ParameterizedLoggingTest.java | 39 ---- .../logging/slf4j/Slf4jBestPracticesTest.java | 2 +- 5 files changed, 11 insertions(+), 339 deletions(-) delete mode 100644 src/main/java/org/openrewrite/java/logging/ConvertLoggingExceptionCastToToString.java delete mode 100644 src/test/java/org/openrewrite/java/logging/ConvertLoggingExceptionCastToToStringTest.java diff --git a/src/main/java/org/openrewrite/java/logging/ConvertLoggingExceptionCastToToString.java b/src/main/java/org/openrewrite/java/logging/ConvertLoggingExceptionCastToToString.java deleted file mode 100644 index f81f9756..00000000 --- a/src/main/java/org/openrewrite/java/logging/ConvertLoggingExceptionCastToToString.java +++ /dev/null @@ -1,96 +0,0 @@ -/* - * 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; - -import lombok.EqualsAndHashCode; -import lombok.Value; -import org.openrewrite.*; -import org.openrewrite.internal.ListUtils; -import org.openrewrite.java.JavaIsoVisitor; -import org.openrewrite.java.JavaTemplate; -import org.openrewrite.java.MethodMatcher; -import org.openrewrite.java.tree.J; -import org.openrewrite.java.tree.TypeUtils; - -import java.util.Arrays; -import java.util.HashSet; -import java.util.Set; - -@EqualsAndHashCode(callSuper = false) -@Value -public class ConvertLoggingExceptionCastToToString extends Recipe { - - @Option(displayName = "Method pattern", - description = "A method pattern to find matching logging statements to update.", - example = "org.slf4j.Logger debug(..)") - String methodPattern; - - @Override - public String getDisplayName() { - return "Convert Logging exception cast to toString() call"; - } - - @Override - public String getDescription() { - //language=markdown - return "Converts `(Object) exception` casts in logging statements to `exception.toString()` calls. " + - "This is more explicit about the intent to log the string representation of the exception " + - "rather than relying on implicit toString() conversion through Object casting. " + - "Run this after ParameterizedLogging is applied to reduce RSPEC-S1905 findings."; - } - - @Override - public Set getTags() { - return new HashSet<>(Arrays.asList("Logging", "RSPEC-S1905")); - } - - @Override - public TreeVisitor getVisitor() { - return new JavaIsoVisitor() { - private final MethodMatcher methodMatcher = new MethodMatcher(methodPattern, true); - - @Override - public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, ExecutionContext ctx) { - J.MethodInvocation m = super.visitMethodInvocation(method, ctx); - - // Check if this matches our target method pattern - if (!methodMatcher.matches(m)) { - return m; - } - - JavaTemplate toStringTemplate = JavaTemplate.builder("#{any(java.lang.Throwable)}.toString()") - .build(); - - m = m.withArguments(ListUtils.map(m.getArguments(), arg -> { - if (arg instanceof J.TypeCast) { - J.TypeCast cast = (J.TypeCast) arg; - if (cast.getType() != null && - TypeUtils.isOfClassType(cast.getType(), "java.lang.Object") && - TypeUtils.isAssignableTo("java.lang.Throwable", cast.getExpression().getType())) { - return toStringTemplate.apply( - new Cursor(getCursor(), arg), - arg.getCoordinates().replace(), - cast.getExpression()); - } - } - return arg; - })); - - return m.getArguments().equals(method.getArguments()) ? method : m; - } - }; - } -} diff --git a/src/main/java/org/openrewrite/java/logging/ParameterizedLogging.java b/src/main/java/org/openrewrite/java/logging/ParameterizedLogging.java index c03d10f4..c7c64b9a 100644 --- a/src/main/java/org/openrewrite/java/logging/ParameterizedLogging.java +++ b/src/main/java/org/openrewrite/java/logging/ParameterizedLogging.java @@ -110,8 +110,14 @@ public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, Execu } } - // Determine if we should cast throwable to Object (only when it's the sole argument) - final boolean shouldCastThrowable = regularArgs.isEmpty() && possibleThrowable == null; + // Check if any of the concatenation arguments is a throwable + // If so, skip parameterization to preserve exception handling behavior + boolean hasThrowableInConcatenation = concatenationArgs.stream() + .anyMatch(arg -> TypeUtils.isAssignableTo("java.lang.Throwable", arg.getType())); + + if (hasThrowableInConcatenation) { + return m; // Skip parameterization when throwables are concatenated + } // Build the message template ListUtils.map(m.getArguments(), (index, message) -> { @@ -123,12 +129,8 @@ public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, Execu MessageAndArguments literalAndArgs = concatenationToLiteral(message, new MessageAndArguments("", new ArrayList<>())); messageBuilder.append(literalAndArgs.message); messageBuilder.append("\""); - // Cast Throwables to Object to preserve toString() behavior, but only when it's the sole argument - literalAndArgs.arguments.forEach(arg -> messageBuilder.append( - TypeUtils.isAssignableTo("java.lang.Throwable", arg.getType()) && - literalAndArgs.arguments.size() == 1 && shouldCastThrowable ? - ", (Object) #{any()}" : - ", #{any()}")); + // Add arguments without casting throwables + literalAndArgs.arguments.forEach(arg -> messageBuilder.append(", #{any()}")); } else { messageBuilder.append("#{any()}"); } diff --git a/src/test/java/org/openrewrite/java/logging/ConvertLoggingExceptionCastToToStringTest.java b/src/test/java/org/openrewrite/java/logging/ConvertLoggingExceptionCastToToStringTest.java deleted file mode 100644 index c1587898..00000000 --- a/src/test/java/org/openrewrite/java/logging/ConvertLoggingExceptionCastToToStringTest.java +++ /dev/null @@ -1,195 +0,0 @@ -/* - * 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; - -import org.junit.jupiter.api.Test; -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 ConvertLoggingExceptionCastToToStringTest implements RewriteTest { - - @Override - public void defaults(RecipeSpec spec) { - spec.parser(JavaParser.fromJavaVersion() - .classpathFromResources(new InMemoryExecutionContext(), "slf4j-api-2.1.+", "log4j-api-2.+", "log4j-core-2.+")); - } - - @DocumentExample - @Test - void convertThrowableCastToToString() { - rewriteRun( - spec -> spec.recipe(new ConvertLoggingExceptionCastToToString("org.slf4j.Logger debug(..)")), - //language=java - java( - """ - import org.slf4j.Logger; - - class Test { - static void asInteger(Logger logger, String numberString) { - try { - Integer i = Integer.valueOf(numberString); - } catch (NumberFormatException ex) { - logger.debug("some big error: {}", (Object) ex); - } - } - } - """, - """ - import org.slf4j.Logger; - - class Test { - static void asInteger(Logger logger, String numberString) { - try { - Integer i = Integer.valueOf(numberString); - } catch (NumberFormatException ex) { - logger.debug("some big error: {}", ex.toString()); - } - } - } - """ - ) - ); - } - - @Test - void convertMultipleThrowableCasts() { - rewriteRun( - spec -> spec.recipe(new ConvertLoggingExceptionCastToToString("org.slf4j.Logger info(..)")), - //language=java - java( - """ - import org.slf4j.Logger; - - class Test { - static void method(Logger logger, Exception e1, RuntimeException e2) { - logger.info("Errors: {} and {}", (Object) e1, (Object) e2); - } - } - """, - """ - import org.slf4j.Logger; - - class Test { - static void method(Logger logger, Exception e1, RuntimeException e2) { - logger.info("Errors: {} and {}", e1.toString(), e2.toString()); - } - } - """ - ) - ); - } - - @Test - void doNotChangeNonThrowableCasts() { - rewriteRun( - spec -> spec.recipe(new ConvertLoggingExceptionCastToToString("org.slf4j.Logger info(..)")), - //language=java - java( - """ - import org.slf4j.Logger; - - class Test { - static void method(Logger logger, String str) { - logger.info("Value: {}", (Object) str); - } - } - """ - ) - ); - } - - @Test - void doNotChangeNonObjectCasts() { - rewriteRun( - spec -> spec.recipe(new ConvertLoggingExceptionCastToToString("org.slf4j.Logger info(..)")), - //language=java - java( - """ - import org.slf4j.Logger; - - class Test { - static void method(Logger logger, Exception ex) { - logger.info("Error: {}", (Throwable) ex); - } - } - """ - ) - ); - } - - @Test - void workWithMarkers() { - rewriteRun( - spec -> spec.recipe(new ConvertLoggingExceptionCastToToString("org.slf4j.Logger info(..)")), - //language=java - java( - """ - import org.slf4j.Logger; - import org.slf4j.Marker; - - class Test { - static void method(Logger logger, Marker marker, Exception ex) { - logger.info(marker, "Error occurred: {}", (Object) ex); - } - } - """, - """ - import org.slf4j.Logger; - import org.slf4j.Marker; - - class Test { - static void method(Logger logger, Marker marker, Exception ex) { - logger.info(marker, "Error occurred: {}", ex.toString()); - } - } - """ - ) - ); - } - - @Test - void workWithLog4j() { - rewriteRun( - spec -> spec.recipe(new ConvertLoggingExceptionCastToToString("org.apache.logging.log4j.Logger error(..)")), - //language=java - java( - """ - import org.apache.logging.log4j.Logger; - - class Test { - static void method(Logger logger, Exception ex) { - logger.error("Failed: {}", (Object) ex); - } - } - """, - """ - import org.apache.logging.log4j.Logger; - - class Test { - static void method(Logger logger, Exception ex) { - logger.error("Failed: {}", ex.toString()); - } - } - """ - ) - ); - } -} diff --git a/src/test/java/org/openrewrite/java/logging/ParameterizedLoggingTest.java b/src/test/java/org/openrewrite/java/logging/ParameterizedLoggingTest.java index 005b90eb..79d9be55 100644 --- a/src/test/java/org/openrewrite/java/logging/ParameterizedLoggingTest.java +++ b/src/test/java/org/openrewrite/java/logging/ParameterizedLoggingTest.java @@ -242,19 +242,6 @@ static void asInteger(Logger logger, String numberString) { } } } - """, - """ - import org.slf4j.Logger; - - class Test { - static void asInteger(Logger logger, String numberString) { - try { - Integer i = Integer.valueOf(numberString); - } catch (NumberFormatException ex) { - logger.debug("some big error: {}", (Object) ex); - } - } - } """ ) ); @@ -278,19 +265,6 @@ static void parseValue(Logger logger, String numberString) { } } } - """, - """ - import org.slf4j.Logger; - - class Test { - static void parseValue(Logger logger, String numberString) { - try { - Integer i = Integer.valueOf(numberString); - } catch (NumberFormatException | IllegalArgumentException ex) { - logger.debug("parsing error: {}", (Object) ex); - } - } - } """ ) ); @@ -314,19 +288,6 @@ static void asInteger(Logger logger, String numberString, String context) { } } } - """, - """ - import org.slf4j.Logger; - - class Test { - static void asInteger(Logger logger, String numberString, String context) { - try { - Integer i = Integer.valueOf(numberString); - } catch (NumberFormatException ex) { - logger.debug("Error in context {}: {}", context, ex); - } - } - } """ ) ); diff --git a/src/test/java/org/openrewrite/java/logging/slf4j/Slf4jBestPracticesTest.java b/src/test/java/org/openrewrite/java/logging/slf4j/Slf4jBestPracticesTest.java index 1f4b055b..5e299b4e 100644 --- a/src/test/java/org/openrewrite/java/logging/slf4j/Slf4jBestPracticesTest.java +++ b/src/test/java/org/openrewrite/java/logging/slf4j/Slf4jBestPracticesTest.java @@ -110,7 +110,7 @@ void test() { try { throw new IllegalStateException("oops"); } catch (Exception e) { - logger.error("aaa: {}", (Object) e); + logger.error("aaa: " + e); logger.error("bbb: {}", String.valueOf(e)); logger.error("ccc: {}", e.toString()); } From f4b9cb522dd194eb28606d187cf7d3a0e50c86bb Mon Sep 17 00:00:00 2001 From: Tim te Beek Date: Tue, 18 Nov 2025 14:20:40 +0100 Subject: [PATCH 5/5] Apply suggestions from code review --- .../java/org/openrewrite/java/logging/ParameterizedLogging.java | 1 - .../org/openrewrite/java/logging/ParameterizedLoggingTest.java | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/src/main/java/org/openrewrite/java/logging/ParameterizedLogging.java b/src/main/java/org/openrewrite/java/logging/ParameterizedLogging.java index c7c64b9a..8f14f608 100644 --- a/src/main/java/org/openrewrite/java/logging/ParameterizedLogging.java +++ b/src/main/java/org/openrewrite/java/logging/ParameterizedLogging.java @@ -129,7 +129,6 @@ public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, Execu MessageAndArguments literalAndArgs = concatenationToLiteral(message, new MessageAndArguments("", new ArrayList<>())); messageBuilder.append(literalAndArgs.message); messageBuilder.append("\""); - // Add arguments without casting throwables literalAndArgs.arguments.forEach(arg -> messageBuilder.append(", #{any()}")); } else { messageBuilder.append("#{any()}"); diff --git a/src/test/java/org/openrewrite/java/logging/ParameterizedLoggingTest.java b/src/test/java/org/openrewrite/java/logging/ParameterizedLoggingTest.java index 79d9be55..4ebba3c7 100644 --- a/src/test/java/org/openrewrite/java/logging/ParameterizedLoggingTest.java +++ b/src/test/java/org/openrewrite/java/logging/ParameterizedLoggingTest.java @@ -271,7 +271,7 @@ static void parseValue(Logger logger, String numberString) { } @Test - void exceptionArgumentsWithOtherParametersNoCast() { + void exceptionArgumentsWithOtherParametersNoChange() { rewriteRun( spec -> spec.recipe(new ParameterizedLogging("org.slf4j.Logger debug(..)", false)), //language=java