Add tests for anonymous queries w|w/o variables #139
Conversation
```
"{\"query\":\"query {\\n greeting(who: \\\"Tim\\\")\\n}\"}"
```
Notice that the query (immediately after the start of the JSON field
`query:`) has no operationName, i.e. it's anonymous.
This gets decoded to:
```
Just (GraphQLPostRequest {query = "query {\n greeting(who: \"Tim\")\n}", operationName = "", variables = fromList []})
```
by a custom/temporary Aeson parser.
:blush: I didn't record it, and now it's gone. Something along the lines
of: `Just(Error{"query document error!definition error!query"})`
Not exactly, but that was the gist of it.
Realized that the parser might be choking on the absence of the
`operationName`, so tried to apply `optempty` to `nameParser` but Name
was not an instance of Monoid. Changed that and ¡viola! it worked
(sounds easy, but I learned something about applying Monoid to a newtype,
and also picked up a prior mistake where I forgot to import Data.Text).
On a side note, the 'custom/temporary' Aeson parser does not yet solve
the ambiguous `variables` problem mentioned here:
and here:
and obliquely here:
|
Thanks for this, and indeed all your other contributions.
I’ll try to get to reviewing this as soon as I may. Please forgive the
lag—it’s the result of competing priorities, not lack of interest.
…On Fri, 22 Dec 2017 at 10:59, Paul Desmond Parker ***@***.***> wrote:
Add tests in ASTTests.hs:
- parses anonymous query documents
- changed previous test of same name to: parses shorthand syntax
documents
- parses anonymous query with variables
Added tests in ValidationTests.hs:
- Treats anonymous queries as valid
- Treats anonymous queries with variables as valid
------------------------------
You can view, comment on, or merge this pull request online at:
#139
Commit Summary
- Make Node instance of HasName. Implement getName
- Add Data.Text import and clean up unused imports.
- Fix error when single query is anonymous.
- Add tests for anonymous queries w|w/o variables
File Changes
- *M* src/GraphQL/Internal/Name.hs
<https://github.com/jml/graphql-api/pull/139/files#diff-0> (90)
- *M* src/GraphQL/Internal/Syntax/AST.hs
<https://github.com/jml/graphql-api/pull/139/files#diff-1> (78)
- *M* src/GraphQL/Internal/Syntax/Encoder.hs
<https://github.com/jml/graphql-api/pull/139/files#diff-2> (47)
- *M* src/GraphQL/Internal/Syntax/Parser.hs
<https://github.com/jml/graphql-api/pull/139/files#diff-3> (41)
- *M* tests/ASTTests.hs
<https://github.com/jml/graphql-api/pull/139/files#diff-4> (53)
- *M* tests/ValidationTests.hs
<https://github.com/jml/graphql-api/pull/139/files#diff-5> (40)
Patch Links:
- https://github.com/jml/graphql-api/pull/139.patch
- https://github.com/jml/graphql-api/pull/139.diff
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#139>, or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAHq6lmOoAEdHl2Za0hmikHas7YgfDCbks5tC4uBgaJpZM4RK6eH>
.
|
src/GraphQL/Internal/Name.hs
Outdated
| -- https://facebook.github.io/graphql/#sec-Names | ||
| newtype Name = Name { unName :: T.Text } deriving (Eq, Ord, Show) | ||
|
|
||
| instance Monoid Name where |
There was a problem hiding this comment.
Why is this instance useful? Would you mind adding a comment that explains why we have this instance?
There was a problem hiding this comment.
What it's doing
If Name is not a monoid, then optempty cannot operate on nameParser (because without it, it only produces a Name or a NameError). As a monoid, optempty returns a Name "SomeValidName" or Name mempty. This allows the following:
node :: Parser AST.Node
node = AST.Node <$> optempty nameParser
<*> optempty variableDefinitions
<*> optempty directives
<*> selectionSet
This makes graphql-api's implementation more forgiving than the spec, without any added ambiguity (because of validation that follows in our validation layer). EDIT: I should not have been referring to graphql.org for spec. In this case it contradicts the canonical reference facebook.github.io #sec-Language.Operations. In OperationDefinition, Nameopt is always optional.
Justification & Discussion
EDIT: The following two paragraphs are predicated on the graphql.org reference, which is contradicted by the actual spec.
According to the spec graphql.org, #operation-name:
Up until now, we have been using a shorthand syntax where we omit both the query keyword and the query name, but in production apps it's useful to use these to make our code less ambiguous. You'll need these optional parts to a GraphQL operation if you want to execute something other than a query or pass dynamic variables.
It says, that BOTH query keyword AND query name MUST be added to mutation, subscription, or a query that passes dynamic variables. Unfortunately, it is not this strongly written, and because query name (operation name) is practically useless, client implementations I've encountered don't follow it strictly.
The utility of operation names is in the contrived case where multiple operations are sent in a single document (dead load because there is currently little use for these extra superfluous operations in the document: look here for some of the justifications I've encountered). Note that I ultimately arguing to USE operation names in the referenced comment.
The spec is NOT strongly written enough to preclude the possibility of client implementations sending something like: EDIT: The following is perfectly valid from the spec http://facebook.github.io/graphql/October2016/#sec-Language.Operations
query {greeting(who: "Tim")}
which we currently fail on, but could theoretically turn into an AST.AnonymousQuery by changing:
EDIT: This is the wrong approach. If OperationType is query, we SHOULD NOT change it to AnonymousQuery because we are not doing a check for dynamic variables. Technically, what we call AnonymousQuery might more correctly be called shorthandQuery as the use case is consonant with the definition of Query Shorthand in the spec.
operationDefinition :: Parser AST.OperationDefinition$
operationDefinition =
AST.Query <$ tok "query" <*> node
<|> AST.Mutation <$ tok "mutation" <*> node
<|> (AST.AnonymousQuery <$> selectionSet)
<?> "operationDefinition error!"
into something like this (untested, not 100% "query" is consumed on failure of AST.Query match):
operationDefinition :: Parser AST.OperationDefinition$
operationDefinition =
AST.Query <$ tok "query" <*> node
<|> (AST.AnonymousQuery <$> selectionSet)
<|> AST.Mutation <$ tok "mutation" <*> node
<|> (AST.AnonymousQuery <$> selectionSet)
<?> "operationDefinition error!"
OR we can do what I have done in this PR, which is make the Name optional, and have a nice day :) In addition, although it's out of spec, we handle the following case as well (EDIT: It SHOULD handle this case):
query ($name : String) { greeting(who: $name) }
I.e. A request explicitly coded as a query, but without a name, and takes variables (this is not possible to misinterpret as anything other than a violation of the spec), but there is no danger in doing so because the validator will pick up ambiguity in the case of multiple operations AND no valid JSON operation name. EDIT: The only time a Name is required is in the presence of multiple operations per Query Document.
src/GraphQL/Internal/Name.hs
Outdated
| instance Monoid Name where | ||
| mempty = Name T.empty | ||
| -- mappend (Name {a}) mempty = Name {a} | ||
| -- mappend mempty (Name {b}) = Name {b} |
There was a problem hiding this comment.
Are the comments here to explain the behaviour?
There was a problem hiding this comment.
Will remove and add proper comment at head of instance declaration.
src/GraphQL/Internal/Name.hs
Outdated
|
|
||
| --newtype Any = Any { getAny :: Bool } | ||
|
|
||
| --instance Monoid Any where |
There was a problem hiding this comment.
What does this commented our block about Any do?
There was a problem hiding this comment.
Working notes, I'll remove.
Following suggestions made at: [pull/139#discussion_r158537349](haskell-graphql#139 (comment)) and [pull/139#discussion_r158537230](haskell-graphql#139 (comment)) and [pull/139#discussion_r158537382](haskell-graphql#139 (comment))
Majority of code change occurred in Validations.hs because the StateT
monad needed to operate on a state of type `Set (Maybe Name)` instead of
`Set Name`. This was complicated by the fact that fragments use a raw
`Name`, not the wrapped `Maybe Name`. Lifted `Name` with `pure Name`
in all places it needed to be used inside StateT`s state.
Internal/Syntax/AST.hs:
* Clean imports
* Change type of Node to replace Name with (Maybe Name)
Internal/Syntax/Parser.hs:
* Make nameParser optional
Internal/Syntax/Encoder.hs:
* Self explanatory.
Internal/Validations.hs:
* Rename variables to clearly reflect that they carry a `Maybe
Name` somewhere within rather than a `Name`.
* Change `StateT`'s state type to `Set (Maybe Name)`
* Wrap any `Name` type that needs to go into `StateT`'s state.
Change tests accordingly.
Added tests in ASTTests.hs: - parses anonymous query documents - changed previous test of same name to: parses shorthand syntax documents - parses anonymous query with variables Added tests in ValidationTests.hs: - Treats anonymous queries as valid - Treats anonymous queries with variables as valid
sunwukonga
force-pushed
the
tests/add-anonymousquery-test
branch
from
11b7e62
to
61cc96b
Compare
Remove unused imports Remove redundant import `Name(Name)` Stop exporting mempty from Internal.Name
|
I've just merged #137 (thanks again for your efforts and your patience!). Can you please hit whatever buttons are necessary to give this PR an up-to-date diff from master? |
|
I just wanted to see a diff vs master. GH is currently showing changes that were made in #137 No worries though, since I created such a diff locally. Everything looks great. Thanks for the tests! |
Add tests in ASTTests.hs:
Added tests in ValidationTests.hs: