Refactor GraphQL execution

[lutter]

Refactor how we execute GraphQL queries by transforming the AST we get from graphql_parser into a form that is easier to execute. The preprocessing step that translates from graphql_parser's AST to our AST does the following:

• interpolate variables
• coerce argument values to their correct type
• expand fragments so that our selection sets only contain fields
• resolve interfaces (and unions) into their constituent object types

This removes a lot of hard-to-follow code in query execution, especially in prefetch.

Besides these simplifications, this PR also fixes a few bugs, and changes the order in which fields appear in the response to what the GraphQL spec mandates. This is a breaking change for the network, as it changes the hash for semantically equivalent query responses (cc @That3Percent on rolling this out on the network)

The refactored code also makes a few weaknesses of the current query execution visible, in particular, field merges (from users specifying the same field multiple times, or from fragment expansion) continue to use a yolo strategy that is not entirely standard conformant. This needs to be addressed in a separate PR.

Fixes #2961.

[kamilkisiela]

@lutter Hi, I started a Pull Request for graphql_parser that introduces Document Transformer graphql-rust/graphql-parser#56

Not sure it's useful but with such trait, we could have one API for transformations and split the transformation logic into multiple pieces. What do you think?

[leoyvens]

Small comments from reading ast.rs.

[leoyvens]

Is items.len() > 1 only when the selection is over a union or interface? If so it would be clarifying to note that in the comment.

[leoyvens]

It would be nice to use a type for strings that represent object names (or type names in general, whatever makes more sense).

[lutter]

I've wanted to use something like &s::ObjectType here, but that's not possible for various reasons. I've now resolved to do the next best thing, which is keeping a copy of all object types in ApiSchema, and wrapping them in an Arc. With that, a SelectionSet now has basically items: Vec<(Arc<s::ObjectType>, Vec<Field>)> (in reality a little more complicated to work around other shortcomings of s::ObjectType)

I am curious what you think of that approach.

[leoyvens]

Looks good! I see that saved some calls to get_object_type_definition.

[lutter]

Added a comment to clarify when items.len() > 1

[That3Percent]

Our arbitration charter allows for flexibility around bugfixes, so we are good to go to push this to The Network @lutter.

[lutter]

@lutter Hi, I started a Pull Request for graphql_parser that introduces Document Transformer graphql-rust/graphql-parser#56

Not sure it's useful but with such trait, we could have one API for transformations and split the transformation logic into multiple pieces. What do you think?

I saw that PR and was sad that it was just a PR (I even went as far as trying to implement a visitor, but then decided it's too much work for what we need) Now that I've been through this refactor, I actually think it would be better if graphql_parser was more opinionated than just offering transformers, and instead had something along the lines of what this PR does, i.e., a preprocessing step for GraphQL queries. A lot of this PR is very generic, and useful for anyone who wants to execute GraphQL queries, but by necessity closely tied to our codebase. The things that I think should live upstream are

• the graph::data::graphql::ext module which is just an attempt to make the graphql_parser API more ergonomic (in graphql_parser, these could just be methods on its types, rather than the awkward dance with extension traits)
• the graphql::execution::ast module and the transformation to turn a graphql_parser query into that form in graphql::execution::query
• the IntrospectionResolver and the code that merges the introspection schema into the user's schema (APISchema.schema in our case) Keeping the introspection schema and the user schema separate lead to a lot of awkward code which is now gone, but even that you have to switch resolvers is a bit awkward.

The biggest question around doing that to me is how values should be handled, since they are user-specific, and the graphql_parser API for the above would need to be templated in some form around the concrete notion of Value, and, for example, how to coerce a q::Value to the user's notion of Value.

Besides these, one big feature request I'd have for graphql_parser is to use more Arcs in the schema AST, especially for types, since the current data structures make it hard to keep pointers to types around, and you end up referencing them with strings that need to be looked up in the schema all the time.

I have no idea what the upstream devs would think about these changes, but it would make our lives a lot easier.

[lutter]

Addressed all the comments so far

[lutter]

Rebased to latest master and a dded a few commits to replicate the behavior of the current GraphQL execution for some queries that are really invalid

[lutter]

This PR is now ready for review (and hopefully merging/rollout soon)

I've tested this branch against all the queries we get in the hosted service, and the only difference in responses comes from treating the (invalid) construct of not having a selection on a field with an object type slightly differently: the current implementation includes an empty object (or list of empty objects, depending on the field type) in the response, with this PR such fields do not appear in the response.

In other words, for a type like

type Person @entity {
  id ID!
  spouse Person!
  friends [Person!]!
}

and the query

{ persons { id spouse friends } }

with current master, the response has { "id": "..", "spouse": {}, friends: [ {}, {}, {}]} for each person in it (with one empty object in the list for each friend). With this PR, the response would contain neither a spouse nor a friends field. The impact on apps should be negligible since they've been getting no real data all along.