From 3d8a31faba944c3b8946ecb7cb711f7151a59416 Mon Sep 17 00:00:00 2001 From: martinfrancois Date: Mon, 10 Aug 2026 13:25:23 +0200 Subject: [PATCH 1/4] RemoveDuplicateDependencies: preserve Maven's effective dependency model The recipe kept the first of a set of duplicate declarations and deleted the later ones, which is not how Maven builds the effective model. In the last declaration is the effective one, so deleting it changed the resolved version, optionality or exclusions. In duplicates merge field-wise, each field coming from the first declaration that sets it while exclusions accumulate across all of them, so a later declaration can be the only source of the managed version. A repeated BOM import at another version can likewise manage entries the first import does not. Duplicates are now resolved over the whole or list instead of one tag at a time. A direct duplicate keeps the later declaration in the position of the first, so the resolved dependency order is unchanged, and declarations are compared with properties resolved so that a version written through a property still collapses onto an identical literal one. A managed duplicate is removed only when it sets no field the earlier declarations leave unset and carries no exclusion they do not already carry; a repeated BOM import only when it resolves to the same version. Two things for review: the existing test removeDependencyWithDifferentVersion expected the earlier version to survive and now expects the later one, and the recipe now leaves differing managed duplicates in place rather than collapsing them, giving up removals it used to make in order to keep the effective model intact. --- .../maven/RemoveDuplicateDependencies.java | 226 +++++- .../RemoveDuplicateDependenciesTest.java | 696 +++++++++++++++++- 2 files changed, 887 insertions(+), 35 deletions(-) diff --git a/rewrite-maven/src/main/java/org/openrewrite/maven/RemoveDuplicateDependencies.java b/rewrite-maven/src/main/java/org/openrewrite/maven/RemoveDuplicateDependencies.java index fb0bc04e48c..74ff72fb529 100644 --- a/rewrite-maven/src/main/java/org/openrewrite/maven/RemoveDuplicateDependencies.java +++ b/rewrite-maven/src/main/java/org/openrewrite/maven/RemoveDuplicateDependencies.java @@ -28,11 +28,14 @@ import org.openrewrite.maven.tree.ResolvedManagedDependency; import org.openrewrite.maven.tree.Scope; import org.openrewrite.xml.XPathMatcher; +import org.openrewrite.xml.tree.Content; import org.openrewrite.xml.tree.Xml; import java.time.Duration; import java.util.*; +import static java.util.Collections.singletonList; + @Value @EqualsAndHashCode(callSuper = false) public class RemoveDuplicateDependencies extends Recipe { @@ -58,42 +61,178 @@ public Xml.Document visitDocument(Xml.Document document, ExecutionContext ctx) { private final XPathMatcher DEPENDENCIES_MATCHER = new XPathMatcher("/project/dependencies"); private final XPathMatcher MANAGED_DEPENDENCIES_MATCHER = new XPathMatcher("/project/dependencyManagement/dependencies"); - @SuppressWarnings("DataFlowIssue") @Override - public Xml.@Nullable Tag visitTag(Xml.Tag tag, ExecutionContext ctx) { + public Xml.Tag visitTag(Xml.Tag tag, ExecutionContext ctx) { + Xml.Tag t = tag; if (isDependenciesTag()) { - getCursor().putMessage("dependencies", new HashMap()); + t = removeDuplicates(t, false); } else if (isManagedDependenciesTag()) { - getCursor().putMessage("managedDependencies", new HashMap()); - } else if (isDependencyTag()) { - Map dependencies = getCursor().getNearestMessage("dependencies"); - DependencyKey dependencyKey = getDependencyKey(tag); - if (dependencyKey != null) { - Xml.Tag existing = dependencies.putIfAbsent(dependencyKey, tag); - if (existing != null && existing != tag) { - maybeUpdateModel(); - return null; + t = removeDuplicates(t, true); + } + if (t != tag) { + maybeUpdateModel(); + } + return super.visitTag(t, ctx); + } + + /** + * Maven expects dependencies to be unique by group, artifact, type and classifier, and warns when a POM + * declares the same one twice ({@code 'dependencies.dependency.(groupId:artifactId:type:classifier)' + * must be unique}), but it still builds an effective model. This recipe only removes a duplicate when + * doing so leaves that model unchanged, and the two sections resolve differently: + *
    + *
  • in {@code } the last declaration is the effective one, see the + * {@code rootDependencies} map in {@link org.openrewrite.maven.tree.ResolvedPom}, so a differing + * duplicate has to take the place of the earlier declaration rather than be dropped;
    • + *
  • in {@code } duplicates merge field-wise: each field comes from the + * first declaration that sets it, and exclusions accumulate across all declarations + * ({@code } excepted: Maven does not inject it from {@code } at + * all, so a duplicate adding only it changes nothing either way and is conservatively kept). A later + * duplicate is therefore only removed when it sets no field the earlier declarations leave unset + * and carries no exclusion they do not already carry; otherwise several declarations make up the + * effective entry and all of them are left in place. A repeated BOM import is likewise only + * removed when it resolves to the same version, because for entries both versions manage the + * first import wins, pinned by {@code ResolvedPomTest#firstUniqueManagedDependencyWins}, while a + * different version may manage entries the first import does not.
    • + *
+ * The surviving declaration keeps the position of the first one, which is the position the resolved + * model already gave it. Removing a duplicate therefore leaves the resolved dependencies, the + * effective dependency management entries and their order exactly as they were. + */ + private Xml.Tag removeDuplicates(Xml.Tag dependencies, boolean managed) { + List content = dependencies.getContent(); + if (content == null) { + return dependencies; + } + + List deduplicated = new ArrayList<>(content.size()); + Map firstDeclarations = new HashMap<>(); + Map> managedFields = new HashMap<>(); + Map> managedExclusions = new HashMap<>(); + boolean removed = false; + for (Content child : content) { + if (child instanceof Xml.Tag && "dependency".equals(((Xml.Tag) child).getName())) { + Xml.Tag dependency = (Xml.Tag) child; + DependencyKey dependencyKey = managed ? getManagedDependencyKey(dependency) : getDependencyKey(dependency); + if (dependencyKey != null) { + if (managed) { + Set fields = declaredManagedFields(dependency); + Set exclusions = declaredExclusions(dependency); + Set earlierFields = managedFields.get(dependencyKey); + if (earlierFields == null) { + managedFields.put(dependencyKey, fields); + managedExclusions.put(dependencyKey, exclusions); + } else { + Set earlierExclusions = managedExclusions.get(dependencyKey); + if (earlierFields.containsAll(fields) && earlierExclusions.containsAll(exclusions)) { + removed = true; + continue; + } + // The duplicate contributes to the effective entry, so it has to stay + earlierFields.addAll(fields); + earlierExclusions.addAll(exclusions); + } + } else { + Integer firstDeclaration = firstDeclarations.putIfAbsent(dependencyKey, deduplicated.size()); + if (firstDeclaration != null) { + Xml.Tag effective = (Xml.Tag) deduplicated.get(firstDeclaration); + if (!isSameDeclaration(effective, dependency)) { + deduplicated.set(firstDeclaration, dependency.withPrefix(effective.getPrefix())); + } + removed = true; + continue; + } + } } } - } else if (isManagedDependencyTag()) { - Map dependencies = getCursor().getNearestMessage("managedDependencies"); - DependencyKey dependencyKey = getManagedDependencyKey(tag); - if (dependencyKey != null) { - // Additionally compare classifier and type, which are only partially compared in `findManagedDependency` - String classifier = getResolutionResult().getPom().getValue(tag.getChildValue("classifier").orElse(null)); - String type = getResolutionResult().getPom().getValue(tag.getChildValue("type").orElse("jar")); - if (Objects.equals(classifier, dependencyKey.getClassifier()) && - Objects.equals(type, dependencyKey.getType())) { - Xml.Tag existing = dependencies.putIfAbsent(dependencyKey, tag); - if (existing != null && existing != tag) { - maybeUpdateModel(); - return null; - } + deduplicated.add(child); + } + return removed ? dependencies.withContent(deduplicated) : dependencies; + } + + /** + * The names of the fields this declaration sets, {@code exclusions} excepted, which + * {@link #declaredExclusions} compares by value because Maven accumulates them across duplicates + * instead of taking them from any one declaration. Values are not compared: whether a duplicate sets + * {@code 2} or restates {@code 1}, the effective entry takes + * the version of the first declaration that sets one. + */ + private Set declaredManagedFields(Xml.Tag dependency) { + Set fields = new HashSet<>(); + for (Xml.Tag field : dependency.getChildren()) { + if (!"exclusions".equals(field.getName()) && !fieldValue(field).isEmpty()) { + fields.add(field.getName()); + } + } + return fields; + } + + private Set declaredExclusions(Xml.Tag dependency) { + Set exclusions = new HashSet<>(); + for (Xml.Tag field : dependency.getChildren()) { + if ("exclusions".equals(field.getName())) { + for (Xml.Tag exclusion : field.getChildren()) { + exclusions.add(fieldValue(exclusion)); } + } + } + return exclusions; + } + + /** + * Compares the fields the effective model of a {@code } entry is built from, ignoring + * formatting and comments and resolving property placeholders, so that a duplicate declared through a + * property is recognised as the same declaration. Any difference that is not known to be irrelevant + * counts as a difference, so that the two are only collapsed onto the earlier declaration when they + * resolve to the same thing. + */ + private boolean isSameDeclaration(Xml.Tag dependency, Xml.Tag other) { + return declaredFields(dependency).equals(declaredFields(other)); + } + + private Map> declaredFields(Xml.Tag dependency) { + Map> fields = new HashMap<>(); + for (Xml.Tag field : dependency.getChildren()) { + fields.computeIfAbsent(field.getName(), name -> new ArrayList<>()).add(fieldValue(field)); + } + // An omitted field is generally not the same as one restating its default, because it can also be + // supplied by ``. `type` is defaulted anyway to keep collapsing a bare + // declaration onto one that spells out `jar`, as `removeDependencyWithDefaultType` + // expects; that stays first-declaration-wins even where the two inherit a managed ``. + fields.putIfAbsent("type", singletonList("jar")); + return fields; + } + private String fieldValue(Xml.Tag field) { + List content = field.getContent(); + if (content == null) { + return ""; + } + StringBuilder value = new StringBuilder(); + boolean plainText = true; + for (Content child : content) { + if (child instanceof Xml.CharData) { + // Read the text directly rather than through `Xml.Tag#getValue`, which gives up as soon as a + // value is interrupted by a comment and would make two different values look identical + value.append(((Xml.CharData) child).getText().trim()); + } else if (child instanceof Xml.Comment) { + // A comment is not part of the value Maven reads + } else if (child instanceof Xml.Tag) { + Xml.Tag nested = (Xml.Tag) child; + value.append(nested.getName()).append('=').append(fieldValue(nested)).append(';'); + plainText = false; + } else { + // Content that cannot be compared as text falls back to its identity, so that two values are + // never considered equal on the strength of a part that was not actually compared + value.append(child.getId()); + plainText = false; } } - return super.visitTag(tag, ctx); + if (!plainText) { + return value.toString(); + } + String resolved = getResolutionResult().getPom().getValue(value.toString()); + return resolved != null ? resolved : value.toString(); } private boolean isDependenciesTag() { @@ -123,11 +262,21 @@ private boolean isManagedDependenciesTag() { } private @Nullable DependencyKey getManagedDependencyKey(Xml.Tag tag) { + DependencyKey dependencyKey; if (tag.getChildValue("scope").filter("import"::equalsIgnoreCase).isPresent()) { - return DependencyKey.from(tag); + dependencyKey = DependencyKey.from(tag, tag.getChild("version").map(this::fieldValue).orElse(null)); + } else { + ResolvedManagedDependency resolvedDependency = findManagedDependency(tag); + dependencyKey = resolvedDependency == null ? null : DependencyKey.from(resolvedDependency); } - ResolvedManagedDependency resolvedDependency = findManagedDependency(tag); - return resolvedDependency != null ? DependencyKey.from(resolvedDependency) : null; + if (dependencyKey == null) { + return null; + } + // Additionally compare classifier and type, which are only partially compared in `findManagedDependency` + String classifier = getResolutionResult().getPom().getValue(tag.getChildValue("classifier").orElse(null)); + String type = getResolutionResult().getPom().getValue(tag.getChildValue("type").orElse("jar")); + return Objects.equals(classifier, dependencyKey.getClassifier()) && + Objects.equals(type, dependencyKey.getType()) ? dependencyKey : null; } }); } @@ -145,22 +294,31 @@ private static class DependencyKey { Scope scope; + /** + * Only set for BOM imports, where a repeated import at a different version is not a duplicate of the + * first import: for entries both versions manage the first import wins, but a different version may + * manage entries the first import does not. + */ + @Nullable + String version; + public static DependencyKey from(ResolvedDependency dependency, Scope scope) { - return new DependencyKey(dependency.getGroupId(), dependency.getArtifactId(), dependency.getType(), dependency.getClassifier(), scope); + return new DependencyKey(dependency.getGroupId(), dependency.getArtifactId(), dependency.getType(), dependency.getClassifier(), scope, null); } public static DependencyKey from(ResolvedManagedDependency dependency) { - return new DependencyKey(dependency.getGroupId(), dependency.getArtifactId(), dependency.getType(), dependency.getClassifier(), Scope.Compile); + return new DependencyKey(dependency.getGroupId(), dependency.getArtifactId(), dependency.getType(), dependency.getClassifier(), Scope.Compile, null); } - public static @Nullable DependencyKey from(Xml.Tag tag) { + public static @Nullable DependencyKey from(Xml.Tag tag, @Nullable String version) { return tag.getChildValue("artifactId").map(artifactId -> new DependencyKey( tag.getChildValue("groupId").orElse(null), artifactId, tag.getChildValue("type").orElse("jar"), tag.getChildValue("classifier").orElse(null), - tag.getChildValue("scope").map(Scope::fromName).orElse(Scope.Compile) + tag.getChildValue("scope").map(Scope::fromName).orElse(Scope.Compile), + version )).orElse(null); } } diff --git a/rewrite-maven/src/test/java/org/openrewrite/maven/RemoveDuplicateDependenciesTest.java b/rewrite-maven/src/test/java/org/openrewrite/maven/RemoveDuplicateDependenciesTest.java index cb019d1d712..671c5c2f3ba 100644 --- a/rewrite-maven/src/test/java/org/openrewrite/maven/RemoveDuplicateDependenciesTest.java +++ b/rewrite-maven/src/test/java/org/openrewrite/maven/RemoveDuplicateDependenciesTest.java @@ -273,7 +273,7 @@ void removeDependencyWithDifferentVersion() { com.google.inject guice - 4.2.1 + 4.2.2 @@ -586,4 +586,698 @@ void retainWithAndWithoutClassifier() { ) ); } + + @Test + void retainLaterDependencyDeclaration() { + rewriteRun( + pomXml( + """ + + 4.0.0 + + com.mycompany.app + my-app + 1 + + + + junit + junit + 4.13.1 + + + junit + junit + 4.13.2 + + + + com.google.guava + guava + 29.0-jre + false + + + com.google.guava + guava + 29.0-jre + true + + + + org.apache.httpcomponents + httpclient + 4.5.13 + + + org.apache.httpcomponents + httpclient + 4.5.13 + + + commons-codec + commons-codec + + + + + + """, + """ + + 4.0.0 + + com.mycompany.app + my-app + 1 + + + + junit + junit + 4.13.2 + + + + com.google.guava + guava + 29.0-jre + true + + + + org.apache.httpcomponents + httpclient + 4.5.13 + + + commons-codec + commons-codec + + + + + + """ + ) + ); + } + + @Test + void retainLaterDependencyDeclarationInReverseOrder() { + rewriteRun( + pomXml( + """ + + 4.0.0 + + com.mycompany.app + my-app + 1 + + + + junit + junit + 4.13.2 + + + junit + junit + 4.13.1 + + + + org.apache.httpcomponents + httpclient + 4.5.13 + + + commons-codec + commons-codec + + + + + org.apache.httpcomponents + httpclient + 4.5.13 + + + + """, + """ + + 4.0.0 + + com.mycompany.app + my-app + 1 + + + + junit + junit + 4.13.1 + + + + org.apache.httpcomponents + httpclient + 4.5.13 + + + + """ + ) + ); + } + + /** + * `` resolves differently from ``: duplicates merge field-wise, with each + * field taken from the first declaration that sets it. Both duplicates here set the same fields, so the later + * declaration contributes nothing to the effective entry and can be removed, whatever its values. + */ + @Test + void retainFirstManagedDependencyDeclaration() { + rewriteRun( + pomXml( + """ + + 4.0.0 + + com.mycompany.app + my-app + 1 + + + + + com.acme + versioned + 1 + + + com.acme + versioned + 2 + + + + com.acme + scoped + 1 + compile + + + com.acme + scoped + 1 + runtime + + + + + """, + """ + + 4.0.0 + + com.mycompany.app + my-app + 1 + + + + + com.acme + versioned + 1 + + + + com.acme + scoped + 1 + compile + + + + + """ + ) + ); + } + + /** + * The field-wise merge means the effective managed entry can be made up of several declarations: here Maven + * takes the version from the second declaration because the first does not set one. Removing the second + * declaration would leave the dependency with no managed version at all. + */ + @Test + void retainManagedDuplicateSettingAFieldTheFirstLeavesUnset() { + rewriteRun( + pomXml( + """ + + 4.0.0 + + com.mycompany.app + my-app + 1 + + + + + com.google.guava + guava + + + com.google.guava + guava + 32.1.3-jre + + + + + """ + ) + ); + } + + /** + * Scope comes from the first declaration that sets it and exclusions accumulate across all duplicates, so + * those later declarations contribute to the effective entry and have to stay. Maven 3.9 does not inject + * {@code } from {@code } at all, so a duplicate that only adds it changes + * nothing either way; the recipe keeps it rather than reasoning about a field the resolved model does not + * carry. + */ + @Test + void retainManagedDuplicateContributingScopeOptionalOrExclusions() { + rewriteRun( + pomXml( + """ + + 4.0.0 + + com.mycompany.app + my-app + 1 + + + + + com.acme + scoped + 1 + + + com.acme + scoped + 1 + test + + + + com.acme + optional + 1 + + + com.acme + optional + 1 + true + + + + com.acme + excluded + 1 + + + com.acme + excluded + 1 + + + commons-logging + commons-logging + + + + + + + """ + ) + ); + } + + /** + * A later duplicate that sets no field the earlier declarations leave unset and carries no exclusion they do + * not already carry contributes nothing to the effective entry, so it can still be removed. + */ + @Test + void removeRedundantManagedDependencyDuplicates() { + rewriteRun( + pomXml( + """ + + 4.0.0 + + com.mycompany.app + my-app + 1 + + + + + com.acme + scoped + 1 + runtime + + + com.acme + scoped + 2 + + + + com.acme + excluded + 1 + + + commons-logging + commons-logging + + + + + com.acme + excluded + 1 + + + commons-logging + commons-logging + + + + + + + """, + """ + + 4.0.0 + + com.mycompany.app + my-app + 1 + + + + + com.acme + scoped + 1 + runtime + + + + com.acme + excluded + 1 + + + commons-logging + commons-logging + + + + + + + """ + ) + ); + } + + /** + * A repeated BOM import at a different version is not redundant: for entries both versions manage the first + * import wins (see `ResolvedPomTest#firstUniqueManagedDependencyWins`), but the second version may manage + * entries the first does not, and those still take effect. Only an import of the same version again changes + * nothing, see `removeDuplicatedDependencyWithImportScope`. + */ + @Test + void retainRepeatedBomImportWithDifferentVersion() { + rewriteRun( + pomXml( + """ + + 4.0.0 + + com.mycompany.app + my-app + 1 + + + + + org.apache.logging.log4j + log4j-bom + 2.24.0 + import + pom + + + org.apache.logging.log4j + log4j-bom + 2.24.1 + import + pom + + + + + """ + ) + ); + } + + /** + * The resolved model orders duplicated dependencies by their first declaration, so the surviving declaration + * has to stay in that position rather than move to where it was written. Comments keep the position they were + * written in, exactly as they do when a duplicate is removed outright, see `preservesComments`. + */ + @Test + void retainLaterDeclarationInTheFirstPosition() { + rewriteRun( + pomXml( + """ + + 4.0.0 + + com.mycompany.app + my-app + 1 + + + + + junit + junit + 4.13.1 + + + + junit + junit + 4.13.2 + + + + """, + """ + + 4.0.0 + + com.mycompany.app + my-app + 1 + + + + + junit + junit + 4.13.2 + + + + + """ + ) + ); + } + + @Test + void removeDuplicateDeclaredThroughProperty() { + rewriteRun( + pomXml( + """ + + 4.0.0 + + com.mycompany.app + my-app + 1 + + + 29.0-jre + + + + + com.google.guava + guava + ${guava.version} + + + com.google.guava + guava + 29.0-jre + + + + """, + """ + + 4.0.0 + + com.mycompany.app + my-app + 1 + + + 29.0-jre + + + + + com.google.guava + guava + ${guava.version} + + + + """ + ) + ); + } + + /** + * The surviving declaration is taken over whole, so it brings its own indentation with it and only the + * indentation of the `` tag itself is taken from the declaration it replaces. Duplicates that were + * written at different indentation levels therefore need a formatting recipe afterwards. + */ + @Test + void retainLaterDeclarationIndentedDifferently() { + rewriteRun( + pomXml( + """ + + 4.0.0 + + com.mycompany.app + my-app + 1 + + + + com.google.inject + guice + 4.2.1 + + + com.google.inject + guice + 4.2.2 + + + + """, + """ + + 4.0.0 + + com.mycompany.app + my-app + 1 + + + + com.google.inject + guice + 4.2.2 + + + + """ + ) + ); + } + + @Test + void retainLaterDeclarationWhenAValueContainsAComment() { + rewriteRun( + pomXml( + """ + + 4.0.0 + + com.mycompany.app + my-app + 1 + + + + com.google.guava + guava + 29.0-jre + false + + + com.google.guava + guava + 29.0-jre + true + + + + """, + """ + + 4.0.0 + + com.mycompany.app + my-app + 1 + + + + com.google.guava + guava + 29.0-jre + true + + + + """ + ) + ); + } } From 75a913123bba8ba06f5072dfa9bc71ecc1b16a03 Mon Sep 17 00:00:00 2001 From: Tim te Beek Date: Tue, 11 Aug 2026 23:17:54 +0200 Subject: [PATCH 2/4] Resolve BOM import coordinates, note the kept declaration, trim commentary A BOM import declared through a property built its key from raw tag text while the classifier and type guard compared resolved values, so it was never recognised as a duplicate. --- .../maven/RemoveDuplicateDependencies.java | 107 +++++++----------- .../resources/META-INF/rewrite/recipes.csv | 2 +- .../RemoveDuplicateDependenciesTest.java | 65 +++++++++++ 3 files changed, 109 insertions(+), 65 deletions(-) diff --git a/rewrite-maven/src/main/java/org/openrewrite/maven/RemoveDuplicateDependencies.java b/rewrite-maven/src/main/java/org/openrewrite/maven/RemoveDuplicateDependencies.java index 74ff72fb529..0a4c84be7fb 100644 --- a/rewrite-maven/src/main/java/org/openrewrite/maven/RemoveDuplicateDependencies.java +++ b/rewrite-maven/src/main/java/org/openrewrite/maven/RemoveDuplicateDependencies.java @@ -42,7 +42,8 @@ public class RemoveDuplicateDependencies extends Recipe { String displayName = "Remove duplicate Maven dependencies"; - String description = "Removes duplicated dependencies in the `` and `` sections of the `pom.xml`."; + String description = "Removes duplicated dependencies in the `` and `` sections of the `pom.xml`. " + + "The declaration Maven resolves to is the one kept, at the position of the first of the duplicates, so the effective dependency model is unchanged."; Duration estimatedEffortPerOccurrence = Duration.ofMinutes(2); @@ -76,28 +77,16 @@ public Xml.Tag visitTag(Xml.Tag tag, ExecutionContext ctx) { } /** - * Maven expects dependencies to be unique by group, artifact, type and classifier, and warns when a POM - * declares the same one twice ({@code 'dependencies.dependency.(groupId:artifactId:type:classifier)' - * must be unique}), but it still builds an effective model. This recipe only removes a duplicate when - * doing so leaves that model unchanged, and the two sections resolve differently: - *
    - *
  • in {@code } the last declaration is the effective one, see the - * {@code rootDependencies} map in {@link org.openrewrite.maven.tree.ResolvedPom}, so a differing - * duplicate has to take the place of the earlier declaration rather than be dropped;
    • - *
  • in {@code } duplicates merge field-wise: each field comes from the - * first declaration that sets it, and exclusions accumulate across all declarations - * ({@code } excepted: Maven does not inject it from {@code } at - * all, so a duplicate adding only it changes nothing either way and is conservatively kept). A later - * duplicate is therefore only removed when it sets no field the earlier declarations leave unset - * and carries no exclusion they do not already carry; otherwise several declarations make up the - * effective entry and all of them are left in place. A repeated BOM import is likewise only - * removed when it resolves to the same version, because for entries both versions manage the - * first import wins, pinned by {@code ResolvedPomTest#firstUniqueManagedDependencyWins}, while a - * different version may manage entries the first import does not.
    • - *
- * The surviving declaration keeps the position of the first one, which is the position the resolved - * model already gave it. Removing a duplicate therefore leaves the resolved dependencies, the - * effective dependency management entries and their order exactly as they were. + * Maven warns when a POM declares the same dependency twice + * ({@code 'dependencies.dependency.(groupId:artifactId:type:classifier)' must be unique}) but still + * builds an effective model, and the two sections resolve differently. In {@code } the + * last declaration wins, see {@code rootDependencies} in {@link org.openrewrite.maven.tree.ResolvedPom}, + * so a differing duplicate takes the place of the earlier one rather than being dropped. In + * {@code } entries merge field-wise from the first declaration that sets each + * field, with exclusions accumulating, so a later duplicate only goes when it adds neither; a repeated + * BOM import is a duplicate only at the same version, since the first import wins for the entries both + * manage. Either way the survivor keeps the first declaration's position, which is where the resolved + * model already put it. */ private Xml.Tag removeDuplicates(Xml.Tag dependencies, boolean managed) { List content = dependencies.getContent(); @@ -152,10 +141,9 @@ private Xml.Tag removeDuplicates(Xml.Tag dependencies, boolean managed) { /** * The names of the fields this declaration sets, {@code exclusions} excepted, which - * {@link #declaredExclusions} compares by value because Maven accumulates them across duplicates - * instead of taking them from any one declaration. Values are not compared: whether a duplicate sets - * {@code 2} or restates {@code 1}, the effective entry takes - * the version of the first declaration that sets one. + * {@link #declaredExclusions} compares by value because Maven accumulates them rather than taking + * them from one declaration. Values are not compared, as the effective entry takes each field from + * the first declaration that sets it whatever a later one says. */ private Set declaredManagedFields(Xml.Tag dependency) { Set fields = new HashSet<>(); @@ -180,11 +168,8 @@ private Set declaredExclusions(Xml.Tag dependency) { } /** - * Compares the fields the effective model of a {@code } entry is built from, ignoring - * formatting and comments and resolving property placeholders, so that a duplicate declared through a - * property is recognised as the same declaration. Any difference that is not known to be irrelevant - * counts as a difference, so that the two are only collapsed onto the earlier declaration when they - * resolve to the same thing. + * Whether both entries resolve to the same declaration, ignoring formatting and comments and + * resolving property placeholders. Any difference not known to be irrelevant counts as one. */ private boolean isSameDeclaration(Xml.Tag dependency, Xml.Tag other) { return declaredFields(dependency).equals(declaredFields(other)); @@ -195,10 +180,9 @@ private Map> declaredFields(Xml.Tag dependency) { for (Xml.Tag field : dependency.getChildren()) { fields.computeIfAbsent(field.getName(), name -> new ArrayList<>()).add(fieldValue(field)); } - // An omitted field is generally not the same as one restating its default, because it can also be - // supplied by ``. `type` is defaulted anyway to keep collapsing a bare - // declaration onto one that spells out `jar`, as `removeDependencyWithDefaultType` - // expects; that stays first-declaration-wins even where the two inherit a managed ``. + // An omitted field is not generally the same as one restating its default, as it can also come from + // ``; `type` is defaulted anyway to keep collapsing a bare declaration onto + // one spelling out `jar`, as `removeDependencyWithDefaultType` expects. fields.putIfAbsent("type", singletonList("jar")); return fields; } @@ -212,18 +196,18 @@ private String fieldValue(Xml.Tag field) { boolean plainText = true; for (Content child : content) { if (child instanceof Xml.CharData) { - // Read the text directly rather than through `Xml.Tag#getValue`, which gives up as soon as a - // value is interrupted by a comment and would make two different values look identical + // Not `Xml.Tag#getValue`, which gives up on a value interrupted by a comment and would + // then make two different values look identical value.append(((Xml.CharData) child).getText().trim()); } else if (child instanceof Xml.Comment) { - // A comment is not part of the value Maven reads + // Not part of the value Maven reads } else if (child instanceof Xml.Tag) { Xml.Tag nested = (Xml.Tag) child; value.append(nested.getName()).append('=').append(fieldValue(nested)).append(';'); plainText = false; } else { - // Content that cannot be compared as text falls back to its identity, so that two values are - // never considered equal on the strength of a part that was not actually compared + // Content that cannot be compared as text falls back to its identity, so two values are + // never equal on the strength of a part that was not actually compared value.append(child.getId()); plainText = false; } @@ -262,19 +246,27 @@ private boolean isManagedDependenciesTag() { } private @Nullable DependencyKey getManagedDependencyKey(Xml.Tag tag) { - DependencyKey dependencyKey; + String classifier = getResolutionResult().getPom().getValue(tag.getChildValue("classifier").orElse(null)); + String type = getResolutionResult().getPom().getValue(tag.getChildValue("type").orElse("jar")); if (tag.getChildValue("scope").filter("import"::equalsIgnoreCase).isPresent()) { - dependencyKey = DependencyKey.from(tag, tag.getChild("version").map(this::fieldValue).orElse(null)); - } else { - ResolvedManagedDependency resolvedDependency = findManagedDependency(tag); - dependencyKey = resolvedDependency == null ? null : DependencyKey.from(resolvedDependency); + String artifactId = getResolutionResult().getPom().getValue(tag.getChildValue("artifactId").orElse(null)); + if (artifactId == null) { + return null; + } + return new DependencyKey( + getResolutionResult().getPom().getValue(tag.getChildValue("groupId").orElse(null)), + artifactId, + type, + classifier, + Scope.Import, + tag.getChild("version").map(this::fieldValue).orElse(null)); } - if (dependencyKey == null) { + ResolvedManagedDependency resolvedDependency = findManagedDependency(tag); + if (resolvedDependency == null) { return null; } + DependencyKey dependencyKey = DependencyKey.from(resolvedDependency); // Additionally compare classifier and type, which are only partially compared in `findManagedDependency` - String classifier = getResolutionResult().getPom().getValue(tag.getChildValue("classifier").orElse(null)); - String type = getResolutionResult().getPom().getValue(tag.getChildValue("type").orElse("jar")); return Objects.equals(classifier, dependencyKey.getClassifier()) && Objects.equals(type, dependencyKey.getType()) ? dependencyKey : null; } @@ -295,9 +287,8 @@ private static class DependencyKey { Scope scope; /** - * Only set for BOM imports, where a repeated import at a different version is not a duplicate of the - * first import: for entries both versions manage the first import wins, but a different version may - * manage entries the first import does not. + * Only set for BOM imports: the first import wins for the entries both versions manage, but a + * different version may manage entries the first one does not. */ @Nullable String version; @@ -309,17 +300,5 @@ public static DependencyKey from(ResolvedDependency dependency, Scope scope) { public static DependencyKey from(ResolvedManagedDependency dependency) { return new DependencyKey(dependency.getGroupId(), dependency.getArtifactId(), dependency.getType(), dependency.getClassifier(), Scope.Compile, null); } - - public static @Nullable DependencyKey from(Xml.Tag tag, @Nullable String version) { - return tag.getChildValue("artifactId").map(artifactId -> - new DependencyKey( - tag.getChildValue("groupId").orElse(null), - artifactId, - tag.getChildValue("type").orElse("jar"), - tag.getChildValue("classifier").orElse(null), - tag.getChildValue("scope").map(Scope::fromName).orElse(Scope.Compile), - version - )).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 ea5ea1ef0dc..bbbcfe95adf 100644 --- a/rewrite-maven/src/main/resources/META-INF/rewrite/recipes.csv +++ b/rewrite-maven/src/main/resources/META-INF/rewrite/recipes.csv @@ -37,7 +37,7 @@ maven,org.openrewrite:rewrite-maven,org.openrewrite.maven.ModernizeObsoletePoms, maven,org.openrewrite:rewrite-maven,org.openrewrite.maven.OrderPomElements,Order POM elements,Order POM elements according to the [recommended](https://maven.apache.org/developers/conventions/code.html#pom-code-convention) order.,1,,Maven,, maven,org.openrewrite:rewrite-maven,org.openrewrite.maven.RemoveBomManagedDirectDependencies,Remove direct dependencies that are managed by a BOM with incompatible versions,"Removes directly declared dependencies when they have a version that is incompatible with the version managed by an imported BOM. This is useful during framework upgrades (e.g., Spring Boot) where transitive dependencies receive major version bumps and explicitly declared older versions should be removed to use the BOM-managed versions instead. A dependency is only removed when it would still be reachable transitively through another direct dependency, so the BOM-managed version takes its place rather than the dependency disappearing from the classpath.",1,,Maven,"[{""name"":""bomGroupPattern"",""type"":""String"",""displayName"":""BOM group pattern"",""description"":""Group ID glob pattern for BOMs to consider. For example, `org.springframework.boot` to match Spring Boot BOMs."",""example"":""org.springframework.boot"",""required"":true},{""name"":""bomArtifactPattern"",""type"":""String"",""displayName"":""BOM artifact pattern"",""description"":""Artifact ID glob pattern for BOMs to consider. For example, `*-dependencies` to match Spring Boot's BOM."",""example"":""*-dependencies""},{""name"":""dependencyGroupPattern"",""type"":""String"",""displayName"":""Dependency group pattern"",""description"":""Group ID glob pattern for dependencies to check against BOM. Use `*` to match all dependencies."",""example"":""*""},{""name"":""dependencyArtifactPattern"",""type"":""String"",""displayName"":""Dependency artifact pattern"",""description"":""Artifact ID glob pattern for dependencies to check against BOM. Use `*` to match all dependencies."",""example"":""*""}]", maven,org.openrewrite:rewrite-maven,org.openrewrite.maven.RemoveDependency,Remove Maven dependency,"Removes a single dependency from the section of the pom.xml. Does not remove usage of the dependency classes, nor guard against the resulting compilation errors.",1,,Maven,"[{""name"":""groupId"",""type"":""String"",""displayName"":""Group"",""description"":""The first part of a dependency coordinate `com.google.guava:guava:VERSION`."",""example"":""com.google.guava"",""required"":true},{""name"":""artifactId"",""type"":""String"",""displayName"":""Artifact"",""description"":""The second part of a dependency coordinate `com.google.guava:guava:VERSION`."",""example"":""guava"",""required"":true},{""name"":""scope"",""type"":""String"",""displayName"":""Scope"",""description"":""Only remove dependencies if they are in this scope. If 'runtime', this willalso remove dependencies in the 'compile' scope because 'compile' dependencies are part of the runtime dependency set"",""example"":""compile"",""valid"":[""compile"",""test"",""runtime"",""provided""]}]", -maven,org.openrewrite:rewrite-maven,org.openrewrite.maven.RemoveDuplicateDependencies,Remove duplicate Maven dependencies,Removes duplicated dependencies in the `` and `` sections of the `pom.xml`.,1,,Maven,, +maven,org.openrewrite:rewrite-maven,org.openrewrite.maven.RemoveDuplicateDependencies,Remove duplicate Maven dependencies,"Removes duplicated dependencies in the `` and `` sections of the `pom.xml`. The declaration Maven resolves to is the one kept, at the position of the first of the duplicates, so the effective dependency model is unchanged.",1,,Maven,, maven,org.openrewrite:rewrite-maven,org.openrewrite.maven.RemoveDuplicatePluginDeclarations,Remove duplicate plugin declarations,"Maven 4 rejects duplicate plugin declarations (same groupId and artifactId) with an error. This recipe removes duplicate plugin declarations, keeping only the first occurrence.",1,,Maven,, maven,org.openrewrite:rewrite-maven,org.openrewrite.maven.RemoveExclusion,Remove exclusion,Remove any matching exclusion from any matching dependency.,1,,Maven,"[{""name"":""groupId"",""type"":""String"",""displayName"":""Group"",""description"":""The first part of a dependency coordinate `com.google.guava:guava:VERSION`. Supports glob."",""example"":""com.google.guava"",""required"":true},{""name"":""artifactId"",""type"":""String"",""displayName"":""Artifact"",""description"":""The second part of a dependency coordinate `com.google.guava:guava:VERSION`. Supports glob."",""example"":""guava"",""required"":true},{""name"":""exclusionGroupId"",""type"":""String"",""displayName"":""Exclusion group"",""description"":""The first part of a dependency coordinate `com.google.guava:guava:VERSION`. Supports glob."",""example"":""com.google.guava"",""required"":true},{""name"":""exclusionArtifactId"",""type"":""String"",""displayName"":""Exclusion artifact"",""description"":""The second part of a dependency coordinate `com.google.guava:guava:VERSION`. Supports glob."",""example"":""guava"",""required"":true},{""name"":""onlyIneffective"",""type"":""Boolean"",""displayName"":""Only ineffective"",""description"":""Default false. If enabled, matching exclusions will only be removed if they are ineffective (if the excluded dependency was not actually a transitive dependency of the target dependency).""}]", maven,org.openrewrite:rewrite-maven,org.openrewrite.maven.RemoveManagedDependency,Remove Maven managed dependency,Removes a single managed dependency from the section of the pom.xml.,1,,Maven,"[{""name"":""groupId"",""type"":""String"",""displayName"":""Group"",""description"":""The first part of a managed dependency coordinate `com.google.guava:guava:VERSION`."",""example"":""com.google.guava"",""required"":true},{""name"":""artifactId"",""type"":""String"",""displayName"":""Artifact"",""description"":""The second part of a managed dependency coordinate `com.google.guava:guava:VERSION`."",""example"":""guava"",""required"":true},{""name"":""scope"",""type"":""String"",""displayName"":""Scope"",""description"":""Only remove managed dependencies if they are in this scope. If `runtime`, this will also remove managed dependencies in the 'compile' scope because `compile` dependencies are part of the runtime dependency set."",""example"":""compile"",""valid"":[""compile"",""test"",""runtime"",""provided""]}]", diff --git a/rewrite-maven/src/test/java/org/openrewrite/maven/RemoveDuplicateDependenciesTest.java b/rewrite-maven/src/test/java/org/openrewrite/maven/RemoveDuplicateDependenciesTest.java index 671c5c2f3ba..8bfa65a23e1 100644 --- a/rewrite-maven/src/test/java/org/openrewrite/maven/RemoveDuplicateDependenciesTest.java +++ b/rewrite-maven/src/test/java/org/openrewrite/maven/RemoveDuplicateDependenciesTest.java @@ -1068,6 +1068,71 @@ void retainRepeatedBomImportWithDifferentVersion() { ); } + @Test + void removeRepeatedBomImportDeclaredThroughProperty() { + rewriteRun( + pomXml( + """ + + 4.0.0 + + com.mycompany.app + my-app + 1 + + + org.apache.logging.log4j + + + + + + ${log4j.groupId} + log4j-bom + 2.24.0 + import + pom + + + org.apache.logging.log4j + log4j-bom + 2.24.0 + import + pom + + + + + """, + """ + + 4.0.0 + + com.mycompany.app + my-app + 1 + + + org.apache.logging.log4j + + + + + + ${log4j.groupId} + log4j-bom + 2.24.0 + import + pom + + + + + """ + ) + ); + } + /** * The resolved model orders duplicated dependencies by their first declaration, so the surviving declaration * has to stay in that position rather than move to where it was written. Comments keep the position they were From a85fc7d997016db0feaee01dc264acdd54549c2c Mon Sep 17 00:00:00 2001 From: Tim te Beek Date: Tue, 11 Aug 2026 23:18:33 +0200 Subject: [PATCH 3/4] Trim test commentary --- .../RemoveDuplicateDependenciesTest.java | 37 +++++++------------ 1 file changed, 14 insertions(+), 23 deletions(-) diff --git a/rewrite-maven/src/test/java/org/openrewrite/maven/RemoveDuplicateDependenciesTest.java b/rewrite-maven/src/test/java/org/openrewrite/maven/RemoveDuplicateDependenciesTest.java index 8bfa65a23e1..5adf7bc0a80 100644 --- a/rewrite-maven/src/test/java/org/openrewrite/maven/RemoveDuplicateDependenciesTest.java +++ b/rewrite-maven/src/test/java/org/openrewrite/maven/RemoveDuplicateDependenciesTest.java @@ -754,9 +754,8 @@ void retainLaterDependencyDeclarationInReverseOrder() { } /** - * `` resolves differently from ``: duplicates merge field-wise, with each - * field taken from the first declaration that sets it. Both duplicates here set the same fields, so the later - * declaration contributes nothing to the effective entry and can be removed, whatever its values. + * Managed duplicates merge field-wise from the first declaration that sets each field, so a later duplicate + * setting only the same fields contributes nothing, whatever its values. */ @Test void retainFirstManagedDependencyDeclaration() { @@ -830,9 +829,8 @@ void retainFirstManagedDependencyDeclaration() { } /** - * The field-wise merge means the effective managed entry can be made up of several declarations: here Maven - * takes the version from the second declaration because the first does not set one. Removing the second - * declaration would leave the dependency with no managed version at all. + * The effective managed entry can be made up of several declarations: Maven takes the version from the + * second because the first sets none, so removing it would leave no managed version at all. */ @Test void retainManagedDuplicateSettingAFieldTheFirstLeavesUnset() { @@ -866,11 +864,9 @@ void retainManagedDuplicateSettingAFieldTheFirstLeavesUnset() { } /** - * Scope comes from the first declaration that sets it and exclusions accumulate across all duplicates, so - * those later declarations contribute to the effective entry and have to stay. Maven 3.9 does not inject - * {@code } from {@code } at all, so a duplicate that only adds it changes - * nothing either way; the recipe keeps it rather than reasoning about a field the resolved model does not - * carry. + * Scope comes from the first declaration that sets it and exclusions accumulate, so those duplicates stay. + * Maven 3.9 never injects {@code } from {@code }, so a duplicate adding only + * it changes nothing either way and is kept rather than reasoned about. */ @Test void retainManagedDuplicateContributingScopeOptionalOrExclusions() { @@ -935,8 +931,7 @@ void retainManagedDuplicateContributingScopeOptionalOrExclusions() { } /** - * A later duplicate that sets no field the earlier declarations leave unset and carries no exclusion they do - * not already carry contributes nothing to the effective entry, so it can still be removed. + * A later duplicate adding neither an unset field nor a new exclusion contributes nothing, so it still goes. */ @Test void removeRedundantManagedDependencyDuplicates() { @@ -1027,10 +1022,8 @@ void removeRedundantManagedDependencyDuplicates() { } /** - * A repeated BOM import at a different version is not redundant: for entries both versions manage the first - * import wins (see `ResolvedPomTest#firstUniqueManagedDependencyWins`), but the second version may manage - * entries the first does not, and those still take effect. Only an import of the same version again changes - * nothing, see `removeDuplicatedDependencyWithImportScope`. + * The first import wins for the entries both versions manage, but the second may manage entries the first + * does not. Only a repeat of the same version changes nothing, see `removeDuplicatedDependencyWithImportScope`. */ @Test void retainRepeatedBomImportWithDifferentVersion() { @@ -1134,9 +1127,8 @@ void removeRepeatedBomImportDeclaredThroughProperty() { } /** - * The resolved model orders duplicated dependencies by their first declaration, so the surviving declaration - * has to stay in that position rather than move to where it was written. Comments keep the position they were - * written in, exactly as they do when a duplicate is removed outright, see `preservesComments`. + * The resolved model orders duplicates by their first declaration, so the survivor stays in that position + * rather than moving to where it was written. Comments keep their own, as in `preservesComments`. */ @Test void retainLaterDeclarationInTheFirstPosition() { @@ -1245,9 +1237,8 @@ void removeDuplicateDeclaredThroughProperty() { } /** - * The surviving declaration is taken over whole, so it brings its own indentation with it and only the - * indentation of the `` tag itself is taken from the declaration it replaces. Duplicates that were - * written at different indentation levels therefore need a formatting recipe afterwards. + * The survivor is taken over whole, so only the `` tag's own indentation comes from the + * declaration it replaces. Duplicates written at different levels need a formatting recipe afterwards. */ @Test void retainLaterDeclarationIndentedDifferently() { From bbe11a5c5b489fa523de6ec018a318fe2ddfa4c3 Mon Sep 17 00:00:00 2001 From: martinfrancois Date: Sun, 16 Aug 2026 23:42:02 +0200 Subject: [PATCH 4/4] Name the default dependency type and explain the nested-tag key format --- .../openrewrite/maven/RemoveDuplicateDependencies.java | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/rewrite-maven/src/main/java/org/openrewrite/maven/RemoveDuplicateDependencies.java b/rewrite-maven/src/main/java/org/openrewrite/maven/RemoveDuplicateDependencies.java index b9e300343b0..56eca948345 100644 --- a/rewrite-maven/src/main/java/org/openrewrite/maven/RemoveDuplicateDependencies.java +++ b/rewrite-maven/src/main/java/org/openrewrite/maven/RemoveDuplicateDependencies.java @@ -40,6 +40,9 @@ @EqualsAndHashCode(callSuper = false) public class RemoveDuplicateDependencies extends Recipe { + // Maven's implicit when the tag is absent + private static final String DEFAULT_DEPENDENCY_TYPE = "jar"; + String displayName = "Remove duplicate Maven dependencies"; String description = "Removes duplicated dependencies in the `` and `` sections of the `pom.xml`. " + @@ -183,7 +186,7 @@ private Map> declaredFields(Xml.Tag dependency) { // An omitted field is not generally the same as one restating its default, as it can also come from // ``; `type` is defaulted anyway to keep collapsing a bare declaration onto // one spelling out `jar`, as `removeDependencyWithDefaultType` expects. - fields.putIfAbsent("type", singletonList("jar")); + fields.putIfAbsent("type", singletonList(DEFAULT_DEPENDENCY_TYPE)); return fields; } @@ -203,6 +206,7 @@ private String fieldValue(Xml.Tag field) { // Not part of the value Maven reads } else if (child instanceof Xml.Tag) { Xml.Tag nested = (Xml.Tag) child; + // Serializes nested tags into a key that is only ever compared for equality, never parsed value.append(nested.getName()).append('=').append(fieldValue(nested)).append(';'); plainText = false; } else { @@ -247,7 +251,7 @@ private boolean isManagedDependenciesTag() { private @Nullable DependencyKey getManagedDependencyKey(Xml.Tag tag) { String classifier = getResolutionResult().getPom().getValue(tag.getChildValue("classifier").orElse(null)); - String type = getResolutionResult().getPom().getValue(tag.getChildValue("type").orElse("jar")); + String type = getResolutionResult().getPom().getValue(tag.getChildValue("type").orElse(DEFAULT_DEPENDENCY_TYPE)); if (tag.getChildValue("scope").filter("import"::equalsIgnoreCase).isPresent()) { String artifactId = getResolutionResult().getPom().getValue(tag.getChildValue("artifactId").orElse(null)); if (artifactId == null) {