Add recipe to clone an exception argument

[ppkarwasz]

What problem are you trying to solve?

When logging exceptions using SLF4J or Log4j API, a common mistake is to add a placeholder for the exception in the message format:

try {
    ...
} catch (Exception e) {
    log.error("An error occurred: {}", e);
}

Describe the solution you'd like

Currently there is a CompleteExceptionLogging rewrite rule that deals with the case:

try {
    ...
} catch (Exception e) {
    log.error("An error occurred: {}", e.getMessage());
}

This rule could be expanded to rewrite the first example into either:

try {
    ...
} catch (Exception e) {
    log.error("An error occurred:", e);
}

or

try {
    ...
} catch (Exception e) {
    log.error("An error occurred: {}", e.getMessage(), e);
}

Have you considered any alternatives or workarounds?

Palantir already offers a LoggerInterpolationConsumesThrowable Error Prone rule, that could be expanded to generate a code change suggestion.

Are you interested in contributing this feature to OpenRewrite?

Right now I don't have time for the OpenRewrite rules already assigned to me. If this is not solved till September, I can provide a PR.

[timtebeek]

Thanks for the suggestion! Replicated with this unit test

@Test
void showStacktraceInsteadOfExceptionToString() {
    //language=java
    rewriteRun(
      java(
        """
          import org.slf4j.Logger;
          
          class A {
              void method(Logger log) {
                  try {
                      throw new RuntimeException("");
                  } catch (Exception e) {
                      log.error("An error occurred: {}", e);
                  }
              }
          }
          """,
        """
          import org.slf4j.Logger;
          
          class A {
              void method(Logger log) {
                  try {
                      throw new RuntimeException("");
                  } catch (Exception e) {
                      log.error("An error occurred: {}", e.getMessage(), e);
                  }
              }
          }
          """
      )
    );
}

That now exits the recipe early, because the last parameter is already an exception. https://github.com/openrewrite/rewrite-logging-frameworks/blob/53f87871ef81a07e03ca4e1dc0577aaf2117fea0/src/main/java/org/openrewrite/java/logging/slf4j/CompleteExceptionLogging.java#L98-L102

Instead we should probably count the number of placeholders in the logging statement early, and from there decide if we should insert a new second to last argument. https://github.com/openrewrite/rewrite-logging-frameworks/blob/53f87871ef81a07e03ca4e1dc0577aaf2117fea0/src/main/java/org/openrewrite/java/logging/slf4j/CompleteExceptionLogging.java#L161-L169

[ppkarwasz]

@timtebeek,

Instead we should probably count the number of placeholders in the logging statement early, and from there decide if we should insert a new second to last argument.

Yes, something like this. There is already a test with e.getMessage() as last parameter:

https://github.com/openrewrite/rewrite-logging-frameworks/blob/f5c66f7bd06425108cd89c64e7d5966555bde9d9/src/test/java/org/openrewrite/java/logging/slf4j/CompleteExceptionLoggingTest.java#L96-L118

It would be nice to have the same refactoring with e as last parameter.

Note: the number of placeholders is slightly more complicated to compute due to simple escaping rules:

• {} is a placeholder,
• \{} is a literal "{}",
• \\{} is a literal "" + placeholder.

[timtebeek]

Agree with you there that the current handling is perhaps too simple; and yes would be nice to have this handling here. I'll be traveling for the next five weeks, but can try to provide guidance if needed on this one.