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

BugFix: Nested List Input Coercion not matching GraphQL spec #194

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

abhinand-c
Copy link

This PR fixes the issue of Nested List Input with multiple values not being properly validated as per the schema.

For a nested List [[Int!]], providing an input value of [1, 2, 3]
Expected Behaviour: should raise validation error
Current Behaviour: coerce value to [ [1], [2], [3] ]

Changes

  • Adds check to validate and raises error for the condition
  • Updated tests to reflect the new logic, and validate error

Refer:

image

@abhinand-c abhinand-c requested a review from Cito as a code owner March 2, 2023 20:03
@Cito
Copy link
Member

Cito commented Mar 2, 2023

Hi @abhinand-c , and thank you for reporting and providing a fix. May I suggest that next time you create an issue before posting a PR ? We can then discuss what's the best way to solve the problem before you invest too much time.

I believe you're right in that the coercion rule (coercion needs to be distinguished from validation) is not properly implemented according to the latest spec. The implementation was correct regarding the older spec of 2016, but it looks like in 2018 the spec was changed, and this change was not incorporated into GraphQL.js of which GraphQL-core is a port.

The proper procedure to fix this is to open an issue for GraphQL.js, maybe followed by a PR with a fix for TypeScript, and when this is merged into GraphQL.js, the same fix will be ported to Python. By following this procedure we make sure the code base does not deviate, the JavaScript users benefit as well, and we can have a discussion with the larger JavaScript community and GraphQL experts on what's the best way to fix it and review the PR.

Let me know if you want to take care of that, or if you want me to do it and if I can take your code as a base for a possible PR in GraphQL.js.

@abhinand-c
Copy link
Author

Hi @Cito , thanks for the guiding me in the right way, I'll definitely follow the proper procedure next time. I have raised the issue in GraphQL.js (graphql/graphql-js#3858).

Also, currently in this PR my commits were not signed, shall I proceed to update my commits and sign them?
Is there anything else that I should have taken care of as a first time contributor here?

@Cito
Copy link
Member

Cito commented Mar 3, 2023

No worries @abhinand-c it's all well. GraphQL.js is our upstream project. The guidelines for contributing to GraphQL.js are these if you want to create a PR there, but here we are a bit more relaxed.

@abhinand-c
Copy link
Author

Hi, I have updated old (unsigned) commits and replaced them with signed commits by force pushing.

@Cito Cito added the upstream Should be solved or discussed upstream label Mar 30, 2023
@erikwrede
Copy link
Member

Upstream update: graphql/graphql-spec#1057 (comment)

Once the spec PR is merged, we can close this PR, as the table this relied on was adjusted to allow our explicit case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
upstream Should be solved or discussed upstream
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants