UpdateMavenWrapper is reported as changing a lot of files (but it does not)

[gsmet]

What version of OpenRewrite are you using?

I am using

• Maven plugin 5.18.0
• Rewrite Recipe BOM: 2.6.1

I checked if the UpdateMavenWrapper had recent changes but I don't see any.

How are you running OpenRewrite?

Using Quarkus CLI and OpenRewrite Maven Plugin.

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

Not exactly sure yet as our recipes are pretty intricate.
I was wondering if the issue was something obvious/expected. If not, I will take the time to dig more into it and prepare a reproducer.

What I currently know is that:

#####
# Update Maven wrapper
#####
---
type: specs.openrewrite.org/v1beta/recipe
name: io.quarkus.updates.core.quarkus37.UpdateMavenWrapper
recipeList:
  - org.openrewrite.maven.UpdateMavenWrapper:
      distributionVersion: 3.9.x
      addIfMissing: false

is applied first and then our usual recipes.

As long as org.openrewrite.maven.UpdateMavenWrapper is declared, the change report is very suspicious so I suspect this recipe is doing something odd.
Or that it interacts with some other recipes we have.

What did you expect to see?

I don't expect org.openrewrite.maven.UpdateMavenWrapper to be reported as applying changes to all the files.

What did you see instead?

The org.openrewrite.maven.UpdateMavenWrapper is reported for each file modified as in:

[WARNING] Changes have been made to src/main/java/io/quarkus/bot/MarkClosedPullRequestInvalid.java by:
[WARNING]     io.quarkus.openrewrite.Quarkus
[WARNING]         io.quarkus.updates.core.quarkus37.UpdateMavenWrapper
[WARNING]             org.openrewrite.maven.UpdateMavenWrapper: {distributionVersion=3.9.x, addIfMissing=false}
[WARNING]         io.quarkus.updates.core.quarkus30.JavaxInjectMigrationToJakartaInject
[WARNING]             org.openrewrite.java.ChangePackage: {oldPackageName=javax.inject, newPackageName=jakarta.inject, recursive=true}
[WARNING] Changes have been made to src/main/java/io/quarkus/bot/PushToProjects.java by:
[WARNING]     io.quarkus.openrewrite.Quarkus
[WARNING]         io.quarkus.updates.core.quarkus37.UpdateMavenWrapper
[WARNING]             org.openrewrite.maven.UpdateMavenWrapper: {distributionVersion=3.9.x, addIfMissing=false}
[WARNING]         io.quarkus.updates.core.quarkus30.JavaxInjectMigrationToJakartaInject
[WARNING]             org.openrewrite.java.ChangePackage: {oldPackageName=javax.inject, newPackageName=jakarta.inject, recursive=true}
[WARNING]         io.quarkus.updates.core.quarkus30.JavaxJsonToJakartaJson
[WARNING]             org.openrewrite.java.ChangePackage: {oldPackageName=javax.json, newPackageName=jakarta.json, recursive=true}

Also, the recipe applying changes to one of the wrapper file is the last one of the recipes we apply and not UpdateMavenWrapper (yes I know, this is very odd):

[WARNING] Deleted file .mvn/wrapper/MavenWrapperDownloader.java by:
[WARNING]     io.quarkus.openrewrite.Quarkus
[WARNING]         org.openrewrite.java.camel.migrate.ChangeManagedFailoverLoadBalancerMBeanMethodName
[WARNING]             org.openrewrite.java.ChangeMethodName: {methodPattern=org.apache.camel.api.management.mbean.ManagedFailoverLoadBalancerMBean exceptionStatistics(), newMethodName=extendedInformation}

[shanman190]

So this is likely because the LST is being built by Maven and you're running the UpdateMavenWrapper recipe. In particular, it updates the build tool markers which will appear on every source file as you can see here.

I'll let another person weight in as well on if we should be updating that marker or if we should stop updating it, but that's going to be the cause.

https://github.com/openrewrite/rewrite/blob/main/rewrite-maven%2Fsrc%2Fmain%2Fjava%2Forg%2Fopenrewrite%2Fmaven%2FUpdateMavenWrapper.java#L350-L366

[timtebeek]

Sharing some very quick insights on what I think factors in, and what we might need to change instead (in between calls).

When we update the Maven wrapper, one of the things we do is change the BuildTool marker on source files to say they (now) use the newer Maven version. I think what you're seeing is an update to this marker reported as a code change, even though it's not printed to the output. The UpdateMavenWrapper recipe itself is an example of a recipe that uses that marker to check what version of Maven you were using, as compared to what version is desired.

