gnodet-bot commented on code in PR #13628:
URL: https://github.com/apache/maven/pull/13628#discussion_r4314880633
##########
impl/maven-core/src/main/java/org/apache/maven/ReactorReader.java:
##########
@@ -575,14 +575,22 @@ private Map<String, Map<String, Map<String,
MavenProject>>> getAllProjects() {
private Map<String, Map<String, Map<String, MavenProject>>> getProjects() {
// compute the projects mapping
if (projects == null) {
- List<MavenProject> projects = session.getProjects();
- if (projects != null) {
+ List<MavenProject> sessionProjects = session.getProjects();
+ if (sessionProjects != null) {
Map<String, Map<String, Map<String, MavenProject>>> map = new
HashMap<>();
- projects.forEach(project ->
map.computeIfAbsent(project.getGroupId(), k -> new HashMap<>())
+ sessionProjects.forEach(project ->
map.computeIfAbsent(project.getGroupId(), k -> new HashMap<>())
.computeIfAbsent(project.getArtifactId(), k -> new
HashMap<>())
.put(project.getVersion(), project));
this.projects = map;
} else {
+ List<MavenProject> allProjects = session.getAllProjects();
+ if (allProjects != null) {
+ Map<String, Map<String, Map<String, MavenProject>>> map =
new HashMap<>();
+ allProjects.forEach(project ->
map.computeIfAbsent(project.getGroupId(), k -> new HashMap<>())
+ .computeIfAbsent(project.getArtifactId(), k -> new
HashMap<>())
+ .put(project.getVersion(), project));
+ return map;
Review Comment:
⚠️ **Bug: `allProjects` branch bypasses the cache**
The `allProjects` fallback branch builds a fresh map and immediately `return
map;` — it never assigns `this.projects = map`. Every subsequent call where
`session.getProjects()` is null will rebuild the map from scratch.
The fix is straightforward: assign before returning, mirroring the
`sessionProjects` branch:
```suggestion
this.projects = map;
return map;
```
Or alternatively fall through to `return projects;` after assigning
`this.projects = map`.
##########
impl/maven-core/src/main/java/org/apache/maven/project/ProjectSorter.java:
##########
@@ -113,6 +120,74 @@ public ProjectSorter(Collection<MavenProject> projects)
throws CycleDetectedExce
false);
}
+ if (project.getArtifacts() != null) {
Review Comment:
⚠️ **`project.getArtifacts()` returns empty set during graph construction**
`MavenProject.getArtifacts()` returns the artifact set populated by the
artifact resolution phase. At graph-building time (well before any resolution),
`resolvedArtifacts` is null and `artifactFilter` is null, so `getArtifacts()`
returns an empty `LinkedHashSet`. This null-check passes but the loop body
never executes — it is effectively dead code in the `DefaultGraphBuilder` flow.
It only activates in test scenarios that explicitly call `setArtifacts()`
(e.g. `ProjectSorterTest`). If the intent is to keep backward compatibility
with direct `ProjectSorter` callers, that's fine — but it should be documented
with a comment, or the dead-code nature should be acknowledged.
##########
impl/maven-core/src/main/java/org/apache/maven/graph/DefaultProjectDependencyGraph.java:
##########
@@ -89,8 +98,26 @@ public DefaultProjectDependencyGraph(List<MavenProject>
allProjects, Collection<
*/
public DefaultProjectDependencyGraph(Collection<MavenProject> allProjects,
Collection<MavenProject> projects)
throws CycleDetectedException, DuplicateProjectException {
+ this(allProjects, projects, Collections.emptyMap());
+ }
+
+ /**
+ * Creates a new project dependency graph based on the specified projects
and transitive dependencies.
+ *
+ * @param allProjects All collected projects.
+ * @param projects The projects to create the dependency
graph with.
+ * @param transitiveDependencies Transitive dependencies discovered for
reactor projects.
+ * @throws DuplicateProjectException
+ * @throws CycleDetectedException
+ * @since 4.0.0
+ */
+ public DefaultProjectDependencyGraph(
Review Comment:
💡 **Missing `@Deprecated` on backward-compat constructor**
The new 3-arg `(Collection, Collection, Map)` constructor is the primary
implementation; the existing 2-arg `(Collection, Collection)` now delegates to
it and exists only for backward compatibility. If you're marking
backward-compat constructors as deprecated (which is the standard Maven pattern
— e.g. the 0-arg no-op constructor already has `@Deprecated`), this constructor
should get the annotation too:
```suggestion
@Deprecated
public DefaultProjectDependencyGraph(
Collection<MavenProject> allProjects,
Collection<MavenProject> projects,
Map<MavenProject, ? extends Collection<?>>
transitiveDependencies)
throws CycleDetectedException, DuplicateProjectException {
```
_(Or clarify if the intent is that callers should use this 3-arg form
directly.)_
##########
impl/maven-core/src/main/java/org/apache/maven/graph/DefaultGraphBuilder.java:
##########
@@ -414,4 +445,138 @@ private void processPackagingAttribute(List<MavenProject>
projects, MavenExecuti
}
}
}
+
+ private Map<MavenProject, Set<Artifact>> getTransitiveReactorDependencies(
+ MavenSession session, List<MavenProject> projects) {
+ if (repoSystem == null
+ || session == null
+ || session.getRepositorySession() == null
+ || projects == null
+ || projects.isEmpty()) {
+ return Collections.emptyMap();
+ }
+
+ String sessionDataKey =
"DefaultGraphBuilder.transitiveReactorDependencies";
Review Comment:
💡 **String session-data key risks collision**
Using a bare `String` as a session-data key is fragile — any other component
using the same string would silently collide. The standard Maven/Aether pattern
is a `private static final Object` identity key:
```suggestion
Object sessionDataKey = SESSION_KEY;
```
with:
```java
private static final Object SESSION_KEY = new Object();
```
This makes the key unique by identity, not by name, and eliminates any
collision risk.
##########
impl/maven-core/src/main/java/org/apache/maven/graph/DefaultProjectDependencyGraph.java:
##########
@@ -78,6 +78,15 @@ public DefaultProjectDependencyGraph(List<MavenProject>
allProjects, Collection<
this((Collection<MavenProject>) allProjects, projects);
}
+ /**
+ * Creates a new project dependency graph based on the specified projects.
+ *
+ * @param allProjects All collected projects.
+ * @param projects The projects to create the dependency graph with.
+ * @throws DuplicateProjectException
+ * @throws CycleDetectedException
+ * @since 4.0.0
+ */
Review Comment:
⚠️ **Duplicate Javadoc block**
Lines 81–89 contain an orphaned Javadoc block — it was added by the diff but
the very same Javadoc already exists immediately after it (lines 104–113
describe the same 2-arg `(Collection, Collection)` constructor). This first
block needs to be removed; it currently leaves two consecutive Javadoc comments
with no code between them.
##########
impl/maven-core/src/main/java/org/apache/maven/graph/DefaultGraphBuilder.java:
##########
@@ -414,4 +445,138 @@ private void processPackagingAttribute(List<MavenProject>
projects, MavenExecuti
}
}
}
+
+ private Map<MavenProject, Set<Artifact>> getTransitiveReactorDependencies(
+ MavenSession session, List<MavenProject> projects) {
+ if (repoSystem == null
+ || session == null
+ || session.getRepositorySession() == null
+ || projects == null
+ || projects.isEmpty()) {
+ return Collections.emptyMap();
+ }
+
+ String sessionDataKey =
"DefaultGraphBuilder.transitiveReactorDependencies";
+ if (session.getRepositorySession().getData() != null) {
+ Object cached =
session.getRepositorySession().getData().get(sessionDataKey);
+ if (cached instanceof Map) {
+ @SuppressWarnings("unchecked")
+ Map<MavenProject, Set<Artifact>> cachedMap =
(Map<MavenProject, Set<Artifact>>) cached;
+ return cachedMap;
+ }
+ }
+
+ Set<String> reactorKeys = new HashSet<>(projects.size() * 2);
+ for (MavenProject p : projects) {
+ reactorKeys.add(ArtifactUtils.versionlessKey(p.getGroupId(),
p.getArtifactId()));
+ }
+
+ Map<MavenProject, Set<Artifact>> result = new HashMap<>();
+ ArtifactTypeRegistry stereotypes =
session.getRepositorySession().getArtifactTypeRegistry();
+
+ for (MavenProject project : projects) {
+ List<org.apache.maven.model.Dependency> dependencies =
project.getDependencies();
+ if (dependencies == null || dependencies.isEmpty()) {
+ continue;
+ }
+
+ boolean hasExternal = false;
+ for (org.apache.maven.model.Dependency dep : dependencies) {
+ if (dep.getGroupId() != null && dep.getArtifactId() != null) {
+ String key =
ArtifactUtils.versionlessKey(dep.getGroupId(), dep.getArtifactId());
+ if (!reactorKeys.contains(key)) {
+ hasExternal = true;
+ break;
+ }
+ }
+ }
+
+ if (!hasExternal) {
+ continue;
+ }
+
+ CollectRequest collectRequest = new CollectRequest();
+ collectRequest.setRequestContext("project");
+
collectRequest.setRepositories(project.getRemoteProjectRepositories());
+ if (project.getArtifact() != null &&
project.getArtifact().getGroupId() != null) {
+
collectRequest.setRootArtifact(RepositoryUtils.toArtifact(project.getArtifact()));
+ }
+
+ for (org.apache.maven.model.Dependency dependency : dependencies) {
+ if (dependency.getGroupId() == null
+ || dependency.getGroupId().isEmpty()
+ || dependency.getArtifactId() == null
+ || dependency.getArtifactId().isEmpty()
+ || dependency.getVersion() == null
+ || dependency.getVersion().isEmpty()) {
+ continue;
+ }
+
collectRequest.addDependency(RepositoryUtils.toDependency(dependency,
stereotypes));
Review Comment:
⚠️ **CollectRequest includes reactor-internal dependencies unnecessarily**
The loop at L505–515 adds _all_ project dependencies to the
`CollectRequest`, including direct reactor-internal ones.
`collectDependencies()` will attempt to fetch metadata from remote repos for
those reactor-internal artifacts (which aren't installed anywhere). Since the
whole point of this method is to find reactor dependencies hiding _behind_
external transitive deps, reactor-internal direct deps can be skipped entirely
— we only care about external deps as roots.
Consider filtering direct reactor deps out before adding to the request:
```java
String depKey = ArtifactUtils.versionlessKey(dependency.getGroupId(),
dependency.getArtifactId());
if (reactorKeys.contains(depKey)) {
continue; // skip direct reactor deps — we're looking for transitive ones
}
collectRequest.addDependency(RepositoryUtils.toDependency(dependency,
stereotypes));
```
This reduces unnecessary remote calls and makes the intent explicit.
##########
impl/maven-core/src/test/java/org/apache/maven/graph/DefaultGraphBuilderTest.java:
##########
@@ -372,6 +376,73 @@ void testProcessPackagingAttribute() throws
ProjectBuildingException {
assertEquals("pom", actualReactorProjects.get(1).getPackaging());
}
+ @Test
+ void testTransitiveReactorDependencyFromExternalDependency() throws
Exception {
+ RepositorySystem repoSystem = mock(RepositorySystem.class);
+ graphBuilder = new DefaultGraphBuilder(
+ mock(BuildResumptionDataRepository.class),
+ pomlessCollectionStrategy,
+ multiModuleCollectionStrategy,
+ requestPomCollectionStrategy,
+ repoSystem);
+
+ MavenProject projectA = getMavenProject("module-a");
+ MavenProject projectB = getMavenProject("module-b");
+
+ Dependency externalDep = new Dependency();
+ externalDep.setGroupId("external.group");
+ externalDep.setArtifactId("external-artifact");
+ externalDep.setVersion("1.0");
+ projectA.getModel().getDependencies().add(externalDep);
+
+ when(session.getRequest()).thenReturn(mavenExecutionRequest);
+ when(session.getProjects()).thenReturn(null);
+ RepositorySystemSession repoSession =
mock(RepositorySystemSession.class);
+ when(session.getRepositorySession()).thenReturn(repoSession);
+ when(repoSession.getArtifactTypeRegistry())
+
.thenReturn(mock(org.eclipse.aether.artifact.ArtifactTypeRegistry.class));
+
when(repoSession.getData()).thenReturn(mock(org.eclipse.aether.SessionData.class));
+
+
when(mavenExecutionRequest.getProjectBuildingRequest()).thenReturn(mock(ProjectBuildingRequest.class));
+ List<ProjectBuildingResult> projectBuildingResults =
+ createProjectBuildingResultMocks(List.of(projectA, projectB));
+ when(projectBuilder.build(anyList(), anyBoolean(),
any(ProjectBuildingRequest.class)))
+ .thenReturn(projectBuildingResults);
+
+ ProjectActivation projectActivation = new ProjectActivation();
+
when(mavenExecutionRequest.getProjectActivation()).thenReturn(projectActivation);
+ when(mavenExecutionRequest.getPom()).thenReturn(null);
Review Comment:
⚠️ **Test will NPE: `pomlessCollectionStrategy` is not a mock**
`pomlessCollectionStrategy` is declared as `new
PomlessCollectionStrategy(projectBuilder)` (a real object, not a mock) at line
109 of the existing test class. When `getPom()` returns `null`,
`DefaultGraphBuilder` routes to
`pomlessCollectionStrategy.collectProjects(request)`, which calls the **2-arg**
`projectBuilder.build(ModelSource, ProjectBuildingRequest)` overload.
The test only stubs the **3-arg** overload:
```java
when(projectBuilder.build(anyList(), anyBoolean(),
any(ProjectBuildingRequest.class)))
.thenReturn(projectBuildingResults);
```
Mockito returns `null` for the unstubbed 2-arg call, so `.getProject()` on
the result throws a NullPointerException.
Fix: either stub the 2-arg overload, or mock `pomlessCollectionStrategy` for
this test and stub `collectProjects()` to return the desired project list
directly.
--
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]
