Merge pull request #3251 from graphql-java/static_property_like_methods_should_not_be_used

static record like methods are no longer supported

Author: bbakerman

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 {

src/test/groovy/graphql/schema/PropertyDataFetcherTest.groovy

@@ -33,7 +33,7 @@ class PropertyDataFetcherTest extends Specification {
                 .build()
     }
 
-    class SomeObject {
+    static class SomeObject {
         String value
     }
 
@@ -483,7 +483,7 @@ class PropertyDataFetcherTest extends Specification {
 
     }
 
-    class ProductDTO {
+    static class ProductDTO {
         String name
         String model
     }
@@ -537,7 +537,7 @@ class PropertyDataFetcherTest extends Specification {
     private static class Bar implements Foo {
         @Override
         String getSomething() {
-            return "bar";
+            return "bar"
         }
     }
 
@@ -562,4 +562,133 @@ class PropertyDataFetcherTest extends Specification {
         then:
         result == "bar"
     }
+
+    def "issue 3247 - record like statics should not be used"() {
+        given:
+        def payload = new UpdateOrganizerSubscriptionPayload(true, new OrganizerSubscriptionError())
+        PropertyDataFetcher propertyDataFetcher = new PropertyDataFetcher("success")
+        def dfe = Mock(DataFetchingEnvironment)
+        dfe.getSource() >> payload
+        when:
+        def result = propertyDataFetcher.get(dfe)
+
+        then:
+        result == true
+
+        // repeat - should be cached
+        when:
+        result = propertyDataFetcher.get(dfe)
+
+        then:
+        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
+    }
+
+    def "issue 3247 - getter statics should be found"() {
+        given:
+        def objectInQuestion = new BarClassWithStaticProperties()
+        PropertyDataFetcher propertyDataFetcher = new PropertyDataFetcher("foo")
+        def dfe = Mock(DataFetchingEnvironment)
+        dfe.getSource() >> objectInQuestion
+        when:
+        def result = propertyDataFetcher.get(dfe)
+
+        then:
+        result == "foo"
+
+        // repeat - should be cached
+        when:
+        result = propertyDataFetcher.get(dfe)
+
+        then:
+        result == "foo"
+
+        when:
+        propertyDataFetcher = new PropertyDataFetcher("bar")
+        result = propertyDataFetcher.get(dfe)
+
+        then:
+        result == "bar"
+
+        // repeat - should be cached
+        when:
+        result = propertyDataFetcher.get(dfe)
+
+        then:
+        result == "bar"
+    }
+
+    /**
+     * 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 " }
+    }
+
+    static class UpdateOrganizerSubscriptionPayload {
+        private final Boolean success
+        private final OrganizerSubscriptionError error
+
+        UpdateOrganizerSubscriptionPayload(Boolean success, OrganizerSubscriptionError error) {
+            this.success = success
+            this.error = error
+        }
+
+        static UpdateOrganizerSubscriptionPayload success() {
+            // 👈 note the static factory method for creating a success payload
+            return new UpdateOrganizerSubscriptionPayload(Boolean.TRUE, null)
+        }
+
+        static UpdateOrganizerSubscriptionPayload error(OrganizerSubscriptionError error) {
+            // 👈 note the static factory method for creating a success payload
+            return new UpdateOrganizerSubscriptionPayload(null, error)
+        }
+
+        Boolean getSuccess() {
+            return success
+        }
+
+        OrganizerSubscriptionError getError() {
+            return error
+        }
+
+
+        @Override
+        String toString() {
+            return new StringJoiner(
+                    ", ", UpdateOrganizerSubscriptionPayload.class.getSimpleName() + "[", "]")
+                    .add("success=" + success)
+                    .add("error=" + error)
+                    .toString()
+        }
+    }
+
+    static class FooClassWithStaticProperties {
+        static String getFoo() { return "foo" }
+    }
+
+    static class BarClassWithStaticProperties extends FooClassWithStaticProperties {
+        static String getBar() { return "bar" }
+    }
 }