search

mojohaus / license-maven-plugin

Maven plugin to download and collect license files from project dependencies.
https://www.mojohaus.org/license-maven-plugin/
GNU Lesser General Public License v3.0
106 stars 126 forks source link

NPE in DependenciesTool when running aggregateAddThirdParty with excludeTransitiveDependencies set to true #310

Open srdo opened 5 years ago

srdo commented 5 years ago

I'm getting an NPE in DependenciesTool.loadProjectDependencies.

The issue seems to be that loadProjectDependencies assumes that all artifacts have a non-null dependency trail.

The artifacts are loaded in loadProjectArtifacts, where reactor projects are excluded as we can't expect Maven to correctly resolve artifacts for projects that may not have been built yet. The method instead replaces reactor projects with their direct dependencies in order to still get the full dependency tree. It then resolves all dependencies except reactor projects. When the dependency lists are returned, the reactor artifacts are included but not fully populated.

I'm not sure what the best fix is here. Maybe loadProjectArtifacts shouldn't return reactor projects at all? I'm not sure why we would want to return reactor projects from that method.

Caused by: java.lang.NullPointerException
    at org.codehaus.mojo.license.api.DependenciesTool.loadProjectDependencies (DependenciesTool.java:243)
    at org.codehaus.mojo.license.api.DefaultThirdPartyHelper.loadDependencies (DefaultThirdPartyHelper.java:156)
ppalaga commented 5 years ago

An integration test in https://github.com/mojohaus/license-maven-plugin/tree/master/src/it reproducing the issue would be highly welcome.

srdo commented 5 years ago

I have the relevant bit of an integration test ready at https://github.com/srdo/license-maven-plugin/tree/issue-310/src/it/ISSUE-310. The test isn't done (the assertion and README are wrong), but it provokes the error.

I think the best way to fix this would be to sidestep the issue and get rid of the excludeTransitiveDependencies parameter. As far as I can tell, the idea is that if you have a project 

A -> B -> C

and you exclude B, the plugin should also exclude C.

The dependencyTrail seems to be a bad way to accomplish what that parameter is trying to do. https://issues.apache.org/jira/browse/MNG-4831 has an example project that is basically

A -> C
B -> C
D -> A & B

where the issue becomes what should the plugin do if you ask it to exclude A plus transitive deps? We might get a dependencyTrail that is D -> A -> C, in which case C will be excluded, but we might also get a trail D -> B -> C, in which case it isn't.

It would be better if we could filter artifacts out during dependency resolution. That way, if you exclude e.g. A, that part of the dependency tree simply won't be resolved. This would be equivalent to having excludeTransitiveDependencies always be true, but I'm not sure there's a use case for having it be false?

Tibor was talking about upgrading the project here https://github.com/mojohaus/license-maven-plugin/issues/219 and the newer artifact resolver API lets you set a filter on resolution requests https://maven.apache.org/resolver/apidocs/org/eclipse/aether/resolution/DependencyRequest.html. It might be worth looking at upgrading to using that resolver?

ppalaga commented 5 years ago

It would be better if we could filter artifacts out during dependency resolution.

I think that would be nice for performance reasons too.

excludeTransitiveDependencies always be true

In case there is no way to keep excludeTransitiveDependencies working in a backwards compatible way, we'd have to release that as a new major version (which I am not against).

Please go ahead and try your idea.

cc @Tibor17