Skip to content

Commit

Permalink
feat: improve support for Java records (#308)
Browse files Browse the repository at this point in the history
Does several things:

1. Records can serve as planning variables; planning ID annotation on
record components no longer throws.
2. Records can no longer be solutions or entities, as these need to be
mutable.
3. Records are no longer deep-cloned, receiving the same treatment as
enums.
4. Throws a better exception when cloning fails due to an exception in
constructor.

The second and third change are technically backwards incompatible
behavior but we introduce it anyway, as the old behavior was erroneous.
  • Loading branch information
triceo authored Oct 9, 2023
1 parent f8bdb36 commit 2c56079
Show file tree
Hide file tree
Showing 13 changed files with 390 additions and 83 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -458,6 +458,16 @@ private static Optional<Class<?>> extractCollectionGenericTypeParameter(
}
}

/**
* This method is heavy, and it is effectively a computed constant.
* It is recommended that its results are cached at call sites.
*
* @param clazz never null
* @param memberAccessorFactory never null
* @param domainAccessType never null
* @return null if no accessor found
* @param <C> the class type
*/
public static <C> MemberAccessor findPlanningIdMemberAccessor(Class<C> clazz,
MemberAccessorFactory memberAccessorFactory, DomainAccessType domainAccessType) {
Member member = getSingleMember(clazz, PlanningId.class);
Expand Down Expand Up @@ -487,14 +497,50 @@ private static <C> Member getSingleMember(Class<C> clazz, Class<? extends Annota
if (memberList.isEmpty()) {
return null;
}
if (memberList.size() > 1) {
throw new IllegalArgumentException("The class (" + clazz
+ ") has " + memberList.size() + " members (" + memberList + ") with a "
+ annotationClass.getSimpleName() + " annotation.");
int size = memberList.size();
if (clazz.isRecord()) {
/*
* A record has a field and a getter for each record component.
* When the component is annotated with @PlanningId,
* the annotation ends up both on the field and on the getter.
*/
if (size == 2) { // The getter is used to retrieve the value of the record component.
var methodMembers = getMembers(memberList, true);
if (methodMembers.isEmpty()) {
throw new IllegalStateException("""
Impossible state: record (%s) doesn't have any method members (%s).
""".formatted(clazz.getCanonicalName(), memberList));
}
return methodMembers.get(0);
} else { // There is more than one component annotated with @PlanningId; take the fields and fail.
var componentList = getMembers(memberList, false)
.stream()
.map(Member::getName)
.toList();
throw new IllegalArgumentException("""
The record (%s) has %s components (%s) with %s annotation.
""".formatted(clazz, componentList.size(), componentList, annotationClass.getSimpleName()));
}
} else if (size > 1) {
throw new IllegalArgumentException("""
The class (%s) has %s members (%s) with %s annotation.
""".formatted(clazz, memberList.size(), memberList, annotationClass.getSimpleName()));
}
return memberList.get(0);
}

private static List<Member> getMembers(List<Member> memberList, boolean needMethod) {
var filteredMemberList = new ArrayList<Member>(memberList.size());
for (var member : memberList) {
if (member instanceof Method && needMethod) {
filteredMemberList.add(member);
} else if (member instanceof Field && !needMethod) {
filteredMemberList.add(member);
}
}
return filteredMemberList;
}

public static String abbreviate(List<String> list, int limit) {
String abbreviation = "";
if (list != null) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,7 @@ public class EntityDescriptor<Solution_> {
// ************************************************************************

public EntityDescriptor(SolutionDescriptor<Solution_> solutionDescriptor, Class<?> entityClass) {
SolutionDescriptor.assertMutable(entityClass, "entityClass");
this.solutionDescriptor = solutionDescriptor;
this.entityClass = entityClass;
isInitializedPredicate = this::isInitialized;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -82,15 +82,7 @@ private boolean deepClone(SolutionDescriptor<?> solutionDescriptor, Class<?> fie
return fieldDeepCloneDecision.get() == 1;
}

private static final class Metadata {

private final Class<?> clz;
private final boolean decision;

public Metadata(Class<?> clz, boolean decision) {
this.clz = clz;
this.decision = decision;
}

private record Metadata(Class<?> clz, boolean decision) {
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -81,22 +81,28 @@ public static boolean isFieldDeepCloned(SolutionDescriptor<?> solutionDescriptor
Class<?> fieldType = field.getType();
if (isImmutable(fieldType)) {
return false;
} else {
return needsDeepClone(solutionDescriptor, field, owningClass);
}

}

public static boolean needsDeepClone(SolutionDescriptor<?> solutionDescriptor, Field field, Class<?> owningClass) {
return isFieldAnEntityPropertyOnSolution(solutionDescriptor, field, owningClass)
|| isFieldAnEntityOrSolution(solutionDescriptor, field)
|| isFieldAPlanningListVariable(field, owningClass)
|| isFieldADeepCloneProperty(field, owningClass);
}

static boolean isImmutable(Class<?> clz) {
if (clz.isPrimitive() || clz.isEnum() || Score.class.isAssignableFrom(clz)) {
if (clz.isPrimitive() || clz.isEnum() || clz.isRecord() || Score.class.isAssignableFrom(clz)) {
return true;
}
return IMMUTABLE_CLASSES.contains(clz);
}

/**
* Return true only if a field represent an entity property on the solution class.
* Return true only if a field represents an entity property on the solution class.
* An entity property is one who type is a PlanningEntity or a collection
* of PlanningEntity.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,15 +8,13 @@
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collection;
import java.util.Comparator;
import java.util.Deque;
import java.util.HashMap;
import java.util.HashSet;
import java.util.IdentityHashMap;
import java.util.LinkedHashMap;
import java.util.LinkedHashSet;
import java.util.LinkedList;
import java.util.List;
import java.util.Map;
import java.util.Queue;
import java.util.Set;
Expand All @@ -26,6 +24,7 @@
import java.util.TreeSet;
import java.util.concurrent.ConcurrentMap;

import ai.timefold.solver.core.api.domain.solution.cloner.DeepPlanningClone;
import ai.timefold.solver.core.api.domain.solution.cloner.SolutionCloner;
import ai.timefold.solver.core.impl.domain.common.accessor.MemberAccessor;
import ai.timefold.solver.core.impl.domain.solution.descriptor.SolutionDescriptor;
Expand Down Expand Up @@ -68,9 +67,9 @@ private Object process(Unprocessed unprocessed, Map<Object, Object> originalToCl
Object originalValue = unprocessed.originalValue;
Field field = unprocessed.field;
Class<?> fieldType = field.getType();
if (originalValue instanceof Collection collection) {
if (originalValue instanceof Collection<?> collection) {
return cloneCollection(fieldType, collection, originalToCloneMap, unprocessedQueue);
} else if (originalValue instanceof Map map) {
} else if (originalValue instanceof Map<?, ?> map) {
return cloneMap(fieldType, map, originalToCloneMap, unprocessedQueue);
} else if (originalValue.getClass().isArray()) {
return cloneArray(fieldType, originalValue, originalToCloneMap, unprocessedQueue);
Expand All @@ -97,20 +96,25 @@ private <C> C clone(C original, Map<Object, Object> originalToCloneMap, Queue<Un
}

private <C> C constructClone(Class<C> clazz) {
var constructor = constructorMemoization.computeIfAbsent(clazz, key -> {
try {
var ctor = (Constructor<C>) key.getDeclaredConstructor();
ctor.setAccessible(true);
return ctor;
} catch (ReflectiveOperationException e) {
throw new IllegalStateException(
"To create a planning clone, the class (%s) must have a no-arg constructor."
.formatted(key.getCanonicalName()),
e);
}
});
try {
return (C) constructorMemoization.computeIfAbsent(clazz, key -> {
try {
Constructor<C> constructor = (Constructor<C>) key.getDeclaredConstructor();
constructor.setAccessible(true);
return constructor;
} catch (ReflectiveOperationException e) {
throw new IllegalStateException("The class (" + key
+ ") should have a no-arg constructor to create a planning clone.", e);
}
}).newInstance();
} catch (ReflectiveOperationException e) {
throw new IllegalStateException("The class (" + clazz
+ ") should have a no-arg constructor to create a planning clone.", e);
return (C) constructor.newInstance();
} catch (Exception e) {
throw new IllegalStateException(
"Can not create a new instance of class (%s) for a planning clone, using its no-arg constructor."
.formatted(clazz.getCanonicalName()),
e);
}
}

Expand Down Expand Up @@ -167,31 +171,24 @@ private <E> Collection<E> cloneCollection(Class<?> expectedType, Collection<E> o

private static <E> Collection<E> constructCloneCollection(Collection<E> originalCollection) {
// TODO Don't hardcode all standard collections
if (originalCollection instanceof List) {
if (originalCollection instanceof ArrayList) {
return new ArrayList<>(originalCollection.size());
} else if (originalCollection instanceof LinkedList) {
return new LinkedList<>();
} else { // Default List
return new ArrayList<>(originalCollection.size());
}
} else if (originalCollection instanceof Set) {
if (originalCollection instanceof SortedSet set) {
Comparator<E> setComparator = set.comparator();
if (originalCollection instanceof LinkedList) {
return new LinkedList<>();
}
var size = originalCollection.size();
if (originalCollection instanceof Set) {
if (originalCollection instanceof SortedSet<E> set) {
var setComparator = set.comparator();
return new TreeSet<>(setComparator);
} else if (originalCollection instanceof LinkedHashSet) {
return new LinkedHashSet<>(originalCollection.size());
} else if (originalCollection instanceof HashSet) {
return new HashSet<>(originalCollection.size());
} else { // Default Set
// Default to a LinkedHashSet to respect order
return new LinkedHashSet<>(originalCollection.size());
} else if (!(originalCollection instanceof LinkedHashSet)) {
return new HashSet<>(size);
} else { // Default to a LinkedHashSet to respect order.
return new LinkedHashSet<>(size);
}
} else if (originalCollection instanceof Deque) {
return new ArrayDeque<>(originalCollection.size());
} else { // Default collection
return new ArrayList<>(originalCollection.size());
return new ArrayDeque<>(size);
}
// Default collection
return new ArrayList<>(size);
}

private <K, V> Map<K, V> cloneMap(Class<?> expectedType, Map<K, V> originalMap, Map<Object, Object> originalToCloneMap,
Expand All @@ -212,17 +209,16 @@ private <K, V> Map<K, V> cloneMap(Class<?> expectedType, Map<K, V> originalMap,
}

private static <K, V> Map<K, V> constructCloneMap(Map<K, V> originalMap) {
// Normally a Map will never be selected for cloning, but extending implementations might anyway
if (originalMap instanceof SortedMap map) {
Comparator<K> setComparator = map.comparator();
// Normally, a Map will never be selected for cloning, but extending implementations might anyway.
if (originalMap instanceof SortedMap<K, V> map) {
var setComparator = map.comparator();
return new TreeMap<>(setComparator);
} else if (originalMap instanceof LinkedHashMap) {
return new LinkedHashMap<>(originalMap.size());
} else if (originalMap instanceof HashMap) {
return new HashMap<>(originalMap.size());
} else { // Default Map
// Default to a LinkedHashMap to respect order
return new LinkedHashMap<>(originalMap.size());
}
var originalMapSize = originalMap.size();
if (!(originalMap instanceof LinkedHashMap)) {
return new HashMap<>(originalMapSize);
} else { // Default to a LinkedHashMap to respect order.
return new LinkedHashMap<>(originalMapSize);
}
}

Expand All @@ -235,12 +231,15 @@ private <C> C cloneCollectionsElementIfNeeded(C original, Map<Object, Object> or
if (original == null) {
return null;
}
// Because an element which is itself a Collection or Map might hold an entity, we clone it too
// Also, the List<Long> in Map<String, List<Long>> needs to be cloned
// if the List<Long> is a shadow, despite that Long never needs to be cloned (because it's immutable).
if (original instanceof Collection collection) {
/*
* Because an element which is itself a Collection or Map might hold an entity,
* we clone it too.
* The List<Long> in Map<String, List<Long>> needs to be cloned if the List<Long> is a shadow,
* despite that Long never needs to be cloned (because it's immutable).
*/
if (original instanceof Collection<?> collection) {
return (C) cloneCollection(Collection.class, collection, originalToCloneMap, unprocessedQueue);
} else if (original instanceof Map map) {
} else if (original instanceof Map<?, ?> map) {
return (C) cloneMap(Map.class, map, originalToCloneMap, unprocessedQueue);
} else if (original.getClass().isArray()) {
return (C) cloneArray(original.getClass(), original, originalToCloneMap, unprocessedQueue);
Expand Down Expand Up @@ -306,7 +305,20 @@ public ShallowCloningFieldCloner[] getCopiedFieldArray() {
copiedFieldArray = Arrays.stream(declaringClass.getDeclaredFields())
.filter(f -> !Modifier.isStatic(f.getModifiers()))
.filter(field -> DeepCloningUtils.isImmutable(field.getType()))
.peek(f -> f.setAccessible(true))
.peek(f -> {
if (DeepCloningUtils.needsDeepClone(solutionDescriptor, f, declaringClass)) {
throw new IllegalStateException("""
The field (%s) of class (%s) needs to be deep-cloned,
but its type (%s) is immutable and can not be deep-cloned.
Maybe remove the @%s annotation from the field?
Maybe do not reference planning entities inside Java records?
"""
.formatted(f.getName(), declaringClass.getCanonicalName(),
f.getType().getCanonicalName(), DeepPlanningClone.class.getSimpleName()));
} else {
f.setAccessible(true);
}
})
.map(ShallowCloningFieldCloner::of)
.toArray(ShallowCloningFieldCloner[]::new);
}
Expand All @@ -327,17 +339,6 @@ public DeepCloningFieldCloner[] getClonedFieldArray() {

}

private static final class Unprocessed {

final Object bean;
final Field field;
final Object originalValue;

public Unprocessed(Object bean, Field field, Object originalValue) {
this.bean = bean;
this.field = field;
this.originalValue = originalValue;
}

private record Unprocessed(Object bean, Field field, Object originalValue) {
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@ public static <Solution_> SolutionDescriptor<Solution_> buildSolutionDescriptor(
public static <Solution_> SolutionDescriptor<Solution_> buildSolutionDescriptor(DomainAccessType domainAccessType,
Class<Solution_> solutionClass, Map<String, MemberAccessor> memberAccessorMap,
Map<String, SolutionCloner> solutionClonerMap, List<Class<?>> entityClassList) {
assertMutable(solutionClass, "solutionClass");
solutionClonerMap = Objects.requireNonNullElse(solutionClonerMap, Collections.emptyMap());
SolutionDescriptor<Solution_> solutionDescriptor = new SolutionDescriptor<>(solutionClass, memberAccessorMap);
DescriptorPolicy descriptorPolicy = new DescriptorPolicy();
Expand All @@ -110,6 +111,20 @@ public static <Solution_> SolutionDescriptor<Solution_> buildSolutionDescriptor(
return solutionDescriptor;
}

public static void assertMutable(Class<?> clz, String classType) {
if (clz.isRecord()) {
throw new IllegalArgumentException("""
The %s (%s) cannot be a record as it needs to be mutable.
Use a regular class instead.
""".formatted(classType, clz.getCanonicalName()));
} else if (clz.isEnum()) {
throw new IllegalArgumentException("""
The %s (%s) cannot be an enum as it needs to be mutable.
Use a regular class instead.
""".formatted(classType, clz.getCanonicalName()));
}
}

private static List<Class<?>> sortEntityClassList(List<Class<?>> entityClassList) {
List<Class<?>> sortedEntityClassList = new ArrayList<>(entityClassList.size());
for (Class<?> entityClass : entityClassList) {
Expand Down
Loading

0 comments on commit 2c56079

Please sign in to comment.