issue 3247 - now bans static recordLike() methods

bbakerman · bbakerman · commit b0d62dc98858 · 2023-06-25T13:24:41.000+10:00
diff --git a/src/main/java/graphql/schema/PropertyFetchingImpl.java b/src/main/java/graphql/schema/PropertyFetchingImpl.java
@@ -138,7 +138,18 @@ public Object getPropertyValue(String propertyName, Object object, GraphQLType g
         //
         // try by public getters name -  object.getPropertyName()
         try {
-            MethodFinder methodFinder = (rootClass, methodName) -> findPubliclyAccessibleMethod(cacheKey, rootClass, methodName, dfeInUse);
+            MethodFinder methodFinder = (rootClass, methodName) -> findPubliclyAccessibleMethod(cacheKey, rootClass, methodName, dfeInUse,false);
+            return getPropertyViaGetterMethod(object, propertyName, graphQLType, methodFinder, singleArgumentValue);
+        } catch (NoSuchMethodException ignored) {
+        }
+        //
+        // try by public getters name -  object.getPropertyName() where its static
+        try {
+            // we allow static getXXX() methods because we always have.  It's strange in retrospect but
+            // in order to not break things we allow statics to be used.  In theory this double code check is not needed
+            // because you CANT have a `static getFoo()` and a `getFoo()` in the same class hierarchy but to make the code read clearer
+            // I have repeated the lookup.  Since we cache methods, this happens only once and does not slow us down
+            MethodFinder methodFinder = (rootClass, methodName) -> findPubliclyAccessibleMethod(cacheKey, rootClass, methodName, dfeInUse,true);
             return getPropertyViaGetterMethod(object, propertyName, graphQLType, methodFinder, singleArgumentValue);
         } catch (NoSuchMethodException ignored) {
         }
@@ -215,7 +226,7 @@ private Object getPropertyViaGetterUsingPrefix(Object object, String propertyNam
      * which have abstract public interfaces implemented by package-protected
      * (generated) subclasses.
      */
-    private Method findPubliclyAccessibleMethod(CacheKey cacheKey, Class<?> rootClass, String methodName, boolean dfeInUse) throws NoSuchMethodException {
+    private Method findPubliclyAccessibleMethod(CacheKey cacheKey, Class<?> rootClass, String methodName, boolean dfeInUse, boolean allowStaticMethods) throws NoSuchMethodException {
         Class<?> currentClass = rootClass;
         while (currentClass != null) {
             if (Modifier.isPublic(currentClass.getModifiers())) {
@@ -224,7 +235,7 @@ private Method findPubliclyAccessibleMethod(CacheKey cacheKey, Class<?> rootClas
                     // try a getter that takes singleArgumentType first (if we have one)
                     try {
                         Method method = currentClass.getMethod(methodName, singleArgumentType);
-                        if (Modifier.isPublic(method.getModifiers())) {
+                        if (isSuitablePublicMethod(method, allowStaticMethods)) {
                             METHOD_CACHE.putIfAbsent(cacheKey, new CachedMethod(method));
                             return method;
                         }
@@ -233,7 +244,7 @@ private Method findPubliclyAccessibleMethod(CacheKey cacheKey, Class<?> rootClas
                     }
                 }
                 Method method = currentClass.getMethod(methodName);
-                if (Modifier.isPublic(method.getModifiers())) {
+                if (isSuitablePublicMethod(method, allowStaticMethods)) {
                     METHOD_CACHE.putIfAbsent(cacheKey, new CachedMethod(method));
                     return method;
                 }
@@ -244,6 +255,18 @@ private Method findPubliclyAccessibleMethod(CacheKey cacheKey, Class<?> rootClas
         return rootClass.getMethod(methodName);
     }
 
+    private boolean isSuitablePublicMethod(Method method, boolean allowStaticMethods) {
+        int methodModifiers = method.getModifiers();
+        if (Modifier.isPublic(methodModifiers)) {
+            //noinspection RedundantIfStatement
+            if (Modifier.isStatic(methodModifiers) && !allowStaticMethods) {
+                return false;
+            }
+            return true;
+        }
+        return false;
+    }
+
     /*
        https://docs.oracle.com/en/java/javase/15/language/records.html
 
@@ -253,9 +276,11 @@ private Method findPubliclyAccessibleMethod(CacheKey cacheKey, Class<?> rootClas
 
        However, we won't just restrict ourselves strictly to true records.  We will find methods that are record like
        and fetch them - e.g. `object.propertyName()`
+
+       We won't allow static methods for record like methods however
      */
     private Method findRecordMethod(CacheKey cacheKey, Class<?> rootClass, String methodName) throws NoSuchMethodException {
-        return findPubliclyAccessibleMethod(cacheKey, rootClass, methodName, false);
+        return findPubliclyAccessibleMethod(cacheKey, rootClass, methodName, false, false);
     }
 
     private Method findViaSetAccessible(CacheKey cacheKey, Class<?> aClass, String methodName, boolean dfeInUse) throws NoSuchMethodException {
diff --git a/src/test/groovy/graphql/schema/PropertyDataFetcherTest.groovy b/src/test/groovy/graphql/schema/PropertyDataFetcherTest.groovy
@@ -563,7 +563,7 @@ class PropertyDataFetcherTest extends Specification {
         result == "bar"
     }
 
-    def "issue 3247 - statics should not be used"() {
+    def "issue 3247 - record like statics should not be used"() {
         given:
         def payload = new UpdateOrganizerSubscriptionPayload(true, new OrganizerSubscriptionError())
         PropertyDataFetcher propertyDataFetcher = new PropertyDataFetcher("success")
@@ -583,8 +583,30 @@ class PropertyDataFetcherTest extends Specification {
         result == true
     }
 
+    def "issue 3247 - record like statics should not be found"() {
+        given:
+        def errorShape = new OrganizerSubscriptionError()
+        PropertyDataFetcher propertyDataFetcher = new PropertyDataFetcher("message")
+        def dfe = Mock(DataFetchingEnvironment)
+        dfe.getSource() >> errorShape
+        when:
+        def result = propertyDataFetcher.get(dfe)
+
+        then:
+        result == null // not found as its a static recordLike() method
+
+        // repeat - should be cached
+        when:
+        result = propertyDataFetcher.get(dfe)
+
+        then:
+        result == null
+    }
+
     /**
      * Classes from issue to ensure we reproduce as reported by customers
+     *
+     * In the UpdateOrganizerSubscriptionPayload class we will find the getSuccess() because static recordLike() methods are no longer allowed
      */
     static class OrganizerSubscriptionError {
         static String message() { return "error " }
