Updating a BOM drops the version of untouched dependencies

gsmet commented 2 weeks ago

What version of OpenRewrite are you using?

I am using

Maven plugin v5.45.0 (latest)

How are you running OpenRewrite?

I just execute the following:

mvn -U org.openrewrite.maven:rewrite-maven-plugin:run  -Drewrite.activeRecipes=io.quarkus.openrewrite.Quarkus

at the root of the project.

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

This problem was initially reported by @siewp in https://github.com/quarkusio/quarkus/issues/44529 . I stripped down their reproducer a bit.

Unzip this tiny reproducer:

update-bom-drops-version.zip

Execute:

mvn -U org.openrewrite.maven:rewrite-maven-plugin:run  -Drewrite.activeRecipes=io.quarkus.openrewrite.Quarkus

What did you expect to see?

I would expect the version of protobuf-java to stay untouched as I didn't perform any change on this dependency:

        <dependency>
            <groupId>com.google.protobuf</groupId>
            <artifactId>protobuf-java</artifactId>
            <!-- this version gets removed after Quarkus update was executed -->
            <version>3.25.5</version>
        </dependency>

What did you see instead?

The version is gone:

        <dependency>
            <groupId>com.google.protobuf</groupId>
            <artifactId>protobuf-java</artifactId>
            <!-- this version gets removed after Quarkus update was executed -->
        </dependency>

I think this issue is quite problematic as OpenRewrite usually does an excellent job at making the changes minimal.

timtebeek commented 2 weeks ago

hi @gsmet ; Thanks for reporting here! From the original bug report it's not clear if the change is merely unexpected, or problematic. As a bit of context: we don't necessarily strive for a minimal change, as much as we strive for the idiomatic change. When folks add or change the parent, that to us means also removing any redundant explicit versions, as verified here: https://github.com/openrewrite/rewrite/blob/36efd20f640902d87926945e5748bb1f4039b7a6/rewrite-maven/src/test/java/org/openrewrite/maven/ChangeParentPomTest.java#L801-L931

Similarly, if a new parent no longer manages a dependency, we should introduce the explicit version such that we do not break the project. We found both cases easier to integrate directly into the ChangeParentPom, as opposed to having to call a second/third recipe for that type of change, or having to be explicit about any dependencies now or no longer managed.

I hope that explains why we're making this change; the question now then is whether that's something to tweak or fix in case there's any problems. I'll cross post to the original issue for visibility to get more input and save you a step.

gsmet commented 2 weeks ago

For me, the issue is that when you point to very precise version, I don't think that "oh, the version is the aligned with the BOM now, let's drop it" always flies. You have enforced a precise version because you wanted this precise version and chances are you might not want it updated at the next BOM update. Or at least, you should have to make a decision about it because there are multiple cases here:

You want this version and no other version - never - you manage this version manually, you have all sorts of internal checks to do before updating
You pointed to an updated version because you wanted a fix in and when the BOM actually updates to this version, it's fine and you will go back to updating with the BOM.

At the moment, in the former case, the decision taken by OpenRewrite is incorrect and will lead to issues next time the BOM updates this dependency. And I could see how it could be interesting for OpenRewrite to add a comment pointing you are now aligned and can remove the version if that's what you want. But I find removing the version altogether a bit aggressive.

knutwannheden commented 2 weeks ago

How about an option for this?

gsmet commented 2 weeks ago

I suppose it would need to be a global option applied to the whole process as this change might get done at any time when the model is updated?

openrewrite / rewrite