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

Accept null as graphql query variables #553

Merged
merged 2 commits into from
Oct 25, 2019

Conversation

firesharker
Copy link
Contributor

Issue

Ajv schema validation fails if graphql query variables is null.
For example {"query":"{ users { email } }","variables":null} returns

[
  {
    "keyword": "type",
    "dataPath": ".variables",
    "schemaPath": "#/properties/variables/type",
    "params": {
      "type": "object"
    },
    "message": "should be object"
  }
]

Solution and steps

Update Ajv schema to accept null for variables.

@LoicPoullain
Copy link
Member

LoicPoullain commented Oct 25, 2019

In which situation would you like to send a null value? It does not seem to be required by GraphQL clients or the GraphQL recommendation: https://graphql.org/learn/serving-over-http/#post-request.

If it does, then we'll need to add also a unit test and maybe update GraphQLController.post

@firesharker
Copy link
Contributor Author

GraphiQL sends it by default if you do not write anything in the query variables tab. Previously we used apollo-server without FoalTS, and it accepts null. I could not find anything about whenever it should be accepted or not, so I assumed that probably it is a good idea to be consistent with them.

@LoicPoullain
Copy link
Member

Ok, this is legitimate.

I added a test to your branch to make sure that it works as expected.

@LoicPoullain
Copy link
Member

Weirdly the Travis web hook is not triggered. I'll need to fix it before merging the PR.

@LoicPoullain LoicPoullain mentioned this pull request Oct 25, 2019
9 tasks
Copy link
Member

@LoicPoullain LoicPoullain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@LoicPoullain LoicPoullain changed the base branch from master to v1-3-0 October 25, 2019 20:45
@LoicPoullain LoicPoullain merged commit 094254d into FoalTS:v1-3-0 Oct 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants