Fixed situation where a changing a managed dependency that does not have an explicit version tag would fail to add a version tag when providing the newVersion option. (#6105)

* Fixed situation where a changing a managed dependency that does not have an explicit `version` tag would fail to add a `version` tag when providing the `newVersion` option.

* WiP - refining prevention of version tag creation on direct dependency. Delegating `maybeUpdateModel()` call to implied `ChangeManagedDependencyGroupIdAndArtifactId` in applicable cases to avoid interim dependency resolution failure.

* Slight cleanup. One test still failing. Need to revisit validity of change proposed in the test itself.

* Adjusted check for managed dependency to be able to traverse through local parent POMs and only consider those managed dependencies that are actually defined in the requested set of managed dependencies, rather than those that get resolved via an import managed dependency. Tidied up some method names and renamed a test to better represent what it's actually doing.

* Corrected `<type>bom</type>` to `<type>pom</type>` in tests, and added a more complicated example for `ChangeDependencyGroupIdAndArtifactId`

Author: steve-aom-elliott

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

@@ -147,7 +147,7 @@ public TreeVisitor<?, ExecutionContext> getVisitor() {
 
             @Override
             public Xml visitDocument(Xml.Document document, ExecutionContext ctx) {
-                isNewDependencyPresent = checkIfNewDependencyPresents(newGroupId, newArtifactId, newVersion);
+                isNewDependencyPresent = checkIfNewDependencyPresent(newGroupId, newArtifactId, newVersion);
                 if (!oldGroupId.contains("*") && !oldArtifactId.contains("*") &&
                         (changeManagedDependency == null || changeManagedDependency)) {
                     doAfterVisit(new ChangeManagedDependencyGroupIdAndArtifactId(
@@ -170,16 +170,26 @@ public Xml visitTag(Xml.Tag tag, ExecutionContext ctx) {
                 }
                 boolean isPluginDependency = isPluginDependencyTag(oldGroupId, oldArtifactId);
                 boolean isAnnotationProcessorPath = isAnnotationProcessorPathTag(oldGroupId, oldArtifactId);
+                boolean deferUpdate = false;
                 if (isOldDependencyTag || isPluginDependency || isAnnotationProcessorPath) {
+                    ResolvedPom rp = getResolutionResult().getPom();
                     String groupId = newGroupId;
                     if (groupId != null) {
-                        t = changeChildTagValue(t, "groupId", groupId, ctx);
+                        Optional<Xml.Tag> groupIdTag = t.getChild("groupId");
+                        Optional<String> groupIdValue = t.getChildValue("groupId");
+                        if (groupIdTag.isPresent() && !groupId.equals(rp.getValue(groupIdValue.orElse(null)))) {
+                            t = changeChildTagValue(t, "groupId", groupId, ctx);
+                        }
                     } else {
                         groupId = t.getChildValue("groupId").orElseThrow(NoSuchElementException::new);
                     }
                     String artifactId = newArtifactId;
                     if (artifactId != null) {
-                        t = changeChildTagValue(t, "artifactId", artifactId, ctx);
+                        Optional<Xml.Tag> artifactIdTag = t.getChild("artifactId");
+                        Optional<String> artifactIdValue = t.getChildValue("artifactId");
+                        if (artifactIdTag.isPresent() && !artifactId.equals(rp.getValue(artifactIdValue.orElse(null)))) {
+                            t = changeChildTagValue(t, "artifactId", artifactId, ctx);
+                        }
                     } else {
                         artifactId = t.getChildValue("artifactId").orElseThrow(NoSuchElementException::new);
                     }
@@ -192,33 +202,37 @@ public Xml visitTag(Xml.Tag tag, ExecutionContext ctx) {
                             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 configuredToOverrideManagedVersion = overrideManagedVersion != null && overrideManagedVersion; // False by default
                             boolean configuredToChangeManagedDependency = changeManagedDependency == null || changeManagedDependency; // True by default
 
                             boolean versionTagPresent = versionTag.isPresent();
                             // dependencyManagement does not apply to plugin dependencies or annotation processor paths
-                            boolean oldDependencyManaged = isOldDependencyTag && isDependencyManaged(scope, oldGroupId, oldArtifactId);
+                            boolean oldDependencyDefinedManaged = isOldDependencyTag && canAffectManagedDependency(getResolutionResult(), scope, oldGroupId, oldArtifactId);
                             boolean newDependencyManaged = isOldDependencyTag && isDependencyManaged(scope, groupId, artifactId);
+
                             if (versionTagPresent) {
                                 // If the previous dependency had a version but the new artifact is managed, removed the version tag.
-                                if (!configuredToOverrideManageVersion && newDependencyManaged || (oldDependencyManaged && configuredToChangeManagedDependency)) {
+                                if (!configuredToOverrideManagedVersion && newDependencyManaged || (oldDependencyDefinedManaged && configuredToChangeManagedDependency)) {
                                     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);
                                 }
-                            } 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.
+                            } else if (configuredToOverrideManagedVersion || (!newDependencyManaged && !(oldDependencyDefinedManaged && configuredToChangeManagedDependency))) {
+                                // 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());
+                            } else if (!newDependencyManaged) {
+                                // We leave it up to the managed dependency update to call `maybeUpdateModel()` instead to avoid interim dependency resolution failure
+                                deferUpdate = true;
                             }
                         } catch (MavenDownloadingException e) {
                             return e.warn(tag);
                         }
                     }
 
-                    if (t != tag) {
+                    if (t != tag && !deferUpdate) {
                         maybeUpdateModel();
                     }
                 }
@@ -227,7 +241,7 @@ public Xml visitTag(Xml.Tag tag, ExecutionContext ctx) {
                 return t;
             }
 
-            private boolean checkIfNewDependencyPresents(@Nullable String groupId, @Nullable String artifactId, @Nullable String version) {
+            private boolean checkIfNewDependencyPresent(@Nullable String groupId, @Nullable String artifactId, @Nullable String version) {
                 if ((groupId == null) || (artifactId == null)) {
                     return false;
                 }
@@ -248,6 +262,23 @@ private boolean isDependencyManaged(Scope scope, String groupId, String artifact
                 return false;
             }
 
+            private boolean canAffectManagedDependency(MavenResolutionResult result, Scope scope, String groupId, String artifactId) {
+                // We're only going to be able to effect managed dependencies that are either direct or are brought in as direct via a local parent
+                // `ChangeManagedDependencyGroupIdAndArtifactId` cannot manipulate BOM imported managed dependencies nor direct dependencies from remote parents
+                Pom requestedPom = result.getPom().getRequested();
+                for (ManagedDependency requestedManagedDependency : requestedPom.getDependencyManagement()) {
+                    if (groupId.equals(requestedManagedDependency.getGroupId()) && artifactId.equals(requestedManagedDependency.getArtifactId())) {
+                        if (requestedManagedDependency instanceof ManagedDependency.Defined) {
+                            return scope.isInClasspathOf(Scope.fromName(((ManagedDependency.Defined) requestedManagedDependency).getScope()));
+                        }
+                    }
+                }
+                if (result.parentPomIsProjectPom() && result.getParent() != null) {
+                    return canAffectManagedDependency(result.getParent(), scope, groupId, artifactId);
+                }
+                return false;
+            }
+
             @SuppressWarnings("ConstantConditions")
             private String resolveSemverVersion(ExecutionContext ctx, String groupId, String artifactId, @Nullable String currentVersion) throws MavenDownloadingException {
                 if (versionComparator == null) {

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.AddToTagVisitor;
 import org.openrewrite.xml.RemoveContentVisitor;
 import org.openrewrite.xml.tree.Xml;
 
@@ -128,7 +129,7 @@ public TreeVisitor<?, ExecutionContext> getVisitor() {
 
             @Override
             public Xml.Document visitDocument(Xml.Document document, ExecutionContext ctx) {
-                isNewDependencyPresent = checkIfNewDependencyPresents(newGroupId, newArtifactId, newVersion);
+                isNewDependencyPresent = checkIfNewDependencyPresent(newGroupId, newArtifactId, newVersion);
                 return super.visitDocument(document, ctx);
             }
 
@@ -154,6 +155,9 @@ public Xml.Tag visitTag(Xml.Tag tag, ExecutionContext ctx) {
                                 }
                                 String resolvedNewVersion = resolveSemverVersion(ctx, newGroupId, resolvedArtifactId, getResolutionResult().getPom().getValue(versionTag.get().getValue().orElse(null)));
                                 t = changeChildTagValue(t, "version", resolvedNewVersion, ctx);
+                            } else {
+                                Xml.Tag newChild = Xml.Tag.build("<version>" + newVersion + "</version>");
+                                t = (Xml.Tag) new AddToTagVisitor<ExecutionContext>(t, newChild, new MavenTagInsertionComparator(t.getChildren())).visitNonNull(t, ctx, getCursor().getParentOrThrow());
                             }
                         } catch (MavenDownloadingException e) {
                             return e.warn(t);
@@ -171,7 +175,7 @@ public Xml.Tag visitTag(Xml.Tag tag, ExecutionContext ctx) {
                 return t;
             }
 
-            private boolean checkIfNewDependencyPresents(@Nullable String groupId, @Nullable String artifactId, @Nullable String version) {
+            private boolean checkIfNewDependencyPresent(@Nullable String groupId, @Nullable String artifactId, @Nullable String version) {
                 if ((groupId == null) || (artifactId == null)) {
                     return false;
                 }