#855 Fix property order when using dependsOn

processor/src/main/java/org/mapstruct/ap/internal/model/BeanMappingMethod.java

@@ -18,6 +18,10 @@
  */
 package org.mapstruct.ap.internal.model;
 
+import static org.mapstruct.ap.internal.util.Collections.first;
+import static org.mapstruct.ap.internal.util.Collections.last;
+import static org.mapstruct.ap.internal.util.Strings.getSaveVariableName;
+
 import java.text.MessageFormat;
 import java.util.ArrayList;
 import java.util.Collection;
@@ -48,18 +52,15 @@ import org.mapstruct.ap.internal.model.source.Mapping;
 import org.mapstruct.ap.internal.model.source.PropertyEntry;
 import org.mapstruct.ap.internal.model.source.SelectionParameters;
 import org.mapstruct.ap.internal.model.source.SourceMethod;
-import org.mapstruct.ap.internal.model.source.TargetReference;
 import org.mapstruct.ap.internal.model.source.SourceReference;
+import org.mapstruct.ap.internal.model.source.TargetReference;
 import org.mapstruct.ap.internal.option.ReportingPolicy;
 import org.mapstruct.ap.internal.prism.BeanMappingPrism;
 import org.mapstruct.ap.internal.prism.CollectionMappingStrategyPrism;
 import org.mapstruct.ap.internal.prism.NullValueMappingStrategyPrism;
-import static org.mapstruct.ap.internal.util.Collections.first;
-import static org.mapstruct.ap.internal.util.Collections.last;
 import org.mapstruct.ap.internal.util.MapperConfiguration;
 import org.mapstruct.ap.internal.util.Message;
 import org.mapstruct.ap.internal.util.Strings;
-import static org.mapstruct.ap.internal.util.Strings.getSaveVariableName;
 
 /**
  * A {@link MappingMethod} implemented by a {@link Mapper} class which maps one bean type to another, optionally
@@ -218,18 +219,10 @@ public class BeanMappingMethod extends MappingMethod {
             else {
                 Collections.sort(
                     propertyMappings, new Comparator<PropertyMapping>() {
-
                         @Override
                         public int compare(PropertyMapping o1, PropertyMapping o2) {
-                            if ( graphAnalyzer.getAllDescendants( o1.getName() ).contains( o2.getName() ) ) {
+                            return graphAnalyzer.getTraversalSequence( o1.getName() )
-                                return 1;
+                                - graphAnalyzer.getTraversalSequence( o2.getName() );
-                            }
-                            else if ( graphAnalyzer.getAllDescendants( o2.getName() ).contains( o1.getName() ) ) {
-                                return -1;
-                            }
-                            else {
-                                return 0;
-                            }
                         }
                     }
                 );

processor/src/main/java/org/mapstruct/ap/internal/model/dependency/GraphAnalyzer.java

@@ -20,9 +20,8 @@ package org.mapstruct.ap.internal.model.dependency;
 
 import java.util.ArrayList;
 import java.util.Arrays;
-import java.util.Collections;
-import java.util.HashMap;
 import java.util.HashSet;
+import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
@@ -38,6 +37,7 @@ public class GraphAnalyzer {
     private final Map<String, Node> nodes;
     private final Set<List<String>> cycles;
     private final Stack<Node> currentPath;
+    private int nextTraversalSequence = 0;
 
     private GraphAnalyzer(Map<String, Node> nodes) {
         this.nodes = nodes;
@@ -64,16 +64,18 @@ public class GraphAnalyzer {
     }
 
     /**
-     * Returns all the descendants of the given node, either direct or transitive ones.
+     * Returns the traversal sequence number of the given node. The ascending order of the traversal sequence numbers of
+     * multiple nodes represents the depth-first traversal order of those nodes.
      * <p>
-     * <b>Note</b>:The list will only be complete if the graph contains no cycles.
+     * <b>Note</b>: The traversal sequence numbers will only be complete if the graph contains no cycles.
      *
-     * @param name the node name to get the descendants for
+     * @param name the node name to get the traversal sequence number for
-     * @return the descendants
+     * @return the traversal sequence number, or {@code -1} if the node doesn't exist or the node was not visited (in
+     *         case of cycles).
      */
