graph, graphql: introduce GraphQL spec-compliant validation phase and rules

[dotansimha]

Background

While @lutter worked on a refactor for the GraphQL execution (#3005), we needed a way to make sure some aspects of execution are valid after merging the refactor PR.

We also noticed that the current implementation of the GraphQL engine does not include a validation phase (#3013).

This PR uses graphq-tools crate (https://github.com/dotansimha/graphql-tools-rs, a set of utils on top of graphql_parser crate) for validations, using visitor pattern (similar to GraphQL-JS ecosystem).

The following rules are now running:

• UniqueOperationNames (https://spec.graphql.org/draft/#sec-Operation-Name-Uniqueness)
• SingleFieldSubscriptions (https://spec.graphql.org/draft/#sec-Operation-Name-Uniqueness)
• KnownTypeNames (https://spec.graphql.org/draft/#sec-Fragment-Spread-Type-Existence)
• VariablesAreInputTypes (https://spec.graphql.org/draft/#sec-Variables-Are-Input-Types)
• FieldsOnCorrectType (https://spec.graphql.org/draft/#sec-Field-Selections)
• LoneAnonymousOperation (https://spec.graphql.org/draft/#sec-Lone-Anonymous-Operation)
• FragmentsOnCompositeTypes (https://spec.graphql.org/draft/#sec-Fragments-On-Composite-Types)
• OverlappingFieldsCanBeMerged (https://spec.graphql.org/draft/#sec-Field-Selection-Merging)
• NoUnusedFragments (https://spec.graphql.org/draft/#sec-Fragments-Must-Be-Used)
• KnownFragmentNamesRule (https://spec.graphql.org/draft/#sec-Fragment-spread-target-defined)
• LeafFieldSelections (https://spec.graphql.org/draft/#sec-Leaf-Field-Selections)
• UniqueFragmentNames (https://spec.graphql.org/draft/#sec-Fragment-Name-Uniqueness)
• NoFragmentsCycle (https://spec.graphql.org/draft/#sec-Fragment-spreads-must-not-form-cycles)
• PossibleFragmentSpreads (https://spec.graphql.org/draft/#sec-Fragment-spread-is-possible)
• NoUnusedVariables (https://spec.graphql.org/draft/#sec-All-Variables-Used)
• NoUndefinedVariables (https://spec.graphql.org/draft/#sec-All-Variable-Uses-Defined)
• KnownArgumentNames (https://spec.graphql.org/draft/#sec-Argument-Names , https://spec.graphql.org/draft/#sec-Directives-Are-In-Valid-Locations)
• UniqueArgumentNames (https://spec.graphql.org/draft/#sec-Argument-Names)
• UniqueVariableNames (https://spec.graphql.org/draft/#sec-Variable-Uniqueness)
• ProvidedRequiredArguments (https://spec.graphql.org/draft/#sec-Required-Arguments)

The following spec rules are not part of this PR:

• ExecutableDefinitions (not actually needed, because of graphql_parser nature)
• ValuesOfCorrectType (WIP - not sure yet about requirement by graph-node)
• VariablesInAllowedPosition (WIP - might be needed by graph-node)
• UniqueInputFieldNames (blocked by Avoid using BTreeMap in Value::Object and use Vec<(String, Value::Object) instead?  graphql-rust/graphql-parser#59)
• KnownDirectives - not needed at the moment by graph-node.
• UniqueDirectivesPerLocation - not needed at the moment by graph-node.

Changes in the PR

A ValidationPlan struct is now constructed in a static way, including all the rules, and can be disabled using the env var DISABLE_GRAPHQL_VALIDATIONS=true.

If there are any validation rules selected, the query fails with a standard error, based on the GraphQL spec:

Future tasks

• We can consider moving complexity checks and a few more validations into an actual ValidationRule and use it there, it should make the GraphQL layer a bit more generic.
• Drop validate_fields since it's covered by new validations.

TODO

• Implement validate flow based on the spec
• Support graphql_parser Document
• Implement initial implementation of OverlappingFieldsCanBeMerged
• Improve OverlappingFieldsCanBeMerged implementation
• Add more crucial validation rules

[lutter]

Nice progress! Two things:

1. With this approach, each validation requires a pass over the AST; probably not a big deal right now, but something to keep in mind as the number of validations grows.
2. It should be possible to turn all validations off with an environment variable - that's just so that when we roll this out in the hosted service, we can turn it off in case it breaks users' queries to give them some time to fix their queries. In the long run, that env variable will go away, but it's good to have that as we introduce new behavior.

Also, would it be possible to add a validation for this We actually have a bug right now where we run queries that violate this sentence:

If selectionType is an interface, union, or object

    The subselection set of that selection must NOT BE empty

[dotansimha]

Nice progress! Two things:

Thank you for the review @lutter !

1. With this approach, each validation requires a pass over the AST; probably not a big deal right now, but something to keep in mind as the number of validations grows.

I agree, that's why I also created this issue: #3077 . We can start with that and then add caching.

2. It should be possible to turn all validations off with an environment variable - that's just so that when we roll this out in the hosted service, we can turn it off in case it breaks users' queries to give them some time to fix their queries. In the long run, that env variable will go away, but it's good to have that as we introduce new behavior.

Sure, a flag like DISABLE_GRAPHQL_VALIDATIONS will do the trick here, I'll add that :)

Also, would it be possible to add a validation for this We actually have a bug right now where we run queries that violate this sentence:

If selectionType is an interface, union, or object

    The subselection set of that selection must NOT BE empty

Sure (issue: dotansimha/graphql-tools-rs#3), I'll work on that now.

[dotansimha]

Ok so now we have more rules :)

The LeafFieldSelections will now check the selection sets, based on the spec definition for that rule:

{
  "errors": [
    {
      "message": "Field \"purpose\" must not have a selection since type \"String\" has no subfields."
    }
  ]
}

{
  "errors": [
    {
      "message": "Field \"sender\" of type \"Sender!\" must have a selection of subfields. Did you mean \"sender { ... }\"?"
    }
  ]
}

Also, added an env var to skip the new validations. And moved it to the Query struct.

I think we are just waiting for dotansimha/graphql-tools-rs#4 and we can continue with this one.

I believe at some point, we can remove the complexity checks and validate_fields and use ValidationRule for both.

[dotansimha]

@lutter I updated to latest graphql-tools and now we have ~20 validation rules. The rules we have now cover everything that validate_fields was doing, so I think we can consider removing it?

[lutter]

This looks great now! I really like how explicit the error messages are.

One small change to move the validation plan out of the runner, and this is good to merge. That change will get rid of a bunch of small modifications to pass the plan around.

Also, before merging, can you squash this? The PR now has a lot of small commits that reflect more this discussion than the sequence of steps needed to get to this point which in the long term don't make that much sense.

As for removing validate_fields: yes, I think we can remove this now. I would like to first roll this PR out so that if we have to disable the validations, we still have whatever validate_fields does right now, and then follow it with a PR in a week or so that gets rid of the env var to turn validations off and removes validate_fields.

[dotansimha]

This looks great now! I really like how explicit the error messages are.

❤️

One small change to move the validation plan out of the runner, and this is good to merge. That change will get rid of a bunch of small modifications to pass the plan around.

Done! it's now lazy_static ✅

Also, before merging, can you squash this? The PR now has a lot of small commits that reflect more this discussion than the sequence of steps needed to get to this point which in the long term don't make that much sense.

Squashed ✅

As for removing validate_fields: yes, I think we can remove this now. I would like to first roll this PR out so that if we have to disable the validations, we still have whatever validate_fields does right now, and then follow it with a PR in a week or so that gets rid of the env var to turn validations off and removes validate_fields.

I added Future tasks to the description, once it's merged, we can open issues for those and track it there?

[lutter]

I just approved this, but then noticed that there are changes to tests that I don't understand why they are necessary. It would be good to remove those changes before merging.

Also, the commit message right now is just a log of squashed commits and is not very helpful. It should be changed to something like:

graph, graphql: Properly validate GraphQL queries before executing them

Uses `graphql-tools-rs` for the actual validation. Validation can be turned off by setting
`DISABLE_GRAPHQL_VALIDATIONS=true` in the environment

[dotansimha]

I just approved this, but then noticed that there are changes to tests that I don't understand why they are necessary. It would be good to remove those changes before merging.

Sure. Please see my comments above - I explained on each test what's the reason for changing it.

Also, the commit message right now is just a log of squashed commits and is not very helpful. It should be changed to something like:

graph, graphql: Properly validate GraphQL queries before executing them

Uses `graphql-tools-rs` for the actual validation. Validation can be turned off by setting
`DISABLE_GRAPHQL_VALIDATIONS=true` in the environment

✅

Also, I rebased and now it seems like I'm getting some build errors (I guess coming from master)