PLANNER-490 4 or more shadow variables might trigger cyclic source de…

…pendencies exception when that's not the case: Topological sorting with Kahn's algorithm + unit tests

optaplanner-core/src/main/java/org/optaplanner/core/impl/domain/solution/descriptor/SolutionDescriptor.java

@@ -24,6 +24,7 @@
 import java.util.Arrays;
 import java.util.Collection;
 import java.util.Collections;
+import java.util.Comparator;
 import java.util.HashMap;
 import java.util.Iterator;
 import java.util.LinkedHashMap;
@@ -32,8 +33,11 @@
 import java.util.ListIterator;
 import java.util.Map;
 import java.util.Set;
+import java.util.SortedMap;
 
 import com.google.common.collect.Iterators;
+import org.apache.commons.lang3.tuple.MutablePair;
+import org.apache.commons.lang3.tuple.Pair;
 import org.optaplanner.core.api.domain.solution.PlanningEntityCollectionProperty;
 import org.optaplanner.core.api.domain.solution.PlanningEntityProperty;
 import org.optaplanner.core.api.domain.solution.PlanningSolution;
@@ -292,39 +296,63 @@ public void afterAnnotationsProcessed(DescriptorPolicy descriptorPolicy) {
     }
 
     private void determineGlobalShadowOrder() {
-        List<ShadowVariableDescriptor> orderedShadowList = new LinkedList<ShadowVariableDescriptor>();
+        // Topological sorting with Kahn's algorithm
+        Comparator<Pair<ShadowVariableDescriptor, Integer>> comparator = new Comparator<Pair<ShadowVariableDescriptor, Integer>>() {
+            @Override
+            public int compare(Pair<ShadowVariableDescriptor, Integer> a, Pair<ShadowVariableDescriptor, Integer> b) {
+                int aSourceSize = a.getValue();
+                int bSourceSize = b.getValue();
+                // TODO replace by Integer.compare() when Java 7 is minimum
+                if (aSourceSize > bSourceSize) {
+                    return 1;
+                } else if (aSourceSize < bSourceSize) {
+                    return -1;
+                } else {
+                    return 0;
+                }
+            }
+        };
+        List<Pair<ShadowVariableDescriptor, Integer>> pairList = new ArrayList<Pair<ShadowVariableDescriptor, Integer>>();
+        Map<ShadowVariableDescriptor, Pair<ShadowVariableDescriptor, Integer>> shadowToPairMap
+                = new HashMap<ShadowVariableDescriptor, Pair<ShadowVariableDescriptor, Integer>>();
         for (EntityDescriptor entityDescriptor : entityDescriptorMap.values()) {
             for (ShadowVariableDescriptor shadow : entityDescriptor.getDeclaredShadowVariableDescriptors()) {
-                Integer insertionIndex = null;
-                for (ListIterator<ShadowVariableDescriptor> it = orderedShadowList.listIterator(); it.hasNext(); ) {
-                    int index = it.nextIndex();
-                    ShadowVariableDescriptor orderedShadow = it.next();
-                    if (insertionIndex == null) {
-                        if (orderedShadow.getSourceVariableDescriptorList().contains(shadow)) {
-                            insertionIndex = index;
-                        }
-                    }
-                    if (insertionIndex != null) {
-                        if (shadow.getSourceVariableDescriptorList().contains(orderedShadow)) {
-                            ShadowVariableDescriptor otherOrderedShadow = orderedShadowList.get(insertionIndex);
-                            throw new IllegalStateException("There is a cyclic shadow variable path"
-                                    + " because the shadowVariable (" + shadow.getSimpleEntityAndVariableName()
-                                    + ") must be earlier than (" + otherOrderedShadow.getSimpleEntityAndVariableName()
-                                    + ") but later than (" + orderedShadow.getSimpleEntityAndVariableName() + ").");
-                        }
-                    }
-                }
-                if (insertionIndex != null) {
-                    orderedShadowList.add(insertionIndex, shadow);
-                } else {
-                    orderedShadowList.add(shadow);
+                int sourceSize = shadow.getSourceVariableDescriptorList().size();
+                Pair<ShadowVariableDescriptor, Integer> pair = MutablePair.of(shadow, sourceSize);
+                pairList.add(pair);
+                shadowToPairMap.put(shadow, pair);
+            }
+        }
+        for (EntityDescriptor entityDescriptor : entityDescriptorMap.values()) {
+            for (GenuineVariableDescriptor genuine : entityDescriptor.getDeclaredGenuineVariableDescriptors()) {
+                for (ShadowVariableDescriptor sink : genuine.getSinkVariableDescriptorList()) {
+                    Pair<ShadowVariableDescriptor, Integer> sinkPair = shadowToPairMap.get(sink);
+                    sinkPair.setValue(sinkPair.getValue() - 1);
                 }
             }
         }
-        for (ListIterator<ShadowVariableDescriptor> it = orderedShadowList.listIterator(); it.hasNext(); ) {
-            int index = it.nextIndex();
-            ShadowVariableDescriptor orderedShadow = it.next();
-            orderedShadow.setGlobalShadowOrder(index);
+        int globalShadowOrder = 0;
+        while (!pairList.isEmpty()) {
+            Collections.sort(pairList, comparator);
+            Pair<ShadowVariableDescriptor, Integer> pair = pairList.remove(0);
+            ShadowVariableDescriptor shadow = pair.getKey();
+            if (pair.getValue() != 0) {
+                if (pair.getValue() < 0) {
+                    throw new IllegalStateException("Impossible state because the shadowVariable ("
+                            + shadow.getSimpleEntityAndVariableName()
+                            + ") can not be used more as a sink than it has sources.");
+                }
+                throw new IllegalStateException("There is a cyclic shadow variable path"
+                        + " that involves the shadowVariable (" + shadow.getSimpleEntityAndVariableName()
+                        + ") because it must be later than its sources (" + shadow.getSourceVariableDescriptorList()
+                        + ") and also earlier than its sinks (" + shadow.getSinkVariableDescriptorList() + ").");
+            }
+            for (ShadowVariableDescriptor sink : shadow.getSinkVariableDescriptorList()) {
+                Pair<ShadowVariableDescriptor, Integer> sinkPair = shadowToPairMap.get(sink);
+                sinkPair.setValue(sinkPair.getValue() - 1);
+            }
+            shadow.setGlobalShadowOrder(globalShadowOrder);
+            globalShadowOrder++;
         }
     }
 

optaplanner-core/src/main/java/org/optaplanner/core/impl/domain/variable/anchor/AnchorShadowVariableDescriptor.java

@@ -70,7 +70,7 @@ public void linkShadowSources(DescriptorPolicy descriptorPolicy) {
                     + ") with sourceVariableName (" + sourceVariableName
                     + ") which is not chained.");
         }
-        sourceVariableDescriptor.registerShadowVariableDescriptor(this);
+        sourceVariableDescriptor.registerSinkVariableDescriptor(this);
     }
 
     @Override

