Copilot commented on code in PR #307:
URL:
https://github.com/apache/maven-remote-resources-plugin/pull/307#discussion_r4282009534
##########
src/main/java/org/apache/maven/plugin/resources/remote/AbstractProcessRemoteResourcesMojo.java:
##########
@@ -536,45 +536,75 @@ protected List<MavenProject> getProjects() {
artifact.setVersion(artifact.getBaseVersion());
}
- getLog().debug("Building project for " + artifact);
- MavenProject p;
- try {
- ProjectBuildingRequest req = new
DefaultProjectBuildingRequest()
-
.setValidationLevel(ModelBuildingRequest.VALIDATION_LEVEL_MINIMAL)
- .setProcessPlugins(false)
-
.setRepositorySession(mavenSession.getRepositorySession())
-
.setSystemProperties(mavenSession.getSystemProperties())
- .setUserProperties(mavenSession.getUserProperties())
- .setLocalRepository(mavenSession.getLocalRepository())
-
.setRemoteRepositories(project.getRemoteArtifactRepositories());
- ProjectBuildingResult res = projectBuilder.build(artifact,
req);
- p = res.getProject();
- } catch (ProjectBuildingException e) {
- getLog().warn("Invalid project model for artifact [" +
artifact.getGroupId() + ":"
- + artifact.getArtifactId() + ":" +
artifact.getVersion() + "]. "
- + "It will be ignored by the remote resources Mojo.");
- continue;
+ MavenProject reactorProject = findReactorProject(artifact,
mavenSession);
+ if (reactorProject == null) {
+
+ getLog().debug("Building project for " + artifact);
+ try {
+ ProjectBuildingRequest projectBuildingRequest = new
DefaultProjectBuildingRequest()
+
.setValidationLevel(ModelBuildingRequest.VALIDATION_LEVEL_MINIMAL)
+ .setProcessPlugins(false)
+
.setRepositorySession(mavenSession.getRepositorySession())
+
.setSystemProperties(mavenSession.getSystemProperties())
+
.setUserProperties(mavenSession.getUserProperties())
+
.setLocalRepository(mavenSession.getLocalRepository())
+
.setRemoteRepositories(project.getRemoteArtifactRepositories());
+ ProjectBuildingResult buildingResult =
projectBuilder.build(artifact, projectBuildingRequest);
+ reactorProject = buildingResult.getProject();
+ } catch (ProjectBuildingException e) {
Review Comment:
The local variable `reactorProject` is used to hold either a matched reactor
project or a newly built external project (assigned from
`buildingResult.getProject()`). This makes the control flow harder to read and
is misleading. Consider renaming it to something neutral like
`resolvedProject`/`matchedOrBuiltProject`, or use two variables (e.g.,
`matchedReactorProject` and `builtProject`) to keep intent clear.
##########
src/main/java/org/apache/maven/plugin/resources/remote/AbstractProcessRemoteResourcesMojo.java:
##########
@@ -536,45 +536,75 @@ protected List<MavenProject> getProjects() {
artifact.setVersion(artifact.getBaseVersion());
}
- getLog().debug("Building project for " + artifact);
- MavenProject p;
- try {
- ProjectBuildingRequest req = new
DefaultProjectBuildingRequest()
-
.setValidationLevel(ModelBuildingRequest.VALIDATION_LEVEL_MINIMAL)
- .setProcessPlugins(false)
-
.setRepositorySession(mavenSession.getRepositorySession())
-
.setSystemProperties(mavenSession.getSystemProperties())
- .setUserProperties(mavenSession.getUserProperties())
- .setLocalRepository(mavenSession.getLocalRepository())
-
.setRemoteRepositories(project.getRemoteArtifactRepositories());
- ProjectBuildingResult res = projectBuilder.build(artifact,
req);
- p = res.getProject();
- } catch (ProjectBuildingException e) {
- getLog().warn("Invalid project model for artifact [" +
artifact.getGroupId() + ":"
- + artifact.getArtifactId() + ":" +
artifact.getVersion() + "]. "
- + "It will be ignored by the remote resources Mojo.");
- continue;
+ MavenProject reactorProject = findReactorProject(artifact,
mavenSession);
+ if (reactorProject == null) {
+
+ getLog().debug("Building project for " + artifact);
+ try {
+ ProjectBuildingRequest projectBuildingRequest = new
DefaultProjectBuildingRequest()
+
.setValidationLevel(ModelBuildingRequest.VALIDATION_LEVEL_MINIMAL)
+ .setProcessPlugins(false)
+
.setRepositorySession(mavenSession.getRepositorySession())
+
.setSystemProperties(mavenSession.getSystemProperties())
+
.setUserProperties(mavenSession.getUserProperties())
+
.setLocalRepository(mavenSession.getLocalRepository())
+
.setRemoteRepositories(project.getRemoteArtifactRepositories());
+ ProjectBuildingResult buildingResult =
projectBuilder.build(artifact, projectBuildingRequest);
+ reactorProject = buildingResult.getProject();
+ } catch (ProjectBuildingException e) {
+ getLog().warn("Invalid project model for artifact [" +
artifact.getGroupId() + ":"
+ + artifact.getArtifactId() + ":" +
artifact.getVersion() + "]. "
+ + "It will be ignored by the remote resources
Mojo.");
+ continue;
+ }
}
String supplementKey = generateSupplementMapKey(
- p.getModel().getGroupId(), p.getModel().getArtifactId());
+ reactorProject.getModel().getGroupId(),
+ reactorProject.getModel().getArtifactId());
if (supplementModels.containsKey(supplementKey)) {
- Model mergedModel = mergeModels(p.getModel(),
supplementModels.get(supplementKey));
+ Model mergedModel = mergeModels(reactorProject.getModel(),
supplementModels.get(supplementKey));
MavenProject mergedProject = new MavenProject(mergedModel);
projects.add(mergedProject);
mergedProject.setArtifact(artifact);
mergedProject.setVersion(artifact.getVersion());
getLog().debug("Adding project with groupId [" +
mergedProject.getGroupId() + "] (supplemented)");
} else {
- projects.add(p);
- getLog().debug("Adding project with groupId [" +
p.getGroupId() + "]");
+ projects.add(reactorProject);
+ getLog().debug("Adding project with groupId [" +
reactorProject.getGroupId() + "]");
}
}
projects.sort(new ProjectComparator());
return projects;
}
+ private static MavenProject findReactorProject(Artifact artifact,
MavenSession session) {
+ if (session == null) {
+ return null;
+ }
+ return findReactorProject(artifact, session.getProjects());
+ }
+
+ static MavenProject findReactorProject(Artifact artifact,
List<MavenProject> reactorProjects) {
+ for (MavenProject reactorProject : reactorProjects) {
+ if (!artifact.getGroupId().equals(reactorProject.getGroupId())
+ ||
!artifact.getArtifactId().equals(reactorProject.getArtifactId())
+ || !artifactVersionMatches(artifact, reactorProject)) {
+ continue;
+ }
+ return reactorProject;
+ }
+ return null;
+ }
+
+ private static boolean artifactVersionMatches(Artifact artifact,
MavenProject reactorProject) {
+ String projectVersion = reactorProject.getVersion();
+ return artifact.getVersion().equals(projectVersion)
+ || (artifact.getBaseVersion() != null
+ && artifact.getBaseVersion().equals(projectVersion));
+ }
Review Comment:
`findReactorProject(Artifact, MavenSession)` forwards directly to
`session.getProjects()` without guarding against a null projects list, and
`artifactVersionMatches()` unconditionally calls
`artifact.getVersion().equals(...)` which will throw if `getVersion()` is null.
Even if current call sites always set these, these helpers are generic and
package-visible (for tests), so they should be null-safe (e.g., return null
when `reactorProjects` is null; compare with
`projectVersion.equals(artifact.getVersion())` after null checks, or use
`Objects.equals`).
##########
src/test/java/org/apache/maven/plugin/resources/remote/RemoteResourcesMojoTest.java:
##########
@@ -89,6 +96,114 @@ public void testNoBundles() throws Exception {
mojo.execute();
}
+ public void testFindReactorProjectMatchesTimestampedSnapshot() throws
Exception {
+ MavenProject reactorProject = new MavenProject(new
org.apache.maven.model.Model());
+ reactorProject.setGroupId("com.example");
+ reactorProject.setArtifactId("reactor-project");
+ reactorProject.setVersion("1.0-SNAPSHOT");
+
+ Artifact artifact = new DefaultArtifact(
+ "com.example",
+ "reactor-project",
+ VersionRange.createFromVersion("1.0-20260926.120000-1"),
+ null,
+ "jar",
+ "",
+ new DefaultArtifactHandler());
+
+ assertSame(
+ reactorProject,
+ AbstractProcessRemoteResourcesMojo.findReactorProject(
+ artifact, Collections.singletonList(reactorProject)));
+
+ Artifact externalArtifact = new DefaultArtifact(
+ "com.example",
+ "external-project",
+ VersionRange.createFromVersion("1.0"),
+ null,
+ "jar",
+ "",
+ new DefaultArtifactHandler());
+ assertNull(AbstractProcessRemoteResourcesMojo.findReactorProject(
+ externalArtifact, Collections.singletonList(reactorProject)));
+ }
+
+ public void testGetProjectsBuildsOnlyExternalArtifacts() throws Exception {
+ MavenProjectResourcesStub project =
createTestProject("reactor-projects");
+ ProcessRemoteResourcesMojo mojo =
lookupProcessMojoWithDefaultSettings(project);
+ setupDefaultProject(project);
+
+ MavenProject reactorProject = new MavenProject(new
org.apache.maven.model.Model());
+ reactorProject.setGroupId("com.example");
+ reactorProject.setArtifactId("reactor-project");
+ reactorProject.setVersion("1.0-SNAPSHOT");
+
+ Artifact reactorArtifact = new DefaultArtifact(
+ "com.example",
+ "reactor-project",
+ VersionRange.createFromVersion("1.0-20260926.120000-1"),
+ "compile",
+ "jar",
+ "",
+ new DefaultArtifactHandler());
+ Artifact externalArtifact = new DefaultArtifact(
+ "com.example",
+ "external-project",
+ VersionRange.createFromVersion("1.0"),
+ "compile",
+ "jar",
+ "",
+ new DefaultArtifactHandler());
+ project.setArtifacts(new
LinkedHashSet<>(Arrays.asList(reactorArtifact, externalArtifact)));
+ reactorProject.setArtifact(reactorArtifact);
+
+ MavenSession session = (MavenSession) getVariableValueFromObject(mojo,
"mavenSession");
+ session.setProjects(Arrays.asList(project, reactorProject));
+
+ MavenProject externalProject = new MavenProject(new
org.apache.maven.model.Model());
+ externalProject.setGroupId("com.example");
+ externalProject.setArtifactId("external-project");
+ externalProject.setVersion("1.0");
+ externalProject.setArtifact(externalArtifact);
+
+ ProjectBuildingResult buildingResult = (ProjectBuildingResult)
Proxy.newProxyInstance(
+ ProjectBuildingResult.class.getClassLoader(),
+ new Class<?>[] {ProjectBuildingResult.class},
+ new InvocationHandler() {
+ @Override
+ public Object invoke(Object proxy, Method method, Object[]
args) {
+ if ("getProject".equals(method.getName())) {
+ return externalProject;
+ }
+ return null;
+ }
+ });
+ List<Artifact> builtArtifacts = new ArrayList<>();
+ ProjectBuilder projectBuilder = (ProjectBuilder)
Proxy.newProxyInstance(
+ ProjectBuilder.class.getClassLoader(), new Class<?>[]
{ProjectBuilder.class}, new InvocationHandler() {
+ @Override
+ public Object invoke(Object proxy, Method method, Object[]
args) {
+ if ("build".equals(method.getName())
+ && args != null
+ && args.length == 2
+ && args[0] instanceof Artifact) {
+ builtArtifacts.add((Artifact) args[0]);
+ return buildingResult;
+ }
+ return null;
+ }
+ });
Review Comment:
These proxies return null for all non-matching methods. If the production
code (or future refactors) call any other
`ProjectBuildingResult`/`ProjectBuilder` methods, this test will fail with
unclear NPEs and be harder to diagnose. Consider handling `Object` methods
(`toString`, `hashCode`, `equals`) and throwing `UnsupportedOperationException`
for unexpected interface methods to make failures explicit and easier to debug.
##########
src/test/java/org/apache/maven/plugin/resources/remote/RemoteResourcesMojoTest.java:
##########
@@ -89,6 +96,114 @@ public void testNoBundles() throws Exception {
mojo.execute();
}
+ public void testFindReactorProjectMatchesTimestampedSnapshot() throws
Exception {
+ MavenProject reactorProject = new MavenProject(new
org.apache.maven.model.Model());
+ reactorProject.setGroupId("com.example");
+ reactorProject.setArtifactId("reactor-project");
+ reactorProject.setVersion("1.0-SNAPSHOT");
+
+ Artifact artifact = new DefaultArtifact(
+ "com.example",
+ "reactor-project",
+ VersionRange.createFromVersion("1.0-20260926.120000-1"),
+ null,
+ "jar",
+ "",
+ new DefaultArtifactHandler());
+
Review Comment:
This test relies on Maven artifact internals to interpret a timestamped
snapshot via `VersionRange.createFromVersion(...)`. To reduce brittleness and
more accurately model resolved snapshot behavior, consider explicitly
configuring the artifact version/baseVersion used by the matching logic (e.g.,
ensure `getVersion()` is the timestamped value while `getBaseVersion()` is
`1.0-SNAPSHOT`, or otherwise set them explicitly) so the test intent doesn’t
depend on `DefaultArtifact` implementation details.
--
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]
