Fix error on anonymous query #137

sunwukonga · 2017-12-20T14:16:02Z

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:

😊 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

Following todo formerly in src/GraphQL/Internal/Syntax/AST.hs: TODO: Just make Node implement HasName. Declared Node as instance of HasName and wrote implementation of getname for it. Because of a cyclic dependency between Name and AST, moved the Name specific code from GraphQL.Internal.Syntax.AST module into the GraphQL.Internal.Name module. Updated imports and exposures in the AST and Name modules described above. Simple import and qualified name changes to: GraphQL/Internal/Syntax/Encoder GraphQL/Internal/Syntax/Parser

Was compiling before, but Name was using Text from some place else.

``` "{\"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:

sunwukonga · 2017-12-22T08:08:29Z

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?

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))

jml

Thanks @sunwukonga.

I've got a few questions.

jml · 2018-01-03T11:48:20Z

src/GraphQL/Internal/Name.hs

@@ -21,19 +21,19 @@ import Protolude
 import qualified Data.Aeson as Aeson
 import GHC.TypeLits (Symbol, KnownSymbol, symbolVal)
 import Data.Char (isDigit)
+import Data.Text as T (Text)


Why did you do this? The Protolude import gets us exactly this Text.

jml · 2018-01-03T11:48:38Z

src/GraphQL/Internal/Syntax/AST.hs

@@ -48,7 +48,7 @@ module GraphQL.Internal.Syntax.AST

 import Protolude

-import Data.String (IsString(..))
+--import Data.String (IsString(..))


Please just delete, rather than comment out.

jml · 2018-01-03T11:50:37Z

src/GraphQL/Internal/Name.hs

+import qualified Data.Attoparsec.Text as A
+import Test.QuickCheck (Arbitrary(..), elements, listOf)
+import Data.String (IsString(..))
+import Data.Text as T (Text, append, empty)


Don't need to import Text here as it's already imported four lines above.

Fixed. Waiting on Name "" vs Nothing to push.

jml · 2018-01-03T11:52:50Z

src/GraphQL/Internal/Name.hs

+-- mempty rather than propagating a failure. 
+instance Monoid Name where
+    mempty  = Name T.empty
+    mappend (Name a1) (Name a2) = Name (T.append a1 a2)


Rather than importing T.empty and T.append, I'd suggest just using mempty and mempty, e.g.

instance Monoid Name where mempty = Name mempty mappend (Name a1) (Name a2) = Name (mappend a1 a2)

Looking at that, if you enable the GeneralizedNewtypeDeriving (sp?) language option, you could just change Name to derive Monoid to get the same definition.

Yes, this was right. The GeneralizedNewtypeDeriving with derived Monoid obsoleted this code.

jml · 2018-01-03T11:55:32Z

src/GraphQL/Internal/Syntax/Parser.hs

@@ -51,7 +52,7 @@ operationDefinition =
  <?> "operationDefinition error!"

 node :: Parser AST.Node
-node = AST.Node <$> AST.nameParser
+node = AST.Node <$> optempty nameParser


Why did you decide to go for optempty rather than optional?

My natural approach for modelling anonymous nodes would be for them to have a name of Nothing, rather than a name of Name "".

I didn't because it was a deeper change. I would have to change the definition of

class HasName a where getName :: a -> Name

to

class HasName a where getName :: a -> Maybe Name

and then handle the Maybe everywhere that getName is used. Is this a direction you'd like to go?

Hmm. Changing the typeclass seems wrong too, since names are only optional for nodes (IIUC).

I guess if we went this direction, we'd drop the typeclass for nodes and re-instate getNodeName :: Node -> Maybe Name.

Well, that's amusing. I suppose that's why Node was NOT an instance of HasName to begin with.

It would have been much simpler to change:

getNodeName :: Node -> Name getNodeName (Node name _ _ _) = name

to

getNodeName :: Node -> Maybe Name getNodeName (Node name _ _ _) = name

with

data Node = Node (Maybe Name) [VariableDefinition] [Directive] SelectionSet deriving (Eq,Show)

The only way to use getName of HasName without changing it would be to use it poorly by returning an empty Name when when encounter a Node with Nothing as Name.

I suppose that's why Node was NOT an instance of HasName to begin with.

Could well have been. To be honest, I can't remember the details.

It would have been much simpler…

Yeah, it would have. I'm sorry I didn't think of this at the time of #134.

Which would you prefer to do?

Go back to the original getNodeName and have it return Maybe Name with the corresponding change in Node. Cleaner with Maybe, but ugly and non-unified get function.

Keep instance of HasName for Node, change Node to

data Node = Node (Maybe Name) [VariableDefinition] [Directive] SelectionSet deriving (Eq,Show)

AND have the getName return an empty Name when we encounter Nothing instead of Just Name "somename"
3. Soldier on, allowing Node to contain an empty Name. One point to make is that the empty Name is not usually reachable through the normal channel for creating a Name, so it really does only represent nothing.
4. Something I haven't considered...

Please:

change Node to be Node (Maybe Name) [VariableDefinition] [Directive] SelectionSet

go back to original getNodeName, except returning Maybe Name

drop the HasName instance for Node

I'm sorry for the confusion here. I did a bit of digging and it looks like the spec was changed to make names for Node optional while we were in the middle of implementing graphql-api.

Will do. Tomorrow. I'm at GMT+8 :)

Turns out to not be that simple. There is some interaction between Name values and the new Maybe Name values in Validation.hs

At line 140 of Validation.hs visitedFrags which contains a Set of Name and usedFrags which contains a Set of Maybe Name are combined. I'm not yet sure how to handle this.

jml · 2018-01-03T12:35:49Z

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:

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.

jml

Thanks @sunwukonga!

jml · 2018-01-14T14:07:26Z

src/GraphQL/Internal/Validation.hs

    assertAllFragmentsUsed fragments used =
-      let unused = Map.keysSet fragments `Set.difference` used
+      let unused = ( Set.map pure (Map.keysSet fragments)) `Set.difference` used


Are names of fragments optional?

They are not. What's happening here is that we've changed the type of the "observed names" set. I don't think it's strictly necessary (you can't have anonymous fragments, so there's no point recording that we've seen Nothing), but it only introduces a minor error message regression that we can fix easily enough later, leaving this PR a net improvement.

There were two possibilities here, if I understood things correctly.

Leave Node Names packed in a Maybe, with the consequence that the accumulated state of StateT must change to contain "Maybe Name" as well. Since names into StateT can also come from Fragments, those names also needed to be packed into Maybes (this seemed like the safest approach to me, and although it does change error messages associated with fragments to imply that Fragment Names can be Nothing [Is this the "minor error message regression"?], in actuality, they never would be).

Unpack Node Names...
a. Under the assumption that Nodes MUST be named in this context (I was not at all sure of this) -> throw an error if Nothing.
b. Under the assumption that Node Names can be Nothing in this context -> No way to proceed (or rather, only hacky ways to proceed, i.e. Set name to "").

That's mostly right.

The StateT is there to track which fragments have been visited. Since all fragments have names, it's impossible for a node without a name visit a fragment. That means if we find an anonymous node, we don't need to update the StateT.

(Multi-tasking heavily at the moment, so I might be horribly wrong).

sunwukonga added 3 commits December 16, 2017 12:18

Add Data.Text import and clean up unused imports.

d27f591

Was compiling before, but Name was using Text from some place else.

Cut commented out code. Comment monoid instance.

0a96e00

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))

jml reviewed Jan 3, 2018

View reviewed changes

jml mentioned this pull request Jan 3, 2018

Add tests for anonymous queries w|w/o variables #139

Merged

jml mentioned this pull request Jan 3, 2018

Revise for October2016 spec release #143

Open

sunwukonga added 2 commits January 5, 2018 11:22

Remove deriving Monoid from Name.

2bf5c01

jml approved these changes Jan 14, 2018

View reviewed changes

jml merged commit d542328 into haskell-graphql:master Jan 14, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix error on anonymous query #137

Fix error on anonymous query #137

sunwukonga commented Dec 20, 2017

sunwukonga commented Dec 22, 2017

jml left a comment •

edited

Loading

jml Jan 3, 2018

jml Jan 3, 2018

jml Jan 3, 2018

sunwukonga Jan 3, 2018

jml Jan 3, 2018

sunwukonga Jan 3, 2018

jml Jan 3, 2018

sunwukonga Jan 3, 2018

jml Jan 3, 2018

sunwukonga Jan 3, 2018 •

edited

Loading

jml Jan 3, 2018

sunwukonga Jan 3, 2018 •

edited

Loading

jml Jan 3, 2018

sunwukonga Jan 3, 2018 •

edited

Loading

sunwukonga Jan 4, 2018

jml commented Jan 3, 2018

jml left a comment

jml Jan 14, 2018

jml Jan 14, 2018

sunwukonga Jan 16, 2018

jml Jan 16, 2018

Fix error on anonymous query #137

Fix error on anonymous query #137

Conversation

sunwukonga commented Dec 20, 2017

Reproduction

Incoming query:

This gets decoded to:

Error:

Solution:

sunwukonga commented Dec 22, 2017

jml left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunwukonga Jan 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunwukonga Jan 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunwukonga Jan 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jml commented Jan 3, 2018

jml left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jml left a comment •

edited

Loading

sunwukonga Jan 3, 2018 •

edited

Loading

sunwukonga Jan 3, 2018 •

edited

Loading

sunwukonga Jan 3, 2018 •

edited

Loading