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

Fix support for null literals #536

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Conversation

mpenick
Copy link

@mpenick mpenick commented Apr 8, 2020

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 special nullValue type.

Give it a try and let me know what you think.

@coveralls
Copy link

coveralls commented Apr 8, 2020

Coverage Status

Coverage increased (+0.02%) to 92.395% when pulling 50ac617 on mpenick:master into 02caa89 on graphql-go:master.

@mpenick
Copy link
Author

mpenick commented Apr 8, 2020

This also fixes issues with incorrect element indexes being used in errors.

@mpenick mpenick mentioned this pull request Apr 8, 2020
@jorgebay
Copy link

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?

@jeduardocosta
Copy link

Is this project still active? I've seen some pull requests with no answers, and that worries me.

@bhoriuchi
Copy link
Contributor

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."}

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

@vyj7
Copy link

vyj7 commented Apr 21, 2022

@chris-cp @ddebrunner Can we merge this PR? As per GraphQL specs we allow "Null" in the list

AndrewSisley added a commit to sourcenetwork/graphql-go that referenced this pull request Sep 12, 2022
Code is mostly copy-pasted from the PR graphql-go#536
AndrewSisley added a commit to sourcenetwork/graphql-go that referenced this pull request Sep 12, 2022
Code is mostly copy-pasted from the PR graphql-go#536
AndrewSisley added a commit to sourcenetwork/graphql-go that referenced this pull request Sep 12, 2022
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).
AndrewSisley added a commit to sourcenetwork/graphql-go that referenced this pull request Sep 12, 2022
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).
AndrewSisley added a commit to sourcenetwork/graphql-go that referenced this pull request Sep 13, 2022
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).
fredcarle pushed a commit to sourcenetwork/graphql-go that referenced this pull request Apr 25, 2023
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).
@krasish
Copy link

krasish commented Oct 31, 2023

We also have a scenario with this issue, can we please fix it 🙏🙏

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.

8 participants