feat(compiler): validate field merging using the Xing algorithm #816
Conversation
This should warn about the `root.overlapping` selection being both `Int` and `Int!` at the same time, but it does not because the validation only compares each selection to the *first* selection. Neither `B` nor `C` conflict with the selection on `A`, so the current implementation considers this valid.
goto-bus-stop
changed the title
|
I think i now have a faithful implementation of https://tech.new-work.se/graphql-overlapping-fields-can-be-merged-fast-ea6e92e0a01, except the caching. I think that it's valid for the output type check and the same field name/arguments check to be ran linearly, just like we did before this change, because of the grouping work that's done beforehand and because the recursions are now independent. Both checks assert that the elements they compare are the same, and Without caching, we end up grouping most selection sets in exactly the same way twice. It should be simple to add but i want to add more tests to make sure I did it right. |
This function can take one field from an object type, and another field from an interface type--then their parent types are not equal.
Field merging validation was applied to operations and fragment definitions. Selection sets of fragment definitions must always be checked in the context of an operation or other fragment, and in the end it always leads to an operation. So we strictly only need to validate operations to find all issues in all reachable fragments. This can still output duplicate errors if a fragment contains a conflict internally, and is used in multiple operations.
| Error: cannot select multiple fields into the same alias `x` | ||
| ╭─[query.graphql:17:3] | ||
| │ | ||
| 17 │ x: a | ||
| │ ──┬─ | ||
| │ ╰─── `x` is selected from `Type.a` here | ||
| │ | ||
| 20 │ x: b | ||
| │ ──┬─ | ||
| │ ╰─── `x` is selected from `Type.b` here | ||
| │ | ||
| │ Help: Both fields may be present on the schema type, so it's not clear which one should be used to fill the response | ||
| ────╯ | ||
| Error: cannot select multiple fields into the same alias `x` | ||
| ╭─[query.graphql:17:3] | ||
| │ | ||
| 13 │ x: c | ||
| │ ──┬─ | ||
| │ ╰─── `x` is selected from `Type.c` here | ||
| │ | ||
| 17 │ x: a | ||
| │ ──┬─ | ||
| │ ╰─── `x` is selected from `Type.a` here | ||
| │ | ||
| │ Help: Both fields may be present on the schema type, so it's not clear which one should be used to fill the response | ||
| ────╯ | ||
| Error: cannot select multiple fields into the same alias `x` | ||
| ╭─[query.graphql:20:3] | ||
| │ | ||
| 17 │ x: a | ||
| │ ──┬─ | ||
| │ ╰─── `x` is selected from `Type.a` here | ||
| │ | ||
| 20 │ x: b | ||
| │ ──┬─ | ||
| │ ╰─── `x` is selected from `Type.b` here | ||
| │ | ||
| │ Help: Both fields may be present on the schema type, so it's not clear which one should be used to fill the response | ||
| ────╯ | ||
| Error: cannot select multiple fields into the same alias `x` | ||
| ╭─[query.graphql:20:3] | ||
| │ | ||
| 13 │ x: c | ||
| │ ──┬─ | ||
| │ ╰─── `x` is selected from `Type.c` here | ||
| │ | ||
| 20 │ x: b | ||
| │ ──┬─ | ||
| │ ╰─── `x` is selected from `Type.b` here | ||
| │ | ||
| │ Help: Both fields may be present on the schema type, so it's not clear which one should be used to fill the response | ||
| ────╯ |
There was a problem hiding this comment.
In graphql-js this test is specifically meant to check that we don't produce too many warnings, and I think this may be one too many (but it may not be). It would be more obvious to see if this is correct as is, if it included a SelectionPath
There was a problem hiding this comment.
One too many isn’t too bad, if we emit a warning for every pair in a set it could get annoying for larger sets. Could a single diagnostic contain a Vec of colliding selections instead of just two?
goto-bus-stop
changed the title
|
I made sure the XING blog post has a copy at https://web.archive.org/web/20240208084612/https://tech.new-work.se/graphql-overlapping-fields-can-be-merged-fast-ea6e92e0a01?gi=3e85e33327df |
| Error: cannot select multiple fields into the same alias `x` | ||
| ╭─[query.graphql:17:3] | ||
| │ | ||
| 17 │ x: a | ||
| │ ──┬─ | ||
| │ ╰─── `x` is selected from `Type.a` here | ||
| │ | ||
| 20 │ x: b | ||
| │ ──┬─ | ||
| │ ╰─── `x` is selected from `Type.b` here | ||
| │ | ||
| │ Help: Both fields may be present on the schema type, so it's not clear which one should be used to fill the response | ||
| ────╯ | ||
| Error: cannot select multiple fields into the same alias `x` | ||
| ╭─[query.graphql:17:3] | ||
| │ | ||
| 13 │ x: c | ||
| │ ──┬─ | ||
| │ ╰─── `x` is selected from `Type.c` here | ||
| │ | ||
| 17 │ x: a | ||
| │ ──┬─ | ||
| │ ╰─── `x` is selected from `Type.a` here | ||
| │ | ||
| │ Help: Both fields may be present on the schema type, so it's not clear which one should be used to fill the response | ||
| ────╯ | ||
| Error: cannot select multiple fields into the same alias `x` | ||
| ╭─[query.graphql:20:3] | ||
| │ | ||
| 17 │ x: a | ||
| │ ──┬─ | ||
| │ ╰─── `x` is selected from `Type.a` here | ||
| │ | ||
| 20 │ x: b | ||
| │ ──┬─ | ||
| │ ╰─── `x` is selected from `Type.b` here | ||
| │ | ||
| │ Help: Both fields may be present on the schema type, so it's not clear which one should be used to fill the response | ||
| ────╯ | ||
| Error: cannot select multiple fields into the same alias `x` | ||
| ╭─[query.graphql:20:3] | ||
| │ | ||
| 13 │ x: c | ||
| │ ──┬─ | ||
| │ ╰─── `x` is selected from `Type.c` here | ||
| │ | ||
| 20 │ x: b | ||
| │ ──┬─ | ||
| │ ╰─── `x` is selected from `Type.b` here | ||
| │ | ||
| │ Help: Both fields may be present on the schema type, so it's not clear which one should be used to fill the response | ||
| ────╯ |
There was a problem hiding this comment.
One too many isn’t too bad, if we emit a warning for every pair in a set it could get annoying for larger sets. Could a single diagnostic contain a Vec of colliding selections instead of just two?
| /// Returns potentially overlapping groups of fields. Fields overlap if they are selected from | ||
| /// the same concrete type or if they are selected from an abstract type (future schema changes | ||
| /// can make any abstract type overlap with any other type). | ||
| fn group_by_common_parents(&self, schema: &schema::Schema) -> &Vec<Vec<FieldSelection<'doc>>> { |
There was a problem hiding this comment.
Should this return a Vec of (hash?)set instead of a Vec of Vec?
Co-authored-by: Simon Sapin <[email protected]>
|
Should add a test for the recursion limit here tomorrow morning. |
| @@ -105,6 +117,7 @@ fn long_fragment_chains_do_not_overflow_stack() { | |||
| .expect_err("must have recursion errors"); | |||
|
|
|||
| let expected = expect_test::expect
|
Now that the recursion tests are in, shall we merge? |
Current situation
This is an offending query:
Here, assume
B.overlappingselects anInt!, andC.overlappingselects anInt. Clearlyroot.overlappingcan either beInt!orInt, and so this must be an error. But previous versions of apollo-compiler do not report anything.Cause
The validation is implemented by drilling in from the top down. The first field it encounters is
root.rootis selected multiple times so we need to see if its subselections can merge. Inmain, we do this by comparing every subselection to the first subselection (... on A { unrelated }). Both theBandCsubselections can be merged withA, as they don't select any conflicting fields.Fix
Comparing all selections of the same field, to the first occurrence of the same field is not enough, as other occurrences can conflict with each other but not with the first field. This PR uses a different algorithm that efficiently groups and
compares all the relevant combinations of fields. https://tech.new-work.se/graphql-overlapping-fields-can-be-merged-fast-ea6e92e0a01
closes #819