Improve NonNullableValueCoercedAsNullException message #2774
Conversation
| executionResult.errors.size() == 1 | ||
| executionResult.errors[0].errorType == ErrorType.ValidationError | ||
| executionResult.errors[0].message == "Variable 'isTrue' has coerced Null value for NonNull type 'Boolean!'" | ||
| executionResult.errors[0].message == "Variable 'isTrue' has invalid value: Variable 'isTrue' has coerced Null value for NonNull type 'Boolean!'" |
There was a problem hiding this comment.
Variable 'isTrue' has invalid value:
Variable 'isTrue' has an invalid value:
| externalValueToInternalValue(fieldVisibility, unwrapOne(graphQLType), value); | ||
| if (returnValue == null) { | ||
| throw new NonNullableValueCoercedAsNullException("", emptyList(), graphQLType); | ||
| throw new NonNullableValueCoercedAsNullException(emptyList(), graphQLType); |
There was a problem hiding this comment.
The problem I see here is emptyList is not much better than "" - that is there is no real info available.
There was a problem hiding this comment.
I mean its better than it was - but is it as good as it could be?
I guess we would have to "track" fields and types as we descending the ValueResolver to get excellent names in the exception
There was a problem hiding this comment.
I am happy to merge this as progress over perfection
There was a problem hiding this comment.
I would rather remove the emptyList() path
There was a problem hiding this comment.
I have another PR - #2773 - that means this is less likely in terms of conditions where this will happen - still possible but less likely
2 participants
In #2761, the message from
NonNullableValueCoercedAsNullExceptionmade it hard to identify the offending value. This PR improves the exception's message.Before and after
Before, we would throw
Which led to this unclear message
Now, the message will include the offending variable's name and the reason for the exception