Avoid reprocessing named fragments (#952)

* Skip already-visited named fragments for finding unused variables

* Skip already-visited named fragment for cycle detection

* Only validate fragment definitions once per operation

* Skip already-visited named fragments in introspection::check_max_depth

* Deduplicate named fragments in `Operation::root_fields` and `named_fields`

* Remember the depth of already-checked fragments

---------

Co-authored-by: Simon Sapin <[email protected]>
Co-authored-by: Renée Kooi <[email protected]>

Author: 3 people

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

@@ -79,6 +79,7 @@ pub use crate::ast::OperationType;
 pub use crate::ast::Type;
 pub use crate::ast::Value;
 pub use crate::ast::VariableDefinition;
+use crate::collections::HashSet;
 use crate::request::RequestError;
 pub use crate::Name;
 
@@ -540,17 +541,18 @@ impl Operation {
     ///
     /// `document` is used to look up fragment definitions.
     ///
-    /// This does **not** perform [field merging] nor fragment spreads de-duplication,
-    /// so multiple items in this iterator may have the same response key,
-    /// point to the same field definition,
-    /// or even be the same field selection (reached through different fragments).
+    /// This does **not** perform [field merging],
+    /// so multiple items in this iterator may have the same response key
+    /// or point to the same field definition.
+    /// Named fragments however are only traversed once even if spread multiple times.
     ///
     /// [field merging]: https://spec.graphql.org/draft/#sec-Field-Selection-Merging
     pub fn root_fields<'doc>(
         &'doc self,
         document: &'doc ExecutableDocument,
     ) -> impl Iterator<Item = &'doc Node<Field>> {
         let mut stack = vec![self.selection_set.selections.iter()];
+        let mut fragments_seen = HashSet::default();
         std::iter::from_fn(move || {
             while let Some(selection_set_iter) = stack.last_mut() {
                 match selection_set_iter.next() {
@@ -564,7 +566,10 @@ impl Operation {
                     }
                     Some(Selection::FragmentSpread(spread)) => {
                         if let Some(def) = document.fragments.get(&spread.fragment_name) {
-                            stack.push(def.selection_set.selections.iter())
+                            let new = fragments_seen.insert(&spread.fragment_name);
+                            if new {
+                                stack.push(def.selection_set.selections.iter())
+                            }
                         } else {
                             // Undefined fragments are silently ignored.
                             // They should never happen in a valid document.
@@ -587,16 +592,18 @@ impl Operation {
     ///
     /// `document` is used to look up fragment definitions.
     ///
-    /// This does **not** perform [field merging] nor fragment spreads de-duplication,
-    /// so multiple items in this iterator may have the same response key,
-    /// point to the same field definition, or even be the same field selection.
+    /// This does **not** perform [field merging],
+    /// so multiple items in this iterator may have the same response key
+    /// or point to the same field definition.
+    /// Named fragments however are only traversed once even if spread multiple times.
     ///
     /// [field merging]: https://spec.graphql.org/draft/#sec-Field-Selection-Merging
     pub fn all_fields<'doc>(
         &'doc self,
         document: &'doc ExecutableDocument,
     ) -> impl Iterator<Item = &'doc Node<Field>> {
         let mut stack = vec![self.selection_set.selections.iter()];
+        let mut fragments_seen = HashSet::default();
         std::iter::from_fn(move || {
             while let Some(selection_set_iter) = stack.last_mut() {
                 match selection_set_iter.next() {
@@ -613,7 +620,10 @@ impl Operation {
                     }
                     Some(Selection::FragmentSpread(spread)) => {
                         if let Some(def) = document.fragments.get(&spread.fragment_name) {
-                            stack.push(def.selection_set.selections.iter())
+                            let new = fragments_seen.insert(&spread.fragment_name);
+                            if new {
+                                stack.push(def.selection_set.selections.iter())
+                            }
                         } else {
                             // Undefined fragments are silently ignored.
                             // They should never happen in a valid document.

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

@@ -61,6 +61,6 @@ pub(crate) fn validate_field_set(
         document,
         Some((schema, &field_set.selection_set.ty)),
         &field_set.selection_set,
-        context.operation_context(&[]),
+        &mut context.operation_context(&[]),
     )
 }

crates/apollo-compiler/src/introspection/max_depth.rs

@@ -1,25 +1,57 @@
+use crate::collections::HashMap;
 use crate::executable::Selection;
 use crate::executable::SelectionSet;
 use crate::request::RequestError;
 use crate::validation::Valid;
 use crate::ExecutableDocument;
+use crate::Name;
 
 const MAX_LISTS_DEPTH: u32 = 3;
 
-pub(super) fn check_selection_set(
-    document: &Valid<ExecutableDocument>,
+pub(super) fn check_selection_set<'doc>(
+    document: &'doc Valid<ExecutableDocument>,
+    fragment_depths: &mut HashMap<&'doc Name, u32>,
     depth_so_far: u32,
-    selection_set: &SelectionSet,
-) -> Result<(), RequestError> {
+    selection_set: &'doc SelectionSet,
+) -> Result<u32, RequestError> {
+    let mut max_depth = depth_so_far;
     for selection in &selection_set.selections {
         match selection {
             Selection::InlineFragment(inline) => {
-                check_selection_set(document, depth_so_far, &inline.selection_set)?
+                max_depth = max_depth.max(check_selection_set(
+                    document,
+                    fragment_depths,
+                    depth_so_far,
+                    &inline.selection_set,
+                )?)
             }
             Selection::FragmentSpread(spread) => {
-                // Validation ensures that `Valid<ExecutableDocument>` does not contain fragment cycles
-                if let Some(def) = document.fragments.get(&spread.fragment_name) {
-                    check_selection_set(document, depth_so_far, &def.selection_set)?
+                let Some(def) = document.fragments.get(&spread.fragment_name) else {
+                    continue;
+                };
+                // Avoiding the entry API because we may have to modify the map in-between this `.get()`
+                // and the `.insert()`.
+                if let Some(fragment_depth) = fragment_depths.get(&spread.fragment_name) {
+                    if depth_so_far + *fragment_depth > MAX_LISTS_DEPTH {
+                        return Err(RequestError {
+                            message: "Maximum introspection depth exceeded".into(),
+                            location: spread.location(),
+                            is_suspected_validation_bug: false,
+                        });
+                    }
+                } else {
+                    // Recursing without marking our fragment spread as used is fine,
+                    // because validation guarantees that we do not have a self-referential
+                    // fragment chain.
+                    let post_fragment_depth = check_selection_set(
+                        document,
+                        fragment_depths,
+                        depth_so_far,
+                        &def.selection_set,
+                    )?;
+                    fragment_depths
+                        .insert(&spread.fragment_name, post_fragment_depth - depth_so_far);
+                    max_depth = max_depth.max(post_fragment_depth);
                 }
             }
             Selection::Field(field) => {
@@ -37,9 +69,14 @@ pub(super) fn check_selection_set(
                         });
                     }
                 }
-                check_selection_set(document, depth, &field.selection_set)?
+                max_depth = max_depth.max(check_selection_set(
+                    document,
+                    fragment_depths,
+                    depth,
+                    &field.selection_set,
+                )?)
             }
         }
     }
-    Ok(())
+    Ok(max_depth)
 }

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

@@ -42,7 +42,13 @@ pub fn check_max_depth(
     operation: &Operation,
 ) -> Result<(), RequestError> {
     let initial_depth = 0;
-    max_depth::check_selection_set(document, initial_depth, &operation.selection_set)
+    max_depth::check_selection_set(
+        document,
+        &mut HashMap::default(),
+        initial_depth,
+        &operation.selection_set,
+    )
+    .map(drop)
 }
 
 /// Excecutes the [schema introspection](https://spec.graphql.org/draft/#sec-Schema-Introspection)

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

@@ -19,7 +19,7 @@ pub(crate) fn validate_field(
     // May be None if a parent selection was invalid
     against_type: Option<(&crate::Schema, &ast::NamedType)>,
     field: &Node<executable::Field>,
-    context: OperationValidationContext<'_>,
+    context: &mut OperationValidationContext<'_>,
 ) {
     // First do all the validation that we can without knowing the type of the field.
 

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

@@ -1,6 +1,7 @@
 use crate::ast;
 use crate::ast::NamedType;
 use crate::collections::HashMap;
+use crate::collections::HashSet;
 use crate::collections::IndexSet;
 use crate::executable;
 use crate::schema;
@@ -55,7 +56,7 @@ fn validate_fragment_spread_type(
     against_type: &NamedType,
     type_condition: &NamedType,
     selection: &executable::Selection,
-    context: OperationValidationContext<'_>,
+    context: &mut OperationValidationContext<'_>,
 ) {
     // Treat a spread that's just literally on the parent type as always valid:
     // by spec text, it shouldn't be, but graphql-{js,java,go} and others all do this.
@@ -118,7 +119,7 @@ pub(crate) fn validate_inline_fragment(
     document: &ExecutableDocument,
     against_type: Option<(&crate::Schema, &ast::NamedType)>,
     inline: &Node<executable::InlineFragment>,
-    context: OperationValidationContext<'_>,
+    context: &mut OperationValidationContext<'_>,
 ) {
     super::directive::validate_directives(
         diagnostics,
@@ -171,7 +172,7 @@ pub(crate) fn validate_fragment_spread(
     document: &ExecutableDocument,
     against_type: Option<(&crate::Schema, &NamedType)>,
     spread: &Node<executable::FragmentSpread>,
-    context: OperationValidationContext<'_>,
+    context: &mut OperationValidationContext<'_>,
 ) {
     super::directive::validate_directives(
         diagnostics,
@@ -194,7 +195,12 @@ pub(crate) fn validate_fragment_spread(
                     context,
                 );
             }
-            validate_fragment_definition(diagnostics, document, def, context);
+            let new = context
+                .validated_fragments
+                .insert(spread.fragment_name.clone());
+            if new {
+                validate_fragment_definition(diagnostics, document, def, context);
+            }
         }
         None => {
             diagnostics.push(
@@ -211,7 +217,7 @@ pub(crate) fn validate_fragment_definition(
     diagnostics: &mut DiagnosticList,
     document: &ExecutableDocument,
     fragment: &Node<executable::Fragment>,
-    context: OperationValidationContext<'_>,
+    context: &mut OperationValidationContext<'_>,
 ) {
     super::directive::validate_directives(
         diagnostics,
@@ -265,35 +271,43 @@ pub(crate) fn validate_fragment_cycles(
 ) {
     /// 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(
-        document: &ExecutableDocument,
-        selection_set: &executable::SelectionSet,
-        visited: &mut RecursionGuard<'_>,
+    fn detect_fragment_cycles<'doc>(
+        document: &'doc ExecutableDocument,
+        selection_set: &'doc executable::SelectionSet,
+        path_from_root: &mut RecursionGuard<'_>,
+        seen: &mut HashSet<&'doc Name>,
     ) -> Result<(), CycleError<executable::FragmentSpread>> {
         for selection in &selection_set.selections {
             match selection {
                 executable::Selection::FragmentSpread(spread) => {
-                    if visited.contains(&spread.fragment_name) {
-                        if visited.first() == Some(&spread.fragment_name) {
+                    if path_from_root.contains(&spread.fragment_name) {
+                        if path_from_root.first() == Some(&spread.fragment_name) {
                             return Err(CycleError::Recursed(vec![spread.clone()]));
                         }
                         continue;
                     }
 
+                    let new = seen.insert(&spread.fragment_name);
+                    if !new {
+                        // We already recursively traversed that fragment and didn’t find a cycle then
+                        continue;
+                    }
+
                     if let Some(fragment) = document.fragments.get(&spread.fragment_name) {
                         detect_fragment_cycles(
                             document,
                             &fragment.selection_set,
-                            &mut visited.push(&fragment.name)?,
+                            &mut path_from_root.push(&fragment.name)?,
+                            seen,
                         )
                         .map_err(|error| error.trace(spread))?;
                     }
                 }
                 executable::Selection::InlineFragment(inline) => {
-                    detect_fragment_cycles(document, &inline.selection_set, visited)?;
+                    detect_fragment_cycles(document, &inline.selection_set, path_from_root, seen)?;
                 }
                 executable::Selection::Field(field) => {
-                    detect_fragment_cycles(document, &field.selection_set, visited)?;
+                    detect_fragment_cycles(document, &field.selection_set, path_from_root, seen)?;
                 }
             }
         }
@@ -303,7 +317,12 @@ pub(crate) fn validate_fragment_cycles(
 
     let mut visited = RecursionStack::with_root(def.name.clone()).with_limit(100);
 
-    match detect_fragment_cycles(document, &def.selection_set, &mut visited.guard()) {
+    match detect_fragment_cycles(
+        document,
+        &def.selection_set,
+        &mut visited.guard(),
+        &mut HashSet::default(),
+    ) {
         Ok(_) => {}
         Err(CycleError::Recursed(trace)) => {
             let head_location = SourceSpan::recompose(def.location(), def.name.location());

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

@@ -24,6 +24,7 @@ pub(crate) mod value;
 pub(crate) mod variable;
 
 use crate::collections::HashMap;
+use crate::collections::HashSet;
 use crate::collections::IndexSet;
 use crate::diagnostic::CliReport;
 use crate::diagnostic::Diagnostic;
@@ -160,18 +161,20 @@ impl<'a> ExecutableValidationContext<'a> {
         OperationValidationContext {
             executable: self,
             variables,
+            validated_fragments: HashSet::default(),
         }
     }
 }
 
 /// Shared context when validating things inside an operation.
-#[derive(Debug, Clone, Copy)]
+#[derive(Debug)]
 pub(crate) struct OperationValidationContext<'a> {
     /// Parent context. Using a reference so the `OnceLock` is shared between all operation
     /// contexts.
     executable: &'a ExecutableValidationContext<'a>,
     /// The variables defined for this operation.
-    pub variables: &'a [Node<VariableDefinition>],
+    pub(crate) variables: &'a [Node<VariableDefinition>],
+    pub(crate) validated_fragments: HashSet<Name>,
 }
 
 impl<'a> OperationValidationContext<'a> {

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

@@ -82,7 +82,7 @@ pub(crate) fn validate_operation(
         document,
         against_type,
         &operation.selection_set,
-        context.operation_context(&operation.variables),
+        &mut context.operation_context(&operation.variables),
     );
 }
 

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

@@ -581,7 +581,7 @@ pub(crate) fn validate_selection_set(
     document: &ExecutableDocument,
     against_type: Option<(&crate::Schema, &NamedType)>,
     selection_set: &SelectionSet,
-    context: OperationValidationContext<'_>,
+    context: &mut OperationValidationContext<'_>,
 ) {
     for selection in &selection_set.selections {
         match selection {