Skip to content

Keep direct dependencies whose scope or exclusions differ from the transitive one - #188

Merged
timtebeek merged 4 commits into
mainfrom
fix-redundant-dependencies-scope-exclusions
Jul 10, 2026
Merged

Keep direct dependencies whose scope or exclusions differ from the transitive one#188
timtebeek merged 4 commits into
mainfrom
fix-redundant-dependencies-scope-exclusions

Conversation

@timtebeek

@timtebeek timtebeek commented Jul 10, 2026

Copy link
Copy Markdown
Member

Problem

RemoveRedundantDependencies removed a direct dependency whenever a matched parent provided the same coordinate transitively, without checking scope or exclusions. This silently changed the effective classpath / exclusion contract whenever the direct declaration carried information the transitive entry did not.

The issue reports five scenarios (all against public Spring Boot starters) where the direct declaration must be kept but was incorrectly removed:

  1. Direct dep has different exclusions than the transitive one.
  2. Direct dep has no exclusions but the transitive one does.
  3. Direct dep is compile while the transitive is test (direct is wider).
  4. Direct dep is pinned to runtime while the transitive is compile (direct is narrower — nearest-wins means the direct declaration actually overrides the transitive scope).
  5. The providing starter is provided, so its transitives are provided-only, but the direct dep is compile (wider).

Fix

A direct dependency is now only removed when the transitive one provides it at the exact same effective scope and with the same exclusions, so removal never changes the effective classpath:

  • Each transitive dependency now tracks its effective exclusions; a direct dependency is redundant only when its exclusions match exactly.
  • Maven transitives are keyed by the parent dependency's own effective scope (derived from getRequested().getScope(), deduped across the scope buckets a direct dependency appears in) and compared against the direct dependency's effective scope. The previous "broader scope covers narrower" logic is dropped for Maven, since a direct declaration at a different scope is not redundant.

Tests

Added the five scenarios from the issue as passing tests. All existing tests continue to pass (13 total).

…ansitive one

RemoveRedundantDependencies removed a direct dependency whenever a matched
parent provided the same coordinate transitively, ignoring scope and
exclusions. This silently changed the effective classpath when the direct
declaration carried information the transitive entry did not.

A direct dependency is now only removed when the transitive one provides it
at the exact same effective scope and with the same exclusions:

- Track each transitive dependency's effective exclusions and only treat a
  direct dependency as redundant when its exclusions match exactly.
- Key Maven transitives by the parent dependency's own effective scope and
  compare against the direct dependency's effective scope, rather than
  treating a broader transitive scope as covering a narrower direct one.

