Fix for #114 - NPE when coercing null enum value in BatchedExecutionStrategy (#115)

tinnou · danielkwinsor · commit d2ee718fb30d · 2016-05-14T16:33:57.000-07:00
* Simplified test to only showcase null enum value coercion. * Added null check in getNameByValue per @danielkwinsor comment.
diff --git 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/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,14 @@ GraphQLSchema createSchema() {
 
                 .build();
 
+
+        GraphQLEnumType enumDayType = GraphQLEnumType.newEnum()
+                .name("Day")
+                .value("MONDAY")
+                .value("TUESDAY")
+                .description("Day of the week")
+                .build();
+
         GraphQLObjectType queryType = GraphQLObjectType.newObject()
                 .name("StringQuery")
                 .field(GraphQLFieldDefinition.newFieldDefinition()
@@ -304,6 +313,15 @@ GraphQLSchema createSchema() {
                             public Object get(DataFetchingEnvironment env) {return env.getArgument("value");}
                         })
                         .build())
+                .name("EnumQuery")
+                .field(GraphQLFieldDefinition.newFieldDefinition()
+                        .name("nullEnum")
+                        .type(enumDayType)
+                        .dataFetcher(new DataFetcher() {
+                           @Override
+                           public Object get(DataFetchingEnvironment env) {return null;}
+                        })
+                        .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
@@ -237,6 +237,20 @@ class GraphqlExecutionSpec extends Specification {
 
     }
 
+    def "Legal null value for enum"() {
+
+        given:
+        String query =
+                "{ nullEnum }";
+
+        Map<String, Object> expected = mapOf(
+                "nullEnum", null);
+
+        expect:
+        runTest(query, expected);
+
+    }
+
     def "Illegal null value for primitives"() {
 
         given:
diff --git 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")

Original file line number	Diff line number	Diff line change
`@@ -45,8 +45,14 @@ private Object getValueByName(Object value) {`
`45`	`45`	`}`
`46`	`46`
`47`	`47`	`private Object getNameByValue(Object value) {`
`48`		`- for (GraphQLEnumValueDefinition valueDefinition : valueDefinitionMap.values()) {`
`49`		`- if (value.equals(valueDefinition.getValue())) return valueDefinition.getName();`
	`48`	`+ if (value == null) {`
	`49`	`+ for (GraphQLEnumValueDefinition valueDefinition : valueDefinitionMap.values()) {`
	`50`	`+ if (valueDefinition.getValue() == null) return valueDefinition.getName();`
	`51`	`+ }`
	`52`	`+ } else {`
	`53`	`+ for (GraphQLEnumValueDefinition valueDefinition : valueDefinitionMap.values()) {`
	`54`	`+ if (value.equals(valueDefinition.getValue())) return valueDefinition.getName();`
	`55`	`+ }`
`50`	`56`	`}`
`51`	`57`	`return null;`
`52`	`58`	`}`