Skip to content

Treat dependencies with different scopes as distinct - #8468

Draft
martinfrancois wants to merge 1 commit into
openrewrite:mainfrom
martinfrancois:repro/remove-duplicate-dependencies-scope
Draft

Treat dependencies with different scopes as distinct#8468
martinfrancois wants to merge 1 commit into
openrewrite:mainfrom
martinfrancois:repro/remove-duplicate-dependencies-scope

Conversation

@martinfrancois

@martinfrancois martinfrancois commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Suggested review order: 51 of 52 (Score: 1)
Review first: openrewrite/rewrite-testing-frameworks#1086

What's changed?

Adds 1 known-failing test to RemoveDuplicateDependenciesTest that shows RemoveDuplicateDependencies does not detect two declarations of the same groupId:artifactId:version when they differ only in <scope>. No recipe code changes. The test is marked @Disabled so the suite stays green; removing the mark shows the failure. I used @Disabled instead of @ExpectedToFail because junit-pioneer is not on the rewrite-maven test classpath. The class had 13 tests before and has 14 with this one.

What's your motivation?

Recipe: org.openrewrite.maven.RemoveDuplicateDependencies.

Before

This pom declares junit twice, first without a scope (so compile) and then with <scope>test</scope>:

<dependencies>
  <dependency>
    <groupId>junit</groupId>
    <artifactId>junit</artifactId>
    <version>4.13.1</version>
  </dependency>
  <dependency>
    <groupId>junit</groupId>
    <artifactId>junit</artifactId>
    <version>4.13.1</version>
    <scope>test</scope>
  </dependency>
</dependencies>

Actual after the recipe

Using current main. the recipe makes no change, so both declarations stay. Without the @Disabled mark the run fails with:

RemoveDuplicateDependenciesTest > removeDuplicatedDependencyDifferingOnlyInScope() FAILED
    java.lang.AssertionError: Recipe was expected to make a change but made no changes.

Expected after the recipe

Only the later dependency declaration, with <scope>test</scope>, MUST remain.

Maven itself does not keep both. With Maven 3.9.16 the effective pom keeps one junit entry, it warns "must be unique... duplicate declaration", and the last declaration wins: swapping the two declarations flips which scope survives. Rewrite's own resolution model agrees. ResolvedPom.doResolveDependencies keys direct dependencies by group and artifact in a LinkedHashMap, with a source comment saying that for duplicated direct dependencies the last declaration wins. In a check against this pom, each scope resolution kept exactly one junit, and the test scope resolution carried the later test declaration. The recipe misses the pair because DependencyKey includes scope, so the first tag keys as compile and the second as test, two distinct keys. The failing test therefore expects only the later declaration, the one with <scope>test</scope>, to remain. After the recipe runs, the pom should match what Maven's effective model actually retains.

I found this while preparing #8446, which touches the same recipe and already discloses this limitation as pre-existing.

Anything in particular you'd like reviewers to focus on?

This is really a behavior question, not just a coverage gap. The existing test keepDependencyWithDifferentScope pins the opposite outcome for this exact shape, so a fix would have to change that expectation. I think removing the shadowed declaration is an improvement, because Maven only honors one of the two anyway, but keeping the current no-change behavior is a defensible choice too. Which behavior do you want? If you agree this should change, I would gladly prepare the fix. If the current behavior is intended, feel free to close this and I know it is settled.

Any additional context

Pre-existing tests changed: None.

This is not the classifier problem from #3832 and #4868, where removal was the bug. Here nothing wrong is removed; the question is whether a scope-shadowed duplicate should be. It is orthogonal to my open #8446, though the two will likely conflict textually in RemoveDuplicateDependenciesTest.java. This reproduction was prepared with AI assistance (Claude Code). I reviewed the tests and this description.

The added reproduction tests and the existing suite together cover changed and unchanged behavior. The known-failing tests remain disabled until implementation. The formatter run was calibrated per file; untouched lines were not reformatted.

Checklist

…g only in scope

Pins the coverage gap where two declarations of the same dependency
differing only in <scope> are left alone, although Maven's effective
model resolves them to a single dependency with the last declaration
winning (verified with Maven 3.9.16, which also warns "must be unique").

Note the existing keepDependencyWithDifferentScope test pins the
opposite outcome for this shape, so closing the gap means changing that
expectation; the pull request description presents both sides.

Marked as known-failing with @disabled since junit-pioneer is not on
the rewrite-maven test classpath.
@github-project-automation github-project-automation Bot moved this to In Progress in OpenRewrite Aug 11, 2026
@martinfrancois martinfrancois changed the title RemoveDuplicateDependencies: add failing test for duplicates differing only in scope Treat dependencies with different scopes as distinct Aug 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: In Progress

Development

Successfully merging this pull request may close these issues.

1 participant