feat(compiler): validate field merging using the Xing algorithm #816

goto-bus-stop · 2024-01-25T15:51:13Z

Current situation
This is an offending query:

{
  root { ... on A { unrelated } }
  root { ... on B { overlapping } }
  root { ... on C { overlapping } }
}

Here, assume B.overlapping selects an Int!, and C.overlapping selects an Int. Clearly root.overlapping can either be Int! or Int, 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. root is selected multiple times so we need to see if its subselections can merge. In main, we do this by comparing every subselection to the first subselection (... on A { unrelated }). Both the B and C subselections can be merged with A, 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

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.

crates/apollo-compiler/src/validation/selection.rs

…nt cycles

goto-bus-stop · 2024-01-31T15:33:09Z

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 a == b && a == c implies b == c. Previously that was also true, but the grouping worked differently, and it could miss out on combinations because of that.

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.

goto-bus-stop · 2024-02-06T14:07:54Z

crates/apollo-compiler/tests/validation/field_merging.rs

+ 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
+ ────╯


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

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?

SimonSapin · 2024-02-08T09:08:33Z

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

crates/apollo-compiler/src/validation/diagnostics.rs

crates/apollo-compiler/src/executable/mod.rs

SimonSapin · 2024-02-08T10:03:08Z

crates/apollo-compiler/tests/validation/field_merging.rs

+ 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
+ ────╯


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?

crates/apollo-compiler/src/validation/selection.rs

SimonSapin · 2024-02-08T15:47:13Z

crates/apollo-compiler/src/validation/selection.rs

+ /// 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>>> {


Should this return a Vec of (hash?)set instead of a Vec of Vec?

Co-authored-by: Simon Sapin <[email protected]>

goto-bus-stop · 2024-02-13T17:04:46Z

Should add a test for the recursion limit here tomorrow morning.

goto-bus-stop · 2024-02-14T09:59:44Z

crates/apollo-compiler/tests/validation/recursion.rs

