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

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

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(())
             }
         }
     }

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<fn() -> (SchemaType, VariablesFields)>,
     selection_set: &'a mut SelectionSet,
     has_typename: bool,
-    recurse_depth: Option<u8>,
     context: BuilderContext<'a>,
 }
 
@@ -19,7 +22,6 @@ impl<'a, T, U> SelectionBuilder<'a, Vec<T>, 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<T>, 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<SchemaType::Type>,
     {
         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<SchemaType::Type>,
     {
         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<FieldMarker>,
         FieldType: Recursable<FieldMarker::Type>,
     {
-        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<fn() -> (Field, SchemaType, VariablesFields)>,
     field: &'a mut FieldSelection,
-    recurse_depth: Option<u8>,
     context: BuilderContext<'a>,
 }
 
@@ -220,10 +219,7 @@ impl<'a, Field, FieldSchemaType, VariablesFields>
     where
         VariablesFields: VariableMatch<InnerVariables>,
     {
-        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<InnerVariablesFields>,
     {
-        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<SchemaType>, Variable
 
         ListArgumentBuilder {
             items,
-            context: self.context,
+            context: self.context.descend(),
             phantom: PhantomData,
         }
     }
@@ -445,4 +441,29 @@ impl<T> VariableMatch<()> for T where T: crate::QueryVariablesFields {}
 struct BuilderContext<'a> {
     features_enabled: &'a HashSet<String>,
     variables_used: &'a Sender<&'static str>,
+    recurse_depth: Option<u8>,
+    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()
+        }
+    }
 }

cynic/tests/recursive-queries.rs

@@ -187,3 +187,56 @@ mod required_recursive_types {
         insta::assert_yaml_snapshot!(serde_json::from_value::<FriendsQuery>(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<PostOrAuthor>,
+        }
+
+        #[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<Box<PostOrAuthor>>,
+        }
+
+        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
+                  }
+                }
+              }
+            }
+          }
+        }
+
+        "###);
+    }
+}

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 {