Avoid unsafe changes in `JulParameterizedArguments` when `JavaType.Array` argument is not a `J.NewArray` #244

pdelagrave · 2025-07-30T13:33:34Z

What's changed?

The org.openrewrite.java.logging.slf4j.JulParameterizedArguments recipe now supports migrating JUL.log("{0}", arrayIdentifier) to SLF4J.
Before that the only supported form was: JUL.log("{0}", new String[]{"foo"}).

Checklist

I've added unit tests to cover both positive and negative cases
I've read and applied the recipe conventions and best practices
I've used the IntelliJ IDEA auto-formatter on affected files

src/main/java/org/openrewrite/java/logging/slf4j/JulParameterizedArguments.java

jevanlingen · 2025-07-30T15:38:11Z

When you are at it, move the static methods one level higher and inline the visitor. The recipe would then look like:

pdelagrave · 2025-07-30T16:08:57Z

When you are at it, move the static methods one level higher and inline the visitor. The recipe would then look like:

We can't have static methods in an anon class so the best practice recipe won't move the helper methods up anymore.
Unsure if there's benefits of having static methods in this scenario but anyway, I'm converting it the way you said, it's also quite more common to see it this way across recipes.

jevanlingen · 2025-07-31T06:25:36Z

We can't have static methods in an anonymous class, so the best practice recipe won't move the helper methods up anymore.

If you look closely at my example, I moved the static method out the visitor class into the upper (recipe) class. That will work 😄.

Unsure if there's benefits of having static methods in this scenario

Yes and no. Not from a functional standpoint, but by having these method declared static, it's easier for the programmer to spot these methods are utility methods (because they don't have dependencies to any fields).

but anyway, I'm converting it the way you said, it's also quite more common to see it this way across recipes.

Sure, that good as well!

# Conflicts: # src/main/java/org/openrewrite/java/logging/slf4j/JulParameterizedArguments.java

github-actions

Some suggestions could not be made:

src/main/java/org/openrewrite/java/logging/slf4j/JulParameterizedArguments.java
- lines 33-33

timtebeek · 2025-07-31T11:22:12Z

I have some reservations about the goal for this recipe change, as I don't think it's safe and changes the failure mode:

import java.util.logging.Level;
import java.util.logging.Logger;

class Foo {
    static Logger log = Logger.getLogger(Foo.class.getName());

    public static void main(String[] args) {
        // Ideal case
        log.log(Level.INFO, "Message with parameters {0}, {1}", new Object[]{"param0", "param1"});

        // A: Intentionally ignore the first parameter
        log.log(Level.INFO, "Message with parameters {1}, {2}", new Object[]{"param0", "param1", "param2"});

        // B: Mismatched array length when using variable
        Object[] params = {"param0"};
        log.log(Level.INFO, "Message with parameters {0}, {1}", params);

        // C: Mismatched array length when using array index throws IndexOutOfBoundsException
//        log.info("Message with parameters {}, {}", params[0], params[1]);
    }
}

Since we're looking at array size I don't think we correctly handle case A.

Worse still, the conversion from case B to C means we will throw an IndexOutOfBoundException, as opposed to silently ignoring a mismatch. That's not a safe change to make, and as such I doubt if we should make changes here at all for array variables.

timtebeek · 2025-07-31T11:28:42Z

If we can't safely make this change of unpacking arrays, then we should codify that in a test asserting no change is made, and restore the recipe functionality, possibly with the refactoring improvements retained. What are your thoughts on this?

Mostly I want to prevent this from becoming a time sink for what's ultimately not a safe change to make, as that would lose trust.

timtebeek · 2025-07-31T13:53:11Z

As discussed; closing this as it's not a safe change to make.

src/test/java/org/openrewrite/java/logging/slf4j/JulParameterizedArgumentsTest.java

github-actions

Some suggestions could not be made:

src/main/java/org/openrewrite/java/logging/slf4j/JulParameterizedArguments.java
- lines 33-33

First time testing for a negative case shows that the recipe isn't producing valid code by itself and that `org.openrewrite.java.logging.slf4j.JulToSlf4j` isn't either. Fixed in the next commit.

pdelagrave · 2025-07-31T16:47:59Z

