Skip to content

Commit 6e336a7

Browse files
timtebeektimtebeek
committed
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.
1 parent 3aa541b commit 6e336a7

2 files changed

Lines changed: 209 additions & 4 deletions

File tree

‎src/main/java/org/openrewrite/java/dependencies/RemoveRedundantDependencies.java‎

Lines changed: 47 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,8 @@ public Accumulator getInitialValue(ExecutionContext ctx) {
7474
@Override
7575
public TreeVisitor<?, ExecutionContext> getScanner(Accumulator acc) {
7676
return new TreeVisitor<Tree, ExecutionContext>() {
77+
private final Map<ResolvedGroupArtifactVersion, Set<GroupArtifact>> closureCache = new HashMap<>();
78+
7779
@Override
7880
public @Nullable Tree visit(@Nullable Tree tree, ExecutionContext ctx) {
7981
if (tree == null) {
@@ -162,7 +164,7 @@ private void resolveTransitivesFromPom(
162164
// Collect all dependencies (both direct and transitive of the parent)
163165
Set<ResolvedGroupArtifactVersion> visited = new HashSet<>();
164166
for (ResolvedDependency dep : resolved) {
165-
collectAllDependencies(dep, transitives, visited);
167+
collectAllDependencies(dep, transitives, visited, effectiveRepos, downloader, ctx);
166168
}
167169
} catch (MavenDownloadingException | MavenDownloadingExceptions e) {
168170
// If we can't download/resolve the POM, fall back to not detecting redundancies
@@ -180,11 +182,52 @@ private ResolvedPom applyExclusions(ResolvedPom resolvedPom, List<GroupArtifact>
180182
}
181183

182184
private void collectAllDependencies(ResolvedDependency dep, Set<TransitiveDependency> transitives,
183-
Set<ResolvedGroupArtifactVersion> visited) {
185+
Set<ResolvedGroupArtifactVersion> visited, List<MavenRepository> repositories,
186+
MavenPomDownloader downloader, ExecutionContext ctx) {
184187
if (visited.add(dep.getGav())) {
185-
transitives.add(new TransitiveDependency(dep.getGav(), new HashSet<>(dep.getEffectiveExclusions())));
188+
transitives.add(new TransitiveDependency(dep.getGav(),
189+
relevantExclusions(dep, repositories, downloader, ctx)));
190+
for (ResolvedDependency transitive : dep.getDependencies()) {
191+
collectAllDependencies(transitive, transitives, visited, repositories, downloader, ctx);
192+
}
193+
}
194+
}
195+
196+
// A dependency's effective exclusions are resolved within the parent's whole tree, so they can
197+
// be polluted by exclusions that other branches declare against artifacts this coordinate could
198+
// never bring on its own (e.g. an optional dependency pruned elsewhere). Keep only the exclusions
199+
// that target something in the coordinate's own dependency closure, so the comparison against a
200+
// clean direct declaration is not thrown off by no-op exclusions.
201+
private Set<GroupArtifact> relevantExclusions(ResolvedDependency dep, List<MavenRepository> repositories,
202+
MavenPomDownloader downloader, ExecutionContext ctx) {
203+
Set<GroupArtifact> exclusions = new HashSet<>(dep.getEffectiveExclusions());
204+
if (!exclusions.isEmpty()) {
205+
exclusions.retainAll(dependencyClosure(dep.getGav(), repositories, downloader, ctx));
206+
}
207+
return exclusions;
208+
}
209+
210+
private Set<GroupArtifact> dependencyClosure(ResolvedGroupArtifactVersion gav, List<MavenRepository> repositories,
211+
MavenPomDownloader downloader, ExecutionContext ctx) {
212+
return closureCache.computeIfAbsent(gav, g -> {
213+
Set<GroupArtifact> closure = new HashSet<>();
214+
try {
215+
Pom pom = downloader.download(g.asGroupArtifactVersion(), null, null, repositories);
216+
ResolvedPom resolvedPom = pom.resolve(emptyList(), downloader, repositories, ctx);
217+
for (ResolvedDependency d : resolvedPom.resolveDependencies(Scope.Compile, downloader, ctx)) {
218+
collectClosure(d, closure);
219+
}
220+
} catch (MavenDownloadingException | MavenDownloadingExceptions e) {
221+
// Best-effort: an unresolvable closure leaves the exclusions unfiltered
222+
}
223+
return closure;
224+
});
225+
}
226+
227+
private void collectClosure(ResolvedDependency dep, Set<GroupArtifact> closure) {
228+
if (closure.add(dep.getGav().asGroupArtifact())) {
186229
for (ResolvedDependency transitive : dep.getDependencies()) {
187-
collectAllDependencies(transitive, transitives, visited);
230+
collectClosure(transitive, closure);
188231
}
189232
}
190233
}

‎src/test/java/org/openrewrite/java/dependencies/RemoveRedundantDependenciesTest.java‎

Lines changed: 162 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -495,6 +495,168 @@ void keepsDirectCompileTomcatEmbedCoreWhenProviderIsProvidedScoped() {
495495
);
496496
}
497497

498+
@Test
499+
void removesJakartaClientWhenTransitiveExclusionsAreEquivalent() {
500+
// Resolved within the starter's whole tree, the transitive jakarta.ws.rs-api gains a spurious
501+
// effective exclusion of jakarta.activation-api (which it can never bring on its own), while the
502+
// direct declaration has none; the two are effectively equivalent so the direct one is redundant.
503+
rewriteRun(
504+
spec -> spec.recipe(new RemoveRedundantDependencies(
505+
"org.springframework.boot", "spring-boot-starter-*")),
506+
//language=xml
507+
pomXml(
508+
"""
509+
<project>
510+
<modelVersion>4.0.0</modelVersion>
511+
<groupId>com.sample</groupId>
512+
<artifactId>sample</artifactId>
513+
<version>1.0-SNAPSHOT</version>
514+
<parent>
515+
<groupId>org.springframework.boot</groupId>
516+
<artifactId>spring-boot-starter-parent</artifactId>
517+
<version>3.2.3</version>
518+
<relativePath/>
519+
</parent>
520+
<dependencies>
521+
<dependency>
522+
<groupId>org.springframework.boot</groupId>
523+
<artifactId>spring-boot-starter-jersey</artifactId>
524+
</dependency>
525+
<dependency>
526+
<groupId>jakarta.ws.rs</groupId>
527+
<artifactId>jakarta.ws.rs-api</artifactId>
528+
<version>3.1.0</version>
529+
</dependency>
530+
</dependencies>
531+
</project>
532+
""",
533+
"""
534+
<project>
535+
<modelVersion>4.0.0</modelVersion>
536+
<groupId>com.sample</groupId>
537+
<artifactId>sample</artifactId>
538+
<version>1.0-SNAPSHOT</version>
539+
<parent>
540+
<groupId>org.springframework.boot</groupId>
541+
<artifactId>spring-boot-starter-parent</artifactId>
542+
<version>3.2.3</version>
543+
<relativePath/>
544+
</parent>
545+
<dependencies>
546+
<dependency>
547+
<groupId>org.springframework.boot</groupId>
548+
<artifactId>spring-boot-starter-jersey</artifactId>
549+
</dependency>
550+
</dependencies>
551+
</project>
552+
"""
553+
)
554+
);
555+
}
556+
557+
@Test
558+
void keepsJerseyClientWhenDirectExclusionsDifferFromTransitive() {
559+
rewriteRun(
560+
spec -> spec.recipe(new RemoveRedundantDependencies(
561+
"org.springframework.boot", "spring-boot-starter-*")),
562+
//language=xml
563+
pomXml(
564+
"""
565+
<project>
566+
<modelVersion>4.0.0</modelVersion>
567+
<groupId>com.sample</groupId>
568+
<artifactId>sample</artifactId>
569+
<version>1.0-SNAPSHOT</version>
570+
<parent>
571+
<groupId>org.springframework.boot</groupId>
572+
<artifactId>spring-boot-starter-parent</artifactId>
573+
<version>3.2.3</version>
574+
<relativePath/>
575+
</parent>
576+
<dependencies>
577+
<dependency>
578+
<groupId>org.springframework.boot</groupId>
579+
<artifactId>spring-boot-starter-jersey</artifactId>
580+
</dependency>
581+
<dependency>
582+
<groupId>org.glassfish.jersey.core</groupId>
583+
<artifactId>jersey-client</artifactId>
584+
<version>3.1.5</version>
585+
<exclusions>
586+
<exclusion>
587+
<groupId>jakarta.inject</groupId>
588+
<artifactId>jakarta.inject-api</artifactId>
589+
</exclusion>
590+
</exclusions>
591+
</dependency>
592+
</dependencies>
593+
</project>
594+
"""
595+
)
596+
);
597+
}
598+
599+
@Test
600+
void removesTomcatEmbedCoreWhenExclusionsMatchTransitive() {
601+
rewriteRun(
602+
spec -> spec.recipe(new RemoveRedundantDependencies(
603+
"org.springframework.boot", "spring-boot-starter-*")),
604+
//language=xml
605+
pomXml(
606+
"""
607+
<project>
608+
<modelVersion>4.0.0</modelVersion>
609+
<groupId>com.sample</groupId>
610+
<artifactId>sample</artifactId>
611+
<version>1.0-SNAPSHOT</version>
612+
<parent>
613+
<groupId>org.springframework.boot</groupId>
614+
<artifactId>spring-boot-starter-parent</artifactId>
615+
<version>3.2.3</version>
616+
<relativePath/>
617+
</parent>
618+
<dependencies>
619+
<dependency>
620+
<groupId>org.springframework.boot</groupId>
621+
<artifactId>spring-boot-starter-web</artifactId>
622+
</dependency>
623+
<dependency>
624+
<groupId>org.apache.tomcat.embed</groupId>
625+
<artifactId>tomcat-embed-core</artifactId>
626+
<exclusions>
627+
<exclusion>
628+
<groupId>org.apache.tomcat</groupId>
629+
<artifactId>tomcat-annotations-api</artifactId>
630+
</exclusion>
631+
</exclusions>
632+
</dependency>
633+
</dependencies>
634+
</project>
635+
""",
636+
"""
637+
<project>
638+
<modelVersion>4.0.0</modelVersion>
639+
<groupId>com.sample</groupId>
640+
<artifactId>sample</artifactId>
641+
<version>1.0-SNAPSHOT</version>
642+
<parent>
643+
<groupId>org.springframework.boot</groupId>
644+
<artifactId>spring-boot-starter-parent</artifactId>
645+
<version>3.2.3</version>
646+
<relativePath/>
647+
</parent>
648+
<dependencies>
649+
<dependency>
650+
<groupId>org.springframework.boot</groupId>
651+
<artifactId>spring-boot-starter-web</artifactId>
652+
</dependency>
653+
</dependencies>
654+
</project>
655+
"""
656+
)
657+
);
658+
}
659+
498660
@Test
499661
void removeRedundantGradleDependency() {
500662
rewriteRun(

0 commit comments

Comments
 (0)