Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix validation stack overflow for fragment cycles inside nested fields #733

Merged
merged 3 commits into from
Nov 10, 2023
Merged

Fix validation stack overflow for fragment cycles inside nested fields #733

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f7fd841
Fix validation stack overflow for fragment cycles inside nested fields
goto-bus-stop Nov 9, 2023
eb0c4e3
label every fragment participating in a cycle
goto-bus-stop Nov 9, 2023
cf7b0f4
use push() instead of repetitive insert(0,x)
goto-bus-stop Nov 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion crates/apollo-compiler/src/validation/fragment.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -324,7 +324,9 @@ pub(crate) fn validate_fragment_cycles(
ast::Selection::InlineFragment(inline) => {
detect_fragment_cycles(named_fragments, &inline.selection_set, visited)?;
}
_ => {}
ast::Selection::Field(field) => {
detect_fragment_cycles(named_fragments, &field.selection_set, visited)?;
}
}
}

Expand Down
9 changes: 9 additions & 0 deletions crates/apollo-compiler/test_data/diagnostics/0092_recursive_fragment_spread.graphql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,16 @@ type Query {
# Query
{
human { ...fragA }
__schema { types { ...cycle1 } }
}
fragment fragA on Human { name, ...fragB }
fragment fragB on Human { name, ...fragC }
fragment fragC on Human { name, ...fragA }

# Indirect cycle
fragment cycle1 on __Type { kind, ...cycle2 }

fragment cycle2 on __Type {
kind
ofType { ...cycle1 }
}
17 changes: 14 additions & 3 deletions crates/apollo-compiler/test_data/diagnostics/0092_recursive_fragment_spread.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,23 @@
Error: `fragA` fragment cannot reference itself
╭─[0092_recursive_fragment_spread.graphql:13:1]
╭─[0092_recursive_fragment_spread.graphql:14:1]
│
13 │ fragment fragA on Human { name, ...fragB }
14 │ fragment fragA on Human { name, ...fragB }
│ ───────┬──────
│ ╰──────── recursive fragment definition
│
15 │ fragment fragC on Human { name, ...fragA }
16 │ fragment fragC on Human { name, ...fragA }
│ ────┬───
│ ╰───── refers to itself here
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "refers to itself" message should probably be tweaked

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the diagnostic even contain a Vec of locations to show the complete cycle?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The label is additional context for the error on the fragA definition, but I agree that it looks a bit odd. Maybe labeling every spread in the cycle and a tweaked message would be ideal?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is easy to do currently. would be helpful to have more context lines :/

Error: `cycle` fragment cannot reference itself
    ╭─[q.graphql:9:1]
    │
  9 │ fragment cycle on __Type {
    │ ───────┬──────
    │        ╰──────── recursive fragment definition
    │
 11 │   ...frag
    │   ───┬───
    │      ╰───── `cycle` references `frag` here...
    │
 17 │     ...cycle
    │     ────┬───
    │         ╰───── `frag` circularly references `cycle` here

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be ideal here if, in TTY formatting, cycle in the final label uses the same colour as fragment cycle in the primary label 🙈

────╯
Error: `cycle1` fragment cannot reference itself
╭─[0092_recursive_fragment_spread.graphql:19:1]
│
19 │ fragment cycle1 on __Type { kind, ...cycle2 }
│ ───────┬───────
│ ╰───────── recursive fragment definition
│
23 │ ofType { ...cycle1 }
│ ────┬────
│ ╰────── refers to itself here
────╯

17 changes: 17 additions & 0 deletions ...s/apollo-compiler/test_data/serializer/diagnostics/0092_recursive_fragment_spread.graphql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,11 @@ query {
human {
...fragA
}
__schema {
types {
...cycle1
}
}
}

fragment fragA on Human {
Expand All @@ -26,3 +31,15 @@ fragment fragC on Human {
name
...fragA
}

fragment cycle1 on __Type {
kind
...cycle2
}

fragment cycle2 on __Type {
kind
ofType {
...cycle1
}
}