Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve NonNullableValueCoercedAsNullException message #2774

Conversation

dondonz
Copy link
Member

@dondonz dondonz commented Mar 26, 2022

In #2761, the message from NonNullableValueCoercedAsNullException made it hard to identify the offending value. This PR improves the exception's message.

Before and after

Before, we would throw

throw new NonNullableValueCoercedAsNullException("", emptyList(), graphQLType);

Which led to this unclear message

Field '' has coerced Null value for NonNull type 'String!'

Now, the message will include the offending variable's name and the reason for the exception

Variable 'foo' has invalid value: Coerced Null value for NonNull type 'String!'

@@ -28,6 +28,6 @@ class Issue743 extends Specification {
executionResult.data == null
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!'"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Variable 'isTrue' has invalid value:

Variable 'isTrue' has an invalid value:

@@ -495,7 +497,7 @@ private Object externalValueToInternalValue(GraphqlFieldVisibility fieldVisibili
Object returnValue =
externalValueToInternalValue(fieldVisibility, unwrapOne(graphQLType), value);
if (returnValue == null) {
throw new NonNullableValueCoercedAsNullException("", emptyList(), graphQLType);
throw new NonNullableValueCoercedAsNullException(emptyList(), graphQLType);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem I see here is emptyList is not much better than "" - that is there is no real info available.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am happy to merge this as progress over perfection

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would rather remove the emptyList() path

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have another PR - #2773 - that means this is less likely in terms of conditions where this will happen - still possible but less likely

@bbakerman bbakerman added this to the 19.0 milestone Mar 27, 2022
@bbakerman bbakerman merged commit e6c95b8 into graphql-java:master Mar 27, 2022
@dondonz dondonz deleted the 2761-Non-nullable-value-coerced-as-null-exception branch March 27, 2022 22:43
@dondonz dondonz modified the milestones: 19.0, 18.1 May 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants