ChangeDependencyGroupIdAndArtifactId should not limit updating managed dependencies

[jevanlingen]

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?

• Fixes UseJakartaSwaggerArtifacts breaks when Managed rewrite-openapi#46

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

[timtebeek]

There's some conflicts here following

• #5815

Would you mind addressing those?

[timtebeek]

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

[jevanlingen]

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]

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?

[timtebeek]

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.

[timtebeek]

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

[timtebeek]

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

• AddJaxbRuntime should use overrideManagedDependency=true rewrite-migrate-java#838

[jevanlingen]

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]

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).