fix: NPE in the selectors

TimefoldAI · Sep 27, 2024 · b2720c0 · b2720c0
1 parent 1fef51c
commit b2720c0
Show file tree

Hide file tree

Showing 14 changed files with 50 additions and 35 deletions.
diff --git a/...fold/solver/core/impl/heuristic/selector/move/composite/CartesianProductMoveSelector.java b/...fold/solver/core/impl/heuristic/selector/move/composite/CartesianProductMoveSelector.java
@@ -1,5 +1,6 @@
 package ai.timefold.solver.core.impl.heuristic.selector.move.composite;
 
+import java.math.BigInteger;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Iterator;
@@ -51,7 +52,7 @@ public boolean isNeverEnding() {
 
     @Override
     public long getSize() {
-        long size = 0L;
+        BigInteger totalSize = BigInteger.valueOf(0L);
         for (MoveSelector<Solution_> moveSelector : childMoveSelectorList) {
             long childSize = moveSelector.getSize();
             if (childSize == 0L) {
@@ -60,15 +61,16 @@ public long getSize() {
                 }
                 // else ignore that child
             } else {
-                if (size == 0L) {
+                if (totalSize.longValue() == 0L) {
                     // There must be at least 1 non-empty child to change the size from 0
-                    size = childSize;
+                    totalSize = BigInteger.valueOf(childSize);
                 } else {
-                    size *= childSize;
+                    totalSize = totalSize.multiply(BigInteger.valueOf(childSize));
                 }
             }
         }
-        return size;
+        var size = totalSize.longValue();
+        return size < 0 ? Long.MAX_VALUE : size;
     }
 
     @Override

diff --git a/...ava/ai/timefold/solver/core/impl/heuristic/selector/move/composite/UnionMoveSelector.java b/...ava/ai/timefold/solver/core/impl/heuristic/selector/move/composite/UnionMoveSelector.java
@@ -1,5 +1,6 @@
 package ai.timefold.solver.core.impl.heuristic.selector.move.composite;
 
+import java.math.BigInteger;
 import java.util.Iterator;
 import java.util.List;
 import java.util.stream.Stream;
@@ -79,11 +80,12 @@ public boolean isNeverEnding() {
 
     @Override
     public long getSize() {
-        long size = 0L;
+        BigInteger totalSize = BigInteger.valueOf(0L);
         for (MoveSelector<Solution_> moveSelector : childMoveSelectorList) {
-            size += moveSelector.getSize();
+            totalSize = totalSize.add(BigInteger.valueOf(moveSelector.getSize()));
         }
-        return size;
+        var size = totalSize.longValue();
+        return size < 0 ? Long.MAX_VALUE : size;
     }
 
     @Override

diff --git a/...efold/solver/core/impl/heuristic/selector/move/decorator/AbstractCachingMoveSelector.java b/...efold/solver/core/impl/heuristic/selector/move/decorator/AbstractCachingMoveSelector.java
@@ -75,7 +75,7 @@ public boolean isCountable() {
 
     @Override
     public long getSize() {
-        return cachedMoveList.size();
+        return cachedMoveList != null ? cachedMoveList.size() : 0L;
     }
 
 }
diff --git a/.../timefold/solver/core/impl/heuristic/selector/move/decorator/ProbabilityMoveSelector.java b/.../timefold/solver/core/impl/heuristic/selector/move/decorator/ProbabilityMoveSelector.java
@@ -83,7 +83,7 @@ public boolean isNeverEnding() {
 
     @Override
     public long getSize() {
-        return cachedMoveMap.size();
+        return cachedMoveMap != null ? cachedMoveMap.size() : 0L;
     }
 
     @Override

diff --git a/...solver/core/impl/heuristic/selector/move/factory/MoveListFactoryToMoveSelectorBridge.java b/...solver/core/impl/heuristic/selector/move/factory/MoveListFactoryToMoveSelectorBridge.java
@@ -76,7 +76,7 @@ public boolean isNeverEnding() {
 
     @Override
     public long getSize() {
-        return cachedMoveList.size();
+        return cachedMoveList != null ? cachedMoveList.size() : 0L;
     }
 
     @Override

diff --git a/...i/timefold/solver/core/impl/heuristic/selector/move/generic/RuinRecreateMoveSelector.java b/...i/timefold/solver/core/impl/heuristic/selector/move/generic/RuinRecreateMoveSelector.java
@@ -1,13 +1,13 @@
 package ai.timefold.solver.core.impl.heuristic.selector.move.generic;
 
+import java.math.BigInteger;
 import java.util.Iterator;
 
 import ai.timefold.solver.core.impl.domain.variable.descriptor.GenuineVariableDescriptor;
 import ai.timefold.solver.core.impl.heuristic.move.Move;
 import ai.timefold.solver.core.impl.heuristic.selector.entity.EntitySelector;
 import ai.timefold.solver.core.impl.solver.scope.SolverScope;
-
-import org.apache.commons.math3.util.CombinatoricsUtils;
+import ai.timefold.solver.core.impl.util.MathUtils;
 
 final class RuinRecreateMoveSelector<Solution_> extends GenericMoveSelector<Solution_> {
 
@@ -35,15 +35,16 @@ public RuinRecreateMoveSelector(EntitySelector<Solution_> entitySelector,
 
     @Override
     public long getSize() {
-        var totalSize = 0L;
+        var totalSize = BigInteger.valueOf(0L);
         var entityCount = entitySelector.getSize();
         var minimumSelectedCount = minimumSelectedCountSupplier.applyAsInt(entityCount);
         var maximumSelectedCount = maximumSelectedCountSupplier.applyAsInt(entityCount);
         for (int selectedCount = minimumSelectedCount; selectedCount <= maximumSelectedCount; selectedCount++) {
             // Order is significant, and each entity can only be picked once
-            totalSize += CombinatoricsUtils.factorial((int) entityCount) / CombinatoricsUtils.factorial(selectedCount);
+            totalSize = totalSize.add(BigInteger.valueOf(MathUtils.factorial((int) entityCount) / MathUtils.factorial(selectedCount)));
         }
-        return totalSize;
+        var size = totalSize.longValue();
+        return size < 0 ? Long.MAX_VALUE : size;
     }
 
     @Override

diff --git a/...fold/solver/core/impl/heuristic/selector/move/generic/list/kopt/KOptListMoveSelector.java b/...fold/solver/core/impl/heuristic/selector/move/generic/list/kopt/KOptListMoveSelector.java
@@ -2,6 +2,7 @@
 
 import static ai.timefold.solver.core.impl.heuristic.selector.move.generic.list.ListChangeMoveSelector.filterPinnedListPlanningVariableValuesWithIndex;
 
+import java.math.BigInteger;
 import java.util.Iterator;
 import java.util.Objects;
 import java.util.function.Supplier;
@@ -71,7 +72,7 @@ public void solvingEnded(SolverScope<Solution_> solverScope) {
 
     @Override
     public long getSize() {
-        long total = 0;
+        BigInteger totalSize = BigInteger.valueOf(0L);
         long valueSelectorSize = valueSelector.getSize();
         for (int i = minK; i < Math.min(valueSelectorSize, maxK); i++) {
             if (valueSelectorSize > i) { // need more than k nodes in order to perform a k-opt
@@ -85,10 +86,11 @@ public long getSize() {
                 } else {
                     edgeChoices = Long.MAX_VALUE;
                 }
-                total += kOptMoveTypes * edgeChoices;
+                totalSize = totalSize.add(BigInteger.valueOf(kOptMoveTypes).multiply(BigInteger.valueOf(edgeChoices)));
             }
         }
-        return total;
+        var size = totalSize.longValue();
+        return size < 0 ? Long.MAX_VALUE : size;
     }
 
     @Override

diff --git a/...ver/core/impl/heuristic/selector/move/generic/list/ruin/ListRuinRecreateMoveSelector.java b/...ver/core/impl/heuristic/selector/move/generic/list/ruin/ListRuinRecreateMoveSelector.java
@@ -1,5 +1,6 @@
 package ai.timefold.solver.core.impl.heuristic.selector.move.generic.list.ruin;
 
+import java.math.BigInteger;
 import java.util.Iterator;
 import java.util.Objects;
 
@@ -12,8 +13,7 @@
 import ai.timefold.solver.core.impl.heuristic.selector.value.EntityIndependentValueSelector;
 import ai.timefold.solver.core.impl.heuristic.selector.value.decorator.FilteringValueSelector;
 import ai.timefold.solver.core.impl.solver.scope.SolverScope;
-
-import org.apache.commons.math3.util.CombinatoricsUtils;
+import ai.timefold.solver.core.impl.util.MathUtils;
 
 final class ListRuinRecreateMoveSelector<Solution_> extends GenericMoveSelector<Solution_> {
 
@@ -47,15 +47,17 @@ private ListVariableStateSupply<Solution_> getListVariableStateSupply() {
 
     @Override
     public long getSize() {
-        var totalSize = 0L;
+        var totalSize = BigInteger.valueOf(0L);
         var valueCount = valueSelector.getSize();
         var minimumSelectedCount = minimumSelectedCountSupplier.applyAsInt(valueCount);
         var maximumSelectedCount = maximumSelectedCountSupplier.applyAsInt(valueCount);
         for (var selectedCount = minimumSelectedCount; selectedCount <= maximumSelectedCount; selectedCount++) {
             // Order is significant, and each entity can only be picked once
-            totalSize += CombinatoricsUtils.factorial((int) valueCount) / CombinatoricsUtils.factorial(selectedCount);
+            totalSize = totalSize
+                    .add(BigInteger.valueOf(MathUtils.factorial((int) valueCount) / MathUtils.factorial(selectedCount)));
         }
-        return totalSize;
+        var size = totalSize.longValue();
+        return size < 0 ? Long.MAX_VALUE : size;
     }
 
     @Override

diff --git a/...old/solver/core/impl/heuristic/selector/value/decorator/AbstractCachingValueSelector.java b/...old/solver/core/impl/heuristic/selector/value/decorator/AbstractCachingValueSelector.java
@@ -90,7 +90,7 @@ public long getSize(Object entity) {
     }
 
     public long getSize() {
-        return cachedValueList.size();
+        return cachedValueList != null ? cachedValueList.size() : 0L;
     }
 
     @Override

diff --git a/...imefold/solver/core/impl/heuristic/selector/value/decorator/ProbabilityValueSelector.java b/...imefold/solver/core/impl/heuristic/selector/value/decorator/ProbabilityValueSelector.java
@@ -96,7 +96,7 @@ public long getSize(Object entity) {
 
     @Override
     public long getSize() {
-        return cachedEntityMap.size();
+        return cachedEntityMap != null ? cachedEntityMap.size() : 0L;
     }
 
     @Override

diff --git a/core/src/main/java/ai/timefold/solver/core/impl/util/MathUtils.java b/core/src/main/java/ai/timefold/solver/core/impl/util/MathUtils.java
@@ -53,4 +53,9 @@ public static long getSpeed(long count, long timeMillisSpent) {
         // Avoid divide by zero exception on a fast CPU
         return count * 1000L / (timeMillisSpent == 0L ? 1L : timeMillisSpent);
     }
+
+    public static long factorial(int n) {
+        // The largest value of n for which n! does not exceed Long.MAX_VALUE is 20.
+        return n <= 20 ? CombinatoricsUtils.factorial(n) : Long.MAX_VALUE;
+    }
 }
diff --git a/...imefold/solver/core/impl/heuristic/selector/move/decorator/FilteringMoveSelectorTest.java b/...imefold/solver/core/impl/heuristic/selector/move/decorator/FilteringMoveSelectorTest.java
@@ -23,25 +23,25 @@ class FilteringMoveSelectorTest {
 
     @Test
     void filterCacheTypeSolver() {
-        filter(SelectionCacheType.SOLVER, 1);
+        filter(SelectionCacheType.SOLVER, 1, 3);
     }
 
     @Test
     void filterCacheTypePhase() {
-        filter(SelectionCacheType.PHASE, 2);
+        filter(SelectionCacheType.PHASE, 2, 4);
     }
 
     @Test
     void filterCacheTypeStep() {
-        filter(SelectionCacheType.STEP, 5);
+        filter(SelectionCacheType.STEP, 5, 7);
     }
 
     @Test
     void filterCacheTypeJustInTime() {
-        filter(SelectionCacheType.JUST_IN_TIME, 5);
+        filter(SelectionCacheType.JUST_IN_TIME, 5, 7);
     }
 
-    public void filter(SelectionCacheType cacheType, int timesCalled) {
+    public void filter(SelectionCacheType cacheType, int iteratorTimesCalled, int sizeTimesCalled) {
         MoveSelector childMoveSelector = SelectorTestUtils.mockMoveSelector(DummyMove.class,
                 new DummyMove("a1"), new DummyMove("a2"), new DummyMove("a3"), new DummyMove("a4"));
 
@@ -99,8 +99,8 @@ public void filter(SelectionCacheType cacheType, int timesCalled) {
         moveSelector.solvingEnded(solverScope);
 
         verifyPhaseLifecycle(childMoveSelector, 1, 2, 5);
-        verify(childMoveSelector, times(timesCalled)).iterator();
-        verify(childMoveSelector, times(timesCalled)).getSize();
+        verify(childMoveSelector, times(iteratorTimesCalled)).iterator();
+        verify(childMoveSelector, times(sizeTimesCalled)).getSize();
     }
 
 }
diff --git a/...olver/core/impl/heuristic/selector/move/decorator/SelectedCountLimitMoveSelectorTest.java b/...olver/core/impl/heuristic/selector/move/decorator/SelectedCountLimitMoveSelectorTest.java
@@ -73,7 +73,7 @@ void selectSizeLimitLowerThanSelectorSize() {
 
         verifyPhaseLifecycle(childMoveSelector, 1, 2, 5);
         verify(childMoveSelector, times(5)).iterator();
-        verify(childMoveSelector, times(5)).getSize();
+        verify(childMoveSelector, times(7)).getSize();
     }
 
     @Test
@@ -131,7 +131,7 @@ void selectSizeLimitHigherThanSelectorSize() {
 
         verifyPhaseLifecycle(childMoveSelector, 1, 2, 5);
         verify(childMoveSelector, times(5)).iterator();
-        verify(childMoveSelector, times(5)).getSize();
+        verify(childMoveSelector, times(7)).getSize();
     }
 
 }
diff --git a/...ver/core/impl/localsearch/decider/acceptor/lateacceptance/LateAcceptanceAcceptorTest.java b/...ver/core/impl/localsearch/decider/acceptor/lateacceptance/LateAcceptanceAcceptorTest.java
@@ -296,6 +296,7 @@ void applyReconfiguration() {
         assertThat(acceptor.isAccepted(moveScope1)).isTrue();
         acceptor.stepEnded(stepScope0);
         var moveScope2 = buildMoveScope(stepScope0, -4000);
+        acceptor.stepStarted(stepScope0);
         assertThat(acceptor.needReconfiguration(stepScope0)).isFalse();
         assertThat(acceptor.isAccepted(moveScope2)).isFalse();
     }