From 6e336a79fbf59d19d03bd3f1eafcc12e2bfa310d Mon Sep 17 00:00:00 2001 From: Tim te Beek Date: Wed, 22 Jul 2026 23:07:18 +0200 Subject: [PATCH 1/2] Ignore spurious transitive exclusions when detecting redundant dependencies A dependency's effective exclusions are resolved within the matched parent's whole tree, so they can be polluted by exclusions other branches declare against artifacts the coordinate could never bring on its own (e.g. an optional dependency pruned elsewhere). Comparing these raw sets against a clean direct declaration made the recipe keep genuinely redundant dependencies. Filter each transitive's effective exclusions down to artifacts in the coordinate's own dependency closure, dropping no-op exclusions before the comparison. This only ever drops exclusions that change nothing, so it never causes an unsafe removal. --- .../RemoveRedundantDependencies.java | 51 +++++- .../RemoveRedundantDependenciesTest.java | 162 ++++++++++++++++++ 2 files changed, 209 insertions(+), 4 deletions(-) diff --git a/src/main/java/org/openrewrite/java/dependencies/RemoveRedundantDependencies.java b/src/main/java/org/openrewrite/java/dependencies/RemoveRedundantDependencies.java index ef8f53ad..34835a9a 100644 --- a/src/main/java/org/openrewrite/java/dependencies/RemoveRedundantDependencies.java +++ b/src/main/java/org/openrewrite/java/dependencies/RemoveRedundantDependencies.java @@ -74,6 +74,8 @@ public Accumulator getInitialValue(ExecutionContext ctx) { @Override public TreeVisitor getScanner(Accumulator acc) { return new TreeVisitor() { + private final Map> closureCache = new HashMap<>(); + @Override public @Nullable Tree visit(@Nullable Tree tree, ExecutionContext ctx) { if (tree == null) { @@ -162,7 +164,7 @@ private void resolveTransitivesFromPom( // Collect all dependencies (both direct and transitive of the parent) Set visited = new HashSet<>(); for (ResolvedDependency dep : resolved) { - collectAllDependencies(dep, transitives, visited); + collectAllDependencies(dep, transitives, visited, effectiveRepos, downloader, ctx); } } catch (MavenDownloadingException | MavenDownloadingExceptions e) { // If we can't download/resolve the POM, fall back to not detecting redundancies @@ -180,11 +182,52 @@ private ResolvedPom applyExclusions(ResolvedPom resolvedPom, List } private void collectAllDependencies(ResolvedDependency dep, Set transitives, - Set visited) { + Set visited, List repositories, + MavenPomDownloader downloader, ExecutionContext ctx) { if (visited.add(dep.getGav())) { - transitives.add(new TransitiveDependency(dep.getGav(), new HashSet<>(dep.getEffectiveExclusions()))); + transitives.add(new TransitiveDependency(dep.getGav(), + relevantExclusions(dep, repositories, downloader, ctx))); + for (ResolvedDependency transitive : dep.getDependencies()) { + collectAllDependencies(transitive, transitives, visited, repositories, downloader, ctx); + } + } + } + + // A dependency's effective exclusions are resolved within the parent's whole tree, so they can + // be polluted by exclusions that other branches declare against artifacts this coordinate could + // never bring on its own (e.g. an optional dependency pruned elsewhere). Keep only the exclusions + // that target something in the coordinate's own dependency closure, so the comparison against a + // clean direct declaration is not thrown off by no-op exclusions. + private Set relevantExclusions(ResolvedDependency dep, List repositories, + MavenPomDownloader downloader, ExecutionContext ctx) { + Set exclusions = new HashSet<>(dep.getEffectiveExclusions()); + if (!exclusions.isEmpty()) { + exclusions.retainAll(dependencyClosure(dep.getGav(), repositories, downloader, ctx)); + } + return exclusions; + } + + private Set dependencyClosure(ResolvedGroupArtifactVersion gav, List repositories, + MavenPomDownloader downloader, ExecutionContext ctx) { + return closureCache.computeIfAbsent(gav, g -> { + Set closure = new HashSet<>(); + try { + Pom pom = downloader.download(g.asGroupArtifactVersion(), null, null, repositories); + ResolvedPom resolvedPom = pom.resolve(emptyList(), downloader, repositories, ctx); + for (ResolvedDependency d : resolvedPom.resolveDependencies(Scope.Compile, downloader, ctx)) { + collectClosure(d, closure); + } + } catch (MavenDownloadingException | MavenDownloadingExceptions e) { + // Best-effort: an unresolvable closure leaves the exclusions unfiltered + } + return closure; + }); + } + + private void collectClosure(ResolvedDependency dep, Set closure) { + if (closure.add(dep.getGav().asGroupArtifact())) { for (ResolvedDependency transitive : dep.getDependencies()) { - collectAllDependencies(transitive, transitives, visited); + collectClosure(transitive, closure); } } } diff --git a/src/test/java/org/openrewrite/java/dependencies/RemoveRedundantDependenciesTest.java b/src/test/java/org/openrewrite/java/dependencies/RemoveRedundantDependenciesTest.java index f163f7da..307a27e6 100644 --- a/src/test/java/org/openrewrite/java/dependencies/RemoveRedundantDependenciesTest.java +++ b/src/test/java/org/openrewrite/java/dependencies/RemoveRedundantDependenciesTest.java @@ -495,6 +495,168 @@ void keepsDirectCompileTomcatEmbedCoreWhenProviderIsProvidedScoped() { ); } + @Test + void removesJakartaClientWhenTransitiveExclusionsAreEquivalent() { + // Resolved within the starter's whole tree, the transitive jakarta.ws.rs-api gains a spurious + // effective exclusion of jakarta.activation-api (which it can never bring on its own), while the + // direct declaration has none; the two are effectively equivalent so the direct one is redundant. + rewriteRun( + spec -> spec.recipe(new RemoveRedundantDependencies( + "org.springframework.boot", "spring-boot-starter-*")), + //language=xml + pomXml( + """ + + 4.0.0 + com.sample + sample + 1.0-SNAPSHOT + + org.springframework.boot + spring-boot-starter-parent + 3.2.3 + + + + + org.springframework.boot + spring-boot-starter-jersey + + + jakarta.ws.rs + jakarta.ws.rs-api + 3.1.0 + + + + """, + """ + + 4.0.0 + com.sample + sample + 1.0-SNAPSHOT + + org.springframework.boot + spring-boot-starter-parent + 3.2.3 + + + + + org.springframework.boot + spring-boot-starter-jersey + + + + """ + ) + ); + } + + @Test + void keepsJerseyClientWhenDirectExclusionsDifferFromTransitive() { + rewriteRun( + spec -> spec.recipe(new RemoveRedundantDependencies( + "org.springframework.boot", "spring-boot-starter-*")), + //language=xml + pomXml( + """ + + 4.0.0 + com.sample + sample + 1.0-SNAPSHOT + + org.springframework.boot + spring-boot-starter-parent + 3.2.3 + + + + + org.springframework.boot + spring-boot-starter-jersey + + + org.glassfish.jersey.core + jersey-client + 3.1.5 + + + jakarta.inject + jakarta.inject-api + + + + + + """ + ) + ); + } + + @Test + void removesTomcatEmbedCoreWhenExclusionsMatchTransitive() { + rewriteRun( + spec -> spec.recipe(new RemoveRedundantDependencies( + "org.springframework.boot", "spring-boot-starter-*")), + //language=xml + pomXml( + """ + + 4.0.0 + com.sample + sample + 1.0-SNAPSHOT + + org.springframework.boot + spring-boot-starter-parent + 3.2.3 + + + + + org.springframework.boot + spring-boot-starter-web + + + org.apache.tomcat.embed + tomcat-embed-core + + + org.apache.tomcat + tomcat-annotations-api + + + + + + """, + """ + + 4.0.0 + com.sample + sample + 1.0-SNAPSHOT + + org.springframework.boot + spring-boot-starter-parent + 3.2.3 + + + + + org.springframework.boot + spring-boot-starter-web + + + + """ + ) + ); + } + @Test void removeRedundantGradleDependency() { rewriteRun( From e5cc17aaab4e4aa2958e026ce06d5b358b5305dc Mon Sep 17 00:00:00 2001 From: Tim te Beek Date: Wed, 22 Jul 2026 23:26:22 +0200 Subject: [PATCH 2/2] Simplify redundant-dependency exclusion filtering Share the POM download/resolve path between resolveTransitivesFromPom and dependencyClosure via resolvePom/withMavenCentral helpers, and hoist the closure cache onto the Accumulator so it survives across source files in a multi-module build. --- .../RemoveRedundantDependencies.java | 48 +++++++++++-------- .../RemoveRedundantDependenciesTest.java | 6 +-- 2 files changed, 30 insertions(+), 24 deletions(-) diff --git a/src/main/java/org/openrewrite/java/dependencies/RemoveRedundantDependencies.java b/src/main/java/org/openrewrite/java/dependencies/RemoveRedundantDependencies.java index 34835a9a..598cad70 100644 --- a/src/main/java/org/openrewrite/java/dependencies/RemoveRedundantDependencies.java +++ b/src/main/java/org/openrewrite/java/dependencies/RemoveRedundantDependencies.java @@ -58,6 +58,8 @@ public class RemoveRedundantDependencies extends ScanningRecipe scope/configuration -> Set of transitive dependencies Map>> transitivesByProjectAndScope; + // Cache of each coordinate's own clean dependency closure, shared across all source files in the run + Map> closureCache; } @Value @@ -68,14 +70,12 @@ public static class TransitiveDependency { @Override public Accumulator getInitialValue(ExecutionContext ctx) { - return new Accumulator(new HashMap<>()); + return new Accumulator(new HashMap<>(), new HashMap<>()); } @Override public TreeVisitor getScanner(Accumulator acc) { return new TreeVisitor() { - private final Map> closureCache = new HashMap<>(); - @Override public @Nullable Tree visit(@Nullable Tree tree, ExecutionContext ctx) { if (tree == null) { @@ -147,17 +147,10 @@ private void resolveTransitivesFromPom( MavenPomDownloader downloader, ExecutionContext ctx, Set transitives) { + List effectiveRepos = withMavenCentral(repositories); try { - // Ensure we have Maven Central in the repositories - List effectiveRepos = new ArrayList<>(repositories); - if (effectiveRepos.stream().noneMatch(r -> r.getUri().contains("repo.maven.apache.org") || - r.getUri().contains("repo1.maven.org"))) { - effectiveRepos.add(MavenRepository.MAVEN_CENTRAL); - } - // Get the resolved dependencies for compile scope (which includes most transitives) - Pom pom = downloader.download(gav.asGroupArtifactVersion(), null, null, effectiveRepos); - ResolvedPom resolvedPom = pom.resolve(emptyList(), downloader, effectiveRepos, ctx); + ResolvedPom resolvedPom = resolvePom(gav, effectiveRepos, downloader, ctx); ResolvedPom patchedPom = applyExclusions(resolvedPom, effectiveExclusions); List resolved = patchedPom.resolveDependencies(Scope.Compile, downloader, ctx); @@ -193,11 +186,8 @@ private void collectAllDependencies(ResolvedDependency dep, Set relevantExclusions(ResolvedDependency dep, List repositories, MavenPomDownloader downloader, ExecutionContext ctx) { Set exclusions = new HashSet<>(dep.getEffectiveExclusions()); @@ -209,12 +199,11 @@ private Set relevantExclusions(ResolvedDependency dep, List dependencyClosure(ResolvedGroupArtifactVersion gav, List repositories, MavenPomDownloader downloader, ExecutionContext ctx) { - return closureCache.computeIfAbsent(gav, g -> { + return acc.closureCache.computeIfAbsent(gav, g -> { Set closure = new HashSet<>(); try { - Pom pom = downloader.download(g.asGroupArtifactVersion(), null, null, repositories); - ResolvedPom resolvedPom = pom.resolve(emptyList(), downloader, repositories, ctx); - for (ResolvedDependency d : resolvedPom.resolveDependencies(Scope.Compile, downloader, ctx)) { + for (ResolvedDependency d : resolvePom(g, repositories, downloader, ctx) + .resolveDependencies(Scope.Compile, downloader, ctx)) { collectClosure(d, closure); } } catch (MavenDownloadingException | MavenDownloadingExceptions e) { @@ -224,6 +213,23 @@ private Set dependencyClosure(ResolvedGroupArtifactVersion gav, L }); } + private ResolvedPom resolvePom(ResolvedGroupArtifactVersion gav, List repositories, + MavenPomDownloader downloader, ExecutionContext ctx) + throws MavenDownloadingException, MavenDownloadingExceptions { + List repos = withMavenCentral(repositories); + Pom pom = downloader.download(gav.asGroupArtifactVersion(), null, null, repos); + return pom.resolve(emptyList(), downloader, repos, ctx); + } + + private List withMavenCentral(List repositories) { + List effectiveRepos = new ArrayList<>(repositories); + if (effectiveRepos.stream().noneMatch(r -> r.getUri().contains("repo.maven.apache.org") || + r.getUri().contains("repo1.maven.org"))) { + effectiveRepos.add(MavenRepository.MAVEN_CENTRAL); + } + return effectiveRepos; + } + private void collectClosure(ResolvedDependency dep, Set closure) { if (closure.add(dep.getGav().asGroupArtifact())) { for (ResolvedDependency transitive : dep.getDependencies()) { diff --git a/src/test/java/org/openrewrite/java/dependencies/RemoveRedundantDependenciesTest.java b/src/test/java/org/openrewrite/java/dependencies/RemoveRedundantDependenciesTest.java index 307a27e6..084e035f 100644 --- a/src/test/java/org/openrewrite/java/dependencies/RemoveRedundantDependenciesTest.java +++ b/src/test/java/org/openrewrite/java/dependencies/RemoveRedundantDependenciesTest.java @@ -497,9 +497,9 @@ void keepsDirectCompileTomcatEmbedCoreWhenProviderIsProvidedScoped() { @Test void removesJakartaClientWhenTransitiveExclusionsAreEquivalent() { - // Resolved within the starter's whole tree, the transitive jakarta.ws.rs-api gains a spurious - // effective exclusion of jakarta.activation-api (which it can never bring on its own), while the - // direct declaration has none; the two are effectively equivalent so the direct one is redundant. + // In the starter's tree the transitive jakarta.ws.rs-api gains a spurious exclusion of + // jakarta.activation-api (which it can never bring on its own); the direct declaration has none, + // so the two are effectively equivalent and the direct one is redundant. rewriteRun( spec -> spec.recipe(new RemoveRedundantDependencies( "org.springframework.boot", "spring-boot-starter-*")),