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 4861dae5ec..2e9348dd46 100755 --- a/rewrite-maven/src/main/java/org/openrewrite/maven/ChangeDependencyGroupIdAndArtifactId.java +++ b/rewrite-maven/src/main/java/org/openrewrite/maven/ChangeDependencyGroupIdAndArtifactId.java @@ -259,14 +259,21 @@ public TreeVisitor getVisitor(Accumulator acc) { final VersionComparator versionComparator = newVersion != null ? Semver.validate(newVersion, versionPattern).getValue() : null; @Nullable private Collection availableVersions; - private boolean isNewDependencyPresent; private Set safeVersionPlaceholdersToChange = new HashSet<>(); + private final String effectiveNewGroupId = renamedGroupId(oldGroupId); + private final String effectiveNewArtifactId = renamedArtifactId(oldArtifactId); + private final boolean dedupeEnabled = !effectiveNewGroupId.equals(oldGroupId) || !effectiveNewArtifactId.equals(oldArtifactId); + private List existingNewDirectDependencies = new ArrayList<>(); + private List existingOldDirectDependencies = new ArrayList<>(); private final boolean configuredToOverrideManagedVersion = overrideManagedVersion != null && overrideManagedVersion; // False by default private final boolean configuredToChangeManagedDependency = changeManagedDependency == null || changeManagedDependency; // True by default @Override public Xml visitDocument(Xml.Document document, ExecutionContext ctx) { - isNewDependencyPresent = checkIfNewDependencyPresent(newGroupId, newArtifactId, newVersion); + if (dedupeEnabled) { + existingNewDirectDependencies = directDependencies(effectiveNewGroupId, effectiveNewArtifactId); + existingOldDirectDependencies = directDependencies(oldGroupId, oldArtifactId); + } safeVersionPlaceholdersToChange = getSafeVersionPlaceholdersToChange(oldGroupId, oldArtifactId, ctx); if (configuredToChangeManagedDependency) { doAfterVisit(new ChangeManagedDependencyGroupIdAndArtifactId( @@ -277,8 +284,6 @@ public Xml visitDocument(Xml.Document document, ExecutionContext ctx) { } // Add sibling exclusions for the new coordinates alongside existing old exclusions if (newGroupId != null || newArtifactId != null) { - String effectiveNewGroupId = newGroupId != null ? newGroupId : oldGroupId; - String effectiveNewArtifactId = newArtifactId != null ? newArtifactId : oldArtifactId; doAfterVisit(new MavenVisitor() { @Override public Xml visitTag(Xml.Tag tag, ExecutionContext ctx) { @@ -333,15 +338,18 @@ public Xml visitTag(Xml.Tag tag, ExecutionContext ctx) { } boolean isOldDependencyTag = isDependencyTag(oldGroupId, oldArtifactId); - if (isOldDependencyTag && isNewDependencyPresent) { + boolean isNewDependencyTag = dedupeEnabled && isDependencyTag(effectiveNewGroupId, effectiveNewArtifactId); + if (isOldDependencyTag && !isNewDependencyTag && renamedOldTagWouldDuplicate(t)) { doAfterVisit(new RemoveContentVisitor<>(tag, true, true)); maybeUpdateModel(); return t; } + boolean isSurvivingNewDependencyTag = isNewDependencyTag && newVersion != null && + isSurvivorOfRemovedOldDependency(t); boolean isPluginDependency = isPluginDependencyTag(oldGroupId, oldArtifactId); boolean isAnnotationProcessorPath = isAnnotationProcessorPathTag(oldGroupId, oldArtifactId); boolean deferUpdate = false; - if (isOldDependencyTag || isPluginDependency || isAnnotationProcessorPath) { + if (isOldDependencyTag || isPluginDependency || isAnnotationProcessorPath || isSurvivingNewDependencyTag) { if (newVersion != null) { String currentVersionValue = t.getChildValue("version").orElse(null); if (isImplicitlyDefinedVersionProperty(currentVersionValue)) { @@ -373,7 +381,7 @@ public Xml visitTag(Xml.Tag tag, ExecutionContext ctx) { boolean versionTagPresent = versionTag.isPresent(); // dependencyManagement does not apply to plugin dependencies or annotation processor paths boolean oldDependencyDefinedManaged = isOldDependencyTag && canAffectManagedDependency(getResolutionResult(), scope, oldGroupId, oldArtifactId); - boolean newDependencyManaged = isOldDependencyTag && isDependencyManaged(scope, groupId, artifactId); + boolean newDependencyManaged = (isOldDependencyTag || isSurvivingNewDependencyTag) && isDependencyManaged(scope, groupId, artifactId); if (versionTagPresent) { // If the previous dependency had a version but the new artifact is managed, removed the version tag. if (!configuredToOverrideManagedVersion && newDependencyManaged || (oldDependencyDefinedManaged && configuredToChangeManagedDependency)) { @@ -412,15 +420,58 @@ public Xml visitTag(Xml.Tag tag, ExecutionContext ctx) { return t; } - private boolean checkIfNewDependencyPresent(@Nullable String groupId, @Nullable String artifactId, @Nullable String version) { - if ((groupId == null) || (artifactId == null)) { - return false; + private List directDependencies(String groupId, String artifactId) { + List direct = new ArrayList<>(); + for (ResolvedDependency rd : findDependencies(groupId, artifactId)) { + if (rd.isDirect()) { + direct.add(rd); + } + } + return direct; + } + + private boolean renamedOldTagWouldDuplicate(Xml.Tag oldTag) { + String groupId = renamedGroupId(oldTag.getChildValue("groupId").orElse(null)); + String artifactId = renamedArtifactId(oldTag.getChildValue("artifactId").orElse(null)); + for (ResolvedDependency rd : existingNewDirectDependencies) { + if (Objects.equals(groupId, rd.getGroupId()) && + Objects.equals(artifactId, rd.getArtifactId()) && + sameClassifierTypeScope(oldTag, rd)) { + return true; + } + } + return false; + } + + private boolean isSurvivorOfRemovedOldDependency(Xml.Tag newTag) { + String newTagGroupId = newTag.getChildValue("groupId").orElse(null); + String newTagArtifactId = newTag.getChildValue("artifactId").orElse(null); + for (ResolvedDependency rd : existingOldDirectDependencies) { + if (Objects.equals(newTagGroupId, renamedGroupId(rd.getGroupId())) && + Objects.equals(newTagArtifactId, renamedArtifactId(rd.getArtifactId())) && + sameClassifierTypeScope(newTag, rd)) { + return true; + } } - List dependencies = findDependencies(groupId, artifactId); - return dependencies.stream() - .filter(ResolvedDependency::isDirect) - .anyMatch(rd -> version == null || - versionComparator != null && versionComparator.compare(null, version, rd.getVersion()) <= 0); + return false; + } + + private boolean sameClassifierTypeScope(Xml.Tag tag, ResolvedDependency rd) { + return Objects.equals(emptyToNull(tag.getChildValue("classifier").orElse(null)), emptyToNull(rd.getClassifier())) && + tag.getChildValue("type").orElse("jar").equals(rd.getType()) && + Scope.fromName(tag.getChildValue("scope").orElse(null)) == Scope.fromName(rd.getRequested().getScope()); + } + + private @Nullable String renamedGroupId(@Nullable String fallback) { + return newGroupId != null ? newGroupId : fallback; + } + + private @Nullable String renamedArtifactId(@Nullable String fallback) { + return newArtifactId != null ? newArtifactId : fallback; + } + + private @Nullable String emptyToNull(@Nullable String value) { + return isBlank(value) ? null : value; } private boolean isDependencyManaged(Scope scope, String groupId, String artifactId) { 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 d26b10172e..adeef58962 100644 --- a/rewrite-maven/src/test/java/org/openrewrite/maven/ChangeDependencyGroupIdAndArtifactIdTest.java +++ b/rewrite-maven/src/test/java/org/openrewrite/maven/ChangeDependencyGroupIdAndArtifactIdTest.java @@ -278,7 +278,7 @@ void shouldNotAddNewIfDependencyAlreadyExists2() { @Issue("https://github.com/openrewrite/rewrite/issues/4514") @Test - void shouldAddNewIfDependencyAlreadyExistsInOlderVersion() { + void shouldUpgradeExistingInPlaceIfDependencyAlreadyExistsInOlderVersion() { rewriteRun( spec -> spec.recipe(new ChangeDependencyGroupIdAndArtifactId( "javax.activation", @@ -321,10 +321,166 @@ void shouldAddNewIfDependencyAlreadyExistsInOlderVersion() { jakarta.activation-api 1.2.2 + + + """ + ) + ); + } + + @Issue("https://github.com/openrewrite/rewrite-migrate-java/pull/1153") + @Test + void shouldNotLeaveDuplicateWhenExistingNewDependencyIsAtLowerVersion() { + rewriteRun( + spec -> spec.recipe(new ChangeDependencyGroupIdAndArtifactId( + "jakarta.jws", + "jakarta.jws-api", + "jakarta.xml.ws", + "jakarta.xml.ws-api", + "4.0.0", + null + )), + pomXml( + """ + + 4.0.0 + com.example + demo + 0.0.1-SNAPSHOT + + + jakarta.jws + jakarta.jws-api + 3.0.0 + + + jakarta.xml.ws + jakarta.xml.ws-api + 3.0.1 + + + + """, + """ + + 4.0.0 + com.example + demo + 0.0.1-SNAPSHOT + + + jakarta.xml.ws + jakarta.xml.ws-api + 4.0.0 + + + + """ + ) + ); + } + + @Test + void shouldNotDeduplicateWhenClassifierDiffers() { + 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 + sources + + + + """, + """ + + 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 + sources + + + + """ + ) + ); + } + + @Test + void shouldDeduplicateWhenOnlyArtifactIdChanges() { + rewriteRun( + spec -> spec.recipe(new ChangeDependencyGroupIdAndArtifactId( + "org.junit.jupiter", + "junit-jupiter-api", + null, + "junit-jupiter-engine", + "5.9.0", + null + )), + pomXml( + """ + + 4.0.0 + com.example + demo + 0.0.1-SNAPSHOT + + + org.junit.jupiter + junit-jupiter-api + 5.8.2 + + + org.junit.jupiter + junit-jupiter-engine + 5.8.2 + + + + """, + """ + + 4.0.0 + com.example + demo + 0.0.1-SNAPSHOT + + + org.junit.jupiter + junit-jupiter-engine + 5.9.0