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.

rewrite-logging-frameworks/src/main/java/org/openrewrite/java/logging/slf4j/CompleteExceptionLogging.java

Lines 98 to 102 in 53f8787

	boolean isLastParameterAnException = lastParameter instanceof J.Identifier &&
	TypeUtils.isAssignableTo("java.lang.Throwable", lastParameter.getType());
	if (isLastParameterAnException) {
	return method;
	}

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.

rewrite-logging-frameworks/src/main/java/org/openrewrite/java/logging/slf4j/CompleteExceptionLogging.java

Lines 161 to 169 in 53f8787

	private static int countPlaceholders(String message) {
	int count = 0;
	Pattern pattern = Pattern.compile("\\{}");
	Matcher matcher = pattern.matcher(message);
	while (matcher.find()) {
	count++;
	}
	return count;
	}

[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:

rewrite-logging-frameworks/src/test/java/org/openrewrite/java/logging/slf4j/CompleteExceptionLoggingTest.java

Lines 96 to 118 in f5c66f7

	"""
	import org.slf4j.Logger;
	import org.slf4j.LoggerFactory;
	class A {
	private final static Logger LOG = LoggerFactory.getLogger(A.class);
	void produceException() {
	throw new RuntimeException("");
	}
	void method() {
	try {
	produceException();
	} catch (Exception e) {
	// #1, String contains format specifiers, add `e` as follows.
	LOG.error("Error message : {}", e.getMessage());
	// #2, Multiple placeholders, add `e` as follows.
	LOG.error("Error message : {} {} {}", 1, 2, e.getMessage());
	}
	}
	}
	""",

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.