Fix error on anonymous query

sunwukonga commented 6 years ago

Reproduction

Incoming query:

"{\"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.

Error:

: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.

Solution:

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: #128 and here: #135 and obliquely here: #132

sunwukonga commented 6 years ago

I've created a couple of parser tests (planning validation tests) for this, but before I make a PR, I'd like some feedback on the form of the test. Importantly, the appropriateness of optempty putting the Name field into a state that cannot be reached through makeName i.e. mempty, otherwise known as "".

    it "parses anonymous query documents" $ do
      let query = [r|query {
                       dog {
                         name
                       }
                     }|]
      let Right parsed = parseOnly Parser.queryDocument query
      let expected = AST.QueryDocument
                     [ AST.DefinitionOperation
                       (AST.Query
                         (AST.Node (Name mempty) [] []
                           [ AST.SelectionField
                             (AST.Field Nothing dog [] []
                               [ AST.SelectionField (AST.Field Nothing someName [] [] [])
                               ])
                           ]))
                     ]
      parsed `shouldBe` expected

The upside of this is that an anonymous query can have variables (also tested), as with:

 query ($userId: String!) {
  user(id: $userId) {
    id
    name(duplicateUserIdTest: $userId)
    role
    creationTime: createdAt
  }
}

This query is simplified from an actual test in the test-suite of the elm-graphql client implementation.

One other thing I've noticed is that the Node parser has a check for directives at parser.hs:56. Why are we checking for directives there, when the graphql spec/documentation says that they only occur on fields?

jml commented 6 years ago

One other thing I've noticed is that the Node parser has a check for directives at parser.hs:56. Why are we checking for directives there, when the graphql spec/documentation says that they only occur on fields?

We're treating the spec as authoritative. According to it, directives can be used in the following places:

haskell-graphql / graphql-api