From 49915b21f95212a24258629ee5b8ca4d09b8ef7e Mon Sep 17 00:00:00 2001 From: Brad Baker Date: Mon, 16 Oct 2017 19:44:09 +1100 Subject: [PATCH 1/2] More aggressive property look up --- .../graphql/schema/PropertyDataFetcher.java | 124 +++++++++++++----- .../schema/PropertyDataFetcherTest.groovy | 22 +++- .../graphql/schema/somepackage/TestClass.java | 5 + 3 files changed, 113 insertions(+), 38 deletions(-) diff --git a/src/main/java/graphql/schema/PropertyDataFetcher.java b/src/main/java/graphql/schema/PropertyDataFetcher.java index 6c3d98b710..97cdc6d51d 100644 --- a/src/main/java/graphql/schema/PropertyDataFetcher.java +++ b/src/main/java/graphql/schema/PropertyDataFetcher.java @@ -8,15 +8,29 @@ import java.lang.reflect.InvocationTargetException; import java.lang.reflect.Method; import java.lang.reflect.Modifier; +import java.util.Arrays; import java.util.Map; +import java.util.Optional; import static graphql.Scalars.GraphQLBoolean; /** * 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. + * maps and POJO java beans for values that match the desired name. + * + * It uses the following strategies + * * * You can write your own data fetchers to get data from some other backing system + * if you need highly customised behaviour. * * @see graphql.schema.DataFetcher */ @@ -40,6 +54,54 @@ public T get(DataFetchingEnvironment environment) { return (T) getPropertyViaGetter(source, environment.getFieldType()); } + private Object getPropertyViaGetter(Object object, GraphQLOutputType outputType) { + try { + return getPropertyViaGetterMethod(object, outputType, this::findPubliclyAccessibleMethod); + } catch (NoSuchMethodException ignored) { + try { + return getPropertyViaGetterMethod(object, outputType, this::findViaSetAccessible); + } catch (NoSuchMethodException ignored2) { + return getPropertyViaFieldAccess(object); + } + } + } + + @FunctionalInterface + private interface MethodFinder { + Method apply(Class aClass, String s) throws NoSuchMethodException; + } + + private Object getPropertyViaGetterMethod(Object object, GraphQLOutputType outputType, MethodFinder methodFinder) throws NoSuchMethodException { + if (isBooleanProperty(outputType)) { + try { + return getPropertyViaGetterUsingPrefix(object, "is", methodFinder); + } catch (NoSuchMethodException e) { + return getPropertyViaGetterUsingPrefix(object, "get", methodFinder); + } + } else { + return getPropertyViaGetterUsingPrefix(object, "get", methodFinder); + } + } + + private Object getPropertyViaGetterUsingPrefix(Object object, String prefix, MethodFinder methodFinder) throws NoSuchMethodException { + String getterName = prefix + propertyName.substring(0, 1).toUpperCase() + propertyName.substring(1); + try { + Method method = methodFinder.apply(object.getClass(), getterName); + return method.invoke(object); + } catch (IllegalAccessException | InvocationTargetException e) { + throw new GraphQLException(e); + } + } + + @SuppressWarnings("SimplifiableIfStatement") + private boolean isBooleanProperty(GraphQLOutputType outputType) { + if (outputType == GraphQLBoolean) return true; + if (outputType instanceof GraphQLNonNull) { + return ((GraphQLNonNull) outputType).getWrappedType() == GraphQLBoolean; + } + return false; + } + /** * Invoking public methods on package-protected classes via reflection * causes exceptions. This method searches a class's hierarchy for @@ -48,7 +110,7 @@ public T get(DataFetchingEnvironment environment) { * which have abstract public interfaces implemented by package-protected * (generated) subclasses. */ - private Method findAccessibleMethod(Class root, String methodName) throws NoSuchMethodException { + private Method findPubliclyAccessibleMethod(Class root, String methodName) throws NoSuchMethodException { Class cur = root; while (cur != null) { if (Modifier.isPublic(cur.getModifiers())) { @@ -64,48 +126,38 @@ private Method findAccessibleMethod(Class root, String methodName) throws NoSuch return root.getMethod(methodName); } - private Object getPropertyViaGetter(Object object, GraphQLOutputType outputType) { - try { - if (isBooleanProperty(outputType)) { - try { - return getPropertyViaGetterUsingPrefix(object, "is"); - } catch (NoSuchMethodException e) { - return getPropertyViaGetterUsingPrefix(object, "get"); - } - } else { - return getPropertyViaGetterUsingPrefix(object, "get"); + private Method findViaSetAccessible(Class aClass, String methodName) throws NoSuchMethodException { + Method[] declaredMethods = aClass.getDeclaredMethods(); + Optional m = Arrays.stream(declaredMethods) + .filter(method -> methodName.equals(method.getName())) + .findFirst(); + if (m.isPresent()) { + try { + // few JVMs actually enforce this but it might happen + m.get().setAccessible(true); + return m.get(); + } catch (SecurityException ignored) { } - } catch (NoSuchMethodException e1) { - return getPropertyViaFieldAccess(object); - } - } - - private Object getPropertyViaGetterUsingPrefix(Object object, String prefix) throws NoSuchMethodException { - String getterName = prefix + propertyName.substring(0, 1).toUpperCase() + propertyName.substring(1); - try { - Method method = findAccessibleMethod(object.getClass(), getterName); - return method.invoke(object); - - } catch (IllegalAccessException | InvocationTargetException e) { - throw new GraphQLException(e); } - } - - @SuppressWarnings("SimplifiableIfStatement") - private boolean isBooleanProperty(GraphQLOutputType outputType) { - if (outputType == GraphQLBoolean) return true; - if (outputType instanceof GraphQLNonNull) { - return ((GraphQLNonNull) outputType).getWrappedType() == GraphQLBoolean; - } - return false; + throw new NoSuchMethodException(methodName); } private Object getPropertyViaFieldAccess(Object object) { + Class aClass = object.getClass(); try { - Field field = object.getClass().getField(propertyName); + Field field = aClass.getField(propertyName); return field.get(object); } catch (NoSuchFieldException e) { - return null; + // if not public fields then try via setAccessible + try { + Field field = aClass.getDeclaredField(propertyName); + field.setAccessible(true); + return field.get(object); + } catch (SecurityException | NoSuchFieldException ignored2) { + return null; + } catch (IllegalAccessException e1) { + throw new GraphQLException(e); + } } catch (IllegalAccessException e) { throw new GraphQLException(e); } diff --git a/src/test/groovy/graphql/schema/PropertyDataFetcherTest.groovy b/src/test/groovy/graphql/schema/PropertyDataFetcherTest.groovy index 0f13eb8804..d37b489672 100644 --- a/src/test/groovy/graphql/schema/PropertyDataFetcherTest.groovy +++ b/src/test/groovy/graphql/schema/PropertyDataFetcherTest.groovy @@ -32,7 +32,7 @@ class PropertyDataFetcherTest extends Specification { def fetcher = new PropertyDataFetcher("privateProperty") def result = fetcher.get(environment) expect: - result == null + result == "privateValue" } def "fetch via public method"() { @@ -56,6 +56,24 @@ class PropertyDataFetcherTest extends Specification { def fetcher = new PropertyDataFetcher("propertyOnlyDefinedOnPackageProtectedImpl") def result = fetcher.get(environment) expect: - result == null + result == "valueOnlyDefinedOnPackageProtectedIpl" + } + + + def "fetch via public field"() { + def environment = env(new TestClass()) + def fetcher = new PropertyDataFetcher("publicField") + def result = fetcher.get(environment) + expect: + result == "publicFieldValue" } + + def "fetch via private field"() { + def environment = env(new TestClass()) + def fetcher = new PropertyDataFetcher("privateField") + def result = fetcher.get(environment) + expect: + result == "privateFieldValue" + } + } diff --git a/src/test/groovy/graphql/schema/somepackage/TestClass.java b/src/test/groovy/graphql/schema/somepackage/TestClass.java index f01af62592..3903b5f9b9 100644 --- a/src/test/groovy/graphql/schema/somepackage/TestClass.java +++ b/src/test/groovy/graphql/schema/somepackage/TestClass.java @@ -1,11 +1,16 @@ package graphql.schema.somepackage; +@SuppressWarnings({"unused", "FieldCanBeLocal"}) public class TestClass { private String privateProperty = "privateValue"; private String publicProperty = "publicValue"; + public String publicField = "publicFieldValue"; + + private String privateField = "privateFieldValue"; + public static TestClass createPackageProtectedImpl(String value) { return new PackageProtectedImpl(value); } From 49d255b097d84ef2f14a75637324a151a668b083 Mon Sep 17 00:00:00 2001 From: Brad Baker Date: Fri, 3 Nov 2017 18:44:08 +1100 Subject: [PATCH 2/2] is for tea --- src/main/java/graphql/schema/PropertyDataFetcher.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/main/java/graphql/schema/PropertyDataFetcher.java b/src/main/java/graphql/schema/PropertyDataFetcher.java index b7452c430c..fd61fb7c32 100644 --- a/src/main/java/graphql/schema/PropertyDataFetcher.java +++ b/src/main/java/graphql/schema/PropertyDataFetcher.java @@ -77,6 +77,7 @@ private PropertyDataFetcher(Function function) { * * * @param propertyName the name of the property to retrieve + * @param the type of result * * @return a new PropertyDataFetcher using the provided function as its source of values */ @@ -100,6 +101,7 @@ public static PropertyDataFetcher fetching(String propertyName) { * * @param function the function to use to obtain a value from the source object * @param the type of the source object + * @param the type of result * * @return a new PropertyDataFetcher using the provided function as its source of values */