When trying to make the updated, now negative-case version, of the arrayIdentifierArgument test pass, I realized that the org.openrewrite.java.logging.slf4j.JulToSlf4j top-level declarative recipe generates uncompilable code if any negative/unsupported migration cases are found. Its sub-recipes aren't meant to be independent and can themselves generate broken code. Some recipes assume that other recipes will fix what they didn't do themselves, but then these later recipes also apply too much change without being aware of the context.
For example:
JulParameterizedArguments converts some of the logger.log() calls parameters to make them compatible with the SLF4J logger.level() methods signature, but after this recipe is run, the logger is still a java.util.logging.Logger and the method .log() doesn't exist at all on SLF4J anyway. This is because org.openrewrite.java.logging.slf4j.JulToSlf4jSimpleCallsWithThrowable (.log(Level.INFO)->.info()) and ChangeType (java.util.logging.Logger->org.slf4j.Logger) are expected to be run afterward.

The blanket execution of the ChangeType (java.util.logging.Logger->org.slf4j.Logger) recipe is also a problem because the leftover JUL log() invocations that weren't converted to the SLF4J are not going to compile anymore.

There's no simple fix. It would be possible but relatively complicated (and often impossible due to many unsupported cases) to support a working partial migration where both the old and new logging systems are present, but I highly doubt that would satisfy most users.

Another solution would be to move the dependent (producing broken code) recipes into one large cohesive imperative recipe (that can still be divided into various modules/visitors, but not recipes), and produce a detailed report on which cases are unsupported and what need to be manually changed before the migration can be re-applied. So the migration recipe would be all-or-nothing, but the "nothing" result would still produce detailed information about why it's impossible to proceed and what exactly need to be changed.

IMO that last suggestion is a better outcome than producing a half-baked uncompilable migration without clues as why it didn't succeed. In which case the user is left with more manual work and has more to figure out themselves, whereas a guided list of points to manually fix and why before re-running the recipe is less work and more agreeable work.

The recipe code already has the conditions to avoid processing elements that can't be automatically refactored, it would be a matter of emitting an english explanation associated with the node where it happened.

# Conflicts: # src/main/java/org/openrewrite/java/logging/slf4j/JulParameterizedArguments.java

timtebeek

Indeed could not avoid all changes beyond JulParameterizedArguments, but we can make that itself safer already with the present changes. Let's merge this increment.

