Merge selections in a set into ordered map of fields #92

jml · 2017-01-28T09:24:26Z

Fixes #59

This makes the job of the resolver much simpler. It doesn't have to worry about inline fragments or whatever, it only deals with ordered maps of fields.

Unfortunately, it also makes the job of the validator more complex. Recursing through the whole query to merge fields is a bit involved. Defining some generic operations on OrderedMap made this much easier to think about.

This PR also introduces the notion of types into the validator. We need to know the type of the object we're building a selection set for, and we need to know the definitions of the type conditions. This (to me) means building and providing a value that represents the whole schema, and making sure each resolver has it. This in turn means passing a new value, allTypes down through each resolver. I think this is actually a perfect use case for ReaderT, and would be happy to change it.

It also means changing the top-level methods to only accept GraphQL Objects. This is fine, because that's all their allowed to take: https://facebook.github.io/graphql/#sec-Initial-types

Something that naturally arises from this work is that the difference between leafs and non-leafs becomes really obvious. We talked in person about different approaches, and for this PR I settled on changing the type of resolver to take Maybe (SelectionSet Value), where Nothing indicates a leaf query.

Testing is pretty shallow. I added one end-to-end test that shows off what this can do, but it's kind of limited. Some of the algorithmic & traversal code could probably do with property tests. Let me know which bits you trust least.

Some things on which I'd particularly like input:

The name of ExecutionField (and names of new functions & types in general)
Whether or not to use ReaderT
Ways to reduce the implementation complexity of groupByResponseKey

teh

This changes so much that I have trouble finding my way in. Continuing but some preliminary questions.

teh · 2017-01-28T15:12:53Z

src/GraphQL/Resolver.hs

@@ -134,17 +133,24 @@ data Result a = Result [ResolverError] a deriving (Show, Functor, Eq)
 aggregateResults :: [Result Value] -> Result Value
 aggregateResults r = toValue <$> sequenceA r

+throwE :: Applicative f => ResolverError -> f (Result Value)


is overloading the well-known throwE name a good idea?

We already do this in Validation.hs. Happy to rename, but totally bored of rewriting the same Result expression a dozen times over.

ok, makes sense

teh · 2017-01-28T15:13:35Z

src/GraphQL/Resolver.hs

 instance Applicative Result where
  pure v = Result [] v
  (Result e1 f) <*> (Result e2 x) = Result (e1 <> e2) (f x)

 ok :: Value -> Result Value
 ok = pure

+type AllTypes = Map Name TypeDefinition


Can you add a comment about AllTypes intent? Not super sure from the code.

Something like:

-- | Dictionary of all types in the schema. A map of type names to type definitions.

?

teh · 2017-01-28T15:16:17Z

src/GraphQL/Resolver.hs

+    case getSelectionSet of
+      Left err -> throwE err
+      Right ss -> do
+        handler <- mHandler


Can you re-add the a comment along the lines of "First we run the handler function itself." ? Phrase how you want it but I find communicating that intent here quite important.

I'd much rather put in a comment saying why we run the handler first. Why do we run the handler first?

Ah right. The idea is that we want to run some monadic action that then determines what goes into the fields. E.g. we could be opening a database connection, or delegating to some other service.

Sorry, am still struggling to figure out why this needs to be written down. It feels like writing "do a -> m b so we can pass b to the next thing".

Wrote this:

-- Run the handler so the field resolvers have access to the object. -- This (and other places, including field resolvers) is where user -- code can do things like look up something in a database.

teh · 2017-01-28T15:21:18Z

src/GraphQL/Resolver.hs

+  -- XXX: Might be nicer to make AllTypes a ReaderT -- easier to ignore when
+  -- not wanted, and wouldn't have required me to change so many call sites.
+  -- OTOH, with all our type weirdness, I got bored trying to figure it out.
+  resolve :: AllTypes -> Handler m a -> Maybe (SelectionSetByType Value) -> m (Result Value)


I don't have an answer here but adding a new argument to HasResolver is not good from an API POV IMO. This now means that our users have to carry around this extra argument which is an implementation detail every time they implement HasResolver.

Some reflections. These are mostly unrelated to each other, just a bunch of points loosely on topic.

What's the resolve API for from a users PoV? I genuinely don't know. Knowing that seems the sine qua non in a discussion about usability.

This is essentially a callback API. It's pretty standard for callbacks to provide arguments that are unused in many cases. We already (before this PR) have many instances that don't use arguments.

Where callbacks don't provide arguments that are unused, it's because there are two different callback points. This would equate to two different type classes, or two different methods on this one.

AllTypes is something of a stand-in for a value that represents the entire global schema, not just the element that's being resolved. This makes it something more than just an implementation detail.