-    public Set<String> getAllDescendants(String name) {
+    public int getTraversalSequence(String name) {
         Node node = nodes.get( name );
-        return node != null ? node.getAllDescendants() : Collections.<String>emptySet();
+        return node != null ? node.getTraversalSequence() : -1;
     }
 
     public Set<List<String>> getCycles() {
@@ -98,10 +100,9 @@ public class GraphAnalyzer {
 
         for ( Node descendant : node.getDescendants() ) {
             depthFirstSearch( descendant );
-            node.getAllDescendants().addAll( descendant.getAllDescendants() );
         }
 
-        node.setProcessed( true );
+        node.setTraversalSequence( nextTraversalSequence++ );
         currentPath.pop();
     }
 
@@ -124,7 +125,7 @@ public class GraphAnalyzer {
 
     public static class GraphAnalyzerBuilder {
 
-        private final Map<String, Node> nodes = new HashMap<String, Node>();
+        private final Map<String, Node> nodes = new LinkedHashMap<String, Node>();
 
         public GraphAnalyzerBuilder withNode(String name, List<String> descendants) {
             Node node = getNode( name );

processor/src/main/java/org/mapstruct/ap/internal/model/dependency/Node.java

@@ -19,9 +19,7 @@
 package org.mapstruct.ap.internal.model.dependency;
 
 import java.util.ArrayList;
-import java.util.HashSet;
 import java.util.List;
-import java.util.Set;
 
 /**
  * A node of a directed graph.
@@ -32,22 +30,16 @@ class Node {
 
     private final String name;
     private boolean visited;
-    private boolean processed;
+    private int traversalSequence = -1;
 
     /**
      * The direct descendants of this node.
      */
     private final List<Node> descendants;
 
-    /**
-     * All descendants of this node, direct and transitive ones, as discovered through graph traversal.
-     */
-    private final Set<String> allDescendants;
-
     Node(String name) {
         this.name = name;
         descendants = new ArrayList<Node>();
-        allDescendants = new HashSet<String>();
     }
 
     public String getName() {
@@ -63,26 +55,25 @@ class Node {
     }
 
     public boolean isProcessed() {
-        return processed;
+        return traversalSequence >= 0;
     }
 
-    public void setProcessed(boolean processed) {
+    public int getTraversalSequence() {
-        this.processed = processed;
+        return traversalSequence;
+    }
+
+    public void setTraversalSequence(int traversalSequence) {
+        this.traversalSequence = traversalSequence;
     }
 
     public void addDescendant(Node node) {
         descendants.add( node );
-        allDescendants.add( node.getName() );
     }
 
     public List<Node> getDescendants() {
         return descendants;
     }
 
-    public Set<String> getAllDescendants() {
-        return allDescendants;
-    }
-
     @Override
     public String toString() {
         return name;

processor/src/test/java/org/mapstruct/ap/test/bugs/_855/OrderDemoMapper.java

@@ -0,0 +1,38 @@
+/**
+ *  Copyright 2012-2016 Gunnar Morling (http://www.gunnarmorling.de/)
+ *  and/or other contributors as indicated by the @authors tag. See the
+ *  copyright.txt file in the distribution for a full listing of all
+ *  contributors.
+ *
+ *  Licensed 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.
+ */
+package org.mapstruct.ap.test.bugs._855;
+
+import org.mapstruct.Mapper;
+import org.mapstruct.Mapping;
+import org.mapstruct.Mappings;
+import org.mapstruct.factory.Mappers;
+
+@Mapper
+public interface OrderDemoMapper {
+    OrderDemoMapper INSTANCE = Mappers.getMapper( OrderDemoMapper.class );
+
+    @Mappings({
+        @Mapping(target = "field0", dependsOn = "field2"),
+        @Mapping(target = "order", ignore = true)
+    })
+    OrderedTarget orderedWithDependsOn(OrderedSource source);
+
+    @Mapping(target = "order", ignore = true)
+    OrderedTarget orderedWithoutDependsOn(OrderedSource source);
+}

processor/src/test/java/org/mapstruct/ap/test/bugs/_855/OrderedSource.java

@@ -16,9 +16,12 @@
  *  See the License for the specific language governing permissions and
  *  limitations under the License.
  */
-package org.mapstruct.ap.test.dependency;
+package org.mapstruct.ap.test.bugs._855;
 
-public class Demo {
+/**
+ * @author Markus Heberling
+ */
+class OrderedSource {
     private String field0;
     private String field1;
     private String field2;

processor/src/test/java/org/mapstruct/ap/test/bugs/_855/OrderedTarget.java

@@ -16,12 +16,15 @@
  *  See the License for the specific language governing permissions and
  *  limitations under the License.
  */
-package org.mapstruct.ap.test.dependency;
+package org.mapstruct.ap.test.bugs._855;
 
 import java.util.LinkedList;
 import java.util.List;
 
-public class DemoDTO {
+/**
+ * @author Markus Heberling
+ */
+class OrderedTarget {
     private List<String> order = new LinkedList<String>();
 
     public void setField0(String field0) {
@@ -36,12 +39,10 @@ public class DemoDTO {
         order.add( "field2" );
     }
 
-
     public void setField3(String field3) {
         order.add( "field3" );
     }
 
-
     public void setField4(String field4) {
         order.add( "field4" );
     }

processor/src/test/java/org/mapstruct/ap/test/bugs/_855/OrderingBug855Test.java

@@ -0,0 +1,55 @@
+/**
+ *  Copyright 2012-2016 Gunnar Morling (http://www.gunnarmorling.de/)
+ *  and/or other contributors as indicated by the @authors tag. See the
+ *  copyright.txt file in the distribution for a full listing of all
+ *  contributors.
+ *
+ *  Licensed 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.
+ */
+package org.mapstruct.ap.test.bugs._855;
+
+import static org.fest.assertions.Assertions.assertThat;
+
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mapstruct.ap.testutil.IssueKey;
+import org.mapstruct.ap.testutil.WithClasses;
+import org.mapstruct.ap.testutil.runner.AnnotationProcessorTestRunner;
+
+/**
+ * @author Andreas Gudian
+ *
+ */
+@WithClasses({ OrderedSource.class, OrderedTarget.class, OrderDemoMapper.class })
+@RunWith(AnnotationProcessorTestRunner.class)
+public class OrderingBug855Test {
+
+    @Test
+    @IssueKey("855")
+    public void shouldApplyCorrectOrderingWithDependsOn() {
+        OrderedSource source = new OrderedSource();
+
+        OrderedTarget target = OrderDemoMapper.INSTANCE.orderedWithDependsOn( source );
+
+        assertThat( target.getOrder() ).containsExactly( "field2", "field0", "field1", "field3", "field4" );
+    }
+
+    @Test
+    public void shouldRetainDefaultOrderWithoutDependsOn() {
+        OrderedSource source = new OrderedSource();
+
+        OrderedTarget target = OrderDemoMapper.INSTANCE.orderedWithoutDependsOn( source );
+
+        assertThat( target.getOrder() ).containsExactly( "field0", "field1", "field2", "field3", "field4" );
+    }
+}

processor/src/test/java/org/mapstruct/ap/test/dependency/AddressMapper.java

@@ -39,10 +39,4 @@ public interface AddressMapper {
         @Mapping(target = "lastName", dependsOn = { "firstName", "middleName" })
     })
     PersonDto personToDto(Person person);
-
-    @Mappings({
-            @Mapping(target = "field0", dependsOn = "field2"),
-            @Mapping(target = "order", ignore = true)
-    })
-    DemoDTO demoToDemoDto(Demo demo);
 }

processor/src/test/java/org/mapstruct/ap/test/dependency/GraphAnalyzerTest.java

@@ -20,7 +20,6 @@ package org.mapstruct.ap.test.dependency;
 
 import static org.fest.assertions.Assertions.assertThat;
 
-import java.util.Collections;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Set;
@@ -47,7 +46,7 @@ public class GraphAnalyzerTest {
         GraphAnalyzer detector = GraphAnalyzer.withNode( "a" ).build();
 
         assertThat( detector.getCycles() ).isEmpty();
-        assertThat( detector.getAllDescendants( "a" ) ).isEmpty();
+        assertThat( detector.getTraversalSequence( "a" ) ).isEqualTo( 0 );
     }
 
     @Test
@@ -57,8 +56,8 @@ public class GraphAnalyzerTest {
                 .build();
 
         assertThat( detector.getCycles() ).isEmpty();
-        assertThat( detector.getAllDescendants( "a" ) ).containsOnly( "b" );
+        assertThat( detector.getTraversalSequence( "b" ) ).isEqualTo( 0 );
-        assertThat( detector.getAllDescendants( "b" ) ).isEmpty();
+        assertThat( detector.getTraversalSequence( "a" ) ).isEqualTo( 1 );
     }
 
     @Test
@@ -87,9 +86,10 @@ public class GraphAnalyzerTest {
                 .build();
 
         assertThat( asStrings( detector.getCycles() ) ).isEmpty();
-        assertThat( detector.getAllDescendants( "a" ) ).containsOnly( "b", "c" );
+
-        assertThat( detector.getAllDescendants( "b" ) ).containsOnly( "c" );
+        assertThat( detector.getTraversalSequence( "c" ) ).isEqualTo( 0 );
-        assertThat( detector.getAllDescendants( "c" ) ).isEmpty();
+        assertThat( detector.getTraversalSequence( "b" ) ).isEqualTo( 1 );
+        assertThat( detector.getTraversalSequence( "a" ) ).isEqualTo( 2 );
     }
 
     @Test
@@ -100,9 +100,9 @@ public class GraphAnalyzerTest {
 
         assertThat( asStrings( detector.getCycles() ) ).isEmpty();
 
-        assertThat( detector.getAllDescendants( "a" ) ).containsOnly( "b" );
+        assertThat( detector.getTraversalSequence( "b" ) ).isEqualTo( 0 );
-        assertThat( detector.getAllDescendants( "b" ) ).isEmpty();
+        assertThat( detector.getTraversalSequence( "a" ) ).isEqualTo( 1 );
-        assertThat( detector.getAllDescendants( "c" ) ).containsOnly( "b" );
+        assertThat( detector.getTraversalSequence( "c" ) ).isEqualTo( 2 );
     }
 
     @Test
@@ -136,10 +136,11 @@ public class GraphAnalyzerTest {
                 .build();
 
         assertThat( asStrings( detector.getCycles() ) ).isEmpty();
-        assertThat( detector.getAllDescendants( "a" ) ).containsOnly( "b1", "b2", "c" );
+
-        assertThat( detector.getAllDescendants( "b1" ) ).containsOnly( "c" );
+        assertThat( detector.getTraversalSequence( "c" ) ).isEqualTo( 0 );
-        assertThat( detector.getAllDescendants( "b2" ) ).containsOnly( "c" );
+        assertThat( detector.getTraversalSequence( "b1" ) ).isEqualTo( 1 );
-        assertThat( detector.getAllDescendants( "c" ) ).isEmpty();
+        assertThat( detector.getTraversalSequence( "b2" ) ).isEqualTo( 2 );
+        assertThat( detector.getTraversalSequence( "a" ) ).isEqualTo( 3 );
     }
 
     @Test
@@ -151,7 +152,8 @@ public class GraphAnalyzerTest {
                 .withNode( "c", "a" )
                 .build();
 
-        assertThat( asStrings( detector.getCycles() ) ).containsOnly( "a -> b1 -> c -> a", "a -> b2 -> c -> a" );
+        assertThat( asStrings( detector.getCycles() ) ).containsOnly( "a -> b1 -> c -> a" );
+        // note: cycle "a -> b2 -> c -> a" is currently not reported, see #856.
     }
 
     @Test
@@ -166,9 +168,13 @@ public class GraphAnalyzerTest {
 
         assertThat( detector.getCycles() ).isEmpty();
 
-        assertThat( detector.getAllDescendants( "a" ) ).containsOnly( "b1", "b2", "c1", "c2", "c3", "c4" );
+        assertThat( detector.getTraversalSequence( "c1" ) ).isEqualTo( 0 );
-        assertThat( detector.getAllDescendants( "b1" ) ).containsOnly( "c1", "c2" );
+        assertThat( detector.getTraversalSequence( "c2" ) ).isEqualTo( 1 );
-        assertThat( detector.getAllDescendants( "b2" ) ).containsOnly( "c3", "c4" );
+        assertThat( detector.getTraversalSequence( "b1" ) ).isEqualTo( 2 );
+        assertThat( detector.getTraversalSequence( "c3" ) ).isEqualTo( 3 );
+        assertThat( detector.getTraversalSequence( "c4" ) ).isEqualTo( 4 );
+        assertThat( detector.getTraversalSequence( "b2" ) ).isEqualTo( 5 );
+        assertThat( detector.getTraversalSequence( "a" ) ).isEqualTo( 6 );
     }
 
     private Set<String> asStrings(Set<List<String>> cycles) {
@@ -182,23 +188,6 @@ public class GraphAnalyzerTest {
     }
 
     private String asString(List<String> cycle) {
-        return Strings.join( normalize( cycle ), " -> " );
+        return Strings.join( cycle, " -> " );
-    }
-
-    /**
-     * "Normalizes" a cycle so that the minimum element comes first. E.g. both the cycles {@code b -> c -> a -> b} and
-     * {@code c -> a -> b -> c} would be normalized to {@code a -> b -> c -> a}.
-     */
-    private List<String> normalize(List<String> cycle) {
-        // remove the first element
-        cycle = cycle.subList( 1, cycle.size() );
-
-        // rotate the cycle so the minimum element comes first
-        Collections.rotate( cycle, -cycle.indexOf( Collections.min( cycle ) ) );
-
-        // add the first element add the end to re-close the cycle
-        cycle.add( cycle.get( 0 ) );
-
-        return cycle;
     }
 }

processor/src/test/java/org/mapstruct/ap/test/dependency/OrderingTest.java

@@ -18,6 +18,8 @@
  */
 package org.mapstruct.ap.test.dependency;
 
+import static org.fest.assertions.Assertions.assertThat;
+
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.mapstruct.Mapping;
@@ -28,27 +30,15 @@ import org.mapstruct.ap.testutil.compilation.annotation.Diagnostic;
 import org.mapstruct.ap.testutil.compilation.annotation.ExpectedCompilationOutcome;
 import org.mapstruct.ap.testutil.runner.AnnotationProcessorTestRunner;
 
-import static org.fest.assertions.Assertions.assertThat;
-
 /**
  * Test for ordering mapped attributes by means of {@link Mapping#dependsOn()}.
  *
  * @author Gunnar Morling
  */
-@WithClasses({ Person.class, PersonDto.class, Address.class, AddressDto.class, AddressMapper.class,
+@WithClasses({ Person.class, PersonDto.class, Address.class, AddressDto.class, AddressMapper.class })
-        Demo.class, DemoDTO.class })
 @RunWith(AnnotationProcessorTestRunner.class)
 public class OrderingTest {
 
-    @Test
-    public void shouldApplyCorrectOrdering() {
-        Demo source = new Demo();
-
-        DemoDTO target = AddressMapper.INSTANCE.demoToDemoDto( source );
-
-        assertThat( target.getOrder() ).containsExactly( "field1", "field2", "field0", "field3", "field4" );
-    }
-
     @Test
     @IssueKey("304")
     public void shouldApplyChainOfDependencies() {