Skip to content

Commit 2d49d4a

Browse files
timtebeektimtebeek
committed
Compare declared exclusions, not effective ones, for redundant dependencies
Within a large dependency tree a coordinate's effective exclusions are corrupted by dependency mediation: an artifact excluded on one path may already have been pruned at a shared node, so it silently drops out of getEffectiveExclusions(). That inverted RemoveRedundantDependencies for e.g. spring-boot-starter-web -> spring-boot-starter-tomcat -> tomcat-embed-websocket (openrewrite/rewrite#8336). Compare each coordinate's declared (requested) exclusions instead, intersected with its own clean closure to drop no-op exclusions, and apply the same relevant -exclusion logic to the direct declaration being evaluated for removal. Fixes openrewrite/rewrite#8336
1 parent e5cc17a commit 2d49d4a

2 files changed

Lines changed: 169 additions & 53 deletions

File tree

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

Lines changed: 65 additions & 53 deletions
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,7 @@ public class RemoveRedundantDependencies extends ScanningRecipe<RemoveRedundantD
5252
"This recipe downloads and resolves the parent dependency's POM to determine its true transitive " +
5353
"dependencies, allowing it to detect redundancies even when both dependencies are explicitly declared. " +
5454
"A direct dependency is only removed when the transitive one provides it at the exact same scope and " +
55-
"with the same exclusions, so that removing it does not change the effective classpath.";
55+
"with the same effective exclusions, so that removing it does not change the effective classpath.";
5656

5757
@Value
5858
public static class Accumulator {
@@ -150,7 +150,8 @@ private void resolveTransitivesFromPom(
150150
List<MavenRepository> effectiveRepos = withMavenCentral(repositories);
151151
try {
152152
// Get the resolved dependencies for compile scope (which includes most transitives)
153-
ResolvedPom resolvedPom = resolvePom(gav, effectiveRepos, downloader, ctx);
153+
Pom pom = downloader.download(gav.asGroupArtifactVersion(), null, null, effectiveRepos);
154+
ResolvedPom resolvedPom = pom.resolve(emptyList(), downloader, effectiveRepos, ctx);
154155
ResolvedPom patchedPom = applyExclusions(resolvedPom, effectiveExclusions);
155156
List<ResolvedDependency> resolved = patchedPom.resolveDependencies(Scope.Compile, downloader, ctx);
156157

@@ -179,65 +180,73 @@ private void collectAllDependencies(ResolvedDependency dep, Set<TransitiveDepend
179180
MavenPomDownloader downloader, ExecutionContext ctx) {
180181
if (visited.add(dep.getGav())) {
181182
transitives.add(new TransitiveDependency(dep.getGav(),
182-
relevantExclusions(dep, repositories, downloader, ctx)));
183+
relevantExclusions(dep, repositories, downloader, ctx, acc.closureCache)));
183184
for (ResolvedDependency transitive : dep.getDependencies()) {
184185
collectAllDependencies(transitive, transitives, visited, repositories, downloader, ctx);
185186
}
186187
}
187188
}
189+
};
190+
}
188191

189-
// Effective exclusions are resolved in the parent's whole tree, so they pick up no-op exclusions
190-
// from sibling branches; keep only those targeting this coordinate's own dependency closure.
191-
private Set<GroupArtifact> relevantExclusions(ResolvedDependency dep, List<MavenRepository> repositories,
192-
MavenPomDownloader downloader, ExecutionContext ctx) {
193-
Set<GroupArtifact> exclusions = new HashSet<>(dep.getEffectiveExclusions());
194-
if (!exclusions.isEmpty()) {
195-
exclusions.retainAll(dependencyClosure(dep.getGav(), repositories, downloader, ctx));
196-
}
197-
return exclusions;
198-
}
192+
// Compares a coordinate's declared (requested) exclusions rather than its effective ones: within a large
193+
// tree the effective set is corrupted by dependency mediation (an artifact excluded here may already have
194+
// been pruned by a shared node elsewhere, so it silently drops out of getEffectiveExclusions). The declared
195+
// exclusions are then intersected with the coordinate's own clean closure, dropping no-op exclusions that
196+
// target artifacts the coordinate could never bring on its own. Comparing this set on the direct and the
197+
// transitively-provided declaration tells us whether removing the direct one changes the effective classpath.
198+
private static Set<GroupArtifact> relevantExclusions(ResolvedDependency dep, List<MavenRepository> repositories,
199+
MavenPomDownloader downloader, ExecutionContext ctx,
200+
Map<ResolvedGroupArtifactVersion, Set<GroupArtifact>> closureCache) {
201+
List<GroupArtifact> requested = dep.getRequested() == null ? null : dep.getRequested().getExclusions();
202+
if (requested == null || requested.isEmpty()) {
203+
return emptySet();
204+
}
205+
Set<GroupArtifact> exclusions = new HashSet<>(requested);
206+
exclusions.retainAll(dependencyClosure(dep.getGav(), repositories, downloader, ctx, closureCache));
207+
return exclusions;
208+
}
199209

