Fix validation stack overflow for fragment cycles inside nested fields (

#733)

* Fix validation stack overflow for fragment cycles inside nested fields

Previously fragment cycle detection only counted fragments that are
directly recursive, where the top-level selection for the fragment
included a fragment spread of itself. But spreading a fragment anywhere
inside a nested field is also problematic and prohibited by spec,
because even if the field you're spreading into is nullable, the client
can't know how deep that recursion goes, and it could be infinite.

Fixes #716

* label every fragment participating in a cycle

* use push() instead of repetitive insert(0,x)

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

@@ -298,17 +298,19 @@ pub(crate) fn validate_fragment_cycles(
     // synthetic trees.
     let named_fragments = db.ast_named_fragments(file_id);
 
+    /// If a fragment spread is recursive, returns a vec containing the spread that refers back to
+    /// the original fragment, and a trace of each fragment spread back to the original fragment.
     fn detect_fragment_cycles(
         named_fragments: &HashMap<ast::Name, Node<ast::FragmentDefinition>>,
         selection_set: &[ast::Selection],
         visited: &mut RecursionGuard<'_>,
-    ) -> Result<(), ast::Selection> {
+    ) -> Result<(), Vec<Node<ast::FragmentSpread>>> {
         for selection in selection_set {
             match selection {
                 ast::Selection::FragmentSpread(spread) => {
                     if visited.contains(&spread.fragment_name) {
                         if visited.first() == Some(&spread.fragment_name) {
-                            return Err(selection.clone());
+                            return Err(vec![spread.clone()]);
                         }
                         continue;
                     }
@@ -318,13 +320,19 @@ pub(crate) fn validate_fragment_cycles(
                             named_fragments,
                             &fragment.selection_set,
                             &mut visited.push(&fragment.name),
-                        )?;
+                        )
+                        .map_err(|mut trace| {
+                            trace.push(spread.clone());
+                            trace
+                        })?;
                     }
                 }
                 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)?;
+                }
             }
         }
 
@@ -338,17 +346,38 @@ pub(crate) fn validate_fragment_cycles(
     {
         let head_location = NodeLocation::recompose(def.location(), def.name.location());
 
-        diagnostics.push(
-            ApolloDiagnostic::new(
-                db,
-                def.location(),
-                DiagnosticData::RecursiveFragmentDefinition {
-                    name: def.name.to_string(),
-                },
-            )
-            .label(Label::new(head_location, "recursive fragment definition"))
-            .label(Label::new(cycle.location(), "refers to itself here")),
-        );
+        let mut diagnostic = ApolloDiagnostic::new(
+            db,
+            def.location(),
+            DiagnosticData::RecursiveFragmentDefinition {
+                name: def.name.to_string(),
+            },
+        )
+        .label(Label::new(head_location, "recursive fragment definition"));
+
+        if let Some((cyclical_spread, path)) = cycle.split_first() {
+            let mut prev_name = &def.name;
+            for spread in path.iter().rev() {
+                diagnostic = diagnostic.label(Label::new(
+                    spread.location(),
+                    format!(
+                        "`{}` references `{}` here...",
+                        prev_name, spread.fragment_name
+                    ),
+                ));
+                prev_name = &spread.fragment_name;
+            }
+
+            diagnostic = diagnostic.label(Label::new(
+                cyclical_spread.location(),
+                format!(
+                    "`{}` circularly references `{}` here",
+                    prev_name, cyclical_spread.fragment_name
+                ),
+            ));
+        }
+
+        diagnostics.push(diagnostic);
     }
 
     diagnostics

crates/apollo-compiler/test_data/diagnostics/0092_recursive_fragment_spread.graphql

@@ -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 }
+}

crates/apollo-compiler/test_data/diagnostics/0092_recursive_fragment_spread.txt

@@ -1,12 +1,29 @@
 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 }
-    │ ───────┬──────  
-    │        ╰──────── recursive fragment definition
-    │ 
- 15 │ fragment fragC on Human { name, ...fragA }
+ 14 │ fragment fragA on Human { name, ...fragB }
+    │ ───────┬──────                  ────┬───  
+    │        ╰────────────────────────────────── recursive fragment definition
+    │                                     │     
+    │                                     ╰───── `fragA` references `fragB` here...
+ 15 │ fragment fragB on Human { name, ...fragC }
+    │                                 ────┬───  
+    │                                     ╰───── `fragB` references `fragC` here...
+ 16 │ fragment fragC on Human { name, ...fragA }
     │                                 ────┬───  
-    │                                     ╰───── refers to itself here
+    │                                     ╰───── `fragC` circularly references `fragA` here
+────╯
+Error: `cycle1` fragment cannot reference itself
+    ╭─[0092_recursive_fragment_spread.graphql:19:1]
+    │
+ 19 │ fragment cycle1 on __Type { kind, ...cycle2 }
+    │ ───────┬───────                   ────┬────  
+    │        ╰───────────────────────────────────── recursive fragment definition
+    │                                       │      
+    │                                       ╰────── `cycle1` references `cycle2` here...
+    │ 
+ 23 │   ofType { ...cycle1 }
+    │            ────┬────  
+    │                ╰────── `cycle2` circularly references `cycle1` here
 ────╯
 

crates/apollo-compiler/test_data/serializer/diagnostics/0092_recursive_fragment_spread.graphql

@@ -10,6 +10,11 @@ query {
   human {
     ...fragA
   }
+  __schema {
+    types {
+      ...cycle1
+    }
+  }
 }
 
 fragment fragA on Human {
@@ -26,3 +31,15 @@ fragment fragC on Human {
   name
   ...fragA
 }
+
+fragment cycle1 on __Type {
+  kind
+  ...cycle2
+}
+
+fragment cycle2 on __Type {
+  kind
+  ofType {
+    ...cycle1
+  }
+}