From 7d7d072f7dcaf87350cb3bc322ab5e015c99dc5e Mon Sep 17 00:00:00 2001 From: Brad Baker Date: Tue, 25 Oct 2022 15:51:27 +1100 Subject: [PATCH 1/7] Record like naming --- .../graphql/schema/PropertyFetchingImpl.java | 84 ++++++++++++++----- .../schema/PropertyDataFetcherTest.groovy | 39 +++++++++ .../schema/somepackage/RecordLikeClass.java | 34 ++++++++ 3 files changed, 137 insertions(+), 20 deletions(-) create mode 100644 src/test/groovy/graphql/schema/somepackage/RecordLikeClass.java diff --git a/src/main/java/graphql/schema/PropertyFetchingImpl.java b/src/main/java/graphql/schema/PropertyFetchingImpl.java index a671388d3..30c1c1637 100644 --- a/src/main/java/graphql/schema/PropertyFetchingImpl.java +++ b/src/main/java/graphql/schema/PropertyFetchingImpl.java @@ -84,27 +84,40 @@ public Object getPropertyValue(String propertyName, Object object, GraphQLType g return null; } // - // ok we haven't cached it and we haven't negatively cached it so we have to find the POJO method which is the most + // ok we haven't cached it, and we haven't negatively cached it, so we have to find the POJO method which is the most // expensive operation here // boolean dfeInUse = singleArgumentValue != null; + // + // try by record name - object.propertyName() + try { + MethodFinder methodFinder = (rootClass, methodName) -> findRecordMethod(cacheKey, rootClass, methodName); + return getPropertyViaRecordMethod(object, propertyName, methodFinder, singleArgumentValue); + } catch (NoSuchMethodException ignored) { + } + // + // try by public getters name - object.getPropertyName() try { - MethodFinder methodFinder = (root, methodName) -> findPubliclyAccessibleMethod(cacheKey, root, methodName, dfeInUse); + MethodFinder methodFinder = (rootClass, methodName) -> findPubliclyAccessibleMethod(cacheKey, rootClass, methodName, dfeInUse); + return getPropertyViaGetterMethod(object, propertyName, graphQLType, methodFinder, singleArgumentValue); + } catch (NoSuchMethodException ignored) { + } + // + // try by accessible getters name - object.getPropertyName() + try { + MethodFinder methodFinder = (aClass, methodName) -> findViaSetAccessible(cacheKey, aClass, methodName, dfeInUse); return getPropertyViaGetterMethod(object, propertyName, graphQLType, methodFinder, singleArgumentValue); } catch (NoSuchMethodException ignored) { - try { - MethodFinder methodFinder = (aClass, methodName) -> findViaSetAccessible(cacheKey, aClass, methodName, dfeInUse); - return getPropertyViaGetterMethod(object, propertyName, graphQLType, methodFinder, singleArgumentValue); - } catch (NoSuchMethodException ignored2) { - try { - return getPropertyViaFieldAccess(cacheKey, object, propertyName); - } catch (FastNoSuchMethodException e) { - // we have nothing to ask for and we have exhausted our lookup strategies - putInNegativeCache(cacheKey); - return null; - } - } } + // + // try by field name - object.getPropertyName() + try { + return getPropertyViaFieldAccess(cacheKey, object, propertyName); + } catch (NoSuchMethodException ignored) { + } + // we have nothing to ask for, and we have exhausted our lookup strategies + putInNegativeCache(cacheKey); + return null; } private boolean isNegativelyCached(CacheKey key) { @@ -124,6 +137,11 @@ private interface MethodFinder { Method apply(Class aClass, String s) throws NoSuchMethodException; } + private Object getPropertyViaRecordMethod(Object object, String propertyName, MethodFinder methodFinder, Object singleArgumentValue) throws NoSuchMethodException { + Method method = methodFinder.apply(object.getClass(), propertyName); + return invokeMethod(object, singleArgumentValue, method, takesSingleArgumentTypeAsOnlyArgument(method)); + } + private Object getPropertyViaGetterMethod(Object object, String propertyName, GraphQLType graphQLType, MethodFinder methodFinder, Object singleArgumentValue) throws NoSuchMethodException { if (isBooleanProperty(graphQLType)) { try { @@ -179,6 +197,28 @@ private Method findPubliclyAccessibleMethod(CacheKey cacheKey, Class rootClas return rootClass.getMethod(methodName); } + /* + https://docs.oracle.com/en/java/javase/15/language/records.html + + A record class declares a sequence of fields, and then the appropriate accessors, constructors, equals, hashCode, and toString methods are created automatically. + + Records cannot extend any class - so we need only check the root class for a publicly declared method with the propertyName + */ + private Method findRecordMethod(CacheKey cacheKey, Class rootClass, String methodName) throws NoSuchMethodException { + if (Modifier.isPublic(rootClass.getModifiers())) { + Method method = rootClass.getDeclaredMethod(methodName); + int modifiers = method.getModifiers(); + if (Modifier.isPublic(modifiers) && + method.getParameterCount() == 0 && + !Modifier.isStatic(modifiers) && + !Modifier.isAbstract(modifiers)) { + METHOD_CACHE.putIfAbsent(cacheKey, new CachedMethod(method)); + return method; + } + } + throw new FastNoSuchMethodException(methodName); + } + private Method findViaSetAccessible(CacheKey cacheKey, Class aClass, String methodName, boolean dfeInUse) throws NoSuchMethodException { if (!USE_SET_ACCESSIBLE.get()) { throw new FastNoSuchMethodException(methodName); @@ -304,8 +344,12 @@ private CacheKey(ClassLoader classLoader, String className, String propertyName) @Override public boolean equals(Object o) { - if (this == o) return true; - if (!(o instanceof CacheKey)) return false; + if (this == o) { + return true; + } + if (!(o instanceof CacheKey)) { + return false; + } CacheKey cacheKey = (CacheKey) o; return Objects.equals(classLoader, cacheKey.classLoader) && Objects.equals(className, cacheKey.className) && Objects.equals(propertyName, cacheKey.propertyName); } @@ -322,10 +366,10 @@ public int hashCode() { @Override public String toString() { return "CacheKey{" + - "classLoader=" + classLoader + - ", className='" + className + '\'' + - ", propertyName='" + propertyName + '\'' + - '}'; + "classLoader=" + classLoader + + ", className='" + className + '\'' + + ", propertyName='" + propertyName + '\'' + + '}'; } } diff --git a/src/test/groovy/graphql/schema/PropertyDataFetcherTest.groovy b/src/test/groovy/graphql/schema/PropertyDataFetcherTest.groovy index 523f0e0da..510b8073e 100644 --- a/src/test/groovy/graphql/schema/PropertyDataFetcherTest.groovy +++ b/src/test/groovy/graphql/schema/PropertyDataFetcherTest.groovy @@ -5,6 +5,7 @@ import graphql.TestUtil import graphql.schema.somepackage.ClassWithDFEMethods import graphql.schema.somepackage.ClassWithInterfaces import graphql.schema.somepackage.ClassWithInteritanceAndInterfaces +import graphql.schema.somepackage.RecordLikeClass import graphql.schema.somepackage.TestClass import graphql.schema.somepackage.TwoClassesDown import spock.lang.Specification @@ -95,6 +96,44 @@ class PropertyDataFetcherTest extends Specification { result == null } + def "fetch via record method"() { + def environment = env(new RecordLikeClass()) + when: + def fetcher = new PropertyDataFetcher("recordProperty") + def result = fetcher.get(environment) + then: + result == "recordProperty" + + // recordArgumentMethod will not work because it takes a parameter + when: + fetcher = new PropertyDataFetcher("recordArgumentMethod") + result = fetcher.get(environment) + then: + result == null + + // equals will not work because it takes a parameter + when: + fetcher = new PropertyDataFetcher("equals") + result = fetcher.get(environment) + then: + result == null + + // we allow hashCode() and toString() because why not - they are valid property names + // they might not be that useful but they can be accessed + + when: + fetcher = new PropertyDataFetcher("hashCode") + result = fetcher.get(environment) + then: + result == 666 + + when: + fetcher = new PropertyDataFetcher("toString") + result = fetcher.get(environment) + then: + result == "toString" + } + def "fetch via public method"() { def environment = env(new TestClass()) def fetcher = new PropertyDataFetcher("publicProperty") diff --git a/src/test/groovy/graphql/schema/somepackage/RecordLikeClass.java b/src/test/groovy/graphql/schema/somepackage/RecordLikeClass.java new file mode 100644 index 000000000..57cc04f2f --- /dev/null +++ b/src/test/groovy/graphql/schema/somepackage/RecordLikeClass.java @@ -0,0 +1,34 @@ +package graphql.schema.somepackage; + +import graphql.schema.DataFetchingEnvironment; + +/** + * This is obviously not an actual record class from Java 14 onwards, but it + * smells like one and that's enough really. Its public, not derived from another + * class and has a public method named after a property + */ +public class RecordLikeClass { + + public String recordProperty() { + return "recordProperty"; + } + + public String recordArgumentMethod(DataFetchingEnvironment environment) { + return "recordArgumentMethod"; + } + + @Override + public int hashCode() { + return 666; + } + + @Override + public boolean equals(Object obj) { + return super.equals(obj); + } + + @Override + public String toString() { + return "toString"; + } +} From 91f703ec279ba0a7035d54985ac20957ebde5309 Mon Sep 17 00:00:00 2001 From: Brad Baker Date: Tue, 25 Oct 2022 16:18:26 +1100 Subject: [PATCH 2/7] Record like naming - tweaks --- src/main/java/graphql/schema/PropertyFetchingImpl.java | 7 +++---- .../groovy/graphql/schema/PropertyDataFetcherTest.groovy | 7 +++++++ 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/src/main/java/graphql/schema/PropertyFetchingImpl.java b/src/main/java/graphql/schema/PropertyFetchingImpl.java index 30c1c1637..6c011acde 100644 --- a/src/main/java/graphql/schema/PropertyFetchingImpl.java +++ b/src/main/java/graphql/schema/PropertyFetchingImpl.java @@ -56,7 +56,7 @@ public Object getPropertyValue(String propertyName, Object object, GraphQLType g } CacheKey cacheKey = mkCacheKey(object, propertyName); - // lets try positive cache mechanisms first. If we have seen the method or field before + // let's try positive cache mechanisms first. If we have seen the method or field before // then we invoke it directly without burning any cycles doing reflection. CachedMethod cachedMethod = METHOD_CACHE.get(cacheKey); if (cachedMethod != null) { @@ -72,9 +72,9 @@ public Object getPropertyValue(String propertyName, Object object, GraphQLType g } // - // if we have tried all strategies before and they have all failed then we negatively cache + // if we have tried all strategies before, and they have all failed then we negatively cache // the cacheKey and assume that it's never going to turn up. This shortcuts the property lookup - // in systems where there was a `foo` graphql property but they never provided an POJO + // in systems where there was a `foo` graphql property, but they never provided an POJO // version of `foo`. // // we do this second because we believe in the positive cached version will mostly prevail @@ -387,7 +387,6 @@ private static Comparator mostMethodArgsFirst() { return Comparator.comparingInt(Method::getParameterCount).reversed(); } - @SuppressWarnings("serial") private static class FastNoSuchMethodException extends NoSuchMethodException { public FastNoSuchMethodException(String methodName) { super(methodName); diff --git a/src/test/groovy/graphql/schema/PropertyDataFetcherTest.groovy b/src/test/groovy/graphql/schema/PropertyDataFetcherTest.groovy index 510b8073e..bde5618a7 100644 --- a/src/test/groovy/graphql/schema/PropertyDataFetcherTest.groovy +++ b/src/test/groovy/graphql/schema/PropertyDataFetcherTest.groovy @@ -104,6 +104,13 @@ class PropertyDataFetcherTest extends Specification { then: result == "recordProperty" + // caching works + when: + fetcher = new PropertyDataFetcher("recordProperty") + result = fetcher.get(environment) + then: + result == "recordProperty" + // recordArgumentMethod will not work because it takes a parameter when: fetcher = new PropertyDataFetcher("recordArgumentMethod") From 0cafaaef221b43bdcfc60e5e849e0bb3273391aa Mon Sep 17 00:00:00 2001 From: Brad Baker Date: Tue, 25 Oct 2022 16:51:30 +1100 Subject: [PATCH 3/7] Record like naming - javadoc tweaks --- src/main/java/graphql/schema/PropertyDataFetcher.java | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/main/java/graphql/schema/PropertyDataFetcher.java b/src/main/java/graphql/schema/PropertyDataFetcher.java index f228f1986..4d6f37112 100644 --- a/src/main/java/graphql/schema/PropertyDataFetcher.java +++ b/src/main/java/graphql/schema/PropertyDataFetcher.java @@ -8,18 +8,18 @@ import java.util.function.Function; /** - * This is the default data fetcher used in graphql-java. It will examine - * maps and POJO java beans for values that match the desired name, typically the field name + * This is the default data fetcher used in graphql-java, and it will examine + * maps, records and POJO java beans for values that match the desired name, typically the field name, * or it will use a provided function to obtain values. - * maps and POJO java beans for values that match the desired name. *