optaplanner-core/src/main/java/org/optaplanner/core/impl/domain/variable/custom/CustomShadowVariableDescriptor.java

@@ -140,6 +140,7 @@ public void linkShadowSources(DescriptorPolicy descriptorPolicy) {
                         + " annotated property (" + variableMemberAccessor.getName()
                         + ") with refVariable (" + refVariableDescriptor + ") that must not be a reference too.");
             }
+            refVariableDescriptor.registerSinkVariableDescriptor(this);
         } else {
             CustomShadowVariable.Source[] sources = shadowVariableAnnotation.sources();
             sourceVariableDescriptorList = new ArrayList<VariableDescriptor>(sources.length);
@@ -171,7 +172,7 @@ public void linkShadowSources(DescriptorPolicy descriptorPolicy) {
                             + sourceEntityDescriptor.getEntityClass() + ").\n"
                             + entityDescriptor.buildInvalidVariableNameExceptionMessage(sourceVariableName));
                 }
-                sourceVariableDescriptor.registerShadowVariableDescriptor(this);
+                sourceVariableDescriptor.registerSinkVariableDescriptor(this);
                 sourceVariableDescriptorList.add(sourceVariableDescriptor);
             }
         }

optaplanner-core/src/main/java/org/optaplanner/core/impl/domain/variable/descriptor/GenuineVariableDescriptor.java

