PLANNER-759 fix ConstraintMatches in excess and lacking does not reco…

…gnize duplicates

optaplanner-core/src/main/java/org/optaplanner/core/config/util/ConfigUtils.java

@@ -37,6 +37,7 @@
 import javax.script.ScriptEngineManager;
 import javax.script.ScriptException;
 
+import org.optaplanner.core.api.domain.lookup.PlanningId;
 import org.optaplanner.core.config.AbstractConfig;
 import org.optaplanner.core.impl.domain.common.AlphabeticMemberComparator;
 import org.optaplanner.core.impl.domain.common.ReflectionHelper;
@@ -45,6 +46,8 @@
 import org.optaplanner.core.impl.domain.common.accessor.MemberAccessor;
 import org.optaplanner.core.impl.domain.common.accessor.MethodMemberAccessor;
 
+import static org.optaplanner.core.config.util.ConfigUtils.MemberAccessorType.FIELD_OR_READ_METHOD;
+
 public class ConfigUtils {
 
     public static <T> T newInstance(Object bean, String propertyName, Class<T> clazz) {
@@ -375,6 +378,29 @@ public static MemberAccessor buildMemberAccessor(Member member, MemberAccessorTy
         }
     }
 
+    public static <C> MemberAccessor findPlanningIdMemberAccessor(Class<C> clazz) {
+        List<Member> memberList = getAllMembers(clazz, PlanningId.class);
+        if (memberList.isEmpty()) {
+            return null;
+        }
+        if (memberList.size() > 1) {
+            throw new IllegalArgumentException("The class (" + clazz
+                    + ") has " + memberList.size() + " members (" + memberList + ") with a "
+                    + PlanningId.class.getSimpleName() + " annotation.");
+        }
+        Member member = memberList.get(0);
+        MemberAccessor memberAccessor = buildMemberAccessor(member, FIELD_OR_READ_METHOD, PlanningId.class);
+        if (!Comparable.class.isAssignableFrom(memberAccessor.getType())) {
+            throw new IllegalArgumentException("The class (" + clazz
+                    + ") has a member (" + member + ") with a " + PlanningId.class.getSimpleName()
+                    + " annotation that returns a type (" + memberAccessor.getType()
+                    + ") that does not implement " + Comparable.class.getSimpleName() + ".\n"
+                    + "Maybe use an " + Integer.class.getSimpleName()
+                    + " or " + String.class.getSimpleName() + " type instead.");
+        }
+        return memberAccessor;
+    }
+
     public enum MemberAccessorType {
         FIELD_OR_READ_METHOD,
         FIELD_OR_GETTER_METHOD,

optaplanner-core/src/main/java/org/optaplanner/core/impl/domain/lookup/ClassAndPlanningIdComparator.java

@@ -0,0 +1,94 @@
+/*
+ * Copyright 2017 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.domain.lookup;
+
+import java.util.Comparator;
+import java.util.HashMap;
+import java.util.Map;
+
+import org.optaplanner.core.api.domain.lookup.PlanningId;
+import org.optaplanner.core.config.util.ConfigUtils;
+import org.optaplanner.core.impl.domain.common.accessor.MemberAccessor;
+
+public class ClassAndPlanningIdComparator implements Comparator<Object> {
+
+    private boolean failFastIfNoPlanningId;
+
+    public ClassAndPlanningIdComparator() {
+        this(true);
+    }
+
+    public ClassAndPlanningIdComparator(boolean failFastIfNoPlanningId) {
+        this.failFastIfNoPlanningId = failFastIfNoPlanningId;
+    }
+
+    private Map<Class, MemberAccessor> decisionCache = new HashMap<>();
+
+    @Override
+    public int compare(Object a, Object b) {
+        if (a == null) {
+            return b == null ? 0 : -1;
+        } else if (b == null) {
+            return 1;
+        }
+        Class<?> aClass = a.getClass();
+        Class<?> bClass = b.getClass();
+        if (aClass != bClass) {
+            return aClass.getName().compareTo(bClass.getName());
+        }
+        MemberAccessor aMemberAccessor = decisionCache.computeIfAbsent(aClass,
+                ConfigUtils::findPlanningIdMemberAccessor);
+        MemberAccessor bMemberAccessor = decisionCache.computeIfAbsent(bClass,
+                ConfigUtils::findPlanningIdMemberAccessor);
+        if (failFastIfNoPlanningId) {
+            if (aMemberAccessor == null) {
+                throw new IllegalArgumentException("The class (" + aClass
+                        + ") does not have a " + PlanningId.class.getSimpleName() + " annotation.\n"
+                        + "Maybe add the " + PlanningId.class.getSimpleName() + " annotation.");
+            }
+            if (bMemberAccessor == null) {
+                throw new IllegalArgumentException("The class (" + bClass
+                        + ") does not have a " + PlanningId.class.getSimpleName() + " annotation.\n"
+                        + "Maybe add the " + PlanningId.class.getSimpleName() + " annotation.");
+            }
+        } else {
+            if (aMemberAccessor == null) {
+                // Return 0 to keep original order
+                return bMemberAccessor == null ? 0 : -1;
+            } else if (bMemberAccessor == null) {
+                return 1;
+            }
+        }
+        Comparable aPlanningId = (Comparable) aMemberAccessor.executeGetter(a);
+        Comparable bPlanningId = (Comparable) bMemberAccessor.executeGetter(b);
+        if (aPlanningId == null) {
+            throw new IllegalArgumentException("The planningId (" + aPlanningId
+                    + ") of the member (" + aMemberAccessor + ") of the class (" + aClass
+                    + ") on object (" + a + ") must not be null.\n"
+                    + "Maybe initialize the planningId of the original object before solving..");
+        }
+        if (bPlanningId == null) {
+            throw new IllegalArgumentException("The planningId (" + bPlanningId
+                    + ") of the member (" + bMemberAccessor + ") of the class (" + bClass
+                    + ") on object (" + a + ") must not be null.\n"
+                    + "Maybe initialize the planningId of the original object before solving..");
+        }
+        // If a and b are different classes, this method would have already returned.
+        return aPlanningId.compareTo(bPlanningId);
+    }
+
+}

optaplanner-core/src/main/java/org/optaplanner/core/impl/domain/lookup/LookUpStrategyResolver.java

@@ -16,14 +16,12 @@
 
 package org.optaplanner.core.impl.domain.lookup;
 
-import java.lang.reflect.Member;
 import java.lang.reflect.Method;
 import java.math.BigDecimal;
 import java.math.BigInteger;
 import java.time.LocalDate;
 import java.time.LocalDateTime;
 import java.time.LocalTime;
-import java.util.List;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.ConcurrentMap;
 
@@ -33,8 +31,6 @@
 import org.optaplanner.core.config.util.ConfigUtils;
 import org.optaplanner.core.impl.domain.common.accessor.MemberAccessor;
 
-import static org.optaplanner.core.config.util.ConfigUtils.MemberAccessorType.FIELD_OR_READ_METHOD;
-
 /**
  * This class is thread-safe.
  */
@@ -72,13 +68,13 @@ public LookUpStrategy determineLookUpStrategy(Object object) {
         return decisionCache.computeIfAbsent(objectClass, key -> {
             switch (lookUpStrategyType) {
                 case PLANNING_ID_OR_NONE:
-                    MemberAccessor memberAccessor1 = findPlanningIdMemberAccessor(objectClass);
+                    MemberAccessor memberAccessor1 = ConfigUtils.findPlanningIdMemberAccessor(objectClass);
                     if (memberAccessor1 == null) {
                         return new NoneLookUpStrategy();
                     }
                     return new PlanningIdLookUpStrategy(memberAccessor1);
                 case PLANNING_ID_OR_FAIL_FAST:
-                    MemberAccessor memberAccessor2 = findPlanningIdMemberAccessor(objectClass);
+                    MemberAccessor memberAccessor2 = ConfigUtils.findPlanningIdMemberAccessor(objectClass);
                     if (memberAccessor2 == null) {
                         throw new IllegalArgumentException("The class (" + objectClass
                                 + ") does not have a " + PlanningId.class.getSimpleName() + " annotation,"
@@ -117,18 +113,4 @@ public LookUpStrategy determineLookUpStrategy(Object object) {
         });
     }
 
-    protected <C> MemberAccessor findPlanningIdMemberAccessor(Class<C> clazz) {
-        List<Member> memberList = ConfigUtils.getAllMembers(clazz, PlanningId.class);
-        if (memberList.isEmpty()) {
-            return null;
-        }
-        if (memberList.size() > 1) {
-            throw new IllegalArgumentException("The class (" + clazz
-                    + ") has " + memberList.size() + " members (" + memberList + ") with a "
-                    + PlanningId.class.getSimpleName() + " annotation.");
-        }
-        Member member = memberList.get(0);
-        return ConfigUtils.buildMemberAccessor(member, FIELD_OR_READ_METHOD, PlanningId.class);
-    }
-
 }

optaplanner-core/src/main/java/org/optaplanner/core/impl/score/director/AbstractScoreDirector.java

@@ -28,12 +28,11 @@
 
 import org.optaplanner.core.api.domain.solution.cloner.SolutionCloner;
 import org.optaplanner.core.api.score.Score;
-import org.optaplanner.core.api.score.buildin.hardsoft.HardSoftScore;
 import org.optaplanner.core.api.score.constraint.ConstraintMatch;
 import org.optaplanner.core.api.score.constraint.ConstraintMatchTotal;
-import org.optaplanner.core.api.score.constraint.Indictment;
 import org.optaplanner.core.impl.domain.entity.descriptor.EntityDescriptor;
 import org.optaplanner.core.impl.domain.lookup.LookUpManager;
+import org.optaplanner.core.impl.domain.lookup.ClassAndPlanningIdComparator;
 import org.optaplanner.core.impl.domain.solution.descriptor.SolutionDescriptor;
 import org.optaplanner.core.impl.domain.variable.descriptor.ShadowVariableDescriptor;
 import org.optaplanner.core.impl.domain.variable.descriptor.VariableDescriptor;
@@ -515,6 +514,19 @@ protected String buildScoreCorruptionAnalysis(ScoreDirector<Solution_> uncorrupt
         Collection<ConstraintMatchTotal> uncorruptedConstraintMatchTotals
                 = uncorruptedScoreDirector.getConstraintMatchTotals();
 
+        // The order of justificationLists for score rules that include accumulates isn't stable, so we make it stable.
+        ClassAndPlanningIdComparator comparator = new ClassAndPlanningIdComparator(false);
+        for (ConstraintMatchTotal constraintMatchTotal : corruptedConstraintMatchTotals) {
+            for (ConstraintMatch constraintMatch : constraintMatchTotal.getConstraintMatchSet()) {
+                constraintMatch.getJustificationList().sort(comparator);
+            }
+        }
+        for (ConstraintMatchTotal constraintMatchTotal : uncorruptedConstraintMatchTotals) {
+            for (ConstraintMatch constraintMatch : constraintMatchTotal.getConstraintMatchSet()) {
+                constraintMatch.getJustificationList().sort(comparator);
+            }
+        }
+
         Map<List<Object>, ConstraintMatch> corruptedMap = createConstraintMatchMap(corruptedConstraintMatchTotals);
         Map<List<Object>, ConstraintMatch> excessMap = new LinkedHashMap<>(corruptedMap);
         Map<List<Object>, ConstraintMatch> missingMap = createConstraintMatchMap(uncorruptedConstraintMatchTotals);