If a user did want to write a resolver that had access to the entire schema (as I did), what should they do?

ReaderT is a pretty good compromise here, IMO, since it can be safely ignored by those who don't need it. It also makes it harder to get it wrong by passing a different value into a child resolver (which we alas have to do)

With all the type magic going on, do you really think an extra argument is the critical usability point?

If we intend people to provide implementations of HasResolver, we should document the class and its methods, and say what the contract of the implementations is.

Thinking about it, a different solution would be to change Validation to require a type dictionary (and later, a validated schema), and then substitute the type conditions for the actual types.

All interesting points!

The API (as envisioned by my so far) would be (in the simple case) that I have a new type, e.g.

data BusinessLogicThing = MoneyYes { amount :: Int } deriving Generic

and then

instance HasResolver BusinessLogicThing where resolve handler _ = map businessLogicThingToValue handler

The more complex (and probably more interesting) case is that users implement new combinators, e.g. ReadonlyObject or DynamicSchemaObject (the latter could e.g. figure out what fields it supports on the fly).

I think at this stage I agree with AllTypes being a stand-in for the full schema, and think we should keep it.

I'm also capitulating in terms of design, I really need some larger-project feedback now before making any other decisions.

Summary:

E.g. one thing that I imagine could be be quite useful is to delegate to a different graphql endpoint in the system, like DelegationObject, i.e. we would not know in advance what to do, and we'd need to re-bundle the query and ship it to the other place (maybe giving it a new name along the way).

Cool. I'll make AllTypes a newtype with only a lookup operation so as to make this a bit more obvious.

teh · 2017-01-28T15:41:12Z

src/GraphQL/Internal/Validation.hs

+-- there's an alias, it's the alias, if not, it's the field name.
+type ResponseKey = Name
+
+-- | A field ready to be executed.


executed == resolved?

Sure. You'd rather "resolved"?

Just came across https://facebook.github.io/graphql/#sec-Executing-Fields

teh · 2017-01-28T15:41:19Z

src/GraphQL/Internal/Validation.hs

+type ResponseKey = Name
+
+-- | A field ready to be executed.
+-- XXX: Reviewer, the 'Field' name has been freed up. Should I rename this to 'Field'.


OK, will do.

teh · 2017-01-28T15:42:29Z

src/GraphQL/Internal/Validation.hs

+                          , arguments = arguments field1
+                          , subSelectionSet = Just mergedSet
+                          }
+    _ -> throwE (IncompatibleFields (name field1))


this would be an implementaiton error, right? Maybe we should panic instead?

Nope, it's what happens when someone tries:

{ dog { barkVolume } dog }

Ah I see, how annoying! I wonder what a query language looks like that doesn't allow this .. probably doesn't have fragments!

Well, it doesn't allow it, that's why we're catching it in validation. We just can't reasonably test the pairings without knowing the types, and we're (contra spec) not doing type-level validation atm. In the next major release (if we get there), I expect that the introspection work and pre-query type-checking will go hand-in-hand, much as the resolver & validation work did in this release.

teh · 2017-01-28T16:01:57Z

src/GraphQL/Internal/Schema.hs

+newtype NonEmptyList a = NonEmptyList [a] deriving (Eq, Show, Functor, Foldable)
+
+-- | A thing that defines types. Excludes definitions of input types.
+class DefinesTypes t where


No HasX naming schema?

I actually thought our naming schema was "predicate phrase", but sure. HasTypeDefinitions?

ah right, not sure. We have a mix, can't really place it anywhere so probably fine

$ gg -h "^class " | sort | uniq class BuildFieldResolver m fieldResolverType where class Defaultable a where class FromValue a where class GenericAnnotatedInputType (f :: Type -> Type) where class GenericEnumValues (f :: Type -> Type) where class GenericFromValue (f :: Type -> Type) where class GenericInputObjectFieldDefinitions (f :: Type -> Type) where class GraphQLEnum a where class GraphQLError e where class HasAnnotatedInputType a where class HasAnnotatedType a where class HasArgumentDefinition a where class HasFieldDefinition a where class HasFieldDefinitions a where class HasInterfaceDefinition a where class HasInterfaceDefinitions a where class HasName a where class HasObjectDefinition a where class HasResolver m a where class RunFields m a where class RunUnion m union objects where class ToValue a where class UnionTypeObjectTypeDefinitionList a where

BuildFieldResolver and UnionTypeObjectTypeDefinitionList really stand out in that list.

teh · 2017-01-28T16:09:22Z

src/GraphQL/Resolver.hs

