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 = SolverStatus.SOLVING_SCHEDULED;
     private @Nullable Future<Solution_> finalBestSolutionFuture;
     private @Nullable ConsumerSupport<Solution_, Object> consumerSupport;
 
@@ -88,9 +88,8 @@ 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();
     }
 
     public void setFinalBestSolutionFuture(Future<Solution_> finalBestSolutionFuture) {
@@ -268,7 +267,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 +277,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 +297,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.

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

@@ -66,7 +66,7 @@ private void validateSolverFactory() {
         solverFactory.buildSolver();
     }
 
-    private Object getProblemIdOrThrow(Object problemId) {
+    private static Object getProblemIdOrThrow(Object problemId) {
         return Objects.requireNonNull(problemId, "Invalid problemId (null) given to SolverManager.");
     }
 

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.

docs/src/modules/ROOT/pages/integration/integration.adoc

@@ -177,8 +177,9 @@ Add a dependency to the `timefold-solver-jackson` jar and register `TimefoldJack
 
 [source,java,options="nowrap"]
 ----
-ObjectMapper objectMapper = new ObjectMapper();
-objectMapper.registerModule(TimefoldJacksonModule.createModule());
+var objectMapper = JsonMapper.builder()
+            .addModule(TimefoldJacksonModule.createModule())
+            .build();
 ----
 
 

docs/src/modules/ROOT/pages/quickstart/quarkus/quarkus-quickstart.adoc

@@ -315,9 +315,13 @@ class TimetableResourceTest {
     @Test
     void solveDemoDataUntilFeasible() throws IOException {
         // Provide the same input data shown in the text above.
-        Timetable testTimetable = new ObjectMapper()
-                .findAndRegisterModules()
-                .readValue(new File("src/test/resources/testing-timetable.json"), Timetable.class);
+        ObjectMapper objectMapper = JsonMapper.builder()
+            .findAndAddModules()
+            .build();
+        Timetable testTimetable = objectMapper
+.readValue(
+            new File("src/test/resources/testing-timetable.json"),
+            Timetable.class);
 
         Timetable solution = given()
                 .contentType(ContentType.JSON)

docs/src/modules/ROOT/pages/using-timefold-solver/benchmarking-and-tweaking.adoc

@@ -219,10 +219,9 @@ public class VehicleRoutePlanJsonSolutionFileIO extends JacksonSolutionFileIO<Ve
     public VehicleRoutePlanJsonSolutionFileIO() {
         // VehicleRoutePlan is the @PlanningSolution class.
         super(VehicleRoutePlan.class,
-                new ObjectMapper()
-                        .registerModule(new JavaTimeModule())
-                        .disable(SerializationFeature.WRITE_DATES_AS_TIMESTAMPS)
-        );
+                new JsonMapper.builder()
+                    ...
+                    .build());
     }
 
 }

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;