Revert "ChangeDependencyGroupIdAndArtifactId does not update properties in parent pom (openrewrite#5815) (openrewrite#5903)

* Revert "`ChangeDependencyGroupIdAndArtifactId` does not update properties in parent pom (openrewrite#5815)"

This reverts commit faf51f5.

* Minimize diff in revert

---------

Co-authored-by: Tim te Beek <tim@moderne.io>

Author: 2 people

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

@@ -28,7 +28,6 @@
 import org.openrewrite.xml.RemoveContentVisitor;
 import org.openrewrite.xml.tree.Xml;
 
-import java.nio.file.Path;
 import java.util.*;
 
 import static java.util.Collections.max;
@@ -38,7 +37,7 @@
 
 @Value
 @EqualsAndHashCode(callSuper = false)
-public class ChangeDependencyGroupIdAndArtifactId extends ScanningRecipe<ChangeDependencyGroupIdAndArtifactId.Accumulator> {
+public class ChangeDependencyGroupIdAndArtifactId extends Recipe {
     transient MavenMetadataFailures metadataFailures = new MavenMetadataFailures(this);
 
     @Option(displayName = "Old groupId",
@@ -144,19 +143,25 @@ public Validated<Object> validate() {
     }
 
     @Override
-    public Accumulator getInitialValue(ExecutionContext ctx) {
-        return new Accumulator();
-    }
-
-    @Override
-    public TreeVisitor<?, ExecutionContext> getScanner(Accumulator acc) {
+    public TreeVisitor<?, ExecutionContext> getVisitor() {
         return new MavenVisitor<ExecutionContext>() {
-            final @Nullable VersionComparator versionComparator = newVersion != null ? Semver.validate(newVersion, versionPattern).getValue() : null;
+            @Nullable
+            final VersionComparator versionComparator = newVersion != null ? Semver.validate(newVersion, versionPattern).getValue() : null;
+            @Nullable
+            private Collection<String> 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(
+                            oldGroupId, oldArtifactId,
+                            Optional.ofNullable(newGroupId).orElse(oldGroupId),
+                            Optional.ofNullable(newArtifactId).orElse(oldArtifactId),
+                            newVersion, versionPattern).getVisitor());
+                }
                 return super.visitDocument(document, ctx);
             }
 
@@ -165,59 +170,58 @@ public Xml visitTag(Xml.Tag tag, ExecutionContext ctx) {
                 Xml.Tag t = (Xml.Tag) super.visitTag(tag, ctx);
                 boolean isOldDependencyTag = isDependencyTag(oldGroupId, oldArtifactId);
                 if (isOldDependencyTag && isNewDependencyPresent) {
-                    acc.isNewDependencyPresent = true;
+                    doAfterVisit(new RemoveContentVisitor<>(tag, true, true));
+                    maybeUpdateModel();
                     return t;
                 }
                 if (isOldDependencyTag || isPluginDependencyTag(oldGroupId, oldArtifactId)) {
-                    Optional<String> groupIdFromTag = t.getChildValue("groupId");
-                    Optional<String> artifactIdFromTag = t.getChildValue("artifactId");
-                    if (!groupIdFromTag.isPresent() || !artifactIdFromTag.isPresent()) {
-                        return t;
+                    String groupId = newGroupId;
+                    if (groupId != null) {
+                        t = changeChildTagValue(t, "groupId", groupId, ctx);
+                    } else {
+                        groupId = t.getChildValue("groupId").orElseThrow(NoSuchElementException::new);
                     }
-
-                    String groupId = groupIdFromTag.get();
-                    if (newGroupId != null) {
-                        storeParentPomProperty(groupId, newGroupId);
-                        groupId = newGroupId;
-                        acc.changeGroupId = groupId;
-                    }
-
-                    String artifactId = artifactIdFromTag.get();
-                    if (newArtifactId != null) {
-                        storeParentPomProperty(artifactId, newArtifactId);
-                        artifactId = newArtifactId;
-                        acc.changeArtifactId = artifactId;
+                    String artifactId = newArtifactId;
+                    if (artifactId != null) {
+                        t = changeChildTagValue(t, "artifactId", artifactId, ctx);
+                    } else {
+                        artifactId = t.getChildValue("artifactId").orElseThrow(NoSuchElementException::new);
                     }
-
+                    String currentVersion = t.getChildValue("version").orElse(null);
                     if (newVersion != null) {
                         try {
-                            Optional<String> currentVersion = t.getChildValue("version");
-                            String resolvedNewVersion = resolveSemverVersion(groupId, artifactId, currentVersion.orElse(newVersion), ctx);
-                            Scope scope = t.getChild("scope").map(xml -> Scope.fromName(xml.getValue().orElse(null))).orElse(Scope.Compile);
+                            String resolvedNewVersion = resolveSemverVersion(ctx, groupId, artifactId, currentVersion);
+                            Optional<Xml.Tag> scopeTag = t.getChild("scope");
+                            Scope scope = scopeTag.map(xml -> Scope.fromName(xml.getValue().orElse("compile"))).orElse(Scope.Compile);
                             Optional<Xml.Tag> versionTag = t.getChild("version");
 
                             boolean configuredToOverrideManageVersion = overrideManagedVersion != null && overrideManagedVersion; // False by default
                             boolean configuredToChangeManagedDependency = changeManagedDependency == null || changeManagedDependency; // True by default
 
+                            boolean versionTagPresent = versionTag.isPresent();
                             boolean oldDependencyManaged = isDependencyManaged(scope, oldGroupId, oldArtifactId);
                             boolean newDependencyManaged = isDependencyManaged(scope, groupId, artifactId);
-                            if (versionTag.isPresent()) {
+                            if (versionTagPresent) {
                                 // If the previous dependency had a version but the new artifact is managed, removed the version tag.
                                 if (!configuredToOverrideManageVersion && newDependencyManaged || (oldDependencyManaged && configuredToChangeManagedDependency)) {
-                                    acc.removeVersionTag = true;
+                                    t = (Xml.Tag) new RemoveContentVisitor<>(versionTag.get(), false, true).visit(t, ctx);
                                 } else {
                                     // Otherwise, change the version to the new value.
-                                    storeParentPomProperty(currentVersion.orElse(null), resolvedNewVersion);
-                                    acc.changeVersion = resolvedNewVersion;
+                                    t = changeChildTagValue(t, "version", resolvedNewVersion, ctx);
                                 }
                             } else if (configuredToOverrideManageVersion || !newDependencyManaged) {
-                                // If the version is not present, add the version if we are explicitly overriding a managed version or if no managed version exists.
-                                acc.createVersion = resolvedNewVersion;
+                                //If the version is not present, add the version if we are explicitly overriding a managed version or if no managed version exists.
+                                Xml.Tag newVersionTag = Xml.Tag.build("<version>" + resolvedNewVersion + "</version>");
+                                //noinspection ConstantConditions
+                                t = (Xml.Tag) new AddToTagVisitor<ExecutionContext>(t, newVersionTag, new MavenTagInsertionComparator(t.getChildren())).visitNonNull(t, ctx, getCursor().getParent());
                             }
                         } catch (MavenDownloadingException e) {
                             return e.warn(tag);
                         }
                     }
+                    if (t != tag) {
+                        maybeUpdateModel();
+                    }
                 }
 
                 //noinspection ConstantConditions
@@ -245,122 +249,23 @@ private boolean isDependencyManaged(Scope scope, String groupId, String artifact
             }
 
             @SuppressWarnings("ConstantConditions")
-            private String resolveSemverVersion(String groupId, String artifactId, String version, ExecutionContext ctx) throws MavenDownloadingException {
-                List<String> availableVersions = new ArrayList<>();
-                MavenMetadata mavenMetadata = metadataFailures.insertRows(ctx, () -> downloadMetadata(groupId, artifactId, ctx));
-                for (String v : mavenMetadata.getVersioning().getVersions()) {
-                    if (versionComparator.isValid(version, v)) {
-                        availableVersions.add(v);
-                    }
+            private String resolveSemverVersion(ExecutionContext ctx, String groupId, String artifactId, @Nullable String currentVersion) throws MavenDownloadingException {
+                if (versionComparator == null) {
+                    return newVersion;
                 }
-                return availableVersions.isEmpty() ? newVersion : max(availableVersions, versionComparator);
-            }
-
-            private void storeParentPomProperty(@Nullable String currentValue, String newValue) {
-                if (isProperty(currentValue)) {
-                    String name = currentValue.substring(2, currentValue.length() - 1).trim();
-                    if (!getResolutionResult().getPom().getRequested().getProperties().containsKey(name)) {
-                        storeParentPomProperty(getResolutionResult(), name, newValue);
-                    }
-                }
-            }
-
-            private void storeParentPomProperty(MavenResolutionResult resolutionResult, String name, String value) {
-                Pom pom = resolutionResult.getPom().getRequested();
-                if (pom.getSourcePath() != null && pom.getProperties().containsKey(name)) {
-                    acc.pomProperties.add(new PomProperty(pom.getSourcePath(), name, value));
-                } else if (resolutionResult.getParent() != null) {
-                    storeParentPomProperty(resolutionResult.getParent(), name, value);
-                }
-            }
-        };
-    }
-
-    @Override
-    public TreeVisitor<?, ExecutionContext> getVisitor(Accumulator acc) {
-        return new MavenVisitor<ExecutionContext>() {
-            @Override
-            public Xml visitDocument(Xml.Document document, ExecutionContext ctx) {
-                // 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(
-                            oldGroupId, oldArtifactId,
-                            Optional.ofNullable(newGroupId).orElse(oldGroupId),
-                            Optional.ofNullable(newArtifactId).orElse(oldArtifactId),
-                            newVersion, versionPattern).getVisitor());
-                }
-                return super.visitDocument(document, ctx);
-            }
-
-            @Override
-            public Xml visitTag(Xml.Tag tag, ExecutionContext ctx) {
-                Xml.Tag t = (Xml.Tag) super.visitTag(tag, ctx);
-                if (isPropertyTag()) {
-                    Path sourcePath = getResolutionResult().getPom().getRequested().getSourcePath();
-                    for (PomProperty prop : acc.pomProperties) {
-                        if (prop.filePath.equals(sourcePath)) {
-                            doAfterVisit(new ChangePropertyValue(prop.name, prop.value, false, false).getVisitor());
+                String finalCurrentVersion = currentVersion != null ? currentVersion : newVersion;
+                if (availableVersions == null) {
+                    availableVersions = new ArrayList<>();
+                    MavenMetadata mavenMetadata = metadataFailures.insertRows(ctx, () -> downloadMetadata(groupId, artifactId, ctx));
+                    for (String v : mavenMetadata.getVersioning().getVersions()) {
+                        if (versionComparator.isValid(finalCurrentVersion, v)) {
+                            availableVersions.add(v);
                         }
                     }
-                    return t;
-                }
-
-                boolean isOldDependencyTag = isDependencyTag(oldGroupId, oldArtifactId);
-                if (isOldDependencyTag && acc.isNewDependencyPresent) {
-                    doAfterVisit(new RemoveContentVisitor<>(t, true, true));
-                    maybeUpdateModel();
-                    return t;
-                }
-                if (isOldDependencyTag || isPluginDependencyTag(oldGroupId, oldArtifactId)) {
-                    if (acc.changeGroupId != null) {
-                        t = changeChildTagValue(t, "groupId", newGroupId, ctx);
-                    }
-                    if (acc.changeArtifactId != null) {
-                        t = changeChildTagValue(t, "artifactId", newArtifactId, ctx);
-                    }
 
-                    if (acc.createVersion != null) {
-                        Xml.Tag newVersionTag = Xml.Tag.build("<version>" + acc.createVersion + "</version>");
-                        t = (Xml.Tag) new AddToTagVisitor<ExecutionContext>(t, newVersionTag, new MavenTagInsertionComparator(t.getChildren())).visitNonNull(t, ctx, getCursor().getParent());
-                    } else if (acc.changeVersion != null) {
-                        t = changeChildTagValue(t, "version", acc.changeVersion, ctx);
-                    } else if (acc.removeVersionTag) {
-                        t = (Xml.Tag) new RemoveContentVisitor<>(t.getChild("version").get(), false, true).visitNonNull(t, ctx);
-                    }
-
-                    if (t != tag) {
-                        maybeUpdateModel();
-                    }
                 }
-
-                return t;
+                return availableVersions.isEmpty() ? newVersion : max(availableVersions, versionComparator);
             }
         };
     }
-
-    public static class Accumulator {
-        Set<PomProperty> pomProperties = new HashSet<>();
-        boolean isNewDependencyPresent;
-
-        @Nullable
-        String changeGroupId;
-
-        @Nullable
-        String changeArtifactId;
-
-        boolean removeVersionTag;
-
-        @Nullable
-        String createVersion;
-
-        @Nullable
-        String changeVersion;
-    }
-
-    @Value
-    public static class PomProperty {
-        Path filePath;
-        String name;
-        String value;
-    }
 }

rewrite-maven/src/main/java/org/openrewrite/maven/MavenVisitor.java

@@ -282,29 +282,19 @@ private boolean isTag(String name) {
         return getCursor().getValue() instanceof Xml.Tag && name.equals(getCursor().<Xml.Tag>getValue().getName());
     }
 
-    /**
-     * Updates the value of a given child tag within an XML tag, if the new value differs from the current one.
-     * <p>
-     * If the current value of the tag is a property (e.g. {@code ${some.property}}), and the property is defined
-     * in the same POM, the method updates the property instead of directly modifying the tag value.
-     * This method does not update properties defined in a parent POM.
-     *
-     * @param tag           The XML tag that contains the child tag.
-     * @param childTagName  The name of the child tag whose value should be changed.
-     * @param newValue      The new value to assign to the child tag. If {@code null} or equal to the current value,
-     *                      no change is made.
-     * @param p             The visitor context.
-     * @return              The potentially updated tag.
-     */
     protected Xml.Tag changeChildTagValue(Xml.Tag tag, String childTagName, @Nullable String newValue, P p) {
+        return changeChildTagValue(tag, childTagName, newValue, false, p);
+    }
+
+    protected Xml.Tag changeChildTagValue(Xml.Tag tag, String childTagName, @Nullable String newValue, @Nullable Boolean addPropertyIfMissing, P p) {
         Optional<Xml.Tag> childTag = tag.getChild(childTagName);
         if (childTag.isPresent()) {
             String oldValue = childTag.get().getValue().orElse(null);
             if (newValue != null && !newValue.equals(oldValue)) {
                 if (isProperty(oldValue)) {
                     String propertyName = oldValue.substring(2, oldValue.length() - 1);
                     if (getResolutionResult().getPom().getRequested().getProperties().containsKey(propertyName)) {
-                        doAfterVisit((TreeVisitor<?, P>) new ChangePropertyValue(propertyName, newValue, false, false).getVisitor());
+                        doAfterVisit((TreeVisitor<?, P>) new ChangePropertyValue(propertyName, newValue, addPropertyIfMissing, false).getVisitor());
                     }
                 } else {
                     tag = (Xml.Tag) new ChangeTagValueVisitor<>(childTag.get(), newValue).visitNonNull(tag, p);