[MNG-3814] Reactor builds fail due to erroneous cycle in project sorting which does not consider versions

git-svn-id: https://svn.apache.org/repos/asf/maven/components/trunk@798906 13f79535-47bb-0310-9956-ffa450edef68

maven-core/src/main/java/org/apache/maven/execution/ProjectSorter.java

@@ -29,10 +29,15 @@ import java.util.Map;
 
 import org.apache.maven.artifact.ArtifactUtils;
 import org.apache.maven.model.Dependency;
+import org.apache.maven.model.Extension;
+import org.apache.maven.model.Parent;
+import org.apache.maven.model.Plugin;
 import org.apache.maven.project.MavenProject;
+import org.codehaus.plexus.util.StringUtils;
 import org.codehaus.plexus.util.dag.CycleDetectedException;
 import org.codehaus.plexus.util.dag.DAG;
 import org.codehaus.plexus.util.dag.TopologicalSorter;
+import org.codehaus.plexus.util.dag.Vertex;
 
 public class ProjectSorter
 {
@@ -70,81 +75,88 @@ public class ProjectSorter
     {
         dag = new DAG();
 
-        Map<String,MavenProject> projectMap = new HashMap<String,MavenProject>();
+        // groupId:artifactId:version -> project
+        Map<String, MavenProject> projectMap = new HashMap<String, MavenProject>( projects.size() * 2 );
+
+        // groupId:artifactId -> (version -> vertex)
+        Map<String, Map<String, Vertex>> vertexMap = new HashMap<String, Map<String, Vertex>>( projects.size() * 2 );
 
         for ( MavenProject project : projects )
         {
-            String id = getId( project );
+            String projectId = getId( project );
 
-            if ( dag.getVertex( id ) != null )
+            MavenProject conflictingProject = projectMap.put( projectId, project );
+
+            if ( conflictingProject != null )
             {
-                MavenProject conflictingProject = projectMap.get( id );
+                throw new DuplicateProjectException( projectId, conflictingProject.getFile(), project.getFile(),
-
+                                                     "Project '" + projectId + "' is duplicated in the reactor" );
-                throw new DuplicateProjectException( id, conflictingProject.getFile(), project.getFile(), "Project '" + id + "' is duplicated in the reactor" );
             }
 
-            dag.addVertex( id );
+            String projectKey = ArtifactUtils.versionlessKey( project.getGroupId(), project.getArtifactId() );
 
-            projectMap.put( id, project );
+            Map<String, Vertex> vertices = vertexMap.get( projectKey );
+            if ( vertices == null )
+            {
+                vertices = new HashMap<String, Vertex>( 2, 1 );
+                vertexMap.put( projectKey, vertices );
+            }
+            vertices.put( project.getVersion(), dag.addVertex( projectId ) );
         }
 
-        for ( MavenProject project : projects )
+        for ( Vertex projectVertex : (List<Vertex>) dag.getVerticies() )
         {
-            String id = getId( project );
+            String projectId = projectVertex.getLabel();
 
-            for( Dependency dependency : project.getDependencies() )
+            MavenProject project = projectMap.get( projectId );
+
+            for ( Dependency dependency : project.getDependencies() )
             {
-                String dependencyId = ArtifactUtils.versionlessKey( dependency.getGroupId(), dependency.getArtifactId() );
+                addEdge( projectMap, vertexMap, project, projectVertex, dependency.getGroupId(),
-
+                         dependency.getArtifactId(), dependency.getVersion(), false, false );
-                if ( dag.getVertex( dependencyId ) != null )
-                {
-                    project.addProjectReference( projectMap.get( dependencyId ) );
-
-                    dag.addEdge( id, dependencyId );
-                }
             }
 
-            MavenProject parent = project.getParent();
+            Parent parent = project.getModel().getParent();
-            
+
             if ( parent != null )
             {
-                String parentId = ArtifactUtils.versionlessKey( parent.getGroupId(), parent.getArtifactId() );
+                // Parent is added as an edge, but must not cause a cycle - so we remove any other edges it has in conflict
-                if ( dag.getVertex( parentId ) != null )
+                addEdge( projectMap, vertexMap, null, projectVertex, parent.getGroupId(), parent.getArtifactId(),
-                {
+                         parent.getVersion(), true, false );
-                    // Parent is added as an edge, but must not cause a cycle - so we remove any other edges it has in conflict
-                    if ( dag.hasEdge( parentId, id ) )
-                    {
-                        dag.removeEdge( parentId, id );
-                    }
-                    
-                    dag.addEdge( id, parentId );
-                }
             }
-            
+
-            /*
+            List<Plugin> buildPlugins = project.getBuildPlugins();
-            
+            if ( buildPlugins != null )
-            TODO: Now that the build plan is fully fleshed out we have cycles
-            
-            if ( project.getBuildPlugins() != null )
             {
-                for( Plugin plugin : project.getBuildPlugins() )
+                for ( Plugin plugin : buildPlugins )
                 {
-                    String pluginId = ArtifactUtils.versionlessKey( plugin.getGroupId(), plugin.getArtifactId() );
+                    addEdge( projectMap, vertexMap, project, projectVertex, plugin.getGroupId(),
-                    
+                             plugin.getArtifactId(), plugin.getVersion(), false, true );
-                    if ( ( dag.getVertex( pluginId ) != null ) && !pluginId.equals( id ) )
+
+                    for ( Dependency dependency : plugin.getDependencies() )
                     {
-                        addEdgeWithParentCheck( projectMap, pluginId, project, id );
+                        addEdge( projectMap, vertexMap, project, projectVertex, dependency.getGroupId(),
+                                 dependency.getArtifactId(), dependency.getVersion(), false, true );
                     }
                 }
             }
-            */
+
+            List<Extension> buildExtensions = project.getBuildExtensions();
+            if ( buildExtensions != null )
+            {
+                for ( Extension extension : buildExtensions )
+                {
+                    addEdge( projectMap, vertexMap, project, projectVertex, extension.getGroupId(),
+                             extension.getArtifactId(), extension.getVersion(), false, true );
+                }
+            }
         }
 
-        List<MavenProject> sortedProjects = new ArrayList<MavenProject>();
+        List<MavenProject> sortedProjects = new ArrayList<MavenProject>( projects.size() );
 
         List<String> sortedProjectLabels = TopologicalSorter.sort( dag );
-         
+
-        for( String id : sortedProjectLabels )
+        for ( String id : sortedProjectLabels )
         {
             sortedProjects.add( projectMap.get( id ) );
         }
@@ -152,30 +164,73 @@ public class ProjectSorter
         this.sortedProjects = Collections.unmodifiableList( sortedProjects );
     }
 
-    private void addEdgeWithParentCheck( Map<String,MavenProject> projectMap, String projectRefId, MavenProject project, String id )
+    private void addEdge( Map<String, MavenProject> projectMap, Map<String, Map<String, Vertex>> vertexMap,
+                          MavenProject project, Vertex projectVertex, String groupId, String artifactId,
+                          String version, boolean force, boolean safe )
         throws CycleDetectedException
     {
-        MavenProject extProject = projectMap.get( projectRefId );
+        String projectKey = ArtifactUtils.versionlessKey( groupId, artifactId );
 
-        if ( extProject == null )
+        Map<String, Vertex> vertices = vertexMap.get( projectKey );
+
+        if ( vertices != null )
+        {
+            if ( isSpecificVersion( version ) )
+            {
+                Vertex vertex = vertices.get( version );
+                if ( vertex != null )
+                {
+                    addEdge( projectVertex, vertex, project, projectMap, force, safe );
+                }
+            }
+            else
+            {
+                for ( Vertex vertex : vertices.values() )
+                {
+                    addEdge( projectVertex, vertex, project, projectMap, force, safe );
+                }
+            }
+        }
+    }
+
+    private void addEdge( Vertex fromVertex, Vertex toVertex, MavenProject fromProject,
+                          Map<String, MavenProject> projectMap, boolean force, boolean safe )
+        throws CycleDetectedException
+    {
+        if ( fromVertex.equals( toVertex ) )
         {
             return;
         }
 
-        project.addProjectReference( extProject );
+        if ( fromProject != null )
-
-        MavenProject extParent = extProject.getParent();
-        if ( extParent != null )
         {
-            String parentId = ArtifactUtils.versionlessKey( extParent.getGroupId(), extParent.getArtifactId() );
+            MavenProject toProject = projectMap.get( toVertex.getLabel() );
-            // Don't add edge from parent to extension if a reverse edge already exists
+            fromProject.addProjectReference( toProject );
-            if ( !dag.hasEdge( projectRefId, id ) || !parentId.equals( id ) )
+        }
+
+        if ( force && toVertex.getChildren().contains( fromVertex ) )
+        {
+            dag.removeEdge( toVertex, fromVertex );
+        }
+
+        try
+        {
+            dag.addEdge( fromVertex, toVertex );
+        }
+        catch ( CycleDetectedException e )
+        {
+            if ( !safe )
             {
-                dag.addEdge( id, projectRefId );
+                throw e;
             }
         }
     }
 
+    private boolean isSpecificVersion( String version )
+    {
+        return !( StringUtils.isEmpty( version ) || version.startsWith( "[" ) || version.startsWith( "(" ) );
+    }
+
     // TODO: !![jc; 28-jul-2005] check this; if we're using '-r' and there are aggregator tasks, this will result in weirdness.
     public MavenProject getTopLevelProject()
     {
@@ -216,7 +271,7 @@ public class ProjectSorter
 
     public static String getId( MavenProject project )
     {
-        return ArtifactUtils.versionlessKey( project.getGroupId(), project.getArtifactId() );
+        return ArtifactUtils.key( project.getGroupId(), project.getArtifactId(), project.getVersion() );
     }
 
 }

maven-core/src/test/java/org/apache/maven/execution/ProjectSorterTest.java

@@ -29,6 +29,9 @@ import org.apache.maven.model.Build;
 import org.apache.maven.model.Dependency;
 import org.apache.maven.model.Extension;
 import org.apache.maven.model.Model;
+import org.apache.maven.model.Parent;
+import org.apache.maven.model.Plugin;
+import org.apache.maven.model.PluginManagement;
 import org.apache.maven.project.MavenProject;
 import org.codehaus.plexus.util.dag.CycleDetectedException;
 
@@ -42,25 +45,115 @@ public class ProjectSorterTest
     extends TestCase
 {
 
+    private Parent createParent( MavenProject project )
+    {
+        return createParent( project.getGroupId(), project.getArtifactId(), project.getVersion() );
+    }
+
+    private Parent createParent( String groupId, String artifactId, String version )
+    {
+        Parent plugin = new Parent();
+        plugin.setGroupId( groupId );
+        plugin.setArtifactId( artifactId );
+        plugin.setVersion( version );
+        return plugin;
+    }
+
+    private Dependency createDependency( MavenProject project )
+    {
+        return createDependency( project.getGroupId(), project.getArtifactId(), project.getVersion() );
+    }
+
+    private Dependency createDependency( String groupId, String artifactId, String version )
+    {
+        Dependency depdendency = new Dependency();
+        depdendency.setGroupId( groupId );
+        depdendency.setArtifactId( artifactId );
+        depdendency.setVersion( version );
+        return depdendency;
+    }
+
+    private Plugin createPlugin( MavenProject project )
+    {
+        return createPlugin( project.getGroupId(), project.getArtifactId(), project.getVersion() );
+    }
+
+    private Plugin createPlugin( String groupId, String artifactId, String version )
+    {
+        Plugin plugin = new Plugin();
+        plugin.setGroupId( groupId );
+        plugin.setArtifactId( artifactId );
+        plugin.setVersion( version );
+        return plugin;
+    }
+
+    private Extension createExtension( String groupId, String artifactId, String version )
+    {
+        Extension extension = new Extension();
+        extension.setGroupId( groupId );
+        extension.setArtifactId( artifactId );
+        extension.setVersion( version );
+        return extension;
+    }
+
+    private static MavenProject createProject( String groupId, String artifactId, String version )
+    {
+        Model model = new Model();
+        model.setGroupId( groupId );
+        model.setArtifactId( artifactId );
+        model.setVersion( version );
+        model.setBuild( new Build() );
+        return new MavenProject( model );
+    }
+
+    public void testShouldNotFailWhenPluginDepReferencesCurrentProject()
+        throws CycleDetectedException, DuplicateProjectException
+    {
+        MavenProject project = createProject( "group", "artifact", "1.0" );
+
+        Build build = project.getModel().getBuild();
+
+        Plugin plugin = createPlugin( "other.group", "other-artifact", "1.0" );
+
+        Dependency dep = createDependency( "group", "artifact", "1.0" );
+
+        plugin.addDependency( dep );
+
+        build.addPlugin( plugin );
+
+        new ProjectSorter( Collections.singletonList( project ) );
+    }
+
+    public void testShouldNotFailWhenManagedPluginDepReferencesCurrentProject()
+        throws CycleDetectedException, DuplicateProjectException
+    {
+        MavenProject project = createProject( "group", "artifact", "1.0" );
+
+        Build build = project.getModel().getBuild();
+
+        PluginManagement pMgmt = new PluginManagement();
+
+        Plugin plugin = createPlugin( "other.group", "other-artifact", "1.0" );
+
+        Dependency dep = createDependency( "group", "artifact", "1.0" );
+
+        plugin.addDependency( dep );
+
+        pMgmt.addPlugin( plugin );
+
+        build.setPluginManagement( pMgmt );
+
+        new ProjectSorter( Collections.singletonList( project ) );
+    }
+
     public void testShouldNotFailWhenProjectReferencesNonExistentProject()
         throws CycleDetectedException, DuplicateProjectException
     {
         MavenProject project = createProject( "group", "artifact", "1.0" );
-        Model model = project.getModel();
 
-        Build build = model.getBuild();
+        Build build = project.getModel().getBuild();
 
-        if ( build == null )
+        Extension extension = createExtension( "other.group", "other-artifact", "1.0" );
-        {
-            build = new Build();
-            model.setBuild( build );
-        }
-
-        Extension extension = new Extension();
-
-        extension.setArtifactId( "other-artifact" );
-        extension.setGroupId( "other.group" );
-        extension.setVersion( "1.0" );
 
         build.addExtension( extension );
 
@@ -70,7 +163,7 @@ public class ProjectSorterTest
     public void testMatchingArtifactIdsDifferentGroupIds()
         throws CycleDetectedException, DuplicateProjectException
     {
-        List projects = new ArrayList();
+        List<MavenProject> projects = new ArrayList<MavenProject>();
         MavenProject project1 = createProject( "groupId1", "artifactId", "1.0" );
         projects.add( project1 );
         MavenProject project2 = createProject( "groupId2", "artifactId", "1.0" );
@@ -86,7 +179,7 @@ public class ProjectSorterTest
     public void testMatchingGroupIdsDifferentArtifactIds()
         throws CycleDetectedException, DuplicateProjectException
     {
-        List projects = new ArrayList();
+        List<MavenProject> projects = new ArrayList<MavenProject>();
         MavenProject project1 = createProject( "groupId", "artifactId1", "1.0" );
         projects.add( project1 );
         MavenProject project2 = createProject( "groupId", "artifactId2", "1.0" );
@@ -102,7 +195,7 @@ public class ProjectSorterTest
     public void testMatchingIdsAndVersions()
         throws CycleDetectedException
     {
-        List projects = new ArrayList();
+        List<MavenProject> projects = new ArrayList<MavenProject>();
         MavenProject project1 = createProject( "groupId", "artifactId", "1.0" );
         projects.add( project1 );
         MavenProject project2 = createProject( "groupId", "artifactId", "1.0" );
@@ -121,41 +214,152 @@ public class ProjectSorterTest
     }
 
     public void testMatchingIdsAndDifferentVersions()
-        throws CycleDetectedException
+        throws CycleDetectedException, DuplicateProjectException
     {
-        List projects = new ArrayList();
+        List<MavenProject> projects = new ArrayList<MavenProject>();
         MavenProject project1 = createProject( "groupId", "artifactId", "1.0" );
         projects.add( project1 );
         MavenProject project2 = createProject( "groupId", "artifactId", "2.0" );
         projects.add( project2 );
 
-        try
+        projects = new ProjectSorter( projects ).getSortedProjects();
-        {
+        assertEquals( project1, projects.get( 0 ) );
-            projects = new ProjectSorter( projects ).getSortedProjects();
+        assertEquals( project2, projects.get( 1 ) );
-            fail( "Duplicate projects should fail" );
-        }
-        catch ( DuplicateProjectException e )
-        {
-            // expected
-            assertTrue( true );
-        }
     }
 
-    private Dependency createDependency( MavenProject project )
+    public void testPluginDependenciesInfluenceSorting()
+        throws Exception
     {
-        Dependency depdendency = new Dependency();
+        List<MavenProject> projects = new ArrayList<MavenProject>();
-        depdendency.setArtifactId( project.getArtifactId() );
+
-        depdendency.setGroupId( project.getGroupId() );
+        MavenProject parentProject = createProject( "groupId", "parent", "1.0" );
-        depdendency.setVersion( project.getVersion() );
+        projects.add( parentProject );
-        return depdendency;
+
+        MavenProject declaringProject = createProject( "groupId", "declarer", "1.0" );
+        declaringProject.setParent( parentProject );
+        declaringProject.getModel().setParent( createParent( parentProject ) );
+        projects.add( declaringProject );
+
+        MavenProject pluginLevelDepProject = createProject( "groupId", "plugin-level-dep", "1.0" );
+        pluginLevelDepProject.setParent( parentProject );
+        pluginLevelDepProject.getModel().setParent( createParent( parentProject ) );
+        projects.add( pluginLevelDepProject );
+
+        MavenProject pluginProject = createProject( "groupId", "plugin", "1.0" );
+        pluginProject.setParent( parentProject );
+        pluginProject.getModel().setParent( createParent( parentProject ) );
+        projects.add( pluginProject );
+
+        Plugin plugin = createPlugin( pluginProject );
+
+        plugin.addDependency( createDependency( pluginLevelDepProject ) );
+
+        Build build = declaringProject.getModel().getBuild();
+
+        build.addPlugin( plugin );
+
+        projects = new ProjectSorter( projects ).getSortedProjects();
+
+        assertEquals( parentProject, projects.get( 0 ) );
+
+        // the order of these two is non-deterministic, based on when they're added to the reactor.
+        assertTrue( projects.contains( pluginProject ) );
+        assertTrue( projects.contains( pluginLevelDepProject ) );
+
+        // the declaring project MUST be listed after the plugin and its plugin-level dep, though.
+        assertEquals( declaringProject, projects.get( 3 ) );
     }
 
-    private static MavenProject createProject( String groupId, String artifactId, String version )
+    public void testPluginDependenciesInfluenceSorting_DeclarationInParent()
+        throws Exception
     {
-        Model model = new Model();
+        List<MavenProject> projects = new ArrayList<MavenProject>();
-        model.setGroupId( groupId );
+
-        model.setArtifactId( artifactId );
+        MavenProject parentProject = createProject( "groupId", "parent-declarer", "1.0" );
-        model.setVersion( version );
+        projects.add( parentProject );
-        return new MavenProject( model );
+
+        MavenProject pluginProject = createProject( "groupId", "plugin", "1.0" );
+        pluginProject.setParent( parentProject );
+        pluginProject.getModel().setParent( createParent( parentProject ) );
+        projects.add( pluginProject );
+
+        MavenProject pluginLevelDepProject = createProject( "groupId", "plugin-level-dep", "1.0" );
+        pluginLevelDepProject.setParent( parentProject );
+        pluginLevelDepProject.getModel().setParent( createParent( parentProject ) );
+        projects.add( pluginLevelDepProject );
+
+        Plugin plugin = createPlugin( pluginProject );
+
+        plugin.addDependency( createDependency( pluginLevelDepProject ) );
+
+        Build build = parentProject.getModel().getBuild();
+
+        build.addPlugin( plugin );
+
+        projects = new ProjectSorter( projects ).getSortedProjects();
+
+        System.out.println( projects );
+
+        assertEquals( parentProject, projects.get( 0 ) );
+
+        // the order of these two is non-deterministic, based on when they're added to the reactor.
+        assertTrue( projects.contains( pluginProject ) );
+        assertTrue( projects.contains( pluginLevelDepProject ) );
     }
+
+    public void testPluginVersionsAreConsidered()
+        throws Exception
+    {
+        List<MavenProject> projects = new ArrayList<MavenProject>();
+
+        MavenProject pluginProjectA = createProject( "group", "plugin-a", "2.0-SNAPSHOT" );
+        projects.add( pluginProjectA );
+        pluginProjectA.getModel().getBuild().addPlugin( createPlugin( "group", "plugin-b", "1.0" ) );
+
+        MavenProject pluginProjectB = createProject( "group", "plugin-b", "2.0-SNAPSHOT" );
+        projects.add( pluginProjectB );
+        pluginProjectB.getModel().getBuild().addPlugin( createPlugin( "group", "plugin-a", "1.0" ) );
+
+        projects = new ProjectSorter( projects ).getSortedProjects();
+
+        assertTrue( projects.contains( pluginProjectA ) );
+        assertTrue( projects.contains( pluginProjectB ) );
+    }
+
+    public void testDependencyPrecedesProjectThatUsesSpecificDependencyVersion()
+        throws Exception
+    {
+        List<MavenProject> projects = new ArrayList<MavenProject>();
+
+        MavenProject usingProject = createProject( "group", "project", "1.0" );
+        projects.add( usingProject );
+        usingProject.getModel().addDependency( createDependency( "group", "dependency", "1.0" ) );
+
+        MavenProject pluginProject = createProject( "group", "dependency", "1.0" );
+        projects.add( pluginProject );
+
+        projects = new ProjectSorter( projects ).getSortedProjects();
+
+        assertEquals( pluginProject, projects.get( 0 ) );
+        assertEquals( usingProject, projects.get( 1 ) );
+    }
+
+    public void testDependencyPrecedesProjectThatUsesUnresolvedDependencyVersion()
+        throws Exception
+    {
+        List<MavenProject> projects = new ArrayList<MavenProject>();
+
+        MavenProject usingProject = createProject( "group", "project", "1.0" );
+        projects.add( usingProject );
+        usingProject.getModel().addDependency( createDependency( "group", "dependency", "[1.0,)" ) );
+
+        MavenProject pluginProject = createProject( "group", "dependency", "1.0" );
+        projects.add( pluginProject );
+
+        projects = new ProjectSorter( projects ).getSortedProjects();
+
+        assertEquals( pluginProject, projects.get( 0 ) );
+        assertEquals( usingProject, projects.get( 1 ) );
+    }
+
 }