@@ -105,6 +117,7 @@ fn long_fragment_chains_do_not_overflow_stack() {
 .expect_err("must have recursion errors");

 let expected = expect_test::expect![[r#"
+ Error: too much recursion


This test now hits a recursion limit in two places, once in fragment validation and once in field merging validation, and emitting two errors for it is correct enough in my opinion.

Recursion errors don't give you a lot of information but hopefully nobody actually hits them by accident 🤪

lrlna · 2024-02-14T10:24:41Z

Now that the recursion tests are in, shall we merge?

goto-bus-stop added 3 commits January 25, 2024 16:42

Fix field merging validation when more than 2 fields are in play

d9d9b25

clippy

344c7e3

goto-bus-stop self-assigned this Jan 25, 2024

goto-bus-stop requested review from lrlna and SimonSapin as code owners January 25, 2024 15:51

SimonSapin approved these changes Jan 25, 2024

View reviewed changes

lrlna approved these changes Jan 25, 2024

View reviewed changes

SimonSapin reviewed Jan 25, 2024

View reviewed changes

crates/apollo-compiler/src/validation/selection.rs Outdated Show resolved Hide resolved

goto-bus-stop added 2 commits January 26, 2024 16:11

Pull some field merging code into smaller functions

54d3dd0

Use high-level representation for field merging validation

68d09c4

goto-bus-stop changed the title ~~Fix field merging validation when multiple sub-selections partially overlap~~ fix(compiler): validate field merging when multiple sub-selections partially overlap Jan 29, 2024

goto-bus-stop added 8 commits January 30, 2024 14:00

Merge branch 'main' into fix-nested-fragment-validation

1f5a2e0

Bail out of field merging validation if selection set contains fragme…

ee6c8ff

…nt cycles

Make same response shape less mega quadratic recursive

a3b9bff

Make same field selection check less mega quadratic recursive

0895bc0

Port subscription validation that uses field selection expansion

b59b3f1

Migrate subscription validation errors to use BuildError

c1a558f

clippy

8db8671

Skip some unnecessary arc clones

91f10db

lrlna mentioned this pull request Jan 31, 2024

validate field merging when multiple sub-selections partially overlap #819

Closed

goto-bus-stop added 3 commits January 31, 2024 16:09

deref

763878d

The individual checks can now both be implemented linearly

ddcc806

Add many repeated fields validation benchmark

6b9b778

goto-bus-stop added 5 commits February 1, 2024 16:10

field(a:1, b:2) and field(b:2, a:1) should compare equal

7d9c58b

Remove now-invalid assertion

29d15db

This function can take one field from an object type, and another field from an interface type--then their parent types are not equal.

Port field merging tests from graphql-js

03c2eb7

doc comments

4828dcb

goto-bus-stop commented Feb 6, 2024

View reviewed changes

Ensure consistent ordering of errors by using indexmap

930d43e

goto-bus-stop changed the title ~~fix(compiler): validate field merging using the Xing algorithm~~ feat(compiler): validate field merging using the Xing algorithm Feb 7, 2024

Push field merging errors directly into DiagnosticList

2214452

Remove unnecessary fragment recursion check

90357cb

SimonSapin reviewed Feb 8, 2024

View reviewed changes

SimonSapin mentioned this pull request Feb 8, 2024

[Apollo-Compiler][Feature Request] Expose validation data access #824

Open

goto-bus-stop and others added 10 commits February 9, 2024 16:05

Use a map to optimize comparing large argument lists

9d35fc8

Benchmark checking a field with very many arguments

850d9b5

Merge branch 'main' into fix-nested-fragment-validation

7df98fb

update diagnostic message: multiple -> different

689e851

remove outdated comment

4d8ee98

useless zip

9be0f6e

Use Entry::or_default

83a4428

Co-authored-by: Simon Sapin <[email protected]>

make clippy happy with the benchmarking code

453de79

Add a recursion limit while merging fields

f5f0380

check -> already_done

a56fcca

goto-bus-stop added 3 commits February 14, 2024 09:32

Move ArgumentLookup out of the function

53db962

Emit an error when the new recursion limit is reached; add test

acdd464

ref archived article

28f8fbf

goto-bus-stop commented Feb 14, 2024

View reviewed changes

Merge branch 'main' into fix-nested-fragment-validation

24949da

goto-bus-stop enabled auto-merge (squash) February 14, 2024 11:02

goto-bus-stop merged commit af4b599 into main Feb 14, 2024
12 checks passed

goto-bus-stop deleted the fix-nested-fragment-validation branch February 14, 2024 11:06

goto-bus-stop mentioned this pull request Feb 14, 2024

[email protected] #837

Merged

goto-bus-stop mentioned this pull request Jul 9, 2024

fix(compiler): remove potential hash collision bug from FieldsInSetCanMerge #878

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(compiler): validate field merging using the Xing algorithm #816

feat(compiler): validate field merging using the Xing algorithm #816

goto-bus-stop commented Jan 25, 2024 •

edited by lrlna

Loading

goto-bus-stop commented Jan 31, 2024

goto-bus-stop Feb 6, 2024 •

edited

Loading

SimonSapin Feb 8, 2024

SimonSapin commented Feb 8, 2024

SimonSapin Feb 8, 2024

SimonSapin Feb 8, 2024

goto-bus-stop commented Feb 13, 2024 •

edited

Loading

goto-bus-stop Feb 14, 2024 •

edited

Loading

lrlna commented Feb 14, 2024

feat(compiler): validate field merging using the Xing algorithm #816

feat(compiler): validate field merging using the Xing algorithm #816

Conversation

goto-bus-stop commented Jan 25, 2024 • edited by lrlna Loading

goto-bus-stop commented Jan 31, 2024

goto-bus-stop Feb 6, 2024 • edited Loading

Choose a reason for hiding this comment

SimonSapin Feb 8, 2024

Choose a reason for hiding this comment

SimonSapin commented Feb 8, 2024

SimonSapin Feb 8, 2024

Choose a reason for hiding this comment

SimonSapin Feb 8, 2024

Choose a reason for hiding this comment

goto-bus-stop commented Feb 13, 2024 • edited Loading

goto-bus-stop Feb 14, 2024 • edited Loading

Choose a reason for hiding this comment

lrlna commented Feb 14, 2024

goto-bus-stop commented Jan 25, 2024 •

edited by lrlna

Loading

goto-bus-stop Feb 6, 2024 •

edited

Loading

goto-bus-stop commented Feb 13, 2024 •

edited

Loading

goto-bus-stop Feb 14, 2024 •

edited

Loading