Skip to content

ChangeDependency skips dependencies whose new coordinates don't resolve #7321

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.

Sign up for GitHub

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

Merged
steve-aom-elliott merged 5 commits into from
Apr 9, 2026
Merged

ChangeDependency skips dependencies whose new coordinates don't resolve #7321

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
af69a30
ChangeDependency skips dependencies whose new coordinates don't resolve
steve-aom-elliott Apr 8, 2026
c57843e
Apply same fix to Maven ChangeDependency variants
steve-aom-elliott Apr 8, 2026
27f89f0
Track per-artifact resolution failures to protect shared version vari…
steve-aom-elliott Apr 8, 2026
6ddc62d
Only suppress resolution warnings when glob patterns are in use
steve-aom-elliott Apr 8, 2026
4fdb800
Tighten instanceof checks and extract isGlobPattern helper consistently
steve-aom-elliott Apr 8, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 13 additions & 2 deletions rewrite-gradle/src/main/java/org/openrewrite/gradle/ChangeDependency.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,7 @@ public Validated<Object> validate() {
public static class Accumulator {
Map<String, Object> versionVariableUpdates = new HashMap<>();
Map<String, Set<GroupArtifact>> versionVariableUsages = new HashMap<>();
Set<GroupArtifact> failedResolutions = new HashSet<>();
}

@Override
Expand Down Expand Up @@ -232,7 +233,10 @@ private void resolveAndRecordVersion(String varName, J.MethodInvocation m, Gradl
acc.versionVariableUpdates.put(varName, resolvedVersion);
}
} catch (MavenDownloadingException e) {
acc.versionVariableUpdates.put(varName, e);
acc.failedResolutions.add(new GroupArtifact(dep.getGroupId(), dep.getArtifactId()));
// Don't overwrite a successful resolution with a failure from a different artifact
// sharing the same version variable (e.g. hibernate-core resolved but hibernate-validator didn't)
acc.versionVariableUpdates.putIfAbsent(varName, e);
}
}
};
Expand Down Expand Up @@ -347,6 +351,10 @@ private boolean canUpdateVariable(String varName) {
}

private J.MethodInvocation updateDependency(J.MethodInvocation m, GradleDependency dep, ExecutionContext ctx) {
if (acc.failedResolutions.contains(new GroupArtifact(dep.getGroupId(), dep.getArtifactId()))) {
return m;
}

GradleDependency updated = dep;

if (!StringUtils.isBlank(newGroupId)) {
Expand All @@ -360,7 +368,7 @@ private J.MethodInvocation updateDependency(J.MethodInvocation m, GradleDependen
if (varName != null && !canUpdateVariable(varName)) {
Object scanResult = acc.versionVariableUpdates.get(varName);
if (scanResult instanceof Exception) {
return ((MavenDownloadingException) scanResult).warn(m);
return m;
}
if (scanResult instanceof String) {
updated = updated.withDeclaredVersion((String) scanResult);
Expand Down Expand Up @@ -514,6 +522,9 @@ private boolean canUpdateVariable(String varName, DependencyMatcher depMatcher,
if (!depMatcher.matches(ga.getGroupId(), ga.getArtifactId())) {
return false;
}
if (acc.failedResolutions.contains(ga)) {
return false;
}
}
return true;
}
Expand Down
39 changes: 39 additions & 0 deletions rewrite-gradle/src/test/java/org/openrewrite/gradle/ChangeDependencyTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1104,4 +1104,43 @@ void isNotAcceptableForPlainTextWithGradleProjectMarker() {
.build())));
assertThat(visitor.isAcceptable(sourceFile, new InMemoryExecutionContext())).isFalse();
}

@Test
void doesNotChangeGroupIdWhenNewCoordinatesDontResolve() {
rewriteRun(
spec -> spec.recipe(new ChangeDependency("org.hibernate", "hibernate-*", "org.hibernate.orm", null, "6.0.x", null, null, true)),
buildGradle(
"""
plugins {
id "java-library"
}

repositories {
mavenCentral()
}

def hibernateVersion = '5.6.15.Final'
dependencies {
implementation "org.hibernate:hibernate-core:${hibernateVersion}"
implementation "org.hibernate:hibernate-validator:${hibernateVersion}"
}
""",
"""
plugins {
id "java-library"
}

repositories {
mavenCentral()
}

def hibernateVersion = '5.6.15.Final'
dependencies {
implementation "org.hibernate.orm:hibernate-core:6.0.2.Final"
implementation "org.hibernate:hibernate-validator:${hibernateVersion}"
}
"""
)
);
}
}
3 changes: 2 additions & 1 deletion rewrite-maven/src/main/java/org/openrewrite/maven/ChangeDependencyGroupIdAndArtifactId.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -391,7 +391,8 @@ public Xml visitTag(Xml.Tag tag, ExecutionContext ctx) {
deferUpdate = true;
}
} catch (MavenDownloadingException e) {
return e.warn(tag);
// New coordinates can't be resolved — leave this dependency unchanged
return tag;
Copy link
Copy Markdown
Member

@timtebeek timtebeek Apr 8, 2026

Choose a reason for hiding this comment

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

I have a small worry here swallowing exceptions when we're running in a new environment that doesn't have credentials or repository access set up correctly. I can understand it if we want to suppress warnings when it's part of wildcard upgrade flow, but might it make sense to make that conditional / explicit here? To only suppress if wildcards are in play? Because if that's not the case as a user I'd want to see the failure. Or what's your view on this?

Same applies elsewhere above.

Copy link
Copy Markdown
Contributor Author

@steve-aom-elliott steve-aom-elliott Apr 8, 2026

Choose a reason for hiding this comment

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

You're correct. I had actually discussed this and asked about it, but it seems to have skipped doing it and I missed it in the last look through. Will get another adjustment to that pushed shortly

}
}

Expand Down
3 changes: 2 additions & 1 deletion ...aven/src/main/java/org/openrewrite/maven/ChangeManagedDependencyGroupIdAndArtifactId.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,8 @@ public Xml.Tag visitTag(Xml.Tag tag, ExecutionContext ctx) {
}
}
} catch (MavenDownloadingException e) {
return e.warn(t);
// New coordinates can't be resolved — leave this dependency unchanged
return tag;
}
}
if (t != tag) {
Expand Down
Loading
⚡