From 646e9084141ff21b48a07f6553bebac1f04da9cf Mon Sep 17 00:00:00 2001 From: martinfrancois Date: Mon, 10 Aug 2026 13:25:07 +0200 Subject: [PATCH 1/3] DependencyManagementDependencyRequiresVersion: remove only inert entries The recipe removed every managed dependency without a ``, on the premise that such an entry cannot affect resolution anywhere. Maven does not honor that premise. An entry without a version still manages `scope`, `exclusions`, `optional` and `systemPath` for a dependency versioned elsewhere, and because Maven merges dependency management one entry at a time on the management key rather than field by field, an entry declaring only coordinates hides, rather than inherits from, a same-key entry coming from a parent or an imported BOM. Removing it silently widened effective scope and dropped exclusions. An entry is now removed only when it declares nothing but `groupId` and `artifactId`, both resolve, and no entry for that key can be hidden: the POM is not one that others inherit from or import, its parent's effective management does not cover the key, no ancestor declares dependency management in a profile or is unresolved here, neither the POM nor its profiles import a BOM, and no sibling entry shares the key. The display description and the generated recipes.csv row are updated to match. The recipe is now strictly more conservative, so it also keeps entries that happen to be inert but whose surroundings cannot be established from the POM under review. The one hidden entry it still cannot see is a BOM import of a POM packaged as something other than `pom`, since only the parent relation is recorded on either side. The existing test for issue 1084 is untouched and still passes: a coordinates-only entry in a standalone POM is still removed. --- ...cyManagementDependencyRequiresVersion.java | 137 ++- .../resources/META-INF/rewrite/recipes.csv | 2 +- .../ManagedDependencyRequiresVersionTest.java | 783 ++++++++++++++++++ 3 files changed, 919 insertions(+), 3 deletions(-) diff --git a/rewrite-maven/src/main/java/org/openrewrite/maven/cleanup/DependencyManagementDependencyRequiresVersion.java b/rewrite-maven/src/main/java/org/openrewrite/maven/cleanup/DependencyManagementDependencyRequiresVersion.java index 9beaaab0188..1f3480c592f 100644 --- a/rewrite-maven/src/main/java/org/openrewrite/maven/cleanup/DependencyManagementDependencyRequiresVersion.java +++ b/rewrite-maven/src/main/java/org/openrewrite/maven/cleanup/DependencyManagementDependencyRequiresVersion.java @@ -16,31 +16,164 @@ package org.openrewrite.maven.cleanup; import lombok.Getter; +import org.jspecify.annotations.Nullable; import org.openrewrite.ExecutionContext; import org.openrewrite.Recipe; import org.openrewrite.TreeVisitor; import org.openrewrite.maven.MavenIsoVisitor; +import org.openrewrite.maven.tree.ManagedDependency; +import org.openrewrite.maven.tree.MavenResolutionResult; +import org.openrewrite.maven.tree.Pom; +import org.openrewrite.maven.tree.Profile; +import org.openrewrite.maven.tree.ResolvedGroupArtifactVersion; import org.openrewrite.xml.RemoveContentVisitor; import org.openrewrite.xml.tree.Xml; +import java.util.HashSet; +import java.util.List; +import java.util.Set; + public class DependencyManagementDependencyRequiresVersion extends Recipe { @Getter final String displayName = "Dependency management dependencies should have a version"; @Getter - final String description = "If they don't have a version, they can't possibly affect dependency resolution anywhere, and can be safely removed."; + final String description = "A dependency management entry that declares nothing but its coordinates manages " + + "nothing of its own, so it can be removed wherever it also hides no entry for the same coordinates " + + "declared elsewhere. A missing `version` alone is not enough: an entry can still manage `scope`, " + + "`exclusions`, `optional` or `systemPath` for a dependency versioned elsewhere. Maven also merges " + + "dependency management one entry at a time on the management key rather than field by field, so an " + + "entry declaring only coordinates hides, rather than inherits from, an entry for the same coordinates " + + "coming from a parent, from an imported BOM, or, once the POM also inherits dependency management, " + + "from an earlier entry of the same POM. Entries are only removed where no such entry can be hidden, " + + "which leaves parent and BOM POMs alone."; @Override public TreeVisitor getVisitor() { return new MavenIsoVisitor() { @Override public Xml.Tag visitTag(Xml.Tag tag, ExecutionContext ctx) { - if (isManagedDependencyTag() && tag.getChildValue("version").orElse(null) == null) { + if (isManagedDependencyTag() && isInert(tag)) { doAfterVisit(new RemoveContentVisitor<>(tag, true, true)); } return super.visitTag(tag, ctx); } + + /** + * A managed dependency is provably inert only when it declares its coordinates and nothing else, and + * nothing else manages those coordinates. A missing {@code version} on its own proves nothing, as the + * entry may still manage {@code scope}, {@code exclusions}, {@code optional} or {@code systemPath} for a + * dependency versioned elsewhere, and {@code type} and {@code classifier} decide which dependencies it + * applies to. + */ + private boolean isInert(Xml.Tag tag) { + for (Xml.Tag child : tag.getChildren()) { + if (!"groupId".equals(child.getName()) && !"artifactId".equals(child.getName())) { + return false; + } + } + String groupId = resolve(tag, "groupId"); + String artifactId = resolve(tag, "artifactId"); + return groupId != null && artifactId != null && !managedElsewhere(tag, groupId, artifactId); + } + + /** + * Maven merges dependency management one entry at a time on the management key rather than field by + * field, so an entry declaring only coordinates hides, rather than inherits from, an entry for the same + * key coming from a parent or an imported BOM, and, once this POM also inherits dependency management, + * an earlier entry of the same POM; removing it lets the hidden entry take effect. Only management that + * this POM can see is provably absent, so a POM that others inherit from or import is left alone: what a + * consumer manages for these coordinates cannot be established from here. The one consumer this cannot + * recognize is a project that imports a POM packaged as anything but {@code pom} as a BOM, in this + * repository or outside it, since only the parent relation is recorded on either side. + */ + private boolean managedElsewhere(Xml.Tag tag, String groupId, String artifactId) { + if ("pom".equals(getResolutionResult().getPom().getPackaging()) || !getResolutionResult().getModules().isEmpty()) { + return true; + } + Pom pom = getResolutionResult().getPom().getRequested(); + if (pom.getParent() != null) { + MavenResolutionResult parent = getResolutionResult().getParent(); + // A parent outside this repository is not resolved here, so what it manages is unknown. + if (parent == null || parent.getPom().getManagedDependency(groupId, artifactId, null, null) != null) { + return true; + } + Set visited = new HashSet<>(); + for (MavenResolutionResult ancestor = parent; ancestor != null; ancestor = ancestor.getParent()) { + if (!visited.add(ancestor.getPom().getGav())) { + // A cyclic ancestry is not a model that can be reasoned about. + return true; + } + // A resolved ancestor only reflects the profiles that were active when it was parsed, and an + // ancestor's profile does not take precedence over the entry under review. + if (declaresProfileDependencyManagement(ancestor.getPom().getRequested())) { + return true; + } + if (ancestor.getPom().getRequested().getParent() != null && ancestor.getParent() == null) { + // The ancestry leaves this repository, so profiles further up cannot be inspected either. + return true; + } + } + } + // Dependency management this POM declares in a profile takes precedence over the entry under review + // whenever that profile is active, so only what its BOM imports might manage is unknown. + if (importsBom(pom.getDependencyManagement())) { + return true; + } + for (Profile profile : pom.getProfiles()) { + if (importsBom(profile.getDependencyManagement())) { + return true; + } + } + // Once this POM also inherits dependency management, Maven's entry-wise merge collapses duplicate + // management keys to the last entry, so removing this entry can change which sibling takes effect. + // Maven itself flags duplicate keys ("must be unique"), so any duplicate is conservatively kept. + Xml.Tag dependencies = getCursor().getParentOrThrow().firstEnclosingOrThrow(Xml.Tag.class); + for (Xml.Tag sibling : dependencies.getChildren("dependency")) { + if (sibling == tag) { + continue; + } + String siblingGroupId = resolve(sibling, "groupId"); + String siblingArtifactId = resolve(sibling, "artifactId"); + if (siblingGroupId == null || siblingArtifactId == null || + (groupId.equals(siblingGroupId) && artifactId.equals(siblingArtifactId))) { + return true; + } + } + return false; + } + + /** + * @return The child element's value with properties resolved, or {@code null} when the element is absent + * or its properties cannot be resolved here, as an unresolvable coordinate leaves it unknown what the + * entry hides. + */ + private @Nullable String resolve(Xml.Tag tag, String childName) { + String value = getResolutionResult().getPom().getValue(tag.getChildValue(childName).orElse(null)); + return value == null || value.contains("${") ? null : value; + } }; } + + private static boolean importsBom(@Nullable List dependencyManagement) { + if (dependencyManagement != null) { + for (ManagedDependency managed : dependencyManagement) { + if (managed instanceof ManagedDependency.Imported) { + return true; + } + } + } + return false; + } + + private static boolean declaresProfileDependencyManagement(Pom pom) { + for (Profile profile : pom.getProfiles()) { + List dependencyManagement = profile.getDependencyManagement(); + if (dependencyManagement != null && !dependencyManagement.isEmpty()) { + return true; + } + } + return false; + } } diff --git a/rewrite-maven/src/main/resources/META-INF/rewrite/recipes.csv b/rewrite-maven/src/main/resources/META-INF/rewrite/recipes.csv index ea5ea1ef0dc..490765354a1 100644 --- a/rewrite-maven/src/main/resources/META-INF/rewrite/recipes.csv +++ b/rewrite-maven/src/main/resources/META-INF/rewrite/recipes.csv @@ -78,7 +78,7 @@ maven,org.openrewrite:rewrite-maven,org.openrewrite.maven.UpgradeTransitiveDepen maven,org.openrewrite:rewrite-maven,org.openrewrite.maven.UseMavenCompilerPluginReleaseConfiguration,Use Maven compiler plugin release configuration,"Replaces any explicit `source` or `target` configuration (if present) on the `maven-compiler-plugin` with `release`, and updates the `release` value if needed. When `testSource` or `testTarget` differ from the main version, introduces `testRelease`. Will not downgrade the Java version if the current version is higher. Also removes stale `maven.compiler.source`, `maven.compiler.target`, `maven.compiler.testSource`, and `maven.compiler.testTarget` properties that are no longer referenced.",1,,Maven,"[{""name"":""releaseVersion"",""type"":""Integer"",""displayName"":""Release version"",""description"":""The new value for the release configuration. This recipe prefers ${java.version} if defined."",""example"":""11"",""required"":true}]", maven,org.openrewrite:rewrite-maven,org.openrewrite.maven.UseParentInference,Use Maven 4 parent inference,"Maven 4.1.0 supports automatic parent version inference when using a relative path. This recipe simplifies parent declarations by using the shorthand `` form when the parent is in the default location (`..`), removing the explicit ``, ``, ``, and `` elements. Maven automatically infers these values from the parent POM.",1,,Maven,, maven,org.openrewrite:rewrite-maven,org.openrewrite.maven.cleanup.AddProjectBuildOutputTimestamp,Add `project.build.outputTimestamp` for reproducible builds,"Adds the `project.build.outputTimestamp` property, which Maven uses to make build outputs reproducible by stamping archive entries with a fixed timestamp instead of the current time. An existing value is preserved. See [Configuring for Reproducible Builds](https://maven.apache.org/guides/mini/guide-reproducible-builds.html).",2,Cleanup,Maven,"[{""name"":""timestamp"",""type"":""String"",""displayName"":""Timestamp"",""description"":""ISO 8601 timestamp, integer seconds since the epoch, or property reference such as `${git.commit.author.time}`. Defaults to `1980-01-01T00:00:00Z`, the earliest value the ZIP format can represent."",""example"":""2024-01-01T00:00:00Z""}]", -maven,org.openrewrite:rewrite-maven,org.openrewrite.maven.cleanup.DependencyManagementDependencyRequiresVersion,Dependency management dependencies should have a version,"If they don't have a version, they can't possibly affect dependency resolution anywhere, and can be safely removed.",1,Cleanup,Maven,, +maven,org.openrewrite:rewrite-maven,org.openrewrite.maven.cleanup.DependencyManagementDependencyRequiresVersion,Dependency management dependencies should have a version,"A dependency management entry that declares nothing but its coordinates manages nothing of its own, so it can be removed wherever it also hides no entry for the same coordinates declared elsewhere. A missing `version` alone is not enough: an entry can still manage `scope`, `exclusions`, `optional` or `systemPath` for a dependency versioned elsewhere. Maven also merges dependency management one entry at a time on the management key rather than field by field, so an entry declaring only coordinates hides, rather than inherits from, an entry for the same coordinates coming from a parent, from an imported BOM, or, once the POM also inherits dependency management, from an earlier entry of the same POM. Entries are only removed where no such entry can be hidden, which leaves parent and BOM POMs alone.",1,Cleanup,Maven,, maven,org.openrewrite:rewrite-maven,org.openrewrite.maven.cleanup.ExplicitDependencyVersion,Add explicit dependency versions,"Add explicit dependency versions to POMs for reproducibility, as the `LATEST` and `RELEASE` version keywords are deprecated.",1,Cleanup,Maven,,"[{""name"":""org.openrewrite.maven.table.MavenMetadataFailures"",""displayName"":""Maven metadata failures"",""instanceName"":""Maven metadata failures"",""description"":""Attempts to resolve maven metadata that failed."",""columns"":[{""name"":""group"",""type"":""String"",""displayName"":""Group id"",""description"":""The groupId of the artifact for which the metadata download failed.""},{""name"":""artifactId"",""type"":""String"",""displayName"":""Artifact id"",""description"":""The artifactId of the artifact for which the metadata download failed.""},{""name"":""version"",""type"":""String"",""displayName"":""Version"",""description"":""The version of the artifact for which the metadata download failed.""},{""name"":""mavenRepositoryUri"",""type"":""String"",""displayName"":""Maven repository"",""description"":""The URL of the Maven repository that the metadata download failed on.""},{""name"":""snapshots"",""type"":""String"",""displayName"":""Snapshots"",""description"":""Does the repository support snapshots.""},{""name"":""releases"",""type"":""String"",""displayName"":""Releases"",""description"":""Does the repository support releases.""},{""name"":""failure"",""type"":""String"",""displayName"":""Failure"",""description"":""The reason the metadata download failed.""}]}]" maven,org.openrewrite:rewrite-maven,org.openrewrite.maven.cleanup.ExplicitPluginGroupId,Add explicit `groupId` to Maven plugins,Add the default `org.apache.maven.plugins` to plugins for clarity.,1,Cleanup,Maven,, maven,org.openrewrite:rewrite-maven,org.openrewrite.maven.cleanup.ExplicitPluginVersion,Add explicit plugin versions,"Add explicit plugin versions to POMs for reproducibility, as [MNG-4173](https://issues.apache.org/jira/browse/MNG-4173) removes automatic version resolution for POM plugins.",1,Cleanup,Maven,, diff --git a/rewrite-maven/src/test/java/org/openrewrite/maven/cleanup/ManagedDependencyRequiresVersionTest.java b/rewrite-maven/src/test/java/org/openrewrite/maven/cleanup/ManagedDependencyRequiresVersionTest.java index ec129a9c9c5..695f66fc67a 100644 --- a/rewrite-maven/src/test/java/org/openrewrite/maven/cleanup/ManagedDependencyRequiresVersionTest.java +++ b/rewrite-maven/src/test/java/org/openrewrite/maven/cleanup/ManagedDependencyRequiresVersionTest.java @@ -16,9 +16,11 @@ package org.openrewrite.maven.cleanup; import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.Timeout; import org.openrewrite.Issue; import org.openrewrite.test.RewriteTest; +import static org.openrewrite.java.Assertions.mavenProject; import static org.openrewrite.maven.Assertions.pomXml; class ManagedDependencyRequiresVersionTest implements RewriteTest { @@ -53,4 +55,785 @@ void dependencyManagementDependencyRequiresVersion() { ) ); } + + @Test + void managedScopeAndExclusionsFromParentManagedVersion() { + rewriteRun( + spec -> spec.recipe(new DependencyManagementDependencyRequiresVersion()), + mavenProject("parent", + pomXml( + """ + + com.example + parent + 1 + pom + + child + + + + + com.google.guava + guava + 33.4.8-jre + + + + + """ + ), + mavenProject("child", + pomXml( + """ + + + com.example + parent + 1 + + child + + + + com.google.guava + guava + runtime + + + com.google.code.findbugs + jsr305 + + + + + + + + com.google.guava + guava + 33.4.8-jre + + + + """ + ) + ) + ) + ); + } + + @Test + void coordinatesOnlyEntryHidesParentManagedVersion() { + rewriteRun( + spec -> spec.recipe(new DependencyManagementDependencyRequiresVersion()), + mavenProject("parent", + pomXml( + """ + + com.example + parent + 1 + pom + + child + + + + + com.google.guava + guava + 33.4.8-jre + + + + + """ + ), + mavenProject("child", + pomXml( + """ + + + com.example + parent + 1 + + child + + + + com.google.guava + guava + + + + + """ + ) + ) + ) + ); + } + + @Test + void coordinatesOnlyEntryWhenTheParentManagesNothingForIt() { + rewriteRun( + spec -> spec.recipe(new DependencyManagementDependencyRequiresVersion()), + mavenProject("parent", + pomXml( + """ + + com.example + parent + 1 + pom + + child + + + + + com.fasterxml.jackson.core + jackson-core + 2.19.0 + + + + + """ + ), + mavenProject("child", + pomXml( + """ + + + com.example + parent + 1 + + child + + + + com.google.guava + guava + + + + + """, + """ + + + com.example + parent + 1 + + child + + """ + ) + ) + ) + ); + } + + @Test + void coordinatesOnlyEntryMayHideAnImportedBom() { + rewriteRun( + spec -> spec.recipe(new DependencyManagementDependencyRequiresVersion()), + mavenProject("bom", + pomXml( + """ + + com.example + bom + 1 + pom + + + + com.google.guava + guava + 33.4.8-jre + + + + + """ + ) + ), + mavenProject("app", + pomXml( + """ + + com.example + app + 1 + + + + com.example + bom + 1 + pom + import + + + com.google.guava + guava + + + + + """ + ) + ) + ); + } + + @Test + void coordinatesOnlyEntryWhenTheParentManagesInAnInactiveProfile() { + rewriteRun( + spec -> spec.recipe(new DependencyManagementDependencyRequiresVersion()), + mavenProject("parent", + pomXml( + """ + + com.example + parent + 1 + pom + + child + + + + pinned + + + + com.google.guava + guava + 33.4.8-jre + + + + + + + """ + ), + mavenProject("child", + pomXml( + """ + + + com.example + parent + 1 + + child + + + + com.google.guava + guava + + + + + """ + ) + ) + ) + ); + } + + @Test + void coordinatesOnlyEntryInAPomAnotherModuleDeclaresAsItsParent() { + rewriteRun( + spec -> spec.recipe(new DependencyManagementDependencyRequiresVersion()), + mavenProject("parent", + pomXml( + """ + + com.example + parent + 1 + + + + com.google.guava + guava + + + + + """ + ), + mavenProject("child", + pomXml( + """ + + + com.example + parent + 1 + + child + + """ + ) + ) + ) + ); + } + + // Without a guard against a cyclic ancestry the walk below never terminates, so fail rather than hang. + @Timeout(30) + @Test + void coordinatesOnlyEntryInAPomWithACyclicAncestry() { + rewriteRun( + spec -> spec.recipe(new DependencyManagementDependencyRequiresVersion()), + mavenProject("a", + pomXml( + """ + + + com.example + b + 1 + + a + + """ + ) + ), + mavenProject("b", + pomXml( + """ + + + com.example + a + 1 + + b + + """ + ) + ), + mavenProject("leaf", + pomXml( + """ + + + com.example + a + 1 + + leaf + + + + com.google.guava + guava + + + + + """ + ) + ) + ); + } + + @Test + void coordinatesOnlyEntryBesideASiblingWithAnUnresolvableProperty() { + rewriteRun( + spec -> spec.recipe(new DependencyManagementDependencyRequiresVersion()), + pomXml( + """ + + com.example + app + 1 + + + + com.google.guava + guava + + + ${guava.group} + guava + 33.4.8-jre + + + + + """ + ) + ); + } + + @Test + void coordinatesOnlyEntryInAPomThatOthersInheritFromOrImport() { + rewriteRun( + spec -> spec.recipe(new DependencyManagementDependencyRequiresVersion()), + pomXml( + """ + + com.example + parent + 1 + pom + + + + com.google.guava + guava + + + + + """ + ) + ); + } + + @Test + void coordinatesOnlyEntryWithAnUnresolvableProperty() { + rewriteRun( + spec -> spec.recipe(new DependencyManagementDependencyRequiresVersion()), + pomXml( + """ + + com.example + app + 1 + + + + ${guava.group} + guava + + + + + """ + ) + ); + } + + @Test + void coordinatesOnlyEntryWithAResolvableProperty() { + rewriteRun( + spec -> spec.recipe(new DependencyManagementDependencyRequiresVersion()), + pomXml( + """ + + com.example + app + 1 + + com.google.guava + + + + + ${guava.group} + guava + + + + + """, + """ + + com.example + app + 1 + + com.google.guava + + + """ + ) + ); + } + + @Test + void coordinatesOnlyEntryBesideADuplicateInTheSamePom() { + rewriteRun( + spec -> spec.recipe(new DependencyManagementDependencyRequiresVersion()), + pomXml( + """ + + com.example + app + 1 + + + + com.google.guava + guava + + + com.google.guava + guava + 33.4.8-jre + + + + + """ + ) + ); + } + + @Test + void managedScope() { + rewriteRun( + spec -> spec.recipe(new DependencyManagementDependencyRequiresVersion()), + pomXml( + """ + + com.example + app + 1 + + + + com.google.guava + guava + runtime + + + + + + com.google.guava + guava + 33.4.8-jre + + + + """ + ) + ); + } + + @Test + void managedExclusions() { + rewriteRun( + spec -> spec.recipe(new DependencyManagementDependencyRequiresVersion()), + pomXml( + """ + + com.example + app + 1 + + + + com.google.guava + guava + + + com.google.code.findbugs + jsr305 + + + + + + + + com.google.guava + guava + 33.4.8-jre + + + + """ + ) + ); + } + + @Test + void managedOptional() { + rewriteRun( + spec -> spec.recipe(new DependencyManagementDependencyRequiresVersion()), + pomXml( + """ + + com.example + app + 1 + + + + com.google.guava + guava + true + + + + + + com.google.guava + guava + 33.4.8-jre + + + + """ + ) + ); + } + + @Test + void declaresMoreThanCoordinates() { + rewriteRun( + spec -> spec.recipe(new DependencyManagementDependencyRequiresVersion()), + pomXml( + """ + + com.example + app + 1 + + + + com.google.guava + guava + test-jar + tests + + + + + """ + ) + ); + } + + @Test + void incompleteCoordinates() { + rewriteRun( + spec -> spec.recipe(new DependencyManagementDependencyRequiresVersion()), + pomXml( + """ + + com.example + app + 1 + + + + com.google.guava + + + + + """ + ) + ); + } + + @Test + void removesOnlyTheEntryThatManagesNothing() { + rewriteRun( + spec -> spec.recipe(new DependencyManagementDependencyRequiresVersion()), + pomXml( + """ + + com.example + app + 1 + + + + com.google.guava + guava + runtime + + + com.fasterxml.jackson.core + jackson-core + + + + + + com.google.guava + guava + 33.4.8-jre + + + + """, + """ + + com.example + app + 1 + + + + com.google.guava + guava + runtime + + + + + + com.google.guava + guava + 33.4.8-jre + + + + """ + ) + ); + } + + @Test + void propertyVersion() { + rewriteRun( + spec -> spec.recipe(new DependencyManagementDependencyRequiresVersion()), + pomXml( + """ + + com.example + app + 1 + + 33.4.8-jre + + + + + com.google.guava + guava + ${guava.version} + + + + + + com.google.guava + guava + + + + """ + ) + ); + } } From e73711dc19a124a20433ea460221b5291dd4eed4 Mon Sep 17 00:00:00 2001 From: Tim te Beek Date: Tue, 11 Aug 2026 23:32:49 +0200 Subject: [PATCH 2/3] Rename to match the narrowed behaviour, cover profile BOM imports, trim commentary The display name promised a version on every entry, which the recipe no longer enforces. Also simplifies the sibling lookup and hoists the recipe into defaults(RecipeSpec). --- ...cyManagementDependencyRequiresVersion.java | 60 ++++++++--------- .../resources/META-INF/rewrite/recipes.csv | 2 +- .../ManagedDependencyRequiresVersionTest.java | 66 +++++++++++++------ 3 files changed, 73 insertions(+), 55 deletions(-) diff --git a/rewrite-maven/src/main/java/org/openrewrite/maven/cleanup/DependencyManagementDependencyRequiresVersion.java b/rewrite-maven/src/main/java/org/openrewrite/maven/cleanup/DependencyManagementDependencyRequiresVersion.java index 1f3480c592f..0b1f0a043c0 100644 --- a/rewrite-maven/src/main/java/org/openrewrite/maven/cleanup/DependencyManagementDependencyRequiresVersion.java +++ b/rewrite-maven/src/main/java/org/openrewrite/maven/cleanup/DependencyManagementDependencyRequiresVersion.java @@ -36,18 +36,16 @@ public class DependencyManagementDependencyRequiresVersion extends Recipe { @Getter - final String displayName = "Dependency management dependencies should have a version"; + final String displayName = "Remove dependency management entries that manage nothing"; @Getter - final String description = "A dependency management entry that declares nothing but its coordinates manages " + - "nothing of its own, so it can be removed wherever it also hides no entry for the same coordinates " + - "declared elsewhere. A missing `version` alone is not enough: an entry can still manage `scope`, " + - "`exclusions`, `optional` or `systemPath` for a dependency versioned elsewhere. Maven also merges " + - "dependency management one entry at a time on the management key rather than field by field, so an " + - "entry declaring only coordinates hides, rather than inherits from, an entry for the same coordinates " + - "coming from a parent, from an imported BOM, or, once the POM also inherits dependency management, " + - "from an earlier entry of the same POM. Entries are only removed where no such entry can be hidden, " + - "which leaves parent and BOM POMs alone."; + final String description = "A dependency management entry declaring nothing but its coordinates manages nothing " + + "of its own. A missing `version` alone is not enough, as an entry can still manage `scope`, `exclusions`, " + + "`optional` or `systemPath` for a dependency versioned elsewhere. Maven also merges dependency management " + + "one entry at a time on the management key rather than field by field, so such an entry hides, rather than " + + "inherits from, an entry for the same coordinates coming from a parent, from an imported BOM, or from an " + + "earlier entry of the same POM. Entries are removed only where no such entry can be hidden, which leaves " + + "parent and BOM POMs alone."; @Override public TreeVisitor getVisitor() { @@ -61,11 +59,10 @@ public Xml.Tag visitTag(Xml.Tag tag, ExecutionContext ctx) { } /** - * A managed dependency is provably inert only when it declares its coordinates and nothing else, and - * nothing else manages those coordinates. A missing {@code version} on its own proves nothing, as the - * entry may still manage {@code scope}, {@code exclusions}, {@code optional} or {@code systemPath} for a - * dependency versioned elsewhere, and {@code type} and {@code classifier} decide which dependencies it - * applies to. + * An entry is provably inert only when it declares its coordinates and nothing else, and nothing else + * manages those coordinates. A missing {@code version} proves nothing on its own: the entry may still + * manage {@code scope}, {@code exclusions}, {@code optional} or {@code systemPath} for a dependency + * versioned elsewhere, and {@code type} and {@code classifier} decide which dependencies it applies to. */ private boolean isInert(Xml.Tag tag) { for (Xml.Tag child : tag.getChildren()) { @@ -79,14 +76,12 @@ private boolean isInert(Xml.Tag tag) { } /** - * Maven merges dependency management one entry at a time on the management key rather than field by - * field, so an entry declaring only coordinates hides, rather than inherits from, an entry for the same - * key coming from a parent or an imported BOM, and, once this POM also inherits dependency management, - * an earlier entry of the same POM; removing it lets the hidden entry take effect. Only management that - * this POM can see is provably absent, so a POM that others inherit from or import is left alone: what a - * consumer manages for these coordinates cannot be established from here. The one consumer this cannot - * recognize is a project that imports a POM packaged as anything but {@code pom} as a BOM, in this - * repository or outside it, since only the parent relation is recorded on either side. + * Because Maven merges dependency management per entry rather than per field, an entry declaring only + * coordinates hides an entry for the same key coming from a parent, an imported BOM or a sibling entry, + * and removing it lets that hidden entry take effect. Only management this POM can see is provably + * absent, so a POM others inherit from or import is left alone. The one consumer this cannot recognize + * is a project importing a POM packaged as anything but {@code pom} as a BOM, since only the parent + * relation is recorded on either side. */ private boolean managedElsewhere(Xml.Tag tag, String groupId, String artifactId) { if ("pom".equals(getResolutionResult().getPom().getPackaging()) || !getResolutionResult().getModules().isEmpty()) { @@ -105,8 +100,7 @@ private boolean managedElsewhere(Xml.Tag tag, String groupId, String artifactId) // A cyclic ancestry is not a model that can be reasoned about. return true; } - // A resolved ancestor only reflects the profiles that were active when it was parsed, and an - // ancestor's profile does not take precedence over the entry under review. + // A resolved ancestor only reflects the profiles active when it was parsed if (declaresProfileDependencyManagement(ancestor.getPom().getRequested())) { return true; } @@ -116,8 +110,8 @@ private boolean managedElsewhere(Xml.Tag tag, String groupId, String artifactId) } } } - // Dependency management this POM declares in a profile takes precedence over the entry under review - // whenever that profile is active, so only what its BOM imports might manage is unknown. + // This POM's own profiles take precedence over the entry under review, so only what a BOM they + // import might manage is unknown if (importsBom(pom.getDependencyManagement())) { return true; } @@ -126,10 +120,9 @@ private boolean managedElsewhere(Xml.Tag tag, String groupId, String artifactId) return true; } } - // Once this POM also inherits dependency management, Maven's entry-wise merge collapses duplicate - // management keys to the last entry, so removing this entry can change which sibling takes effect. - // Maven itself flags duplicate keys ("must be unique"), so any duplicate is conservatively kept. - Xml.Tag dependencies = getCursor().getParentOrThrow().firstEnclosingOrThrow(Xml.Tag.class); + // The entry-wise merge collapses duplicate management keys to the last entry, so removing this one + // can change which sibling takes effect; Maven flags duplicates itself ("must be unique") + Xml.Tag dependencies = getCursor().getParentOrThrow().getValue(); for (Xml.Tag sibling : dependencies.getChildren("dependency")) { if (sibling == tag) { continue; @@ -145,9 +138,8 @@ private boolean managedElsewhere(Xml.Tag tag, String groupId, String artifactId) } /** - * @return The child element's value with properties resolved, or {@code null} when the element is absent - * or its properties cannot be resolved here, as an unresolvable coordinate leaves it unknown what the - * entry hides. + * @return The child element's value with properties resolved, or {@code null} when it is absent or + * unresolvable, which leaves it unknown what the entry hides. */ private @Nullable String resolve(Xml.Tag tag, String childName) { String value = getResolutionResult().getPom().getValue(tag.getChildValue(childName).orElse(null)); diff --git a/rewrite-maven/src/main/resources/META-INF/rewrite/recipes.csv b/rewrite-maven/src/main/resources/META-INF/rewrite/recipes.csv index 490765354a1..6217568b4d9 100644 --- a/rewrite-maven/src/main/resources/META-INF/rewrite/recipes.csv +++ b/rewrite-maven/src/main/resources/META-INF/rewrite/recipes.csv @@ -78,7 +78,7 @@ maven,org.openrewrite:rewrite-maven,org.openrewrite.maven.UpgradeTransitiveDepen maven,org.openrewrite:rewrite-maven,org.openrewrite.maven.UseMavenCompilerPluginReleaseConfiguration,Use Maven compiler plugin release configuration,"Replaces any explicit `source` or `target` configuration (if present) on the `maven-compiler-plugin` with `release`, and updates the `release` value if needed. When `testSource` or `testTarget` differ from the main version, introduces `testRelease`. Will not downgrade the Java version if the current version is higher. Also removes stale `maven.compiler.source`, `maven.compiler.target`, `maven.compiler.testSource`, and `maven.compiler.testTarget` properties that are no longer referenced.",1,,Maven,"[{""name"":""releaseVersion"",""type"":""Integer"",""displayName"":""Release version"",""description"":""The new value for the release configuration. This recipe prefers ${java.version} if defined."",""example"":""11"",""required"":true}]", maven,org.openrewrite:rewrite-maven,org.openrewrite.maven.UseParentInference,Use Maven 4 parent inference,"Maven 4.1.0 supports automatic parent version inference when using a relative path. This recipe simplifies parent declarations by using the shorthand `` form when the parent is in the default location (`..`), removing the explicit ``, ``, ``, and `` elements. Maven automatically infers these values from the parent POM.",1,,Maven,, maven,org.openrewrite:rewrite-maven,org.openrewrite.maven.cleanup.AddProjectBuildOutputTimestamp,Add `project.build.outputTimestamp` for reproducible builds,"Adds the `project.build.outputTimestamp` property, which Maven uses to make build outputs reproducible by stamping archive entries with a fixed timestamp instead of the current time. An existing value is preserved. See [Configuring for Reproducible Builds](https://maven.apache.org/guides/mini/guide-reproducible-builds.html).",2,Cleanup,Maven,"[{""name"":""timestamp"",""type"":""String"",""displayName"":""Timestamp"",""description"":""ISO 8601 timestamp, integer seconds since the epoch, or property reference such as `${git.commit.author.time}`. Defaults to `1980-01-01T00:00:00Z`, the earliest value the ZIP format can represent."",""example"":""2024-01-01T00:00:00Z""}]", -maven,org.openrewrite:rewrite-maven,org.openrewrite.maven.cleanup.DependencyManagementDependencyRequiresVersion,Dependency management dependencies should have a version,"A dependency management entry that declares nothing but its coordinates manages nothing of its own, so it can be removed wherever it also hides no entry for the same coordinates declared elsewhere. A missing `version` alone is not enough: an entry can still manage `scope`, `exclusions`, `optional` or `systemPath` for a dependency versioned elsewhere. Maven also merges dependency management one entry at a time on the management key rather than field by field, so an entry declaring only coordinates hides, rather than inherits from, an entry for the same coordinates coming from a parent, from an imported BOM, or, once the POM also inherits dependency management, from an earlier entry of the same POM. Entries are only removed where no such entry can be hidden, which leaves parent and BOM POMs alone.",1,Cleanup,Maven,, +maven,org.openrewrite:rewrite-maven,org.openrewrite.maven.cleanup.DependencyManagementDependencyRequiresVersion,Remove dependency management entries that manage nothing,"A dependency management entry declaring nothing but its coordinates manages nothing of its own. A missing `version` alone is not enough, as an entry can still manage `scope`, `exclusions`, `optional` or `systemPath` for a dependency versioned elsewhere. Maven also merges dependency management one entry at a time on the management key rather than field by field, so such an entry hides, rather than inherits from, an entry for the same coordinates coming from a parent, from an imported BOM, or from an earlier entry of the same POM. Entries are removed only where no such entry can be hidden, which leaves parent and BOM POMs alone.",1,Cleanup,Maven,, maven,org.openrewrite:rewrite-maven,org.openrewrite.maven.cleanup.ExplicitDependencyVersion,Add explicit dependency versions,"Add explicit dependency versions to POMs for reproducibility, as the `LATEST` and `RELEASE` version keywords are deprecated.",1,Cleanup,Maven,,"[{""name"":""org.openrewrite.maven.table.MavenMetadataFailures"",""displayName"":""Maven metadata failures"",""instanceName"":""Maven metadata failures"",""description"":""Attempts to resolve maven metadata that failed."",""columns"":[{""name"":""group"",""type"":""String"",""displayName"":""Group id"",""description"":""The groupId of the artifact for which the metadata download failed.""},{""name"":""artifactId"",""type"":""String"",""displayName"":""Artifact id"",""description"":""The artifactId of the artifact for which the metadata download failed.""},{""name"":""version"",""type"":""String"",""displayName"":""Version"",""description"":""The version of the artifact for which the metadata download failed.""},{""name"":""mavenRepositoryUri"",""type"":""String"",""displayName"":""Maven repository"",""description"":""The URL of the Maven repository that the metadata download failed on.""},{""name"":""snapshots"",""type"":""String"",""displayName"":""Snapshots"",""description"":""Does the repository support snapshots.""},{""name"":""releases"",""type"":""String"",""displayName"":""Releases"",""description"":""Does the repository support releases.""},{""name"":""failure"",""type"":""String"",""displayName"":""Failure"",""description"":""The reason the metadata download failed.""}]}]" maven,org.openrewrite:rewrite-maven,org.openrewrite.maven.cleanup.ExplicitPluginGroupId,Add explicit `groupId` to Maven plugins,Add the default `org.apache.maven.plugins` to plugins for clarity.,1,Cleanup,Maven,, maven,org.openrewrite:rewrite-maven,org.openrewrite.maven.cleanup.ExplicitPluginVersion,Add explicit plugin versions,"Add explicit plugin versions to POMs for reproducibility, as [MNG-4173](https://issues.apache.org/jira/browse/MNG-4173) removes automatic version resolution for POM plugins.",1,Cleanup,Maven,, diff --git a/rewrite-maven/src/test/java/org/openrewrite/maven/cleanup/ManagedDependencyRequiresVersionTest.java b/rewrite-maven/src/test/java/org/openrewrite/maven/cleanup/ManagedDependencyRequiresVersionTest.java index 695f66fc67a..c0d85108e76 100644 --- a/rewrite-maven/src/test/java/org/openrewrite/maven/cleanup/ManagedDependencyRequiresVersionTest.java +++ b/rewrite-maven/src/test/java/org/openrewrite/maven/cleanup/ManagedDependencyRequiresVersionTest.java @@ -18,17 +18,23 @@ import org.junit.jupiter.api.Test; import org.junit.jupiter.api.Timeout; import org.openrewrite.Issue; +import org.openrewrite.test.RecipeSpec; import org.openrewrite.test.RewriteTest; import static org.openrewrite.java.Assertions.mavenProject; import static org.openrewrite.maven.Assertions.pomXml; class ManagedDependencyRequiresVersionTest implements RewriteTest { + + @Override + public void defaults(RecipeSpec spec) { + spec.recipe(new DependencyManagementDependencyRequiresVersion()); + } + @Issue("https://github.com/openrewrite/rewrite/issues/1084") @Test void dependencyManagementDependencyRequiresVersion() { rewriteRun( - spec -> spec.recipe(new DependencyManagementDependencyRequiresVersion()), pomXml( """ @@ -59,7 +65,6 @@ void dependencyManagementDependencyRequiresVersion() { @Test void managedScopeAndExclusionsFromParentManagedVersion() { rewriteRun( - spec -> spec.recipe(new DependencyManagementDependencyRequiresVersion()), mavenProject("parent", pomXml( """ @@ -126,7 +131,6 @@ void managedScopeAndExclusionsFromParentManagedVersion() { @Test void coordinatesOnlyEntryHidesParentManagedVersion() { rewriteRun( - spec -> spec.recipe(new DependencyManagementDependencyRequiresVersion()), mavenProject("parent", pomXml( """ @@ -179,7 +183,6 @@ void coordinatesOnlyEntryHidesParentManagedVersion() { @Test void coordinatesOnlyEntryWhenTheParentManagesNothingForIt() { rewriteRun( - spec -> spec.recipe(new DependencyManagementDependencyRequiresVersion()), mavenProject("parent", pomXml( """ @@ -242,7 +245,6 @@ void coordinatesOnlyEntryWhenTheParentManagesNothingForIt() { @Test void coordinatesOnlyEntryMayHideAnImportedBom() { rewriteRun( - spec -> spec.recipe(new DependencyManagementDependencyRequiresVersion()), mavenProject("bom", pomXml( """ @@ -293,10 +295,48 @@ void coordinatesOnlyEntryMayHideAnImportedBom() { ); } + @Test + void coordinatesOnlyEntryMayHideABomImportedInAProfile() { + rewriteRun( + pomXml( + """ + + com.example + app + 1 + + + + com.google.guava + guava + + + + + + pinned + + + + com.example + bom + 1 + pom + import + + + + + + + """ + ) + ); + } + @Test void coordinatesOnlyEntryWhenTheParentManagesInAnInactiveProfile() { rewriteRun( - spec -> spec.recipe(new DependencyManagementDependencyRequiresVersion()), mavenProject("parent", pomXml( """ @@ -354,7 +394,6 @@ void coordinatesOnlyEntryWhenTheParentManagesInAnInactiveProfile() { @Test void coordinatesOnlyEntryInAPomAnotherModuleDeclaresAsItsParent() { rewriteRun( - spec -> spec.recipe(new DependencyManagementDependencyRequiresVersion()), mavenProject("parent", pomXml( """ @@ -396,7 +435,6 @@ void coordinatesOnlyEntryInAPomAnotherModuleDeclaresAsItsParent() { @Test void coordinatesOnlyEntryInAPomWithACyclicAncestry() { rewriteRun( - spec -> spec.recipe(new DependencyManagementDependencyRequiresVersion()), mavenProject("a", pomXml( """ @@ -453,7 +491,6 @@ void coordinatesOnlyEntryInAPomWithACyclicAncestry() { @Test void coordinatesOnlyEntryBesideASiblingWithAnUnresolvableProperty() { rewriteRun( - spec -> spec.recipe(new DependencyManagementDependencyRequiresVersion()), pomXml( """ @@ -482,7 +519,6 @@ void coordinatesOnlyEntryBesideASiblingWithAnUnresolvableProperty() { @Test void coordinatesOnlyEntryInAPomThatOthersInheritFromOrImport() { rewriteRun( - spec -> spec.recipe(new DependencyManagementDependencyRequiresVersion()), pomXml( """ @@ -507,7 +543,6 @@ void coordinatesOnlyEntryInAPomThatOthersInheritFromOrImport() { @Test void coordinatesOnlyEntryWithAnUnresolvableProperty() { rewriteRun( - spec -> spec.recipe(new DependencyManagementDependencyRequiresVersion()), pomXml( """ @@ -531,7 +566,6 @@ void coordinatesOnlyEntryWithAnUnresolvableProperty() { @Test void coordinatesOnlyEntryWithAResolvableProperty() { rewriteRun( - spec -> spec.recipe(new DependencyManagementDependencyRequiresVersion()), pomXml( """ @@ -568,7 +602,6 @@ void coordinatesOnlyEntryWithAResolvableProperty() { @Test void coordinatesOnlyEntryBesideADuplicateInTheSamePom() { rewriteRun( - spec -> spec.recipe(new DependencyManagementDependencyRequiresVersion()), pomXml( """ @@ -597,7 +630,6 @@ void coordinatesOnlyEntryBesideADuplicateInTheSamePom() { @Test void managedScope() { rewriteRun( - spec -> spec.recipe(new DependencyManagementDependencyRequiresVersion()), pomXml( """ @@ -629,7 +661,6 @@ void managedScope() { @Test void managedExclusions() { rewriteRun( - spec -> spec.recipe(new DependencyManagementDependencyRequiresVersion()), pomXml( """ @@ -666,7 +697,6 @@ void managedExclusions() { @Test void managedOptional() { rewriteRun( - spec -> spec.recipe(new DependencyManagementDependencyRequiresVersion()), pomXml( """ @@ -698,7 +728,6 @@ void managedOptional() { @Test void declaresMoreThanCoordinates() { rewriteRun( - spec -> spec.recipe(new DependencyManagementDependencyRequiresVersion()), pomXml( """ @@ -724,7 +753,6 @@ void declaresMoreThanCoordinates() { @Test void incompleteCoordinates() { rewriteRun( - spec -> spec.recipe(new DependencyManagementDependencyRequiresVersion()), pomXml( """ @@ -747,7 +775,6 @@ void incompleteCoordinates() { @Test void removesOnlyTheEntryThatManagesNothing() { rewriteRun( - spec -> spec.recipe(new DependencyManagementDependencyRequiresVersion()), pomXml( """ @@ -806,7 +833,6 @@ void removesOnlyTheEntryThatManagesNothing() { @Test void propertyVersion() { rewriteRun( - spec -> spec.recipe(new DependencyManagementDependencyRequiresVersion()), pomXml( """ From 0f67c6fdd23134190ca7e32a259e8a2ea6fa1742 Mon Sep 17 00:00:00 2001 From: martinfrancois Date: Sun, 16 Aug 2026 23:06:39 +0200 Subject: [PATCH 3/3] Extract explaining method for the unresolved placeholder check --- .../DependencyManagementDependencyRequiresVersion.java | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/rewrite-maven/src/main/java/org/openrewrite/maven/cleanup/DependencyManagementDependencyRequiresVersion.java b/rewrite-maven/src/main/java/org/openrewrite/maven/cleanup/DependencyManagementDependencyRequiresVersion.java index a013b9b96d2..e4ba144f651 100644 --- a/rewrite-maven/src/main/java/org/openrewrite/maven/cleanup/DependencyManagementDependencyRequiresVersion.java +++ b/rewrite-maven/src/main/java/org/openrewrite/maven/cleanup/DependencyManagementDependencyRequiresVersion.java @@ -143,11 +143,16 @@ private boolean managedElsewhere(Xml.Tag tag, String groupId, String artifactId) */ private @Nullable String resolve(Xml.Tag tag, String childName) { String value = getResolutionResult().getPom().getValue(tag.getChildValue(childName).orElse(null)); - return value == null || value.contains("${") ? null : value; + return value == null || containsUnresolvedPlaceholder(value) ? null : value; } }; } + // A surviving `${` means Maven could not resolve the property, so the value is unusable for comparison + private static boolean containsUnresolvedPlaceholder(String value) { + return value.contains("${"); + } + private static boolean importsBom(@Nullable List dependencyManagement) { if (dependencyManagement != null) { for (ManagedDependency managed : dependencyManagement) {