Guaranteed correct name in AST #42

jml · 2016-12-23T11:56:19Z

Move all the name stuff from Value to AST
Guarantee that only valid names ever appear in the AST or anywhere else
Add a HasName type class that is implemented by all named things in the schema (can be extended to other things later, this is just the only place I needed it)
Change the getFooDefinition methods to return Either NameError Foo. This involves changing most of the code to be applicative, which in turn involves a fair chunk of using the composition operator. Sorry.
While doing that, removed some duplication (mostly because it was less boring than typing the same thing twice)
Update Server module to use the API module and to get names from there, rather than directly from symbols. This simplifies some of the types (but I tried to tread lightly here). Dodgy names are now thrown as QueryError. I think we can do better than this, but await your error handling branch.

New TODOs:

src/GraphQL/API.hs:
  -- TODO(jml): Given that AnnotatedType is parametrised, we can probably reduce
  -- a great deal of duplication by making HasAnnotatedType a parametrised type
  -- class.

src/GraphQL/API.hs:
  -- TODO(jml): Be smarter and figure out how to say "all integral types" rather
  -- than listing each individually.

src/GraphQL/Server.hs:
  -- TODO(jml): I don't understand why I can't extract (Object typeName interfaces fields)
  -- from this Union instance as I did for the above Union instance.

Instead of using it in Server, use the already-existing getFooDefinition methods.

jml · 2016-12-23T11:58:35Z

Should point out that you can tell where we can use more structure in error type by searching for formatNameError.

teh · 2016-12-23T12:20:11Z

src/GraphQL/API.hs

@@ -78,25 +81,31 @@ data Argument (name :: Symbol) (argType :: Type)
 -- https://hackage.haskell.org/package/optional-args-1.0.1)
 data DefaultArgument (name :: Symbol) (argType :: Type)

+-- | Convert a type-level 'Symbol' into a GraphQL 'Name'.
+nameFromSymbol :: forall (n :: Symbol) (proxy :: Symbol -> *). KnownSymbol n => proxy n -> Either NameError Name


if you feel adventurous you could use type application here as well for consistency with the rest of our APIs. I.e.: nameFromSymbol @name

I felt a little bit adventurous and tried this earlier, but couldn't figure it out in the time I had allotted.

ack, if you add a todo I'll do it

teh · 2016-12-23T12:24:19Z

src/GraphQL/Internal/AST.hs

+-- | A name in GraphQL.
+--
+-- https://facebook.github.io/graphql/#sec-Names
+newtype Name = Name { getNameText :: Text } deriving (Eq, Ord, Show)


where is the getNameText style from? I find it a bit weird but can of course get used to it. I guess the main point is so people can write point-free instead of unpacking in the argument? I find the argument-unpacking more readable personally because it doesn't introduce a new identifier that I have to map back to the original type in my head, but that's very subjective.

It's just record field syntax. It's pretty common for newtypes as a way of unwrapping the type.

Main point is composition, not point-free style. (e.g. map getNameText names). Encoder has a couple of motivating examples. Everything could be written with pattern matching, but we'd have to do much more typing.

Also note that if we wanted to use argument unpacking, we'd have to export a view pattern, because we don't want to export the constructor.

Maybe renaming it to unName or nameToText would make it clearer? Similarly, I could write a whole separate function definition that does it.

Ah I see, thanks for explaining! Not exporting the constructor is a good point. I think I'd then prefer getName or unName (like unwrap in https://github.com/purescript/purescript-newtype/blob/master/src/Data/Newtype.purs)

teh · 2016-12-23T12:29:39Z

src/GraphQL/Internal/AST.hs

@@ -76,12 +130,12 @@ data OperationDefinition = Query    { getNode :: Node }
                         | Mutation { getNode :: Node }
                           deriving (Eq,Show)

-data Node = Node Name [VariableDefinition] [Directive] SelectionSet
+data Node = Node (Maybe Name) [VariableDefinition] [Directive] SelectionSet


Node is one of those types that would benefit from named fields IMO. Why is the name Maybe? Should node creation fail if the name is invalid?

Imagine:

{ me { name } }

The top-level node has no name. There can only be one such node in a document. I'm open to better ideas on how to represent this.

