Problem

As mentioned in #2509 (comment), the way we currently parse and validate GraphQL queries leaves too much parsing logic for resolvers. This leads to a proliferation of throw500 calls in resolvers, which could be ruled impossible by the type system with a more structured approach, but currently cause runtime errors.

This has historically been a source of real bugs, such as the recent #2754.

Example: Boolean expressions

To give a concrete example, let’s consider the way we currently resolve GraphQL boolean expressions. The relevant code is in src-lib/Hasura/GraphQL/Resolve/BoolExp.hs, which provides the following function:

parseBoolExp
  :: (MonadError QErr m, MonadReader r m, Has FieldMap r)
  => AnnInpVal -> m (AnnBoolExp UnresolvedVal)

Just looking at the types of this function, it’s clear that it must be doing some kind of parsing. The result type, AnnBoolExp, is a type alias for a sum type with a very particular shape:

data GBoolExp a
  = BoolAnd ![GBoolExp a]
  | BoolOr  ![GBoolExp a]
  | BoolNot !(GBoolExp a)
  | BoolFld !a

However, the only input to parseBoolExp has type AnnInpVal, which is little more than a small wrapper around an AnnGValue, which is a very general type that essentially describes an arbitrary GraphQL value:

data AnnGValue
  = AGScalar !PGScalarType !(Maybe PGScalarValue)
  | AGEnum !G.NamedType !AnnGEnumValue
  | AGObject !G.NamedType !(Maybe AnnGObject)
  | AGArray !G.ListType !(Maybe [AnnInpVal])
  deriving (Show, Eq)

This is not necessarily a bad thing—parseBoolExp is doing parsing, after all—but the problem is that parseBoolExp should not actually ever fail. It’s a bug if it does! Why is that? Well, we’ve actually already checked that the AnnInpVal has the proper shape, since we checked it against GraphQL schema we generated.

Specifically, during schema generation, we generate InpObjTyInfos that describe the type of boolean expressions. When we validate the GraphQL schema, we call validateObject in src-lib/Hasura/GraphQL/Validate/InputValue.hs to parse an AnnGObject result. The problem, however, is that AnnGObject is far too coarse-grained: really, we’ve just done the work to parse a boolean expression, but we’ve thrown that information away and turned it back into a fairly meaningless hash map.

This means that parseBoolExp has to do that very same parsing logic again, and dangerously, if the schema we generate and parseBoolExp get out of sync, we don’t get any compile-time feedback—we just get runtime errors!

Proposed Solution

In my mind, executing a GraphQL query is a three-pass process:

1. First, we parse the query text, which ensures that the input given by the user is well-formed.

2. Second, we resolve the parsed query, which bridges the gap between GraphQL’s graph-based object model and SQL’s relational model.

3. Third, we execute the query by optimizing the resolved SQL query, preparing and/or multiplexing it, and sending it over the wire to Postgres.

Step 1 can clearly fail if the user sends us a malformed query. Step 3 can fail for various reasons if Postgres returns an error to us. But I don’t think Step 2 should ever fail, and I think we should be able to prove it with the type system. To do that, we’ll need to move the parsing logic that’s leaked into the resolvers back into the parsing code.

RQL vs GraphQL

It’s worth thinking for a moment about why RQL doesn’t have this problem. In RQL, parsing does produce structured datatypes. For example, RQL’s version of boolean expression parsing, parseGBoolExp, turns a JSON Value into a GBoolExp directly. Any parse errors there are not internal errors, they’re actually parse errors that indicate the input query was malformed.

That works for RQL, but we can’t easily do that for GraphQL. Why? Well, with GraphQL, we also need to be able to introspect the schema, while the “schema” in RQL cannot be reified as a datatype since it’s just implicitly defined by whatever the parsing code does.

Typed schema generation

Given the above, I think the solution here actually involves fusing schema generation and schema validation. Like with RQL, our GraphQL schema should arise from our parsing code—that way they can never get out of sync—but unlike with RQL, we also need an explicit representation of the schema itself.

Therefore, I think the right solution here is to provide an API much like Aeson’s Parser, except restricted to allow it to be used both ways:

runParser :: Parser a b -> a -> Either ParseError b
parserSchema :: Parser a b -> TypeMap

To implement this, we’d provide a set of combinators, similar to Aeson’s withObject and withArray combinators. For example, we have might combinators like these:

parseSelectionSet
  :: G.NamedType
  -> HashMap G.Name (Parser Field a)
  -> Parser SelectionSet [a]

parseField
  :: G.NamedType
  -> Parser [Argument] a
  -> Parser SelectionSet b
  -> Parser Field (a, b)

parseScalar
  :: (FromJSON a)
  => G.NamedType
  -> Parser Value a 

Our Parser would be more restrictive than Aeson’s Parser, since our Parser can’t be a monad—that would make it impossible to implement parserSchema—but it should be able to implement Functor, Applicative, Profunctor, Strong, Choice, Arrow, and ArrowChoice. We could even use arrow notation for composing them together, if we found it useful.

This approach would have the interesting effect of changing GraphQL schema generation to have roughly the following type:

data GQLOperation a
  = GOSelect !(QueryRootFldAST a)
  | GOInsert !(AnnInsObjG a)
  | GOUpdate !(AnnUpdG a)
  | GODelete !(AnnDelG a)

mkGCtxMap
  :: (MonadError QErr m)
  => TableCache PGColumnInfo
  -> FunctionCache
  -> m (HashMap RoleName (Parser QueryParts (GQLOperation UnresolvedVal)))

Introspection can be performed by calling parserSchema and handing the result to schemaR as usual, but validation and resolution become much simpler: all you have to do is call runParser on the input, and the resolvers already have access to the parsed, typed query AST!

Impact on code organization

This change would obviously be very invasive. It would require restructuring significant chunks of the code in src/Hasura/GraphQL/{Resolve,Schema,Validate}/. Specifically:

1. The functionality in Validate/ would basically become the new Parser combinators.

2. The parsing code in Resolve/ would be relocated into Schema/, leaving Resolve/ significantly simpler.

3. The Schema/ code would be the most dramatically affected, since instead of generating just the GraphQL schema, it would be building up the query Parser as well. However, my bet is that the code size would not actually increase that significantly, since a lot of the parsing logic in Resolve/ was actually duplicating structure in Schema/. That duplication would go away.

The changes in Schema/

Most of the Schema/ code would change in a fairly predictable way: where it currently calls mkHsraObjFldInfo, it would instead call parseSelectionSet, and so on. Much of the code would not actually change in structure.

However, one significant difference would be a change in the way type references work. Currently, a GraphQL type that references another type just includes its name inside of it, so for example, an input object type that contains a boolean expression simply passes G.NamedType $ G.Name (qualObjectToName tableName <> "_bool_exp") to mkHsraInpTyInfo. In the new approach, the parseInputObject combinator would actually need the boolean expression’s parser. How would that work, especially with mutually-recursive types?

If we called parseBooleanExpression to build a new parser every time we needed to use the boolean expression type, it would be a disaster, since we’d be generating an infinite number of boolean expressions (boolean expressions are recursive!). To avoid that, we can take advantage of Haskell’s laziness. We can create a memoized version of parseBooleanExpression, which can then be passed to other functions that need to construct boolean expressions (including parseBooleanExpression itself). Laziness will tie the recursive knots for us, and we’ll end up with a clean parser graph rather than an infinitely large data structure.