WIP: Validation #3
Conversation
* documentation on valid response * instance for "leaf" API * tests & exports
Although I prefer something separate from Servant terminology, while I'm figuring out how to get this to compile, it's useful to have one fewer mental transformation to make.
GraphQL spec provides a limited range of things it can be. This is now encoded in `Response`. Doing this makes it obvious that the return value of the top-level process cannot be the same as that of the individual "handlers". It also makes it obvious that we're allowing non-leaf nodes to be queried, which is against the spec.
teh
left a comment
teh
left a comment
There was a problem hiding this comment.
generally LGTM though I think we should discuss the type encoding - I don't think what you currently have can capture what we want with good compiler errors. As mentioned in the other graph maybe we can try to retrieve the value-level encoding from types as a first step. Then we can write the query interpreter.
| -- | ||
| -- Other interesting things: | ||
| -- | ||
| -- * Doesn't have to be JSON, but does have to have maps, strings, lists, |
There was a problem hiding this comment.
interestingly it's almost like a protobuf - and I'm sure we could encode it as that (but for internal RPC sth like a Binary encoding might be faster)
There was a problem hiding this comment.
Yep. Or thrift, which is I think what FB use.
| = Success Map | ||
| | PreExecutionFailure Errors | ||
| | ExecutionFailure Errors | ||
| | PartialSuccess Map Errors |
There was a problem hiding this comment.
PartialSuccess doesn't make sense if errors/data is either/or (i.e. an Either) as described above
There was a problem hiding this comment.
I'm pretty sure it does make sense (although maybe my comment doesn't).
If the data entry in the response is not null, the errors entry in the response may contain any errors that occurred during execution. If errors occurred during execution, it should contain those errors.
From https://facebook.github.io/graphql/#sec-Errors
Note that data can be absent or null, and there are different meanings associated with each.
|
|
||
| newtype ValidDocument = Valid AST.Document deriving (Eq, Show) | ||
|
|
||
| validate :: Alternative m => AST.Document -> m ValidDocument |
There was a problem hiding this comment.
what's the purpose of validate? Make sure the query is valid? Matches the types?
There was a problem hiding this comment.
Implementing https://facebook.github.io/graphql/#sec-Validation
Context: I wanted to write code that "normalised" a query. i.e. that evaluated variable names, inserted fragments, etc. I also wanted to do this correctly, so I referred to the spec.
The spec isn't written in terms of normalisation or evaluation. Instead, it's written in terms of validation and execution.
My thinking is that if I implement validation as per the spec then:
a) that'll be useful to us & others later (because I suspect not everyone will like the type thing)
b) it will help us better understand GraphQL
c) it will provide a basis for testing our other more experimental approaches
| singleton = FieldSet . Set.singleton | ||
|
|
||
| -- TODO: Fail on duplicate keys. | ||
| union :: Alternative m => FieldSet -> FieldSet -> m FieldSet |
There was a problem hiding this comment.
is union related to graphql union types? If so I'm not sure why it should fail on duplicates
There was a problem hiding this comment.
No, it's the name of an operation that I think we'll want to perform on FieldSet. https://facebook.github.io/graphql/#sec-Executing-Selection-Sets is probably the most relevant section of the spec.
A field set is a set of KV pairs and this defines how they compose.
I'm not sold on this layout, but the idea is that the "foo" :> GraphQL Bar operator returns a singleton fieldset mapping "foo" to bar. If there's a parallel "foo" at the same level, then that's invalid (see spec).
|
I'll wait until I've had a chance to review your work and talk to you about it before I merge this. |
|
Feel free to merge - I think this is good as is |
Depends on #2
This change is