From 359a8e619ee217fe0ca0ae4bbe7bb8607ac024b2 Mon Sep 17 00:00:00 2001 From: Antoine Boyer Date: Fri, 25 Mar 2016 12:56:51 -0700 Subject: [PATCH 1/5] Fix NPE when coercing null enum value in BatchedExecutionStrategy --- .../batched/BatchedExecutionStrategy.java | 2 +- .../batched/FunWithStringsSchemaFactory.java | 31 +++++++++++++++++++ .../batched/GraphqlExecutionSpec.groovy | 14 +++++++++ 3 files changed, 46 insertions(+), 1 deletion(-) diff --git a/src/main/java/graphql/execution/batched/BatchedExecutionStrategy.java b/src/main/java/graphql/execution/batched/BatchedExecutionStrategy.java index 19baffa365..2ae080aee4 100644 --- a/src/main/java/graphql/execution/batched/BatchedExecutionStrategy.java +++ b/src/main/java/graphql/execution/batched/BatchedExecutionStrategy.java @@ -209,7 +209,7 @@ private GraphQLObjectType getGraphQLObjectType(GraphQLType fieldType, Object val private void handlePrimitives(List values, String fieldName, GraphQLType type) { for (GraphQLExecutionNodeValue value : values) { - Object coercedValue = coerce(type, value.getValue()); + Object coercedValue = value.getValue() == null ? null : coerce(type, value.getValue()); value.getResultContainer().putResult(fieldName, coercedValue); } } diff --git a/src/test/groovy/graphql/execution/batched/FunWithStringsSchemaFactory.java b/src/test/groovy/graphql/execution/batched/FunWithStringsSchemaFactory.java index dc7a273ceb..db70e4851c 100644 --- a/src/test/groovy/graphql/execution/batched/FunWithStringsSchemaFactory.java +++ b/src/test/groovy/graphql/execution/batched/FunWithStringsSchemaFactory.java @@ -4,6 +4,7 @@ import graphql.schema.DataFetcher; import graphql.schema.DataFetchingEnvironment; import graphql.schema.GraphQLArgument; +import graphql.schema.GraphQLEnumType; import graphql.schema.GraphQLFieldDefinition; import graphql.schema.GraphQLList; import graphql.schema.GraphQLNonNull; @@ -290,6 +291,23 @@ GraphQLSchema createSchema() { .build(); + + GraphQLEnumType enumDayType = GraphQLEnumType.newEnum() + .name("Day") + .value("MONDAY") + .value("TUESDAY") + .description("Day of the week") + .build(); + + GraphQLObjectType enumObjectType = GraphQLObjectType.newObject() + .name("EnumObject") + .field(GraphQLFieldDefinition.newFieldDefinition() + .name("value") + .type(enumDayType) + .dataFetcher(stringObjectValueFetcher) + .build()) + .build(); + GraphQLObjectType queryType = GraphQLObjectType.newObject() .name("StringQuery") .field(GraphQLFieldDefinition.newFieldDefinition() @@ -304,6 +322,19 @@ GraphQLSchema createSchema() { public Object get(DataFetchingEnvironment env) {return env.getArgument("value");} }) .build()) + .name("EnumQuery") + .field(GraphQLFieldDefinition.newFieldDefinition() + .name("enum") + .type(enumObjectType) + .argument(GraphQLArgument.newArgument() + .name("value") + .type(Scalars.GraphQLString) + .build()) + .dataFetcher(new DataFetcher() { + @Override + public Object get(DataFetchingEnvironment env) {return env.getArgument("value");} + }) + .build()) .build(); return GraphQLSchema.newSchema() .query(queryType) diff --git a/src/test/groovy/graphql/execution/batched/GraphqlExecutionSpec.groovy b/src/test/groovy/graphql/execution/batched/GraphqlExecutionSpec.groovy index d8f8a03a11..577a0b5aa8 100644 --- a/src/test/groovy/graphql/execution/batched/GraphqlExecutionSpec.groovy +++ b/src/test/groovy/graphql/execution/batched/GraphqlExecutionSpec.groovy @@ -237,6 +237,20 @@ class GraphqlExecutionSpec extends Specification { } + def "Legal null value for enum"() { + + given: + String query = + "{ enum(value: \"null\") { value } }"; + + Map expected = mapOf( + "enum", mapOf("value", null)); + + expect: + runTest(query, expected); + + } + def "Illegal null value for primitives"() { given: From 937521ae11d0ec3cb521e16000d7162e749a4048 Mon Sep 17 00:00:00 2001 From: Antoine Boyer Date: Mon, 9 May 2016 09:58:51 -0700 Subject: [PATCH 2/5] Simplified test to only showcase null enum value coercion. --- .../batched/BatchedExecutionStrategy.java | 2 +- .../batched/FunWithStringsSchemaFactory.java | 19 +++---------------- .../batched/GraphqlExecutionSpec.groovy | 4 ++-- 3 files changed, 6 insertions(+), 19 deletions(-) diff --git a/src/main/java/graphql/execution/batched/BatchedExecutionStrategy.java b/src/main/java/graphql/execution/batched/BatchedExecutionStrategy.java index 2ae080aee4..19baffa365 100644 --- a/src/main/java/graphql/execution/batched/BatchedExecutionStrategy.java +++ b/src/main/java/graphql/execution/batched/BatchedExecutionStrategy.java @@ -209,7 +209,7 @@ private GraphQLObjectType getGraphQLObjectType(GraphQLType fieldType, Object val private void handlePrimitives(List values, String fieldName, GraphQLType type) { for (GraphQLExecutionNodeValue value : values) { - Object coercedValue = value.getValue() == null ? null : coerce(type, value.getValue()); + Object coercedValue = coerce(type, value.getValue()); value.getResultContainer().putResult(fieldName, coercedValue); } } diff --git a/src/test/groovy/graphql/execution/batched/FunWithStringsSchemaFactory.java b/src/test/groovy/graphql/execution/batched/FunWithStringsSchemaFactory.java index db70e4851c..e0ac768782 100644 --- a/src/test/groovy/graphql/execution/batched/FunWithStringsSchemaFactory.java +++ b/src/test/groovy/graphql/execution/batched/FunWithStringsSchemaFactory.java @@ -299,15 +299,6 @@ GraphQLSchema createSchema() { .description("Day of the week") .build(); - GraphQLObjectType enumObjectType = GraphQLObjectType.newObject() - .name("EnumObject") - .field(GraphQLFieldDefinition.newFieldDefinition() - .name("value") - .type(enumDayType) - .dataFetcher(stringObjectValueFetcher) - .build()) - .build(); - GraphQLObjectType queryType = GraphQLObjectType.newObject() .name("StringQuery") .field(GraphQLFieldDefinition.newFieldDefinition() @@ -324,15 +315,11 @@ GraphQLSchema createSchema() { .build()) .name("EnumQuery") .field(GraphQLFieldDefinition.newFieldDefinition() - .name("enum") - .type(enumObjectType) - .argument(GraphQLArgument.newArgument() - .name("value") - .type(Scalars.GraphQLString) - .build()) + .name("nullEnum") + .type(enumDayType) .dataFetcher(new DataFetcher() { @Override - public Object get(DataFetchingEnvironment env) {return env.getArgument("value");} + public Object get(DataFetchingEnvironment env) {return null;} }) .build()) .build(); diff --git a/src/test/groovy/graphql/execution/batched/GraphqlExecutionSpec.groovy b/src/test/groovy/graphql/execution/batched/GraphqlExecutionSpec.groovy index 577a0b5aa8..5ee16cca40 100644 --- a/src/test/groovy/graphql/execution/batched/GraphqlExecutionSpec.groovy +++ b/src/test/groovy/graphql/execution/batched/GraphqlExecutionSpec.groovy @@ -241,10 +241,10 @@ class GraphqlExecutionSpec extends Specification { given: String query = - "{ enum(value: \"null\") { value } }"; + "{ nullEnum }"; Map expected = mapOf( - "enum", mapOf("value", null)); + "nullEnum", null); expect: runTest(query, expected); From cd6848f0648c88903688de32149e3af2ae797ea2 Mon Sep 17 00:00:00 2001 From: Antoine Boyer Date: Mon, 9 May 2016 11:19:53 -0700 Subject: [PATCH 3/5] Fix NPE by handling Enums differently than primitives --- .../batched/BatchedExecutionStrategy.java | 24 +++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/src/main/java/graphql/execution/batched/BatchedExecutionStrategy.java b/src/main/java/graphql/execution/batched/BatchedExecutionStrategy.java index 19baffa365..14c1591143 100644 --- a/src/main/java/graphql/execution/batched/BatchedExecutionStrategy.java +++ b/src/main/java/graphql/execution/batched/BatchedExecutionStrategy.java @@ -90,7 +90,10 @@ private List completeValues(ExecutionContext executionCont if (isPrimitive(fieldType)) { handlePrimitives(values, fieldName, fieldType); return Collections.emptyList(); - } else if (isObject(fieldType)) { + } else if (isEnum(fieldType)) { + handleEnums(values, fieldName, fieldType); + return Collections.emptyList(); + } if (isObject(fieldType)) { return handleObject(executionContext, values, fieldName, fields, fieldType); } else if (isList(fieldType)) { return handleList(executionContext, values, fieldName, fields, parentType, (GraphQLList) fieldType); @@ -214,6 +217,19 @@ private void handlePrimitives(List values, String fie } } + private void handleEnums(List values, String fieldName, + GraphQLType type) { + for (GraphQLExecutionNodeValue value : values) { + Object coercedValue; + if (value.getValue() == null) { + coercedValue = null; + } else { + coercedValue = coerce(type, value.getValue()); + } + value.getResultContainer().putResult(fieldName, coercedValue); + } + } + private Object coerce(GraphQLType type, Object value) { if (type instanceof GraphQLEnumType) { return ((GraphQLEnumType) type).getCoercing().serialize(value); @@ -227,7 +243,11 @@ private boolean isList(GraphQLType type) { } private boolean isPrimitive(GraphQLType type) { - return type instanceof GraphQLScalarType || type instanceof GraphQLEnumType; + return type instanceof GraphQLScalarType; + } + + private boolean isEnum(GraphQLType type) { + return type instanceof GraphQLEnumType; } private boolean isObject(GraphQLType type) { From 5fbf7f0d3a5cb6f91b74e3b45933f760ec05ea78 Mon Sep 17 00:00:00 2001 From: Antoine Boyer Date: Thu, 12 May 2016 13:20:48 -0700 Subject: [PATCH 4/5] Revert "Fix NPE by handling Enums differently than primitives" This reverts commit cd6848f0648c88903688de32149e3af2ae797ea2. --- .../batched/BatchedExecutionStrategy.java | 24 ++----------------- 1 file changed, 2 insertions(+), 22 deletions(-) diff --git a/src/main/java/graphql/execution/batched/BatchedExecutionStrategy.java b/src/main/java/graphql/execution/batched/BatchedExecutionStrategy.java index 14c1591143..19baffa365 100644 --- a/src/main/java/graphql/execution/batched/BatchedExecutionStrategy.java +++ b/src/main/java/graphql/execution/batched/BatchedExecutionStrategy.java @@ -90,10 +90,7 @@ private List completeValues(ExecutionContext executionCont if (isPrimitive(fieldType)) { handlePrimitives(values, fieldName, fieldType); return Collections.emptyList(); - } else if (isEnum(fieldType)) { - handleEnums(values, fieldName, fieldType); - return Collections.emptyList(); - } if (isObject(fieldType)) { + } else if (isObject(fieldType)) { return handleObject(executionContext, values, fieldName, fields, fieldType); } else if (isList(fieldType)) { return handleList(executionContext, values, fieldName, fields, parentType, (GraphQLList) fieldType); @@ -217,19 +214,6 @@ private void handlePrimitives(List values, String fie } } - private void handleEnums(List values, String fieldName, - GraphQLType type) { - for (GraphQLExecutionNodeValue value : values) { - Object coercedValue; - if (value.getValue() == null) { - coercedValue = null; - } else { - coercedValue = coerce(type, value.getValue()); - } - value.getResultContainer().putResult(fieldName, coercedValue); - } - } - private Object coerce(GraphQLType type, Object value) { if (type instanceof GraphQLEnumType) { return ((GraphQLEnumType) type).getCoercing().serialize(value); @@ -243,11 +227,7 @@ private boolean isList(GraphQLType type) { } private boolean isPrimitive(GraphQLType type) { - return type instanceof GraphQLScalarType; - } - - private boolean isEnum(GraphQLType type) { - return type instanceof GraphQLEnumType; + return type instanceof GraphQLScalarType || type instanceof GraphQLEnumType; } private boolean isObject(GraphQLType type) { From 625f69d0dff907ff968dc7ce3c20bdc3acdd7939 Mon Sep 17 00:00:00 2001 From: Antoine Boyer Date: Thu, 12 May 2016 13:36:50 -0700 Subject: [PATCH 5/5] Added null check in getNameByBalue per @danielkwinsor comment. --- src/main/java/graphql/schema/GraphQLEnumType.java | 10 ++++++++-- .../graphql/schema/GraphQLEnumTypeTest.groovy | 15 +++++++++++++++ 2 files changed, 23 insertions(+), 2 deletions(-) diff --git a/src/main/java/graphql/schema/GraphQLEnumType.java b/src/main/java/graphql/schema/GraphQLEnumType.java index 25eee304bb..de3d90cbb8 100644 --- a/src/main/java/graphql/schema/GraphQLEnumType.java +++ b/src/main/java/graphql/schema/GraphQLEnumType.java @@ -45,8 +45,14 @@ private Object getValueByName(Object value) { } private Object getNameByValue(Object value) { - for (GraphQLEnumValueDefinition valueDefinition : valueDefinitionMap.values()) { - if (value.equals(valueDefinition.getValue())) return valueDefinition.getName(); + if (value == null) { + for (GraphQLEnumValueDefinition valueDefinition : valueDefinitionMap.values()) { + if (valueDefinition.getValue() == null) return valueDefinition.getName(); + } + } else { + for (GraphQLEnumValueDefinition valueDefinition : valueDefinitionMap.values()) { + if (value.equals(valueDefinition.getValue())) return valueDefinition.getName(); + } } return null; } diff --git a/src/test/groovy/graphql/schema/GraphQLEnumTypeTest.groovy b/src/test/groovy/graphql/schema/GraphQLEnumTypeTest.groovy index d0a4db10cc..97cc60f76b 100644 --- a/src/test/groovy/graphql/schema/GraphQLEnumTypeTest.groovy +++ b/src/test/groovy/graphql/schema/GraphQLEnumTypeTest.groovy @@ -38,6 +38,21 @@ class GraphQLEnumTypeTest extends Specification { enumType.getCoercing().serialize(12) == null } + def "serialize returns NULL for null and null being an known value"() { + setup: + def enumType = newEnum().name("TestEnum") + .value("NAME", 42) + .value("NULL", null) + .build(); + expect: + enumType.getCoercing().serialize(null) == "NULL" + } + + def "serialize returns null for null value "() { + expect: + enumType.getCoercing().serialize(null) == null + } + def "duplicate value definition fails"() { when: newEnum().name("AnotherTestEnum")