gnodet-bot commented on code in PR #13649:
URL: https://github.com/apache/maven/pull/13649#discussion_r4324488251
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultDependencyResolver.java:
##########
@@ -127,7 +129,8 @@ public DependencyResolverResult collect(@Nonnull
DependencyResolverRequest reque
Project project = request.getProject().get();
rootArtifact = project.getPomArtifact();
root = null;
- dependencies = project.getDependencies();
+ dependencies =
+ request.getDependencies().isEmpty() ?
project.getDependencies() : request.getDependencies();
Review Comment:
💡 **Intentional behavior change — needs a code comment**
This silently alters semantics: previously, when a `Project` was present,
`project.getDependencies()` was always used; now caller-supplied dependencies
take precedence. The intent is clear from context, but a comment would help
future maintainers:
```suggestion
dependencies = // caller-supplied deps take precedence over
project deps (allows plugin-specific resolution)
request.getDependencies().isEmpty() ?
project.getDependencies() : request.getDependencies();
```
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultDependencyResolver.java:
##########
@@ -182,6 +192,51 @@ public DependencyResolverResult collect(@Nonnull
DependencyResolverRequest reque
}
}
+ /**
+ * Applies dependency management to dependencies supplied outside the
project model. Resolver applies managed
+ * dependencies only below the root's direct dependencies, while project
model building normally performs this
+ * merge before resolution. Dependencies supplied by a plugin therefore
need the same merge here.
+ */
+ static List<org.eclipse.aether.graph.Dependency> applyDependencyManagement(
+ Collection<org.eclipse.aether.graph.Dependency> dependencies,
+ Collection<org.eclipse.aether.graph.Dependency>
managedDependencies) {
+ Map<String, org.eclipse.aether.graph.Dependency> managed = new
HashMap<>();
+ for (org.eclipse.aether.graph.Dependency dependency :
managedDependencies) {
+ managed.put(dependencyKey(dependency), dependency);
+ }
+
+ List<org.eclipse.aether.graph.Dependency> result = new
ArrayList<>(dependencies.size());
+ for (org.eclipse.aether.graph.Dependency dependency : dependencies) {
+ org.eclipse.aether.graph.Dependency management =
managed.get(dependencyKey(dependency));
+ if (management == null) {
+ result.add(dependency);
+ continue;
+ }
+
+ org.eclipse.aether.artifact.Artifact artifact =
dependency.getArtifact();
+ if (artifact.getVersion().isEmpty()) {
+ artifact =
artifact.setVersion(management.getArtifact().getVersion());
+ }
+ String scope = dependency.getScope();
+ if (scope == null || scope.isEmpty()) {
Review Comment:
💡 **Dead code — `scope == null` branch is unreachable**
Per the maven-resolver-api 2.0.x contract, `Dependency.getScope()` returns
an empty string if not set — never `null`. The `scope == null` check is
therefore dead code.
```suggestion
if (scope.isEmpty()) {
```
##########
api/maven-api-core/src/main/java/org/apache/maven/api/services/DependencyResolverRequest.java:
##########
@@ -155,6 +155,50 @@ static DependencyResolverRequest build(Session session,
RequestType requestType,
.build();
}
+ @Nonnull
+ static DependencyResolverRequest build(
+ Session session, RequestType requestType, Project project,
DependencyCoordinates dependency) {
+ return build(session, requestType, project, dependency,
PathScope.MAIN_RUNTIME);
Review Comment:
💡 **Default scope undocumented on no-scope `build()` overloads**
The no-scope overloads silently default to `PathScope.MAIN_RUNTIME`, but
neither the `build()` methods here nor the `DependencyResolver` convenience
methods document this. Plugin authors who need a different scope (e.g.
`MAIN_COMPILE`) will unknowingly receive runtime resolution unless they
discover the scope-taking overload independently.
Consider adding at minimum an end-of-line note, e.g.:
```suggestion
return build(session, requestType, project, dependency,
PathScope.MAIN_RUNTIME); // default; use scope-taking overload for other scopes
```
Or add a proper `@implNote Defaults to {@link PathScope#MAIN_RUNTIME}.`
Javadoc block above each no-scope overload.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
