Skip to content

Commit 7e93ded

Browse files
authored
Fix inverted exclusion comparison in RemoveRedundantDependencies (#191)
* 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. * 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. * 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 * Resolve dependency closures lazily when comparing exclusions Closure resolution was eager: every visited node carrying an inherited exclusion triggered a POM download, resolve and compile-scope graph resolution. Since ancestor exclusions are propagated into each child's requested dependency, the "no exclusions" early-out rarely fired, so a spring-boot-starter tree performed ~38 extra resolutions of which one was ever consulted. TransitiveDependency now carries declared exclusions, and the closure intersection is deferred into isRedundant, where it runs only for a coordinate that actually matched and only when the two declared sets differ. Both sides share a single closure for that coordinate. Also collapse the (repositories, downloader, ctx, cache) tuple into a ClosureResolver, dedupe the two recursive dependency walks, key the cache on GroupArtifactVersion so it is not repository-sensitive, and correct the description and comments that described the abandoned mechanism. * Drop redundant null check on non-null getRequested() * Name the closure cache for what it holds * Regenerate recipes.csv * Keep dependencies whose declared exclusions differ Comparing declared exclusions still required resolving each coordinate's own dependency closure to discard exclusions it could never have honoured, costing an isolated POM download and graph resolve per candidate. Treat any difference in declared exclusions as "not redundant" instead. This is strictly conservative: it can only keep a dependency that was previously removed, never the reverse. The cost is that a transitive which merely inherited an unrelated exclusion from an ancestor no longer compares equal to a direct declaration carrying none, so jakarta.ws.rs-api is now kept rather than removed. That test is retained, inverted, to document the limitation. * Regenerate recipes.csv
1 parent 05c32f7 commit 7e93ded

3 files changed

Lines changed: 290 additions & 31 deletions

File tree

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

Lines changed: 24 additions & 14 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 declared exclusions, so that removing it does not change the effective classpath.";
5656

5757
@Value
5858
public static class Accumulator {
@@ -145,23 +145,16 @@ private void resolveTransitivesFromPom(
145145
MavenPomDownloader downloader,
146146
ExecutionContext ctx,
147147
Set<TransitiveDependency> transitives) {
148+
List<MavenRepository> repos = withMavenCentral(repositories);
148149
try {
149-
// Ensure we have Maven Central in the repositories
150-
List<MavenRepository> effectiveRepos = new ArrayList<>(repositories);
151-
if (effectiveRepos.stream().noneMatch(r -> r.getUri().contains("repo.maven.apache.org") ||
152-
r.getUri().contains("repo1.maven.org"))) {
153-
effectiveRepos.add(MavenRepository.MAVEN_CENTRAL);
154-
}
155-
156150
// Get the resolved dependencies for compile scope (which includes most transitives)
157-
Pom pom = downloader.download(gav.asGroupArtifactVersion(), null, null, effectiveRepos);
158-
ResolvedPom resolvedPom = pom.resolve(emptyList(), downloader, effectiveRepos, ctx);
151+
Pom pom = downloader.download(gav.asGroupArtifactVersion(), null, null, repos);
152+
ResolvedPom resolvedPom = pom.resolve(emptyList(), downloader, repos, ctx);
159153
ResolvedPom patchedPom = applyExclusions(resolvedPom, effectiveExclusions);
160-
List<ResolvedDependency> resolved = patchedPom.resolveDependencies(Scope.Compile, downloader, ctx);
161154

162155
// Collect all dependencies (both direct and transitive of the parent)
163156
Set<ResolvedGroupArtifactVersion> visited = new HashSet<>();
164-
for (ResolvedDependency dep : resolved) {
157+
for (ResolvedDependency dep : patchedPom.resolveDependencies(Scope.Compile, downloader, ctx)) {
165158
collectAllDependencies(dep, transitives, visited);
166159
}
167160
} catch (MavenDownloadingException | MavenDownloadingExceptions e) {
@@ -182,7 +175,7 @@ private ResolvedPom applyExclusions(ResolvedPom resolvedPom, List<GroupArtifact>
182175
private void collectAllDependencies(ResolvedDependency dep, Set<TransitiveDependency> transitives,
183176
Set<ResolvedGroupArtifactVersion> visited) {
184177
if (visited.add(dep.getGav())) {
185-
transitives.add(new TransitiveDependency(dep.getGav(), new HashSet<>(dep.getEffectiveExclusions())));
178+
transitives.add(new TransitiveDependency(dep.getGav(), declaredExclusions(dep)));
186179
for (ResolvedDependency transitive : dep.getDependencies()) {
187180
collectAllDependencies(transitive, transitives, visited);
188181
}
@@ -191,6 +184,23 @@ private void collectAllDependencies(ResolvedDependency dep, Set<TransitiveDepend
191184
};
192185
}
193186

187+
// Declared (requested) exclusions rather than effective ones: within a large tree the effective set is
188+
// corrupted by dependency mediation, as an artifact excluded here may already have been pruned by a shared
189+
// node elsewhere, in which case the exclusion never fires and drops out of getEffectiveExclusions().
190+
private static Set<GroupArtifact> declaredExclusions(ResolvedDependency dep) {
191+
List<GroupArtifact> exclusions = dep.getRequested().getExclusions();
192+
return exclusions == null || exclusions.isEmpty() ? emptySet() : new HashSet<>(exclusions);
193+
}
194+
195+
private static List<MavenRepository> withMavenCentral(List<MavenRepository> repositories) {
196+
List<MavenRepository> effectiveRepos = new ArrayList<>(repositories);
197+
if (effectiveRepos.stream().noneMatch(r -> r.getUri().contains("repo.maven.apache.org") ||
198+
r.getUri().contains("repo1.maven.org"))) {
199+
effectiveRepos.add(MavenRepository.MAVEN_CENTRAL);
200+
}
201+
return effectiveRepos;
202+
}
203+
194204
@Override
195205
public TreeVisitor<?, ExecutionContext> getVisitor(Accumulator acc) {
196206
return new TreeVisitor<Tree, ExecutionContext>() {
@@ -278,7 +288,7 @@ private boolean doesNotMatchArguments(ResolvedDependency dep) {
278288
}
279289

280290
private boolean isRedundant(ResolvedDependency dep, Set<TransitiveDependency> transitives) {
281-
Set<GroupArtifact> depExclusions = new HashSet<>(dep.getEffectiveExclusions());
291+
Set<GroupArtifact> depExclusions = declaredExclusions(dep);
282292
for (TransitiveDependency transitive : transitives) {
283293
ResolvedGroupArtifactVersion gav = transitive.getGav();
284294
if (dep.getGroupId().equals(gav.getGroupId()) &&

0 commit comments

Comments
 (0)