+    where
+      getSelectionSet = do
+        defn <- first SchemaError $ API.getDefinition @(API.Object typeName interfaces fields)
+        (SelectionSet ss') <- first ValidationError $ getSelectionSetForType (flip Map.lookup allTypes) defn selectionSet


Can you add a comment why we need the lookup in allTypes?

Something like...

-- Fields of a selection set may be behind "type conditions", due to inline fragments -- or the use of fragment spreads. These type conditions are represented in the -- schema by the name of a type (e.g. "Dog"). To determine which type conditions -- (and thus which fields) are relevant for this selection set, we need to look up the -- actual types they refer to, as interfaces (say) match objects differently than -- unions. -- -- See <https://facebook.github.io/graphql/#sec-Field-Collection> for more details.

?

jml · 2017-01-28T19:20:45Z

This changes so much that I have trouble finding my way in. Continuing but some preliminary questions.

Thanks for making the effort. I realise it's pretty big, but (aside from the OrderedMap changes) I couldn't break it up into smaller pieces without either having chunks of unused code, or making changes that would seem to be without any justification.

Not actually used yet. Adds new types `SelectionSetByResponseKey` and `ExecutionField`. Also, remove unused language PRAGMA

* Rename `RealSelectionSet` to `SelectionSet` * Stop exporting so much from Validation * Create a dictionary of defined output types * Use that type dictionary to resolve fields * Test to show off some of the new functionality * Remove many categories of error from Resolver, they just aren't possible now

Means that: - validation needs the schema - resolver doesn't

jml mentioned this pull request Jan 28, 2017

Cannot define recursive schema #93

Open

teh reviewed Jan 28, 2017

View reviewed changes

teh approved these changes Jan 29, 2017

View reviewed changes

jml mentioned this pull request Jan 29, 2017

Resolver lacks user-facing documentation #94

Open

teh mentioned this pull request Jan 29, 2017

Homogenize various type class names #95

Closed

teh approved these changes Jan 29, 2017

View reviewed changes

jml added 18 commits January 29, 2017 16:07

Don't mind if we use concatMap instead of =<<

21586d9

Union operations for OrderedMap

14e27bc

Implement catMaybes for OrderedMap

5ad240c

Shuffle some things around

1997e65

Make Arguments and Directives traversable

6d59236

Group selection sets by response key and then type condition

c4d3f80

Not actually used yet. Adds new types `SelectionSetByResponseKey` and `ExecutionField`. Also, remove unused language PRAGMA

Implement doesFragmentTypeApply

9f49777

Merge selection sets, eliminating type information

4d18be6

Shuffle stuff around. No code changes.

20b14a7

Some more docs

e7e7386

Rename ExecutionField to Field

2eb54c6

Internal comments

6c4a900

Make AllTypes 'Schema', opaque and in internal

52d15f1

Pipe through an unused schema to validation

ed57836

Implement Ord in Schema

1e929bf

Lookup types in validation phase

43c95b4

Means that: - validation needs the schema - resolver doesn't

Don't pass schema through Resolver

fe03260

jml force-pushed the selection-set-2 branch from fea07bb to fe03260 Compare January 29, 2017 16:08

Update WAI for new API

5b8be19

jml merged commit dfcd95a into master Jan 29, 2017

jml deleted the selection-set-2 branch January 29, 2017 16:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge selections in a set into ordered map of fields #92

Merge selections in a set into ordered map of fields #92

jml commented Jan 28, 2017

teh left a comment

teh Jan 28, 2017

jml Jan 28, 2017

teh Jan 29, 2017

teh Jan 28, 2017

jml Jan 28, 2017

teh Jan 28, 2017

jml Jan 28, 2017

teh Jan 28, 2017

jml Jan 29, 2017

teh Jan 28, 2017

jml Jan 28, 2017

jml Jan 28, 2017

teh Jan 28, 2017

teh Jan 28, 2017

jml Jan 29, 2017

teh Jan 28, 2017

jml Jan 28, 2017

jml Jan 28, 2017

teh Jan 28, 2017

jml Jan 28, 2017

teh Jan 28, 2017

jml Jan 28, 2017

teh Jan 28, 2017

jml Jan 29, 2017

teh Jan 28, 2017

jml Jan 28, 2017

teh Jan 28, 2017

jml Jan 29, 2017

teh Jan 28, 2017

jml Jan 28, 2017 •

edited

Loading

jml commented Jan 28, 2017

Merge selections in a set into ordered map of fields #92

Merge selections in a set into ordered map of fields #92

Conversation

jml commented Jan 28, 2017

teh 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

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 Jan 28, 2017 • edited Loading

Choose a reason for hiding this comment

jml commented Jan 28, 2017

jml Jan 28, 2017 •

edited

Loading