Skip to content

Commit 7ff611d

Browse files
triceotriceo
committed
chore: enforce consistency between variable getters and setters
If a getter exists, a setter has to. If a setter exists, a getter has to. Both getter and setter need to have the same visibility.
1 parent 7ae3365 commit 7ff611d

19 files changed

+221
-74
lines changed

‎core/src/main/java/ai/timefold/solver/core/impl/domain/common/ReflectionHelper.java‎

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -136,7 +136,7 @@ public static Method getDeclaredGetterMethod(Class<?> containingClass, String pr
136136
var baseClass = containingClass;
137137
while (baseClass != null) {
138138
try {
139-
return containingClass.getDeclaredMethod(getterName);
139+
return baseClass.getDeclaredMethod(getterName);
140140
} catch (NoSuchMethodException e) {
141141
baseClass = baseClass.getSuperclass();
142142
}
@@ -158,12 +158,12 @@ public static Method getDeclaredGetterMethod(Class<?> containingClass, String pr
158158
* @param methodName never null
159159
* @return sometimes null
160160
*/
161-
public static Method getDeclaredMethod(Class<?> containingClass, String methodName) {
161+
public static Method getDeclaredMethod(Class<?> containingClass, String methodName, Class<?>... parameterTypes) {
162162
var baseClass = containingClass;
163163

164164
while (baseClass != null) {
165165
try {
166-
return containingClass.getDeclaredMethod(methodName);
166+
return baseClass.getDeclaredMethod(methodName, parameterTypes);
167167
} catch (NoSuchMethodException e) {
168168
baseClass = baseClass.getSuperclass();
169169
}
@@ -233,6 +233,17 @@ public static Method getSetterMethod(Class<?> containingClass, Class<?> property
233233
}
234234
}
235235

236+
/**
237+
* @param containingClass never null
238+
* @param propertyType never null
239+
* @param propertyName never null
240+
* @return null if it doesn't exist
241+
*/
242+
public static Method getDeclaredSetterMethod(Class<?> containingClass, Class<?> propertyType, String propertyName) {
243+
String setterName = PROPERTY_MUTATOR_PREFIX + capitalizePropertyName(propertyName);
244+
return getDeclaredMethod(containingClass, setterName, propertyType);
245+
}
246+
236247
/**
237248
* @param containingClass never null
238249
* @param propertyName never null

‎core/src/main/java/ai/timefold/solver/core/impl/domain/common/accessor/ReflectionBeanPropertyMemberAccessor.java‎

Lines changed: 56 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,9 @@
44
import java.lang.invoke.MethodHandle;
55
import java.lang.invoke.MethodHandles;
66
import java.lang.reflect.Method;
7+
import java.lang.reflect.Modifier;
78
import java.lang.reflect.Type;
9+
import java.util.function.IntPredicate;
810

911
import ai.timefold.solver.core.impl.domain.common.ReflectionHelper;
1012

@@ -34,38 +36,73 @@ public ReflectionBeanPropertyMemberAccessor(Method getterMethod, boolean getterO
3436
} catch (IllegalAccessException e) {
3537
throw new IllegalStateException("""
3638
Impossible state: method (%s) not accessible.
37-
%s
38-
"""
39-
.strip()
39+
%s"""
4040
.formatted(getterMethod, MemberAccessorFactory.CLASSLOADER_NUDGE_MESSAGE), e);
4141
}
4242
Class<?> declaringClass = getterMethod.getDeclaringClass();
4343
if (!ReflectionHelper.isGetterMethod(getterMethod)) {
44-
throw new IllegalArgumentException("The getterMethod (" + getterMethod + ") is not a valid getter.");
44+
throw new IllegalArgumentException("The getterMethod (%s) is not a valid getter."
45+
.formatted(getterMethod));
4546
}
4647
propertyType = getterMethod.getReturnType();
4748
propertyName = ReflectionHelper.getGetterPropertyName(getterMethod);
4849
if (getterOnly) {
4950
setterMethod = null;
5051
setterMethodHandle = null;
5152
} else {
52-
setterMethod = ReflectionHelper.getSetterMethod(declaringClass, getterMethod.getReturnType(), propertyName);
53-
if (setterMethod != null) {
54-
try {
55-
setterMethod.setAccessible(true); // Performance hack by avoiding security checks
56-
this.setterMethodHandle = lookup.unreflect(setterMethod)
57-
.asFixedArity();
58-
} catch (IllegalAccessException e) {
59-
throw new IllegalStateException("""
60-
Impossible state: method (%s) not accessible.
61-
%s
62-
"""
63-
.strip()
64-
.formatted(setterMethod, MemberAccessorFactory.CLASSLOADER_NUDGE_MESSAGE), e);
53+
setterMethod = ReflectionHelper.getDeclaredSetterMethod(declaringClass, getterMethod.getReturnType(), propertyName);
54+
if (setterMethod == null) {
55+
throw new IllegalArgumentException("The getterMethod (%s) does not have a matching setterMethod on class (%s)."
56+
.formatted(getterMethod.getName(), declaringClass.getCanonicalName()));
57+
}
58+
var getterAccess = AccessModifier.forMethod(getterMethod);
59+
var setterAccess = AccessModifier.forMethod(setterMethod);
60+
if (getterAccess != setterAccess) {
61+
throw new IllegalArgumentException(
62+
"The getterMethod (%s) has access modifier (%s) which does not match the setterMethod (%s) access modifier (%s) on class (%s)."
63+
.formatted(getterMethod.getName(), getterAccess, setterMethod.getName(), setterAccess,
64+
declaringClass.getCanonicalName()));
65+
}
66+
try {
67+
setterMethod.setAccessible(true); // Performance hack by avoiding security checks
68+
this.setterMethodHandle = lookup.unreflect(setterMethod)
69+
.asFixedArity();
70+
} catch (IllegalAccessException e) {
71+
throw new IllegalStateException("""
72+
Impossible state: method (%s) not accessible.
73+
%s"""
74+
.formatted(setterMethod, MemberAccessorFactory.CLASSLOADER_NUDGE_MESSAGE), e);
75+
}
76+
}
77+
}
78+
79+
private enum AccessModifier {
80+
PUBLIC("public", Modifier::isPublic),
81+
PROTECTED("protected", Modifier::isProtected),
82+
PACKAGE_PRIVATE("package-private", modifier -> false),
83+
PRIVATE("private", Modifier::isPrivate);
84+
85+
final String name;
86+
final IntPredicate predicate;
87+
88+
AccessModifier(String name, IntPredicate predicate) {
89+
this.name = name;
90+
this.predicate = predicate;
91+
}
92+
93+
public static AccessModifier forMethod(Method method) {
94+
var modifiers = method.getModifiers();
95+
for (var accessModifier : AccessModifier.values()) {
96+
if (accessModifier.predicate.test(modifiers)) {
97+
return accessModifier;
6598
}
66-
} else {
67-
setterMethodHandle = null;
6899
}
100+
return PACKAGE_PRIVATE;
101+
}
102+
103+
@Override
104+
public String toString() {
105+
return name;
69106
}
70107
}
71108

‎core/src/main/java/ai/timefold/solver/core/impl/domain/entity/descriptor/EntityDescriptor.java‎

Lines changed: 17 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,5 @@
11
package ai.timefold.solver.core.impl.domain.entity.descriptor;
22

3-
import static ai.timefold.solver.core.impl.domain.common.accessor.MemberAccessorFactory.MemberAccessorType.FIELD_OR_GETTER_METHOD;
43
import static ai.timefold.solver.core.impl.domain.common.accessor.MemberAccessorFactory.MemberAccessorType.FIELD_OR_GETTER_METHOD_WITH_SETTER;
54
import static ai.timefold.solver.core.impl.domain.common.accessor.MemberAccessorFactory.MemberAccessorType.FIELD_OR_READ_METHOD;
65
import static ai.timefold.solver.core.impl.domain.entity.descriptor.EntityDescriptorValidator.assertNotMixedInheritance;
@@ -46,7 +45,6 @@
4645
import ai.timefold.solver.core.config.util.ConfigUtils;
4746
import ai.timefold.solver.core.impl.domain.common.ReflectionHelper;
4847
import ai.timefold.solver.core.impl.domain.common.accessor.MemberAccessor;
49-
import ai.timefold.solver.core.impl.domain.common.accessor.MemberAccessorFactory;
5048
import ai.timefold.solver.core.impl.domain.policy.DescriptorPolicy;
5149
import ai.timefold.solver.core.impl.domain.solution.descriptor.ProblemScaleTracker;
5250
import ai.timefold.solver.core.impl.domain.solution.descriptor.SolutionDescriptor;
@@ -323,44 +321,27 @@ private void processValueRangeProviderAnnotation(DescriptorPolicy descriptorPoli
323321
}
324322
}
325323

324+
@SuppressWarnings("unchecked")
326325
private void processPlanningVariableAnnotation(MutableInt variableDescriptorCounter, DescriptorPolicy descriptorPolicy,
327326
Member member) {
328-
var variableAnnotationClass = ConfigUtils.extractAnnotationClass(
329-
member, VARIABLE_ANNOTATION_CLASSES);
327+
var variableAnnotationClass = ConfigUtils.extractAnnotationClass(member, VARIABLE_ANNOTATION_CLASSES);
330328
if (variableAnnotationClass != null) {
331-
MemberAccessorFactory.MemberAccessorType memberAccessorType;
332-
if (variableAnnotationClass.equals(ShadowVariable.class)) {
333-
// Need to check only the single annotation version,
334-
// since supplier variable can only be used with a single
335-
// annotation.
336-
ShadowVariable annotation;
337-
if (member instanceof Field field) {
338-
annotation = field.getAnnotation(ShadowVariable.class);
339-
} else if (member instanceof Method method) {
340-
annotation = method.getAnnotation(ShadowVariable.class);
341-
} else {
342-
throw new IllegalStateException(
343-
"Member must be a field or a method, but was (%s).".formatted(member.getClass().getSimpleName()));
344-
}
345-
if (annotation == null) {
346-
throw new IllegalStateException("Impossible state: cannot get %s annotation on a %s annotated member (%s)."
347-
.formatted(ShadowVariable.class.getSimpleName(), ShadowVariable.class.getSimpleName(), member));
348-
}
349-
if (!annotation.supplierName().isEmpty()) {
350-
memberAccessorType = FIELD_OR_GETTER_METHOD_WITH_SETTER;
351-
} else {
352-
memberAccessorType = FIELD_OR_GETTER_METHOD;
353-
}
354-
} else if (variableAnnotationClass.equals(CustomShadowVariable.class)
355-
|| variableAnnotationClass.equals(ShadowVariable.List.class)
356-
|| variableAnnotationClass.equals(PiggybackShadowVariable.class)
357-
|| variableAnnotationClass.equals(CascadingUpdateShadowVariable.class)) {
358-
memberAccessorType = FIELD_OR_GETTER_METHOD;
329+
Object annotation;
330+
if (member instanceof Field field) {
331+
annotation = field.getAnnotation(variableAnnotationClass);
332+
} else if (member instanceof Method method) {
333+
annotation = method.getAnnotation(variableAnnotationClass);
359334
} else {
360-
memberAccessorType = FIELD_OR_GETTER_METHOD_WITH_SETTER;
335+
throw new IllegalStateException("Member must be a field or a method, but was (%s)."
336+
.formatted(member.getClass().getSimpleName()));
361337
}
362-
var memberAccessor = descriptorPolicy.getMemberAccessorFactory().buildAndCacheMemberAccessor(member,
363-
memberAccessorType, variableAnnotationClass, descriptorPolicy.getDomainAccessType());
338+
if (annotation == null) {
339+
throw new IllegalStateException("Impossible state: cannot get annotation on a %s-annotated member (%s)."
340+
.formatted(variableAnnotationClass, member));
341+
}
342+
var memberAccessor = descriptorPolicy.getMemberAccessorFactory()
343+
.buildAndCacheMemberAccessor(member, FIELD_OR_GETTER_METHOD_WITH_SETTER, variableAnnotationClass,
344+
descriptorPolicy.getDomainAccessType());
364345
registerVariableAccessor(variableDescriptorCounter.intValue(), variableAnnotationClass, memberAccessor);
365346
variableDescriptorCounter.increment();
366347
}
@@ -695,10 +676,6 @@ public GenuineVariableDescriptor<Solution_> getGenuineVariableDescriptor(String
695676
return shadowVariableLoopedDescriptor;
696677
}
697678

698-
public boolean hasAnyGenuineVariables() {
699-
return !effectiveGenuineVariableDescriptorMap.isEmpty();
700-
}
701-
702679
public boolean hasBothGenuineListAndBasicVariables() {
703680
if (!isGenuine()) {
704681
return false;
@@ -732,7 +709,7 @@ public boolean hasAnyGenuineListVariables() {
732709
}
733710

734711
public boolean isGenuine() {
735-
return hasAnyGenuineVariables();
712+
return !effectiveGenuineVariableDescriptorMap.isEmpty();
736713
}
737714

738715
public ListVariableDescriptor<Solution_> getGenuineListVariableDescriptor() {

‎core/src/main/java/ai/timefold/solver/core/impl/score/stream/bavet/BavetConstraintFactory.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -135,7 +135,7 @@ public <Stream_ extends BavetAbstractConstraintStream<Solution_>> Stream_ share(
135135
public @NonNull <A> UniConstraintStream<A> from(@NonNull Class<A> fromClass) {
136136
assertValidFromType(fromClass);
137137
var entityDescriptor = solutionDescriptor.findEntityDescriptor(fromClass);
138-
if (entityDescriptor != null && entityDescriptor.hasAnyGenuineVariables()) {
138+
if (entityDescriptor != null && entityDescriptor.isGenuine()) {
139139
var predicate = (Predicate<A>) entityDescriptor.getIsInitializedPredicate();
140140
return share(new BavetForEachUniConstraintStream<>(this, fromClass, predicate, RetrievalSemantics.LEGACY));
141141
} else {

‎core/src/test/java/ai/timefold/solver/core/impl/domain/common/accessor/ReflectionBeanPropertyMemberAccessorTest.java‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,13 @@
11
package ai.timefold.solver.core.impl.domain.common.accessor;
22

33
import static org.assertj.core.api.Assertions.assertThat;
4+
import static org.assertj.core.api.Assertions.assertThatCode;
45

56
import ai.timefold.solver.core.api.domain.variable.PlanningVariable;
67
import ai.timefold.solver.core.testdomain.TestdataEntity;
78
import ai.timefold.solver.core.testdomain.TestdataValue;
9+
import ai.timefold.solver.core.testdomain.invalid.gettersetter.TestdataDifferentGetterSetterVisibilityEntity;
10+
import ai.timefold.solver.core.testdomain.invalid.gettersetter.TestdataInvalidGetterEntity;
811

912
import org.junit.jupiter.api.Test;
1013

@@ -26,4 +29,31 @@ void methodAnnotatedEntity() throws NoSuchMethodException {
2629
assertThat(e1.getValue()).isSameAs(v2);
2730
}
2831

32+
@Test
33+
void getterSetterVisibilityDoesNotMatch() {
34+
assertThatCode(() -> new ReflectionBeanPropertyMemberAccessor(
35+
TestdataDifferentGetterSetterVisibilityEntity.class.getDeclaredMethod("getValue1")))
36+
.hasMessageContainingAll("getterMethod (getValue1)",
37+
"has access modifier (public)",
38+
"not match the setterMethod (setValue1)",
39+
"access modifier (private)",
40+
"on class (ai.timefold.solver.core.testdomain.invalid.gettersetter.TestdataDifferentGetterSetterVisibilityEntity)");
41+
assertThatCode(() -> new ReflectionBeanPropertyMemberAccessor(
42+
TestdataDifferentGetterSetterVisibilityEntity.class.getDeclaredMethod("getValue2")))
43+
.hasMessageContainingAll("getterMethod (getValue2)",
44+
"has access modifier (package-private)",
45+
"not match the setterMethod (setValue2)",
46+
"access modifier (protected)",
47+
"on class (ai.timefold.solver.core.testdomain.invalid.gettersetter.TestdataDifferentGetterSetterVisibilityEntity)");
48+
}
49+
50+
@Test
51+
void setterMissing() {
52+
assertThatCode(() -> new ReflectionBeanPropertyMemberAccessor(
53+
TestdataInvalidGetterEntity.class.getDeclaredMethod("getValueWithoutSetter")))
54+
.hasMessageContainingAll("getterMethod (getValueWithoutSetter)",
55+
"does not have a matching setterMethod",
56+
"on class (ai.timefold.solver.core.testdomain.invalid.gettersetter.TestdataInvalidGetterEntity)");
57+
}
58+
2959
}

‎core/src/test/java/ai/timefold/solver/core/testdomain/invalid/gettersetter/TestdataDifferentGetterSetterVisibilityEntity.java‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
package ai.timefold.solver.core.testdomain.invalid.gettersetter;
2+
3+
import ai.timefold.solver.core.api.domain.entity.PlanningEntity;
4+
import ai.timefold.solver.core.api.domain.variable.PlanningVariable;
5+
import ai.timefold.solver.core.testdomain.TestdataValue;
6+
7+
@PlanningEntity
8+
public class TestdataDifferentGetterSetterVisibilityEntity {
9+
10+
private TestdataValue value1;
11+
private TestdataValue value2;
12+
13+
@PlanningVariable
14+
public TestdataValue getValue1() {
15+
return value1;
16+
}
17+
18+
private void setValue1(TestdataValue value1) {
19+
this.value1 = value1;
20+
}
21+
22+
@PlanningVariable
23+
TestdataValue getValue2() {
24+
return value2;
25+
}
26+
27+
@PlanningVariable
28+
protected TestdataValue setValue2(TestdataValue value2) {
29+
return value2;
30+
}
31+
32+
}

‎core/src/test/java/ai/timefold/solver/core/testdomain/invalid/gettersetter/TestdataInvalidGetterEntity.java‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
package ai.timefold.solver.core.testdomain.invalid.gettersetter;
2+
3+
import ai.timefold.solver.core.api.domain.entity.PlanningEntity;
4+
import ai.timefold.solver.core.api.domain.variable.PlanningVariable;
5+
import ai.timefold.solver.core.testdomain.TestdataValue;
6+
7+
@PlanningEntity
8+
public class TestdataInvalidGetterEntity {
9+
10+
@PlanningVariable
11+
private TestdataValue valueWithoutSetter;
12+
13+
public TestdataValue getValueWithoutSetter() {
14+
return valueWithoutSetter;
15+
}
16+
17+
}

‎core/src/test/java/ai/timefold/solver/core/testdomain/score/TestdataBendableBigDecimalScoreSolution.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -75,7 +75,7 @@ public void setEntityList(List<TestdataEntity> entityList) {
7575
}
7676

7777
@PlanningScore(bendableHardLevelsSize = 1, bendableSoftLevelsSize = 2)
78-
BendableBigDecimalScore getScore() {
78+
public BendableBigDecimalScore getScore() {
7979
return score;
8080
}
8181

‎core/src/test/java/ai/timefold/solver/core/testdomain/score/TestdataBendableLongScoreSolution.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -75,7 +75,7 @@ public void setEntityList(List<TestdataEntity> entityList) {
7575
}
7676

7777
@PlanningScore(bendableHardLevelsSize = 1, bendableSoftLevelsSize = 2)
78-
BendableLongScore getScore() {
78+
public BendableLongScore getScore() {
7979
return score;
8080
}
8181

‎core/src/test/java/ai/timefold/solver/core/testdomain/score/TestdataBendableScoreSolution.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -75,7 +75,7 @@ public void setEntityList(List<TestdataEntity> entityList) {
7575
}
7676

7777
@PlanningScore(bendableHardLevelsSize = 1, bendableSoftLevelsSize = 2)
78-
BendableScore getScore() {
78+
public BendableScore getScore() {
7979
return score;
8080
}
8181

0 commit comments

Comments
 (0)