JUL->slf4j parametrized recipe fails if placeholders doesn't match number of arguments

woj-tek commented 1 month ago

Followup to: https://github.com/openrewrite/rewrite-logging-frameworks/issues/155#issuecomment-2216788376

What version of OpenRewrite are you using?

I am using

mvn -U org.openrewrite.maven:rewrite-maven-plugin:run \
-Drewrite.recipeArtifactCoordinates=org.openrewrite.recipe:rewrite-logging-frameworks:2.11.0-SNAPSHOT \
-Drewrite.activeRecipes=org.openrewrite.java.logging.slf4j.JulToSlf4j

…

$ mvn --version
Apache Maven 3.9.8 (36645f6c9b5079805ea5009217e36f2cffd34256)
Java version: 22.0.1, vendor: Eclipse Adoptium, runtime: /Library/Java/JavaVirtualMachines/temurin-22.jdk/Contents/Home
Default locale: en_PL, platform encoding: UTF-8
OS name: "mac os x", version: "14.5", arch: "aarch64", family: "mac"

What is the smallest, simplest way to reproduce the problem?

java.util.Logger.log(Level.FINEST, "Elements for {0} are {1}", new Object[]{Arrays.asList(a)});

What did you expect to see?

Not sure to be honest - this looks like invalid call but it's not sanitised at any point...

Usually with JUL, if parameter is missing then the formatter simply doesn't do anything with positional argument, i.e. above would result in String:

Elements for ["item1", "item2"] are {1}"

What did you see instead?

Not sure :-)

What is the full stack trace of any errors you encountered?

09:17:59,492 [ERROR] Failed to execute goal org.openrewrite.maven:rewrite-maven-plugin:5.35.0:run (default-cli) on project: Execution default-cli of goal org.openrewrite.maven:rewrite-maven-plugin:5.35.0:run failed: Error while visiting src/main/java/<…>.java: java.lang.IndexOutOfBoundsException: Index 1 out of bounds for length 1
09:17:59,492 [ERROR]   java.base/jdk.internal.util.Preconditions.outOfBounds(Preconditions.java:100)
09:17:59,492 [ERROR]   java.base/jdk.internal.util.Preconditions.outOfBoundsCheckIndex(Preconditions.java:106)
09:17:59,492 [ERROR]   java.base/jdk.internal.util.Preconditions.checkIndex(Preconditions.java:302)
09:17:59,492 [ERROR]   java.base/java.util.Objects.checkIndex(Objects.java:365)
09:17:59,492 [ERROR]   java.base/java.util.ArrayList.get(ArrayList.java:428)
09:17:59,492 [ERROR]   org.openrewrite.java.logging.slf4j.JulParameterizedArguments$JulParameterizedToSlf4jVisitor.lambda$visitMethodInvocation$0(JulParameterizedArguments.java:116)
09:17:59,492 [ERROR]   java.base/java.util.ArrayList.forEach(ArrayList.java:1597)
09:17:59,492 [ERROR]   org.openrewrite.java.logging.slf4j.JulParameterizedArguments$JulParameterizedToSlf4jVisitor.visitMethodInvocation(JulParameterizedArguments.java:116)
09:17:59,492 [ERROR]   org.openrewrite.java.logging.slf4j.JulParameterizedArguments$JulParameterizedToSlf4jVisitor.visitMethodInvocation(JulParameterizedArguments.java:59)
09:17:59,492 [ERROR]   org.openrewrite.java.tree.J$MethodInvocation.acceptJava(J.java:3945)
09:17:59,492 [ERROR]   org.openrewrite.java.tree.J.accept(J.java:59)
09:17:59,493 [ERROR]   org.openrewrite.TreeVisitor.visit(TreeVisitor.java:250)
09:17:59,493 [ERROR]   org.openrewrite.TreeVisitor.visitAndCast(TreeVisitor.java:324)
09:17:59,493 [ERROR]   org.openrewrite.java.JavaVisitor.visitRightPadded(JavaVisitor.java:1369)
09:17:59,493 [ERROR]   org.openrewrite.java.JavaVisitor.lambda$visitBlock$4(JavaVisitor.java:401)
09:17:59,493 [ERROR]   org.openrewrite.internal.ListUtils.map(ListUtils.java:176)

Are you interested in contributing a fix to OpenRewrite?

timtebeek commented 1 month ago

Thanks for logging an issue! Indeed hard to say what to do when there's no clear best way to go about it.

We could prevent the IndexOutOfBoundsException with a quick check, and if there's no argument insert a literal {i} just to keep the effect the same. What are your thoughts on that?

Then we can still defer and write a recipe later that cleans that up either before or after conversion, but at least don't blow up on larger migrations where the odds increase of such problems existing in the project.

woj-tek commented 1 month ago

We could prevent the IndexOutOfBoundsException with a quick check, and if there's no argument insert a literal {i} just to keep the effect the same. What are your thoughts on that?

Make sense.

Though I'd log error just as well to inform user that the statement is wrong.

On the other hand converting it as is and leaving exact number of parameters, which would yield warning in any IDE, would also be convenient to get them squash.

The more I ponder it the more I'm leaning towards the latter :-) The former would mostly just keep hiding the issue.

timtebeek commented 1 month ago

Let's opt for the IDE warnings first then; should be doable. Logging isn't really an option given how many different ways there are to run recipes.

openrewrite / rewrite-logging-frameworks