GraphQL: Improve documents validation before running execution flow

[dotansimha]

Continue from: #3005

@lutter : "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."

I guess we can do that by adding unit tests? :)

[lutter]

Continue from: #3005

@lutter : "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."

I guess we can do that by adding unit tests? :)

Yes, that should be possible. The big question is what these tests check for, but this should be testable by adding to graphql/tests/query.rs. On what to test: that's why I am wondering if there are some sort of GraphQL conformance tests; for example, I beleive the query query { musicians(where: {name: "Tom" }) { id } musicians(where: {name: "Tom" }) { id } } is legal and should respond with one musicians field, but if you change Tom to Roy in the second copy of musicians, the query should result in an error. I suspect there's lots of these cases to handle.

[dotansimha]

On what to test: that's why I am wondering if there are some sort of GraphQL conformance tests; for example, I beleive the query query { musicians(where: {name: "Tom" }) { id } musicians(where: {name: "Tom" }) { id } } is legal and should respond with one musicians field, but if you change Tom to Roy in the second copy of musicians, the query should result in an error. I suspect there's lots of these cases to handle.

I checked in graphql-js and it seems like execution doesn't really care about having multiple fields, and this is done in the validate phase.
In the JS ecosystem, there is a clear distinction between the request flow phases (parse, validate, execute) and what is the role of each one, and based on the spec, there are some checks that are happening only during validate and not checked during execute.

@n1ru4l and @saihaj wrote a great article about the request flow: https://github.com/saihaj/graphql-ts-node-advanced-tutorial/blob/main/docs/1-anatomy-of-a-graphql-request.md

I tested the following GraphQL schema:

type Query {
  a(b: String): String
}

And the following GraphQL query:

query test {
  a(b: "1")
  a(b: "2")
  a(b: "3")
}

• parse phase passes because the AST is valid
• validate phase failed on a validation rule OverlappingFieldsCanBeMergedRule (code)
• execute never called because of failure.

The error I got is:

GraphQLError: Fields "a" conflict because they have differing arguments. Use different aliases on the fields to fetch both if this was intentional.

When I used the same value for b argument, the validation phase passes, so it builds an "identity" for each selection set field and then checks if it's duplicated, so arguments and aliases affect the result of this validation phase.

When I'm skipping the validate phase and go directly to execute phase, I see that the query is being executed correctly, but it does not run the additional fields (with the query above, I get {data: { a: "1" }} and the resolvers of Query.a is being called only once - so the rest of the fields are just ignored.

@lutter I noticed that query.rs does only parse and then execute (ref) and it doesn't implement any validation phase as specified by the GraphQL spec.

I guess we can take the query after it's being parsed and implement the validation rules that GraphQL spec defines, and run it before getting into the whole execution phase - this way we can find these specific issues before running any GraphQL-related code/resolver.

The GraphQL spec / graphql-js implementation has ~30 validation rules that should cover most edge cases and prevent ambiguity during execution phase. Also it also has a spec for what does a validation rule means, and how developers can extend and write their own (we are doing similar things in envelop plugins). So we can start with the defaults and then see if graphql-node needs specific validation rules.

@lutter what do you think?