Exclude direct test-scope dependencies when building dependency graph

Fixes mojohaus#185

pom.xml

@@ -162,7 +162,7 @@
     <dependency>
       <groupId>org.apache.maven.shared</groupId>
       <artifactId>maven-dependency-tree</artifactId>
-      <version>2.2</version>
+      <version>3.1.1</version>
     </dependency>
     <dependency>
       <groupId>org.codehaus.plexus</groupId>

src/it/projects/flatten-dependency-all-both-test-and-transitive/pom.xml

@@ -0,0 +1,35 @@
+<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
+  <modelVersion>4.0.0</modelVersion>
+  <groupId>org.codehaus.mojo.flatten.its</groupId>
+  <artifactId>flatten-dependency-all-both-test-and-transitive</artifactId>
+  <version>0.0.1-SNAPSHOT</version>
+
+  <build>
+    <defaultGoal>verify</defaultGoal>
+    <plugins>
+      <plugin>
+        <groupId>org.codehaus.mojo</groupId>
+        <artifactId>flatten-maven-plugin</artifactId>
+        <configuration>
+          <flattenMode>oss</flattenMode>
+          <flattenDependencyMode>all</flattenDependencyMode>
+        </configuration>
+      </plugin>
+    </plugins>
+  </build>
+
+  <dependencies>
+    <dependency>
+      <groupId>org.codehaus.mojo.flatten.its</groupId>
+      <artifactId>core</artifactId>
+      <!-- This artifact depends on dep:3.2.1 with compile scope -->
+      <version>3.2.1</version>
+    </dependency>
+    <dependency>
+      <groupId>org.codehaus.mojo.flatten.its</groupId>
+      <artifactId>dep</artifactId>
+      <version>3.2.1</version>
+      <scope>test</scope>
+    </dependency>
+  </dependencies>
+</project>

src/it/projects/flatten-dependency-all-both-test-and-transitive/verify.groovy

@@ -0,0 +1,45 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+File originalPom = new File( basedir, 'pom.xml' )
+assert originalPom.exists()
+
+def originalProject = new XmlSlurper().parse( originalPom )
+assert 2 ==  originalProject.dependencies.dependency.size()
+assert "dep" ==  originalProject.dependencies.dependency[1].artifactId.text()
+assert "3.2.1" ==  originalProject.dependencies.dependency[1].version.text()
+assert "test" ==  originalProject.dependencies.dependency[1].scope.text()
+
+File flattenedPom = new File( basedir, '.flattened-pom.xml' )
+assert flattenedPom.exists()
+
+def flattenedProject = new XmlSlurper().parse( flattenedPom )
+
+// core and dep should be there. It's because while the test-scope dep (the
+// direct dependency), core declares dep as compile-scope (default) dependency.
+assert 2 ==  flattenedProject.dependencies.dependency.size()
+
+assert "core" ==  flattenedProject.dependencies.dependency[0].artifactId.text()
+assert "3.2.1" ==  flattenedProject.dependencies.dependency[0].version.text()
+assert "compile" ==  flattenedProject.dependencies.dependency[0].scope.text()
+
+// The flattened pom.xml should declare the dep under core as compile scope.
+// It's ok to ignore the one in the test-scope dependency.
+assert "dep" ==  flattenedProject.dependencies.dependency[1].artifactId.text()
+assert "3.2.1" ==  flattenedProject.dependencies.dependency[1].version.text()
+assert "compile" ==  flattenedProject.dependencies.dependency[1].scope.text()

src/it/projects/profile-with-deps-inherit-parent-depMgmt-test/parent/pom.xml

@@ -3,14 +3,10 @@
          xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
   <modelVersion>4.0.0</modelVersion>
   <groupId>org.codehaus.mojo.flatten.its</groupId>
-  <artifactId>profile-with-deps-inherit-parent-depMgmt-jdk_parent</artifactId>
+  <artifactId>profile-with-deps-inherit-parent-depMgmt-test_parent</artifactId>
   <version>0.1-SNAPSHOT</version>
   <packaging>pom</packaging>
 
-  <properties>
-    <javax.annotations.version>1.3.2</javax.annotations.version>
-  </properties>
-
   <dependencyManagement>
     <dependencies>
       <dependency>

src/it/projects/profile-with-deps-inherit-parent-depMgmt-test/pom.xml

@@ -3,12 +3,12 @@
          xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
   <modelVersion>4.0.0</modelVersion>
   <groupId>org.codehaus.mojo.flatten.its</groupId>
