Skip to content

ChangeDependencyGroupIdAndArtifactId should not limit updating managed dependencies#5880

Merged
timtebeek merged 7 commits intomainfrom
ChangeDependencyGroupIdAndArtifactId-should-not-limit-managed-dependencies-by-version-only
Aug 27, 2025
Merged

ChangeDependencyGroupIdAndArtifactId should not limit updating managed dependencies#5880
timtebeek merged 7 commits intomainfrom
ChangeDependencyGroupIdAndArtifactId-should-not-limit-managed-dependencies-by-version-only

Conversation

@jevanlingen
Copy link
Copy Markdown
Contributor

@jevanlingen jevanlingen commented Aug 7, 2025 •
edited by timtebeek
Loading

What's changed?

The ChangeDependencyGroupIdAndArtifactId recipe does always update managed dependencies, regardless of whether a new version is requested or not.

What's your motivation?

To be more specific, the org.openrewrite.openapi.swagger.UseJakartaSwaggerArtifacts recipe has listed as one of the steps:

  - org.openrewrite.java.dependencies.ChangeDependency:
      oldGroupId: io.swagger.core.v3
      oldArtifactId: swagger-annotations
      newArtifactId: swagger-annotations-jakarta

This does not work for managed dependencies without this fix.

--

On top of that, this fix also brings this recipe on par with its Gradle's counterpart:

rewrite/rewrite-gradle/src/main/java/org/openrewrite/gradle/ChangeDependency.java

Lines 198 to 200 in 8266018

if (changeManagedDependency == null || changeManagedDependency) {
doAfterVisit(new ChangeManagedDependency(oldGroupId, oldArtifactId, newGroupId, newArtifactId, newVersion, versionPattern).getVisitor());
}

Any additional context

This change could lead to downstream failing tests! As the managed dependencies will now also be changed when no version is given, running recipes that use ChangeDependencyGroupIdAndArtifactId / ChangeDependency without a newVersion or versionPattern will behave a little differently, which could lead to a different outcome. We should fix this by either fixing the tests or setting the changeManagedDependency option to false in such cases.

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

@jevanlingen jevanlingen requested a review from MBoegers August 7, 2025 12:53
@jevanlingen jevanlingen self-assigned this Aug 7, 2025
@jevanlingen jevanlingen added bug Something isn't working recipe Requested Recipe labels Aug 7, 2025
@github-project-automation github-project-automation bot moved this to In Progress in OpenRewrite Aug 7, 2025
@jevanlingen jevanlingen moved this from In Progress to Ready to Review in OpenRewrite Aug 7, 2025
github-actions[bot]

This comment was marked as spam.

Sign in to view

@timtebeek timtebeek self-requested a review August 7, 2025 15:27
timtebeek
timtebeek reviewed Aug 11, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@timtebeek timtebeek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's some conflicts here following

Would you mind addressing those?

github-actions[bot]

This comment was marked as spam.

Sign in to view

@timtebeek
Copy link
Copy Markdown
Member

timtebeek commented Aug 22, 2025

Is this still relevant given the conflicts we see on those same lines @jevanlingen ?

@jevanlingen
Merge remote-tracking branch 'origin/main' into ChangeDependencyGroup…
4008aee 
…IdAndArtifactId-should-not-limit-managed-dependencies-by-version-only

# Conflicts:
#	rewrite-maven/src/main/java/org/openrewrite/maven/ChangeDependencyGroupIdAndArtifactId.java
@jevanlingen
Copy link
Copy Markdown
Contributor Author

jevanlingen commented Aug 25, 2025

Yep, it's still relevant. These changes you see have everything to do with the recipe flipping between a Recipe and a ScanningRecipe. This PR is just a loosening of an if-check, with results in the ChangeManagedDependencyGroupIdAndArtifactId recipe be called more often.

timtebeek
timtebeek reviewed Aug 27, 2025
View reviewed changes
rewrite-maven/src/test/java/org/openrewrite/maven/ChangeDependencyGroupIdAndArtifactIdTest.java
Comment on lines +40 to +41
false,
false
Copy link
Copy Markdown
Member

@timtebeek timtebeek Aug 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What has me slightly worried is that we're changing these values here from what they were (implied false, true), to now explicitly setting the second to false to keep the test passing. That to me signals that we're changing the behavior in a way that might surprise folks using the explicit constructor, as we for instance do here:
https://github.com/openrewrite/rewrite-migrate-java/blob/05fa3e0a9e228a4d5568702e36ac62ee0d4cf889/src/main/java/org/openrewrite/java/migrate/javax/AddJaxbRuntime.java#L221-L224

Can you explain why this change in default is needed, and what changes we should be making downstream as a result?

Copy link
Copy Markdown
Member

@timtebeek timtebeek Aug 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I'm mostly trying to look out for is that we don't inadvertently start to see more changes that we hadn't anticipated or desired following this change; and that's hard to say with just a single test added and the defaults flipped on three more. Just trying to make sure we can move forward with this with confidence.

Copy link
Copy Markdown
Member

@timtebeek timtebeek Aug 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from what it looks like we're only sparsely using that recipe from Yaml; with no changes there expected (version was already used)
https://github.com/search?q=org%3Aopenrewrite+ChangeDependencyGroupIdAndArtifactId+language%3AYAML&type=code&l=YAML

Copy link
Copy Markdown
Member

@timtebeek timtebeek Aug 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this is the only downstream usage of the outdated constructor:

Copy link
Copy Markdown
Contributor Author

@jevanlingen jevanlingen Aug 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What has me slightly worried is that we're changing these values here from what they were (implied false, true), to now explicitly setting the second to false to keep the test passing.

Yes I can explain this. The tests that I changed do not take managed dependencies into account. Thus by setting these managed properties to explicit false, the behaviour stays the same. This is in line with the thing I mentioned in the starting post:

As the managed dependencies will now also be changed when no version is given, running recipes that use ChangeDependencyGroupIdAndArtifactId / ChangeDependency without a newVersion or versionPattern will behave a little differently, which could lead to a different outcome.

@timtebeek timtebeek mentioned this pull request Aug 27, 2025
Merged
timtebeek
timtebeek approved these changes Aug 27, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@timtebeek timtebeek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I verified the tests changed; seems like those predated the addition of these new options, which combined with the scenario they are testing makes sense to set these to not change managed dependencies (or alternatively update their expectations).

@timtebeek timtebeek merged commit c10a363 into main Aug 27, 2025
2 checks passed
@timtebeek timtebeek deleted the ChangeDependencyGroupIdAndArtifactId-should-not-limit-managed-dependencies-by-version-only branch August 27, 2025 09:17
@github-project-automation github-project-automation bot moved this from Ready to Review to Done in OpenRewrite Aug 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@github-actions github-actions[bot] github-actions[bot] left review comments

@timtebeek timtebeek timtebeek approved these changes

@MBoegers MBoegers Awaiting requested review from MBoegers

Assignees

@jevanlingen jevanlingen

Labels

bug Something isn't working recipe Requested Recipe

Projects

OpenRewrite
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

UseJakartaSwaggerArtifacts breaks when Managed

2 participants

@jevanlingen @timtebeek