Skip to content

Commit 75019a9

Browse files
Jenson3210timtebeek
Jenson3210
and
timtebeek
authored
Preserve old exclusions and add sibling for new coordinates on dependency change (#6800)
* Add failing test for incorrect exclusion renaming When ChangeDependencyGroupIdAndArtifactId renames commons-lang:commons-lang to org.apache.commons:commons-lang3, it also incorrectly renames exclusions for commons-lang:commons-lang in other dependencies. The exclusion should remain unchanged because it blocks the old vulnerable transitive dependency. * Do not auto-update exclusions when changing dependency coordinates ChangeDependencyGroupIdAndArtifactId previously called ChangeExclusion to rename all exclusions matching the old coordinates to the new ones. This is incorrect because exclusions exist to block OLD transitive dependencies that other libraries may still pull in. Changing the exclusion from the old to the new coordinates means the old vulnerable artifact is no longer blocked. A stale exclusion (excluding something no longer pulled in transitively) is harmless, but an incorrect exclusion (no longer blocking a vulnerable transitive dependency) causes real problems. * Add sibling exclusion for new coordinates instead of replacing old ones ChangeDependencyGroupIdAndArtifactId previously called ChangeExclusion to rename exclusions matching the old coordinates to the new ones. This was incorrect because the old exclusion exists to block the OLD vulnerable transitive dependency that other libraries may still pull in. Now, when changing dependency coordinates, existing exclusions for the old coordinates are preserved and a sibling exclusion for the new coordinates is added alongside them. This ensures both old and new transitive dependencies are excluded. * Use exclusion-scoped patterns in test assertions Tighten pattern assertions to match within <exclusion> tags so they don't accidentally match on direct dependency coordinates. * Return early without checking second condition --------- Co-authored-by: Tim te Beek <tim@moderne.io>
1 parent afc5028 commit 75019a9

2 files changed

Lines changed: 97 additions & 9 deletions

File tree

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

Lines changed: 32 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -123,7 +123,7 @@ public String getInstanceNameSuffix() {
123123

124124
String description = "Change a Maven dependency coordinates. The `newGroupId` or `newArtifactId` **MUST** be different from before. " +
125125
"Matching `<dependencyManagement>` coordinates are also updated if a `newVersion` or `versionPattern` is provided. " +
126-
"Exclusions that reference the old dependency coordinates will also be updated to match the new coordinates.";
126+
"Exclusions that reference the old dependency coordinates are preserved, and a sibling exclusion for the new coordinates is added alongside them.";
127127

128128
@Override
129129
public Validated<Object> validate() {
@@ -270,11 +270,38 @@ public Xml visitDocument(Xml.Document document, ExecutionContext ctx) {
270270
newArtifactId,
271271
newVersion, versionPattern).getVisitor());
272272
}
273-
// Update any exclusions that reference the old coordinates
273+
// Add sibling exclusions for the new coordinates alongside existing old exclusions
274274
if (newGroupId != null || newArtifactId != null) {
275-
doAfterVisit(new ChangeExclusion(
276-
oldGroupId, oldArtifactId,
277-
newGroupId, newArtifactId).getVisitor());
275+
String effectiveNewGroupId = newGroupId != null ? newGroupId : oldGroupId;
276+
String effectiveNewArtifactId = newArtifactId != null ? newArtifactId : oldArtifactId;
277+
doAfterVisit(new MavenVisitor<ExecutionContext>() {
278+
@Override
279+
public Xml visitTag(Xml.Tag tag, ExecutionContext ctx) {
280+
Xml.Tag t = (Xml.Tag) super.visitTag(tag, ctx);
281+
if (!"exclusions".equals(t.getName())) {
282+
return t;
283+
}
284+
boolean hasOldExclusion = t.getChildren("exclusion").stream()
285+
.anyMatch(e -> matchesGlob(e.getChildValue("groupId").orElse(null), oldGroupId) &&
286+
matchesGlob(e.getChildValue("artifactId").orElse(null), oldArtifactId));
287+
if (!hasOldExclusion) {
288+
return t;
289+
}
290+
boolean hasNewExclusion = t.getChildren("exclusion").stream()
291+
.anyMatch(e -> effectiveNewGroupId.equals(e.getChildValue("groupId").orElse(null)) &&
292+
effectiveNewArtifactId.equals(e.getChildValue("artifactId").orElse(null)));
293+
if (!hasNewExclusion) {
294+
t = (Xml.Tag) new AddToTagVisitor<>(t, Xml.Tag.build(
295+
"<exclusion>\n" +
296+
"<groupId>" + effectiveNewGroupId + "</groupId>\n" +
297+
"<artifactId>" + effectiveNewArtifactId + "</artifactId>\n" +
298+
"</exclusion>"))
299+
.visitNonNull(t, ctx, getCursor().getParentOrThrow());
300+
maybeUpdateModel();
301+
}
302+
return t;
303+
}
304+
});
278305
}
279306
return super.visitDocument(document, ctx);
280307
}

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