rewrite/rewrite-maven/src/main/java/org/openrewrite/maven/UpdateMavenWrapper.java

Lines 189 to 190 in 73313f6

	acc.needsWrapperUpdate = true;
	acc.updatedMarker = buildTool.withVersion(mavenWrapper.getDistributionVersion());

What I think could be improved is that we now unconditionally add that marker to all source files in the Maven plugin, whereas we could be more selective to only add that to Maven associated files like any pom.xml, maven config files and the Maven wrapper files. That would prevent such Marker updates from being reported on unrelated files
https://github.com/openrewrite/rewrite-maven-plugin/blob/d323f46f5673209b89b129381ad30064229f193a/src/main/java/org/openrewrite/maven/MavenMojoProjectParser.java#L291-L295

I don't yet know why that would suddenly pop up now, which makes me not entirely sure this exactly what's going on, but it would be me first attempt at fixing this.

Alternatively we can look at not reporting changes to files in the plugin if there's no printed difference; that would hide the issue in both a good and a bad way.

[gsmet]

As far as we are concerned, it appears now because we just included the update in our recipes.

I understand what you're saying and it sounds logical, even if I definitely would prefer it not appearing for my Java files.

Now, what's even more puzzling is the second item I was reporting namely:

[WARNING] Deleted file .mvn/wrapper/MavenWrapperDownloader.java by:
[WARNING]     io.quarkus.openrewrite.Quarkus
[WARNING]         org.openrewrite.java.camel.migrate.ChangeManagedFailoverLoadBalancerMBeanMethodName
[WARNING]             org.openrewrite.java.ChangeMethodName: {methodPattern=org.apache.camel.api.management.mbean.ManagedFailoverLoadBalancerMBean exceptionStatistics(), newMethodName=extendedInformation}

This change seems to be randomly reported by the last recipe we apply.

Would it be because this change is not affected to the org.openrewrite.maven.UpdateMavenWrapper recipe as you don't actually handle this case in the recipe so it's affected to the last recipe standing?

[shanman190]

So what is probably happening for that one is that the org.openrewrite.java.ChangeMethodName starts off making a change (because it came earlier) to the MavenDownloader.java source file, then the UpdateMavemWrapper recipe deleted it.

https://github.com/openrewrite/rewrite/blob/main/rewrite-maven%2Fsrc%2Fmain%2Fjava%2Forg%2Fopenrewrite%2Fmaven%2FUpdateMavenWrapper.java#L395-L397

It seems like the recipe log might be missing recording of the fact that a recipe deleted a file, so that it can output it in the log section you provided.

[gsmet]

As mentioned, the org.openrewrite.java.ChangeMethodName comes last. It's the last recipe of the build.

And it specifically changes an Apache Camel class so I doubt it would apply any changes to the deleted file:

---
type: specs.openrewrite.org/v1beta/recipe
name: org.openrewrite.java.camel.migrate.ChangeManagedFailoverLoadBalancerMBeanMethodName
displayName: Change of method names brought by Camel JMX API changes
recipeList:
  - org.openrewrite.java.ChangeMethodName:
      methodPattern: org.apache.camel.api.management.mbean.ManagedFailoverLoadBalancerMBean exceptionStatistics()
      newMethodName: extendedInformation
      matchOverrides: null
      ignoreDefinition: null

It's a wild guess but I wouldn't be surprised if you were affecting the change to the last recipe around if not affected at all :).

[gsmet]

BTW, sorry for not providing the recipe file but it's 4k lines long so trying to push you the relevant info instead :)

[shanman190]

Hmm, true. That is interesting.

[shanman190]

Just as a curious experiment, have you tried changing what would be the last recipe executed to see if it follows that one? It definitely sounds like a bug somewhere though.

[gsmet]

I can confirm it's always the last recipe applied that is reported as making this change.

[timtebeek]

Wrestled a bit with similar change in the Gradle plugin this week; I think this will resolve the over reported marker changes:

• Only add BuildTool marker to Maven Wrapper and pom.xml files rewrite-maven-plugin#721

[timtebeek]

Looking at it some more I can say the problem is at least not obvious/expected. We already have some handling in the plugin to not report results when there are no visible changes; I'd have expected that to not produce these lines if there's indeed no code change to go along with it. 🤔
https://github.com/openrewrite/rewrite-maven-plugin/blob/a6161cb4f30eac96a84183eae4d1f65a307f6bd2/src/main/java/org/openrewrite/maven/AbstractRewriteMojo.java#L406-L408

And we've since revised how we do marker updates for Java upgrades, which we could learn from for this problem here.

• openrewrite/rewrite-migrate-java@9c6acfb