diff --git a/processor/src/main/java/org/mapstruct/ap/internal/model/BeanMappingMethod.java b/processor/src/main/java/org/mapstruct/ap/internal/model/BeanMappingMethod.java index 67cca6fbc..cf5f50252 100644 --- a/processor/src/main/java/org/mapstruct/ap/internal/model/BeanMappingMethod.java +++ b/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() { - @Override public int compare(PropertyMapping o1, PropertyMapping o2) { - if ( graphAnalyzer.getAllDescendants( o1.getName() ).contains( o2.getName() ) ) { - return 1; - } - else if ( graphAnalyzer.getAllDescendants( o2.getName() ).contains( o1.getName() ) ) { - return -1; - } - else { - return 0; - } + return graphAnalyzer.getTraversalSequence( o1.getName() ) + - graphAnalyzer.getTraversalSequence( o2.getName() ); } } ); diff --git a/processor/src/main/java/org/mapstruct/ap/internal/model/dependency/GraphAnalyzer.java b/processor/src/main/java/org/mapstruct/ap/internal/model/dependency/GraphAnalyzer.java index 810fa32da..961a10a55 100644 --- a/processor/src/main/java/org/mapstruct/ap/internal/model/dependency/GraphAnalyzer.java +++ b/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 nodes; private final Set> cycles; private final Stack currentPath; + private int nextTraversalSequence = 0; private GraphAnalyzer(Map 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. *

- * Note:The list will only be complete if the graph contains no cycles. + * Note: The traversal sequence numbers will only be complete if the graph contains no cycles. * - * @param name the node name to get the descendants for - * @return the descendants + * @param name the node name to get the traversal sequence number for + * @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 getAllDescendants(String name) { + public int getTraversalSequence(String name) { Node node = nodes.get( name ); - return node != null ? node.getAllDescendants() : Collections.emptySet(); + return node != null ? node.getTraversalSequence() : -1; } public Set> 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 nodes = new HashMap(); + private final Map nodes = new LinkedHashMap(); public GraphAnalyzerBuilder withNode(String name, List descendants) { Node node = getNode( name ); diff --git a/processor/src/main/java/org/mapstruct/ap/internal/model/dependency/Node.java b/processor/src/main/java/org/mapstruct/ap/internal/model/dependency/Node.java index e891313d0..b17d73961 100644 --- a/processor/src/main/java/org/mapstruct/ap/internal/model/dependency/Node.java +++ b/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 descendants; - /** - * All descendants of this node, direct and transitive ones, as discovered through graph traversal. - */ - private final Set allDescendants; - Node(String name) { this.name = name; descendants = new ArrayList(); - allDescendants = new HashSet(); } public String getName() { @@ -63,26 +55,25 @@ class Node { } public boolean isProcessed() { - return processed; + return traversalSequence >= 0; } - public void setProcessed(boolean processed) { - this.processed = processed; + public int getTraversalSequence() { + return traversalSequence; + } + + public void setTraversalSequence(int traversalSequence) { + this.traversalSequence = traversalSequence; } public void addDescendant(Node node) { descendants.add( node ); - allDescendants.add( node.getName() ); } public List getDescendants() { return descendants; } - public Set getAllDescendants() { - return allDescendants; - } - @Override public String toString() { return name; diff --git a/processor/src/test/java/org/mapstruct/ap/test/bugs/_855/OrderDemoMapper.java b/processor/src/test/java/org/mapstruct/ap/test/bugs/_855/OrderDemoMapper.java new file mode 100644 index 000000000..01c62de5f --- /dev/null +++ b/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); +} diff --git a/processor/src/test/java/org/mapstruct/ap/test/dependency/Demo.java b/processor/src/test/java/org/mapstruct/ap/test/bugs/_855/OrderedSource.java similarity index 94% rename from processor/src/test/java/org/mapstruct/ap/test/dependency/Demo.java rename to processor/src/test/java/org/mapstruct/ap/test/bugs/_855/OrderedSource.java index 15d504931..dbd108189 100644 --- a/processor/src/test/java/org/mapstruct/ap/test/dependency/Demo.java +++ b/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; diff --git a/processor/src/test/java/org/mapstruct/ap/test/dependency/DemoDTO.java b/processor/src/test/java/org/mapstruct/ap/test/bugs/_855/OrderedTarget.java similarity index 93% rename from processor/src/test/java/org/mapstruct/ap/test/dependency/DemoDTO.java rename to processor/src/test/java/org/mapstruct/ap/test/bugs/_855/OrderedTarget.java index 2735042a5..d62819a75 100644 --- a/processor/src/test/java/org/mapstruct/ap/test/dependency/DemoDTO.java +++ b/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 order = new LinkedList(); 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" ); } diff --git a/processor/src/test/java/org/mapstruct/ap/test/bugs/_855/OrderingBug855Test.java b/processor/src/test/java/org/mapstruct/ap/test/bugs/_855/OrderingBug855Test.java new file mode 100644 index 000000000..1904a9a08 --- /dev/null +++ b/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" ); + } +} diff --git a/processor/src/test/java/org/mapstruct/ap/test/dependency/AddressMapper.java b/processor/src/test/java/org/mapstruct/ap/test/dependency/AddressMapper.java index 42ef48a8c..c2bb58522 100644 --- a/processor/src/test/java/org/mapstruct/ap/test/dependency/AddressMapper.java +++ b/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); } diff --git a/processor/src/test/java/org/mapstruct/ap/test/dependency/GraphAnalyzerTest.java b/processor/src/test/java/org/mapstruct/ap/test/dependency/GraphAnalyzerTest.java index 2a3b28aa8..3e57c1cc9 100644 --- a/processor/src/test/java/org/mapstruct/ap/test/dependency/GraphAnalyzerTest.java +++ b/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.getAllDescendants( "b" ) ).isEmpty(); + assertThat( detector.getTraversalSequence( "b" ) ).isEqualTo( 0 ); + 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.getAllDescendants( "c" ) ).isEmpty(); + + assertThat( detector.getTraversalSequence( "c" ) ).isEqualTo( 0 ); + 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.getAllDescendants( "b" ) ).isEmpty(); - assertThat( detector.getAllDescendants( "c" ) ).containsOnly( "b" ); + assertThat( detector.getTraversalSequence( "b" ) ).isEqualTo( 0 ); + assertThat( detector.getTraversalSequence( "a" ) ).isEqualTo( 1 ); + 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.getAllDescendants( "b2" ) ).containsOnly( "c" ); - assertThat( detector.getAllDescendants( "c" ) ).isEmpty(); + + assertThat( detector.getTraversalSequence( "c" ) ).isEqualTo( 0 ); + assertThat( detector.getTraversalSequence( "b1" ) ).isEqualTo( 1 ); + 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.getAllDescendants( "b1" ) ).containsOnly( "c1", "c2" ); - assertThat( detector.getAllDescendants( "b2" ) ).containsOnly( "c3", "c4" ); + assertThat( detector.getTraversalSequence( "c1" ) ).isEqualTo( 0 ); + assertThat( detector.getTraversalSequence( "c2" ) ).isEqualTo( 1 ); + 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 asStrings(Set> cycles) { @@ -182,23 +188,6 @@ public class GraphAnalyzerTest { } private String asString(List cycle) { - return Strings.join( normalize( 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 normalize(List 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; + return Strings.join( cycle, " -> " ); } } diff --git a/processor/src/test/java/org/mapstruct/ap/test/dependency/OrderingTest.java b/processor/src/test/java/org/mapstruct/ap/test/dependency/OrderingTest.java index c0be4978a..2591f63a2 100644 --- a/processor/src/test/java/org/mapstruct/ap/test/dependency/OrderingTest.java +++ b/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, - Demo.class, DemoDTO.class }) +@WithClasses({ Person.class, PersonDto.class, Address.class, AddressDto.class, AddressMapper.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() {