From 28201efe6facc29ca7ee60b4490d59a77b3dd9f2 Mon Sep 17 00:00:00 2001 From: Christopher Chianelli Date: Fri, 29 Nov 2024 21:55:36 +0100 Subject: [PATCH] fix: Recurse through @DeepPlanningCloned classes to discover all @DeepPlanningCloned classes (#1237) The `GizmoSolutionClonerImplementor` requires a set of all `@DeepPlanningCloned` classes so it can create cloners for all of them. `GizmoCloningUtils.getDeepClonedClasses` incorrectly assume all `@DeepPlanningCloned classes` are directly accessible from either the `@PlanningSolution` class or an `@PlanningEntity` class. However, a `@DeepPlanningCloned` class might be referenced from another `@DeepPlanningCloned` class, which would not be detected. In order to detect all `@DeepPlanningCloned` classes, when a class that is `@DeepPlanningCloned` is found and was not processed yet, it is added to the to process queue. Fixes #1208. --- .../cloner/gizmo/GizmoCloningUtils.java | 26 ++++++++++++-- .../gizmo/GizmoSolutionClonerImplementor.java | 9 +++-- .../cloner/AbstractSolutionClonerTest.java | 2 ++ .../cloner/gizmo/GizmoCloningUtilsTest.java | 34 +++++++++++++++++++ .../deepcloning/ExtraDeepClonedObject.java | 15 ++++++++ .../deepcloning/TestdataVariousTypes.java | 3 ++ .../solver/quarkus/deployment/DotNames.java | 4 +++ 7 files changed, 88 insertions(+), 5 deletions(-) create mode 100644 core/src/test/java/ai/timefold/solver/core/impl/domain/solution/cloner/gizmo/GizmoCloningUtilsTest.java create mode 100644 core/src/test/java/ai/timefold/solver/core/impl/testdata/domain/clone/deepcloning/ExtraDeepClonedObject.java diff --git a/core/src/main/java/ai/timefold/solver/core/impl/domain/solution/cloner/gizmo/GizmoCloningUtils.java b/core/src/main/java/ai/timefold/solver/core/impl/domain/solution/cloner/gizmo/GizmoCloningUtils.java index 7eae67f93c..7bfe3d916f 100644 --- a/core/src/main/java/ai/timefold/solver/core/impl/domain/solution/cloner/gizmo/GizmoCloningUtils.java +++ b/core/src/main/java/ai/timefold/solver/core/impl/domain/solution/cloner/gizmo/GizmoCloningUtils.java @@ -24,12 +24,32 @@ public static Set> getDeepClonedClasses(SolutionDescriptor solutionD Set> classesToProcess = new LinkedHashSet<>(solutionDescriptor.getEntityClassSet()); classesToProcess.add(solutionDescriptor.getSolutionClass()); classesToProcess.addAll(entitySubclasses); - for (Class clazz : classesToProcess) { + + // deepClonedClassSet contains all processed classes so far, + // so we can use it to determine if we need to add a class + // to the classesToProcess set. + // + // This is important, since SolverConfig does not contain + // info on @DeepPlanningCloned classes, so we need to discover + // them from the domain classes, possibly recursively + // (when a @DeepPlanningCloned class reference another @DeepPlanningCloned + // that is not referenced by any entity). + while (!classesToProcess.isEmpty()) { + var clazz = classesToProcess.iterator().next(); + classesToProcess.remove(clazz); deepClonedClassSet.add(clazz); for (Field field : getAllFields(clazz)) { - deepClonedClassSet.addAll(getDeepClonedTypeArguments(solutionDescriptor, field.getGenericType())); + var deepClonedTypeArguments = getDeepClonedTypeArguments(solutionDescriptor, field.getGenericType()); + for (var deepClonedTypeArgument : deepClonedTypeArguments) { + if (!deepClonedClassSet.contains(deepClonedTypeArgument)) { + classesToProcess.add(deepClonedTypeArgument); + deepClonedClassSet.add(deepClonedTypeArgument); + } + } if (DeepCloningUtils.isFieldDeepCloned(solutionDescriptor, field, clazz) - && !PlanningCloneable.class.isAssignableFrom(field.getType())) { + && !PlanningCloneable.class.isAssignableFrom(field.getType()) + && !deepClonedClassSet.contains(field.getType())) { + classesToProcess.add(field.getType()); deepClonedClassSet.add(field.getType()); } } diff --git a/core/src/main/java/ai/timefold/solver/core/impl/domain/solution/cloner/gizmo/GizmoSolutionClonerImplementor.java b/core/src/main/java/ai/timefold/solver/core/impl/domain/solution/cloner/gizmo/GizmoSolutionClonerImplementor.java index dabb0e5f1d..c2a13eb164 100644 --- a/core/src/main/java/ai/timefold/solver/core/impl/domain/solution/cloner/gizmo/GizmoSolutionClonerImplementor.java +++ b/core/src/main/java/ai/timefold/solver/core/impl/domain/solution/cloner/gizmo/GizmoSolutionClonerImplementor.java @@ -25,6 +25,7 @@ import java.util.SortedSet; import java.util.TreeSet; import java.util.concurrent.ConcurrentHashMap; +import java.util.function.Predicate; import java.util.function.Supplier; import java.util.stream.Collectors; @@ -121,6 +122,10 @@ public static void defineClonerFor(ClassCreator classCreator, memoizedSolutionOrEntityDescriptorMap, deepClonedClassSet); } + public static boolean isCloneableClass(Class clazz) { + return !clazz.isInterface() && !Modifier.isAbstract(clazz.getModifiers()); + } + /** * Generates the constructor and implementations of SolutionCloner * methods for the given SolutionDescriptor using the given ClassCreator @@ -139,7 +144,7 @@ public static void defineClonerFor(Supplier impl Set> deepCloneClassesThatAreNotSolutionSet = deepClonedClassSet.stream() .filter(clazz -> !solutionClassSet.contains(clazz) && !clazz.isArray()) - .filter(clazz -> !clazz.isInterface() && !Modifier.isAbstract(clazz.getModifiers())) + .filter(GizmoSolutionClonerImplementor::isCloneableClass) .collect(Collectors.toSet()); Comparator> instanceOfComparator = getInstanceOfComparator(deepClonedClassSet); @@ -163,7 +168,7 @@ public static void defineClonerFor(Supplier impl Set> abstractDeepCloneClassSet = deepClonedClassSet.stream() .filter(clazz -> !solutionClassSet.contains(clazz) && !clazz.isArray()) - .filter(clazz -> clazz.isInterface() || Modifier.isAbstract(clazz.getModifiers())) + .filter(Predicate.not(GizmoSolutionClonerImplementor::isCloneableClass)) .collect(Collectors.toSet()); for (Class abstractDeepClonedClass : abstractDeepCloneClassSet) { diff --git a/core/src/test/java/ai/timefold/solver/core/impl/domain/solution/cloner/AbstractSolutionClonerTest.java b/core/src/test/java/ai/timefold/solver/core/impl/domain/solution/cloner/AbstractSolutionClonerTest.java index 2082729f5e..4ab4522423 100644 --- a/core/src/test/java/ai/timefold/solver/core/impl/domain/solution/cloner/AbstractSolutionClonerTest.java +++ b/core/src/test/java/ai/timefold/solver/core/impl/domain/solution/cloner/AbstractSolutionClonerTest.java @@ -902,6 +902,8 @@ private void assertTestdataVariousTypes(TestdataVariousTypes original, TestdataV softly.assertThat(cloned.deepClonedListRef) .first() .isSameAs(original.deepClonedListRef.get(0)); + softly.assertThat(cloned.extraDeepClonedObject).isNotSameAs(original.extraDeepClonedObject); + softly.assertThat(cloned.extraDeepClonedObject.id).isSameAs(original.extraDeepClonedObject.id); }); assertSoftly(softly -> { softly.assertThat(cloned.shallowClonedListRef).isSameAs(original.shallowClonedListRef); diff --git a/core/src/test/java/ai/timefold/solver/core/impl/domain/solution/cloner/gizmo/GizmoCloningUtilsTest.java b/core/src/test/java/ai/timefold/solver/core/impl/domain/solution/cloner/gizmo/GizmoCloningUtilsTest.java new file mode 100644 index 0000000000..b111c40610 --- /dev/null +++ b/core/src/test/java/ai/timefold/solver/core/impl/domain/solution/cloner/gizmo/GizmoCloningUtilsTest.java @@ -0,0 +1,34 @@ +package ai.timefold.solver.core.impl.domain.solution.cloner.gizmo; + +import static org.assertj.core.api.Assertions.assertThat; + +import java.util.List; +import java.util.Map; + +import ai.timefold.solver.core.impl.testdata.domain.clone.deepcloning.AnnotatedTestdataVariousTypes; +import ai.timefold.solver.core.impl.testdata.domain.clone.deepcloning.ExtraDeepClonedObject; +import ai.timefold.solver.core.impl.testdata.domain.clone.deepcloning.TestdataDeepCloningEntity; +import ai.timefold.solver.core.impl.testdata.domain.clone.deepcloning.TestdataVariousTypes; +import ai.timefold.solver.core.impl.testdata.domain.clone.deepcloning.field.TestdataFieldAnnotatedDeepCloningEntity; +import ai.timefold.solver.core.impl.testdata.domain.clone.deepcloning.field.TestdataFieldAnnotatedDeepCloningSolution; + +import org.junit.jupiter.api.Test; + +class GizmoCloningUtilsTest { + + @Test + void getDeepClonedClasses() { + assertThat(GizmoCloningUtils.getDeepClonedClasses( + TestdataFieldAnnotatedDeepCloningSolution.buildSolutionDescriptor(), + List.of(TestdataDeepCloningEntity.class))) + .containsExactlyInAnyOrder( + TestdataDeepCloningEntity.class, + ExtraDeepClonedObject.class, + TestdataFieldAnnotatedDeepCloningEntity.class, + AnnotatedTestdataVariousTypes.class, + TestdataFieldAnnotatedDeepCloningSolution.class, + TestdataVariousTypes.class, + List.class, + Map.class); + } +} \ No newline at end of file diff --git a/core/src/test/java/ai/timefold/solver/core/impl/testdata/domain/clone/deepcloning/ExtraDeepClonedObject.java b/core/src/test/java/ai/timefold/solver/core/impl/testdata/domain/clone/deepcloning/ExtraDeepClonedObject.java new file mode 100644 index 0000000000..a86a2e0a20 --- /dev/null +++ b/core/src/test/java/ai/timefold/solver/core/impl/testdata/domain/clone/deepcloning/ExtraDeepClonedObject.java @@ -0,0 +1,15 @@ +package ai.timefold.solver.core.impl.testdata.domain.clone.deepcloning; + +import ai.timefold.solver.core.api.domain.solution.cloner.DeepPlanningClone; + +@DeepPlanningClone +public class ExtraDeepClonedObject { + public String id; + + public ExtraDeepClonedObject() { + } + + public ExtraDeepClonedObject(String id) { + this.id = id; + } +} diff --git a/core/src/test/java/ai/timefold/solver/core/impl/testdata/domain/clone/deepcloning/TestdataVariousTypes.java b/core/src/test/java/ai/timefold/solver/core/impl/testdata/domain/clone/deepcloning/TestdataVariousTypes.java index 61655a9ae5..ae386bea77 100644 --- a/core/src/test/java/ai/timefold/solver/core/impl/testdata/domain/clone/deepcloning/TestdataVariousTypes.java +++ b/core/src/test/java/ai/timefold/solver/core/impl/testdata/domain/clone/deepcloning/TestdataVariousTypes.java @@ -42,6 +42,9 @@ public class TestdataVariousTypes { @DeepPlanningClone public List deepClonedListRef = Collections.singletonList(stringRef); + // And a DeepPlanningCloned object + public ExtraDeepClonedObject extraDeepClonedObject = new ExtraDeepClonedObject("extra"); + public record NonClonedRecord(int a, String b) { } diff --git a/quarkus-integration/quarkus/deployment/src/main/java/ai/timefold/solver/quarkus/deployment/DotNames.java b/quarkus-integration/quarkus/deployment/src/main/java/ai/timefold/solver/quarkus/deployment/DotNames.java index c79ed5f1af..d2ec645879 100644 --- a/quarkus-integration/quarkus/deployment/src/main/java/ai/timefold/solver/quarkus/deployment/DotNames.java +++ b/quarkus-integration/quarkus/deployment/src/main/java/ai/timefold/solver/quarkus/deployment/DotNames.java @@ -7,6 +7,7 @@ import ai.timefold.solver.core.api.domain.constraintweight.ConstraintWeight; import ai.timefold.solver.core.api.domain.entity.PlanningEntity; import ai.timefold.solver.core.api.domain.entity.PlanningPin; +import ai.timefold.solver.core.api.domain.entity.PlanningPinToIndex; import ai.timefold.solver.core.api.domain.lookup.PlanningId; import ai.timefold.solver.core.api.domain.solution.ConstraintWeightOverrides; import ai.timefold.solver.core.api.domain.solution.PlanningEntityCollectionProperty; @@ -55,6 +56,7 @@ public final class DotNames { static final DotName PLANNING_ENTITY = DotName.createSimple(PlanningEntity.class.getName()); static final DotName PLANNING_PIN = DotName.createSimple(PlanningPin.class.getName()); + static final DotName PLANNING_PIN_TO_INDEX = DotName.createSimple(PlanningPinToIndex.class.getName()); static final DotName PLANNING_ID = DotName.createSimple(PlanningId.class.getName()); static final DotName PLANNING_VARIABLE = DotName.createSimple(PlanningVariable.class.getName()); @@ -79,6 +81,7 @@ public final class DotNames { static final DotName[] PLANNING_ENTITY_FIELD_ANNOTATIONS = { PLANNING_PIN, + PLANNING_PIN_TO_INDEX, PLANNING_VARIABLE, PLANNING_LIST_VARIABLE, ANCHOR_SHADOW_VARIABLE, @@ -101,6 +104,7 @@ public final class DotNames { CONSTRAINT_CONFIGURATION_PROVIDER, CONSTRAINT_WEIGHT, PLANNING_PIN, + PLANNING_PIN_TO_INDEX, PLANNING_ID, PLANNING_VARIABLE, PLANNING_LIST_VARIABLE,