…rom 3.7.0 to 3.13.0 [skip ci] Bumps [org.openrewrite.recipe:rewrite-logging-frameworks](https://github.com/openrewrite/rewrite-logging-frameworks) from 3.7.0 to 3.13.0. Release notes *Sourced from [org.openrewrite.recipe:rewrite-logging-frameworks's releases](https://github.com/openrewrite/rewrite-logging-frameworks/releases).* > 3.13.0 > ------ > > What's Changed > -------------- > > * Recipe to migrate deprecated JBoss Logging calls by [`@pdelagrave`](https://github.com/pdelagrave) in [openrewrite/rewrite-logging-frameworks#245](https://redirect.github.com/openrewrite/rewrite-logging-frameworks/pull/245) > * Add `ArgumentArrayToVarargs` for logger methods that take a var args argument Object array by [`@timtebeek`](https://github.com/timtebeek) in [openrewrite/rewrite-logging-frameworks#246](https://redirect.github.com/openrewrite/rewrite-logging-frameworks/pull/246) > * Avoid unsafe changes in `JulParameterizedArguments` when `JavaType.Array` argument is not a `J.NewArray` by [`@pdelagrave`](https://github.com/pdelagrave) in [openrewrite/rewrite-logging-frameworks#244](https://redirect.github.com/openrewrite/rewrite-logging-frameworks/pull/244) > * LoggerLevelArgumentToMethod for JBoss Logging by [`@pdelagrave`](https://github.com/pdelagrave) in [openrewrite/rewrite-logging-frameworks#243](https://redirect.github.com/openrewrite/rewrite-logging-frameworks/pull/243) > * Add a recipe to migrate from JBoss Logging to SLF4J by [`@pdelagrave`](https://github.com/pdelagrave) in [openrewrite/rewrite-logging-frameworks#241](https://redirect.github.com/openrewrite/rewrite-logging-frameworks/pull/241) > * rename settings.local.json to settings.json by [`@zieka`](https://github.com/zieka) in [openrewrite/rewrite-logging-frameworks#247](https://redirect.github.com/openrewrite/rewrite-logging-frameworks/pull/247) > > **Full Changelog**: <openrewrite/rewrite-logging-frameworks@v3.12.0...v3.13.0> > > 3.12.1 > ------ > > What's Changed > -------------- > > * OpenRewrite v8.59.1: <https://github.com/openrewrite/rewrite> > > **Full Changelog**: <https://github.com/openrewrite/rewrite-logging-frameworks> > > 3.12.0 > ------ > > What's Changed > -------------- > > * Adds filePattern parameter to ConfigureLoggerLevel by [`@simonzn`](https://github.com/simonzn) in [openrewrite/rewrite-logging-frameworks#235](https://redirect.github.com/openrewrite/rewrite-logging-frameworks/pull/235) > * Handle existing arguments alongside concatenation in `ParameterizedLogging` by [`@timtebeek`](https://github.com/timtebeek) in [openrewrite/rewrite-logging-frameworks#237](https://redirect.github.com/openrewrite/rewrite-logging-frameworks/pull/237) > * Fix Slf4jLogShouldBeConstant to preserve format specifiers with width, alignment, and precision. by [`@motlin`](https://github.com/motlin) in [openrewrite/rewrite-logging-frameworks#239](https://redirect.github.com/openrewrite/rewrite-logging-frameworks/pull/239) > > New Contributors > ---------------- > > * [`@simonzn`](https://github.com/simonzn) made their first contribution in [openrewrite/rewrite-logging-frameworks#235](https://redirect.github.com/openrewrite/rewrite-logging-frameworks/pull/235) > * [`@motlin`](https://github.com/motlin) made their first contribution in [openrewrite/rewrite-logging-frameworks#239](https://redirect.github.com/openrewrite/rewrite-logging-frameworks/pull/239) > > **Full Changelog**: <openrewrite/rewrite-logging-frameworks@v3.11.0...v3.12.0> > > 3.11.0 > ------ > > What's Changed > -------------- > > * fix: any last argument matching `Throwable.toString()` is left as is by [`@pdelagrave`](https://github.com/pdelagrave) in [openrewrite/rewrite-logging-frameworks#229](https://redirect.github.com/openrewrite/rewrite-logging-frameworks/pull/229) > * refactor: org.openrewrite.mavencentral by [`@Laurens-W`](https://github.com/Laurens-W) in [openrewrite/rewrite-logging-frameworks#230](https://redirect.github.com/openrewrite/rewrite-logging-frameworks/pull/230) > * Don't if-wrap logs when arguments are getters by [`@pdelagrave`](https://github.com/pdelagrave) in [openrewrite/rewrite-logging-frameworks#233](https://redirect.github.com/openrewrite/rewrite-logging-frameworks/pull/233) > > **Full Changelog**: <openrewrite/rewrite-logging-frameworks@v3.10.0...v3.11.0> > > 3.10.0 > ------ > > What's Changed > -------------- > > * Add recipe to change logger fields to private by [`@jhl221123`](https://github.com/jhl221123) in [openrewrite/rewrite-logging-frameworks#221](https://redirect.github.com/openrewrite/rewrite-logging-frameworks/pull/221) > * Add `MatchIsLogLevelEnabledWithLogStatements` to SLF4J best practices by [`@timtebeek`](https://github.com/timtebeek) in [openrewrite/rewrite-logging-frameworks#222](https://redirect.github.com/openrewrite/rewrite-logging-frameworks/pull/222) > * Exclude spotbugs dependency by [`@greg-at-moderne`](https://github.com/greg-at-moderne) in [openrewrite/rewrite-logging-frameworks#225](https://redirect.github.com/openrewrite/rewrite-logging-frameworks/pull/225) > * Recipe to remove `.toString()` called on parameterized logging statement arguments by [`@pdelagrave`](https://github.com/pdelagrave) in [openrewrite/rewrite-logging-frameworks#224](https://redirect.github.com/openrewrite/rewrite-logging-frameworks/pull/224) > * fix: match `.toString()` on any type, not just java.lang.Object by [`@pdelagrave`](https://github.com/pdelagrave) in [openrewrite/rewrite-logging-frameworks#226](https://redirect.github.com/openrewrite/rewrite-logging-frameworks/pull/226) > > New Contributors > ---------------- > > * [`@jhl221123`](https://github.com/jhl221123) made their first contribution in [openrewrite/rewrite-logging-frameworks#221](https://redirect.github.com/openrewrite/rewrite-logging-frameworks/pull/221) > * [`@pdelagrave`](https://github.com/pdelagrave) made their first contribution in [openrewrite/rewrite-logging-frameworks#224](https://redirect.github.com/openrewrite/rewrite-logging-frameworks/pull/224) ... (truncated) Commits * [`946d0ce`](openrewrite/rewrite-logging-frameworks@946d0ce) Update documentation examples * [`210eff9`](openrewrite/rewrite-logging-frameworks@210eff9) rename settings.local.json to settings.json ([#247](https://redirect.github.com/openrewrite/rewrite-logging-frameworks/issues/247)) * [`2e67627`](openrewrite/rewrite-logging-frameworks@2e67627) Add a recipe to migrate from JBoss Logging to SLF4J ([#241](https://redirect.github.com/openrewrite/rewrite-logging-frameworks/issues/241)) * [`c59c405`](openrewrite/rewrite-logging-frameworks@c59c405) LoggerLevelArgumentToMethod for JBoss Logging ([#243](https://redirect.github.com/openrewrite/rewrite-logging-frameworks/issues/243)) * [`36a5200`](openrewrite/rewrite-logging-frameworks@36a5200) OpenRewrite recipe best practices * [`aa3f14f`](openrewrite/rewrite-logging-frameworks@aa3f14f) Avoid unsafe changes in `JulParameterizedArguments` when `JavaType.Array` arg... * [`249944e`](openrewrite/rewrite-logging-frameworks@249944e) Add `ArgumentArrayToVarargs` for logger methods that take a var args argument... * [`bb34ecf`](openrewrite/rewrite-logging-frameworks@bb34ecf) Create Claude settings file * [`ce8bc2d`](openrewrite/rewrite-logging-frameworks@ce8bc2d) refactor: Static imports for Collections and Collectors * [`af44c67`](openrewrite/rewrite-logging-frameworks@af44c67) Recipe to migrate deprecated JBoss Logging calls ([#245](https://redirect.github.com/openrewrite/rewrite-logging-frameworks/issues/245)) * Additional commits viewable in [compare view](openrewrite/rewrite-logging-frameworks@v3.7.0...v3.13.0) [![Dependabot compatibility score](https://dependabot-badges.githubapp.com/badges/compatibility\_score?dependency-name=org.openrewrite.recipe:rewrite-logging-frameworks&package-manager=maven&previous-version=3.7.0&new-version=3.13.0)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- Dependabot commands and options You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot merge` will merge this PR after your CI passes on it - `@dependabot squash and merge` will squash and merge this PR after your CI passes on it - `@dependabot cancel merge` will cancel a previously requested merge and block automerging - `@dependabot reopen` will reopen this PR if it is closed - `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually - `@dependabot show ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)

support migrating JUL.log("{0}", arrayIdentifier) to slf4j

3a8de9d

moderne-meeseeks bot added this to OpenRewrite Jul 30, 2025

github-project-automation bot moved this to In Progress in OpenRewrite Jul 30, 2025

pdelagrave self-assigned this Jul 30, 2025

pdelagrave moved this from In Progress to Ready to Review in OpenRewrite Jul 30, 2025

pdelagrave added the enhancement New feature or request label Jul 30, 2025

pdelagrave requested a review from timtebeek July 30, 2025 13:38

jevanlingen reviewed Jul 30, 2025

View reviewed changes

src/main/java/org/openrewrite/java/logging/slf4j/JulParameterizedArguments.java Outdated Show resolved Hide resolved

jevanlingen reviewed Jul 30, 2025

View reviewed changes

src/main/java/org/openrewrite/java/logging/slf4j/JulParameterizedArguments.java Outdated Show resolved Hide resolved

jevanlingen reviewed Jul 30, 2025

View reviewed changes

src/main/java/org/openrewrite/java/logging/slf4j/JulParameterizedArguments.java Outdated Show resolved Hide resolved

jevanlingen reviewed Jul 30, 2025

View reviewed changes

src/main/java/org/openrewrite/java/logging/slf4j/JulParameterizedArguments.java Outdated Show resolved Hide resolved

pdelagrave added 2 commits July 30, 2025 12:25

applied multiple review suggestions

6ff6128

Merge branch 'main' into julparamsimprovement

3c1c0d1

timtebeek added 2 commits July 31, 2025 13:00

Merge remote-tracking branch 'origin/main' into julparamsimprovement

2e9d58c

# Conflicts: # src/main/java/org/openrewrite/java/logging/slf4j/JulParameterizedArguments.java

Use static import for Collections

fdd33bd

github-actions bot reviewed Jul 31, 2025

View reviewed changes

timtebeek closed this Jul 31, 2025

github-project-automation bot moved this from Ready to Review to Done in OpenRewrite Jul 31, 2025

timtebeek reopened this Jul 31, 2025

github-project-automation bot moved this from Done to In Progress in OpenRewrite Jul 31, 2025

timtebeek reviewed Jul 31, 2025

View reviewed changes

src/test/java/org/openrewrite/java/logging/slf4j/JulParameterizedArgumentsTest.java Outdated Show resolved Hide resolved

Apply suggestions from code review

f8f51e3

github-actions bot reviewed Jul 31, 2025

View reviewed changes

Revert the array unpacking improvement

b82f618

First time testing for a negative case shows that the recipe isn't producing valid code by itself and that `org.openrewrite.java.logging.slf4j.JulToSlf4j` isn't either. Fixed in the next commit.

pdelagrave removed their assignment Aug 4, 2025

timtebeek added 2 commits August 4, 2025 17:33

Merge remote-tracking branch 'origin/main' into julparamsimprovement

f533f53

# Conflicts: # src/main/java/org/openrewrite/java/logging/slf4j/JulParameterizedArguments.java

Skip JulParameterizedArguments to avoid unsafe change there

03dfd97

timtebeek approved these changes Aug 4, 2025

View reviewed changes

github-project-automation bot moved this from In Progress to Ready to Review in OpenRewrite Aug 4, 2025

timtebeek changed the title ~~Support migrating JUL.log("{0}", arrayIdentifier) to SLF4J~~ Avoid unsafe changes in JulParameterizedArguments when JavaType.Array argument is not a J.NewArray Aug 4, 2025

timtebeek assigned pdelagrave Aug 4, 2025

timtebeek merged commit aa3f14f into main Aug 4, 2025
2 checks passed

timtebeek deleted the julparamsimprovement branch August 4, 2025 15:54

github-project-automation bot moved this from Ready to Review to Done in OpenRewrite Aug 4, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Avoid unsafe changes in `JulParameterizedArguments` when `JavaType.Array` argument is not a `J.NewArray` #244

Avoid unsafe changes in `JulParameterizedArguments` when `JavaType.Array` argument is not a `J.NewArray` #244

Uh oh!

pdelagrave commented Jul 30, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jevanlingen commented Jul 30, 2025

Uh oh!

pdelagrave commented Jul 30, 2025

Uh oh!

jevanlingen commented Jul 31, 2025

Uh oh!

github-actions bot left a comment

Uh oh!

timtebeek commented Jul 31, 2025 •

edited

Loading

Uh oh!

timtebeek commented Jul 31, 2025

Uh oh!

timtebeek commented Jul 31, 2025

Uh oh!

Uh oh!

github-actions bot left a comment

Uh oh!

pdelagrave commented Jul 31, 2025 •

edited

Loading

Uh oh!

timtebeek left a comment

Uh oh!

Uh oh!

Uh oh!

Avoid unsafe changes in JulParameterizedArguments when JavaType.Array argument is not a J.NewArray #244

Avoid unsafe changes in JulParameterizedArguments when JavaType.Array argument is not a J.NewArray #244

Uh oh!

Conversation

pdelagrave commented Jul 30, 2025

What's changed?

Checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jevanlingen commented Jul 30, 2025

Uh oh!

pdelagrave commented Jul 30, 2025

Uh oh!

jevanlingen commented Jul 31, 2025

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

timtebeek commented Jul 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

timtebeek commented Jul 31, 2025

Uh oh!

timtebeek commented Jul 31, 2025

Uh oh!

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

pdelagrave commented Jul 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

timtebeek left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Avoid unsafe changes in `JulParameterizedArguments` when `JavaType.Array` argument is not a `J.NewArray` #244

Avoid unsafe changes in `JulParameterizedArguments` when `JavaType.Array` argument is not a `J.NewArray` #244

timtebeek commented Jul 31, 2025 •

edited

Loading

pdelagrave commented Jul 31, 2025 •

edited

Loading