From d0895c485c5d25dfbb65b2d8d4dfa1496daf4634 Mon Sep 17 00:00:00 2001 From: osher-sade Date: Thu, 10 May 2018 16:16:40 +0300 Subject: [PATCH 1/2] Fixed a bug where MethodDataFetcher caused NullPointerException --- .../dataFetchers/MethodDataFetcher.java | 23 ++++- .../processor/util/ReflectionKit.java | 2 - .../annotations/MethodDataFetcherTest.java | 92 ++++++++++++++++++- 3 files changed, 108 insertions(+), 9 deletions(-) diff --git a/src/main/java/graphql/annotations/dataFetchers/MethodDataFetcher.java b/src/main/java/graphql/annotations/dataFetchers/MethodDataFetcher.java index 080aa61e..fc192cff 100644 --- a/src/main/java/graphql/annotations/dataFetchers/MethodDataFetcher.java +++ b/src/main/java/graphql/annotations/dataFetchers/MethodDataFetcher.java @@ -14,20 +14,19 @@ */ package graphql.annotations.dataFetchers; -import graphql.annotations.processor.ProcessingElementsContainer; import graphql.annotations.annotationTypes.GraphQLInvokeDetached; import graphql.annotations.annotationTypes.GraphQLName; +import graphql.annotations.processor.ProcessingElementsContainer; import graphql.annotations.processor.typeFunctions.TypeFunction; import graphql.schema.*; -import graphql.schema.GraphQLType; import java.lang.reflect.*; import java.util.ArrayList; import java.util.List; import java.util.Map; -import static graphql.annotations.processor.util.ReflectionKit.constructNewInstance; import static graphql.annotations.processor.util.NamingKit.toGraphqlName; +import static graphql.annotations.processor.util.ReflectionKit.constructNewInstance; import static graphql.annotations.processor.util.ReflectionKit.newInstance; public class MethodDataFetcher implements DataFetcher { @@ -59,7 +58,13 @@ public T get(DataFetchingEnvironment environment) { return null; } } - return (T)method.invoke(obj, invocationArgs(environment, container)); + + if (obj == null && environment.getSource() != null) { + Object value = getFieldValue(environment.getSource(), method.getName()); + if (value != null) return (T) value; + } + + return (T) method.invoke(obj, invocationArgs(environment, container)); } catch (IllegalAccessException | InvocationTargetException e) { throw new RuntimeException(e); } @@ -131,4 +136,14 @@ private Object buildArg(Type p, GraphQLType graphQLType, Object arg) { return arg; } } + + private Object getFieldValue(Object source, String fieldName) throws IllegalAccessException { + try { + Field field = source.getClass().getDeclaredField(fieldName); + field.setAccessible(true); + return field.get(source); + } catch (NoSuchFieldException e) { + return null; + } + } } diff --git a/src/main/java/graphql/annotations/processor/util/ReflectionKit.java b/src/main/java/graphql/annotations/processor/util/ReflectionKit.java index d4eef6a8..33bc5cfb 100644 --- a/src/main/java/graphql/annotations/processor/util/ReflectionKit.java +++ b/src/main/java/graphql/annotations/processor/util/ReflectionKit.java @@ -69,6 +69,4 @@ public static T newInstance(Class clazz, Object parameter) { } return null; } - - } diff --git a/src/test/java/graphql/annotations/MethodDataFetcherTest.java b/src/test/java/graphql/annotations/MethodDataFetcherTest.java index a619ede2..9ebaaade 100644 --- a/src/test/java/graphql/annotations/MethodDataFetcherTest.java +++ b/src/test/java/graphql/annotations/MethodDataFetcherTest.java @@ -14,15 +14,27 @@ */ package graphql.annotations; +import graphql.ExecutionResult; +import graphql.GraphQL; +import graphql.annotations.annotationTypes.GraphQLDataFetcher; +import graphql.annotations.annotationTypes.GraphQLField; +import graphql.annotations.annotationTypes.GraphQLInvokeDetached; +import graphql.annotations.annotationTypes.GraphQLType; import graphql.annotations.dataFetchers.MethodDataFetcher; import graphql.annotations.processor.GraphQLAnnotations; -import graphql.schema.DataFetchingEnvironmentImpl; +import graphql.schema.*; import org.testng.annotations.BeforeMethod; import org.testng.annotations.Test; import java.util.ArrayList; import java.util.HashMap; +import java.util.Map; +import static graphql.schema.GraphQLSchema.newSchema; +import static org.testng.Assert.assertEquals; +import static org.testng.Assert.assertTrue; + +@SuppressWarnings("unchecked") public class MethodDataFetcherTest { @BeforeMethod @@ -41,10 +53,84 @@ public String method() throws TestException { @Test(expectedExceptions = RuntimeException.class) public void exceptionRethrowing() { try { - MethodDataFetcher methodDataFetcher = new MethodDataFetcher(getClass().getMethod("method"),null,null); - methodDataFetcher.get(new DataFetchingEnvironmentImpl(this, new HashMap(), null, null, null, new ArrayList<>(), null, null, null, null, null, null, null)); + MethodDataFetcher methodDataFetcher = new MethodDataFetcher(getClass().getMethod("method"), null, null); + methodDataFetcher.get(new DataFetchingEnvironmentImpl(this, new HashMap<>(), null, null, null, new ArrayList<>(), null, null, null, null, null, null, null)); } catch (NoSuchMethodException e) { e.printStackTrace(); } } + + + @GraphQLType + public static class ApiType { + @GraphQLField + public int a() { + return 1; + } + + @GraphQLField + @GraphQLInvokeDetached + public int b() { + return 2; + } + } + + public static class InternalType { + public int a = 123; + public int b; + } + + @GraphQLType + public static class Query { + @GraphQLField + @GraphQLDataFetcher(MyFetcher.class) + public ApiType field; + + @GraphQLField + @GraphQLDataFetcher(MyApiFetcher.class) + public ApiType apiField; + } + + public static class MyFetcher implements DataFetcher { + public InternalType get(DataFetchingEnvironment environment) { + return new InternalType(); + } + } + + public static class MyApiFetcher implements DataFetcher { + public ApiType get(DataFetchingEnvironment environment) { + return new ApiType(); + } + } + + @Test + public void queryingOneFieldNotAnnotatedWithGraphQLInvokeDetached_valueIsDeterminedByEntity() { + GraphQLObjectType object = GraphQLAnnotations.object(Query.class); + GraphQLSchema schema = newSchema().query(object).build(); + + ExecutionResult result = GraphQL.newGraphQL(schema).build().execute("query { field { a } }"); + assertTrue(result.getErrors().isEmpty()); + assertEquals(((Map>) result.getData()).get("field").get("a").toString(), "123"); + } + + @Test + public void queryingOneFieldAnnotatedWithGraphQLInvokeDetached_valueIsDeterminedByApiEntity() { + GraphQLObjectType object = GraphQLAnnotations.object(Query.class); + GraphQLSchema schema = newSchema().query(object).build(); + + ExecutionResult result = GraphQL.newGraphQL(schema).build().execute("query { field { b } }"); + assertTrue(result.getErrors().isEmpty()); + assertEquals(((Map>) result.getData()).get("field").get("b").toString(), "2"); + } + + @Test + public void queryingFieldsFromApiEntityFetcher_valueIsDeterminedByApiEntity() { + GraphQLObjectType object = GraphQLAnnotations.object(Query.class); + GraphQLSchema schema = newSchema().query(object).build(); + + ExecutionResult result = GraphQL.newGraphQL(schema).build().execute("query { apiField { a b } }"); + assertTrue(result.getErrors().isEmpty()); + assertEquals(((Map>) result.getData()).get("apiField").get("a").toString(), "1"); + assertEquals(((Map>) result.getData()).get("apiField").get("b").toString(), "2"); + } } \ No newline at end of file From 48d7bc3542f5f0111b2d0482e90c64f7cbbf7d31 Mon Sep 17 00:00:00 2001 From: osher-sade Date: Thu, 10 May 2018 17:20:58 +0300 Subject: [PATCH 2/2] add test fix batched --- .../dataFetchers/MethodDataFetcher.java | 22 +++++++------------ .../annotations/MethodDataFetcherTest.java | 20 +++++++++++++++-- 2 files changed, 26 insertions(+), 16 deletions(-) diff --git a/src/main/java/graphql/annotations/dataFetchers/MethodDataFetcher.java b/src/main/java/graphql/annotations/dataFetchers/MethodDataFetcher.java index fc192cff..9dbaa281 100644 --- a/src/main/java/graphql/annotations/dataFetchers/MethodDataFetcher.java +++ b/src/main/java/graphql/annotations/dataFetchers/MethodDataFetcher.java @@ -14,6 +14,7 @@ */ package graphql.annotations.dataFetchers; +import graphql.annotations.annotationTypes.GraphQLBatched; import graphql.annotations.annotationTypes.GraphQLInvokeDetached; import graphql.annotations.annotationTypes.GraphQLName; import graphql.annotations.processor.ProcessingElementsContainer; @@ -45,10 +46,7 @@ public MethodDataFetcher(Method method, TypeFunction typeFunction, ProcessingEle public T get(DataFetchingEnvironment environment) { try { T obj; - - if (Modifier.isStatic(method.getModifiers())) { - obj = null; - } else if (method.getAnnotation(GraphQLInvokeDetached.class) != null) { + if (method.isAnnotationPresent(GraphQLBatched.class) || method.isAnnotationPresent(GraphQLInvokeDetached.class)) { obj = newInstance((Class) method.getDeclaringClass()); } else if (!method.getDeclaringClass().isInstance(environment.getSource())) { obj = newInstance((Class) method.getDeclaringClass(), environment.getSource()); @@ -61,11 +59,11 @@ public T get(DataFetchingEnvironment environment) { if (obj == null && environment.getSource() != null) { Object value = getFieldValue(environment.getSource(), method.getName()); - if (value != null) return (T) value; + return (T) value; } return (T) method.invoke(obj, invocationArgs(environment, container)); - } catch (IllegalAccessException | InvocationTargetException e) { + } catch (IllegalAccessException | InvocationTargetException | NoSuchFieldException e) { throw new RuntimeException(e); } } @@ -137,13 +135,9 @@ private Object buildArg(Type p, GraphQLType graphQLType, Object arg) { } } - private Object getFieldValue(Object source, String fieldName) throws IllegalAccessException { - try { - Field field = source.getClass().getDeclaredField(fieldName); - field.setAccessible(true); - return field.get(source); - } catch (NoSuchFieldException e) { - return null; - } + private Object getFieldValue(Object source, String fieldName) throws IllegalAccessException, NoSuchFieldException { + Field field = source.getClass().getDeclaredField(fieldName); + field.setAccessible(true); + return field.get(source); } } diff --git a/src/test/java/graphql/annotations/MethodDataFetcherTest.java b/src/test/java/graphql/annotations/MethodDataFetcherTest.java index 9ebaaade..3bdcc37b 100644 --- a/src/test/java/graphql/annotations/MethodDataFetcherTest.java +++ b/src/test/java/graphql/annotations/MethodDataFetcherTest.java @@ -14,6 +14,7 @@ */ package graphql.annotations; +import graphql.ExceptionWhileDataFetching; import graphql.ExecutionResult; import graphql.GraphQL; import graphql.annotations.annotationTypes.GraphQLDataFetcher; @@ -31,8 +32,7 @@ import java.util.Map; import static graphql.schema.GraphQLSchema.newSchema; -import static org.testng.Assert.assertEquals; -import static org.testng.Assert.assertTrue; +import static org.testng.Assert.*; @SuppressWarnings("unchecked") public class MethodDataFetcherTest { @@ -73,6 +73,11 @@ public int a() { public int b() { return 2; } + + @GraphQLField + public int c() { + return 4; + } } public static class InternalType { @@ -103,6 +108,7 @@ public ApiType get(DataFetchingEnvironment environment) { } } + @Test public void queryingOneFieldNotAnnotatedWithGraphQLInvokeDetached_valueIsDeterminedByEntity() { GraphQLObjectType object = GraphQLAnnotations.object(Query.class); @@ -133,4 +139,14 @@ public void queryingFieldsFromApiEntityFetcher_valueIsDeterminedByApiEntity() { assertEquals(((Map>) result.getData()).get("apiField").get("a").toString(), "1"); assertEquals(((Map>) result.getData()).get("apiField").get("b").toString(), "2"); } + + @Test + public void queryingFieldsFromNoApiEntityFetcher_noMatchingFieldInEntity_throwException(){ + GraphQLObjectType object = GraphQLAnnotations.object(Query.class); + GraphQLSchema schema = newSchema().query(object).build(); + + ExecutionResult result = GraphQL.newGraphQL(schema).build().execute("query { field { c } }"); + assertFalse(result.getErrors().isEmpty()); + assertTrue(((ExceptionWhileDataFetching)result.getErrors().get(0)).getException().getCause() instanceof NoSuchFieldException); + } } \ No newline at end of file