diff --git a/src/main/java/org/openrewrite/java/dependencies/RemoveRedundantDependencies.java b/src/main/java/org/openrewrite/java/dependencies/RemoveRedundantDependencies.java index 2062339c..ef8f53ad 100644 --- a/src/main/java/org/openrewrite/java/dependencies/RemoveRedundantDependencies.java +++ b/src/main/java/org/openrewrite/java/dependencies/RemoveRedundantDependencies.java @@ -50,12 +50,20 @@ public class RemoveRedundantDependencies extends ScanningRecipe scope/configuration -> Set of transitive GAVs - Map>> transitivesByProjectAndScope; + // Map from project identifier -> scope/configuration -> Set of transitive dependencies + Map>> transitivesByProjectAndScope; + } + + @Value + public static class TransitiveDependency { + ResolvedGroupArtifactVersion gav; + Set exclusions; } @Override @@ -82,7 +90,7 @@ public TreeVisitor getScanner(Accumulator acc) { StringUtils.matchesGlob(dep.getGroupId(), groupId) && StringUtils.matchesGlob(dep.getArtifactId(), artifactId)) { // This is a matching parent dependency, resolve its transitives independently - Set transitives = acc.transitivesByProjectAndScope + Set transitives = acc.transitivesByProjectAndScope .computeIfAbsent(projectId, k -> new HashMap<>()) .computeIfAbsent(conf.getName(), k -> new HashSet<>()); resolveTransitivesFromPom( @@ -101,14 +109,18 @@ public TreeVisitor getScanner(Accumulator acc) { String projectId = maven.getPom().getGroupId() + ":" + maven.getPom().getArtifactId(); MavenPomDownloader downloader = new MavenPomDownloader(ctx); - for (Map.Entry> entry : maven.getDependencies().entrySet()) { - Scope depScope = entry.getKey(); - for (ResolvedDependency dep : entry.getValue()) { + // A direct dependency appears under every scope bucket it is visible in, so process + // each matching parent once, keyed by its own effective (declared) scope. + Set processed = new HashSet<>(); + for (List deps : maven.getDependencies().values()) { + for (ResolvedDependency dep : deps) { if (dep.isDirect() && StringUtils.matchesGlob(dep.getGroupId(), groupId) && - StringUtils.matchesGlob(dep.getArtifactId(), artifactId)) { + StringUtils.matchesGlob(dep.getArtifactId(), artifactId) && + processed.add(dep.getGroupId() + ":" + dep.getArtifactId())) { // This is a matching parent dependency, resolve its transitives independently - Set transitives = acc.transitivesByProjectAndScope + Scope depScope = Scope.fromName(dep.getRequested().getScope()); + Set transitives = acc.transitivesByProjectAndScope .computeIfAbsent(projectId, k -> new HashMap<>()) .computeIfAbsent(depScope.name().toLowerCase(), k -> new HashSet<>()); resolveTransitivesFromPom( @@ -132,7 +144,7 @@ private void resolveTransitivesFromPom( List repositories, MavenPomDownloader downloader, ExecutionContext ctx, - Set transitives) { + Set transitives) { try { // Ensure we have Maven Central in the repositories List effectiveRepos = new ArrayList<>(repositories); @@ -148,8 +160,9 @@ private void resolveTransitivesFromPom( List resolved = patchedPom.resolveDependencies(Scope.Compile, downloader, ctx); // Collect all dependencies (both direct and transitive of the parent) + Set visited = new HashSet<>(); for (ResolvedDependency dep : resolved) { - collectAllDependencies(dep, transitives); + collectAllDependencies(dep, transitives, visited); } } catch (MavenDownloadingException | MavenDownloadingExceptions e) { // If we can't download/resolve the POM, fall back to not detecting redundancies @@ -166,10 +179,12 @@ private ResolvedPom applyExclusions(ResolvedPom resolvedPom, List return patchedPom; } - private void collectAllDependencies(ResolvedDependency dep, Set transitives) { - if (transitives.add(dep.getGav())) { + private void collectAllDependencies(ResolvedDependency dep, Set transitives, + Set visited) { + if (visited.add(dep.getGav())) { + transitives.add(new TransitiveDependency(dep.getGav(), new HashSet<>(dep.getEffectiveExclusions()))); for (ResolvedDependency transitive : dep.getDependencies()) { - collectAllDependencies(transitive, transitives); + collectAllDependencies(transitive, transitives, visited); } } } @@ -185,70 +200,76 @@ public TreeVisitor getVisitor(Accumulator acc) { return tree; } - SourceFile sf = (SourceFile) tree; - Tree result = sf; - - // Handle Gradle - Optional gradleOpt = sf.getMarkers().findFirst(GradleProject.class); + Optional gradleOpt = tree.getMarkers().findFirst(GradleProject.class); if (gradleOpt.isPresent()) { GradleProject gradle = gradleOpt.get(); - String projectId = gradle.getGroup() + ":" + gradle.getName(); - Map> scopeToTransitives = - acc.transitivesByProjectAndScope.getOrDefault(projectId, emptyMap()); - - for (GradleDependencyConfiguration conf : gradle.getConfigurations()) { - Set transitives = getCompatibleTransitives( - scopeToTransitives, conf.getName(), true); - if (transitives.isEmpty()) { - continue; - } - - for (ResolvedDependency dep : conf.getResolved()) { - if (dep.isDirect() && - doesNotMatchArguments(dep) && - isInTransitives(dep, transitives)) { - // This direct dependency is transitively provided, remove it - // Don't specify configuration - Gradle's resolved config names differ from declaration names - result = new RemoveDependency( - dep.getGroupId(), dep.getArtifactId(), null, null, null) - .getVisitor().visit(result, ctx); - } - } - } - return result; + return handleGradle(ctx, gradle, tree); } - // Handle Maven - Optional mavenOpt = sf.getMarkers().findFirst(MavenResolutionResult.class); + Optional mavenOpt = tree.getMarkers().findFirst(MavenResolutionResult.class); if (mavenOpt.isPresent()) { MavenResolutionResult maven = mavenOpt.get(); - String projectId = maven.getPom().getGroupId() + ":" + maven.getPom().getArtifactId(); - Map> scopeToTransitives = - acc.transitivesByProjectAndScope.getOrDefault(projectId, emptyMap()); - - for (Map.Entry> entry : maven.getDependencies().entrySet()) { - String scope = entry.getKey().name().toLowerCase(); - Set transitives = getCompatibleTransitives( - scopeToTransitives, scope, false); - if (transitives.isEmpty()) { - continue; + return handleMaven(ctx, maven, tree); + } + + return tree; + } + + private @Nullable Tree handleGradle(ExecutionContext ctx, GradleProject gradle, Tree result) { + String projectId = gradle.getGroup() + ":" + gradle.getName(); + Map> scopeToTransitives = + acc.transitivesByProjectAndScope.getOrDefault(projectId, emptyMap()); + + for (GradleDependencyConfiguration conf : gradle.getConfigurations()) { + Set transitives = getCompatibleGradleTransitives( + scopeToTransitives, conf.getName()); + if (transitives.isEmpty()) { + continue; + } + + for (ResolvedDependency dep : conf.getResolved()) { + if (dep.isDirect() && + doesNotMatchArguments(dep) && + isRedundant(dep, transitives)) { + // This direct dependency is transitively provided, remove it + // Don't specify configuration - Gradle's resolved config names differ from declaration names + result = new RemoveDependency( + dep.getGroupId(), dep.getArtifactId(), null, null, null) + .getVisitor().visit(result, ctx); } + } + } + return result; + } - for (ResolvedDependency dep : entry.getValue()) { - if (dep.isDirect() && - doesNotMatchArguments(dep) && - isInTransitives(dep, transitives)) { - // This direct dependency is transitively provided, remove it + private @Nullable Tree handleMaven(ExecutionContext ctx, MavenResolutionResult maven, Tree result) { + String projectId = maven.getPom().getGroupId() + ":" + maven.getPom().getArtifactId(); + Map> scopeToTransitives = + acc.transitivesByProjectAndScope.getOrDefault(projectId, emptyMap()); + + // A direct dependency appears under every scope bucket it is visible in; evaluate each + // one once using its own effective scope so a wider transitive scope does not falsely + // mark a narrower direct declaration as redundant. + Set processed = new HashSet<>(); + for (List deps : maven.getDependencies().values()) { + for (ResolvedDependency dep : deps) { + if (dep.isDirect() && + doesNotMatchArguments(dep) && + processed.add(dep.getGroupId() + ":" + dep.getArtifactId())) { + Scope depScope = Scope.fromName(dep.getRequested().getScope()); + Set transitives = scopeToTransitives.getOrDefault( + depScope.name().toLowerCase(), emptySet()); + if (isRedundant(dep, transitives)) { + // This direct dependency is transitively provided at the same scope and + // with the same exclusions, remove it. result = new RemoveDependency( - dep.getGroupId(), dep.getArtifactId(), null, null, scope) + dep.getGroupId(), dep.getArtifactId(), null, null, depScope.name().toLowerCase()) .getVisitor().visit(result, ctx); } } } - return result; } - - return tree; + return result; } private boolean doesNotMatchArguments(ResolvedDependency dep) { @@ -256,41 +277,35 @@ private boolean doesNotMatchArguments(ResolvedDependency dep) { !StringUtils.matchesGlob(dep.getArtifactId(), artifactId); } - private boolean isInTransitives(ResolvedDependency dep, Set transitives) { - // Check if this dependency's GAV matches any transitive - // We match on groupId:artifactId:version exactly - for (ResolvedGroupArtifactVersion transitive : transitives) { - if (dep.getGroupId().equals(transitive.getGroupId()) && - dep.getArtifactId().equals(transitive.getArtifactId()) && - dep.getVersion().equals(transitive.getVersion())) { + private boolean isRedundant(ResolvedDependency dep, Set transitives) { + Set depExclusions = new HashSet<>(dep.getEffectiveExclusions()); + for (TransitiveDependency transitive : transitives) { + ResolvedGroupArtifactVersion gav = transitive.getGav(); + if (dep.getGroupId().equals(gav.getGroupId()) && + dep.getArtifactId().equals(gav.getArtifactId()) && + dep.getVersion().equals(gav.getVersion()) && + depExclusions.equals(transitive.getExclusions())) { return true; } } return false; } - /** - * Get transitives from this scope/configuration and any broader ones. - */ - private Set getCompatibleTransitives( - Map> scopeToTransitives, - String targetScope, - boolean isGradle) { + private Set getCompatibleGradleTransitives( + Map> scopeToTransitives, + String targetScope) { - Set result = new HashSet<>(); + Set result = new HashSet<>(); // Always include transitives from the same scope - Set sameScope = scopeToTransitives.get(targetScope); + Set sameScope = scopeToTransitives.get(targetScope); if (sameScope != null) { result.addAll(sameScope); } // Include transitives from broader scopes - List broaderScopes = isGradle ? - getBroaderGradleScopes(targetScope) : - getBroaderMavenScopes(targetScope); - for (String broader : broaderScopes) { - Set broaderTransitives = scopeToTransitives.get(broader); + for (String broader : getBroaderGradleScopes(targetScope)) { + Set broaderTransitives = scopeToTransitives.get(broader); if (broaderTransitives != null) { result.addAll(broaderTransitives); } @@ -313,19 +328,6 @@ private List getBroaderGradleScopes(String scope) { return emptyList(); } } - - private List getBroaderMavenScopes(String scope) { - switch (scope.toLowerCase()) { - case "runtime": - return singletonList("compile"); - case "provided": - return Arrays.asList("compile", "runtime"); - case "test": - return Arrays.asList("compile", "runtime", "provided"); - default: - return emptyList(); - } - } }; } } diff --git a/src/test/java/org/openrewrite/java/dependencies/RemoveRedundantDependenciesTest.java b/src/test/java/org/openrewrite/java/dependencies/RemoveRedundantDependenciesTest.java index ef8d0014..f163f7da 100644 --- a/src/test/java/org/openrewrite/java/dependencies/RemoveRedundantDependenciesTest.java +++ b/src/test/java/org/openrewrite/java/dependencies/RemoveRedundantDependenciesTest.java @@ -311,6 +311,190 @@ void noMatchingParentDependency() { ); } + @Test + void keepsTomcatEmbedCoreWhenExclusionsDiffer() { + 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-embed-programmatic + + + + + + """ + ) + ); + } + + @Test + void keepsTomcatEmbedCoreWhenDirectHasNoExclusions() { + 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 + + + + """ + ) + ); + } + + @Test + void keepsDirectCompileJunitJupiterWhenTransitiveIsTestScoped() { + 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.junit.jupiter + junit-jupiter + + + org.springframework.boot + spring-boot-starter-test + test + + + + """ + ) + ); + } + + @Test + void keepsRuntimeTomcatEmbedCoreWhenTransitiveIsCompileScoped() { + 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 + runtime + + + + """ + ) + ); + } + + @Test + void keepsDirectCompileTomcatEmbedCoreWhenProviderIsProvidedScoped() { + 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 + provided + + + org.apache.tomcat.embed + tomcat-embed-core + + + + """ + ) + ); + } + @Test void removeRedundantGradleDependency() { rewriteRun(