Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Updating a BOM drops the version of untouched dependencies #4679

Open
gsmet opened this issue Nov 16, 2024 · 4 comments
Open

Updating a BOM drops the version of untouched dependencies #4679

gsmet opened this issue Nov 16, 2024 · 4 comments
Labels
bug Something isn't working

Comments

@gsmet
Copy link
Contributor

gsmet commented Nov 16, 2024

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 quarkusio/quarkus#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
Copy link
Contributor

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:

@Test
void removesRedundantExplicitVersionsMatchingOldParent() {
rewriteRun(
spec -> spec.recipe(new ChangeParentPom(
"org.junit",
null,
"junit-bom",
null,
"5.9.1",
null,
null,
null,
false
)),
pomXml(
"""
<project>
<modelVersion>4.0.0</modelVersion>
<parent>
<groupId>org.junit</groupId>
<artifactId>junit-bom</artifactId>
<version>5.9.0</version>
<relativePath/>
</parent>
<groupId>com.mycompany.app</groupId>
<artifactId>my-app</artifactId>
<version>1</version>
<dependencies>
<dependency>
<groupId>org.junit.jupiter</groupId>
<artifactId>junit-jupiter-api</artifactId>
<version>5.9.0</version>
</dependency>
</dependencies>
</project>
""",
"""
<project>
<modelVersion>4.0.0</modelVersion>
<parent>
<groupId>org.junit</groupId>
<artifactId>junit-bom</artifactId>
<version>5.9.1</version>
<relativePath/>
</parent>
<groupId>com.mycompany.app</groupId>
<artifactId>my-app</artifactId>
<version>1</version>
<dependencies>
<dependency>
<groupId>org.junit.jupiter</groupId>
<artifactId>junit-jupiter-api</artifactId>
</dependency>
</dependencies>
</project>
"""
)
);
}
@Test
void removesRedundantExplicitVersionsMatchingNewParent() {
rewriteRun(
spec -> spec.recipe(new ChangeParentPom(
"org.junit",
null,
"junit-bom",
null,
"5.9.1",
null,
null,
null,
false
)),
pomXml(
"""
<project>
<modelVersion>4.0.0</modelVersion>
<parent>
<groupId>org.junit</groupId>
<artifactId>junit-bom</artifactId>
<version>5.9.0</version>
<relativePath/>
</parent>
<groupId>com.mycompany.app</groupId>
<artifactId>my-app</artifactId>
<version>1</version>
<dependencies>
<dependency>
<groupId>org.junit.jupiter</groupId>
<artifactId>junit-jupiter-api</artifactId>
<version>5.9.1</version>
</dependency>
</dependencies>
</project>
""",
"""
<project>
<modelVersion>4.0.0</modelVersion>
<parent>
<groupId>org.junit</groupId>
<artifactId>junit-bom</artifactId>
<version>5.9.1</version>
<relativePath/>
</parent>
<groupId>com.mycompany.app</groupId>
<artifactId>my-app</artifactId>
<version>1</version>
<dependencies>
<dependency>
<groupId>org.junit.jupiter</groupId>
<artifactId>junit-jupiter-api</artifactId>
</dependency>
</dependencies>
</project>
"""
)
);
}

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
Copy link
Contributor Author

gsmet commented Nov 18, 2024

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
Copy link
Contributor

How about an option for this?

@gsmet
Copy link
Contributor Author

gsmet commented Nov 18, 2024

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: No status
Development

No branches or pull requests

3 participants