-
Notifications
You must be signed in to change notification settings - Fork 2k
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
add additional runtime errors for pre-coercion OneOf errors #4195
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for compassionate-pike-271cb3 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Hi @yaacovCR, I'm @github-actions bot happy to help you with this PR 👋 Supported commandsPlease post this commands in separate comments and only one per comment:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your work on this! It seems right; but let's improve the error messages
// A nullable variable in a oneOf field position would be caught at validation-time | ||
// hence the vague error message here. | ||
message: | ||
'Argument "input" of type "TestInputObject!" has invalid value { a: $a, b: $b }.', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand the errors are vague and why; but the { a: $a }
error messages below are confusing. Maybe it would be better to replace all the errors with:
Argument "input" of type "TestInputObject!" has invalid value - exactly one field must be specified, and the value for that field must not be `null`.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the OneOf error messages to something uniform, but the vague ones are a result of the uniform behavior within getArgumentValues()
graphql-js/src/execution/values.ts
Lines 222 to 238 in 7875552
const coercedValue = coerceInputLiteral( | |
valueNode, | |
argType, | |
variableValues, | |
fragmentVariables, | |
); | |
if (coercedValue === undefined) { | |
// Note: ValuesOfCorrectTypeRule validation should catch this before | |
// execution. This is a runtime check to ensure execution does not | |
// continue with an invalid argument value. | |
throw new GraphQLError( | |
`Argument "${name}" of type "${inspect( | |
argType, | |
)}" has invalid value ${print(valueNode)}.`, | |
{ nodes: valueNode }, | |
); | |
} |
Within #3813 , part of the rebased Default Value Coercion/Validation stack, @leebyron has fixed this to provide more informative errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think another way of saying what I am saying that all of the tests within the oneOf test file, including the new ones, actually belong within the tests for coerceInputValue()
and coerceInputLiteral
() (formerly known as valueFromAST()
).
I'll set about to do that. Then these tests won't have opaque error messages in our test files that make it difficult to understand what exactly is being tested, although users who choose not to validate will still get opaque errors, at least until #3813 lands.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just being picky over capitalisation now...
Do we generally capitalise Object Type
/ Interface Type
and so on in error messages? If so, the suggestions to reduce capitalization should be dismissed.
@@ -193,7 +193,7 @@ function validateOneOfInputObject( | |||
if (isNotExactlyOneField) { | |||
context.reportError( | |||
new GraphQLError( | |||
`OneOf Input Object "${type}" must specify exactly one key.`, | |||
`Within OneOf Input Object Type "${type}", exactly one field must be specified, and the value for that field must be non-null.`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
`Within OneOf Input Object Type "${type}", exactly one field must be specified, and the value for that field must be non-null.`, | |
`Within OneOf Input Object type "${type}", exactly one field must be specified, and the value for that field must be non-null.`, |
nodes: [node], | ||
}), | ||
new GraphQLError( | ||
`Within OneOf Input Object Type "${type}", exactly one field must be specified, and the value for that field must be non-null.`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
`Within OneOf Input Object Type "${type}", exactly one field must be specified, and the value for that field must be non-null.`, | |
`Within OneOf input object type "${type}", exactly one field must be specified, and the value for that field must be non-null.`, |
@@ -221,7 +224,7 @@ function validateOneOfInputObject( | |||
if (isNullableVariable) { | |||
context.reportError( | |||
new GraphQLError( | |||
`Variable "$${variableName}" must be non-nullable to be used for OneOf Input Object "${type}".`, | |||
`Variable "$${variableName}" must be non-nullable to be used for Within OneOf Input Object Type "${type}".`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
`Variable "$${variableName}" must be non-nullable to be used for Within OneOf Input Object Type "${type}".`, | |
`Variable "$${variableName}" must be non-nullable to be used for within OneOf input object type "${type}".`, |
@@ -152,13 +157,13 @@ describe('Execute: Handles OneOf Input Objects', () => { | |||
{ | |||
locations: [{ column: 16, line: 2 }], | |||
message: | |||
'Variable "$input" got invalid value { a: "abc", b: 123 }; Exactly one key must be specified for OneOf type "TestInputObject".', | |||
'Variable "$input" got invalid value { a: "abc", b: 123 }; Within OneOf Input Object Type "TestInputObject", exactly one field must be specified, and the value for that field must be non-null.', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This error seems to be a combination of two errors; ideally this Within
should not have a capital letter since it's in the middle of a sentence. Alternatively; can the preceding semicolon be changed to a fullstop?
This changes the implementation to match the specification.
Adds additional tests as well.