andreas / ocaml-graphql-server

GraphQL servers in OCaml
MIT License
624 stars 60 forks source link

Missing variable value for nullable field argument #181

Closed andreas closed 4 years ago

andreas commented 4 years ago

(Originally posted by @osener in https://github.com/andreas/ocaml-graphql-server/issues/179#issuecomment-558626494)

@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 commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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.