Skip to content

Skip dependencies with no resolved artifact file instead of throwing NPE - #386

Open
kratos0718 wants to merge 1 commit into
apache:masterfrom
kratos0718:fix/npe-unresolved-dependency-property
Open

Skip dependencies with no resolved artifact file instead of throwing NPE#386
kratos0718 wants to merge 1 commit into
apache:masterfrom
kratos0718:fix/npe-unresolved-dependency-property

Conversation

@kratos0718

Copy link
Copy Markdown

Fixes #372.

Problem

copyProperties(MavenProject, Project) registers one path property per dependency:

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

copyProperties() registered one path property per dependency by calling
artifact.getFile().getPath() with no null check. An artifact's file is
null when it was never resolved -- under partial or offline resolution,
for instance -- so any such dependency aborted the whole mojo with a bare
NullPointerException that named nothing, leaving no hint which dependency
caused it.

Such dependencies are now skipped with a warning naming the artifact, so
the property is simply absent and a build that never references it still
runs. One unresolvable dependency also no longer prevents the remaining
ones from being registered.

The loop moves into a package-private setDependencyFileProperties() so it
can be tested: copyProperties() itself needs a MavenSession and a POM
file, and the project has no mocking framework, so testing it directly
would have meant adding one.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

copyProperties throws NPE on dependencies with no resolved artifact file

1 participant