Skip to content

Commit 2af0299

Browse files
timtebeektimtebeek
authored
Fix ChangeManagedDependencyGroupIdAndArtifactId overwriting artifactId with glob pattern (#6783)
* Fix ChangeManagedDependencyGroupIdAndArtifactId overwriting artifactId with glob pattern When ChangeDependencyGroupIdAndArtifactId is invoked with a wildcard oldArtifactId (e.g. "swagger-*") and newArtifactId=null, the managed dependency handler incorrectly overwrites the artifactId XML tag with the literal glob string instead of preserving the original value. Only change groupId/artifactId tags when the new value actually differs from the old value. When they match (glob passthrough), preserve the original tag value. Also read actual groupId/artifactId from the XML tag for version resolution instead of using potentially-glob values. * Fix glob pattern leaking as newArtifactId in managed dependency delegation ChangeDependencyGroupIdAndArtifactId was passing glob patterns (e.g. "swagger-*") as concrete newArtifactId/newGroupId values when delegating to ChangeManagedDependencyGroupIdAndArtifactId. Fix the root cause by making newGroupId/newArtifactId nullable (null = preserve existing value) and passing them directly instead of falling back to old glob values. Also move the duplicate dependency check from visitDocument into visitTag so it uses the actual tag coordinates rather than potentially-null or glob new values.
1 parent adf0613 commit 2af0299

4 files changed

Lines changed: 164 additions & 18 deletions

File tree

‎rewrite-maven/src/main/java/org/openrewrite/maven/ChangeDependencyGroupIdAndArtifactId.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -266,8 +266,8 @@ public Xml visitDocument(Xml.Document document, ExecutionContext ctx) {
266266
if (configuredToChangeManagedDependency) {
267267
doAfterVisit(new ChangeManagedDependencyGroupIdAndArtifactId(
268268
oldGroupId, oldArtifactId,
269-
Optional.ofNullable(newGroupId).orElse(oldGroupId),
270-
Optional.ofNullable(newArtifactId).orElse(oldArtifactId),
269+
newGroupId,
270+
newArtifactId,
271271
newVersion, versionPattern).getVisitor());
272272
}
273273
// Update any exclusions that reference the old coordinates

‎rewrite-maven/src/main/java/org/openrewrite/maven/ChangeManagedDependencyGroupIdAndArtifactId.java‎

Lines changed: 27 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -54,13 +54,17 @@ public class ChangeManagedDependencyGroupIdAndArtifactId extends Recipe {
5454
String oldArtifactId;
5555

5656
@Option(displayName = "New groupId",
57-
description = "The new groupId to use.",
58-
example = "corp.internal.openrewrite.recipe")
57+
description = "The new groupId to use. Defaults to the existing group id.",
58+
example = "corp.internal.openrewrite.recipe",
59+
required = false)
60+
@Nullable
5961
String newGroupId;
6062

6163
@Option(displayName = "New artifactId",
62-
description = "The new artifactId to use.",
63-
example = "rewrite-testing-frameworks")
64+
description = "The new artifactId to use. Defaults to the existing artifact id.",
65+
example = "rewrite-testing-frameworks",
66+
required = false)
67+
@Nullable
6468
String newArtifactId;
6569

6670
@Option(displayName = "New version",
@@ -78,7 +82,7 @@ public class ChangeManagedDependencyGroupIdAndArtifactId extends Recipe {
7882
@Nullable
7983
String versionPattern;
8084

81-
public ChangeManagedDependencyGroupIdAndArtifactId(String oldGroupId, String oldArtifactId, String newGroupId, String newArtifactId, @Nullable String newVersion) {
85+
public ChangeManagedDependencyGroupIdAndArtifactId(String oldGroupId, String oldArtifactId, @Nullable String newGroupId, @Nullable String newArtifactId, @Nullable String newVersion) {
8286
this.oldGroupId = oldGroupId;
8387
this.oldArtifactId = oldArtifactId;
8488
this.newGroupId = newGroupId;
@@ -88,7 +92,7 @@ public ChangeManagedDependencyGroupIdAndArtifactId(String oldGroupId, String old
8892
}
8993

9094
@JsonCreator
91-
public ChangeManagedDependencyGroupIdAndArtifactId(String oldGroupId, String oldArtifactId, String newGroupId, String newArtifactId, @Nullable String newVersion, @Nullable String versionPattern) {
95+
public ChangeManagedDependencyGroupIdAndArtifactId(String oldGroupId, String oldArtifactId, @Nullable String newGroupId, @Nullable String newArtifactId, @Nullable String newVersion, @Nullable String versionPattern) {
9296
this.oldGroupId = oldGroupId;
9397
this.oldArtifactId = oldArtifactId;
9498
this.newGroupId = newGroupId;
@@ -126,12 +130,10 @@ public TreeVisitor<?, ExecutionContext> getVisitor() {
126130
final VersionComparator versionComparator = newVersion != null ? Semver.validate(newVersion, versionPattern).getValue() : null;
127131
@Nullable
128132
private Collection<String> availableVersions;
129-
private boolean isNewDependencyPresent;
130133
private Set<String> safeVersionPlaceholdersToChange = new HashSet<>();
131134

132135
@Override
133136
public Xml.Document visitDocument(Xml.Document document, ExecutionContext ctx) {
134-
isNewDependencyPresent = checkIfNewDependencyPresent(newGroupId, newArtifactId, newVersion);
135137
safeVersionPlaceholdersToChange = getSafeVersionPlaceholdersToChange(oldGroupId, oldArtifactId, ctx);
136138
return super.visitDocument(document, ctx);
137139
}
@@ -147,19 +149,26 @@ public Xml.Tag visitTag(Xml.Tag tag, ExecutionContext ctx) {
147149
}
148150
}
149151

150-
t = (Xml.Tag) new ChangeTagValueVisitor<>(t.getChild("groupId").orElse(null), newGroupId).visitNonNull(t, ctx);
151-
t = (Xml.Tag) new ChangeTagValueVisitor<>(t.getChild("artifactId").orElse(null), newArtifactId).visitNonNull(t, ctx);
152+
if (newGroupId != null) {
153+
t = (Xml.Tag) new ChangeTagValueVisitor<>(t.getChild("groupId").orElse(null), newGroupId).visitNonNull(t, ctx);
154+
}
155+
if (newArtifactId != null) {
156+
t = (Xml.Tag) new ChangeTagValueVisitor<>(t.getChild("artifactId").orElse(null), newArtifactId).visitNonNull(t, ctx);
157+
}
152158
if (newVersion != null) {
153159
try {
154160
Optional<Xml.Tag> versionTag = t.getChild("version");
155161
if (versionTag.isPresent()) {
156-
String resolvedArtifactId = newArtifactId;
162+
ResolvedPom pom = getResolutionResult().getPom();
163+
String resolvedGroupId = t.getChildValue("groupId").orElse(newGroupId != null ? newGroupId : oldGroupId);
164+
String resolvedArtifactId = t.getChildValue("artifactId").orElse(newArtifactId != null ? newArtifactId : oldArtifactId);
157165
if (resolvedArtifactId.contains("${")) {
158-
ResolvedPom pom = getResolutionResult().getPom();
159-
Map<String, String> properties = pom.getProperties();
160-
resolvedArtifactId = ResolvedPom.placeholderHelper.replacePlaceholders(newArtifactId, properties::get);
166+
resolvedArtifactId = ResolvedPom.placeholderHelper.replacePlaceholders(resolvedArtifactId, pom.getProperties()::get);
167+
}
168+
if (resolvedGroupId.contains("${")) {
169+
resolvedGroupId = ResolvedPom.placeholderHelper.replacePlaceholders(resolvedGroupId, pom.getProperties()::get);
161170
}
162-
String resolvedNewVersion = resolveSemverVersion(ctx, newGroupId, resolvedArtifactId, getResolutionResult().getPom().getValue(versionTag.get().getValue().orElse(null)));
171+
String resolvedNewVersion = resolveSemverVersion(ctx, resolvedGroupId, resolvedArtifactId, pom.getValue(versionTag.get().getValue().orElse(null)));
163172
String versionTagValue = t.getChildValue("version").orElse(null);
164173
if (versionTagValue == null || !safeVersionPlaceholdersToChange.contains(versionTagValue)) {
165174
t = (Xml.Tag) new ChangeTagValueVisitor<>(versionTag.get(), resolvedNewVersion).visitNonNull(t, ctx);
@@ -174,7 +183,9 @@ public Xml.Tag visitTag(Xml.Tag tag, ExecutionContext ctx) {
174183
if (t != tag) {
175184
maybeUpdateModel();
176185
doAfterVisit(new RemoveRedundantDependencyVersions(null, null, null, null).getVisitor());
177-
if (isNewDependencyPresent) {
186+
String effectiveGroupId = newGroupId != null ? newGroupId : tag.getChildValue("groupId").orElse(null);
187+
String effectiveArtifactId = newArtifactId != null ? newArtifactId : tag.getChildValue("artifactId").orElse(null);
188+
if (checkIfNewDependencyPresent(effectiveGroupId, effectiveArtifactId, newVersion)) {
178189
doAfterVisit(new RemoveContentVisitor<>(t, true, true));
179190
maybeUpdateModel();
180191
}

‎rewrite-maven/src/test/java/org/openrewrite/maven/ChangeDependencyGroupIdAndArtifactIdTest.java‎

Lines changed: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3149,6 +3149,85 @@ void handlesGlobCorrectlyWhenAlsoConflictingManagedLeavingPropertyIfNecessary()
31493149
);
31503150
}
31513151

3152+
@Test
3153+
void handlesGlobCorrectlyWithManagedDependencies() {
3154+
rewriteRun(
3155+
spec -> spec.recipe(new ChangeDependencyGroupIdAndArtifactId(
3156+
"io.swagger",
3157+
"swagger-*",
3158+
"io.swagger.core.v3",
3159+
null,
3160+
"2.2.0",
3161+
null
3162+
)),
3163+
//language=xml
3164+
pomXml(
3165+
"""
3166+
<project>
3167+
<groupId>com.example</groupId>
3168+
<artifactId>demo-child</artifactId>
3169+
<version>0.0.1-SNAPSHOT</version>
3170+
<dependencyManagement>
3171+
<dependencies>
3172+
<dependency>
3173+
<groupId>io.swagger</groupId>
3174+
<artifactId>swagger-annotations</artifactId>
3175+
<version>1.5.16</version>
3176+
</dependency>
3177+
<dependency>
3178+
<groupId>io.swagger</groupId>
3179+
<artifactId>swagger-models</artifactId>
3180+
<version>1.5.16</version>
3181+
</dependency>
3182+
</dependencies>
3183+
</dependencyManagement>
3184+
<dependencies>
3185+
<dependency>
3186+
<groupId>io.swagger</groupId>
3187+
<artifactId>swagger-annotations</artifactId>
3188+
</dependency>
3189+
<dependency>
3190+
<groupId>io.swagger</groupId>
3191+
<artifactId>swagger-models</artifactId>
3192+
</dependency>
3193+
</dependencies>
3194+
</project>
3195+
""",
3196+
"""
3197+
<project>
3198+
<groupId>com.example</groupId>
3199+
<artifactId>demo-child</artifactId>
3200+
<version>0.0.1-SNAPSHOT</version>
3201+
<dependencyManagement>
3202+
<dependencies>
3203+
<dependency>
3204+
<groupId>io.swagger.core.v3</groupId>
3205+
<artifactId>swagger-annotations</artifactId>
3206+
<version>2.2.0</version>
3207+
</dependency>
3208+
<dependency>
3209+
<groupId>io.swagger.core.v3</groupId>
3210+
<artifactId>swagger-models</artifactId>
3211+
<version>2.2.0</version>
3212+
</dependency>
3213+
</dependencies>
3214+
</dependencyManagement>
3215+
<dependencies>
3216+
<dependency>
3217+
<groupId>io.swagger.core.v3</groupId>
3218+
<artifactId>swagger-annotations</artifactId>
3219+
</dependency>
3220+
<dependency>
3221+
<groupId>io.swagger.core.v3</groupId>
3222+
<artifactId>swagger-models</artifactId>
3223+
</dependency>
3224+
</dependencies>
3225+
</project>
3226+
"""
3227+
)
3228+
);
3229+
}
3230+
31523231
@Test
31533232
void handlesCollisionInDeepRemoteParent() {
31543233
rewriteRun(
@@ -3566,4 +3645,5 @@ void shouldNotChangeDependencyWithImplicitlyDefinedVersionProperty() {
35663645
)
35673646
);
35683647
}
3648+
35693649
}

‎rewrite-maven/src/test/java/org/openrewrite/maven/ChangeManagedDependencyGroupIdAndArtifactIdTest.java‎

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -631,6 +631,61 @@ void doesNotMakeChangeWhenChangingBomImportManagedDependency() {
631631
);
632632
}
633633

634+
@Test
635+
void globArtifactIdRetainedWhenNewArtifactIdIsNull() {
636+
rewriteRun(
637+
spec -> spec.recipe(new ChangeManagedDependencyGroupIdAndArtifactId(
638+
"io.swagger",
639+
"swagger-*",
640+
"io.swagger.core.v3",
641+
null,
642+
null
643+
)),
644+
pomXml(
645+
"""
646+
<project>
647+
<modelVersion>4.0.0</modelVersion>
648+
<groupId>com.mycompany.app</groupId>
649+
<artifactId>my-app</artifactId>
650+
<version>1</version>
651+
<properties>
652+
<swagger.version>1.5.16</swagger.version>
653+
</properties>
654+
<dependencyManagement>
655+
<dependencies>
656+
<dependency>
657+
<groupId>io.swagger</groupId>
658+
<artifactId>swagger-annotations</artifactId>
659+
<version>${swagger.version}</version>
660+
</dependency>
661+
</dependencies>
662+
</dependencyManagement>
663+
</project>
664+
""",
665+
"""
666+
<project>
667+
<modelVersion>4.0.0</modelVersion>
668+
<groupId>com.mycompany.app</groupId>
669+
<artifactId>my-app</artifactId>
670+
<version>1</version>
671+
<properties>
672+
<swagger.version>1.5.16</swagger.version>
673+
</properties>
674+
<dependencyManagement>
675+
<dependencies>
676+
<dependency>
677+
<groupId>io.swagger.core.v3</groupId>
678+
<artifactId>swagger-annotations</artifactId>
679+
<version>${swagger.version}</version>
680+
</dependency>
681+
</dependencies>
682+
</dependencyManagement>
683+
</project>
684+
"""
685+
)
686+
);
687+
}
688+
634689
@Test
635690
void shouldNotChangeManagedDependencyWithImplicitlyDefinedVersionProperty() {
636691
rewriteRun(

0 commit comments

Comments
 (0)
⚡