@@ -255,8 +255,7 @@ public long getValueCount(Solution solution, Object entity) {
 
     @Override
     public String toString() {
-        return getClass().getSimpleName() + "(" + variableMemberAccessor.getName()
-                + " of " + entityDescriptor.getEntityClass().getName() + ")";
+        return getSimpleEntityAndVariableName() + " variable";
     }
 
 }

optaplanner-core/src/main/java/org/optaplanner/core/impl/domain/variable/descriptor/ShadowVariableDescriptor.java

@@ -38,6 +38,10 @@ public ShadowVariableDescriptor(EntityDescriptor entityDescriptor,
 
     public abstract void linkShadowSources(DescriptorPolicy descriptorPolicy);
 
+    /**
+     * Inverse of {@link #getSinkVariableDescriptorList()}.
+     * @return never null, only variables affect this shadow variable directly
+     */
     public abstract List<VariableDescriptor> getSourceVariableDescriptorList();
 
     public int getGlobalShadowOrder() {
@@ -67,4 +71,9 @@ public boolean hasVariableListener(InnerScoreDirector scoreDirector) {
      */
     public abstract VariableListener buildVariableListener(InnerScoreDirector scoreDirector);
 
+    @Override
+    public String toString() {
+        return getSimpleEntityAndVariableName() + " shadow";
+    }
+
 }

optaplanner-core/src/main/java/org/optaplanner/core/impl/domain/variable/descriptor/VariableDescriptor.java

@@ -29,7 +29,7 @@ public abstract class VariableDescriptor {
     protected final MemberAccessor variableMemberAccessor;
     protected final String variableName;
 
-    private List<ShadowVariableDescriptor> shadowVariableDescriptorList = new ArrayList<ShadowVariableDescriptor>(4);
+    protected List<ShadowVariableDescriptor> sinkVariableDescriptorList = new ArrayList<ShadowVariableDescriptor>(4);
 
     public VariableDescriptor(EntityDescriptor entityDescriptor, MemberAccessor variableMemberAccessor) {
         this.entityDescriptor = entityDescriptor;
@@ -61,16 +61,16 @@ public Class<?> getVariablePropertyType() {
     // Shadows
     // ************************************************************************
 
-    public void registerShadowVariableDescriptor(ShadowVariableDescriptor shadowVariableDescriptor) {
-        shadowVariableDescriptorList.add(shadowVariableDescriptor);
+    public void registerSinkVariableDescriptor(ShadowVariableDescriptor shadowVariableDescriptor) {
+        sinkVariableDescriptorList.add(shadowVariableDescriptor);
     }
 
     /**
-     * Primary shadow variables are direct as well as non-referencing.
-     * @return never null, only direct, non-referencing shadow variables
+     * Inverse of {@link ShadowVariableDescriptor#getSourceVariableDescriptorList()}.
+     * @return never null, only direct shadow variables that are affected by this variable
      */
-    public List<ShadowVariableDescriptor> getPrimaryShadowVariableDescriptorList() {
-        return shadowVariableDescriptorList;
+    public List<ShadowVariableDescriptor> getSinkVariableDescriptorList() {
+        return sinkVariableDescriptorList;
     }
 
     /**
@@ -93,10 +93,4 @@ public void setValue(Object entity, Object value) {
         variableMemberAccessor.executeSetter(entity, value);
     }
 
-    @Override
-    public String toString() {
-        return getClass().getSimpleName() + "(" + variableName
-                + " of " + entityDescriptor.getEntityClass().getName() + ")";
-    }
-
 }

optaplanner-core/src/main/java/org/optaplanner/core/impl/domain/variable/inverserelation/InverseRelationShadowVariableDescriptor.java

@@ -131,7 +131,7 @@ public void linkShadowSources(DescriptorPolicy descriptorPolicy) {
                         + ") which is chained. A chained variable supports only a singleton inverse.");
             }
         }
-        sourceVariableDescriptor.registerShadowVariableDescriptor(this);
+        sourceVariableDescriptor.registerSinkVariableDescriptor(this);
     }
 
     @Override

optaplanner-core/src/test/java/org/optaplanner/core/impl/domain/variable/custom/CustomVariableListenerTest.java

@@ -26,6 +26,7 @@
 import org.optaplanner.core.impl.testdata.domain.TestdataValue;
 import org.optaplanner.core.impl.testdata.domain.shadow.cyclic.TestdataCyclicShadowedSolution;
 import org.optaplanner.core.impl.testdata.domain.shadow.cyclic.reference.TestdataCyclicReferencedShadowedSolution;
+import org.optaplanner.core.impl.testdata.domain.shadow.cyclic.seven.TestdataSevenNonCyclicShadowedSolution;
 import org.optaplanner.core.impl.testdata.domain.shadow.extended.TestdataExtendedShadowedChildEntity;
 import org.optaplanner.core.impl.testdata.domain.shadow.extended.TestdataExtendedShadowedParentEntity;
 import org.optaplanner.core.impl.testdata.domain.shadow.extended.TestdataExtendedShadowedSolution;
@@ -47,6 +48,11 @@ public void cyclicReferenced() {
         SolutionDescriptor solutionDescriptor = TestdataCyclicReferencedShadowedSolution.buildSolutionDescriptor();
     }
 
+    @Test()
+    public void nonCyclicWithSevenDisorderedShadows() {
+        SolutionDescriptor solutionDescriptor = TestdataSevenNonCyclicShadowedSolution.buildSolutionDescriptor();
+    }
+
     @Test
     public void extendedZigZag() {
         GenuineVariableDescriptor variableDescriptor = TestdataExtendedShadowedParentEntity.buildVariableDescriptorForValue();

optaplanner-core/src/test/java/org/optaplanner/core/impl/testdata/domain/shadow/cyclic/seven/TestdataSevenNonCyclicShadowedEntity.java

@@ -0,0 +1,149 @@
+/*
+ * Copyright 2015 Red Hat, Inc. and/or its affiliates.
+ *
+ * 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.optaplanner.core.impl.testdata.domain.shadow.cyclic.seven;
+
+import org.optaplanner.core.api.domain.entity.PlanningEntity;
+import org.optaplanner.core.api.domain.variable.CustomShadowVariable;
+import org.optaplanner.core.api.domain.variable.PlanningVariable;
+import org.optaplanner.core.impl.domain.entity.descriptor.EntityDescriptor;
+import org.optaplanner.core.impl.domain.solution.descriptor.SolutionDescriptor;
+import org.optaplanner.core.impl.domain.variable.descriptor.GenuineVariableDescriptor;
+import org.optaplanner.core.impl.domain.variable.listener.VariableListenerAdapter;
+import org.optaplanner.core.impl.score.director.ScoreDirector;
+import org.optaplanner.core.impl.testdata.domain.TestdataObject;
+import org.optaplanner.core.impl.testdata.domain.TestdataValue;
+
+@PlanningEntity
+public class TestdataSevenNonCyclicShadowedEntity extends TestdataObject {
+
+    public static EntityDescriptor buildEntityDescriptor() {
+        SolutionDescriptor solutionDescriptor = TestdataSevenNonCyclicShadowedSolution.buildSolutionDescriptor();
+        return solutionDescriptor.findEntityDescriptorOrFail(TestdataSevenNonCyclicShadowedEntity.class);
+    }
+
+    public static GenuineVariableDescriptor buildVariableDescriptorForValue() {
+        SolutionDescriptor solutionDescriptor = TestdataSevenNonCyclicShadowedSolution.buildSolutionDescriptor();
+        EntityDescriptor entityDescriptor = solutionDescriptor.findEntityDescriptorOrFail(TestdataSevenNonCyclicShadowedEntity.class);
+        return entityDescriptor.getGenuineVariableDescriptor("value");
+    }
+
+    private TestdataValue value;
+    // Intentionally out of order
+    private String thirdShadow;
+    private String fifthShadow;
+    private String firstShadow;
+    private String fourthShadow;
+    private String secondShadow;
+    private String seventhShadow;
+    private String sixthShadow;
+
+    public TestdataSevenNonCyclicShadowedEntity() {
+    }
+
+    public TestdataSevenNonCyclicShadowedEntity(String code) {
+        super(code);
+    }
+
+    public TestdataSevenNonCyclicShadowedEntity(String code, TestdataValue value) {
+        this(code);
+        this.value = value;
+    }
+
+    @PlanningVariable(valueRangeProviderRefs = "valueRange")
+    public TestdataValue getValue() {
+        return value;
+    }
+
+    public void setValue(TestdataValue value) {
+        this.value = value;
+    }
+
+    @CustomShadowVariable(variableListenerClass = VariableListenerAdapter.class,
+            sources = {@CustomShadowVariable.Source(variableName = "secondShadow")})
+    public String getThirdShadow() {
+        return thirdShadow;
+    }
+
+    public void setThirdShadow(String thirdShadow) {
+        this.thirdShadow = thirdShadow;
+    }
+
+    @CustomShadowVariable(variableListenerClass = VariableListenerAdapter.class,
+            sources = {@CustomShadowVariable.Source(variableName = "fourthShadow")})
+    public String getFifthShadow() {
+        return fifthShadow;
+    }
+
+    public void setFifthShadow(String fifthShadow) {
+        this.fifthShadow = fifthShadow;
+    }
+
+    @CustomShadowVariable(variableListenerClass = VariableListenerAdapter.class,
+            sources = {@CustomShadowVariable.Source(variableName = "value")})
+    public String getFirstShadow() {
+        return firstShadow;
+    }
+
+    public void setFirstShadow(String firstShadow) {
+        this.firstShadow = firstShadow;
+    }
+
+    @CustomShadowVariable(variableListenerClass = VariableListenerAdapter.class,
+            sources = {@CustomShadowVariable.Source(variableName = "thirdShadow")})
+    public String getFourthShadow() {
+        return fourthShadow;
+    }
+
+    public void setFourthShadow(String fourthShadow) {
+        this.fourthShadow = fourthShadow;
+    }
+
+    @CustomShadowVariable(variableListenerClass = VariableListenerAdapter.class,
+            sources = {@CustomShadowVariable.Source(variableName = "firstShadow")})
+    public String getSecondShadow() {
+        return secondShadow;
+    }
+
+    public void setSecondShadow(String secondShadow) {
+        this.secondShadow = secondShadow;
+    }
+
+    @CustomShadowVariable(variableListenerClass = VariableListenerAdapter.class,
+            sources = {@CustomShadowVariable.Source(variableName = "sixthShadow")})
+    public String getSeventhShadow() {
+        return seventhShadow;
+    }
+
+    public void setSeventhShadow(String seventhShadow) {
+        this.seventhShadow = seventhShadow;
+    }
+
+    @CustomShadowVariable(variableListenerClass = VariableListenerAdapter.class,
+            sources = {@CustomShadowVariable.Source(variableName = "fifthShadow")})
+    public String getSixthShadow() {
+        return sixthShadow;
+    }
+
+    public void setSixthShadow(String sixthShadow) {
+        this.sixthShadow = sixthShadow;
+    }
+
+    // ************************************************************************
+    // Complex methods
+    // ************************************************************************
+
+}