In #40, I added tests demonstrating the broken behaviour of the parser in this area. We probably will have to change the types to deal with this, but I'd rather not in this PR.

Ah I see, it's a parser limitation. OK for now then, though not a huge fan. Can you add a note / todo?

teh · 2016-12-23T12:31:17Z

src/GraphQL/Internal/AST.hs

@@ -76,12 +130,12 @@ data OperationDefinition = Query    { getNode :: Node }
                         | Mutation { getNode :: Node }
                           deriving (Eq,Show)

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

 -- XXX: Lots of things have names. Maybe we should define a typeclass for


-1 from me I think. We already have some pretty weird compiler errors because we're writing generic code. Nailing certain types into place (instead of type classes or type families) does reduce the overall movement and makes compiler errors more readable.

To be clear, are you suggesting that I remove the HasName type class and all of its instances and replace them with specifically named functions that each do the job at hand?

Even golang supports this level of genericness.

No it's fine I misunderstood what you were suggesting.

teh · 2016-12-23T12:34:23Z

src/GraphQL/Internal/Encoder.hs

-       <> ":"
-       <> type_ ty
+  AST.getNameText name <> optempty argumentsDefinition args
+                   <> ":"


indentation seems off?

teh · 2016-12-23T12:36:27Z

src/GraphQL/Internal/Parser.hs

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

 node :: Parser AST.Node
-node = AST.Node <$> AST.nameParser
+node = AST.Node <$> (pure <$> AST.nameParser)


same comment as above: if the name is invalid why not bail?

It's not invalid. It's an anonymous operation.

teh · 2016-12-23T12:41:49Z

src/GraphQL/Internal/Validation.hs

@@ -64,7 +64,7 @@ validate :: Alternative m => AST.Document -> m ValidDocument
 validate = pure . Valid

 data ValidationError
-  = DuplicateOperation AST.Name
+  = DuplicateOperation (Maybe AST.Name)


Not sure why this is Maybe either .. what's a DuplicateOperation?

An "operation" in a GraphQL document is a query or a mutation. e.g.

query foo { ... } mutation bar { ... }

You cannot have more than one operation with the same name.

DuplicateOperation is the error that means that you have more than one operation with the same name.

Because GraphQL allows anonymous queries, you can theoretically write:

{ me { name } } { me { age } }

But this is also invalid, because it has more than one anonymous query.

This validation code takes the cheap approach of treating that as a DuplicateOperation Nothing. I could adjust the data type to have separate errors for these cases, if you'd like

I know it's extra work but maybe it's worth adding a new Maybe-like sum type for unnamed queries? It's not super clear what Maybe means in this context. I do know now though, so up to you.

teh · 2016-12-23T12:48:47Z

src/GraphQL/Server.hs

+         , HasObjectDefinition object
+         ) => RunUnion m (object :<|> rest) where
+  runUnion (lh :<|> rh) fragment@(AST.SelectionInlineFragment (AST.InlineFragment (AST.NamedType queryTypeName) [] subSelection)) =
+    case getDefinition @object of


I don't think that's the right place to handle definition errors, design wise. IMO we should get the definition once before running any resolver code, and catch all errors at that stage. By moving the getDefinition error handling into the resolver we obfuscate what's going on.

tl;dr - I'd vastly prefer not to have this code here but instead in a pre-resolver step.

tl;dr - I'd vastly prefer not to have this code here but instead in a pre-resolver step.

Me too, but this is the first step to getting there.

Specifically, as long as this code needs to read out of symbols (either via getDefinition or by arbitrarily duplicating some of the logic in getDefinition), it will need to handle errors like this. Our options are to panic (which means the compiler does less work for us and we need to be more careful), to mask the error (which we were doing before), or to represent the error explicitly.

By making it explicit, we're giving ourselves to tools to improve it.

I think I'm bad at explaining. I don't think we should check schema validity at all in buildResolver. IIUC we can do a full validity check by running getDefinition on the top level once, and then call buildResolver. I.e. our top-level entry point would look like this:

buildServer = do d <- getDefinition @IO @API case d of Left err -> throwError "why are you doing this to me" Right _ -> buildResolver @IO @API

