From 1edbbe80bcdea33a9b462d1b6fcaf349b9a5f923 Mon Sep 17 00:00:00 2001 From: Brad Baker Date: Thu, 15 Jun 2023 21:54:48 +1000 Subject: [PATCH 1/4] issue 3247 - reproduction of reported case --- .../schema/PropertyDataFetcherTest.groovy | 71 ++++++++++++++++++- 1 file changed, 68 insertions(+), 3 deletions(-) diff --git a/src/test/groovy/graphql/schema/PropertyDataFetcherTest.groovy b/src/test/groovy/graphql/schema/PropertyDataFetcherTest.groovy index 3d289c3688..fea3b50e38 100644 --- a/src/test/groovy/graphql/schema/PropertyDataFetcherTest.groovy +++ b/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,69 @@ class PropertyDataFetcherTest extends Specification { then: result == "bar" } + + def "issue 3247 - 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 + } + + /** + * Classes from issue to ensure we reproduce as reported by customers + */ + 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() + } + } } From b0d62dc988587fa43c6439908494aa75c158dcaa Mon Sep 17 00:00:00 2001 From: Brad Baker Date: Sun, 25 Jun 2023 13:24:41 +1000 Subject: [PATCH 2/4] issue 3247 - now bans static recordLike() methods --- .../graphql/schema/PropertyFetchingImpl.java | 35 ++++++++++++++++--- .../schema/PropertyDataFetcherTest.groovy | 24 ++++++++++++- 2 files changed, 53 insertions(+), 6 deletions(-) diff --git a/src/main/java/graphql/schema/PropertyFetchingImpl.java b/src/main/java/graphql/schema/PropertyFetchingImpl.java index 5a7a92989a..9e46a88660 100644 --- 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 index fea3b50e38..be6124c610 100644 --- 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 " } From 6d72ef2dd76a7cf794db4fde265d5c2c5f25540f Mon Sep 17 00:00:00 2001 From: Brad Baker Date: Sun, 25 Jun 2023 13:44:54 +1000 Subject: [PATCH 3/4] issue 3247 - more tests that include static getters --- .../schema/PropertyDataFetcherTest.groovy | 42 +++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/src/test/groovy/graphql/schema/PropertyDataFetcherTest.groovy b/src/test/groovy/graphql/schema/PropertyDataFetcherTest.groovy index be6124c610..f771835575 100644 --- a/src/test/groovy/graphql/schema/PropertyDataFetcherTest.groovy +++ b/src/test/groovy/graphql/schema/PropertyDataFetcherTest.groovy @@ -603,6 +603,40 @@ class PropertyDataFetcherTest extends Specification { 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 * @@ -649,4 +683,12 @@ class PropertyDataFetcherTest extends Specification { .toString() } } + + static class FooClassWithStaticProperties { + static String getFoo() { return "foo" } + } + + static class BarClassWithStaticProperties extends FooClassWithStaticProperties { + static String getBar() { return "bar" } + } } From f059a009f57df844e1220cc5ad9ead8ac5ce021a Mon Sep 17 00:00:00 2001 From: Brad Baker Date: Sun, 25 Jun 2023 13:45:54 +1000 Subject: [PATCH 4/4] issue 3247 - reformat code --- src/main/java/graphql/schema/PropertyFetchingImpl.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/main/java/graphql/schema/PropertyFetchingImpl.java b/src/main/java/graphql/schema/PropertyFetchingImpl.java index 9e46a88660..da8b153e3d 100644 --- a/src/main/java/graphql/schema/PropertyFetchingImpl.java +++ b/src/main/java/graphql/schema/PropertyFetchingImpl.java @@ -138,7 +138,7 @@ 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,false); + MethodFinder methodFinder = (rootClass, methodName) -> findPubliclyAccessibleMethod(cacheKey, rootClass, methodName, dfeInUse, false); return getPropertyViaGetterMethod(object, propertyName, graphQLType, methodFinder, singleArgumentValue); } catch (NoSuchMethodException ignored) { } @@ -149,7 +149,7 @@ public Object getPropertyValue(String propertyName, Object object, GraphQLType g // 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); + MethodFinder methodFinder = (rootClass, methodName) -> findPubliclyAccessibleMethod(cacheKey, rootClass, methodName, dfeInUse, true); return getPropertyViaGetterMethod(object, propertyName, graphQLType, methodFinder, singleArgumentValue); } catch (NoSuchMethodException ignored) { }