Skip to content

Unspecified scope for managed dependency is all scopes - #8841

Open
BoykoAlex wants to merge 6 commits into
openrewrite:mainfrom
BoykoAlex:add-managed-dep-bug
Open

BoykoAlex wants to merge 6 commits into
openrewrite:mainfrom
BoykoAlex:add-managed-dep-bug

Conversation

@BoykoAlex

Copy link
Copy Markdown
Contributor

See the unit test.
AddManagedDependency with unspecified scope adds a dependency where initial version is null, (i.e. 0.0.0) hence if the parameter is the "latest.patch" one would end up with 0.0.X for example

@kdelay kdelay left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The non-null branch is untouched and has the same defect. Measured on 8.69.1, same POM as your test, varying only scope:

scope hits version
null (this PR) 1 1.3.2
test 1 1.3.2
provided, runtime 0 0.0.2
import 0 0.0.2

RESOLVE_SCOPES is {Compile, Runtime, Test, Provided}, so Scope.Import and Scope.System never get a map key and return empty for any POM. import is this option's example and what onlyAddedWhenUsing passes.

Passing null unconditionally would cover all four documented values.

@BoykoAlex
BoykoAlex requested a review from kdelay September 10, 2026 19:43

@kdelay kdelay left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Confirmed: findDependencies documents null as "search all scopes", so the lookup no longer depends on the tag value, and the added comment states the reason clearly.

One gap in the new test: @ValueSource covers test/provided/runtime plus null, but not import. That is the remaining entry in the option's valid list, it is the option's declared example, and it was one of the two values I measured at 0 hits / 0.0.2 before this change. It would need type=pom alongside it to stay valid Maven, but it is the case this fix most directly rescues.

@BoykoAlex

Copy link
Copy Markdown
Contributor Author

Should be good to go now

// The version of the dependency currently in use (if any) might influence the version comparator
// For example, "latest.patch" gives very different results depending on the version in use
String currentVersion = getResolutionResult().findDependencies(convertedGroup, convertedArtifact, Scope.fromName(scope)).stream()
// The version of the dependency currently in use (if any) might influence the version comparator.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We try hard to cut down what we not so lovingly call "doc essays" like this. They tend to overdocument and can easily get out of control.

@kdelay kdelay left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Two notes on the new fallback in existingManagedDependencyVersion() (line 302).

It reads getRequested().getDependencyManagement(), the unresolved model, so getVersion() can come back as ${some.version}. That value becomes currentVersion and is passed to findNewerVersion and versionComparator.compare. Line 266 already wraps the same method's result in pom.getValue(...), so the raw value looks like it needs that here too.

Neither new test has a <dependencyManagement> block in the before POM, so the fallback returns null in every parameterization. A BOM case would pin it.

@BoykoAlex

Copy link
Copy Markdown
Contributor Author

Two notes on the new fallback in existingManagedDependencyVersion() (line 302).

It reads getRequested().getDependencyManagement(), the unresolved model, so getVersion() can come back as ${some.version}. That value becomes currentVersion and is passed to findNewerVersion and versionComparator.compare. Line 266 already wraps the same method's result in pom.getValue(...), so the raw value looks like it needs that here too.

Neither new test has a <dependencyManagement> block in the before POM, so the fallback returns null in every parameterization. A BOM case would pin it.

It's fine that existingManagedDependencyVersion() returns ${some.version} because that call is wrapped in
pom.getValue(existingManagedDependencyVersion()) which resolves the version from a property. I've added support for the case when groupId/artifactId are specified as properties and a test for that however, I think this all going beyond the initial scope of the issue and I'd stop at this for now. Thanks for the reviews though @kdelay - they were really helpful!!!

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.

4 participants