diff --git a/crates/apollo-compiler/CHANGELOG.md b/crates/apollo-compiler/CHANGELOG.md index 15e03e5ae..b1f325824 100644 --- a/crates/apollo-compiler/CHANGELOG.md +++ b/crates/apollo-compiler/CHANGELOG.md @@ -130,6 +130,19 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm [pull/724]: https://github.com/apollographql/apollo-rs/pull/724 [pull/752]: https://github.com/apollographql/apollo-rs/pull/752 +## Fixes + +- **Limit recursion in validation - [goto-bus-stop], [pull/748] fixing [issue/742]** + Validation now bails out of very long chains of definitions that refer to each other, + even if they don't strictly form a cycle. These could previously cause extremely long validation + times or stack overflows. + + The limit for input objects and directives is set at 32. For fragments, the limit is set at 100. + Based on our datasets, real-world documents don't come anywhere close to this. + +[goto-bus-stop]: https://github.com/goto-bus-stop +[issue/742]: https://github.com/apollographql/apollo-rs/issues/742 +[pull/748]: https://github.com/apollographql/apollo-rs/pull/748 # [1.0.0-beta.7](https://crates.io/crates/apollo-compiler/1.0.0-beta.7) - 2023-11-17 diff --git a/crates/apollo-compiler/src/diagnostics.rs b/crates/apollo-compiler/src/diagnostics.rs index 67ff0c350..2023606ee 100644 --- a/crates/apollo-compiler/src/diagnostics.rs +++ b/crates/apollo-compiler/src/diagnostics.rs @@ -129,6 +129,8 @@ pub(crate) enum DiagnosticData { RecursiveInputObjectDefinition { name: String }, #[error("`{name}` fragment cannot reference itself")] RecursiveFragmentDefinition { name: String }, + #[error("`{name}` contains too much nesting")] + DeeplyNestedType { name: String }, #[error("type does not satisfy interface `{interface}`: missing field `{field}`")] MissingInterfaceField { interface: String, field: String }, #[error("the required argument `{name}` is not provided")] @@ -238,6 +240,8 @@ pub(crate) enum DiagnosticData { /// Name of the argument where variable is used arg_name: String, }, + #[error("too much recursion")] + RecursionError {}, } impl ApolloDiagnostic { diff --git a/crates/apollo-compiler/src/validation/directive.rs b/crates/apollo-compiler/src/validation/directive.rs index f7a617f33..3386b5455 100644 --- a/crates/apollo-compiler/src/validation/directive.rs +++ b/crates/apollo-compiler/src/validation/directive.rs @@ -3,6 +3,8 @@ use crate::validation::{NodeLocation, RecursionGuard, RecursionStack}; use crate::{ast, schema, Node, ValidationDatabase}; use std::collections::{HashMap, HashSet}; +use super::CycleError; + /// This struct just groups functions that are used to find self-referential directives. /// The way to use it is to call `FindRecursiveDirective::check`. struct FindRecursiveDirective<'s> { @@ -14,7 +16,7 @@ impl FindRecursiveDirective<'_> { &self, seen: &mut RecursionGuard<'_>, def: &schema::ExtendedType, - ) -> Result<(), Node> { + ) -> Result<(), CycleError> { match def { schema::ExtendedType::Scalar(scalar_type_definition) => { self.directives(seen, &scalar_type_definition.directives)?; @@ -49,7 +51,7 @@ impl FindRecursiveDirective<'_> { &self, seen: &mut RecursionGuard<'_>, input_value: &Node, - ) -> Result<(), Node> { + ) -> Result<(), CycleError> { for directive in &input_value.directives { self.directive(seen, directive)?; } @@ -66,7 +68,7 @@ impl FindRecursiveDirective<'_> { &self, seen: &mut RecursionGuard<'_>, enum_value: &Node, - ) -> Result<(), Node> { + ) -> Result<(), CycleError> { for directive in &enum_value.directives { self.directive(seen, directive)?; } @@ -78,7 +80,7 @@ impl FindRecursiveDirective<'_> { &self, seen: &mut RecursionGuard<'_>, directives: &[schema::Component], - ) -> Result<(), Node> { + ) -> Result<(), CycleError> { for directive in directives { self.directive(seen, directive)?; } @@ -89,17 +91,18 @@ impl FindRecursiveDirective<'_> { &self, seen: &mut RecursionGuard<'_>, directive: &Node, - ) -> Result<(), Node> { + ) -> Result<(), CycleError> { if !seen.contains(&directive.name) { if let Some(def) = self.schema.directive_definitions.get(&directive.name) { - self.directive_definition(seen.push(&directive.name), def)?; + self.directive_definition(seen.push(&directive.name)?, def) + .map_err(|error| error.trace(directive))?; } } else if seen.first() == Some(&directive.name) { // Only report an error & bail out early if this is the *initial* directive. // This prevents raising confusing errors when a directive `@b` which is not // self-referential uses a directive `@a` that *is*. The error with `@a` should // only be reported on its definition, not on `@b`'s. - return Err(directive.clone()); + return Err(CycleError::Recursed(vec![directive.clone()])); } Ok(()) @@ -109,7 +112,7 @@ impl FindRecursiveDirective<'_> { &self, mut seen: RecursionGuard<'_>, def: &Node, - ) -> Result<(), Node> { + ) -> Result<(), CycleError> { for input_value in &def.arguments { self.input_value(&mut seen, input_value)?; } @@ -120,7 +123,7 @@ impl FindRecursiveDirective<'_> { fn check( schema: &schema::Schema, directive_def: &Node, - ) -> Result<(), Node> { + ) -> Result<(), CycleError> { let mut recursion_stack = RecursionStack::with_root(directive_def.name.clone()); FindRecursiveDirective { schema } .directive_definition(recursion_stack.guard(), directive_def) @@ -143,22 +146,59 @@ pub(crate) fn validate_directive_definition( // references itself directly. // // Returns Recursive Definition error. - if let Err(directive) = FindRecursiveDirective::check(&db.schema(), &def) { - let definition_location = def.location(); - let head_location = NodeLocation::recompose(def.location(), def.name.location()); - let directive_location = directive.location(); - - diagnostics.push( - ApolloDiagnostic::new( + match FindRecursiveDirective::check(&db.schema(), &def) { + Ok(_) => {} + Err(CycleError::Recursed(trace)) => { + let definition_location = def.location(); + let head_location = NodeLocation::recompose(def.location(), def.name.location()); + let mut diagnostic = ApolloDiagnostic::new( db, definition_location, DiagnosticData::RecursiveDirectiveDefinition { name: def.name.to_string(), }, ) - .label(Label::new(head_location, "recursive directive definition")) - .label(Label::new(directive_location, "refers to itself here")), - ); + .label(Label::new(head_location, "recursive directive definition")); + + if let Some((cyclical_application, path)) = trace.split_first() { + let mut prev_name = &def.name; + for directive_application in path.iter().rev() { + diagnostic = diagnostic.label(Label::new( + directive_application.location(), + format!( + "`{}` references `{}` here...", + prev_name, directive_application.name + ), + )); + prev_name = &directive_application.name; + } + + diagnostic = diagnostic.label(Label::new( + cyclical_application.location(), + format!( + "`{}` circularly references `{}` here", + prev_name, cyclical_application.name + ), + )); + } + + diagnostics.push(diagnostic); + } + Err(CycleError::Limit(_)) => { + let diagnostic = ApolloDiagnostic::new( + db, + def.location(), + DiagnosticData::DeeplyNestedType { + name: def.name.to_string(), + }, + ) + .label(Label::new( + def.location(), + "directive references a very long chain of directives", + )); + + diagnostics.push(diagnostic); + } } diagnostics diff --git a/crates/apollo-compiler/src/validation/fragment.rs b/crates/apollo-compiler/src/validation/fragment.rs index 17864253e..ec1237528 100644 --- a/crates/apollo-compiler/src/validation/fragment.rs +++ b/crates/apollo-compiler/src/validation/fragment.rs @@ -1,10 +1,9 @@ use crate::diagnostics::{ApolloDiagnostic, DiagnosticData, Label}; -use crate::validation::{FileId, NodeLocation, RecursionGuard, RecursionStack}; +use crate::validation::operation::OperationValidationConfig; +use crate::validation::{CycleError, FileId, NodeLocation, RecursionGuard, RecursionStack}; use crate::{ast, schema, Node, ValidationDatabase}; use std::collections::{HashMap, HashSet}; -use super::operation::OperationValidationConfig; - /// Given a type definition, find all the type names that can be used for fragment spreading. /// /// Spec: https://spec.graphql.org/October2021/#GetPossibleTypes() @@ -300,13 +299,13 @@ pub(crate) fn validate_fragment_cycles( named_fragments: &HashMap>, selection_set: &[ast::Selection], visited: &mut RecursionGuard<'_>, - ) -> Result<(), Vec>> { + ) -> Result<(), CycleError> { 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(vec![spread.clone()]); + return Err(CycleError::Recursed(vec![spread.clone()])); } continue; } @@ -315,12 +314,9 @@ pub(crate) fn validate_fragment_cycles( detect_fragment_cycles( named_fragments, &fragment.selection_set, - &mut visited.push(&fragment.name), + &mut visited.push(&fragment.name)?, ) - .map_err(|mut trace| { - trace.push(spread.clone()); - trace - })?; + .map_err(|error| error.trace(spread))?; } } ast::Selection::InlineFragment(inline) => { @@ -335,45 +331,63 @@ pub(crate) fn validate_fragment_cycles( Ok(()) } - let mut visited = RecursionStack::with_root(def.name.clone()); + let mut visited = RecursionStack::with_root(def.name.clone()).with_limit(100); - if let Err(cycle) = - detect_fragment_cycles(&named_fragments, &def.selection_set, &mut visited.guard()) - { - let head_location = NodeLocation::recompose(def.location(), def.name.location()); + match detect_fragment_cycles(&named_fragments, &def.selection_set, &mut visited.guard()) { + Ok(_) => {} + Err(CycleError::Recursed(trace)) => { + let head_location = NodeLocation::recompose(def.location(), def.name.location()); - let mut diagnostic = ApolloDiagnostic::new( - db, - def.location(), - DiagnosticData::RecursiveFragmentDefinition { - name: def.name.to_string(), - }, - ) - .label(Label::new(head_location, "recursive fragment definition")); + 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)) = trace.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; + } - 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(), + cyclical_spread.location(), format!( - "`{}` references `{}` here...", - prev_name, spread.fragment_name + "`{}` circularly references `{}` here", + prev_name, cyclical_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); } + Err(CycleError::Limit(_)) => { + let head_location = NodeLocation::recompose(def.location(), def.name.location()); - diagnostics.push(diagnostic); + let diagnostic = ApolloDiagnostic::new( + db, + def.location(), + DiagnosticData::DeeplyNestedType { + name: def.name.to_string(), + }, + ) + .label(Label::new( + head_location, + "fragment references a very long chain of fragments", + )); + + diagnostics.push(diagnostic); + } } diagnostics diff --git a/crates/apollo-compiler/src/validation/input_object.rs b/crates/apollo-compiler/src/validation/input_object.rs index 361fb9789..3590ec752 100644 --- a/crates/apollo-compiler/src/validation/input_object.rs +++ b/crates/apollo-compiler/src/validation/input_object.rs @@ -2,7 +2,7 @@ use crate::{ ast, diagnostics::{ApolloDiagnostic, DiagnosticData, Label}, schema, - validation::{RecursionGuard, RecursionStack}, + validation::{CycleError, RecursionGuard, RecursionStack}, Node, ValidationDatabase, }; use std::collections::HashMap; @@ -18,7 +18,7 @@ impl FindRecursiveInputValue<'_> { &self, seen: &mut RecursionGuard<'_>, def: &Node, - ) -> Result<(), Node> { + ) -> Result<(), CycleError> { return match &*def.ty { // NonNull type followed by Named type is the one that's not allowed // to be cyclical, so this is only case we care about. @@ -26,11 +26,12 @@ impl FindRecursiveInputValue<'_> { // Everything else may be a cyclical input value. ast::Type::NonNullNamed(name) => { if !seen.contains(name) { - if let Some(def) = self.db.ast_types().input_objects.get(name) { - self.input_object_definition(seen.push(name), def)? + if let Some(object_def) = self.db.ast_types().input_objects.get(name) { + self.input_object_definition(seen.push(name)?, object_def) + .map_err(|err| err.trace(def))? } } else if seen.first() == Some(name) { - return Err(def.clone()); + return Err(CycleError::Recursed(vec![def.clone()])); } Ok(()) @@ -43,7 +44,7 @@ impl FindRecursiveInputValue<'_> { &self, mut seen: RecursionGuard<'_>, input_object: &ast::TypeWithExtensions, - ) -> Result<(), Node> { + ) -> Result<(), CycleError> { for input_value in input_object.fields() { self.input_value_definition(&mut seen, input_value)?; } @@ -54,7 +55,7 @@ impl FindRecursiveInputValue<'_> { fn check( db: &dyn ValidationDatabase, input_object: &ast::TypeWithExtensions, - ) -> Result<(), Node> { + ) -> Result<(), CycleError> { let mut recursion_stack = RecursionStack::with_root(input_object.definition.name.clone()); FindRecursiveInputValue { db } .input_object_definition(recursion_stack.guard(), input_object) @@ -85,23 +86,56 @@ pub(crate) fn validate_input_object_definition( Default::default(), ); - if let Err(input_val) = FindRecursiveInputValue::check(db, &input_object) { - let mut labels = vec![Label::new( - input_object.definition.location(), - "cyclical input object definition", - )]; - let loc = input_val.location(); - labels.push(Label::new(loc, "refers to itself here")); - diagnostics.push( - ApolloDiagnostic::new( + match FindRecursiveInputValue::check(db, &input_object) { + Ok(_) => {} + Err(CycleError::Recursed(trace)) => { + let mut diagnostic = ApolloDiagnostic::new( db, input_object.definition.location(), DiagnosticData::RecursiveInputObjectDefinition { name: input_object.definition.name.to_string(), }, ) - .labels(labels), - ) + .label(Label::new( + input_object.definition.location(), + "cyclical input object definition", + )); + + if let Some((cyclical_reference, path)) = trace.split_first() { + let mut prev_name = &input_object.definition.name; + for reference in path.iter().rev() { + diagnostic = diagnostic.label(Label::new( + reference.location(), + format!("`{}` references `{}` here...", prev_name, reference.name), + )); + prev_name = &reference.name; + } + + diagnostic = diagnostic.label(Label::new( + cyclical_reference.location(), + format!( + "`{}` circularly references `{}` here", + prev_name, cyclical_reference.name + ), + )); + } + diagnostics.push(diagnostic); + } + Err(CycleError::Limit(_)) => { + let diagnostic = ApolloDiagnostic::new( + db, + input_object.definition.location(), + DiagnosticData::DeeplyNestedType { + name: input_object.definition.name.to_string(), + }, + ) + .label(Label::new( + input_object.definition.location(), + "input object references a very long chain of input objects", + )); + + diagnostics.push(diagnostic); + } } // Fields in an Input Object Definition must be unique diff --git a/crates/apollo-compiler/src/validation/mod.rs b/crates/apollo-compiler/src/validation/mod.rs index b32b2bad2..ceff5a041 100644 --- a/crates/apollo-compiler/src/validation/mod.rs +++ b/crates/apollo-compiler/src/validation/mod.rs @@ -25,6 +25,7 @@ use crate::ast::Name; use crate::executable::BuildError as ExecutableBuildError; use crate::execution::{GraphQLError, GraphQLLocation}; use crate::schema::BuildError as SchemaBuildError; +use crate::Node; use crate::SourceFile; use crate::SourceMap; use indexmap::IndexSet; @@ -565,21 +566,44 @@ impl From for Details { } } +const DEFAULT_RECURSION_LIMIT: usize = 32; + +#[derive(Debug, Clone, thiserror::Error)] +#[error("Recursion limit reached")] +#[non_exhaustive] +struct RecursionLimitError {} + /// Track used names in a recursive function. +#[derive(Debug)] struct RecursionStack { seen: IndexSet, + high: usize, + limit: usize, } impl RecursionStack { + fn new() -> Self { + Self { + seen: IndexSet::new(), + high: 0, + limit: DEFAULT_RECURSION_LIMIT, + } + } + fn with_root(root: Name) -> Self { - let mut seen = IndexSet::new(); - seen.insert(root); - Self { seen } + let mut stack = Self::new(); + stack.seen.insert(root); + stack + } + + fn with_limit(mut self, limit: usize) -> Self { + self.limit = limit; + self } /// Return the actual API for tracking recursive uses. pub(crate) fn guard(&mut self) -> RecursionGuard<'_> { - RecursionGuard(&mut self.seen) + RecursionGuard(self) } } @@ -588,29 +612,55 @@ impl RecursionStack { /// Pass the result of `guard.push(name)` to recursive calls. Use `guard.contains(name)` to check /// if the name was used somewhere up the call stack. When a guard is dropped, its name is removed /// from the list. -struct RecursionGuard<'a>(&'a mut IndexSet); +struct RecursionGuard<'a>(&'a mut RecursionStack); impl RecursionGuard<'_> { - /// Mark that we saw a name. - fn push(&mut self, name: &Name) -> RecursionGuard<'_> { + /// Mark that we saw a name. If there are too many names, return an error. + fn push(&mut self, name: &Name) -> Result, RecursionLimitError> { debug_assert!( - self.0.insert(name.clone()), + self.0.seen.insert(name.clone()), "cannot push the same name twice to RecursionGuard, check contains() first" ); - RecursionGuard(self.0) + self.0.high = self.0.high.max(self.0.seen.len()); + if self.0.seen.len() > self.0.limit { + Err(RecursionLimitError {}) + } else { + Ok(RecursionGuard(self.0)) + } } /// Check if we saw a name somewhere up the call stack. fn contains(&self, name: &Name) -> bool { - self.0.iter().any(|seen| seen == name) + self.0.seen.iter().any(|seen| seen == name) } /// Return the name where we started. fn first(&self) -> Option<&Name> { - self.0.first() + self.0.seen.first() } } impl Drop for RecursionGuard<'_> { fn drop(&mut self) { // This may already be empty if it's the original `stack.guard()` result, but that's fine - self.0.pop(); + let _ = self.0.seen.pop(); + } +} + +/// Errors that can happen when chasing potentially cyclical references. +#[derive(Debug, Clone, thiserror::Error)] +enum CycleError { + /// Detected a cycle, value contains the path from the offending node back to the node where we + /// started. + #[error("Cycle detected")] + Recursed(Vec>), + /// Ran into recursion limit before a cycle could be detected. + #[error(transparent)] + Limit(#[from] RecursionLimitError), +} + +impl CycleError { + fn trace(mut self, node: &Node) -> Self { + if let Self::Recursed(trace) = &mut self { + trace.push(node.clone()); + } + self } } diff --git a/crates/apollo-compiler/src/validation/variable.rs b/crates/apollo-compiler/src/validation/variable.rs index 911af1345..eb05aa71b 100644 --- a/crates/apollo-compiler/src/validation/variable.rs +++ b/crates/apollo-compiler/src/validation/variable.rs @@ -1,5 +1,7 @@ use crate::diagnostics::{ApolloDiagnostic, DiagnosticData, Label}; -use crate::validation::{FileId, NodeLocation}; +use crate::validation::{ + FileId, NodeLocation, RecursionGuard, RecursionLimitError, RecursionStack, +}; use crate::{ast, schema, Node, ValidationDatabase}; use std::collections::hash_map::Entry; use std::collections::{HashMap, HashSet}; @@ -105,16 +107,11 @@ pub(crate) fn validate_variable_definitions2( diagnostics } -enum SelectionPathElement<'ast> { - Field(&'ast ast::Field), - Fragment(&'ast ast::FragmentSpread), - InlineFragment(&'ast ast::InlineFragment), -} fn walk_selections( document: &ast::Document, selections: &[ast::Selection], mut f: impl FnMut(&ast::Selection), -) { +) -> Result<(), RecursionLimitError> { type NamedFragments = HashMap>; let named_fragments: NamedFragments = document .definitions @@ -127,55 +124,48 @@ fn walk_selections( }) .collect(); - fn walk_selections_inner<'ast: 'path, 'path>( + fn walk_selections_inner<'ast, 'guard>( named_fragments: &'ast NamedFragments, selections: &'ast [ast::Selection], - path: &mut Vec>, + guard: &mut RecursionGuard<'guard>, f: &mut dyn FnMut(&ast::Selection), - ) { + ) -> Result<(), RecursionLimitError> { for selection in selections { f(selection); match selection { ast::Selection::Field(field) => { - path.push(SelectionPathElement::Field(field)); - walk_selections_inner(named_fragments, &field.selection_set, path, f); - path.pop(); + walk_selections_inner(named_fragments, &field.selection_set, guard, f)?; } ast::Selection::FragmentSpread(fragment) => { - // prevent recursion - if path.iter().any(|element| { - matches!(element, SelectionPathElement::Fragment(walked) if walked.fragment_name == fragment.fragment_name) - }) { + // Prevent chasing a cyclical reference. + // Note we do not report `CycleError::Recursed` here, as that is already caught + // by the cyclical fragment validation--we just need to ensure that we don't + // overflow the stack. + if guard.contains(&fragment.fragment_name) { continue; } - path.push(SelectionPathElement::Fragment(fragment)); if let Some(fragment_definition) = named_fragments.get(&fragment.fragment_name) { walk_selections_inner( named_fragments, &fragment_definition.selection_set, - path, + &mut guard.push(&fragment.fragment_name)?, f, - ); + )?; } - path.pop(); } ast::Selection::InlineFragment(fragment) => { - path.push(SelectionPathElement::InlineFragment(fragment)); - walk_selections_inner(named_fragments, &fragment.selection_set, path, f); - path.pop(); + walk_selections_inner(named_fragments, &fragment.selection_set, guard, f)?; } } } + Ok(()) } - walk_selections_inner( - &named_fragments, - selections, - &mut Default::default(), - &mut f, - ); + let mut stack = RecursionStack::new().with_limit(100); + let result = walk_selections_inner(&named_fragments, selections, &mut stack.guard(), &mut f); + result } fn variables_in_value(value: &ast::Value) -> impl Iterator + '_ { @@ -218,6 +208,8 @@ pub(crate) fn validate_unused_variables( file_id: FileId, operation: Node, ) -> Vec { + let mut diagnostics = Vec::new(); + let defined_vars: HashSet<_> = operation .variables .iter() @@ -234,24 +226,31 @@ pub(crate) fn validate_unused_variables( }) .collect(); let mut used_vars = HashSet::::new(); - walk_selections( - &db.ast(file_id), - &operation.selection_set, - |selection| match selection { - ast::Selection::Field(field) => { - used_vars.extend(variables_in_directives(&field.directives)); - used_vars.extend(variables_in_arguments(&field.arguments)); - } - ast::Selection::FragmentSpread(fragment) => { - used_vars.extend(variables_in_directives(&fragment.directives)); - } - ast::Selection::InlineFragment(fragment) => { - used_vars.extend(variables_in_directives(&fragment.directives)); - } - }, - ); - - let mut diagnostics = Vec::new(); + let walked = + walk_selections( + &db.ast(file_id), + &operation.selection_set, + |selection| match selection { + ast::Selection::Field(field) => { + used_vars.extend(variables_in_directives(&field.directives)); + used_vars.extend(variables_in_arguments(&field.arguments)); + } + ast::Selection::FragmentSpread(fragment) => { + used_vars.extend(variables_in_directives(&fragment.directives)); + } + ast::Selection::InlineFragment(fragment) => { + used_vars.extend(variables_in_directives(&fragment.directives)); + } + }, + ); + if walked.is_err() { + diagnostics.push(ApolloDiagnostic::new( + db, + None, + DiagnosticData::RecursionError {}, + )); + return diagnostics; + } let unused_vars = defined_vars.difference(&used_vars); diff --git a/crates/apollo-compiler/test_data/diagnostics/0070_self_referential_directive_definition.txt b/crates/apollo-compiler/test_data/diagnostics/0070_self_referential_directive_definition.txt index 1f0d8cf46..c83967beb 100644 --- a/crates/apollo-compiler/test_data/diagnostics/0070_self_referential_directive_definition.txt +++ b/crates/apollo-compiler/test_data/diagnostics/0070_self_referential_directive_definition.txt @@ -5,14 +5,14 @@ Error: `invalidExample` directive definition cannot reference itself │ ────────────┬──────────── ───────┬─────── │ ╰────────────────────────────────────────── recursive directive definition │ │ - │ ╰───────── refers to itself here + │ ╰───────── `invalidExample` circularly references `invalidExample` here ───╯ Error: `deprecatedType` directive definition cannot reference itself ╭─[0070_self_referential_directive_definition.graphql:11:1] │ 10 │ extend scalar String @deprecatedType(reason: "use OurCustomString instead") │ ───────────────────────────┬────────────────────────── - │ ╰──────────────────────────── refers to itself here + │ ╰──────────────────────────── `deprecatedType` circularly references `deprecatedType` here 11 │ directive @deprecatedType(reason: String!) on OBJECT | INTERFACE | ENUM | SCALAR | UNION │ ────────────┬──────────── │ ╰────────────── recursive directive definition @@ -21,39 +21,53 @@ Error: `loopA` directive definition cannot reference itself ╭─[0070_self_referential_directive_definition.graphql:13:1] │ 13 │ directive @loopA(arg: Boolean @loopB) on ARGUMENT_DEFINITION - │ ────────┬─────── - │ ╰───────── recursive directive definition - │ + │ ────────┬─────── ───┬── + │ ╰───────────────────────────── recursive directive definition + │ │ + │ ╰──── `loopA` references `loopB` here... + 14 │ directive @loopB(arg: Boolean @loopC) on ARGUMENT_DEFINITION + │ ───┬── + │ ╰──── `loopB` references `loopC` here... 15 │ directive @loopC(arg: Boolean @loopA) on ARGUMENT_DEFINITION │ ───┬── - │ ╰──── refers to itself here + │ ╰──── `loopC` circularly references `loopA` here ────╯ Error: `loopB` directive definition cannot reference itself ╭─[0070_self_referential_directive_definition.graphql:14:1] │ 13 │ directive @loopA(arg: Boolean @loopB) on ARGUMENT_DEFINITION │ ───┬── - │ ╰──── refers to itself here + │ ╰──── `loopA` circularly references `loopB` here 14 │ directive @loopB(arg: Boolean @loopC) on ARGUMENT_DEFINITION - │ ────────┬─────── - │ ╰───────── recursive directive definition + │ ────────┬─────── ───┬── + │ ╰───────────────────────────── recursive directive definition + │ │ + │ ╰──── `loopB` references `loopC` here... + 15 │ directive @loopC(arg: Boolean @loopA) on ARGUMENT_DEFINITION + │ ───┬── + │ ╰──── `loopC` references `loopA` here... ────╯ Error: `loopC` directive definition cannot reference itself ╭─[0070_self_referential_directive_definition.graphql:15:1] │ + 13 │ directive @loopA(arg: Boolean @loopB) on ARGUMENT_DEFINITION + │ ───┬── + │ ╰──── `loopA` references `loopB` here... 14 │ directive @loopB(arg: Boolean @loopC) on ARGUMENT_DEFINITION │ ───┬── - │ ╰──── refers to itself here + │ ╰──── `loopB` circularly references `loopC` here 15 │ directive @loopC(arg: Boolean @loopA) on ARGUMENT_DEFINITION - │ ────────┬─────── - │ ╰───────── recursive directive definition + │ ────────┬─────── ───┬── + │ ╰───────────────────────────── recursive directive definition + │ │ + │ ╰──── `loopC` references `loopA` here... ────╯ Error: `wrong` directive definition cannot reference itself ╭─[0070_self_referential_directive_definition.graphql:21:1] │ 18 │ extend enum Enum { RECURSIVE @wrong } │ ───┬── - │ ╰──── refers to itself here + │ ╰──── `wrong` circularly references `wrong` here │ 21 │ directive @wrong(input: InputObject) on INPUT_FIELD_DEFINITION | ENUM_VALUE │ ────────┬─────── diff --git a/crates/apollo-compiler/test_data/diagnostics/0084_circular_non_nullable_input_objects.txt b/crates/apollo-compiler/test_data/diagnostics/0084_circular_non_nullable_input_objects.txt index b7b89e61c..e32b24529 100644 --- a/crates/apollo-compiler/test_data/diagnostics/0084_circular_non_nullable_input_objects.txt +++ b/crates/apollo-compiler/test_data/diagnostics/0084_circular_non_nullable_input_objects.txt @@ -2,50 +2,93 @@ Error: `First` input object cannot reference itself ╭─[0084_circular_non_nullable_input_objects.graphql:6:1] │ 6 │ ╭─▶ input First { + 7 │ │ second: Second! + │ │ ───────┬─────── + │ │ ╰───────── `First` references `second` here... ┆ ┆ 9 │ ├─▶ } │ │ │ ╰─────── cyclical input object definition │ + 12 │ third: Third! + │ ──────┬────── + │ ╰──────── `second` references `third` here... + │ + 17 │ fourth: Fourth! + │ ───────┬─────── + │ ╰───────── `third` references `fourth` here... + │ 22 │ first: First! │ ──────┬────── - │ ╰──────── refers to itself here + │ ╰──────── `fourth` circularly references `first` here ────╯ Error: `Second` input object cannot reference itself ╭─[0084_circular_non_nullable_input_objects.graphql:11:1] │ 7 │ second: Second! │ ───────┬─────── - │ ╰───────── refers to itself here + │ ╰───────── `first` circularly references `second` here │ 11 │ ╭─▶ input Second { + 12 │ │ third: Third! + │ │ ──────┬────── + │ │ ╰──────── `Second` references `third` here... ┆ ┆ 14 │ ├─▶ } │ │ │ ╰─────── cyclical input object definition + │ + 17 │ fourth: Fourth! + │ ───────┬─────── + │ ╰───────── `third` references `fourth` here... + │ + 22 │ first: First! + │ ──────┬────── + │ ╰──────── `fourth` references `first` here... ────╯ Error: `Third` input object cannot reference itself ╭─[0084_circular_non_nullable_input_objects.graphql:16:1] │ + 7 │ second: Second! + │ ───────┬─────── + │ ╰───────── `first` references `second` here... + │ 12 │ third: Third! │ ──────┬────── - │ ╰──────── refers to itself here + │ ╰──────── `second` circularly references `third` here │ 16 │ ╭─▶ input Third { + 17 │ │ fourth: Fourth! + │ │ ───────┬─────── + │ │ ╰───────── `Third` references `fourth` here... ┆ ┆ 19 │ ├─▶ } │ │ │ ╰─────── cyclical input object definition + │ + 22 │ first: First! + │ ──────┬────── + │ ╰──────── `fourth` references `first` here... ────╯ Error: `Fourth` input object cannot reference itself ╭─[0084_circular_non_nullable_input_objects.graphql:21:1] │ + 7 │ second: Second! + │ ───────┬─────── + │ ╰───────── `first` references `second` here... + │ + 12 │ third: Third! + │ ──────┬────── + │ ╰──────── `second` references `third` here... + │ 17 │ fourth: Fourth! │ ───────┬─────── - │ ╰───────── refers to itself here + │ ╰───────── `third` circularly references `fourth` here │ 21 │ ╭─▶ input Fourth { - ┆ ┆ + 22 │ │ first: First! + │ │ ──────┬────── + │ │ ╰──────── `Fourth` references `first` here... 23 │ ├─▶ } │ │ │ ╰────── cyclical input object definition diff --git a/crates/apollo-compiler/tests/validation/mod.rs b/crates/apollo-compiler/tests/validation/mod.rs index 686ad0f69..e76be1169 100644 --- a/crates/apollo-compiler/tests/validation/mod.rs +++ b/crates/apollo-compiler/tests/validation/mod.rs @@ -1,6 +1,7 @@ mod interface; mod object; mod operation; +mod recursion; mod types; mod variable; diff --git a/crates/apollo-compiler/tests/validation/recursion.rs b/crates/apollo-compiler/tests/validation/recursion.rs new file mode 100644 index 000000000..6639e0179 --- /dev/null +++ b/crates/apollo-compiler/tests/validation/recursion.rs @@ -0,0 +1,178 @@ +fn build_fragment_chain(size: usize) -> String { + let mut query = r#" + query Introspection{ + __schema { + types { + ...typeFragment1 + } + } + } + "# + .to_string(); + + for i in 1..size { + query.push_str(&format!( + " + fragment typeFragment{i} on __Type {{ + ofType {{ + ...typeFragment{} + }} + }}", + i + 1 + )); + } + query.push_str(&format!( + " + fragment typeFragment{size} on __Type {{ + ofType {{ + name + }} + }}" + )); + + query +} + +fn build_directive_chain(size: usize) -> String { + let mut schema = r#" + type Query { + field: Int! @directive(arg: true) + } + + directive @directive(arg: Boolean @argDir1) on FIELD_DEFINITION + "# + .to_string(); + + for i in 1..size { + schema.push_str(&format!( + " + directive @argDir{i}(arg: Boolean @argDir{}) on ARGUMENT_DEFINITION + ", + i + 1 + )); + } + schema.push_str(&format!( + " + directive @argDir{size}(arg: Boolean) on ARGUMENT_DEFINITION + " + )); + + schema +} + +fn build_input_object_chain(size: usize) -> String { + let mut schema = r#" + type Query { + field(arg: VeryVeryDeep): Boolean + } + + input VeryVeryDeep { + nest: VeryVeryDeep1! + } + "# + .to_string(); + + for i in 1..size { + schema.push_str(&format!( + " + input VeryVeryDeep{i} {{ nest: VeryVeryDeep{}! }} + ", + i + 1 + )); + } + schema.push_str(&format!( + " + input VeryVeryDeep{size} {{ final: Boolean }} + " + )); + + schema +} + +#[test] +fn long_fragment_chains_do_not_overflow_stack() { + // Build a query that applies 1K fragments + // Validating it would take a lot of recursion and blow the stack + let query = build_fragment_chain(1_000); + + let errors = apollo_compiler::parse_mixed_validate( + format!( + "type Query {{ a: Int }} + {query}" + ), + "overflow.graphql", + ) + .expect_err("must have recursion errors"); + + let expected = expect_test::expect![[r#" + Error: too much recursion + Error: `typeFragment1` contains too much nesting + ╭─[overflow.graphql:11:11] + │ + 11 │ fragment typeFragment1 on __Type { + │ ───────────┬────────── + │ ╰──────────── fragment references a very long chain of fragments + ────╯ + "#]]; + expected.assert_eq(&errors.to_string_no_color()); +} + +#[test] +fn not_long_enough_fragment_chain_applies_correctly() { + // Stay just under the recursion limit + let query = build_fragment_chain(99); + + let _ = apollo_compiler::parse_mixed_validate( + format!( + "type Query {{ a: Int }} + {query}" + ), + "no_overflow.graphql", + ) + .expect("must not have recursion errors"); +} + +#[test] +fn long_directive_chains_do_not_overflow_stack() { + // Build a schema that defines hundreds of directives that all use each other in their + // argument list + // Validating it would take a lot of recursion and a lot of time + let schema = build_directive_chain(500); + + let partial = apollo_compiler::Schema::parse_and_validate(schema, "directives.graphql") + .expect_err("must have recursion errors"); + + assert_eq!(partial.errors.len(), 469); +} + +#[test] +fn not_long_enough_directive_chain_applies_correctly() { + // Stay just under the recursion limit + let schema = build_directive_chain(31); + + let _schema = apollo_compiler::Schema::parse_and_validate(schema, "directives.graphql") + .expect("must not have recursion errors"); +} + +#[test] +fn long_input_object_chains_do_not_overflow_stack() { + // Build a very deeply nested input object + // Validating it would take a lot of recursion and a lot of time + let schema = build_input_object_chain(500); + + let partial = apollo_compiler::Schema::parse_and_validate(schema, "input_objects.graphql") + .expect_err("must have recursion errors"); + + // The final 199 input objects do not cause recursion errors because the chain is less than 200 + // directives deep. + assert_eq!(partial.errors.len(), 469); +} + +#[test] +fn not_long_enough_input_object_chain_applies_correctly() { + // Stay just under the recursion limit + let schema = build_input_object_chain(31); + + let _schema = apollo_compiler::Schema::parse_and_validate(schema, "input_objects.graphql") + .expect("must not have recursion errors"); +}