diff --git a/src/main/java/graphql/schema/PropertyFetchingImpl.java b/src/main/java/graphql/schema/PropertyFetchingImpl.java index a671388d3b..298063cbc4 100644 --- a/src/main/java/graphql/schema/PropertyFetchingImpl.java +++ b/src/main/java/graphql/schema/PropertyFetchingImpl.java @@ -2,6 +2,7 @@ import graphql.GraphQLException; import graphql.Internal; +import graphql.schema.fetching.LambdaFetchingSupport; import java.lang.reflect.Field; import java.lang.reflect.InvocationTargetException; @@ -15,6 +16,7 @@ import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ConcurrentMap; import java.util.concurrent.atomic.AtomicBoolean; +import java.util.function.Function; import java.util.function.Predicate; import static graphql.Assert.assertShouldNeverHappen; @@ -30,6 +32,7 @@ public class PropertyFetchingImpl { private final AtomicBoolean USE_SET_ACCESSIBLE = 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<>(); private final ConcurrentMap FIELD_CACHE = new ConcurrentHashMap<>(); private final ConcurrentMap NEGATIVE_CACHE = new ConcurrentHashMap<>(); @@ -39,15 +42,22 @@ public PropertyFetchingImpl(Class singleArgumentType) { this.singleArgumentType = singleArgumentType; } - private class CachedMethod { - Method method; - boolean takesSingleArgumentTypeAsOnlyArgument; + private final class CachedMethod { + private final Method method; + private final boolean takesSingleArgumentTypeAsOnlyArgument; CachedMethod(Method method) { this.method = method; this.takesSingleArgumentTypeAsOnlyArgument = takesSingleArgumentTypeAsOnlyArgument(method); } + } + + private static final class CachedLambdaFunction { + private final Function getter; + CachedLambdaFunction(Function getter) { + this.getter = getter; + } } public Object getPropertyValue(String propertyName, Object object, GraphQLType graphQLType, Object singleArgumentValue) { @@ -56,8 +66,13 @@ 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. + CachedLambdaFunction cachedFunction = LAMBDA_CACHE.get(cacheKey); + if (cachedFunction != null) { + return cachedFunction.getter.apply(object); + } CachedMethod cachedMethod = METHOD_CACHE.get(cacheKey); if (cachedMethod != null) { try { @@ -72,9 +87,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 @@ -83,10 +98,20 @@ public Object getPropertyValue(String propertyName, Object object, GraphQLType g if (isNegativelyCached(cacheKey)) { 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 // + + Optional> getterOpt = LambdaFetchingSupport.createGetter(object.getClass(), propertyName); + if (getterOpt.isPresent()) { + Function getter = getterOpt.get(); + cachedFunction = new CachedLambdaFunction(getter); + LAMBDA_CACHE.putIfAbsent(cacheKey, cachedFunction); + return getter.apply(object); + } + boolean dfeInUse = singleArgumentValue != null; try { MethodFinder methodFinder = (root, methodName) -> findPubliclyAccessibleMethod(cacheKey, root, methodName, dfeInUse); @@ -99,7 +124,7 @@ public Object getPropertyValue(String propertyName, Object object, GraphQLType g try { return getPropertyViaFieldAccess(cacheKey, object, propertyName); } catch (FastNoSuchMethodException e) { - // we have nothing to ask for and we have exhausted our lookup strategies + // we have nothing to ask for, and we have exhausted our lookup strategies putInNegativeCache(cacheKey); return null; } @@ -272,6 +297,7 @@ private boolean isBooleanProperty(GraphQLType graphQLType) { } public void clearReflectionCache() { + LAMBDA_CACHE.clear(); METHOD_CACHE.clear(); FIELD_CACHE.clear(); NEGATIVE_CACHE.clear(); @@ -304,8 +330,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 +352,10 @@ public int hashCode() { @Override public String toString() { return "CacheKey{" + - "classLoader=" + classLoader + - ", className='" + className + '\'' + - ", propertyName='" + propertyName + '\'' + - '}'; + "classLoader=" + classLoader + + ", className='" + className + '\'' + + ", propertyName='" + propertyName + '\'' + + '}'; } } @@ -343,7 +373,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/main/java/graphql/schema/fetching/LambdaFetchingSupport.java b/src/main/java/graphql/schema/fetching/LambdaFetchingSupport.java new file mode 100644 index 0000000000..9095c66c08 --- /dev/null +++ b/src/main/java/graphql/schema/fetching/LambdaFetchingSupport.java @@ -0,0 +1,193 @@ +package graphql.schema.fetching; + +import graphql.Internal; +import graphql.VisibleForTesting; + +import java.lang.invoke.CallSite; +import java.lang.invoke.LambdaMetafactory; +import java.lang.invoke.MethodHandle; +import java.lang.invoke.MethodHandles; +import java.lang.invoke.MethodType; +import java.lang.reflect.Method; +import java.lang.reflect.Modifier; +import java.util.ArrayList; +import java.util.Comparator; +import java.util.List; +import java.util.Optional; +import java.util.function.Function; + +import static java.util.stream.Collectors.toList; + +@Internal +public class LambdaFetchingSupport { + + + /** + * This support class will use {@link LambdaMetafactory} and {@link MethodHandles} to create a dynamic function that allows access to a public + * getter method on the nominated class. {@link MethodHandles} is a caller senstive lookup mechanism. If the graphql-java cant lookup a class, then + * it won't be able to make dynamic lambda function to it. + *

+ * If one cant be made, because it doesn't exist or the calling class does not have access to the method, then it will return + * an empty result indicating that this strategy cant be used. + * + * @param sourceClass the class that has the property getter method + * @param propertyName the name of the property to get + * + * @return a function that can be used to pass in an instance of source class and returns its getter method value + */ + public static Optional> createGetter(Class sourceClass, String propertyName) { + Method candidateMethod = getCandidateMethod(sourceClass, propertyName); + if (candidateMethod != null) { + try { + 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 + } + } + return Optional.empty(); + } + + + private static Method getCandidateMethod(Class sourceClass, String propertyName) { + List allGetterMethods = findGetterMethodsForProperty(sourceClass, propertyName); + List pojoGetterMethods = allGetterMethods.stream() + .filter(LambdaFetchingSupport::isPossiblePojoMethod) + .collect(toList()); + if (!pojoGetterMethods.isEmpty()) { + Method method = pojoGetterMethods.get(0); + if (isBooleanGetter(method)) { + method = findBestBooleanGetter(pojoGetterMethods); + } + return checkForSingleParameterPeer(method, allGetterMethods); + } else { + return null; + } + + } + + private static Method checkForSingleParameterPeer(Method candidateMethod, List allMethods) { + // getFoo(DataFetchingEnv ev) is allowed, but we don't want to handle it in this class + // so this find those edge cases + for (Method allMethod : allMethods) { + if (allMethod.getParameterCount() > 0) { + // we have some method with the property name that takes more than 1 argument + // we don't want to handle this here, so we are saying there is one + return null; + } + } + return candidateMethod; + } + + private static Method findBestBooleanGetter(List methods) { + // we prefer isX() over getX() if both happen to be present + Optional isMethod = methods.stream().filter(method -> method.getName().startsWith("is")).findFirst(); + return isMethod.orElse(methods.get(0)); + } + + /** + * 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 + * + * @return a list of getter methods for that property + */ + private static List findGetterMethodsForProperty(Class sourceClass, String propertyName) { + List methods = new ArrayList<>(); + Class currentClass = sourceClass; + while (currentClass != null) { + Method[] declaredMethods = currentClass.getDeclaredMethods(); + for (Method declaredMethod : declaredMethods) { + if (isGetterNamed(declaredMethod)) { + if (nameMatches(propertyName, declaredMethod)) { + methods.add(declaredMethod); + } + } + } + currentClass = currentClass.getSuperclass(); + } + + return methods.stream() + .sorted(Comparator.comparing(Method::getName)) + .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) && + isGetterNamed(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)); + } + + private static boolean hasNoParameters(Method method) { + return method.getParameterCount() == 0; + } + + private static boolean isGetterNamed(Method method) { + String name = method.getName(); + return ((name.startsWith("get") && name.length() > 4) || (name.startsWith("is") && name.length() > 3)); + } + + private static boolean returnsSomething(Method method) { + return !method.getReturnType().equals(Void.class); + } + + private static boolean isPublic(Method method) { + return Modifier.isPublic(method.getModifiers()); + } + + private static boolean isObjectMethod(Method method) { + return method.getDeclaringClass().equals(Object.class); + } + + private static String mkPropertyName(Method method) { + // + // getFooName becomes fooName + // isFoo becomes foo + // + String name = method.getName(); + if (name.startsWith("get")) { + name = name.substring(3); + } else if (name.startsWith("is")) { + name = name.substring(2); + } + return decapitalize(name); + } + + private static String decapitalize(String name) { + if (name.length() == 0) { + return name; + } + return name.substring(0, 1).toLowerCase() + name.substring(1); + } + + + @VisibleForTesting + static Function mkCallFunction(Class targetClass, String targetMethod, Class targetMethodReturnType) throws Throwable { + MethodHandles.Lookup lookup = MethodHandles.lookup(); + MethodHandle virtualMethodHandle = lookup.findVirtual(targetClass, targetMethod, MethodType.methodType(targetMethodReturnType)); + CallSite site = LambdaMetafactory.metafactory(lookup, + "apply", + MethodType.methodType(Function.class), + MethodType.methodType(Object.class, Object.class), + virtualMethodHandle, + MethodType.methodType(targetMethodReturnType, targetClass)); + @SuppressWarnings("unchecked") + Function getterFunction = (Function) site.getTarget().invokeExact(); + return getterFunction; + } + +} diff --git a/src/test/groovy/graphql/schema/PropertyDataFetcherTest.groovy b/src/test/groovy/graphql/schema/PropertyDataFetcherTest.groovy index 523f0e0dad..39f83fa123 100644 --- a/src/test/groovy/graphql/schema/PropertyDataFetcherTest.groovy +++ b/src/test/groovy/graphql/schema/PropertyDataFetcherTest.groovy @@ -106,9 +106,16 @@ class PropertyDataFetcherTest extends Specification { def "fetch via public method declared two classes up"() { def environment = env(new TwoClassesDown("aValue")) def fetcher = new PropertyDataFetcher("publicProperty") + when: def result = fetcher.get(environment) - expect: + then: result == "publicValue" + + when: + result = fetcher.get(environment) + then: + result == "publicValue" + } def "fetch via property only defined on package protected impl"() { diff --git a/src/test/groovy/graphql/schema/fetching/LambdaFetchingSupportTest.groovy b/src/test/groovy/graphql/schema/fetching/LambdaFetchingSupportTest.groovy new file mode 100644 index 0000000000..19b78cbd03 --- /dev/null +++ b/src/test/groovy/graphql/schema/fetching/LambdaFetchingSupportTest.groovy @@ -0,0 +1,93 @@ +package graphql.schema.fetching + +import spock.lang.Specification + +class LambdaFetchingSupportTest extends Specification { + + def "can proxy Pojo methods"() { + + def pojo = new Pojo("Brad", 42) + when: + def getName = LambdaFetchingSupport.mkCallFunction(Pojo.class, "getName", String.class) + def getAge = LambdaFetchingSupport.mkCallFunction(Pojo.class, "getAge", Integer.TYPE) + + then: + getName.apply(pojo) == "Brad" + getAge.apply(pojo) == 42 + } + + def "get make getters based on property names"() { + def pojo = new Pojo("Brad", 42) + when: + def getter = LambdaFetchingSupport.createGetter(Pojo.class, "name") + then: + getter.isPresent() + getter.get().apply(pojo) == "Brad" + + when: + getter = LambdaFetchingSupport.createGetter(Pojo.class, "age") + then: + getter.isPresent() + getter.get().apply(pojo) == 42 + + } + + def "will handle bad methods and missing ones"() { + + when: + def getter = LambdaFetchingSupport.createGetter(Pojo.class, "nameX") + then: + !getter.isPresent() + + when: + getter = LambdaFetchingSupport.createGetter(Pojo.class, "get") + then: + !getter.isPresent() + + when: + getter = LambdaFetchingSupport.createGetter(Pojo.class, "is") + then: + !getter.isPresent() + + } + + def "can handle boolean setters - is by preference"() { + + def pojo = new Pojo("Brad", 42) + when: + def getter = LambdaFetchingSupport.createGetter(Pojo.class, "interesting") + then: + getter.isPresent() + getter.get().apply(pojo) == true + + when: + getter = LambdaFetchingSupport.createGetter(Pojo.class, "alone") + then: + getter.isPresent() + getter.get().apply(pojo) == true + + when: + getter = LambdaFetchingSupport.createGetter(Pojo.class, "booleanAndNullish") + then: + getter.isPresent() + getter.get().apply(pojo) == null + } + + def "will ignore non public methods"() { + + when: + def getter = LambdaFetchingSupport.createGetter(Pojo.class, "protectedLevelMethod") + then: + !getter.isPresent() + + when: + getter = LambdaFetchingSupport.createGetter(Pojo.class, "privateLevelMethod") + then: + !getter.isPresent() + + when: + getter = LambdaFetchingSupport.createGetter(Pojo.class, "packageLevelMethod") + then: + !getter.isPresent() + } +} diff --git a/src/test/groovy/graphql/schema/fetching/Pojo.java b/src/test/groovy/graphql/schema/fetching/Pojo.java new file mode 100644 index 0000000000..3b478f0712 --- /dev/null +++ b/src/test/groovy/graphql/schema/fetching/Pojo.java @@ -0,0 +1,68 @@ +package graphql.schema.fetching; + +import com.google.common.collect.ImmutableList; + +import java.util.List; + +public class Pojo { + final String name; + final int age; + + public Pojo(String name, int age) { + this.name = name; + this.age = age; + } + + public String getName() { + return name; + } + + public int getAge() { + return age; + } + + public Integer getHeight() { + return null; + } + + public List getOtherNames() { + return ImmutableList.of("A", "B"); + } + + protected String protectedLevelMethod() { + return "protectedLevelMethod"; + } + + private String privateLevelMethod() { + return "privateLevelMethod"; + } + + String packageLevelMethod() { + return "packageLevelMethod"; + } + + public boolean getInteresting() { + return false; + } + + public boolean isInteresting() { + return true; + } + + public Boolean getAlone() { + return true; + } + + public Boolean getBooleanAndNullish() { + return null; + } + + public String get() { + return "get"; + } + + public String is() { + return "is"; + } + +} \ No newline at end of file diff --git a/src/test/java/benchmark/GetterAccessBenchmark.java b/src/test/java/benchmark/GetterAccessBenchmark.java new file mode 100644 index 0000000000..7dd8c3d719 --- /dev/null +++ b/src/test/java/benchmark/GetterAccessBenchmark.java @@ -0,0 +1,71 @@ +package benchmark; + +import graphql.schema.fetching.LambdaFetchingSupport; +import org.openjdk.jmh.annotations.Benchmark; +import org.openjdk.jmh.annotations.Measurement; +import org.openjdk.jmh.annotations.Warmup; +import org.openjdk.jmh.infra.Blackhole; + +import java.lang.reflect.InvocationTargetException; +import java.lang.reflect.Method; +import java.util.function.Function; + +@Warmup(iterations = 2, time = 2, batchSize = 3) +@Measurement(iterations = 3, time = 2, batchSize = 4) +public class GetterAccessBenchmark { + + public static class Pojo { + final String name; + final int age; + + public Pojo(String name, int age) { + this.name = name; + this.age = age; + } + + public String getName() { + return name; + } + + public int getAge() { + return age; + } + } + + static Pojo pojo = new Pojo("Brad", 42); + + static Function getter = LambdaFetchingSupport.createGetter(Pojo.class, "name").get(); + + static Method getterMethod; + + static { + try { + getterMethod = Pojo.class.getMethod("getName"); + } catch (NoSuchMethodException e) { + throw new RuntimeException(e); + } + } + + + @Benchmark + public void measureDirectAccess(Blackhole bh) { + Object name = pojo.getName(); + bh.consume(name); + } + + @Benchmark + public void measureLambdaAccess(Blackhole bh) { + Object value = getter.apply(pojo); + bh.consume(value); + } + + @Benchmark + public void measureReflectionAccess(Blackhole bh) { + try { + Object name = getterMethod.invoke(pojo); + bh.consume(name); + } catch (IllegalAccessException | InvocationTargetException e) { + throw new RuntimeException(e); + } + } +}