-  <artifactId>profile-with-deps-inherit-parent-depMgmt-jdk</artifactId>
+  <artifactId>profile-with-deps-inherit-parent-depMgmt-test</artifactId>
   <version>0.1-SNAPSHOT</version>
   <packaging>jar</packaging>
   <parent>
     <groupId>org.codehaus.mojo.flatten.its</groupId>
-    <artifactId>profile-with-deps-inherit-parent-depMgmt-jdk_parent</artifactId>
+    <artifactId>profile-with-deps-inherit-parent-depMgmt-test_parent</artifactId>
     <version>0.1-SNAPSHOT</version>
     <relativePath>parent</relativePath>
   </parent>

src/main/java/org/codehaus/mojo/flatten/FlattenMojo.java

@@ -19,7 +19,6 @@
  * under the License.
  */
 
-import java.util.Queue;
 import org.apache.maven.artifact.Artifact;
 import org.apache.maven.artifact.factory.ArtifactFactory;
 import org.apache.maven.artifact.repository.ArtifactRepository;
@@ -51,11 +50,13 @@
 import org.apache.maven.plugins.annotations.Mojo;
 import org.apache.maven.plugins.annotations.Parameter;
 import org.apache.maven.plugins.annotations.ResolutionScope;
+import org.apache.maven.project.DefaultProjectBuildingRequest;
 import org.apache.maven.project.MavenProject;
-import org.apache.maven.shared.dependency.tree.DependencyNode;
-import org.apache.maven.shared.dependency.tree.DependencyTreeBuilder;
-import org.apache.maven.shared.dependency.tree.DependencyTreeBuilderException;
-import org.apache.maven.shared.dependency.tree.traversal.DependencyNodeVisitor;
+import org.apache.maven.project.ProjectBuildingRequest;
+import org.apache.maven.shared.dependency.graph.DependencyGraphBuilder;
+import org.apache.maven.shared.dependency.graph.DependencyGraphBuilderException;
+import org.apache.maven.shared.dependency.graph.DependencyNode;
+import org.apache.maven.shared.dependency.graph.traversal.DependencyNodeVisitor;
 import org.apache.maven.shared.transfer.dependencies.resolve.DependencyResolver;
 import org.codehaus.mojo.flatten.cifriendly.CiInterpolator;
 import org.codehaus.mojo.flatten.model.resolution.FlattenModelResolver;
@@ -86,9 +87,11 @@
 import java.util.Collection;
 import java.util.Collections;
 import java.util.HashSet;
+import java.util.LinkedHashSet;
 import java.util.LinkedList;
 import java.util.List;
 import java.util.Properties;
+import java.util.Queue;
 import java.util.Set;
 
 /**
@@ -224,7 +227,7 @@ public class FlattenMojo
     @Parameter( property = "updatePomFile" )
     private Boolean updatePomFile;
 
-    /** The {@link ArtifactRepository} required to resolve POM using {@link #modelBuilder}. */
+    /** The {@link ArtifactRepository} required to resolve POM. */
     @Parameter( defaultValue = "${localRepository}", readonly = true, required = true )
     private ArtifactRepository localRepository;
 
@@ -284,7 +287,7 @@ public class FlattenMojo
      * </tr>
      * <tr>
      * <td>bom</td>
