Skip to content

Apply do no harm to ParameterizedLogging by not changing statements with exceptions #264

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 5 commits into from
Nov 18, 2025
Merged

Apply do no harm to ParameterizedLogging by not changing statements with exceptions #264

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a7437ba
Reduce casting to Object to situations that are ambiguous.
Laurens-W Nov 12, 2025
aff3f14
Polish
Laurens-W Nov 13, 2025
ce00760
Polish
Laurens-W Nov 13, 2025
3a9f791
Apply do no harm; when an exception is logged do not change anything …
Laurens-W Nov 18, 2025
f4b9cb5
Apply suggestions from code review
timtebeek Nov 18, 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
96 changes: 96 additions & 0 deletions src/main/java/org/openrewrite/java/logging/ConvertLoggingExceptionCastToToString.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,96 @@
/*
* Copyright 2025 the original author or authors.
* <p>
* 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
* <p>
* https://docs.moderne.io/licensing/moderne-source-available-license
* <p>
* 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<String> getTags() {
return new HashSet<>(Arrays.asList("Logging", "RSPEC-S1905"));
}

@Override
public TreeVisitor<?, ExecutionContext> getVisitor() {
return new JavaIsoVisitor<ExecutionContext>() {
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.equals(method) ? method : m;
}
};
}
}
8 changes: 6 additions & 2 deletions src/main/java/org/openrewrite/java/logging/ParameterizedLogging.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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 {
Expand Down
195 changes: 195 additions & 0 deletions src/test/java/org/openrewrite/java/logging/ConvertLoggingExceptionCastToToStringTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,195 @@
/*
* Copyright 2025 the original author or authors.
* <p>
* 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
* <p>
* https://docs.moderne.io/licensing/moderne-source-available-license
* <p>
* 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());
}
}
"""
)
);
}
}
72 changes: 72 additions & 0 deletions src/test/java/org/openrewrite/java/logging/ParameterizedLoggingTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -260,6 +260,78 @@ 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(
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() {
Expand Down
Loading