-
Notifications
You must be signed in to change notification settings - Fork 842
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
Fix support for null literals #536
base: master
Are you sure you want to change the base?
Conversation
This also fixes issues with incorrect element indexes being used in errors. |
We've been using this fix for the past week and works perfectly. The fix is straightforward and provides good coverage. Is there anything I can do to help land this? |
Is this project still active? I've seen some pull requests with no answers, and that worries me. |
It is but @chris-ramon is the only one approving/merging PRs. He seems to approve the straightforward ones every few months or so. |
_, messages := isValidLiteralValue(itemType, value) | ||
var messages []string | ||
if value.GetKind() == kinds.NullValue { | ||
messages = []string{"Unexpected null literal."} |
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.
@mpenick Is there a reason for this disallowing null in a list? There's even a test for it so it doesn't seem to be an accident.
Section 2.9.7 seems to allow nulls in a list.
https://spec.graphql.org/June2018/#ListValue
@chris-cp @ddebrunner Can we merge this PR? As per GraphQL specs we allow "Null" in the list |
Code is mostly copy-pasted from the PR graphql-go#536
Code is mostly copy-pasted from the PR graphql-go#536
Code is mostly copy-pasted from the PR graphql-go#536 - main difference is that I haven't copied over a couple of new tests, and that I permitted null within arrays (unanswered question in original PR, and I see no reason for it too).
Code is mostly copy-pasted from the PR graphql-go#536 - main difference is that I haven't copied over a couple of new tests, and that I permitted null within arrays (unanswered question in original PR, and I see no reason for it too).
Code is mostly copy-pasted from the PR graphql-go#536 - main difference is that I haven't copied over a couple of new tests, and that I permitted null within arrays (unanswered question in original PR, and I see no reason for it too).
Code is mostly copy-pasted from the PR graphql-go#536 - main difference is that I haven't copied over a couple of new tests, and that I permitted null within arrays (unanswered question in original PR, and I see no reason for it too).
We also have a scenario with this issue, can we please fix it 🙏🙏 |
This builds on an existing PR: #401. It fixes issues found in that PR (it works for enums, objects, and lists values) and adds tests.
Externally, this uses
nil
instead of a special value/type i.e.NullValue
. Internally, it differentiates between "not present" and "null" using a specialnullValue
type.Give it a try and let me know what you think.