I don't think null is a valid value in GraphQL (see http://facebook.github.io/graphql/#sec-Types.Non-Null).

What do you think?

Thanks

Yes, null is not a valid input value but it is a valid result value for enum.

The test I added tried to follow the FunWithStringsSchemaFactory test strategy but in this case it might be a little confusing. Let me try to explain.

TheLegal null value for enum test checks that the execution strategies coerce a null enum result value.
The stringObjectValueFetcher takes a string as source and if the source value is "null" it transforms it to null. All the other cases it just returns the same string.

Does that make sense?

It does make some amount of sense but is still confusing. Can you rewrite your test to make a clearer separation between input coercion and result coercion?

I do think that "null" is a valid thing to try to coerce to an enum result, maybe null too (though not in the base GraphQLEnumValueDefinition).

However, your change to handlePrimitives has the side effect of now all primitives return null without coercion. The spec says in 3.1.8 that

In all of the above result coercion, null was considered a valid value.

So really we should be allowing the result coercion to attempt to take place if null, and fix the NPE someplace else. You either need a null check in getNameByValue or someplace else.
This should be good:

        if (value == null) {
            for (GraphQLEnumValueDefinition valueDefinition : valueDefinitionMap.values()) {
                if (valueDefinition.getValue() == null) return valueDefinition.getName();
            }
        }
        for (GraphQLEnumValueDefinition valueDefinition : valueDefinitionMap.values()) {
            if (value.equals(valueDefinition.getValue())) return valueDefinition.getName();
        }

And for easy reference here is Enum Values Spec

And now..I'm confused. What does the following mean? From the spec: An enum value cannot be “null” in order to avoid confusion. GraphQL does not supply a value literal to represent the concept null.
Does it mean an enum result value is never null and therefore has to be one and only one of the declared values?

If yes, ExecutionStrategy is too lenient and we probably need to change the ExecutionStrategy.completeValue to disallow null by moving completeValueForEnum above the null check

If not we are back to our original issue and BatchedExecutionStrategy shouldn't throw a NPE.
@danielkwinsor: Good catch, indeed the check shouldn't be in BatchedExecutionStrategy.handlePrimitives for the reason you stated. Now if we want to be consistent with the other handleXxx methods we could probably do the null check before calling BatchedExecutionStrategy.coerce. Like so cd6848f.

I also simplified the test method to only showcase the enum null result value coercion. We can clearly see the regular ExecutionStrategy let null return null but not the BatchedExecutionStrategy.

Either way, we need to make the strategies consistent.

At this time, I see the issue is only when trying to give back a result that is null. Null is a valid thing to return from a DataFetcher, even a DF for an enum. The NPE happens when attempting to coerce this null value into some some name as in getNameByValue.

So getNameByValue needs a null check, it should go here and not in BES. The only question to me is whether we detect null and return null (to avoid NPE on value.equals()) or really attempt to match up null with a GraphQLEnumValueDefinition with value Object == null.

Can you please limit your changes to a null check in getNameByValue (plus test of course) and verify whether there are any other breaking places?

@danielkwinsor Made the changes per your comment. I added additional tests in GraphQLEnumTypeTest to cover the new code.