kratos0718 opened a new pull request, #386:
URL: https://github.com/apache/maven-antrun-plugin/pull/386
Fixes #372.
## Problem
`copyProperties(MavenProject, Project)` registers one path property per
dependency:
```java
for (Artifact artifact : depArtifacts) {
String propName = artifact.getDependencyConflictId();
antProject.setProperty(propertyPrefix + propName,
artifact.getFile().getPath());
}
```
`artifact.getFile()` is `null` when the artifact was never resolved —
partial or offline resolution, for example — so the mojo aborts with a bare
`NullPointerException` that names nothing:
```
java.lang.NullPointerException: Cannot invoke "java.io.File.getPath()"
because the return value of "org.apache.maven.artifact.Artifact.getFile()"
is null
```
Nothing in that tells the user which dependency is at fault.
## Fix
Skip such dependencies with a warning that names the artifact:
```
[WARNING] Not setting property "org.example:unresolved:jar": dependency
org.example:unresolved:jar:1.0 has no resolved artifact file.
```
The property is simply absent, so a build that never references it still
runs, and one unresolvable dependency no longer prevents the remaining ones
from being registered.
### Why skip rather than throw
The issue notes `getPathFromArtifacts` throws
`DependencyResolutionRequiredException` in the same situation, and consistency
with it would be reasonable. I did not do that here because **`copyProperties`
is public and does not declare `throws`** — adding a checked exception to it
would break any caller outside this plugin. Skipping fixes the crash without
touching the published signature.
If you would rather have the failure, I am happy to switch it: the natural
shape would be deprecating the current `copyProperties` and adding one declared
to throw, which is a larger change than this issue needs. Your call.
## Tests
The loop moved into a package-private
`setDependencyFileProperties(Set<Artifact>, Project)` so it can be tested at
all — `copyProperties` itself requires a `MavenSession` and a POM file, and
this project has no mocking framework, so testing it directly would have meant
adding one as a dependency. The extraction keeps the public API unchanged.
Three cases, using `DefaultArtifact` (whose `getFile()` is null unless set),
so no mocks are needed:
| test | before | after |
|---|---|---|
| `unresolvedDependencyIsSkippedInsteadOfThrowing` | ERROR — NPE | pass |
| `resolvedDependenciesStillGetTheirPathProperty` | FAIL — NPE aborts before
the resolved one is reached | pass |
| `nullArtifactSetIsTolerated` | pass | pass |
The second lists the unresolved artifact *first*, so it also proves one bad
dependency no longer blocks the rest.
```
before fix: Tests run: 3, Failures: 1, Errors: 1
after fix: Tests run: 3, Failures: 0, Errors: 0
full suite: Tests run: 7, Failures: 0, Errors: 0
```
--
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]
