diff --git a/src/main/java/graphql/schema/PropertyFetchingImpl.java b/src/main/java/graphql/schema/PropertyFetchingImpl.java index 5a7a92989a..da8b153e3d 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 3d289c3688..f771835575 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,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" } + } }