gnodet-bot commented on code in PR #468:
URL:
https://github.com/apache/maven-install-plugin/pull/468#discussion_r4235590459
##########
src/main/java/org/apache/maven/plugins/install/InstallMojo.java:
##########
@@ -279,13 +280,18 @@ private ArtifactInstallerRequest processProject(Project
project) {
}
}
- return ArtifactInstallerRequest.build(session, installables);
+ return ArtifactInstallerRequest.build(session, toInstall);
}
private boolean isValidPath(Artifact a) {
return
getArtifactManager().getPath(a).filter(Files::isRegularFile).isPresent();
}
+ private static boolean usesModuleSourceHierarchy(Project project,
ProjectManager projectManager) {
+ return projectManager.getSourceRoots(project).stream()
+ .anyMatch(sr -> sr.module().isPresent());
Review Comment:
⚠️ **Missing `enabled()` filter** — `ProjectManager.getSourceRoots()`
explicitly documents that it returns "all source root directories, **including
the disabled ones**", and recommends `.filter(SourceRoot::enabled)` for
enabled-only access. As written, this method will also match disabled module
source roots and incorrectly suppress the main artifact.
```suggestion
private static boolean usesModuleSourceHierarchy(Project project,
ProjectManager projectManager) {
return projectManager.getSourceRoots(project).stream()
.filter(SourceRoot::enabled)
.anyMatch(sr -> sr.module().isPresent());
}
```
##########
src/test/java/org/apache/maven/plugins/install/InstallMojoTest.java:
##########
@@ -154,6 +156,34 @@ void basicInstallWithAttachedArtifacts(InstallMojo mojo)
throws Exception {
artifacts.stream().map(Artifact::key).toList());
}
+ @Test
+ @InjectMojo(goal = "install")
+ @MojoParameter(name = "installAtEnd", value = "false")
+ void moduleSourceHierarchySkipsMainArtifact(InstallMojo mojo) throws
Exception {
+ assertNotNull(mojo);
+ Project project = (Project) getVariableValueFromObject(mojo,
"project");
+
+ SourceRoot moduleRoot = mock(SourceRoot.class);
+
when(moduleRoot.module()).thenReturn(Optional.of("org.example.module"));
+
when(projectManager.getSourceRoots(project)).thenReturn(List.of(moduleRoot));
Review Comment:
💡 **Mock `enabled()` explicitly** — if the production code is updated to
filter by `enabled()`, this test will need it. Adding it now makes the intent
clear and keeps the test aligned with production behaviour:
```suggestion
SourceRoot moduleRoot = mock(SourceRoot.class);
when(moduleRoot.enabled()).thenReturn(true);
when(moduleRoot.module()).thenReturn(Optional.of("org.example.module"));
when(projectManager.getSourceRoots(project)).thenReturn(List.of(moduleRoot));
```
--
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]
