Fix issue in DefaultDependenciesTool.loadProjectArtifacts causing the…

… plugin to use the repository list of the parent project, rather than the repos declared by the child in some cases

src/it/ISSUE-145-2/README.txt

@@ -1,3 +1,3 @@
-Demonstrates that the aggregateAddThirdParty goal can handle long sibling project dependency chains in the same reactor.
+Demonstrates that the aggregateAddThirdParty goal can handle long sibling project dependency chains in the same reactor, and that it resolves dependencies based on the right remote repositories list.
 
-The project has parent A and children B, C, D, E with B depending on C, C dependending on D and D depending on E. The E dependency has a certain license. When the plugin generates the license list, it should pick up the license for E.
+The project has parent A and children B, C, D, E with B depending on C, C dependending on D and D depending on E. The D dependency has a custom repository specified, and a dependency from that repo. When the plugin generates the license list, it should pick up the license for the dependency from the custom repo. E has a custom license specified, which should also be picked up.

src/it/ISSUE-145-2/invoker.properties

@@ -19,5 +19,7 @@
 # <http://www.gnu.org/licenses/lgpl-3.0.html>.
 # #L%
 ###
-invoker.goals=clean package
-invoker.failureBehavior=fail-fast
+invoker.goals=clean package -e
+invoker.failureBehavior=fail-fast
+#Disable the default settings.xml for integration tests. This test needs to always resolve dependencies from remote repositories, not from the local.
+invoker.profiles=!it-repo

src/it/ISSUE-145-2/postbuild.groovy

@@ -40,6 +40,7 @@ file = new File(basedir, 'target/generated-sources/license/THIRD-PARTY.txt');
 assertExistsFile(file);
 
 content = file.text;
+assert assertContains(file, content, 'Eclipse Public License 1.0');
 assert assertContains(file, content, 'Custom ISSUE-145 license');
 
 return true;

src/it/ISSUE-145-2/submodule3/pom.xml

@@ -11,7 +11,20 @@
 
     <artifactId>test-ISSUE-145-2-submodule3</artifactId>
 
+    <repositories>
+        <repository>
+            <id>clojars.org</id>
+            <url>https://repo.clojars.org</url>
+        </repository>
+    </repositories>
+
     <dependencies>
+        <dependency>
+            <!-- This is not available on Central, which is essential to the test -->
+            <groupId>org.clojure-android</groupId>
+            <artifactId>tools.logging</artifactId>
+            <version>0.3.2</version>
+        </dependency>
         <dependency>
             <groupId>org.codehaus.mojo.license.test</groupId>
             <artifactId>test-ISSUE-145-2-submodule4</artifactId>

src/main/java/org/codehaus/mojo/license/AddThirdPartyMojo.java

@@ -101,22 +101,18 @@ public class AddThirdPartyMojo extends AbstractAddThirdPartyMojo implements Mave
     private boolean isAggregatorBuild = false;
 
     /**
-     * Map from reactor project G/A to direct dependencies. When loading dependencies, the aggregator goal replaces all
-     * reactor projects with their direct dependencies, to avoid trying to load artifacts for projects that haven't been
-     * built/published yet. This field is used to keep track of which dependencies are also projects in the reactor.
+     * The reactor projects. When resolving dependencies, the aggregator goal needs to do custom handling of sibling dependencies for
+     * projects in the reactor, to avoid trying to load artifacts for projects that haven't been built/published yet. 
      */
-    private Map<String, List<Dependency>> reactorProjectDependencies;
+    private List<MavenProject> reactorProjectDependencies;
 
     /**
      * Copies of the project's dependency sets. AddThirdParty needs to load dependencies only for the single project it
      * is run for, while AggregateAddThirdParty needs to load dependencies for the parent project, as well as all child
      * projects in the reactor.
      *
-     * The aggregator goal replaces all reactor projects with their direct dependencies, to avoid trying to load
-     * artifacts for projects that haven't been built/published yet. This is necessary in cases where one child project
-     * A in a reactor depends on another project B in the same reactor. Since B is not necessarily built/published, the
-     * plugin needs to replace B with its dependencies when processing A. This field stores that modified view of the
-     * project's dependencies.
+     * In cases where one child project A in a reactor depends on another project B in the same reactor, B is not necessarily
+     * built/published. The plugin needs to resolve B's dependencies manually. This field stores the result of that manual resolution.
      */
     private ResolvedProjectDependencies dependencyArtifacts;
 
@@ -253,7 +249,7 @@ protected ResolvedProjectDependencies resolveDependencyArtifacts() throws Depend
     protected SortedProperties createUnsafeMapping()
       throws ProjectBuildingException, IOException, ThirdPartyToolException, MojoExecutionException, DependenciesToolException
     {
-
+        
         SortedSet<MavenProject> unsafeDependencies = getUnsafeDependencies();
 
         SortedProperties unsafeMappings =
@@ -472,9 +468,7 @@ private void writeMissingFile()
         }
     }
 
