Revert generalized maven recipe property handling (#5516)

timtebeek · web-flow · commit ae563f82a085 · 2025-05-28T15:02:44.000+02:00
* Revert "Generalize changing of properties in Maven recipes (#5503)" This reverts commit 0050d30. * Already introduce the new methods we will leverage soon * Avoid conflict with method from MavenVisitor * Avoid another conflict that IDE didn't warn about
diff --git a/rewrite-maven/src/main/java/org/openrewrite/maven/ChangeDependencyGroupIdAndArtifactId.java b/rewrite-maven/src/main/java/org/openrewrite/maven/ChangeDependencyGroupIdAndArtifactId.java
@@ -25,6 +25,7 @@
 import org.openrewrite.semver.Semver;
 import org.openrewrite.semver.VersionComparator;
 import org.openrewrite.xml.AddToTagVisitor;
+import org.openrewrite.xml.ChangeTagValueVisitor;
 import org.openrewrite.xml.RemoveContentVisitor;
 import org.openrewrite.xml.tree.Xml;
 
@@ -177,13 +178,13 @@ public Xml visitTag(Xml.Tag tag, ExecutionContext ctx) {
                 if (isOldDependencyTag || isPluginDependencyTag(oldGroupId, oldArtifactId)) {
                     String groupId = newGroupId;
                     if (groupId != null) {
-                        t = changeChildTagValue(t, "groupId", groupId, ctx);
+                        t = changeChildTagValue2(t, "groupId", groupId, ctx);
                     } else {
                         groupId = t.getChildValue("groupId").orElseThrow(NoSuchElementException::new);
                     }
                     String artifactId = newArtifactId;
                     if (artifactId != null) {
-                        t = changeChildTagValue(t, "artifactId", artifactId, ctx);
+                        t = changeChildTagValue2(t, "artifactId", artifactId, ctx);
                     } else {
                         artifactId = t.getChildValue("artifactId").orElseThrow(NoSuchElementException::new);
                     }
@@ -207,7 +208,7 @@ public Xml visitTag(Xml.Tag tag, ExecutionContext ctx) {
                                     t = (Xml.Tag) new RemoveContentVisitor<>(versionTag.get(), false, true).visit(t, ctx);
                                 } else {
                                     // Otherwise, change the version to the new value.
-                                    t = changeChildTagValue(t, "version", resolvedNewVersion, ctx);
+                                    t = changeChildTagValue2(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.
@@ -238,7 +239,17 @@ private boolean checkIfNewDependencyPresents(@Nullable String groupId, @Nullable
                         .anyMatch(rd -> (version == null) || version.equals(rd.getVersion()));
             }
 
+            @Deprecated // Use changeChildTagValue from MavenVisitor instead: https://github.com/openrewrite/rewrite/pull/5516
+            private Xml.Tag changeChildTagValue2(Xml.Tag tag, String childTagName, String newValue, ExecutionContext ctx) {
+                Optional<Xml.Tag> childTag = tag.getChild(childTagName);
+                if (childTag.isPresent() && !newValue.equals(childTag.get().getValue().orElse(null))) {
+                    tag = (Xml.Tag) new ChangeTagValueVisitor<>(childTag.get(), newValue).visitNonNull(tag, ctx);
+                }
+                return tag;
+            }
+
             private boolean isDependencyManaged(Scope scope, String groupId, String artifactId) {
+
                 MavenResolutionResult result = getResolutionResult();
                 for (ResolvedManagedDependency managedDependency : result.getPom().getDependencyManagement()) {
                     if (groupId.equals(managedDependency.getGroupId()) && artifactId.equals(managedDependency.getArtifactId())) {
diff --git a/rewrite-maven/src/main/java/org/openrewrite/maven/ChangeManagedDependencyGroupIdAndArtifactId.java b/rewrite-maven/src/main/java/org/openrewrite/maven/ChangeManagedDependencyGroupIdAndArtifactId.java
@@ -26,6 +26,7 @@
 import org.openrewrite.maven.tree.ResolvedPom;
 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;
 
@@ -40,6 +41,11 @@ public class ChangeManagedDependencyGroupIdAndArtifactId extends Recipe {
     @EqualsAndHashCode.Exclude
     MavenMetadataFailures metadataFailures = new MavenMetadataFailures(this);
 
+    // there are several implicitly defined version properties that we should never attempt to update
+    private static final Set<String> implicitlyDefinedVersionProperties = new HashSet<>(Arrays.asList(
+            "${version}", "${project.version}", "${pom.version}", "${project.parent.version}"
+    ));
+
     @Option(displayName = "Old groupId",
             description = "The old groupId to replace. The groupId is the first part of a managed dependency coordinate `com.google.guava:guava:VERSION`.",
             example = "org.openrewrite.recipe")
@@ -140,17 +146,24 @@ public Xml.Document visitDocument(Xml.Document document, ExecutionContext ctx) {
 
             @Override
             public Xml.Tag visitTag(Xml.Tag tag, ExecutionContext ctx) {
+
                 Xml.Tag t = super.visitTag(tag, ctx);
                 if (isManagedDependencyTag(oldGroupId, oldArtifactId)) {
-                    if (t.getChild("groupId").isPresent()) {
-                        t = changeChildTagValue(t, "groupId", newGroupId, ctx);
+                    Optional<Xml.Tag> groupIdTag = t.getChild("groupId");
+                    boolean changed = false;
+                    if (groupIdTag.isPresent() && !newGroupId.equals(groupIdTag.get().getValue().orElse(null))) {
+                        doAfterVisit(new ChangeTagValueVisitor<>(groupIdTag.get(), newGroupId));
+                        changed = true;
                     }
-                    if (t.getChild("artifactId").isPresent()) {
-                        t = changeChildTagValue(t, "artifactId", newArtifactId, ctx);
+                    Optional<Xml.Tag> artifactIdTag = t.getChild("artifactId");
+                    if (artifactIdTag.isPresent() && !newArtifactId.equals(artifactIdTag.get().getValue().orElse(null))) {
+                        doAfterVisit(new ChangeTagValueVisitor<>(artifactIdTag.get(), newArtifactId));
+                        changed = true;
                     }
                     if (newVersion != null) {
                         try {
                             Optional<Xml.Tag> versionTag = t.getChild("version");
+
                             if (versionTag.isPresent()) {
                                 String resolvedArtifactId = newArtifactId;
                                 if (resolvedArtifactId.contains("${")) {
@@ -159,13 +172,14 @@ public Xml.Tag visitTag(Xml.Tag tag, ExecutionContext ctx) {
                                     resolvedArtifactId = ResolvedPom.placeholderHelper.replacePlaceholders(newArtifactId, properties::get);
                                 }
                                 String resolvedNewVersion = resolveSemverVersion(ctx, newGroupId, resolvedArtifactId, getResolutionResult().getPom().getValue(versionTag.get().getValue().orElse(null)));
-                                t = changeChildTagValue(t, "version", resolvedNewVersion, ctx);
+                                t = (Xml.Tag) new ChangeTagValueVisitor<>(versionTag.get(), resolvedNewVersion).visitNonNull(t, 0, getCursor().getParentOrThrow());
                             }
+                            changed = true;
                         } catch (MavenDownloadingException e) {
                             return e.warn(t);
                         }
                     }
-                    if (t != tag) {
+                    if (changed) {
                         maybeUpdateModel();
                         doAfterVisit(new RemoveRedundantDependencyVersions(null, null, null, null).getVisitor());
                         if (isNewDependencyPresent) {
diff --git a/rewrite-maven/src/main/java/org/openrewrite/maven/ChangePluginGroupIdAndArtifactId.java b/rewrite-maven/src/main/java/org/openrewrite/maven/ChangePluginGroupIdAndArtifactId.java
@@ -23,8 +23,11 @@
 import org.openrewrite.Recipe;
 import org.openrewrite.TreeVisitor;
 import org.openrewrite.maven.table.MavenMetadataFailures;
+import org.openrewrite.xml.ChangeTagValueVisitor;
 import org.openrewrite.xml.tree.Xml;
 
+import java.util.Optional;
+
 @Value
 @EqualsAndHashCode(callSuper = false)
 public class ChangePluginGroupIdAndArtifactId extends Recipe {
@@ -87,13 +90,13 @@ public Xml visitTag(Xml.Tag tag, ExecutionContext ctx) {
                 Xml.Tag t = (Xml.Tag) super.visitTag(tag, ctx);
                 if (isPluginTag(oldGroupId, oldArtifactId)) {
                     if (newGroupId != null) {
-                        t = changeChildTagValue(t, "groupId", newGroupId, ctx);
+                        t = changeChildTagValue2(t, "groupId", newGroupId, ctx);
                     }
                     if (newArtifactId != null) {
-                        t = changeChildTagValue(t, "artifactId", newArtifactId, ctx);
+                        t = changeChildTagValue2(t, "artifactId", newArtifactId, ctx);
                     }
                     if (newVersion != null) {
-                        t = changeChildTagValue(t, "version", newVersion, ctx);
+                        t = changeChildTagValue2(t, "version", newVersion, ctx);
                     }
                     if (t != tag) {
                         maybeUpdateModel();
@@ -102,6 +105,14 @@ public Xml visitTag(Xml.Tag tag, ExecutionContext ctx) {
                 //noinspection ConstantConditions
                 return t;
             }
+
+            private Xml.Tag changeChildTagValue2(Xml.Tag tag, String childTagName, String newValue, ExecutionContext ctx) {
+                Optional<Xml.Tag> childTag = tag.getChild(childTagName);
+                if (childTag.isPresent() && !newValue.equals(childTag.get().getValue().orElse(null))) {
+                    tag = (Xml.Tag) new ChangeTagValueVisitor<>(childTag.get(), newValue).visitNonNull(tag, ctx);
+                }
+                return tag;
+            }
         };
     }
 }
diff --git a/rewrite-maven/src/main/java/org/openrewrite/maven/MavenVisitor.java b/rewrite-maven/src/main/java/org/openrewrite/maven/MavenVisitor.java
@@ -50,7 +50,13 @@ public class MavenVisitor<P> extends XmlVisitor<P> {
 
     // there are several implicitly defined version properties that we should never attempt to update
     private static final Set<String> IMPLICITLY_DEFINED_VERSION_PROPERTIES = new HashSet<>(Arrays.asList(
-            "${version}", "${project.version}", "${pom.version}", "${project.parent.version}"
+            "${version}",
+            "${project.version}",
+            "${pom.version}",
+            "${project.parent.version}",
+            "${revision}",
+            "${sha1}",
+            "${changelist}"
     ));
 
     private transient Xml.@Nullable Document document;
diff --git a/rewrite-maven/src/main/java/org/openrewrite/maven/UpgradeDependencyVersion.java b/rewrite-maven/src/main/java/org/openrewrite/maven/UpgradeDependencyVersion.java
@@ -52,6 +52,11 @@ public class UpgradeDependencyVersion extends ScanningRecipe<UpgradeDependencyVe
     @EqualsAndHashCode.Exclude
     transient MavenMetadataFailures metadataFailures = new MavenMetadataFailures(this);
 
+    // there are several implicitly defined version properties that we should never attempt to update
+    private static final Collection<String> implicitlyDefinedVersionProperties = Arrays.asList(
+            "${version}", "${project.version}", "${pom.version}", "${project.parent.version}"
+    );
+
     @Option(displayName = "Group",
             description = "The first part of a dependency coordinate `com.google.guava:guava:VERSION`. This can be a glob expression.",
             example = "com.fasterxml.jackson*")
@@ -151,7 +156,8 @@ public Xml.Tag visitTag(final Xml.Tag tag, final ExecutionContext ctx) {
                                 Optional<Xml.Tag> version = tag.getChild("version");
                                 if (version.isPresent()) {
                                     String requestedVersion = d.getRequested().getVersion();
-                                    if (isProperty(requestedVersion)) {
+                                    if (requestedVersion != null && requestedVersion.startsWith("${") &&
+                                        !implicitlyDefinedVersionProperties.contains(requestedVersion)) {
                                         String propertyName = requestedVersion.substring(2, requestedVersion.length() - 1);
                                         if (!getResolutionResult().getPom().getRequested().getProperties().containsKey(propertyName)) {
                                             storeParentPomProperty(getResolutionResult().getParent(), propertyName, newerVersion);
@@ -174,7 +180,8 @@ public Xml.Tag visitTag(final Xml.Tag tag, final ExecutionContext ctx) {
              * @param propertyName the name of the property to update, if found in any the parent pom source file
              * @param newerVersion the resolved newer version that any matching parent pom property should be updated to
              */
-            private void storeParentPomProperty(@Nullable MavenResolutionResult currentMavenResolutionResult, String propertyName, String newerVersion) {
+            private void storeParentPomProperty(
+                    @Nullable MavenResolutionResult currentMavenResolutionResult, String propertyName, String newerVersion) {
                 if (currentMavenResolutionResult == null) {
                     return; // No parent contained the property; might then be in the same source file, or an import BOM
                 }
@@ -263,8 +270,10 @@ public Xml.Tag visitTag(Xml.Tag tag, ExecutionContext ctx) {
                 }
 
                 private void retainVersions() {
-                    RetainVersions.plan(this, retainVersions == null ? emptyList() : retainVersions)
-                            .forEach(it -> doAfterVisit(it.getVisitor()));
+                    for (Recipe retainVersionRecipe : RetainVersions.plan(this, retainVersions == null ?
+                            emptyList() : retainVersions)) {
+                        doAfterVisit(retainVersionRecipe.getVisitor());
+                    }
                 }
             }
 
@@ -275,13 +284,26 @@ private Xml.Tag upgradeDependency(ExecutionContext ctx, Xml.Tag t) throws MavenD
                     // as a source file, attempt to find a new version.
                     String newerVersion = findNewerVersion(d.getGroupId(), d.getArtifactId(), d.getVersion(), ctx);
                     if (newerVersion != null) {
-                        if (t.getChild("version").isPresent()) {
-                            t = changeChildTagValue(t, "version", newerVersion, overrideManagedVersion, ctx);
+                        Optional<Xml.Tag> version = t.getChild("version");
+                        if (version.isPresent()) {
+                            String requestedVersion = d.getRequested().getVersion();
+                            if (requestedVersion != null && requestedVersion.startsWith("${") && !implicitlyDefinedVersionProperties.contains(requestedVersion)) {
+                                String propertyName = requestedVersion.substring(2, requestedVersion.length() - 1);
+                                if (getResolutionResult().getPom().getRequested().getProperties().containsKey(propertyName)) {
+                                    doAfterVisit(new ChangePropertyValue(propertyName, newerVersion, overrideManagedVersion, false).getVisitor());
+                                }
+                            } else {
+                                t = (Xml.Tag) new ChangeTagValueVisitor<>(version.get(), newerVersion).visitNonNull(t, 0, getCursor().getParentOrThrow());
+                            }
                         } else if (Boolean.TRUE.equals(overrideManagedVersion)) {
                             ResolvedManagedDependency dm = findManagedDependency(t);
                             // if a managed dependency is expressed as a property, change the property value
                             // do this only when a requested bom is absent, otherwise changing property has no effect
-                            if (dm != null && isProperty(dm.getRequested().getVersion()) && dm.getRequestedBom() == null) {
+                            if (dm != null &&
+                                dm.getRequested().getVersion() != null &&
+                                dm.getRequested().getVersion().startsWith("${") &&
+                                !implicitlyDefinedVersionProperties.contains(dm.getRequested().getVersion()) &&
+                                dm.getRequestedBom() == null) {
                                 doAfterVisit(new ChangePropertyValue(dm.getRequested().getVersion().substring(2,
                                         dm.getRequested().getVersion().length() - 1),
                                         newerVersion, overrideManagedVersion, false).getVisitor());
@@ -337,14 +359,20 @@ private Xml.Tag upgradePluginDependency(ExecutionContext ctx, Xml.Tag t) throws
                 if (groupId != null && artifactId != null && version != null) {
                     String newerVersion = findNewerVersion(groupId, artifactId, resolveVersion(version), ctx);
                     if (newerVersion != null) {
-                        t = changeChildTagValue(t, "version", newerVersion, overrideManagedVersion, ctx);
+                        if (version.startsWith("${") && !implicitlyDefinedVersionProperties.contains(version)) {
+                            doAfterVisit(new ChangePropertyValue(version.substring(2, version.length() - 1), newerVersion, overrideManagedVersion, false).getVisitor());
+                        } else {
+                            Optional<Xml.Tag> versionTag = t.getChild("version");
+                            assert versionTag.isPresent();
+                            t = (Xml.Tag) new ChangeTagValueVisitor<>(versionTag.get(), newerVersion).visitNonNull(t, 0, getCursor().getParentOrThrow());
+                        }
                     }
                 }
                 return t;
             }
 
             private String resolveVersion(String version) {
-                if (isProperty(version)) {
+                if (version.startsWith("${") && !implicitlyDefinedVersionProperties.contains(version)) {
                     Map<String, String> properties = getResolutionResult().getPom().getProperties();
                     String property = version.substring(2, version.length() - 1);
                     return properties.getOrDefault(property, version);
@@ -356,7 +384,7 @@ private String resolveVersion(String version) {
                 String newerVersion = findNewerVersion(groupId, artifactId, version2, ctx);
                 if (newerVersion == null) {
                     return null;
-                } else if (isProperty(requestedVersion)) {
+                } else if (requestedVersion != null && requestedVersion.startsWith("${")) {
                     //noinspection unchecked
                     return (TreeVisitor<Xml, ExecutionContext>) new ChangePropertyValue(requestedVersion.substring(2, requestedVersion.length() - 1), newerVersion, overrideManagedVersion, false)
                             .getVisitor();
diff --git a/rewrite-maven/src/test/java/org/openrewrite/maven/ChangeDependencyGroupIdAndArtifactIdTest.java b/rewrite-maven/src/test/java/org/openrewrite/maven/ChangeDependencyGroupIdAndArtifactIdTest.java
diff --git a/rewrite-maven/src/test/java/org/openrewrite/maven/ChangeManagedDependencyGroupIdAndArtifactIdTest.java b/rewrite-maven/src/test/java/org/openrewrite/maven/ChangeManagedDependencyGroupIdAndArtifactIdTest.java
