From 637266ba89ac00f693881549a1e0960411d0a947 Mon Sep 17 00:00:00 2001 From: ashakirin <2254222+ashakirin@users.noreply.github.com> Date: Fri, 11 Oct 2024 20:59:46 +0200 Subject: [PATCH] feat: added check for duplicated dependency (#4515) * feat: added check for duplicated dependency Refs: #4514 * chore: renaming Refs: #4514 * refactor: moved delete old recipe after super() call Refs: #4514 * Add issue references to tests * Apply formatter --------- Co-authored-by: Andrei Shakirin Co-authored-by: Tim te Beek --- .../ChangeDependencyGroupIdAndArtifactId.java | 25 +- ...ManagedDependencyGroupIdAndArtifactId.java | 44 ++- ...ngeDependencyGroupIdAndArtifactIdTest.java | 310 ++++++++++++++++++ ...gedDependencyGroupIdAndArtifactIdTest.java | 109 ++++++ 4 files changed, 479 insertions(+), 9 deletions(-) diff --git a/rewrite-maven/src/main/java/org/openrewrite/maven/ChangeDependencyGroupIdAndArtifactId.java b/rewrite-maven/src/main/java/org/openrewrite/maven/ChangeDependencyGroupIdAndArtifactId.java index 6b3b61157b4..71277d86c36 100755 --- a/rewrite-maven/src/main/java/org/openrewrite/maven/ChangeDependencyGroupIdAndArtifactId.java +++ b/rewrite-maven/src/main/java/org/openrewrite/maven/ChangeDependencyGroupIdAndArtifactId.java @@ -21,10 +21,7 @@ import org.jspecify.annotations.Nullable; import org.openrewrite.*; import org.openrewrite.maven.table.MavenMetadataFailures; -import org.openrewrite.maven.tree.MavenMetadata; -import org.openrewrite.maven.tree.MavenResolutionResult; -import org.openrewrite.maven.tree.ResolvedManagedDependency; -import org.openrewrite.maven.tree.Scope; +import org.openrewrite.maven.tree.*; import org.openrewrite.semver.Semver; import org.openrewrite.semver.VersionComparator; import org.openrewrite.xml.AddToTagVisitor; @@ -153,9 +150,11 @@ public TreeVisitor getVisitor() { final VersionComparator versionComparator = newVersion != null ? Semver.validate(newVersion, versionPattern).getValue() : null; @Nullable private Collection availableVersions; + private boolean isNewDependencyPresent; @Override public Xml visitDocument(Xml.Document document, ExecutionContext ctx) { + isNewDependencyPresent = checkIfNewDependencyPresents(newGroupId, newArtifactId, newVersion); // Any managed dependency change is unlikely to use the same version, so only update selectively. if ((changeManagedDependency == null || changeManagedDependency) && newVersion != null || versionPattern != null) { doAfterVisit(new ChangeManagedDependencyGroupIdAndArtifactId( @@ -170,7 +169,13 @@ public Xml visitDocument(Xml.Document document, ExecutionContext ctx) { @Override public Xml visitTag(Xml.Tag tag, ExecutionContext ctx) { Xml.Tag t = (Xml.Tag) super.visitTag(tag, ctx); - if (isDependencyTag(oldGroupId, oldArtifactId)) { + boolean isOldDependencyTag = isDependencyTag(oldGroupId, oldArtifactId); + if (isOldDependencyTag && isNewDependencyPresent) { + doAfterVisit(new RemoveContentVisitor<>(tag, true)); + maybeUpdateModel(); + return t; + } + if (isOldDependencyTag) { String groupId = newGroupId; if (groupId != null) { t = changeChildTagValue(t, "groupId", groupId, ctx); @@ -224,6 +229,16 @@ public Xml visitTag(Xml.Tag tag, ExecutionContext ctx) { return t; } + private boolean checkIfNewDependencyPresents(@Nullable String groupId, @Nullable String artifactId, @Nullable String version) { + if ((groupId == null) || (artifactId == null)) { + return false; + } + List dependencies = findDependencies(groupId, artifactId); + return dependencies.stream() + .filter(ResolvedDependency::isDirect) + .anyMatch(rd -> (version == null) || version.equals(rd.getVersion())); + } + private Xml.Tag changeChildTagValue(Xml.Tag tag, String childTagName, String newValue, ExecutionContext ctx) { Optional childTag = tag.getChild(childTagName); if (childTag.isPresent() && !newValue.equals(childTag.get().getValue().orElse(null))) { diff --git a/rewrite-maven/src/main/java/org/openrewrite/maven/ChangeManagedDependencyGroupIdAndArtifactId.java b/rewrite-maven/src/main/java/org/openrewrite/maven/ChangeManagedDependencyGroupIdAndArtifactId.java index cc7204ea5a3..f7708a25a60 100644 --- a/rewrite-maven/src/main/java/org/openrewrite/maven/ChangeManagedDependencyGroupIdAndArtifactId.java +++ b/rewrite-maven/src/main/java/org/openrewrite/maven/ChangeManagedDependencyGroupIdAndArtifactId.java @@ -22,9 +22,11 @@ import org.openrewrite.*; import org.openrewrite.maven.table.MavenMetadataFailures; import org.openrewrite.maven.tree.MavenMetadata; +import org.openrewrite.maven.tree.ResolvedManagedDependency; import org.openrewrite.semver.Semver; import org.openrewrite.semver.VersionComparator; import org.openrewrite.xml.ChangeTagValueVisitor; +import org.openrewrite.xml.RemoveContentVisitor; import org.openrewrite.xml.tree.Xml; import java.util.*; @@ -103,8 +105,7 @@ public Validated validate() { if (newVersion != null) { validated = validated.and(Semver.validate(newVersion, versionPattern)); } - validated = - validated.and(test( + validated = validated.and(test( "coordinates", "newGroupId OR newArtifactId must be different from before", this, @@ -113,7 +114,7 @@ public Validated validate() { boolean sameArtifactId = isBlank(r.newArtifactId) || Objects.equals(r.oldArtifactId, r.newArtifactId); return !(sameGroupId && sameArtifactId); } - )); + )); return validated; } @@ -134,6 +135,14 @@ public TreeVisitor getVisitor() { final VersionComparator versionComparator = newVersion != null ? Semver.validate(newVersion, versionPattern).getValue() : null; @Nullable private Collection availableVersions; + private boolean isNewDependencyPresent; + + @Override + public Xml.Document visitDocument(Xml.Document document, ExecutionContext ctx) { + isNewDependencyPresent = checkIfNewDependencyPresents(newGroupId, newArtifactId, newVersion); + return super.visitDocument(document, ctx); + } + @Override public Xml.Tag visitTag(Xml.Tag tag, ExecutionContext ctx) { @@ -160,18 +169,45 @@ public Xml.Tag visitTag(Xml.Tag tag, ExecutionContext ctx) { t = (Xml.Tag) new ChangeTagValueVisitor<>(versionTag.get(), resolvedNewVersion).visitNonNull(t, 0, getCursor().getParentOrThrow()); } changed = true; - } catch(MavenDownloadingException e) { + } catch (MavenDownloadingException e) { return e.warn(t); } } if (changed) { maybeUpdateModel(); doAfterVisit(new RemoveRedundantDependencyVersions(null, null, (RemoveRedundantDependencyVersions.Comparator) null, null).getVisitor()); + if (isNewDependencyPresent) { + doAfterVisit(new RemoveContentVisitor<>(t, true)); + maybeUpdateModel(); + } } } return t; } + private boolean checkIfNewDependencyPresents(@Nullable String groupId, @Nullable String artifactId, @Nullable String version) { + if ((groupId == null) || (artifactId == null)) { + return false; + } + ResolvedManagedDependency managedDependency = findManagedDependency(groupId, artifactId); + if (managedDependency != null) { + return compareVersions(version, managedDependency.getVersion()); + } else { + return false; + } + } + + private boolean compareVersions(@Nullable String targetVersion, @Nullable String foundVersion) { + if (targetVersion == null) { + return true; + } + if ((versionComparator != null) && (foundVersion != null)) { + return versionComparator.isValid(targetVersion, foundVersion); + } else { + return targetVersion.equals(foundVersion); + } + } + @SuppressWarnings("ConstantConditions") private String resolveSemverVersion(ExecutionContext ctx, String groupId, String artifactId, @Nullable String currentVersion) throws MavenDownloadingException { if (versionComparator == null) { diff --git a/rewrite-maven/src/test/java/org/openrewrite/maven/ChangeDependencyGroupIdAndArtifactIdTest.java b/rewrite-maven/src/test/java/org/openrewrite/maven/ChangeDependencyGroupIdAndArtifactIdTest.java index c161d490a51..70bfef61989 100644 --- a/rewrite-maven/src/test/java/org/openrewrite/maven/ChangeDependencyGroupIdAndArtifactIdTest.java +++ b/rewrite-maven/src/test/java/org/openrewrite/maven/ChangeDependencyGroupIdAndArtifactIdTest.java @@ -98,6 +98,316 @@ void changeDependencyGroupIdAndArtifactId() { ); } + @Issue("https://github.com/openrewrite/rewrite/issues/4514") + @Test + void shouldNotAddNewIfDependencyAlreadyExists() { + rewriteRun( + spec -> spec.recipe(new ChangeDependencyGroupIdAndArtifactId( + "javax.activation", + "javax.activation-api", + "jakarta.activation", + "jakarta.activation-api", + null, + null + )), + pomXml( + """ + + 4.0.0 + com.mycompany.app + my-app + 1 + + + javax.activation + javax.activation-api + 1.2.0 + + + jakarta.activation + jakarta.activation-api + 1.2.1 + + + + """, + """ + + 4.0.0 + com.mycompany.app + my-app + 1 + + + jakarta.activation + jakarta.activation-api + 1.2.1 + + + + """ + ) + ); + } + + @Issue("https://github.com/openrewrite/rewrite/issues/4514") + @Test + void shouldAddNewIfDependencyAlreadyExistsInOlderVersion() { + rewriteRun( + spec -> spec.recipe(new ChangeDependencyGroupIdAndArtifactId( + "javax.activation", + "javax.activation-api", + "jakarta.activation", + "jakarta.activation-api", + "1.2.2", + null + )), + pomXml( + """ + + 4.0.0 + com.mycompany.app + my-app + 1 + + + javax.activation + javax.activation-api + 1.2.0 + + + jakarta.activation + jakarta.activation-api + 1.2.1 + + + + """, + """ + + 4.0.0 + com.mycompany.app + my-app + 1 + + + jakarta.activation + jakarta.activation-api + 1.2.2 + + + jakarta.activation + jakarta.activation-api + 1.2.1 + + + + """ + ) + ); + } + + @Issue("https://github.com/openrewrite/rewrite/issues/4514") + @Test + void shouldNotAddNewIfDependencyAlreadyExistsWithVersion() { + rewriteRun( + spec -> spec.recipe(new ChangeDependencyGroupIdAndArtifactId( + "javax.activation", + "javax.activation-api", + "jakarta.activation", + "jakarta.activation-api", + "1.2.1", + null + )), + pomXml( + """ + + 4.0.0 + com.mycompany.app + my-app + 1 + + + javax.activation + javax.activation-api + 1.2.0 + + + jakarta.activation + jakarta.activation-api + 1.2.1 + + + + """, + """ + + 4.0.0 + com.mycompany.app + my-app + 1 + + + jakarta.activation + jakarta.activation-api + 1.2.1 + + + + """ + ) + ); + } + + @Issue("https://github.com/openrewrite/rewrite/issues/4514") + @Test + void shouldNotAddNewIfDependencyAlreadyExistsManaged() { + rewriteRun( + spec -> spec.recipe(new ChangeDependencyGroupIdAndArtifactId( + "javax.activation", + "javax.activation-api", + "jakarta.activation", + "jakarta.activation-api", + null, + null + )), + pomXml( + """ + + 4.0.0 + com.mycompany.app + my-app + 1 + + + javax.activation + javax.activation-api + + + jakarta.activation + jakarta.activation-api + + + + + + javax.activation + javax.activation-api + 1.2.0 + + + jakarta.activation + jakarta.activation-api + 1.2.1 + + + + + """, + """ + + 4.0.0 + com.mycompany.app + my-app + 1 + + + jakarta.activation + jakarta.activation-api + + + + + + javax.activation + javax.activation-api + 1.2.0 + + + jakarta.activation + jakarta.activation-api + 1.2.1 + + + + + """ + ) + ); + } + + @Issue("https://github.com/openrewrite/rewrite/issues/4514") + @Test + void shouldNotAddNewIfDependencyAlreadyExistsManagedWithVersion() { + rewriteRun( + spec -> spec.recipe(new ChangeDependencyGroupIdAndArtifactId( + "javax.activation", + "javax.activation-api", + "jakarta.activation", + "jakarta.activation-api", + "1.2.1", + null + )), + pomXml( + """ + + 4.0.0 + com.mycompany.app + my-app + 1 + + + javax.activation + javax.activation-api + + + jakarta.activation + jakarta.activation-api + + + + + + javax.activation + javax.activation-api + 1.2.0 + + + jakarta.activation + jakarta.activation-api + 1.2.1 + + + + + """, + """ + + 4.0.0 + com.mycompany.app + my-app + 1 + + + jakarta.activation + jakarta.activation-api + + + + + + jakarta.activation + jakarta.activation-api + 1.2.1 + + + + + """ + ) + ); + } + @Test void changeManagedDependencyGroupIdAndArtifactId() { rewriteRun( diff --git a/rewrite-maven/src/test/java/org/openrewrite/maven/ChangeManagedDependencyGroupIdAndArtifactIdTest.java b/rewrite-maven/src/test/java/org/openrewrite/maven/ChangeManagedDependencyGroupIdAndArtifactIdTest.java index b1458dda26d..fa2d894528a 100644 --- a/rewrite-maven/src/test/java/org/openrewrite/maven/ChangeManagedDependencyGroupIdAndArtifactIdTest.java +++ b/rewrite-maven/src/test/java/org/openrewrite/maven/ChangeManagedDependencyGroupIdAndArtifactIdTest.java @@ -127,6 +127,115 @@ void changeManagedDependencyWithDynamicVersion() { ); } + @Issue("https://github.com/openrewrite/rewrite/issues/4514") + @Test + void removeOldDependencyIfNewAlreadyExists() { + rewriteRun( + spec -> spec.recipe(new ChangeManagedDependencyGroupIdAndArtifactId( + "javax.activation", + "javax.activation-api", + "jakarta.activation", + "jakarta.activation-api", + "2.1.0" + )), + pomXml( + """ + + 4.0.0 + com.mycompany.app + my-app + 1 + + + + javax.activation + javax.activation-api + 1.2.0 + + + jakarta.activation + jakarta.activation-api + 2.1.0 + + + + + """, + """ + + 4.0.0 + com.mycompany.app + my-app + 1 + + + + jakarta.activation + jakarta.activation-api + 2.1.0 + + + + + """ + ) + ); + } + + @Issue("https://github.com/openrewrite/rewrite/issues/4514") + @Test + void removeOldDependencyVersionWithPattern() { + rewriteRun( + spec -> spec.recipe(new ChangeManagedDependencyGroupIdAndArtifactId( + "jakarta.activation", + "jakarta.activation-api", + "com.google.guava", + "guava", + "32.0.X", + "-jre" + )), + pomXml( + """ + + 4.0.0 + com.mycompany.app + my-app + 1 + + + + com.google.guava + guava + 32.0.1-jre + + + jakarta.activation + jakarta.activation-api + 2.1.0 + + + + """, + """ + + 4.0.0 + com.mycompany.app + my-app + 1 + + + + com.google.guava + guava + 32.0.1-jre + + + + """ + ) + ); + } + @Test void latestPatchMangedDependency() { rewriteRun(