-    void initFromMojo( AggregatorAddThirdPartyMojo mojo, MavenProject mavenProject,
-            Map<String, List<Dependency>> reactorProjects ) throws Exception
-    {
+    void initFromMojo(AggregatorAddThirdPartyMojo mojo, MavenProject mavenProject, List<MavenProject> reactorProjects) throws Exception {
         project = mavenProject;
         deployMissingFile = mojo.deployMissingFile;
         useRepositoryMissingFiles = mojo.useRepositoryMissingFiles;
@@ -497,7 +491,7 @@ void initFromMojo( AggregatorAddThirdPartyMojo mojo, MavenProject mavenProject,
                 mojo.overrideFile.getAbsolutePath().substring( absolutePath.length() ) );
         missingLicensesFileArtifact = mojo.missingLicensesFileArtifact;
         localRepository = mojo.localRepository;
-        remoteRepositories = mojo.remoteRepositories;
+        remoteRepositories = mavenProject.getRemoteArtifactRepositories();
         dependencies = new HashSet<>(mavenProject.getDependencies());
         licenseMerges = mojo.licenseMerges;
         licenseMergesFile = mojo.licenseMergesFile;
@@ -519,7 +513,7 @@ void initFromMojo( AggregatorAddThirdPartyMojo mojo, MavenProject mavenProject,
 
         isAggregatorBuild = true;
         reactorProjectDependencies = reactorProjects;
-
+        
         init();
 
         consolidate();

src/main/java/org/codehaus/mojo/license/AggregatorAddThirdPartyMojo.java

@@ -208,11 +208,6 @@ protected void doAction()
 
         String addThirdPartyRoleHint = groupId + ":" + artifactId + ":" + version + ":" + "add-third-party";
 
-        Map<String, List<Dependency>> reactorProjectDependencies = new TreeMap<>();
-        for (MavenProject reactorProject : this.reactorProjects) {
-            reactorProjectDependencies.put( String.format( "%s:%s", reactorProject.getGroupId(), reactorProject.getArtifactId() ), reactorProject.getDependencies() );
-        }
-
         for (MavenProject reactorProject : reactorProjects) {
             if (getProject().equals(reactorProject)) {
                 // do not process pom
@@ -222,7 +217,7 @@ protected void doAction()
             AddThirdPartyMojo mojo = (AddThirdPartyMojo) getSession()
                     .lookup( AddThirdPartyMojo.ROLE, addThirdPartyRoleHint );
 
-            mojo.initFromMojo(this, reactorProject, reactorProjectDependencies);
+            mojo.initFromMojo(this, reactorProject, new ArrayList<>(this.reactorProjects));
 
             LicenseMap childLicenseMap = mojo.getLicenseMap();
             if ( isVerbose() )

src/main/java/org/codehaus/mojo/license/api/DefaultDependenciesTool.java

@@ -26,9 +26,9 @@
 import java.util.ArrayList;
 import java.util.HashMap;
 import java.util.HashSet;
-import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Map;
+import java.util.Map.Entry;
 import java.util.Queue;
 import java.util.Set;
 import java.util.SortedMap;
@@ -38,6 +38,7 @@
 import java.util.regex.PatternSyntaxException;
 import org.apache.commons.collections.CollectionUtils;
 import org.apache.commons.lang3.StringUtils;
+import org.apache.commons.lang3.tuple.Pair;
 import org.apache.maven.artifact.Artifact;
 import org.apache.maven.artifact.factory.ArtifactFactory;
 import org.apache.maven.artifact.metadata.ArtifactMetadataSource;
@@ -317,68 +318,96 @@ public SortedMap<String, MavenProject> loadProjectDependencies( ResolvedProjectD
      * {@inheritDoc}
      */
     public ResolvedProjectDependencies loadProjectArtifacts( ArtifactRepository localRepository, List remoteRepositories,
-                                      MavenProject project, Map<String,List<Dependency>> reactorProjectDependencies )
+                                      MavenProject project, List<MavenProject> reactorProjects )
         throws DependenciesToolException
 
     {
+        Map<String, MavenProject> idToReactorProject = new HashMap<>();
+        if ( reactorProjects != null ) {
+            for ( MavenProject reactorProject : reactorProjects ) {
+                idToReactorProject.put(String.format("%s:%s", reactorProject.getGroupId(), reactorProject.getArtifactId()), reactorProject);
+            }
+        }
+
+        /*
+         * Find the list of dependencies to resolve transitively. Some projects may be in the reactor.
+         * Reactor projects can't be resolved by the artifact resolver yet.
+         * In order to still get the complete dependency tree for the project, we will add the transitive
+         * dependencies of the reactor project to the list of dependencies to resolve. Since the transitive dependencies could
+         * also be reactor projects, we need to repeat this check for each of those. Note that since the dependency reactor
+         * project may specify its own list of repositories, we need to keep track of which project the transitive dependency is
+         * declared in.
+         */
         List<Dependency> directDependencies = new ArrayList<>( project.getDependencies() );
-        List<Dependency> transitiveDependencies = new ArrayList<>();
-        Queue<Dependency> dependenciesToCheck = new ArrayDeque<>( project.getDependencies() );
-            if ( reactorProjectDependencies != null )
-            {
+        Map<MavenProject, List<Dependency>> reactorProjectToTransitiveDependencies = new HashMap<>();
+        Queue<Pair<MavenProject, Dependency>> dependenciesToCheck = new ArrayDeque<>();
+        for (Dependency dependency : directDependencies ) {
+            dependenciesToCheck.add( Pair.of( project, dependency ) );
+        }
+        if ( reactorProjects != null )
+        {
             while ( !dependenciesToCheck.isEmpty() )
+            {
+                Pair<MavenProject, Dependency> pair = dependenciesToCheck.remove();
+                Dependency dependency = pair.getRight();
+                String id = String.format( "%s:%s", dependency.getGroupId(), dependency.getArtifactId() );
+                MavenProject dependencyReactorProject = idToReactorProject.get( id );
+                if ( dependencyReactorProject != null )
                 {
-                Dependency dependency = dependenciesToCheck.remove();
-                    String id = String.format( "%s:%s", dependency.getGroupId(), dependency.getArtifactId() );
-                    List<Dependency> projectDependencies = reactorProjectDependencies.get( id );
-                    if ( projectDependencies != null )
-                    {
                     /*
-                     * Since the project is in the reactor, the artifact resolver may not be able to resolve the artifact plus transitive dependencies yet.
-                     * In order to still get the complete dependency tree for the project,
-                     * we will add the transitive dependencies of the reactor project to the list of dependencies to resolve.
-                     * Since the transitive dependencies could also be reactor projects, we need to repeat this check for each of those.
+                     * Since the project is in the reactor, the artifact resolver may not be able to resolve the artifact plus transitive
+                     * dependencies yet. In order to still get the complete dependency tree for the project, we will add the transitive
+                     * dependencies of the reactor project to the list of dependencies to resolve. Since the transitive dependencies could
+                     * also be reactor projects, we need to repeat this check for each of those. Note that since the dependency reactor
+                     * project may specify its own list of repositories, we need to keep track of which project the transitive dependency is
+                     * declared in.
                      */
-                    dependenciesToCheck.addAll( projectDependencies );
+                    for (Dependency transitiveDependency : (List<Dependency>)dependencyReactorProject.getDependencies() ) {
+                        dependenciesToCheck.add( Pair.of( dependencyReactorProject, transitiveDependency ));
                     }
+                }
                 if ( !directDependencies.contains( dependency ) )
                 {
-                    transitiveDependencies.add( dependency );
+                    List<Dependency> transitiveForSameProject = reactorProjectToTransitiveDependencies.get( pair.getLeft() );
+                    if ( transitiveForSameProject == null ) {
+                        transitiveForSameProject = new ArrayList<>();
+                        reactorProjectToTransitiveDependencies.put( pair.getLeft(), transitiveForSameProject);
+                    }
+                    transitiveForSameProject.add( dependency );
                 }
             }
         }
 
+        //Create artifacts for all dependencies, keep the transitive dependencies grouped by project they are declared in
         Set<Artifact> directDependencyArtifacts = createDependencyArtifacts( project, directDependencies );
-        Set<Artifact> transitiveDependencyArtifactsForReactorProjects = createDependencyArtifacts( project, transitiveDependencies );
+        Map<MavenProject, Set<Artifact>> reactorProjectToDependencyArtifacts = new HashMap<>();
+        for ( Entry<MavenProject, List<Dependency>> entry : reactorProjectToTransitiveDependencies.entrySet() ) {
+            reactorProjectToDependencyArtifacts.put( entry.getKey(), createDependencyArtifacts( entry.getKey(), entry.getValue()));
+        }
 
-        Artifact artifact = project.getArtifact();
+        //Resolve artifacts. Transitive dependencies are resolved with the settings of the POM they are declared in.
+        //Skip reactor projects, since they can't necessarily be resolved yet. The transitive handling above ensures we still get a complete list of dependencies.
         Set<Artifact> reactorArtifacts = new HashSet<>();
-        Set<Artifact> artifactsToResolve = new HashSet<>();
-        if (reactorProjectDependencies == null) { 
-            artifactsToResolve.addAll( directDependencyArtifacts );
+        Set<Artifact> directArtifactsToResolve = new HashSet<>();
+        if ( reactorProjects == null ) { 
+            directArtifactsToResolve.addAll( directDependencyArtifacts );
         } else {
-            // let's not include sibling dependencies, since artifact files may not be generated
-            // (aggregate mode without forking mode)
-            // Transitive dependencies for sibling dependencies are handled above
-            partitionByIsReactorProject( directDependencyArtifacts, reactorArtifacts, artifactsToResolve, reactorProjectDependencies );
-            partitionByIsReactorProject( transitiveDependencyArtifactsForReactorProjects, reactorArtifacts, artifactsToResolve, reactorProjectDependencies );
-        }
-        ArtifactResolutionResult result;
-        try
-        {
-            result = artifactResolver.resolveTransitively( artifactsToResolve, artifact, remoteRepositories,
-                                                           localRepository, artifactMetadataSource );
+            partitionByIsReactorProject( directDependencyArtifacts, reactorArtifacts, directArtifactsToResolve, idToReactorProject.keySet() );
+            for ( Entry<MavenProject, Set<Artifact>> entry : reactorProjectToDependencyArtifacts.entrySet() ) {
+                Set<Artifact> nonReactorArtifacts = new HashSet<>();
+                partitionByIsReactorProject( entry.getValue(), reactorArtifacts, nonReactorArtifacts, idToReactorProject.keySet() );
+                entry.setValue( nonReactorArtifacts );
         }
-        catch ( ArtifactResolutionException e )
-        {
-            throw new DependenciesToolException( e );
-        }
-        catch ( ArtifactNotFoundException e )
-        {
-            throw new DependenciesToolException( e );
         }
         Set<Artifact> allDependencies = new HashSet<>( reactorArtifacts );
-        allDependencies.addAll( result.getArtifacts() );
+        allDependencies.addAll( resolve( directArtifactsToResolve, project.getArtifact(), localRepository, remoteRepositories ).getArtifacts() );
+        for ( Entry<MavenProject, Set<Artifact>> entry : reactorProjectToDependencyArtifacts.entrySet() ) {
+            MavenProject reactorProject = entry.getKey();
+            Set<Artifact> toResolve = entry.getValue();
+            Artifact reactorProjectArtifact = reactorProject.getArtifact();
+            List<ArtifactRepository> reactorRemoteRepositories = reactorProject.getRemoteArtifactRepositories();
+            allDependencies.addAll( resolve( toResolve, reactorProjectArtifact, localRepository, reactorRemoteRepositories ).getArtifacts() );
+        }
 
         return new ResolvedProjectDependencies( allDependencies, directDependencyArtifacts );
     }
@@ -396,17 +425,36 @@ private Set<Artifact> createDependencyArtifacts( MavenProject project, List<Depe
         }
     }
 
-    private void partitionByIsReactorProject( Set<Artifact> artifacts, Set<Artifact> reactorArtifacts, Set<Artifact> nonReactorArtifacts, Map<String,List<Dependency>> reactorProjectDependencies ) 
+    private void partitionByIsReactorProject( Set<Artifact> artifacts, Set<Artifact> reactorArtifacts, Set<Artifact> nonReactorArtifacts, Set<String> reactorProjectIds ) 
     {
         for (Artifact dependencyArtifact : artifacts) {
                 String artifactKey = String.format( "%s:%s", dependencyArtifact.getGroupId(), dependencyArtifact.getArtifactId() );
-                if (reactorProjectDependencies.containsKey(artifactKey)) {
+                if (reactorProjectIds.contains(artifactKey)) {
                     reactorArtifacts.add(dependencyArtifact);
                 } else {
                     nonReactorArtifacts.add(dependencyArtifact);
                 }
             }
     }
+
+    private ArtifactResolutionResult resolve( Set<Artifact> artifacts, Artifact projectArtifact, ArtifactRepository localRepository, List remoteRepositories )
+        throws DependenciesToolException
+    {
+        try
+        {
+            getLogger().info(" resolving for project " + projectArtifact.getArtifactId() + " from remotes " + remoteRepositories);
+            return artifactResolver.resolveTransitively( artifacts, projectArtifact, remoteRepositories,
+                                                           localRepository, artifactMetadataSource );
+        }
+        catch ( ArtifactResolutionException e )
+        {
+            throw new DependenciesToolException( e );
+        }
+        catch ( ArtifactNotFoundException e )
+        {
+            throw new DependenciesToolException( e );
+            }
+    }
 
     /**
      * Tests if the given project is includeable against a groupdId pattern and a artifact pattern.