haskell-graphql / graphql-api

Write type-safe GraphQL services in Haskell
BSD 3-Clause "New" or "Revised" License
406 stars 35 forks source link

[Question] Can schema match the root nodes {"query" : "...", "operationName": ... ? #135

Open sunwukonga opened 6 years ago

sunwukonga commented 6 years ago

Before I go off and write Aeson FromJSON instances for the root nodes of the GraphQL specification (for POSTs with content type "application/json"), is there already a way to wrangle it out of the Object data type when writing the schema?

I want to follow the format given at GraphQL Post Request.

That is:

{
  "query": "...",
  "operationName": "...",
  "variables": { "myVariable": "someValue", ... }
}

where either operationName, namely, query or mutation, are nested under "query":.

Just making sure before I do it.

teh commented 6 years ago

@sunwukonga https://github.com/jml/graphql-api/pull/128/files might be interesting. We haven't quite figured out yet what to do about the FromJSON for ConstScalar instance. As implemented in that PR it has some limitations, and because of that I'm a bit nervous about introducing a FromJSON instance that'll tie us down in what we can decode in the future.

I.e. I'd prefer a solution that's interpretJSONRequest and does the variable decoding based on the schema but I don't have a fully formed picture of what that would look like yet.

Does that make sense?

sunwukonga commented 6 years ago

The schema may not be necessary, so long as I have all of the possibilities straight in my head. The only variable types we can get into the Query or Mutation Node are:

String        : A UTF‐8 character sequence.
ID            : a string, I presume the same as above, but not explicitly stated.
Enum          : a string, as above. (Often all-caps, but not sure this is 100% consistent).

Int           : A signed 32‐bit integer.
Float         : A signed double-precision floating-point value.

Boolean       : true or false.
Input Type    : Nested {}, making it easy to distinguish.

I considered Unions, but they don't go in any argument, which seems to be a condition for variable use. The problem: without context, there is ambiguity between the first three, and separately, between Int and Float.

However, with the QueryDocument, (particularly the selected operationName Node) we can distinguish instances of String and ID easily. Filtering down, all JSON strings that remain must be Enums. NB: no attempt is made to validate at this stage, i.e. search through the SchemaDocument for EnumTypeDefinition Name's that match our Enum input.

By the same mechanism, Int and Float can be easily distinguished from each other. And even scalar Date if it's implemented.

The good thing is that we will have access to the newly parsed QueryDocument and the operationName input by the time we are extracting values from variables, and I think we can use this information to correctly parse variables without leaving the block.

HC SVNT DRACONES -- These were my initial thoughts, you may cease reading. The first option is dodgey because it does what variables were instituted to avoid in the first place. String manipulation. It was also working under the assumption that the attoparsec parsing being done on the default value was doing the right thing [value :: Parser AST.Value](Line 136: Parser.hs), I think the matching for Enum should come before the matching for string, otherwise stringValue will gobble up anything that could be an Enum. Edit: OK, I just looked at nameParser and it only checks for the validity of the name, not if it exists in the QueryDocument as an Enum (how to pass the QueryDocument into this context?)

The second option below is much like @jml`s 3rd idea on the referenced PR. Really, I'm just leaving here for myself to read later. It can be safely ignored.

Possible solution 1:

What about inserting the variables into the query before we parse it? As we move down the structure from root, {query, operationName, variables} and BEFORE we start attoparsec-ing "query", we do a search and replace on the variables in the "query". Everything is then in it's context and gets parsed automatically without any further effort.

Under the assumption that we are already handling default variables properly. We can just insert the "variables" as default variables, i.e.

query HeroNameAndFriends($episode: Episode = "JEDI") {
  hero(episode: $episode) {
    name
    friends {
      name
    }
  }
}

over-writing existing defaults ^^^("JEDI" above), and appending everything else as a default value.

This would only need to be done for the incoming operationName (HeroNameAndFriends above ^^^) OR for the only operation name in the query, which ever it is. (What if there is no named operation? I don't know...)

A possible downside may be that it shifts the site of any errors encountered. It would not, for instance, tell you that your variable was bad, but rather that the field it had been inserted into was bad. Any error message would have to mention the dual possibilities. Mis-spelled variables might be haunting .

Possible Solution 2:

Add a new data type to:

 data ConstScalar
   = ConstInt Int32
   | ConstFloat Double
   | ConstBoolean Bool
   | ConstString String
   | ConstEnum Name
   | ConstWildCard String
   | ConstNull
   deriving (Eq, Ord, Show)

That is ConstWildCard Aeson.Value, which defers the real parsing until it is in it's proper context in the query.

jml commented 6 years ago

Thanks for such a high-quality bug report.

I'm uncomfortable with inserting the variables before we parse it ("Possible solution 1"). As you say, it reduces the quality of error messages, and I think good error messages are critical for usability. Less importantly, it means we'll have to re-parse the entire query when we have new variables, and I'd like to avoid that if we can.

Re "Possible Solution 2", I hadn't considered adding a new branch to ConstScalar. How would be be better/worse than adding wholly new AmbiguousConstScalar type?