chore: address review comments

Author: triceo

core/src/main/java/ai/timefold/solver/core/api/score/constraint/ConstraintRef.java

@@ -17,22 +17,12 @@ public record ConstraintRef(String constraintName)
         implements
             Comparable<ConstraintRef> {
 
-    private static final char PACKAGE_SEPARATOR = '/';
-
     public static ConstraintRef of(String constraintName) {
         return new ConstraintRef(constraintName);
     }
 
     public ConstraintRef {
-        var sanitized = AbstractConstraintBuilder.sanitize("constraintName", constraintName);
-        if (sanitized.isEmpty()) {
-            throw new IllegalArgumentException("The %s cannot be empty."
-                    .formatted("constraint name"));
-        } else if (sanitized.contains("" + PACKAGE_SEPARATOR)) {
-            throw new IllegalArgumentException("The %s (%s) cannot contain a package separator (%s)."
-                    .formatted("constraint name", sanitized, PACKAGE_SEPARATOR));
-        }
-        constraintName = sanitized;
+        constraintName = AbstractConstraintBuilder.sanitize("constraintName", constraintName);
     }
 
     @Override

core/src/main/java/ai/timefold/solver/core/api/score/stream/ConstraintBuilder.java

@@ -22,6 +22,7 @@ default Constraint asConstraint(String constraintName) {
      * placing the constraint in the {@link Constraint#DEFAULT_CONSTRAINT_GROUP default constraint group}.
      *
      * @param constraintName shows up in {@link ConstraintMatchTotal} during score justification
+     * @param constraintDescription can contain any character, but it is recommended to keep it short and concise.
      */
     default Constraint asConstraintDescribed(String constraintName, String constraintDescription) {
         return asConstraintDescribed(constraintName, constraintDescription, Constraint.DEFAULT_CONSTRAINT_GROUP);
@@ -38,6 +39,7 @@ default Constraint asConstraintDescribed(String constraintName, String constrain
      * and therefore doesn't need to be URL-friendly, or protected against injection attacks.
      *
      * @param constraintName shows up in {@link ConstraintMatchTotal} during score justification
+     * @param constraintDescription can contain any character, but it is recommended to keep it short and concise.
      * @param constraintGroup not used by the solver directly, but may be used by external tools to group constraints together,
      *        such as by their source or by their purpose
      */

core/src/main/java/ai/timefold/solver/core/impl/score/stream/common/AbstractConstraintBuilder.java

@@ -38,7 +38,7 @@ public final Constraint asConstraintDescribed(String constraintName, String cons
 
     public static String sanitize(String fieldName, String fieldValue) {
         if (fieldValue == null || fieldValue.equalsIgnoreCase("null") || fieldValue.equalsIgnoreCase("nil")) {
-            throw new IllegalArgumentException("The %s cannot be null.".formatted(fieldName));
+            throw new IllegalArgumentException("The %s (%s) cannot be null.".formatted(fieldName, fieldValue));
         }
         if (!fieldValue.matches("^[a-zA-Z0-9]+[a-zA-Z0-9 _.'-]*$")) {
             throw new IllegalArgumentException(

core/src/main/java/ai/timefold/solver/core/impl/solver/DefaultSolverJob.java

@@ -58,13 +58,13 @@ public final class DefaultSolverJob<Solution_> implements SolverJob<Solution_>,
     private final @Nullable Consumer<SolverJobStartedEvent<Solution_>> solverJobStartedConsumer;
     private final BiConsumer<? super Object, ? super Throwable> exceptionHandler;
 
-    private volatile SolverStatus solverStatus;
     private final CountDownLatch terminatedLatch;
     private final ReentrantLock solverStatusModifyingLock;
     private final AtomicBoolean terminatedEarly = new AtomicBoolean(false);
     private final BestSolutionHolder<Solution_> bestSolutionHolder = new BestSolutionHolder<>();
     private final AtomicReference<@Nullable ProblemSizeStatistics> temporaryProblemSizeStatistics = new AtomicReference<>();
 
+    private volatile SolverStatus solverStatus;
     private @Nullable Future<Solution_> finalBestSolutionFuture;
     private @Nullable ConsumerSupport<Solution_, Object> consumerSupport;
 
@@ -88,9 +88,9 @@ public DefaultSolverJob(DefaultSolverManager<Solution_> solverManager, Solver<So
         this.firstInitializedSolutionConsumer = firstInitializedSolutionConsumer;
         this.solverJobStartedConsumer = solverJobStartedConsumer;
         this.exceptionHandler = exceptionHandler;
-        solverStatus = SolverStatus.SOLVING_SCHEDULED;
-        terminatedLatch = new CountDownLatch(1);
-        solverStatusModifyingLock = new ReentrantLock();
+        this.terminatedLatch = new CountDownLatch(1);
+        this.solverStatusModifyingLock = new ReentrantLock();
+        this.solverStatus = SolverStatus.SOLVING_SCHEDULED;
     }
 
     public void setFinalBestSolutionFuture(Future<Solution_> finalBestSolutionFuture) {
@@ -268,7 +268,7 @@ public ProblemSizeStatistics getProblemSizeStatistics() {
         // before the solving has started.
         // Once the solving has started, the problem size statistics will be computed
         // using the ScoreDirector's hot ValueRangeManager.
-        return temporaryProblemSizeStatistics.updateAndGet(oldStatistics -> {
+        var result = temporaryProblemSizeStatistics.updateAndGet(oldStatistics -> {
             if (oldStatistics != null) {
                 // If the problem size statistics were already computed, return them.
                 // This can happen if the problem size statistics were computed before the solving started.
@@ -278,6 +278,11 @@ public ProblemSizeStatistics getProblemSizeStatistics() {
             var valueManager = ValueRangeManager.of(solutionDescriptor, problemFinder.apply(problemId));
             return valueManager.getProblemSizeStatistics();
         });
+        // Avoids nullness issues reported by IDE which cannot actually happen.
+        // The result can never be null, because none of the methods called in the lambda can return null,
+        // and the lambda is the only way to set the value of the temporaryProblemSizeStatistics,
+        // which is the only way for it to be null.
+        return Objects.requireNonNull(result);
     }
 
     public SolverTermination<Solution_> getSolverTermination() {
@@ -293,21 +298,33 @@ void close() {
 
     /**
      * A listener that unlocks the solverStatusModifyingLock when Solving has started.
-     *
      * It prevents the following scenario caused by unlocking before Solving started:
      *
-     * Thread 1:
-     * solverStatusModifyingLock.unlock()
-     * >solver.solve(...) // executes second
-     *
-     * Thread 2:
-     * case SOLVING_ACTIVE:
-     * >solver.terminateEarly(); // executes first
+     * <dl>
+     * <dt>Thread 1</dt>
+     * <dd>
+     * 
+     * <pre>
+     *           solverStatusModifyingLock.unlock()
+     *           > solver.solve(...) // executes second
+     * </pre>
+     * 
+     * </dd>
+     * <dt>Thread 2</dt>
+     * <dd>
+     * 
+     * <pre>
+     *           case SOLVING_ACTIVE:
+     *           >solver.terminateEarly(); // executes first
+     * </pre>
+     * 
+     * </dd>
+     * </dl>
      *
      * The solver.solve() call resets the terminateEarly flag, and thus the solver will not be terminated
      * by the call, which means terminatedLatch will not be decremented, causing Thread 2 to wait forever
      * (at least until another Thread calls terminateEarly again).
-     *
+     * <p>
      * To prevent Thread 2 from potentially waiting forever, we only unlock the lock after the
      * solvingStarted phase lifecycle event is fired, meaning the terminateEarly flag will not be
      * reset and thus the solver will actually terminate.

docs/TODO.md

@@ -38,5 +38,6 @@
 - [ ] `ProblemChangeDirector#lookUpWorkingObject()` no longer returns `Optional`.
 - [ ] `AutoDiscoverMemberType` is gone
 - [ ] Constraint name and group now forces strict validation.
+- [ ] `SolverManager` and `SolverJob` no longer have the `ProblemId_` type parameter.
 
 Remove this file when done.

persistence/jackson/src/test/java/ai/timefold/solver/jackson/api/AbstractJacksonRoundTripTest.java

@@ -9,10 +9,6 @@ public abstract class AbstractJacksonRoundTripTest {
     // Helper methods
     // ************************************************************************
 
-    protected static <W> W serializeAndDeserialize(W input) {
-        return serializeAndDeserialize(new ObjectMapper(), input);
-    }
-
     protected static <W> W serializeAndDeserialize(ObjectMapper objectMapper, W input) {
         String jsonString;
         W output;

persistence/jackson/src/test/java/ai/timefold/solver/jackson/api/score/AbstractScoreJacksonRoundTripTest.java

@@ -31,11 +31,11 @@ public abstract class AbstractScoreJacksonRoundTripTest extends AbstractJacksonR
         if (expectedScore != null) {
             regex = "\\{\\s*" // Start of element
                     + "\"score\":\""
-                    + expectedScore.toString().replaceAll("\\[", "\\\\[").replaceAll("\\]", "\\\\]") // Score
+                    + expectedScore.toString().replaceAll("\\[", "\\\\[").replaceAll("]", "\\\\]") // Score
                     + "\""
-                    + "\\s*\\}"; // End of element
+                    + "\\s*}"; // End of element
         } else {
-            regex = "\\{\"score\":null\\}"; // Start and end of element
+            regex = "\\{\"score\":null}"; // Start and end of element
         }
         if (!jsonString.matches(regex)) {
             fail("Regular expression match failed.\nExpected regular expression: " + regex + "\nActual string: " + jsonString);

quarkus-integration/quarkus-jackson/runtime/src/test/java/ai/timefold/solver/quarkus/jackson/AbstractJacksonRoundTripTest.java

@@ -10,10 +10,6 @@ public abstract class AbstractJacksonRoundTripTest {
     // Helper methods
     // ************************************************************************
 
-    protected static <W> W serializeAndDeserialize(W input) {
-        return serializeAndDeserialize(new ObjectMapper(), input);
-    }
-
     protected static <W> W serializeAndDeserialize(ObjectMapper objectMapper, W input) {
         String jsonString;
         W output;

quarkus-integration/quarkus-jackson/runtime/src/test/java/ai/timefold/solver/quarkus/jackson/score/AbstractScoreJacksonRoundTripTest.java

@@ -32,11 +32,11 @@ public abstract class AbstractScoreJacksonRoundTripTest extends AbstractJacksonR
         if (expectedScore != null) {
             regex = "\\{\\s*" // Start of element
                     + "\"score\":\""
-                    + expectedScore.toString().replaceAll("\\[", "\\\\[").replaceAll("\\]", "\\\\]") // Score
+                    + expectedScore.toString().replaceAll("\\[", "\\\\[").replaceAll("]", "\\\\]") // Score
                     + "\""
-                    + "\\s*\\}"; // End of element
+                    + "\\s*}"; // End of element
         } else {
-            regex = "\\{\"score\":null\\}"; // Start and end of element
+            regex = "\\{\"score\":null}"; // Start and end of element
         }
         if (!jsonString.matches(regex)) {
             fail("Regular expression match failed.\nExpected regular expression: " + regex + "\nActual string: " + jsonString);

quarkus-integration/quarkus/runtime/src/main/java/ai/timefold/solver/quarkus/bean/UnavailableTimefoldBeanProvider.java

@@ -35,7 +35,7 @@ <Solution_> SolverFactory<Solution_> solverFactory() {
     @DefaultBean
     @Dependent
     @Produces
-    <Solution_, ProblemId_> SolverManager<Solution_> solverManager() {
+    <Solution_> SolverManager<Solution_> solverManager() {
         throw createException(SolverManager.class);
     }