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.
@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 🙏🙏 |
|
Just wanted to report that we merged this fix into our (currently private) graphql-go fork at NYT about six months ago, and have been running it in production ever since. If there's anything I can do to help this PR land here upstream, I'd be really happy to! |
9 participants
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
nilinstead of a special value/type i.e.NullValue. Internally, it differentiates between "not present" and "null" using a specialnullValuetype.Give it a try and let me know what you think.