200-
private Set<GroupArtifact> dependencyClosure(ResolvedGroupArtifactVersion gav, List<MavenRepository> repositories,
201-
MavenPomDownloader downloader, ExecutionContext ctx) {
202-
return acc.closureCache.computeIfAbsent(gav, g -> {
203-
Set<GroupArtifact> closure = new HashSet<>();
204-
try {
205-
for (ResolvedDependency d : resolvePom(g, repositories, downloader, ctx)
206-
.resolveDependencies(Scope.Compile, downloader, ctx)) {
207-
collectClosure(d, closure);
208-
}
209-
} catch (MavenDownloadingException | MavenDownloadingExceptions e) {
210-
// Best-effort: an unresolvable closure leaves the exclusions unfiltered
211-
}
212-
return closure;
213-
});
210+
private static Set<GroupArtifact> dependencyClosure(ResolvedGroupArtifactVersion gav, List<MavenRepository> repositories,
211+
MavenPomDownloader downloader, ExecutionContext ctx,
212+
Map<ResolvedGroupArtifactVersion, Set<GroupArtifact>> closureCache) {
213+
return closureCache.computeIfAbsent(gav, g -> {
214+
Set<GroupArtifact> closure = new HashSet<>();
215+
try {
216+
for (ResolvedDependency d : resolvePom(g, repositories, downloader, ctx)
217+
.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
214222
}
223+
return closure;
224+
});
225+
}
215226

216-
private ResolvedPom resolvePom(ResolvedGroupArtifactVersion gav, List<MavenRepository> repositories,
217-
MavenPomDownloader downloader, ExecutionContext ctx)
218-
throws MavenDownloadingException, MavenDownloadingExceptions {
219-
List<MavenRepository> repos = withMavenCentral(repositories);
220-
Pom pom = downloader.download(gav.asGroupArtifactVersion(), null, null, repos);
221-
return pom.resolve(emptyList(), downloader, repos, ctx);
222-
}
227+
private static ResolvedPom resolvePom(ResolvedGroupArtifactVersion gav, List<MavenRepository> repositories,
228+
MavenPomDownloader downloader, ExecutionContext ctx)
229+
throws MavenDownloadingException, MavenDownloadingExceptions {
230+
List<MavenRepository> repos = withMavenCentral(repositories);
231+
Pom pom = downloader.download(gav.asGroupArtifactVersion(), null, null, repos);
232+
return pom.resolve(emptyList(), downloader, repos, ctx);
233+
}
223234

224-
private List<MavenRepository> withMavenCentral(List<MavenRepository> repositories) {
225-
List<MavenRepository> effectiveRepos = new ArrayList<>(repositories);
226-
if (effectiveRepos.stream().noneMatch(r -> r.getUri().contains("repo.maven.apache.org") ||
227-
r.getUri().contains("repo1.maven.org"))) {
228-
effectiveRepos.add(MavenRepository.MAVEN_CENTRAL);
229-
}
230-
return effectiveRepos;
231-
}
235+
private static List<MavenRepository> withMavenCentral(List<MavenRepository> repositories) {
236+
List<MavenRepository> effectiveRepos = new ArrayList<>(repositories);
237+
if (effectiveRepos.stream().noneMatch(r -> r.getUri().contains("repo.maven.apache.org") ||
238+
r.getUri().contains("repo1.maven.org"))) {
239+
effectiveRepos.add(MavenRepository.MAVEN_CENTRAL);
240+
}
241+
return effectiveRepos;
242+
}
232243

233-
private void collectClosure(ResolvedDependency dep, Set<GroupArtifact> closure) {
234-
if (closure.add(dep.getGav().asGroupArtifact())) {
235-
for (ResolvedDependency transitive : dep.getDependencies()) {
236-
collectClosure(transitive, closure);
237-
}
238-
}
244+
private static void collectClosure(ResolvedDependency dep, Set<GroupArtifact> closure) {
245+
if (closure.add(dep.getGav().asGroupArtifact())) {
246+
for (ResolvedDependency transitive : dep.getDependencies()) {
247+
collectClosure(transitive, closure);
239248
}
240-
};
249+
}
241250
}
242251

243252
@Override
@@ -268,6 +277,7 @@ public TreeVisitor<?, ExecutionContext> getVisitor(Accumulator acc) {
268277
String projectId = gradle.getGroup() + ":" + gradle.getName();
269278
Map<String, Set<TransitiveDependency>> scopeToTransitives =
270279
acc.transitivesByProjectAndScope.getOrDefault(projectId, emptyMap());
280+
MavenPomDownloader downloader = new MavenPomDownloader(ctx);
271281

272282
for (GradleDependencyConfiguration conf : gradle.getConfigurations()) {
273283
Set<TransitiveDependency> transitives = getCompatibleGradleTransitives(
@@ -279,7 +289,7 @@ public TreeVisitor<?, ExecutionContext> getVisitor(Accumulator acc) {
279289
for (ResolvedDependency dep : conf.getResolved()) {
280290
if (dep.isDirect() &&
281291
doesNotMatchArguments(dep) &&
282-
isRedundant(dep, transitives)) {
292+
isRedundant(dep, transitives, gradle.getMavenRepositories(), downloader, ctx)) {
283293
// This direct dependency is transitively provided, remove it
284294
// Don't specify configuration - Gradle's resolved config names differ from declaration names
285295
result = new RemoveDependency(
@@ -295,6 +305,7 @@ public TreeVisitor<?, ExecutionContext> getVisitor(Accumulator acc) {
295305
String projectId = maven.getPom().getGroupId() + ":" + maven.getPom().getArtifactId();
296306
Map<String, Set<TransitiveDependency>> scopeToTransitives =
297307
acc.transitivesByProjectAndScope.getOrDefault(projectId, emptyMap());
308+
MavenPomDownloader downloader = new MavenPomDownloader(ctx);
298309

299310
// A direct dependency appears under every scope bucket it is visible in; evaluate each
300311
// one once using its own effective scope so a wider transitive scope does not falsely
@@ -308,7 +319,7 @@ public TreeVisitor<?, ExecutionContext> getVisitor(Accumulator acc) {
308319
Scope depScope = Scope.fromName(dep.getRequested().getScope());
309320
Set<TransitiveDependency> transitives = scopeToTransitives.getOrDefault(
310321
depScope.name().toLowerCase(), emptySet());
311-
if (isRedundant(dep, transitives)) {
322+
if (isRedundant(dep, transitives, maven.getPom().getRepositories(), downloader, ctx)) {
312323
// This direct dependency is transitively provided at the same scope and
313324
// with the same exclusions, remove it.
314325
result = new RemoveDependency(
@@ -326,8 +337,9 @@ private boolean doesNotMatchArguments(ResolvedDependency dep) {
326337
!StringUtils.matchesGlob(dep.getArtifactId(), artifactId);
327338
}
328339

329-
private boolean isRedundant(ResolvedDependency dep, Set<TransitiveDependency> transitives) {
330-
Set<GroupArtifact> depExclusions = new HashSet<>(dep.getEffectiveExclusions());
340+
private boolean isRedundant(ResolvedDependency dep, Set<TransitiveDependency> transitives,
341+
List<MavenRepository> repositories, MavenPomDownloader downloader, ExecutionContext ctx) {
342+
Set<GroupArtifact> depExclusions = relevantExclusions(dep, repositories, downloader, ctx, acc.closureCache);
331343
for (TransitiveDependency transitive : transitives) {
332344
ResolvedGroupArtifactVersion gav = transitive.getGav();
333345
if (dep.getGroupId().equals(gav.getGroupId()) &&

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

Lines changed: 104 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -657,6 +657,110 @@ void removesTomcatEmbedCoreWhenExclusionsMatchTransitive() {
657657
);
658658
}
659659

660+
@Test
661+
void keepsWebsocketWhenDirectHasNoExclusionButTransitiveDoes() {
662+
// https://github.com/openrewrite/rewrite/issues/8336
663+
// spring-boot-starter-tomcat excludes tomcat-annotations-api from tomcat-embed-websocket, which still
664+
// brings its own tomcat-embed-core. A direct websocket with no exclusion is therefore NOT equivalent to
665+
// the transitively-provided one and must be kept, even though websocket's effective-exclusion set is
666+
// emptied by mediation (annotations-api is pruned at the shared embed-core node first).
667+
rewriteRun(
668+
spec -> spec.recipe(new RemoveRedundantDependencies(
669+
"org.springframework.boot", "spring-boot-starter-web")),
670+
//language=xml
671+
pomXml(
672+
"""
673+
<project>
674+
<modelVersion>4.0.0</modelVersion>
675+
<groupId>com.sample</groupId>
676+
<artifactId>sample</artifactId>
677+
<version>1.0-SNAPSHOT</version>
678+
<parent>
679+
<groupId>org.springframework.boot</groupId>
680+
<artifactId>spring-boot-starter-parent</artifactId>
681+
<version>3.2.3</version>
682+
<relativePath/>
683+
</parent>
684+
<dependencies>
685+
<dependency>
686+
<groupId>org.springframework.boot</groupId>
687+
<artifactId>spring-boot-starter-web</artifactId>
688+
</dependency>
689+
<dependency>
690+
<groupId>org.apache.tomcat.embed</groupId>
691+
<artifactId>tomcat-embed-websocket</artifactId>
692+
</dependency>
693+
</dependencies>
694+
</project>
695+
"""
696+
)
697+
);
698+
}
699+
700+
@Test
701+
void removesWebsocketWhenDirectRepeatsTheTransitiveExclusion() {
702+
// https://github.com/openrewrite/rewrite/issues/8336
703+
// The mirror image: a direct websocket that repeats the same tomcat-annotations-api exclusion the
704+
// starter applies is truly equivalent to the transitively-provided one and should be removed.
705+
rewriteRun(
706+
spec -> spec.recipe(new RemoveRedundantDependencies(
707+
"org.springframework.boot", "spring-boot-starter-web")),
708+
//language=xml
709+
pomXml(
710+
"""
711+
<project>
712+
<modelVersion>4.0.0</modelVersion>
713+
<groupId>com.sample</groupId>
714+
<artifactId>sample</artifactId>
715+
<version>1.0-SNAPSHOT</version>
716+
<parent>
717+
<groupId>org.springframework.boot</groupId>
718+
<artifactId>spring-boot-starter-parent</artifactId>
719+
<version>3.2.3</version>
720+
<relativePath/>
721+
</parent>
722+
<dependencies>
723+
<dependency>
724+
<groupId>org.springframework.boot</groupId>
725+
<artifactId>spring-boot-starter-web</artifactId>
726+
</dependency>
727+
<dependency>
728+
<groupId>org.apache.tomcat.embed</groupId>
729+
<artifactId>tomcat-embed-websocket</artifactId>
730+
<exclusions>
731+
<exclusion>
732+
<groupId>org.apache.tomcat</groupId>
733+
<artifactId>tomcat-annotations-api</artifactId>
734+
</exclusion>
735+
</exclusions>
736+
</dependency>
737+
</dependencies>
738+
</project>
739+
""",
740+
"""
741+
<project>
742+
<modelVersion>4.0.0</modelVersion>
743+
<groupId>com.sample</groupId>
744+
<artifactId>sample</artifactId>
745+
<version>1.0-SNAPSHOT</version>
746+
<parent>
747+
<groupId>org.springframework.boot</groupId>
748+
<artifactId>spring-boot-starter-parent</artifactId>
749+
<version>3.2.3</version>
750+
<relativePath/>
751+
</parent>
752+
<dependencies>
753+
<dependency>
754+
<groupId>org.springframework.boot</groupId>
755+
<artifactId>spring-boot-starter-web</artifactId>
756+
</dependency>
757+
</dependencies>
758+
</project>
759+
"""
760+
)
761+
);
762+
}
763+
660764
@Test
661765
void removeRedundantGradleDependency() {
662766
rewriteRun(

0 commit comments

Comments
 (0)