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

Missing variable value for nullable field argument #181

Closed
andreas opened this issue Nov 26, 2019 · 4 comments · Fixed by #184
Closed

Missing variable value for nullable field argument #181

andreas opened this issue Nov 26, 2019 · 4 comments · Fixed by #184

Comments

@andreas
Copy link
Owner

andreas commented Nov 26, 2019

(Originally posted by @osener in #179 (comment))

@andreas It's more of a question I guess, I don't know what behavior is correct and I'm curious whether a possible OGS solution to this issue would be relevant to my issue.

query getUsers($first: Int, $after: Cursor) {
  users(first: $first, after: $after) {
    nodes {
      id
    }
  }
}

If I call this query with { "first": 10 } I get this error back:

{
  "errors": [
    {
      "message": "Missing variable `after`"
    }
  ],
  "data": null
}

while { "first": 10, "after": null } works.

In fact, this was the way graphql_ppx worked. It would force me to declare every variable and construct something like { "first": Some(10), "after": None }, and it would encode None with null.

After switching to graphql_ppx_re because of newer BuckleScript support I noticed my null (None) values started to get dropped, which ocaml-graphql-server did not like.

During the subsequent discussion in the issue the author said he made this change to work around an issue with Sangria, but also that this change is compliant with the spec.

Now I realize that my issue is about variables passed to the query, which is not this issue is about. But I'm still curious about your opinion on this, is this something I should attempt to solve on the client side, or at ocaml-graphql-server side?

@andreas
Copy link
Owner Author

andreas commented Nov 26, 2019

@osener Thanks for reporting this. I've re-read the spec a bit, and it seems unclear to me what the right behavior is for nullable field arguments and missing variable vales. I'll need to investigate a bit further. If you have any insights, I'd be happy to hear.

While looking over the variable handling code, there appears to be some other issues around default values, which I'll look into also..

@ozanmakes
Copy link
Contributor

Thank you @andreas! I'll try to understand the correct (or common) approach to this better and report back if I have any additional insight. Right now I have a working client-side workaround.

@andreas
Copy link
Owner Author

andreas commented Dec 8, 2019

Based on this test case from graphql-js, it appears a missing variable for a nullable argument should be treated as null. I'll update the implementation accordingly.

@andreas
Copy link
Owner Author

andreas commented Dec 8, 2019

A quick note: Nullable arguments are currently represented via an option type. This means you cannot distinguish between a missing argument value and null. We could choose to use a type with three constructors instead to allow this, e.g. type 'a nullable = Null | Undefined | Defined of 'a. This ties into #179.

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 a pull request may close this issue.

2 participants