chore: enfore getter match setter visibility for variables and score

Author: Christopher-Chianelli

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

@@ -136,7 +136,7 @@ public static Method getDeclaredGetterMethod(Class<?> containingClass, String pr
         var baseClass = containingClass;
         while (baseClass != null) {
             try {
-                return containingClass.getDeclaredMethod(getterName);
+                return baseClass.getDeclaredMethod(getterName);
             } catch (NoSuchMethodException e) {
                 baseClass = baseClass.getSuperclass();
             }
@@ -158,12 +158,12 @@ public static Method getDeclaredGetterMethod(Class<?> containingClass, String pr
      * @param methodName never null
      * @return sometimes null
      */
-    public static Method getDeclaredMethod(Class<?> containingClass, String methodName) {
+    public static Method getDeclaredMethod(Class<?> containingClass, String methodName, Class<?>... parameterTypes) {
         var baseClass = containingClass;
 
         while (baseClass != null) {
             try {
-                return containingClass.getDeclaredMethod(methodName);
+                return baseClass.getDeclaredMethod(methodName, parameterTypes);
             } catch (NoSuchMethodException e) {
                 baseClass = baseClass.getSuperclass();
             }
@@ -233,6 +233,17 @@ public static Method getSetterMethod(Class<?> containingClass, Class<?> property
         }
     }
 
+    /**
+     * @param containingClass never null
+     * @param propertyType never null
+     * @param propertyName never null
+     * @return null if it doesn't exist
+     */
+    public static Method getDeclaredSetterMethod(Class<?> containingClass, Class<?> propertyType, String propertyName) {
+        String setterName = PROPERTY_MUTATOR_PREFIX + capitalizePropertyName(propertyName);
+        return getDeclaredMethod(containingClass, setterName, propertyType);
+    }
+
     /**
      * @param containingClass never null
      * @param propertyName never null

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

@@ -4,7 +4,9 @@
 import java.lang.invoke.MethodHandle;
 import java.lang.invoke.MethodHandles;
 import java.lang.reflect.Method;
+import java.lang.reflect.Modifier;
 import java.lang.reflect.Type;
+import java.util.function.IntPredicate;
 
 import ai.timefold.solver.core.impl.domain.common.ReflectionHelper;
 
@@ -49,8 +51,16 @@ public ReflectionBeanPropertyMemberAccessor(Method getterMethod, boolean getterO
             setterMethod = null;
             setterMethodHandle = null;
         } else {
-            setterMethod = ReflectionHelper.getSetterMethod(declaringClass, getterMethod.getReturnType(), propertyName);
+            setterMethod = ReflectionHelper.getDeclaredSetterMethod(declaringClass, getterMethod.getReturnType(), propertyName);
             if (setterMethod != null) {
+                var getterAccess = AccessModifier.forMethod(getterMethod);
+                var setterAccess = AccessModifier.forMethod(setterMethod);
+                if (getterAccess != setterAccess) {
+                    throw new IllegalArgumentException(
+                            "The getter method (%s) has access modifier (%s) which does not match the setter method's (%s) access modifier (%s) on class (%s)."
+                                    .formatted(getterMethod.getName(), getterAccess, setterMethod.getName(), setterAccess,
+                                            getterMethod.getDeclaringClass().getSimpleName()));
+                }
                 try {
                     setterMethod.setAccessible(true); // Performance hack by avoiding security checks
                     this.setterMethodHandle = lookup.unreflect(setterMethod)
@@ -69,6 +79,41 @@ public ReflectionBeanPropertyMemberAccessor(Method getterMethod, boolean getterO
         }
     }
 
+    private enum AccessModifier {
+        PUBLIC("public", Modifier::isPublic),
+        PROTECTED("protected", Modifier::isProtected),
+        PACKAGE_PRIVATE("package-private", modifier -> false),
+        PRIVATE("private", Modifier::isPrivate);
+
+        final String name;
+        final IntPredicate predicate;
+
+        AccessModifier(String name, IntPredicate predicate) {
+            this.name = name;
+            this.predicate = predicate;
+        }
+
+        public static AccessModifier forMethod(Method method) {
+            var modifiers = method.getModifiers();
+            for (var accessModifier : AccessModifier.values()) {
+                if (accessModifier.predicate.test(modifiers)) {
+                    return accessModifier;
+                }
+            }
+            return PACKAGE_PRIVATE;
+        }
+
+        @Override
+        public String toString() {
+            return name;
+        }
+    }
+
+    private int getAccessLevel(Method method) {
+        int mask = Modifier.PUBLIC | Modifier.PROTECTED | Modifier.PRIVATE;
+        return method.getModifiers() & mask;
+    }
+
     @Override
     public Class<?> getDeclaringClass() {
         return getterMethod.getDeclaringClass();

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

@@ -1,10 +1,12 @@
 package ai.timefold.solver.core.impl.domain.common.accessor;
 
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatCode;
 
 import ai.timefold.solver.core.api.domain.variable.PlanningVariable;
 import ai.timefold.solver.core.testdomain.TestdataEntity;
 import ai.timefold.solver.core.testdomain.TestdataValue;
+import ai.timefold.solver.core.testdomain.invalid.gettersettervisibility.TestdataDifferentGetterSetterVisibilityEntity;
 
 import org.junit.jupiter.api.Test;
 
@@ -26,4 +28,22 @@ void methodAnnotatedEntity() throws NoSuchMethodException {
         assertThat(e1.getValue()).isSameAs(v2);
     }
 
+    @Test
+    void getterSetterVisibilityDoesNotMatch() {
+        assertThatCode(() -> new ReflectionBeanPropertyMemberAccessor(
+                TestdataDifferentGetterSetterVisibilityEntity.class.getDeclaredMethod("getValue1")))
+                .hasMessageContainingAll("getter method (getValue1)",
+                        "has access modifier (public)",
+                        "not match the setter method's (setValue1)",
+                        "access modifier (private)",
+                        "on class (TestdataDifferentGetterSetterVisibilityEntity)");
+        assertThatCode(() -> new ReflectionBeanPropertyMemberAccessor(
+                TestdataDifferentGetterSetterVisibilityEntity.class.getDeclaredMethod("getValue2")))
+                .hasMessageContainingAll("getter method (getValue2)",
+                        "has access modifier (package-private)",
+                        "not match the setter method's (setValue2)",
+                        "access modifier (protected)",
+                        "on class (TestdataDifferentGetterSetterVisibilityEntity)");
+    }
+
 }

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

@@ -0,0 +1,30 @@
+package ai.timefold.solver.core.testdomain.invalid.gettersettervisibility;
+
+import ai.timefold.solver.core.api.domain.entity.PlanningEntity;
+import ai.timefold.solver.core.api.domain.variable.PlanningVariable;
+import ai.timefold.solver.core.testdomain.TestdataValue;
+
+@PlanningEntity
+public class TestdataDifferentGetterSetterVisibilityEntity {
+    private TestdataValue value1;
+    private TestdataValue value2;
+
+    @PlanningVariable
+    public TestdataValue getValue1() {
+        return value1;
+    }
+
+    private void setValue1(TestdataValue value1) {
+        this.value1 = value1;
+    }
+
+    @PlanningVariable
+    TestdataValue getValue2() {
+        return value2;
+    }
+
+    @PlanningVariable
+    protected TestdataValue setValue2(TestdataValue value2) {
+        return value2;
+    }
+}

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

@@ -75,7 +75,7 @@ public void setEntityList(List<TestdataEntity> entityList) {
     }
 
     @PlanningScore(bendableHardLevelsSize = 1, bendableSoftLevelsSize = 2)
-    BendableBigDecimalScore getScore() {
+    public BendableBigDecimalScore getScore() {
         return score;
     }
 

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

@@ -75,7 +75,7 @@ public void setEntityList(List<TestdataEntity> entityList) {
     }
 
     @PlanningScore(bendableHardLevelsSize = 1, bendableSoftLevelsSize = 2)
-    BendableLongScore getScore() {
+    public BendableLongScore getScore() {
         return score;
     }
 

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

@@ -75,7 +75,7 @@ public void setEntityList(List<TestdataEntity> entityList) {
     }
 
     @PlanningScore(bendableHardLevelsSize = 1, bendableSoftLevelsSize = 2)
-    BendableScore getScore() {
+    public BendableScore getScore() {
         return score;
     }
 

core/src/test/java/ai/timefold/solver/core/testdomain/score/TestdataHardMediumSoftBigDecimalScoreSolution.java

@@ -77,7 +77,7 @@ public void setEntityList(List<TestdataEntity> entityList) {
     }
 
     @PlanningScore
-    HardMediumSoftBigDecimalScore getScore() {
+    public HardMediumSoftBigDecimalScore getScore() {
         return score;
     }
 

core/src/test/java/ai/timefold/solver/core/testdomain/score/TestdataHardMediumSoftLongScoreSolution.java

@@ -75,7 +75,7 @@ public void setEntityList(List<TestdataEntity> entityList) {
     }
 
     @PlanningScore
-    HardMediumSoftLongScore getScore() {
+    public HardMediumSoftLongScore getScore() {
         return score;
     }
 

core/src/test/java/ai/timefold/solver/core/testdomain/score/TestdataHardMediumSoftScoreSolution.java

@@ -75,7 +75,7 @@ public void setEntityList(List<TestdataEntity> entityList) {
     }
 
     @PlanningScore
-    HardMediumSoftScore getScore() {
+    public HardMediumSoftScore getScore() {
         return score;
     }
 

core/src/test/java/ai/timefold/solver/core/testdomain/score/TestdataHardSoftBigDecimalScoreSolution.java

@@ -75,7 +75,7 @@ public void setEntityList(List<TestdataEntity> entityList) {
     }
 
     @PlanningScore
-    HardSoftBigDecimalScore getScore() {
+    public HardSoftBigDecimalScore getScore() {
         return score;
     }
 

core/src/test/java/ai/timefold/solver/core/testdomain/score/TestdataHardSoftLongScoreSolution.java

@@ -75,7 +75,7 @@ public void setEntityList(List<TestdataEntity> entityList) {
     }
 
     @PlanningScore
-    HardSoftLongScore getScore() {
+    public HardSoftLongScore getScore() {
         return score;
     }
 

core/src/test/java/ai/timefold/solver/core/testdomain/score/TestdataHardSoftScoreSolution.java

@@ -75,7 +75,7 @@ public void setEntityList(List<TestdataEntity> entityList) {
     }
 
     @PlanningScore
-    HardSoftScore getScore() {
+    public HardSoftScore getScore() {
         return score;
     }
 

quarkus-integration/quarkus/deployment/src/test/java/ai/timefold/solver/quarkus/TimefoldProcessorGeneratedGizmoSupplierTest.java

@@ -107,9 +107,9 @@ public interface DummyInterfaceEntity {
     public abstract static class DummyAbstractEntity {
         @ShadowVariable(variableListenerClass = DummyVariableListener.class,
                 sourceEntityClass = TestdataEntity.class, sourceVariableName = "value")
-        abstract Integer getLength();
+        public abstract Integer getLength();
 
-        abstract void setLength(Integer length);
+        public abstract void setLength(Integer length);
     }
 
     public static class DummySolutionPartitioner implements SolutionPartitioner<TestdataSolution> {

quarkus-integration/quarkus/deployment/src/test/java/ai/timefold/solver/quarkus/testdomain/gizmo/TestDataKitchenSinkEntity.java

@@ -52,7 +52,7 @@ public class TestDataKitchenSinkEntity {
     private boolean isPinned;
 
     @PlanningVariable(valueRangeProviderRefs = { "ints" })
-    private Integer getIntVariable() {
+    public Integer getIntVariable() {
         return intVariable;
     }