* It uses the following strategies *

    *
  • If the source is null, return null
    • *
  • If the source is a Map, return map.get(propertyName)
    • *
  • If a function is provided, it is used
    • - *
  • Find a public JavaBean getter method named `propertyName`
    • - *
  • Find any getter method named `propertyName` and call method.setAccessible(true)
    • + *
  • Find a public Record like method named `propertyName()`
    • + *
  • Find a public JavaBean getter method named `getPropertyName()` or `isPropertyName()`
    • + *
  • Find any getter method named `getPropertyName()` or `isPropertyName()` and call method.setAccessible(true)
    • *
  • Find a public field named `propertyName`
    • *
  • Find any field named `propertyName` and call field.setAccessible(true)
    • *
  • If this cant find anything, then null is returned
    • From ebc445ef8b69ff686c8495d76450852d4382d68f Mon Sep 17 00:00:00 2001 From: Brad Baker Date: Tue, 1 Nov 2022 17:54:58 +1100 Subject: [PATCH 4/7] Updated record like code including the Lambda mechanism --- .../graphql/schema/PropertyFetchingImpl.java | 34 ++++++++----------- .../fetching/LambdaFetchingSupport.java | 28 +++++++++++---- .../schema/PropertyDataFetcherTest.groovy | 10 ++++++ .../graphql/schema/fetching/ConfusedPojo.java | 12 +++++++ .../fetching/LambdaFetchingSupportTest.groovy | 19 +++++++++++ .../groovy/graphql/schema/fetching/Pojo.java | 4 +++ .../somepackage/RecordLikeTwoClassesDown.java | 4 +++ 7 files changed, 85 insertions(+), 26 deletions(-) create mode 100644 src/test/groovy/graphql/schema/fetching/ConfusedPojo.java create mode 100644 src/test/groovy/graphql/schema/somepackage/RecordLikeTwoClassesDown.java diff --git a/src/main/java/graphql/schema/PropertyFetchingImpl.java b/src/main/java/graphql/schema/PropertyFetchingImpl.java index f3cbee2cb..1f0b53854 100644 --- a/src/main/java/graphql/schema/PropertyFetchingImpl.java +++ b/src/main/java/graphql/schema/PropertyFetchingImpl.java @@ -114,13 +114,6 @@ public Object getPropertyValue(String propertyName, Object object, GraphQLType g boolean dfeInUse = singleArgumentValue != null; // - // try by record name - object.propertyName() - try { - MethodFinder methodFinder = (rootClass, methodName) -> findRecordMethod(cacheKey, rootClass, methodName); - return getPropertyViaRecordMethod(object, propertyName, methodFinder, singleArgumentValue); - } catch (NoSuchMethodException ignored) { - } - // // try by public getters name - object.getPropertyName() try { MethodFinder methodFinder = (rootClass, methodName) -> findPubliclyAccessibleMethod(cacheKey, rootClass, methodName, dfeInUse); @@ -135,11 +128,20 @@ public Object getPropertyValue(String propertyName, Object object, GraphQLType g } catch (NoSuchMethodException ignored) { } // - // try by field name - object.getPropertyName() + // try by field name - object.propertyName; try { return getPropertyViaFieldAccess(cacheKey, object, propertyName); } catch (NoSuchMethodException ignored) { } + // + // try by record name - object.propertyName() + try { + // we do records last because if there was ever a previous situation where there was a `getProp()` and a `prop()` in place + // then previously it would use the `getProp()` method so we want that same behavior + MethodFinder methodFinder = (rootClass, methodName) -> findRecordMethod(cacheKey, rootClass, methodName); + return getPropertyViaRecordMethod(object, propertyName, methodFinder, singleArgumentValue); + } catch (NoSuchMethodException ignored) { + } // we have nothing to ask for, and we have exhausted our lookup strategies putInNegativeCache(cacheKey); return null; @@ -228,20 +230,12 @@ private Method findPubliclyAccessibleMethod(CacheKey cacheKey, Class rootClas A record class declares a sequence of fields, and then the appropriate accessors, constructors, equals, hashCode, and toString methods are created automatically. Records cannot extend any class - so we need only check the root class for a publicly declared method with the propertyName + + 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()` */ private Method findRecordMethod(CacheKey cacheKey, Class rootClass, String methodName) throws NoSuchMethodException { - if (Modifier.isPublic(rootClass.getModifiers())) { - Method method = rootClass.getDeclaredMethod(methodName); - int modifiers = method.getModifiers(); - if (Modifier.isPublic(modifiers) && - method.getParameterCount() == 0 && - !Modifier.isStatic(modifiers) && - !Modifier.isAbstract(modifiers)) { - METHOD_CACHE.putIfAbsent(cacheKey, new CachedMethod(method)); - return method; - } - } - throw new FastNoSuchMethodException(methodName); + return findPubliclyAccessibleMethod(cacheKey,rootClass,methodName,false); } private Method findViaSetAccessible(CacheKey cacheKey, Class aClass, String methodName, boolean dfeInUse) throws NoSuchMethodException { diff --git a/src/main/java/graphql/schema/fetching/LambdaFetchingSupport.java b/src/main/java/graphql/schema/fetching/LambdaFetchingSupport.java index 9095c66c0..6b366bc2a 100644 --- a/src/main/java/graphql/schema/fetching/LambdaFetchingSupport.java +++ b/src/main/java/graphql/schema/fetching/LambdaFetchingSupport.java @@ -15,6 +15,7 @@ import java.util.List; import java.util.Optional; import java.util.function.Function; +import java.util.function.Predicate; import static java.util.stream.Collectors.toList; @@ -42,7 +43,13 @@ public static Optional> createGetter(Class sourceCla Function getterFunction = mkCallFunction(sourceClass, candidateMethod.getName(), candidateMethod.getReturnType()); return Optional.of(getterFunction); } catch (Throwable ignore) { + // // if we cant make a dynamic lambda here, then we give up and let the old property fetching code do its thing + // this can happen on runtimes such as GraalVM native where LambdaMetafactory is not supported + // and will throw something like : + // + // com.oracle.svm.core.jdk.UnsupportedFeatureError: Defining hidden classes at runtime is not supported. + // at org.graalvm.nativeimage.builder/com.oracle.svm.core.util.VMError.unsupportedFeature(VMError.java:89) } } return Optional.empty(); @@ -50,7 +57,7 @@ public static Optional> createGetter(Class sourceCla private static Method getCandidateMethod(Class sourceClass, String propertyName) { - List allGetterMethods = findGetterMethodsForProperty(sourceClass, propertyName); + List allGetterMethods = findMethodsForProperty(sourceClass, propertyName, LambdaFetchingSupport::isGetterNamed); List pojoGetterMethods = allGetterMethods.stream() .filter(LambdaFetchingSupport::isPossiblePojoMethod) .collect(toList()); @@ -60,10 +67,12 @@ private static Method getCandidateMethod(Class sourceClass, String propertyNa method = findBestBooleanGetter(pojoGetterMethods); } return checkForSingleParameterPeer(method, allGetterMethods); - } else { - return null; } - + List recordLikeMethods = findMethodsForProperty(sourceClass, propertyName, LambdaFetchingSupport::isRecordLike); + if (!recordLikeMethods.isEmpty()) { + return recordLikeMethods.get(0); + } + return null; } private static Method checkForSingleParameterPeer(Method candidateMethod, List allMethods) { @@ -93,13 +102,13 @@ private static Method findBestBooleanGetter(List methods) { * * @return a list of getter methods for that property */ - private static List findGetterMethodsForProperty(Class sourceClass, String propertyName) { + private static List findMethodsForProperty(Class sourceClass, String propertyName, Predicate predicate) { List methods = new ArrayList<>(); Class currentClass = sourceClass; while (currentClass != null) { Method[] declaredMethods = currentClass.getDeclaredMethods(); for (Method declaredMethod : declaredMethods) { - if (isGetterNamed(declaredMethod)) { + if (predicate.test(declaredMethod)) { if (nameMatches(propertyName, declaredMethod)) { methods.add(declaredMethod); } @@ -127,6 +136,13 @@ private static boolean isPossiblePojoMethod(Method method) { isPublic(method); } + private static boolean isRecordLike(Method method) { + return !isObjectMethod(method) && + returnsSomething(method) && + hasNoParameters(method) && + isPublic(method); + } + private static boolean isBooleanGetter(Method method) { Class returnType = method.getReturnType(); return isGetterNamed(method) && (returnType.equals(Boolean.class) || returnType.equals(Boolean.TYPE)); diff --git a/src/test/groovy/graphql/schema/PropertyDataFetcherTest.groovy b/src/test/groovy/graphql/schema/PropertyDataFetcherTest.groovy index b4309d81d..d50fe51c7 100644 --- a/src/test/groovy/graphql/schema/PropertyDataFetcherTest.groovy +++ b/src/test/groovy/graphql/schema/PropertyDataFetcherTest.groovy @@ -6,6 +6,7 @@ import graphql.schema.somepackage.ClassWithDFEMethods import graphql.schema.somepackage.ClassWithInterfaces import graphql.schema.somepackage.ClassWithInteritanceAndInterfaces import graphql.schema.somepackage.RecordLikeClass +import graphql.schema.somepackage.RecordLikeTwoClassesDown import graphql.schema.somepackage.TestClass import graphql.schema.somepackage.TwoClassesDown import spock.lang.Specification @@ -141,6 +142,15 @@ class PropertyDataFetcherTest extends Specification { result == "toString" } + def "can fetch record like methods that are public and on super classes"() { + def environment = env(new RecordLikeTwoClassesDown()) + when: + def fetcher = new PropertyDataFetcher("recordProperty") + def result = fetcher.get(environment) + then: + result == "recordProperty" + } + def "fetch via public method"() { def environment = env(new TestClass()) def fetcher = new PropertyDataFetcher("publicProperty") diff --git a/src/test/groovy/graphql/schema/fetching/ConfusedPojo.java b/src/test/groovy/graphql/schema/fetching/ConfusedPojo.java new file mode 100644 index 000000000..8deedf7d0 --- /dev/null +++ b/src/test/groovy/graphql/schema/fetching/ConfusedPojo.java @@ -0,0 +1,12 @@ +package graphql.schema.fetching; + +public class ConfusedPojo { + + public String getRecordLike() { + return "getRecordLike"; + } + + public String recordLike() { + return "recordLike"; + } +} diff --git a/src/test/groovy/graphql/schema/fetching/LambdaFetchingSupportTest.groovy b/src/test/groovy/graphql/schema/fetching/LambdaFetchingSupportTest.groovy index 19b78cbd0..cabc1505b 100644 --- a/src/test/groovy/graphql/schema/fetching/LambdaFetchingSupportTest.groovy +++ b/src/test/groovy/graphql/schema/fetching/LambdaFetchingSupportTest.groovy @@ -32,6 +32,25 @@ class LambdaFetchingSupportTest extends Specification { } + def "get make getters based on record like names"() { + def pojo = new Pojo("Brad", 42) + when: + def getter = LambdaFetchingSupport.createGetter(Pojo.class, "recordLike") + then: + getter.isPresent() + getter.get().apply(pojo) == "recordLike" + + // + // pojo getters will be found first - to prevent escalation from the old way to the new record like way + def confusedPojo = new ConfusedPojo() + when: + getter = LambdaFetchingSupport.createGetter(ConfusedPojo.class, "recordLike") + then: + getter.isPresent() + getter.get().apply(confusedPojo) == "getRecordLike" + + } + def "will handle bad methods and missing ones"() { when: diff --git a/src/test/groovy/graphql/schema/fetching/Pojo.java b/src/test/groovy/graphql/schema/fetching/Pojo.java index 3b478f071..dac6ce914 100644 --- a/src/test/groovy/graphql/schema/fetching/Pojo.java +++ b/src/test/groovy/graphql/schema/fetching/Pojo.java @@ -65,4 +65,8 @@ public String is() { return "is"; } + public String recordLike() { + return "recordLike"; + } + } \ No newline at end of file diff --git a/src/test/groovy/graphql/schema/somepackage/RecordLikeTwoClassesDown.java b/src/test/groovy/graphql/schema/somepackage/RecordLikeTwoClassesDown.java new file mode 100644 index 000000000..4e744f287 --- /dev/null +++ b/src/test/groovy/graphql/schema/somepackage/RecordLikeTwoClassesDown.java @@ -0,0 +1,4 @@ +package graphql.schema.somepackage; + +public class RecordLikeTwoClassesDown extends RecordLikeClass { +} From 759b1fa209c7ee609cc012d14fa930f97a5d11aa Mon Sep 17 00:00:00 2001 From: Brad Baker Date: Tue, 1 Nov 2022 20:12:50 +1100 Subject: [PATCH 5/7] Added more tests and a flag to turn Lambda off --- .../schema/PropertyDataFetcherHelper.java | 6 ++++++ .../graphql/schema/PropertyFetchingImpl.java | 13 +++++++++++- .../schema/PropertyDataFetcherTest.groovy | 20 +++++++++++++++++++ 3 files changed, 38 insertions(+), 1 deletion(-) diff --git a/src/main/java/graphql/schema/PropertyDataFetcherHelper.java b/src/main/java/graphql/schema/PropertyDataFetcherHelper.java index a7223763f..03e7b8ec7 100644 --- a/src/main/java/graphql/schema/PropertyDataFetcherHelper.java +++ b/src/main/java/graphql/schema/PropertyDataFetcherHelper.java @@ -1,6 +1,7 @@ package graphql.schema; import graphql.Internal; +import graphql.VisibleForTesting; /** * This class is the guts of a property data fetcher and also used in AST code to turn @@ -27,6 +28,11 @@ public static boolean setUseSetAccessible(boolean flag) { return impl.setUseSetAccessible(flag); } + @VisibleForTesting + public static boolean setUseLambdaFactory(boolean flag) { + return impl.setUseLambdaFactory(flag); + } + public static boolean setUseNegativeCache(boolean flag) { return impl.setUseNegativeCache(flag); } diff --git a/src/main/java/graphql/schema/PropertyFetchingImpl.java b/src/main/java/graphql/schema/PropertyFetchingImpl.java index 1f0b53854..8d32abc47 100644 --- a/src/main/java/graphql/schema/PropertyFetchingImpl.java +++ b/src/main/java/graphql/schema/PropertyFetchingImpl.java @@ -31,6 +31,7 @@ public class PropertyFetchingImpl { private final AtomicBoolean USE_SET_ACCESSIBLE = new AtomicBoolean(true); + private final AtomicBoolean USE_LAMBDA_FACTORY = new AtomicBoolean(true); private final AtomicBoolean USE_NEGATIVE_CACHE = new AtomicBoolean(true); private final ConcurrentMap LAMBDA_CACHE = new ConcurrentHashMap<>(); private final ConcurrentMap METHOD_CACHE = new ConcurrentHashMap<>(); @@ -104,7 +105,7 @@ public Object getPropertyValue(String propertyName, Object object, GraphQLType g // expensive operation here // - Optional> getterOpt = LambdaFetchingSupport.createGetter(object.getClass(), propertyName); + Optional> getterOpt = lambdaGetter(propertyName, object); if (getterOpt.isPresent()) { Function getter = getterOpt.get(); cachedFunction = new CachedLambdaFunction(getter); @@ -147,6 +148,13 @@ public Object getPropertyValue(String propertyName, Object object, GraphQLType g return null; } + private Optional> lambdaGetter(String propertyName, Object object) { + if (USE_LAMBDA_FACTORY.get()) { + return LambdaFetchingSupport.createGetter(object.getClass(), propertyName); + } + return Optional.empty(); + } + private boolean isNegativelyCached(CacheKey key) { if (USE_NEGATIVE_CACHE.get()) { return NEGATIVE_CACHE.containsKey(key); @@ -340,6 +348,9 @@ public void clearReflectionCache() { public boolean setUseSetAccessible(boolean flag) { return USE_SET_ACCESSIBLE.getAndSet(flag); } + public boolean setUseLambdaFactory(boolean flag) { + return USE_LAMBDA_FACTORY.getAndSet(flag); + } public boolean setUseNegativeCache(boolean flag) { return USE_NEGATIVE_CACHE.getAndSet(flag); diff --git a/src/test/groovy/graphql/schema/PropertyDataFetcherTest.groovy b/src/test/groovy/graphql/schema/PropertyDataFetcherTest.groovy index d50fe51c7..2faea7dee 100644 --- a/src/test/groovy/graphql/schema/PropertyDataFetcherTest.groovy +++ b/src/test/groovy/graphql/schema/PropertyDataFetcherTest.groovy @@ -22,6 +22,7 @@ class PropertyDataFetcherTest extends Specification { PropertyDataFetcher.setUseSetAccessible(true) PropertyDataFetcher.setUseNegativeCache(true) PropertyDataFetcher.clearReflectionCache() + PropertyDataFetcherHelper.setUseLambdaFactory(true) } def env(obj) { @@ -151,6 +152,25 @@ class PropertyDataFetcherTest extends Specification { result == "recordProperty" } + def "fetch via record method without lambda support"() { + PropertyDataFetcherHelper.setUseLambdaFactory(false) + PropertyDataFetcherHelper.clearReflectionCache() + + when: + def environment = env(new RecordLikeClass()) + def fetcher = new PropertyDataFetcher("recordProperty") + def result = fetcher.get(environment) + then: + result == "recordProperty" + + when: + environment = env(new RecordLikeTwoClassesDown()) + fetcher = new PropertyDataFetcher("recordProperty") + result = fetcher.get(environment) + then: + result == "recordProperty" + } + def "fetch via public method"() { def environment = env(new TestClass()) def fetcher = new PropertyDataFetcher("publicProperty") From 2921e63ee4e213afa7b640f22606c358378a966d Mon Sep 17 00:00:00 2001 From: Brad Baker Date: Tue, 1 Nov 2022 20:24:33 +1100 Subject: [PATCH 6/7] JavaDoc update --- src/main/java/graphql/schema/PropertyDataFetcher.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/graphql/schema/PropertyDataFetcher.java b/src/main/java/graphql/schema/PropertyDataFetcher.java index 4d6f37112..385589733 100644 --- a/src/main/java/graphql/schema/PropertyDataFetcher.java +++ b/src/main/java/graphql/schema/PropertyDataFetcher.java @@ -17,11 +17,11 @@ *
  • If the source is null, return null
    • *
  • If the source is a Map, return map.get(propertyName)
    • *
  • If a function is provided, it is used
    • - *
  • Find a public Record like method named `propertyName()`
    • *
  • Find a public JavaBean getter method named `getPropertyName()` or `isPropertyName()`
    • *
  • Find any getter method named `getPropertyName()` or `isPropertyName()` and call method.setAccessible(true)
    • *
  • Find a public field named `propertyName`
    • *
  • Find any field named `propertyName` and call field.setAccessible(true)
    • + *
  • Find a public Record like method named `propertyName()`
    • *
  • If this cant find anything, then null is returned
    • *
*

From c3a69e22560b9058126bde01d15d37673b771a1f Mon Sep 17 00:00:00 2001 From: Brad Baker Date: Mon, 21 Nov 2022 12:57:21 +1100 Subject: [PATCH 7/7] More work on record like access Moved record like to the front of the search pattern --- .../graphql/schema/PropertyFetchingImpl.java | 16 +++---- .../fetching/LambdaFetchingSupport.java | 34 +++++++------- .../schema/PropertyDataFetcherTest.groovy | 13 ++++++ .../graphql/schema/fetching/ConfusedPojo.java | 16 +++++++ .../fetching/LambdaFetchingSupportTest.groovy | 46 ++++++++++++++++--- .../SchemaGeneratorDirectiveHelperTest.groovy | 2 + 6 files changed, 93 insertions(+), 34 deletions(-) diff --git a/src/main/java/graphql/schema/PropertyFetchingImpl.java b/src/main/java/graphql/schema/PropertyFetchingImpl.java index 8d32abc47..56d4984a3 100644 --- a/src/main/java/graphql/schema/PropertyFetchingImpl.java +++ b/src/main/java/graphql/schema/PropertyFetchingImpl.java @@ -115,6 +115,13 @@ public Object getPropertyValue(String propertyName, Object object, GraphQLType g boolean dfeInUse = singleArgumentValue != null; // + // try by record like name - object.propertyName() + try { + MethodFinder methodFinder = (rootClass, methodName) -> findRecordMethod(cacheKey, rootClass, methodName); + return getPropertyViaRecordMethod(object, propertyName, methodFinder, singleArgumentValue); + } catch (NoSuchMethodException ignored) { + } + // // try by public getters name - object.getPropertyName() try { MethodFinder methodFinder = (rootClass, methodName) -> findPubliclyAccessibleMethod(cacheKey, rootClass, methodName, dfeInUse); @@ -134,15 +141,6 @@ public Object getPropertyValue(String propertyName, Object object, GraphQLType g return getPropertyViaFieldAccess(cacheKey, object, propertyName); } catch (NoSuchMethodException ignored) { } - // - // try by record name - object.propertyName() - try { - // we do records last because if there was ever a previous situation where there was a `getProp()` and a `prop()` in place - // then previously it would use the `getProp()` method so we want that same behavior - MethodFinder methodFinder = (rootClass, methodName) -> findRecordMethod(cacheKey, rootClass, methodName); - return getPropertyViaRecordMethod(object, propertyName, methodFinder, singleArgumentValue); - } catch (NoSuchMethodException ignored) { - } // we have nothing to ask for, and we have exhausted our lookup strategies putInNegativeCache(cacheKey); return null; diff --git a/src/main/java/graphql/schema/fetching/LambdaFetchingSupport.java b/src/main/java/graphql/schema/fetching/LambdaFetchingSupport.java index 6b366bc2a..0056819b3 100644 --- a/src/main/java/graphql/schema/fetching/LambdaFetchingSupport.java +++ b/src/main/java/graphql/schema/fetching/LambdaFetchingSupport.java @@ -57,7 +57,18 @@ public static Optional> createGetter(Class sourceCla private static Method getCandidateMethod(Class sourceClass, String propertyName) { - List allGetterMethods = findMethodsForProperty(sourceClass, propertyName, LambdaFetchingSupport::isGetterNamed); + // property() methods first + Predicate recordLikePredicate = method -> isRecordLike(method) && propertyName.equals(decapitalize(method.getName())); + List recordLikeMethods = findMethodsForProperty(sourceClass, + recordLikePredicate); + if (!recordLikeMethods.isEmpty()) { + return recordLikeMethods.get(0); + } + + // getProperty() POJO methods next + Predicate getterPredicate = method -> isGetterNamed(method) && propertyName.equals(mkPropertyNameGetter(method)); + List allGetterMethods = findMethodsForProperty(sourceClass, + getterPredicate); List pojoGetterMethods = allGetterMethods.stream() .filter(LambdaFetchingSupport::isPossiblePojoMethod) .collect(toList()); @@ -68,10 +79,6 @@ private static Method getCandidateMethod(Class sourceClass, String propertyNa } return checkForSingleParameterPeer(method, allGetterMethods); } - List recordLikeMethods = findMethodsForProperty(sourceClass, propertyName, LambdaFetchingSupport::isRecordLike); - if (!recordLikeMethods.isEmpty()) { - return recordLikeMethods.get(0); - } return null; } @@ -97,21 +104,18 @@ private static Method findBestBooleanGetter(List methods) { /** * Finds all methods in a class hierarchy that match the property name - they might not be suitable but they * - * @param sourceClass the class we are looking to work on - * @param propertyName the name of the property + * @param sourceClass the class we are looking to work on * * @return a list of getter methods for that property */ - private static List findMethodsForProperty(Class sourceClass, String propertyName, Predicate predicate) { + private static List findMethodsForProperty(Class sourceClass, Predicate predicate) { List methods = new ArrayList<>(); Class currentClass = sourceClass; while (currentClass != null) { Method[] declaredMethods = currentClass.getDeclaredMethods(); for (Method declaredMethod : declaredMethods) { if (predicate.test(declaredMethod)) { - if (nameMatches(propertyName, declaredMethod)) { - methods.add(declaredMethod); - } + methods.add(declaredMethod); } } currentClass = currentClass.getSuperclass(); @@ -122,12 +126,6 @@ private static List findMethodsForProperty(Class sourceClass, String .collect(toList()); } - - private static boolean nameMatches(String propertyName, Method declaredMethod) { - String methodPropName = mkPropertyName(declaredMethod); - return propertyName.equals(methodPropName); - } - private static boolean isPossiblePojoMethod(Method method) { return !isObjectMethod(method) && returnsSomething(method) && @@ -169,7 +167,7 @@ private static boolean isObjectMethod(Method method) { return method.getDeclaringClass().equals(Object.class); } - private static String mkPropertyName(Method method) { + private static String mkPropertyNameGetter(Method method) { // // getFooName becomes fooName // isFoo becomes foo diff --git a/src/test/groovy/graphql/schema/PropertyDataFetcherTest.groovy b/src/test/groovy/graphql/schema/PropertyDataFetcherTest.groovy index 2faea7dee..3d289c368 100644 --- a/src/test/groovy/graphql/schema/PropertyDataFetcherTest.groovy +++ b/src/test/groovy/graphql/schema/PropertyDataFetcherTest.groovy @@ -2,6 +2,7 @@ package graphql.schema import graphql.ExecutionInput import graphql.TestUtil +import graphql.schema.fetching.ConfusedPojo import graphql.schema.somepackage.ClassWithDFEMethods import graphql.schema.somepackage.ClassWithInterfaces import graphql.schema.somepackage.ClassWithInteritanceAndInterfaces @@ -171,6 +172,18 @@ class PropertyDataFetcherTest extends Specification { result == "recordProperty" } + def "fetch via record method without lambda support in preference to getter methods"() { + PropertyDataFetcherHelper.setUseLambdaFactory(false) + PropertyDataFetcherHelper.clearReflectionCache() + + when: + def environment = env(new ConfusedPojo()) + def fetcher = new PropertyDataFetcher("recordLike") + def result = fetcher.get(environment) + then: + result == "recordLike" + } + def "fetch via public method"() { def environment = env(new TestClass()) def fetcher = new PropertyDataFetcher("publicProperty") diff --git a/src/test/groovy/graphql/schema/fetching/ConfusedPojo.java b/src/test/groovy/graphql/schema/fetching/ConfusedPojo.java index 8deedf7d0..aa0bc174c 100644 --- a/src/test/groovy/graphql/schema/fetching/ConfusedPojo.java +++ b/src/test/groovy/graphql/schema/fetching/ConfusedPojo.java @@ -9,4 +9,20 @@ public String getRecordLike() { public String recordLike() { return "recordLike"; } + + public String gettingConfused() { + return "gettingConfused"; + } + + public String getTingConfused() { + return "getTingConfused"; + } + + public boolean issues() { + return true; + } + + public boolean isSues() { + return false; + } } diff --git a/src/test/groovy/graphql/schema/fetching/LambdaFetchingSupportTest.groovy b/src/test/groovy/graphql/schema/fetching/LambdaFetchingSupportTest.groovy index cabc1505b..d3a68f992 100644 --- a/src/test/groovy/graphql/schema/fetching/LambdaFetchingSupportTest.groovy +++ b/src/test/groovy/graphql/schema/fetching/LambdaFetchingSupportTest.groovy @@ -41,33 +41,65 @@ class LambdaFetchingSupportTest extends Specification { getter.get().apply(pojo) == "recordLike" // - // pojo getters will be found first - to prevent escalation from the old way to the new record like way + // record like getters will be found first - this is new behavior but more sensible behavior def confusedPojo = new ConfusedPojo() when: getter = LambdaFetchingSupport.createGetter(ConfusedPojo.class, "recordLike") then: getter.isPresent() - getter.get().apply(confusedPojo) == "getRecordLike" + getter.get().apply(confusedPojo) == "recordLike" + + // weird arse getter methods like `issues` versus `isSues` + when: + getter = LambdaFetchingSupport.createGetter(ConfusedPojo.class, "gettingConfused") + then: + getter.isPresent() + getter.get().apply(confusedPojo) == "gettingConfused" + + when: + getter = LambdaFetchingSupport.createGetter(ConfusedPojo.class, "tingConfused") + then: + getter.isPresent() + getter.get().apply(confusedPojo) == "getTingConfused" + + // weird arse getter methods like `issues` versus `isSues` + when: + getter = LambdaFetchingSupport.createGetter(ConfusedPojo.class, "issues") + then: + getter.isPresent() + getter.get().apply(confusedPojo) == true + + when: + getter = LambdaFetchingSupport.createGetter(ConfusedPojo.class, "sues") + then: + getter.isPresent() + getter.get().apply(confusedPojo) == false } - def "will handle bad methods and missing ones"() { + def "will handle missing ones"() { when: def getter = LambdaFetchingSupport.createGetter(Pojo.class, "nameX") then: !getter.isPresent() + } + + def "will handle weird ones"() { + + def pojo = new Pojo("Brad", 42) when: - getter = LambdaFetchingSupport.createGetter(Pojo.class, "get") + def getter = LambdaFetchingSupport.createGetter(Pojo.class, "get") then: - !getter.isPresent() + getter.isPresent() + getter.get().apply(pojo) == "get" when: getter = LambdaFetchingSupport.createGetter(Pojo.class, "is") then: - !getter.isPresent() - + getter.isPresent() + getter.get().apply(pojo) == "is" } def "can handle boolean setters - is by preference"() { diff --git a/src/test/groovy/graphql/schema/idl/SchemaGeneratorDirectiveHelperTest.groovy b/src/test/groovy/graphql/schema/idl/SchemaGeneratorDirectiveHelperTest.groovy index f021c6542..e54df617c 100644 --- a/src/test/groovy/graphql/schema/idl/SchemaGeneratorDirectiveHelperTest.groovy +++ b/src/test/groovy/graphql/schema/idl/SchemaGeneratorDirectiveHelperTest.groovy @@ -14,6 +14,7 @@ import graphql.schema.FieldCoordinates import graphql.schema.GraphQLAppliedDirective import graphql.schema.GraphQLArgument import graphql.schema.GraphQLCodeRegistry +import graphql.schema.GraphQLDirectiveContainer import graphql.schema.GraphQLEnumType import graphql.schema.GraphQLEnumValueDefinition import graphql.schema.GraphQLFieldDefinition @@ -21,6 +22,7 @@ import graphql.schema.GraphQLFieldsContainer import graphql.schema.GraphQLInputObjectField import graphql.schema.GraphQLInputObjectType import graphql.schema.GraphQLInterfaceType +import graphql.schema.GraphQLNamedType import graphql.schema.GraphQLObjectType import graphql.schema.GraphQLScalarType import graphql.schema.GraphQLUnionType