ChangeDependency skips dependencies whose new coordinates don't resolve#7321
Conversation
When using glob patterns like `hibernate-*` to change a dependency's groupId, artifacts that don't exist under the new groupId (e.g. hibernate-validator under org.hibernate.orm) were incorrectly modified. Three fixes: - Scanner: use putIfAbsent for exceptions so a failed resolution from one artifact doesn't overwrite a successful resolution from another sharing the same version variable - Edit (!canUpdateVariable path): return the original tree instead of adding a warning marker when the new coordinates can't be resolved - Edit (canUpdateVariable path): verify the new coordinates resolve before applying groupId/artifactId changes
ChangeDependencyGroupIdAndArtifactId already reverted the groupId change on resolution failure (returning `tag` not `t`), but still added a warning marker via `e.warn(tag)`. Changed to return `tag` unchanged. ChangeManagedDependencyGroupIdAndArtifactId was worse — it returned `e.warn(t)` which kept the modified groupId/artifactId AND added a warning. Changed to return the original `tag`.
…ables When a glob pattern matches artifacts that don't exist under the new coordinates (e.g. hibernate-validator under org.hibernate.orm), the shared version variable should not be updated. Instead, deps that resolve successfully get their version hardcoded, and deps that fail are left completely unchanged. Previously, canUpdateVariable only checked whether all deps sharing a variable matched the pattern. Now it also checks whether any of those deps failed version resolution in the scanner. This routes failed deps through the !canUpdateVariable path where they are skipped, while successful deps get hardcoded versions and detach from the variable.
| // New coordinates can't be resolved — leave this dependency unchanged | ||
| return tag; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
When a specific (non-glob) artifact fails to resolve under the new coordinates, that is a real problem the user should see — e.g. missing repository credentials or misconfiguration. Only suppress warnings and skip silently when the pattern contains wildcards, where a resolution failure is the expected signal that the glob matched an artifact that doesn't exist under the new group.
steve-aom-elliott
left a comment
There was a problem hiding this comment.
Good call Tim — pushed a commit that gates all warning suppression on whether the pattern contains wildcards (* or ?). For non-glob patterns, the original e.warn() behavior is preserved so users still see resolution failures from misconfigured environments.
- Change instanceof Exception to instanceof MavenDownloadingException to match the actual cast, since the map only stores String or MavenDownloadingException values - Extract isGlobPattern() helper in all three classes instead of inlining the check only in the Maven variants
timtebeek
left a comment
There was a problem hiding this comment.
Thanks for the quick turnaround; should make the downstream recipes easier to write and reason about.
…ordinates With openrewrite/rewrite#7321 merged, ChangeDependency automatically skips dependencies whose new coordinates don't resolve when using glob patterns. The hibernate-validator and hibernate-search-* revert entries are no longer needed.
Summary
When
ChangeDependencyuses glob patterns (e.g.org.hibernate:hibernate-*→org.hibernate.orm), it was incorrectly changing the groupId of artifacts that don't exist under the new group — likehibernate-validator, which belongs toorg.hibernate.validatornotorg.hibernate.orm. It also updated the shared version variable, breaking the unchanged dependency.All warning suppression and silent-skip behavior is gated on glob pattern usage (
*or?inoldGroupId/oldArtifactId). For non-glob patterns, the originale.warn()behavior is preserved so users still see resolution failures from misconfigured environments.Gradle
ChangeDependencyCore change: track per-artifact resolution failures when using globs. The scanner now records which specific
GroupArtifactcoordinates fail version resolution infailedResolutions(only when the pattern contains wildcards). This feeds into two places:canUpdateVariable— returns false when any matching artifact sharing a version variable failed resolution. This prevents the shared variable from being updated, and routes deps through the!canUpdateVariablepath where successful deps get hardcoded versions and failed deps are skipped.updateDependency— checksfailedResolutionsat the top and returns the original tree for deps whose new coordinates don't exist.Additional fix: Scanner uses
putIfAbsentfor exceptions (glob only) so a successful resolution from one artifact isn't overwritten by a failure from another sharing the same variable.Result: Given
hibernateVersion = '5.6.15.Final'shared byhibernate-coreandhibernate-validator:hibernate-core→org.hibernate.orm:hibernate-core:6.0.2.Final(hardcoded, detached from variable)hibernate-validator→ unchanged (org.hibernate:hibernate-validator:${hibernateVersion})hibernateVersion→ stays at5.6.15.FinalMaven
ChangeDependencyGroupIdAndArtifactIdOn resolution failure: returns
tagunchanged when glob patterns are in use (artifact doesn't exist under new coordinates). Preservese.warn(tag)for non-glob patterns.Maven
ChangeManagedDependencyGroupIdAndArtifactIdOn resolution failure: returns
tagunchanged when glob patterns are in use. Preservese.warn(t)for non-glob patterns. (Previously returnede.warn(t)which kept the modified groupId/artifactId AND added a warning — the originaltagis now returned for the glob case.)Test plan
doesNotChangeGroupIdWhenNewCoordinatesDontResolve: Gradle build withhibernateVersionproperty shared byhibernate-coreandhibernate-validator, globChangeDependencyhardcodeshibernate-core's version, leaveshibernate-validatorand the shared property untouchedChangeDependencyTesttests passChangeDependencyGroupIdAndArtifactIdTesttests passChangeManagedDependencyGroupIdAndArtifactIdTesttests pass