Schema and statement validation prior to execution

[aew]

Fixes a number of outstanding bugs and incompatibilities between validation output and execution input. Backwards compatible.

Changes include:

• Perform schema and statement validation prior to execution, with more robust error handling (backwards compatible)
• Line and column numbers in validation error output
• Fix bug that could cause an infinite loop in the absence of a parent spec
• Enable memoization of schema and statement preparation prior to execution
• Post-process validated schema to eliminate unnecessary complexity
• Fix potential collision of specs for root query and mutation fields sharing the same type
• Support validation of arguments for root fields
• Fix bug in validation of unused fragments
• Fix bugs in validation of scalar leafs for lists of scalars and lists of objects
• Deprecate graphql-clj.type/create-schema in favor of graphql-clj.validator/validate-schema
• Fix bug in fragment cycles validation
• Properly assign specs for recursively nested fields (switch to pre-order traversal for adding statement specs)

This is a large change because the parser was eliminating line and column metadata necessary for producing validation error messages. The executor was coupled to parser output rather than validator output. This PR proposes that the executor take the validator output as an input rather than the parser output.

Related discussion: https://github.com/tendant/graphql-clj/issues/22

[tendant]

To maintain backward compatibility, it is better to create new version of type/create-schema with validation build-in.

[aew]

Initially, I didn't add a new version of create-schema because this creates a circular dependency. Maybe there is another way around this - will take a look.

[EDIT] - actually it's quite easy to avoid the circular dependency - I've made this change.

[tendant]

I am testing the new version. Looks like the introspection query is not working properly.

I am using https://github.com/tendant/graphql-clj-starter for testing introspection query.

[aew]

I'm seeing the error: Cannot query field 'subscriptionType' on type '__Schema'. I don't see the subscriptionType field on the introspection schema. Trying to understand where it's coming from.

https://github.com/graphql/graphiql/blob/0172a425036299b1d99dbd1ae82aa37f31781866/src/utility/introspectionQueries.js#L12

[tendant]

GraphiQL sends first query with subscriptionType, if server does not support subscriptionType, it will send out second query with subscriptionType commented out.

Subscription type is a future feature still in development. It is not in the GraphqQL Spec yet.

[aew]

I think I understand the issue - a query failing validation (because of this subscriptionType issue) is returning more than just the errors key, including things that don't serialize to JSON. I'll patch this.

[aew]

There were actually additional bugs hidden by a bug in the fragment cycles validator which was inappropriately short-circuiting the DFS. I've fixed these bugs and added a couple missing unit tests for things that were breaking the Graphiql starter project.

[tendant]

Just had simple test in GraphiQL, looks like it work fine now.

Thanks a lot for your great work. The validation really makes huge difference on usability of the library. It helps provide much better error messages.

[tendant]

Do you have more commits coming? If not, I might release a new version tonight.

[aew]

I'm not planning to commit any more unless we discover more bugs, so feel free to release a new version. Thanks!

[tendant]

Found a new issue in inspector:

https://github.com/tendant/graphql-clj-starter/tree/testing-validation-loc

Error result:

{
  "errors": [
    {
      "error": "Cannot query field 'kind' on type 'FullType'.",
      "loc": {
        "line": 21,
        "column": 1
      }
    }
  ]
}

[aew]

Ok - taking a look at this. Can you clarify which query you used to produce this error?

[tendant]

It came from introspection query.

Just open graphiql, the error will show up in graphql result.

[tendant]

Problem might relate to reloading of schema.

Looks like after updating schema in dev environment. Introspection query returns:

{
  "errors": [
    {
      "error": "Cannot query field 'kind' on type 'FullType'.",
      "loc": {
        "line": 21,
        "column": 1
      }
    }
  ]
}

When try to run any query after changing schema in dev server, I got below error for all root query fields:

{:errors [{:node-type :field, :parent {:node-type :operation-definition, :v/parent {:node-type :statement-root}, :v/path [Query]}, :node {:node-type :field, :field-name #object[graphql_clj.box.Box 0x231e19a3 {:status :ready, :val chapters}], :selection-set [{:node-type :field, :field-name #object[graphql_clj.box.Box 0x702e24d5 {:status :ready, :val id}], :v/path [Query chapters id], :spec :graphql-clj.1623193917/id}], :v/path [Query chapters], :spec :graphql-clj.1623193917/chapters, :v/parent {:node-type :operation-definition, :v/parent {:node-type :statement-root}, :v/path [Query]}, :v/parentk :selection-set}, :error Parent type not found}]}

[tendant]

BTW: I tried to use validator/validate-schema* function, it still has the same issue.

[tendant]

The problem goes away when all memoize function calls are removed.

Also, I would like to propose to expose execute function to be as simple as we can:

(defn execute
  [context schema-or-state resolver-fn statement-or-state variables]
    ...))

User can cache validated schema, validated statement and resolver-fn as they wish.

[tendant]

Another issue with the validated schema and statement: validation result is more than 10x bigger than parsing result.

Below are size comparison for schema "graphql-clj-starter.graphql/starter-schema"

6.2K  parsed-schema
70K   validated-schema

Below are size comparison for statement "{droid}"

242B parsed-statement
71K    validated-statement

Looks like the validated data contains the schema in addition to the result.

[aew]

Sounds good re: removing memoize and the single arity for the execute function - I'll make those changes.

Re: size of the validation output - I'm waiting to see how validation output interacts with a simplified execution phase to understand exactly what we need to keep, then we can remove everything else and it will be small.

[aew]

I've made the changes discussed above - @tendant worth taking another look.

For additional context, there are a lot of outstanding issues that concern me, but I don't know how many of them we want to fix in a single PR:

• Getting to "of type" is complex, happens in different ways in different places, and most likely needs to be solved a different way to maintain the same behavior with simpler code
• I'm not very happy with the dynamic use of spec - we may be better served using prismatic schema given spec's global state, eval, minimal leverage from the specs after they are created, etc.
• I would expect the validation phase to be slow (I haven't stopped to benchmark it). I don't think that optimizing this is a high priority at the moment, because ideally validation would behave like JIT compilation, whereas execution affects latency of serving each request. I think we want the output of the validation phase to inline everything necessary to execute a query (probably including the specific resolver function for each field, which would be a breaking change from today). I don't have a clear idea of what the statement validation output format should be until we re-implement the execution phase.
• With this change, current library users may start doing validation inside the inner loop, which could be an unexpected breaking change from a performance perspective (this is why I initially thought to do memoization inside the library to keep things working as expected). We may want to think about how we make it clear where people should be memoizing, or bite the bullet and make an actual breaking change once the execution phase is clear.

Given all of this, we could either merge this and hope for the best, or remove validation as a requirement for the execution phase, and perhaps add a different execution function for previously "prepared" inputs. WDYT?

[tendant]

I will test it and merge it, if there is no major issue.

As long as we keep the public API simple and consistent, it is not too hard to change the internal implementation if needed.

[tendant]

Merged. Thanks you @aew for your great contribution!