Lines changed: 65 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3489,16 +3489,17 @@ void versionNotDroppedWhenSwaggerMigratedFirst() {
34893489

34903490
@Issue("https://github.com/moderneinc/customer-requests/issues/1330")
34913491
@Test
3492-
void exclusionUpdatedWhenDependencyGroupIdChanges() {
3493-
// When changing a dependency's groupId/artifactId, any exclusions that match
3494-
// the old coordinates should be updated to match the new coordinates.
3492+
void exclusionPreservedAndSiblingAddedWhenDependencyGroupIdChanges() {
3493+
// When changing a dependency's groupId/artifactId, exclusions that match
3494+
// the old coordinates should be preserved, and a sibling exclusion for the
3495+
// new coordinates should be added alongside them.
34953496
rewriteRun(
34963497
spec -> spec.recipe(new ChangeDependencyGroupIdAndArtifactId(
34973498
"com.fasterxml.jackson.jaxrs",
34983499
"jackson-jaxrs-json-provider",
34993500
"com.fasterxml.jackson.jakarta.rs",
35003501
"jackson-jakarta-rs-json-provider",
3501-
"2.x",
3502+
"2.18.x",
35023503
null
35033504
)),
35043505
pomXml(
@@ -3530,13 +3531,73 @@ void exclusionUpdatedWhenDependencyGroupIdChanges() {
35303531
</project>
35313532
""",
35323533
spec -> spec.after(actual -> assertThat(actual)
3534+
// Old exclusion preserved
3535+
.containsPattern("<exclusion>\\s*<groupId>com\\.fasterxml\\.jackson\\.jaxrs</groupId>\\s*<artifactId>jackson-jaxrs-json-provider</artifactId>\\s*</exclusion>")
3536+
// Sibling exclusion added for new coordinates
3537+
.containsPattern("<exclusion>\\s*<groupId>com\\.fasterxml\\.jackson\\.jakarta\\.rs</groupId>\\s*<artifactId>jackson-jakarta-rs-json-provider</artifactId>\\s*</exclusion>")
3538+
// Version updated
35333539
.containsPattern("<groupId>com\\.fasterxml\\.jackson\\.jakarta\\.rs</groupId>\\s*<artifactId>jackson-jakarta-rs-json-provider</artifactId>\\s*<version>2\\.\\d+\\.\\d+</version>")
35343540
.doesNotContain("<version>2.9.7</version>")
35353541
.actual())
35363542
)
35373543
);
35383544
}
35393545

3546+
@Test
3547+
void exclusionPreservedAndSiblingAddedForNewCoordinates() {
3548+
// When changing commons-lang:commons-lang to org.apache.commons:commons-lang3,
3549+
// the old exclusion for commons-lang:commons-lang should be preserved (to block
3550+
// the vulnerable transitive dependency), and a sibling exclusion for the new
3551+
// coordinates should be added alongside it.
3552+
rewriteRun(
3553+
spec -> spec.recipe(new ChangeDependencyGroupIdAndArtifactId(
3554+
"commons-lang",
3555+
"commons-lang",
3556+
"org.apache.commons",
3557+
"commons-lang3",
3558+
"3.x",
3559+
null
3560+
)),
3561+
pomXml(
3562+
"""
3563+
<project>
3564+
<modelVersion>4.0.0</modelVersion>
3565+
<groupId>com.mycompany.app</groupId>
3566+
<artifactId>my-app</artifactId>
3567+
<version>1</version>
3568+
<dependencies>
3569+
<dependency>
3570+
<groupId>commons-lang</groupId>
3571+
<artifactId>commons-lang</artifactId>
3572+
<version>2.6</version>
3573+
</dependency>
3574+
<dependency>
3575+
<groupId>org.apache.struts</groupId>
3576+
<artifactId>struts2-core</artifactId>
3577+
<version>2.5.30</version>
3578+
<exclusions>
3579+
<exclusion>
3580+
<groupId>commons-lang</groupId>
3581+
<artifactId>commons-lang</artifactId>
3582+
</exclusion>
3583+
</exclusions>
3584+
</dependency>
3585+
</dependencies>
3586+
</project>
3587+
""",
3588+
spec -> spec.after(actual -> assertThat(actual)
3589+
// Direct dependency changed to commons-lang3
3590+
.containsPattern("<groupId>org\\.apache\\.commons</groupId>\\s*<artifactId>commons-lang3</artifactId>\\s*<version>3\\.\\d+\\.\\d+</version>")
3591+
.doesNotContain("<version>2.6</version>")
3592+
// Old exclusion preserved
3593+
.containsPattern("<exclusion>\\s*<groupId>commons-lang</groupId>\\s*<artifactId>commons-lang</artifactId>\\s*</exclusion>")
3594+
// Sibling exclusion added for new coordinates
3595+
.containsPattern("<exclusion>\\s*<groupId>org\\.apache\\.commons</groupId>\\s*<artifactId>commons-lang3</artifactId>\\s*</exclusion>")
3596+
.actual())
3597+
)
3598+
);
3599+
}
3600+
35403601
@Test
35413602
void shouldNotChangeDependencyWithImplicitlyDefinedVersionProperty() {
35423603
rewriteRun(

0 commit comments

Comments
 (0)
⚡