Correctly override parent property to set Java version

[DidierLoiseau]

What's changed?

• Added a test case on UpdateMavenProjectPropertyJavaVersionTest expecting no change when upgrading to Java 17 but parent is already spring-boot 3.3 (declares java.version and maven.compiler.release with value 17 already) .
• Partially reverted 12c8f37
• Implemented a fix to only override properties that actually parse to a number, and to add maven.release.plugin only when no release property is found

What's your motivation?

• Reproduce & fix maven.compiler.release set in parent is not recognized in child module #523

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

[timtebeek]

Much appreciated to have this runnable @DidierLoiseau ! Did you already look into a potential fix following the commit you referenced?

I'm a little behind on reviews, but will try to get to this one as well if you can't work out a fix already, as I understand the frustration of seeing a needless maven.compiler.release introduced.

[DidierLoiseau]

Much appreciated to have this runnable @DidierLoiseau ! Did you already look into a potential fix following the commit you referenced?

No, I’m new to OpenRewrite. I just quickly glanced over the commits of v21.0 and noticed one was a big refactoring/rewrite of UpgradeJavaVersion.

There is a side-question to this which is: which property should actually be set by UpgradeJavaVersion when an override of the parent is actually necessary? Because when the parent has specific/custom properties handling this (such as Spring’s java.version), it would be better to override that property than to set maven.compiler.release. For instance in Spring, this affects the configuration of kotlin-maven-plugin.

[DidierLoiseau]

I have moved the test to UpdateMavenProjectPropertyJavaVersionTest as it seemed more relevant there. While doing so, I noticed it contradicts bringsDownExplicitlyUsedPropertyFromRemoteParent(), but I think the latter is wrong: if you want to override the Java version from the parent, it’s not just the properties used in the current pom that need to be overridden, but also the properties used in any parent pom.

Assuming the parent poms are properly written, there should probably be a single property to override, and the other properties should derive from it. Based on this, I think a solution could be to check in mrr.getPom().getProperties() (not just the requested ones), and override all JAVA_VERSION_PROPERTIES which are numbers and are lower than the intended version (like it’s done with the JAVA_VERSION_XPATH_MATCHERS, maybe it’s not needed to have both). Then, only if none is found, override maven.compiler.release.

[DidierLoiseau]

Much appreciated to have this runnable @DidierLoiseau ! Did you already look into a potential fix following the commit you referenced?

In the end it seems to be commit 12c8f37 combined with that commit which caused the issue: the former was only taking into account properties used in the current pom, while the latter was adding maven.compiler.release when no property was updated.

I don’t really understand the purpose of 12c8f37: if the parent defines a property, it’s certainly to be used by the parent (like in Spring Boot) so we have to update it. I doubt there is a use case where you wouldn’t want that, but that commit didn’t reference a specific issue number.

I thus implemented a fix that reverted it – all properties from the (remote) parent are now overridden when they have a numeric value. With Spring Boot for example, this means that only java.version will be overridden (if needed) but not maven.compiler.release.

If this is not satisfying, I think the only solution would be to parse the maven.compiler.* properties + the maven-compiler-plugin configuration to determine which variable actually defines the version (at least maintaining JAVA_VERSION_PROPERTIES wouldn’t be needed any more…).

There are still 2 limitations (already there before):

• If the child overrides a parent property, it does not try to check if the parent already has the desired value to remove it. For instance, let’s say someone working with Spring Boot 2 first upgraded to Java 11 by setting java.version to 11. Now they want to upgrade to SB 3 and thus Java 17. The recipe will not detect that the SB3 parent already sets java.version to 17, it will thus just update it in the child. I think it’s easy to work around this with RemoveProperty.
• If the child overrides a local parent property, no update is performed, so it could stay in an old version defined in the child.

[DidierLoiseau]

I think maybe @sambsnyd should have a look since he is the one who implemented 12c8f37.

[timtebeek]

Thanks a lot for the work already done here @DidierLoiseau ! I've tried for a bit to get the new failures in org.openrewrite.java.migrate.UpgradeToJava17Test going, but can't immediately work it out. They fail with

java.lang.AssertionError: Expected recipe to complete in 1 cycle, but took 2 cycles. This usually indicates the recipe is making changes after it should have stabilized.

We usually see that if a recipe unconditionally makes a change, as opposed to only when the existing value does not match the target value.

Could be good to have @sambsnyd weigh in indeed, as I've seen reports of these excess properties over the past week or so.

[DidierLoiseau]

@timtebeek I fixed the test. The problem was related to the second comment I made: previously, the code was working with mrr.getPom().getRequested().getProperties(), so updating that was enough to prevent a second cycle of the recipe. Now that it works with mrr.getPom().getProperties(), that’s what needs to be updated.

It does not look like there is a convenient way to do that, so I just used maybeUpdateModel() in boh cases.

[DidierLoiseau]

Actually, there seems to be a bug in AddPropertyVisitor.visitTag() as it does not call maybeUpdateModel() when updating a property. It’s only done when adding one.

Previously it was working because UpdateMavenProjectPropertyJavaVersion was never using AddProperty for updating properties… except in the maven.compiler.release case, where it was changing the mrr.getPom().getRequested().getProperties() directly to prevent entering that block again.

Side note: when visitors instantiate and call other visitors which call maybeUpdateModel() (like it should have been the case here, actually), I think the UpdateMavenModel visitor will be added to each individual visitor’s afterVisit, so it will be executed several times.

[github-actions]

Some suggestions could not be made:

• src/main/java/org/openrewrite/java/migrate/maven/UpdateMavenProjectPropertyJavaVersion.java
  • lines 26-27

[timtebeek]

I like the simplifications done here with increased focus, and pointing to UseMavenCompilerPluginReleaseConfiguration where a change would be out of scope for this particular recipe. The added tests for common cases in practice will help guard regression in the future. Thanks a lo!