-     * <td>Like {@link #ossrh} but additionally keeps {@link Model#getDependencyManagement() dependencyManagement} and
+     * <td>Like <code>ossrh</code> but additionally keeps {@link Model#getDependencyManagement() dependencyManagement} and
      * {@link Model#getProperties() properties}. Especially it will keep the {@link Model#getDependencyManagement()
      * dependencyManagement} <em>as-is</em> without resolving parent influences and import-scoped dependencies. This is
      * useful if your POM represents a <a href=
@@ -343,7 +346,7 @@ public class FlattenMojo
     private FlattenDependencyMode flattenDependencyMode;
 
     /**
-     * The ArtifactFactory required to resolve POM using {@link #modelBuilder}.
+     * The ArtifactFactory required to resolve POM.
      */
     // Neither ArtifactFactory nor DefaultArtifactFactory tells what to use instead
     @Component
@@ -371,7 +374,7 @@ public class FlattenMojo
     private DependencyResolver dependencyResolver;
 
     @Component( hint = "default" )
-    private DependencyTreeBuilder dependencyTreeBuilder;
+    private DependencyGraphBuilder dependencyGraphBuilder;
 
     @Component(role = ArtifactDescriptorReader.class)
     private ArtifactDescriptorReader artifactDescriptorReader;
@@ -1070,29 +1073,31 @@ private void createFlattenedDependenciesDirect( List<Dependency> projectDependen
      * The collected dependencies are stored in order, so that the leaf dependencies are prioritized in front of direct dependencies.
      * In addition, every non-leaf dependencies will exclude its own direct dependency, since all transitive dependencies
      * will be collected.
-	 *
+     *
      * Transitive dependencies are all going to be collected and become a direct dependency. Maven should already resolve
      * versions properly because now the transitive dependencies are closer to the artifact. However, when this artifact is
      * being consumed, Maven Enforcer Convergence rule will fail because there may be multiple versions for the same transitive dependency.
-	 *
+     *
      * Typically, exclusion can be done by using the wildcard. However, a known Maven issue prevents convergence enforcer from
      * working properly w/ wildcard exclusions. Thus, this will exclude each dependencies explicitly rather than using the wildcard.
      *
      * @param projectDependencies is the effective POM {@link Model}'s current dependencies
      * @param flattenedDependencies is the {@link List} where to add the collected {@link Dependency dependencies}.
-     * @throws DependencyTreeBuilderException
+     * @throws DependencyGraphBuilderException
      * @throws ArtifactDescriptorException
      */
     private void createFlattenedDependenciesAll( List<Dependency> projectDependencies, List<Dependency> flattenedDependencies )
-            throws DependencyTreeBuilderException, ArtifactDescriptorException
+        throws ArtifactDescriptorException, DependencyGraphBuilderException
     {
         final Queue<DependencyNode> dependencyNodeLinkedList = new LinkedList<>();
         final Set<String> processedDependencies = new HashSet<>();
 
         final Artifact projectArtifact = this.project.getArtifact();
 
-        final DependencyNode dependencyNode = this.dependencyTreeBuilder.buildDependencyTree(this.project,
-                this.localRepository, null);
+        ProjectBuildingRequest buildingRequest = new DefaultProjectBuildingRequest( session.getProjectBuildingRequest() );
+        buildingRequest.setProject( cloneProjectWithoutTestDependencies( project ) );
+
+        final DependencyNode dependencyNode = this.dependencyGraphBuilder.buildDependencyGraph( buildingRequest, null);
 
         dependencyNode.accept(new DependencyNodeVisitor()
         {
@@ -1110,10 +1115,6 @@ private void createFlattenedDependenciesAll( List<Dependency> projectDependencie
                         return false;
                     }
                 }
-                if (node.getState() != DependencyNode.INCLUDED)
-                {
-                    return false;
-                }
                 if (node.getArtifact().isOptional())
                 {
                     return false;
@@ -1183,6 +1184,43 @@ private void createFlattenedDependenciesAll( List<Dependency> projectDependencie
         }
     }
 
+    /**
+     * Returns a cloned project that does not have direct test-scope dependencies.
+     *
+     * Test-scope project dependencies may hinder transitive dependencies by marking them as 'omitted for duplicate' when
+     * building dependency tree. This was a problem when the transitive dependency is actually needed by another non-test dependency
+     * of the project (See https://github.com/mojohaus/flatten-maven-plugin/issues/185). To avoid this interference of
+     * test-scope project dependencies, this plugin builds a dependency tree of the project without direct, test-scope dependencies.
+     *
+     * Removal of test scope dependencies is safe because these dependencies do not appear in library users' class path in
+     * any case.
+     *
+     * @param project is the original project to clone.
+     * @return a cloned project without direct test-scope dependencies.
+     */
+    private static MavenProject cloneProjectWithoutTestDependencies( MavenProject project )
+    {
+        final Set<String> testScopeProjectDependencyKeys = new HashSet<>();
+        for ( Dependency projectDependency : project.getDependencies() )
+        {
+            if ( "test".equals( projectDependency.getScope() ) )
+            {
+                testScopeProjectDependencyKeys.add( projectDependency.getManagementKey() );
+            }
+        }
+        // LinkedHashSet preserves the order.
+        final Set<Artifact> dependencyArtifactsWithoutTest = new LinkedHashSet<>( project.getDependencyArtifacts() );
+        dependencyArtifactsWithoutTest.removeIf(artifact -> {
+            // The same logic as org.apache.maven.model.Dependency.getManagementKey()
+            String managementKey = artifact.getGroupId() + ":" + artifact.getArtifactId() + ":" + artifact.getType()
+                + ( artifact.getClassifier() != null ? ":" + artifact.getClassifier() : "" );
+            return testScopeProjectDependencyKeys.contains( managementKey );
+        });
+        final MavenProject projectWithoutTestScopeDeps = project.clone();
+        projectWithoutTestScopeDeps.setDependencyArtifacts( dependencyArtifactsWithoutTest );
+        return projectWithoutTestScopeDeps;
+    }
+
     /**
      * Collects the resolved {@link Dependency dependencies} from the given <code>effectiveModel</code>.
      *