From 1c2cead500812bf6a87c392e3b94933d18141980 Mon Sep 17 00:00:00 2001 From: Graeme Coupar Date: Mon, 22 Jan 2024 12:07:13 +0000 Subject: [PATCH] Fix some problems around recursive queries (#839) 1. The recurse directive was not working properly if there was an `InlineFragments` in the recursive path. 2. Impose a maximum query depth of 4096 and panic if that's exceeded rather than leaving it to a stack overflow. Hopefully that's enough for people though if not I can bump it up. 3. Fixed an issue where we could end up accidentally printing an empty inline fragment. Fixes #838 --- CHANGELOG.md | 12 +++++++ cynic/src/queries/ast.rs | 12 ++++--- cynic/src/queries/builders.rs | 55 ++++++++++++++++++++++---------- cynic/tests/recursive-queries.rs | 53 ++++++++++++++++++++++++++++++ cynic/tests/test-schema.graphql | 3 ++ 5 files changed, 114 insertions(+), 21 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 22ee43e0f..311452274 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,18 @@ This project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.html ## Unreleased - xxxx-xx-xx +### Changes + +- If you accidentally write a recursive query without the `recurse` attribute + you'll now get a panic suggesting to use `recurse`. This may cause false + positives if you're writing a particularly large query - users should raise + an issue if I've picked a number that's too low. + +### Bug Fixes + +- The `recurse` attribute now works if an `InlineFragments` derive is used in + the recursive path. + ## v3.4.1 - 2024-01-22 ### Bug Fixes diff --git a/cynic/src/queries/ast.rs b/cynic/src/queries/ast.rs index cf2b7b0df..f09fafcf3 100644 --- a/cynic/src/queries/ast.rs +++ b/cynic/src/queries/ast.rs @@ -132,11 +132,15 @@ impl std::fmt::Display for Selection { write!(f, "{}", field_selection.children) } Selection::InlineFragment(inline_fragment) => { - write!(f, "...")?; - if let Some(on_type) = inline_fragment.on_clause { - write!(f, " on {}", on_type)?; + // Don't print any empty fragments - this can happen in recursive queries... + if !inline_fragment.children.selections.is_empty() { + write!(f, "...")?; + if let Some(on_type) = inline_fragment.on_clause { + write!(f, " on {}", on_type)?; + } + write!(f, "{}", inline_fragment.children)?; } - write!(f, "{}", inline_fragment.children) + Ok(()) } } } diff --git a/cynic/src/queries/builders.rs b/cynic/src/queries/builders.rs index 24b308e84..557cdab0d 100644 --- a/cynic/src/queries/builders.rs +++ b/cynic/src/queries/builders.rs @@ -4,12 +4,15 @@ use crate::{coercions::CoercesTo, schema, variables::VariableDefinition}; use super::{ast::*, to_input_literal, FlattensInto, IsFieldType, Recursable}; +// The maximum depth we'll recurse to before assuming something is wrong +// and giving up +const MAX_DEPTH: u16 = 4096; + /// Builds a SelectionSet for the given `SchemaType` and `VariablesFields` pub struct SelectionBuilder<'a, SchemaType, VariablesFields> { phantom: PhantomData (SchemaType, VariablesFields)>, selection_set: &'a mut SelectionSet, has_typename: bool, - recurse_depth: Option, context: BuilderContext<'a>, } @@ -19,7 +22,6 @@ impl<'a, T, U> SelectionBuilder<'a, Vec, U> { selection_set: self.selection_set, has_typename: self.has_typename, phantom: PhantomData, - recurse_depth: self.recurse_depth, context: self.context, } } @@ -31,7 +33,6 @@ impl<'a, T, U> SelectionBuilder<'a, Option, U> { selection_set: self.selection_set, has_typename: self.has_typename, phantom: PhantomData, - recurse_depth: self.recurse_depth, context: self.context, } } @@ -46,6 +47,8 @@ impl<'a, SchemaType, VariablesFields> SelectionBuilder<'a, SchemaType, Variables SelectionBuilder::private_new( selection_set, BuilderContext { + recurse_depth: None, + overall_depth: 0, features_enabled, variables_used, }, @@ -57,7 +60,6 @@ impl<'a, SchemaType, VariablesFields> SelectionBuilder<'a, SchemaType, Variables phantom: PhantomData, has_typename: false, selection_set, - recurse_depth: None, context, } } @@ -76,7 +78,6 @@ impl<'a, SchemaType, VariablesFields> SelectionBuilder<'a, SchemaType, Variables FieldType: IsFieldType, { FieldSelectionBuilder { - recurse_depth: self.recurse_depth, context: self.context, field: self.push_selection(FieldMarker::NAME), phantom: PhantomData, @@ -98,7 +99,6 @@ impl<'a, SchemaType, VariablesFields> SelectionBuilder<'a, SchemaType, Variables FieldType: IsFieldType, { FieldSelectionBuilder { - recurse_depth: self.recurse_depth, context: self.context, field: self.push_selection(FieldMarker::NAME), phantom: PhantomData, @@ -118,14 +118,14 @@ impl<'a, SchemaType, VariablesFields> SelectionBuilder<'a, SchemaType, Variables SchemaType: schema::HasField, FieldType: Recursable, { - let new_depth = self.recurse_depth.map(|d| d + 1).unwrap_or(0); + let context = self.context.recurse(); + let new_depth = context.recurse_depth.unwrap(); if new_depth >= max_depth { return None; } Some(FieldSelectionBuilder { - recurse_depth: Some(new_depth), - context: self.context, + context, field: self.push_selection(FieldMarker::NAME), phantom: PhantomData, }) @@ -181,7 +181,6 @@ pub struct FieldSelectionBuilder<'a, Field, SchemaType, VariablesFields> { #[allow(clippy::type_complexity)] phantom: PhantomData (Field, SchemaType, VariablesFields)>, field: &'a mut FieldSelection, - recurse_depth: Option, context: BuilderContext<'a>, } @@ -220,10 +219,7 @@ impl<'a, Field, FieldSchemaType, VariablesFields> where VariablesFields: VariableMatch, { - SelectionBuilder { - recurse_depth: self.recurse_depth, - ..SelectionBuilder::private_new(&mut self.field.children, self.context) - } + SelectionBuilder::private_new(&mut self.field.children, self.context.descend()) } } @@ -260,7 +256,7 @@ impl<'a, SchemaType, VariablesFields> InlineFragmentBuilder<'a, SchemaType, Vari where VariablesFields: VariableMatch, { - SelectionBuilder::private_new(&mut self.inline_fragment.children, self.context) + SelectionBuilder::private_new(&mut self.inline_fragment.children, self.context.descend()) } } @@ -323,7 +319,7 @@ where ObjectArgumentBuilder { fields, - context: self.context, + context: self.context.descend(), phantom: PhantomData, } } @@ -365,7 +361,7 @@ impl<'a, SchemaType, VariablesFields> InputBuilder<'a, Vec, Variable ListArgumentBuilder { items, - context: self.context, + context: self.context.descend(), phantom: PhantomData, } } @@ -445,4 +441,29 @@ impl VariableMatch<()> for T where T: crate::QueryVariablesFields {} struct BuilderContext<'a> { features_enabled: &'a HashSet, variables_used: &'a Sender<&'static str>, + recurse_depth: Option, + overall_depth: u16, +} + +impl BuilderContext<'_> { + pub fn descend(&self) -> Self { + let overall_depth = self.overall_depth + 1; + + assert!( + overall_depth < MAX_DEPTH, + "Maximum query depth exceeded. Have you forgotten to mark a query as recursive?", + ); + + Self { + overall_depth, + ..*self + } + } + + pub fn recurse(&self) -> Self { + Self { + recurse_depth: self.recurse_depth.map(|d| d + 1).or(Some(0)), + ..self.descend() + } + } } diff --git a/cynic/tests/recursive-queries.rs b/cynic/tests/recursive-queries.rs index affac4493..cc5b7c842 100644 --- a/cynic/tests/recursive-queries.rs +++ b/cynic/tests/recursive-queries.rs @@ -187,3 +187,56 @@ mod required_recursive_types { insta::assert_yaml_snapshot!(serde_json::from_value::(data).unwrap()); } } + +mod recursing_through_inline_fragments { + use cynic_proc_macros::InlineFragments; + + #[test] + fn test_inline_fragment_preserves_recurse_behaviour() { + use super::*; + + #[derive(QueryFragment, Serialize)] + #[cynic(graphql_type = "Query", schema_path = "tests/test-schema.graphql")] + struct AllDataQuery { + all_data: Vec, + } + + #[derive(InlineFragments, Serialize)] + #[cynic(schema_path = "tests/test-schema.graphql")] + enum PostOrAuthor { + Author(Author), + #[cynic(fallback)] + Other, + } + + #[derive(QueryFragment, Serialize)] + #[cynic(schema_path = "tests/test-schema.graphql")] + struct Author { + #[cynic(recurse = "2")] + silly_me: Option>, + } + + use cynic::QueryBuilder; + + let operation = AllDataQuery::build(()); + + insta::assert_display_snapshot!(operation.query, @r###" + query AllDataQuery { + allData { + __typename + ... on Author { + sillyMe { + __typename + ... on Author { + sillyMe { + __typename + } + } + } + } + } + } + + "###); + } +} diff --git a/cynic/tests/test-schema.graphql b/cynic/tests/test-schema.graphql index 557a46a22..468eb14aa 100644 --- a/cynic/tests/test-schema.graphql +++ b/cynic/tests/test-schema.graphql @@ -33,6 +33,9 @@ type Author implements Node { # A nonsense self referential field # Don't think this would make sense usually, but it's useful for testing. me: Author! + + # an even more nonsense self referential field + sillyMe: PostOrAuthor! } type EmptyType {