Copilot commented on code in PR #1655:
URL:
https://github.com/apache/maven-dependency-plugin/pull/1655#discussion_r3537963612
##########
src/main/java/org/apache/maven/plugins/dependency/PropertiesMojo.java:
##########
@@ -130,10 +133,12 @@ public void execute() throws MojoExecutionException {
resolverUtil.createArtifactFromParams(paramArtifact);
artifact = resolverUtil.resolveArtifact(artifact,
project.getRemoteProjectRepositories());
- this.project
- .getProperties()
- .setProperty(
- toConflictId(artifact),
artifact.getFile().getAbsolutePath());
+ File file = artifact.getFile();
+ if (file == null) {
+ throw new MojoExecutionException("Could not resolve
extra artifact " + toConflictId(artifact)
+ + " to a file; the dependency:properties goal
requires a resolved artifact.");
+ }
+
this.project.getProperties().setProperty(toConflictId(artifact),
file.getAbsolutePath());
Review Comment:
Same as above: `artifact.getFile() == null` doesn’t necessarily indicate a
resolution failure, just that there is no file to set as a property. Reword the
message to reflect “no associated file” and compute the conflict id once so the
message and property key stay consistent.
##########
src/test/java/org/apache/maven/plugins/dependency/TestPropertiesMojo.java:
##########
@@ -102,4 +104,15 @@ void testSetPropertiesForExtractArtifacts(PropertiesMojo
mojo) throws Exception
assertTrue(project.getProperties().containsKey(depId2));
assertTrue(new
File(project.getProperties().getProperty(depId1)).exists());
}
+
+ @Test
+ @InjectMojo(goal = "properties")
+ void testNullFileThrowsException(PropertiesMojo mojo) {
+ Artifact artifact = Mockito.mock(Artifact.class);
+ when(artifact.getDependencyConflictId()).thenReturn("group:artifact");
+ when(artifact.getFile()).thenReturn(null);
+ when(this.project.getArtifacts()).thenReturn(new
HashSet<>(Arrays.asList(artifact)));
+
+ assertThrows(MojoExecutionException.class, mojo::execute);
Review Comment:
The test currently only asserts the exception type. Since this PR’s intent
is to provide a clear diagnostic, it would be useful to also assert the message
contains the artifact id (or another stable substring) to prevent regressions
back to an unhelpful message.
##########
src/main/java/org/apache/maven/plugins/dependency/PropertiesMojo.java:
##########
@@ -112,10 +113,12 @@ public void execute() throws MojoExecutionException {
Set<Artifact> artifacts = project.getArtifacts();
for (Artifact artifact : artifacts) {
- project.getProperties()
- .setProperty(
- artifact.getDependencyConflictId(),
- artifact.getFile().getAbsolutePath());
+ File file = artifact.getFile();
+ if (file == null) {
+ throw new MojoExecutionException("Could not resolve artifact "
+ artifact.getDependencyConflictId()
+ + " to a file; the dependency:properties goal requires
a resolved artifact.");
+ }
+
project.getProperties().setProperty(artifact.getDependencyConflictId(),
file.getAbsolutePath());
Review Comment:
The error message is a bit misleading: `artifact.getFile() == null` can mean
the artifact is resolved but has no associated file (e.g., POM packaging), not
necessarily that resolution failed. Consider wording the message in terms of
“no associated file”, and reuse the conflict id to avoid calling
`getDependencyConflictId()` twice.
--
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]