does that make sense? I think control flow in buildResolver is complicated enough without us bringing getDefinition in.

There's also a secondary concern which is that we're now checking validity on each request, even though the schema is compiled and therefore never changes. I suspect that the GHC optimizer can float number of checks up but I'd rather have a execute-once by construction.

so yea, what I am saying is we should ignore the error (raise a patter match failure) for easier control flow

But I can see your reaction already :)

I.e. leave it in, I'll work with what we have

jml · 2016-12-23T13:33:33Z

Build failure is CircleCI being buggy.

In the end I've made very few changes because I'm not clear what the actionable feedback is.

teh

lgtm though I'm not super happy about pulling in HasObjectDefinition and schema checks at runtime as mentioned.

teh · 2016-12-23T15:57:37Z

src/GraphQL/Server.hs

-    where typeName = toS (symbolVal (Proxy :: Proxy typeName))
+         , HasGraph m object
+         , HasObjectDefinition object
+         ) => RunUnion m (object :<|> rest) where


Replacing "Object .." works in this instance because we only have Objects to stick into unions but in the general case we need to spell out the full Object name interfaces fields for the type checker. (not actionable but please don't replace any of the other instances)

teh · 2016-12-27T19:02:31Z

Survived Christmas :)

Can I merge this?

jml · 2016-12-30T11:38:11Z

Merged. Sorry for the delay.

@teh

As per @teh suggestion in #42

jml added 13 commits December 22, 2016 15:17

Ignore tutorial stack work

d874d36

Make AST.Name a protected type

5eae1f7

Admit possibility of failing names

f19e742

Do not panic when making schemas

a77d786

Rename getName to getNameText

f5f26fe

More error knowledge in AST

e42254b

Type class for getting names

6373393

Type class for HasArgumentDefinition

4d8c14d

Make Int32 an annotated type

0e336b3

Kill unsafeNameFromSymbol

85d1b1d

Instead of using it in Server, use the already-existing getFooDefinition methods.

Throw query errors on unsafe names, rather than panic

b6a5d36

Remove spurious TODO

f5fd20e

Fix broken doc test

bfcac46

teh requested changes Dec 23, 2016

View reviewed changes

Fix indentation

307b56c

teh approved these changes Dec 23, 2016

View reviewed changes

jml merged commit dd93fb9 into master Dec 30, 2016

jml deleted the safe-ast-name branch December 30, 2016 11:38

This was referenced Dec 31, 2016

Anonymous queries #45

Merged

Validate schema before using it for resolution #62

Open

jml added a commit that referenced this pull request Jan 20, 2017

Rename getNameText to unName

67c62ff

As per @teh suggestion in #42

jml mentioned this pull request Jan 20, 2017

Rename getNameText to unName #82

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Guaranteed correct name in AST #42

Guaranteed correct name in AST #42

jml commented Dec 23, 2016

jml commented Dec 23, 2016

teh Dec 23, 2016

jml Dec 23, 2016

teh Dec 23, 2016

teh Dec 23, 2016

jml Dec 23, 2016

teh Dec 23, 2016

teh Dec 23, 2016

jml Dec 23, 2016

jml Dec 23, 2016

teh Dec 23, 2016

teh Dec 23, 2016

jml Dec 23, 2016

teh Dec 23, 2016

teh Dec 23, 2016

jml Dec 23, 2016

teh Dec 23, 2016

jml Dec 23, 2016

teh Dec 23, 2016

jml Dec 23, 2016

teh Dec 23, 2016

teh Dec 23, 2016

jml Dec 23, 2016

jml Dec 23, 2016

teh Dec 23, 2016

teh Dec 23, 2016

teh Dec 23, 2016

jml commented Dec 23, 2016

teh left a comment

teh Dec 23, 2016

teh commented Dec 27, 2016

jml commented Dec 30, 2016

Guaranteed correct name in AST #42

Guaranteed correct name in AST #42

Conversation

jml commented Dec 23, 2016

jml commented Dec 23, 2016

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

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

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

jml commented Dec 23, 2016

teh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teh commented Dec 27, 2016

jml commented Dec 30, 2016