Fixes the five scenarios from openrewrite/rewrite#8235.
Comment on lines -289 to +311
List<String> broaderScopes = isGradle ?
getBroaderGradleScopes(targetScope) :
getBroaderMavenScopes(targetScope);
for (String broader : broaderScopes) {
Set<ResolvedGroupArtifactVersion> broaderTransitives = scopeToTransitives.get(broader);
for (String broader : getBroaderGradleScopes(targetScope)) {
Set<TransitiveDependency> broaderTransitives = scopeToTransitives.get(broader);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did we end up dropping the differing scopes check for Maven and now only use the broader gradle scopes? I may have missed something here.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question — the short version is that Maven still checks scope, it just does it differently, so the widening helper is no longer needed there.

Maven (handleMaven): each direct dependency is now evaluated against transitives collected under its own declared scope, matched exactly:

Scope depScope = Scope.fromName(dep.getRequested().getScope());
Set<TransitiveDependency> transitives = scopeToTransitives.getOrDefault(depScope.name().toLowerCase(), emptySet());
if (isRedundant(dep, transitives)) { ... }

The scan phase keys the parent's transitives by the parent's own declared scope as well, so it's an apples-to-apples comparison. getBroaderMavenScopes only existed because the old code iterated maven.getDependencies() resolution buckets — where a compile dep also shows up in the runtime/test buckets — and keyed transitives by bucket; that mismatch is what needed reconciling. Keying by declared scope removes the need.

This is also a deliberate behavior change: we no longer treat a broader-scoped transitive as covering a narrower-scoped direct declaration, since that changes the effective classpath (e.g. dropping a direct runtime dep because a compile transitive provides it). It's locked in by keepsRuntimeTomcatEmbedCoreWhenTransitiveIsCompileScoped and keepsDirectCompileTomcatEmbedCoreWhenProviderIsProvidedScoped.

Gradle (handleGradle): still uses getCompatibleTransitives/getBroaderGradleScopes because there transitives are keyed by configuration name, and Gradle configurations genuinely inherit (testImplementationimplementationapi). A dependency in one configuration legitimately sees transitives from the configurations it extends, so we still walk those. Maven's flat declared-scope model has no such inheritance, so exact-scope matching is sufficient.

Happy to add a code comment making the Maven-vs-Gradle asymmetry explicit if that would help.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe just a method rename to getCompatibleGradleTransitives or something like that. I guess I missed that it was no longer called by the handleMaven that was extracted.

@github-project-automation github-project-automation Bot moved this from In Progress to Ready to Review in OpenRewrite Jul 10, 2026
@timtebeek
timtebeek merged commit 0371a6c into main Jul 10, 2026
1 check passed
@timtebeek
timtebeek deleted the fix-redundant-dependencies-scope-exclusions branch July 10, 2026 23:13
@github-project-automation github-project-automation Bot moved this from Ready to Review to Done in OpenRewrite Jul 10, 2026
sambsnyd pushed a commit that referenced this pull request Jul 14, 2026
…ansitive one (#188)

* Keep direct dependencies whose scope or exclusions differ from the transitive one

RemoveRedundantDependencies removed a direct dependency whenever a matched
parent provided the same coordinate transitively, ignoring scope and
exclusions. This silently changed the effective classpath when the direct
declaration carried information the transitive entry did not.

A direct dependency is now only removed when the transitive one provides it
at the exact same effective scope and with the same exclusions:

- Track each transitive dependency's effective exclusions and only treat a
  direct dependency as redundant when its exclusions match exactly.
- Key Maven transitives by the parent dependency's own effective scope and
  compare against the direct dependency's effective scope, rather than
  treating a broader transitive scope as covering a narrower direct one.

Fixes the five scenarios from openrewrite/rewrite#8235.

* Simplify test POMs: drop XML declaration and project attributes

* Extract handleGradle/handleMaven and trim comments

* Rename getCompatibleTransitives to getCompatibleGradleTransitives
sambsnyd added a commit that referenced this pull request Jul 14, 2026
… dependencies being reported on (#189)

* Include in Dependency Report the path to the build file declaring the dependencies being reported on

* OpenRewrite recipe best practices

Use this link to re-run the recipe: https://app.moderne.io/recipes/org.openrewrite.recipes.rewrite.OpenRewriteRecipeBestPractices?organizationId=QUxML01vZGVybmUvTW9kZXJuZSArIE9wZW5SZXdyaXRl

Co-authored-by: Moderne <team@moderne.io>

* Pin empty MavenSettings in DependencyResolutionDiagnosticTest.maven() (#178)

openrewrite/gh-automation#95 installs ~/.m2/settings.xml with `<mirrorOf>*</mirrorOf>` to route Maven through `artifactory.moderne.ninja` and avoid HTTP 429 from Maven Central. The mirror rewrites every repository request — including the test pom's deliberately-unreachable `<repository><url>https://nonexistent.moderne.io/maven2</url></repository>` — so the diagnostic recipe only sees the mirror and the local file cache. Neither Maven Central nor the test repo appear in the data table, and both assertions fail.

Inject an explicit empty `MavenSettings` via `MavenExecutionContextView` (same pattern as `mavenSettingsWithMirrors`) so the recipe ignores `~/.m2/settings.xml` and sees the pom-declared repositories directly. The original three assertions are restored unchanged.

* Speed up ModuleHasDependency and RepositoryHasDependency by removing DependencyInsight (#176)

* Speed up ModuleHasDependency and RepositoryHasDependency by removing DependencyInsight

Both recipes ran a fresh DependencyInsight visitor against every source file during
scanning, which performs exhaustive Gradle + Maven dependency analysis just to answer
a binary yes/no question about whether the module contains a given GAV.

Replace the visitor with a direct check against MavenResolutionResult.findDependencies
for Maven modules, and against GradleProject's configurations + ResolvedDependency.findDependency
for Gradle modules. This mirrors the pattern Sam applied to the single-build-system versions
of ModuleHasDependency in openrewrite/rewrite (commit 919c9f5 / PR #6664).

These recipes are used as preconditions in many declarative migration recipes.
Avoiding the DependencyInsight allocation + traversal per source file substantially
reduces scanner time on repositories with many Java files.

* Skip dependency scan in ModuleHasDependency once project is known to match

Avoids re-running the per-tree dependency lookup for every source file in a
module after the JavaProject has already been added to the accumulator.

---------

Co-authored-by: Tim te Beek <tim@moderne.io>

* Match requested dependencies in ModuleHasDependency and RepositoryHasDependency (#179)

* Regenerate recipes.csv (#180)

* Update Gradle wrapper 9.5.1

Use this link to re-run the recipe: https://app.moderne.io/recipes/org.openrewrite.gradle.UpdateGradleWrapper?organizationId=QUxML01vZGVybmUgKyBPcGVuUmV3cml0ZS9PcGVuUmV3cml0ZQ%3D%3D#defaults=W3sidmFsdWUiOiI5LjUuMSIsIm5hbWUiOiJ2ZXJzaW9uIn0seyJ2YWx1ZSI6ImJpbiIsIm5hbWUiOiJkaXN0cmlidXRpb24ifSx7InZhbHVlIjoiYmFmYzE0MWI2MTlhZDYzNTBmZDk3NWZjOTAzMTU2ZGQ1YzE1MTk5OGNjOGIwNThlOGMxMDQ0YWI1ZjdiMDMxZiIsIm5hbWUiOiJkaXN0cmlidXRpb25DaGVja3N1bSJ9XQ==

Co-authored-by: Moderne <team@moderne.io>

* Trust resolved version over declared dep in dependency search (#181)

* Trust resolved version over declared dep in dependency search

PR #179 extended ModuleHasDependency / RepositoryHasDependency to also
match against requested/declared dependencies so they still match when
resolution fails. This introduced false positives whenever a version
range is supplied: the declared version string could satisfy the
comparator even when the resolved version did not (e.g. a Gradle
resolutionStrategy.force or platform alignment overriding the declared
coordinate).

- When a coordinate is present in the resolved dependencies, trust the
  resolved check and skip the declared-dependency fallback for that
  group:artifact. Only fall back for declared deps not already resolved.
- Tighten versionMatches so a null declared version no longer
  auto-matches when a version constraint is supplied (same treatment as
  a ${...} property reference).

Adds regression tests covering both rules in ModuleHasDependencyTest and
RepositoryHasDependencyTest.

* Apply suggestions from code review

Co-authored-by: Jente Sondervorst <jentesondervorst@gmail.com>

* Add Maven regression tests for BOM-managed dependency version range

---------

Co-authored-by: Jente Sondervorst <jentesondervorst@gmail.com>

* git-ignore .context/

Use this link to re-run the recipe: https://app.moderne.io/recipes/org.openrewrite.AddToGitignore?organizationId=QUxML01vZGVybmUgKyBPcGVuUmV3cml0ZQ%3D%3D#defaults=W3sidmFsdWUiOiIuY29udGV4dC8iLCJuYW1lIjoiZW50cmllcyJ9XQ==

Co-authored-by: Moderne <team@moderne.io>

* Fix RemoveDependency.unlessUsing for multi-module Maven projects (#183)

* Fix `RemoveDependency.unlessUsing` for multi-module Maven projects

`RemoveDependency` scans every source file and records whether `unlessUsing`
matched against the file's `JavaProject` marker. In a multi-module Maven
project the parent pom carries its own `JavaProject` distinct from the
children, so the parent module's accumulator entry never reflects type
usages found in child modules. The visitor would then strip the
`<dependency>` from the parent pom even though child modules in the same
reactor still relied on inheriting it.

Now the scanner additionally records the `MavenResolutionResult.id ->
JavaProject` mapping for every pom it visits. When the visitor is about to
remove a `<dependency>` from a pom, it walks that pom's descendant modules
via `MavenResolutionResult.getModules()` and preserves the declaration if
any descendant's `JavaProject` shows the type in use.

* Allow `RemoveDependency` to remove ancestor decl when descendants self-declare

The descendant-aware preservation introduced in the previous commit was
intentionally conservative: any descendant module whose Java sources used
the `unlessUsing` type blocked removal of the dep from an ancestor pom.
That over-preserves when a descendant module also declares the same
dependency directly in its own `<dependencies>` — in that case the
descendant is self-sufficient and the ancestor's declaration is dead
weight.

Refine the check: a descendant only blocks removal if it uses the type AND
does not declare the dependency itself. The self-declaration check looks
at the raw `requested` pom (as-written), not the resolved/inherited
dependency list, so a child that inherits the dep from the ancestor we
might modify still correctly blocks removal.

* Remove out-of-date OWASP suppressions

Use this link to re-run the recipe: https://app.moderne.io/recipes/org.openrewrite.xml.security.RemoveOwaspSuppressions?organizationId=QUxML01vZGVybmUgKyBPcGVuUmV3cml0ZQ%3D%3D#defaults=W3sidmFsdWUiOiIyMDI2LTA2LTE3IiwibmFtZSI6ImN1dE9mZkRhdGUifV0=

Co-authored-by: Moderne <team@moderne.io>

* Use GitHub actions/checkout@v7

Use this link to re-run the recipe: https://app.moderne.io/recipes/org.openrewrite.github.ChangeActionVersion?organizationId=QUxML01vZGVybmUgKyBPcGVuUmV3cml0ZQ%3D%3D#defaults=W3sidmFsdWUiOiJhY3Rpb25zL2NoZWNrb3V0IiwibmFtZSI6ImFjdGlvbiJ9LHsidmFsdWUiOiJ2NyIsIm5hbWUiOiJ2ZXJzaW9uIn1d

Co-authored-by: Moderne <team@moderne.io>

* Cache delegate recipes per recipe instance to avoid repeated construction (#184)

The Gradle/Maven dependency wrapper recipes (`AddDependency`, `ChangeDependency`, `UpgradeDependencyVersion`, `UpgradeTransitiveDependencyVersion`) each delegate to a Gradle and a Maven `ScanningRecipe`. Previously a fresh delegate instance was constructed on every `getInitialValue`/`getScanner`/`getVisitor`/`validate` call (and, in `AddDependency`, once per source file inside the scanner's `visit`).

Each `ScanningRecipe` construction runs `UUID.randomUUID()` for its accumulator-message key, which draws from `SecureRandom` and dominated profiles of recipe-heavy runs.

Memoize each delegate lazily on the wrapper recipe instance (a `final transient AtomicReference` that is excluded from the generated constructor and from `equals`/`hashCode`) so it is built at most once per wrapper instance and reused across the scanning and editing phases. The delegate is derived from the wrapper's options, so it belongs to the recipe instance rather than the accumulator: callers such as `rewrite-spring` deliberately reuse a single accumulator across two wrapper instances that have different options, so caching the delegate on the accumulator would reuse the wrong options.

The `Accumulator` types are left unchanged, preserving their public constructors for downstream callers that build them directly.

* Add software.amazon.ion:ion-java -> com.amazon.ion:ion-java relocation (stopgap) (#185)

* [Auto] Old GroupId migrations as of 2026-06-23T1723

* Update findRelocatedIonJava test for new relocation context (#186)

The [Auto] Old GroupId migrations update (5253cf5) added a context note
to the software.amazon.ion:ion-java relocation in migrations.csv. The
RelocatedDependencyCheck recipe now surfaces that context both in the
RelocatedDependencyReport data table row and in the inline search-result
marker comment, so update the test expectations to match.

Fixes the scheduled CI failure in run 28122079598.

* Keep direct dependencies whose scope or exclusions differ from the transitive one (#188)

* Keep direct dependencies whose scope or exclusions differ from the transitive one

RemoveRedundantDependencies removed a direct dependency whenever a matched
parent provided the same coordinate transitively, ignoring scope and
exclusions. This silently changed the effective classpath when the direct
declaration carried information the transitive entry did not.

A direct dependency is now only removed when the transitive one provides it
at the exact same effective scope and with the same exclusions:

- Track each transitive dependency's effective exclusions and only treat a
  direct dependency as redundant when its exclusions match exactly.
- Key Maven transitives by the parent dependency's own effective scope and
  compare against the direct dependency's effective scope, rather than
  treating a broader transitive scope as covering a narrower direct one.

Fixes the five scenarios from openrewrite/rewrite#8235.

* Simplify test POMs: drop XML declaration and project attributes

* Extract handleGradle/handleMaven and trim comments

* Rename getCompatibleTransitives to getCompatibleGradleTransitives

* Polish

---------

Co-authored-by: Tim te Beek <timtebeek@gmail.com>
Co-authored-by: Moderne <team@moderne.io>
Co-authored-by: Tim te Beek <tim@moderne.io>
Co-authored-by: Steve Elliott <steve@moderne.io>
Co-authored-by: Jente Sondervorst <jentesondervorst@gmail.com>
Co-authored-by: Greg Oledzki <greg.oledzki@moderne.io>
Co-authored-by: Knut Wannheden <knut@moderne.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

RemoveDuplicateDependencies - exclusions and scopes

2 participants