Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import java.util.Map;
import java.util.Objects;
import java.util.concurrent.ConcurrentHashMap;
import java.util.stream.Collectors;

import org.apache.maven.execution.ProjectDependencyGraph;
import org.apache.maven.project.MavenProject;
Expand Down Expand Up @@ -123,7 +124,24 @@ private List<MavenProject> applyFilter(
filtered.addAll(upstream ? getUpstreamProjects(project, false) : getDownstreamProjects(project, false));
}
}
return filtered;
if (filtered.isEmpty() || filtered.size() == 1) {
// Optimization to skip streaming, distincting, and collecting to a new list when there is zero or one
// project, aka there can't be duplicates.
return filtered;
}

// Distinct the projects to avoid duplicates. Duplicates are possible in multi-module projects.
//
// Given a scenario where there is an aggregate POM with modules A, B, C, D, and E and project E depends on
// A, B, C, and D. If the aggregate POM is being filtered for non-transitive and downstream dependencies where
// only A, C, and E are whitelisted duplicates will occur. When scanning projects A, C, and E, those will be
// added to 'filtered' as they are whitelisted. When scanning B and D, they are not whitelisted, and since
// transitive is false, their downstream dependencies will be added to 'filtered'. E is a downstream dependency
// of A, B, C, and D, so when scanning B and D, E will be added again 'filtered'.
//
// Without de-duplication, the final list would contain E three times, once for E being in the projects and
// whitelisted, and twice more for E being a downstream dependency of B and D.
return filtered.stream().distinct().collect(Collectors.toList());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added and used import of Collectors.toList() here instead of Stream.toList() as they have mutability differences.

Stream.toList() returns an immutable list, akin to Collections.unmodifiableList().

Stream.collect(Collectors.toList()) returns a mutable list, akin to ArrayList.addAll() or Arrays.asList().

I'm uncertain whether mutability or immutability is preferred here. So, I went with a straight copy, but if immutability is preferred I'll change this to Stream.toList().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI @cstamas

Copy link
Member

Choose a reason for hiding this comment

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

if Stream.toList() can not be used, I would like to add such info also to code.

@alzimmermsft Did you try with immutable list here by executing ITs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't, I just copied what I had in the other PR.

Will change to Stream.toList() and test locally. If it passes, I'll use it to match the other stream collections in the file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@slawekjaranowski switched to Stream.toList() as the ITs executed without issue.

}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,13 @@ class DefaultProjectDependencyGraphTest {

private final MavenProject cProject = createProject(Arrays.asList(toDependency(bProject)), "cProject");

private final MavenProject dProject = createProject(
Arrays.asList(toDependency(aProject), toDependency(bProject), toDependency(cProject)), "dProject");

private final MavenProject eProject = createProject(
Arrays.asList(toDependency(aProject), toDependency(bProject), toDependency(cProject), toDependency(dProject)),
"eProject");

private final MavenProject depender1 = createProject(Arrays.asList(toDependency(aProject)), "depender1");

private final MavenProject depender2 = createProject(Arrays.asList(toDependency(aProject)), "depender2");
Expand All @@ -64,6 +71,37 @@ void testNonTransitiveFiltering() throws DuplicateProjectException, CycleDetecte
assertTrue(graph.getDownstreamProjects(aProject, false).contains(cProject));
}

// Test verifying that getDownstreamProjects does not contain duplicates.
// This is a regression test for https://github.com/apache/maven/issues/2487.
//
// The graph is:
// aProject -> bProject
// | -> dProject
// | -> eProject
// bProject -> cProject
// | -> dProject
// | -> eProject
// cProject -> dProject
// | -> eProject
// dProject -> eProject
//
// When getting the non-transitive, downstream projects of aProject with a whitelist of aProject, dProject,
// and eProject, we expect to get dProject, and eProject with no duplicates.
// Before the fix, this would return dProject and eProject twice, once from bProject and once from cProject. As
// aProject is whitelisted, it should not be returned as a downstream project for itself. bProject and cProject
// are not whitelisted, so they should return their downstream projects, both have dProject and eProject as
// downstream projects. Which would result in dProject and eProject being returned twice, but now the results are
// made unique.
public void testGetDownstreamDoesNotDuplicateProjects() throws CycleDetectedException, DuplicateProjectException {
ProjectDependencyGraph graph = new DefaultProjectDependencyGraph(
Arrays.asList(aProject, bProject, cProject, dProject, eProject));
graph = new FilteredProjectDependencyGraph(graph, Arrays.asList(aProject, dProject, eProject));
final List<MavenProject> downstreamProjects = graph.getDownstreamProjects(aProject, false);
assertEquals(2, downstreamProjects.size());
assertTrue(downstreamProjects.contains(dProject));
assertTrue(downstreamProjects.contains(eProject));
}

@Test
void testGetSortedProjects() throws DuplicateProjectException, CycleDetectedException {
ProjectDependencyGraph graph = new DefaultProjectDependencyGraph(Arrays.asList(depender1, aProject));
Expand Down
2 changes: 1 addition & 1 deletion pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ under the License.
<distributionShortName>Maven</distributionShortName>
<distributionName>Apache Maven</distributionName>
<maven.site.path>ref/4-LATEST</maven.site.path>
<project.build.outputTimestamp>2025-03-05T09:43:59Z</project.build.outputTimestamp>
<project.build.outputTimestamp>2025-06-18T10:29:55Z</project.build.outputTimestamp>
<!-- various versions -->
<assertjVersion>3.27.3</assertjVersion>
<asmVersion>9.8</asmVersion>
Expand Down
Loading