chore(federation): add unit tests and documentation for conditions handling

apollo-federation/src/query_plan/conditions.rs

@@ -1,3 +1,4 @@
+use std::fmt::Display;
 use std::sync::Arc;
 
 use apollo_compiler::ast::Directive;
@@ -35,17 +36,50 @@ impl ConditionKind {
     }
 }
 
-/// This struct is meant for tracking whether a selection set in a `FetchDependencyGraphNode` needs
+impl Display for ConditionKind {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        self.as_str().fmt(f)
+    }
+}
+
+/// Represents a combined set of conditions.
+///
+/// This struct is meant for tracking whether a selection set in a [FetchDependencyGraphNode] needs
 /// to be queried, based on the `@skip`/`@include` applications on the selections within.
-/// Accordingly, there is much logic around merging and short-circuiting; `OperationConditional` is
+/// Accordingly, there is much logic around merging and short-circuiting; [OperationConditional] is
 /// the more appropriate struct when trying to record the original structure/intent of those
 /// `@skip`/`@include` applications.
+///
+/// [FetchDependencyGraphNode]: crate::query_plan::fetch_dependency_graph::FetchDependencyGraphNode
+/// [OperationConditional]: crate::link::graphql_definition::OperationConditional
 #[derive(Debug, Clone, PartialEq, Serialize)]
 pub(crate) enum Conditions {
     Variables(VariableConditions),
     Boolean(bool),
 }
 
+impl Display for Conditions {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        // This uses GraphQL directive syntax.
+        // Add brackets to distinguish it from a real directive list.
+        write!(f, "[")?;
+
+        match self {
+            Conditions::Boolean(constant) => write!(f, "{constant:?}")?,
+            Conditions::Variables(variables) => {
+                for (index, (name, kind)) in variables.iter().enumerate() {
+                    if index > 0 {
+                        write!(f, " ")?;
+                    }
+                    write!(f, "@{kind}(if: ${name})")?;
+                }
+            }
+        }
+
+        write!(f, "]")
+    }
+}
+
 /// A list of variable conditions, represented as a map from variable names to whether that variable
 /// is negated in the condition. We maintain the invariant that there's at least one condition (i.e.
 /// the map is non-empty), and that there's at most one condition per variable name.
@@ -96,23 +130,28 @@ impl VariableConditions {
     }
 }
 
-#[derive(Debug, Clone, PartialEq)]
-pub(crate) struct VariableCondition {
-    variable: Name,
-    kind: ConditionKind,
-}
-
 impl Conditions {
-    /// Create conditions from a map of variable conditions. If empty, instead returns a
-    /// condition that always evaluates to true.
+    /// Create conditions from a map of variable conditions.
+    ///
+    /// If empty, instead returns a condition that always evaluates to true.
     fn from_variables(map: IndexMap<Name, ConditionKind>) -> Self {
         if map.is_empty() {
-            Self::Boolean(true)
+            Self::always()
         } else {
             Self::Variables(VariableConditions::new_unchecked(map))
         }
     }
 
+    /// Create conditions that always evaluate to true.
+    pub(crate) const fn always() -> Self {
+        Self::Boolean(true)
+    }
+
+    /// Create conditions that always evaluate to false.
+    pub(crate) const fn never() -> Self {
+        Self::Boolean(false)
+    }
+
     /// Parse @skip and @include conditions from a directive list.
     ///
     /// # Errors
@@ -127,7 +166,7 @@ impl Conditions {
 
             match value.as_ref() {
                 // Constant @skip(if: true) can never match
-                Value::Boolean(true) => return Ok(Self::Boolean(false)),
+                Value::Boolean(true) => return Ok(Self::never()),
                 // Constant @skip(if: false) always matches
                 Value::Boolean(_) => {}
                 Value::Variable(name) => {
@@ -146,7 +185,7 @@ impl Conditions {
 
             match value.as_ref() {
                 // Constant @include(if: false) can never match
-                Value::Boolean(false) => return Ok(Self::Boolean(false)),
+                Value::Boolean(false) => return Ok(Self::never()),
                 // Constant @include(if: true) always matches
                 Value::Boolean(true) => {}
                 // If both @skip(if: $var) and @include(if: $var) exist, the condition can also
@@ -155,7 +194,7 @@ impl Conditions {
                     if variables.insert(name.clone(), ConditionKind::Include)
                         == Some(ConditionKind::Skip)
                     {
-                        return Ok(Self::Boolean(false));
+                        return Ok(Self::never());
                     }
                 }
                 _ => {
@@ -167,10 +206,34 @@ impl Conditions {
         Ok(Self::from_variables(variables))
     }
 
-    // TODO(@goto-bus-stop): what exactly is the difference between this and `Self::merge`?
-    pub(crate) fn update_with(&self, new_conditions: &Self) -> Self {
-        match (new_conditions, self) {
-            (Conditions::Boolean(_), _) | (_, Conditions::Boolean(_)) => new_conditions.clone(),
+    /// Returns a new set of conditions that omits those conditions that are already handled by the
+    /// argument.
+    ///
+    /// For example, if we have a selection set like so:
+    /// ```graphql
+    /// {
+    ///   a @skip(if: $a) {
+    ///     b @skip(if: $a) @include(if: $b) {
+    ///       c
+    ///     }
+    ///   }
+    /// }
+    /// ```
+    /// Then we may call `b.conditions().update_with( a.conditions() )`, and get:
+    /// ```graphql
+    /// {
+    ///   a @skip(if: $a) {
+    ///     b @include(if: $b) {
+    ///       c
+    ///     }
+    ///   }
+    /// }
+    /// ```
+    /// because the `@skip(if: $a)` condition in `b` must always match, as implied by
+    /// being nested inside `a`.
+    pub(crate) fn update_with(&self, handled_conditions: &Self) -> Self {
+        match (self, handled_conditions) {
+            (Conditions::Boolean(_), _) | (_, Conditions::Boolean(_)) => self.clone(),
             (Conditions::Variables(new_conditions), Conditions::Variables(handled_conditions)) => {
                 let mut filtered = IndexMap::default();
                 for (cond_name, &cond_kind) in new_conditions.0.iter() {
@@ -179,7 +242,7 @@ impl Conditions {
                             // If we've already handled that exact condition, we can skip it.
                             // But if we've already handled the _negation_ of this condition, then this mean the overall conditions
                             // are unreachable and we can just return `false` directly.
-                            return Conditions::Boolean(false);
+                            return Conditions::never();
                         }
                         Some(_) => {}
                         None => {
@@ -198,7 +261,7 @@ impl Conditions {
         match (self, other) {
             // Absorbing element
             (Conditions::Boolean(false), _) | (_, Conditions::Boolean(false)) => {
-                Conditions::Boolean(false)
+                Conditions::never()
             }
 
             // Neutral element
@@ -207,7 +270,7 @@ impl Conditions {
             (Conditions::Variables(self_vars), Conditions::Variables(other_vars)) => {
                 match self_vars.merge(other_vars) {
                     Some(vars) => Conditions::Variables(vars),
-                    None => Conditions::Boolean(false),
+                    None => Conditions::never(),
                 }
             }
         }
@@ -366,3 +429,168 @@ fn matches_condition_for_kind(
         None => false,
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use apollo_compiler::ExecutableDocument;
+    use apollo_compiler::Schema;
+
+    use super::*;
+
+    fn parse(directives: &str) -> Conditions {
+        let schema =
+            Schema::parse_and_validate("type Query { a: String }", "schema.graphql").unwrap();
+        let doc =
+            ExecutableDocument::parse(&schema, format!("{{ a {directives} }}"), "query.graphql")
+                .unwrap();
+        let operation = doc.operations.get(None).unwrap();
+        let directives = operation.selection_set.selections[0].directives();
+        Conditions::from_directives(&DirectiveList::from(directives.clone())).unwrap()
+    }
+
+    #[test]
+    fn merge_conditions() {
+        assert_eq!(
+            parse("@skip(if: $a)")
+                .merge(parse("@include(if: $b)"))
+                .to_string(),
+            "[@skip(if: $a) @include(if: $b)]",
+            "combine skip/include"
+        );
+        assert_eq!(
+            parse("@skip(if: $a)")
+                .merge(parse("@skip(if: $b)"))
+                .to_string(),
+            "[@skip(if: $a) @skip(if: $b)]",
+            "combine multiple skips"
+        );
+        assert_eq!(
+            parse("@include(if: $a)")
+                .merge(parse("@include(if: $b)"))
+                .to_string(),
+            "[@include(if: $a) @include(if: $b)]",
+            "combine multiple includes"
+        );
+        assert_eq!(
+            parse("@skip(if: $a)").merge(parse("@include(if: $a)")),
+            Conditions::never(),
+            "skip/include with same variable conflicts"
+        );
+        assert_eq!(
+            parse("@skip(if: $a)").merge(Conditions::always()),
+            parse("@skip(if: $a)"),
+            "merge with `true` returns original"
+        );
+        assert_eq!(
+            Conditions::always().merge(Conditions::always()),
+            Conditions::always(),
+            "merge with `true` returns original"
+        );
+        assert_eq!(
+            parse("@skip(if: $a)").merge(Conditions::never()),
+            Conditions::never(),
+            "merge with `false` returns `false`"
+        );
+        assert_eq!(
+            parse("@include(if: $a)").merge(Conditions::never()),
+            Conditions::never(),
+            "merge with `false` returns `false`"
+        );
+        assert_eq!(
+            Conditions::always().merge(Conditions::never()),
+            Conditions::never(),
+            "merge with `false` returns `false`"
+        );
+        assert_eq!(
+            parse("@skip(if: true)").merge(parse("@include(if: $a)")),
+            Conditions::never(),
+            "@skip with hardcoded if: true can never evaluate to true"
+        );
+        assert_eq!(
+            parse("@skip(if: false)").merge(parse("@include(if: $a)")),
+            parse("@include(if: $a)"),
+            "@skip with hardcoded if: false returns other side"
+        );
+        assert_eq!(
+            parse("@include(if: true)").merge(parse("@include(if: $a)")),
+            parse("@include(if: $a)"),
+            "@include with hardcoded if: true returns other side"
+        );
+        assert_eq!(
+            parse("@include(if: false)").merge(parse("@include(if: $a)")),
+            Conditions::never(),
+            "@include with hardcoded if: false can never evaluate to true"
+        );
+    }
+
+    #[test]
+    fn update_conditions() {
+        assert_eq!(
+            parse("@skip(if: $a)")
+                .merge(parse("@include(if: $b)"))
+                .update_with(&parse("@include(if: $b)")),
+            parse("@skip(if: $a)"),
+            "trim @include(if:) condition"
+        );
+        assert_eq!(
+            parse("@skip(if: $a)")
+                .merge(parse("@include(if: $b)"))
+                .update_with(&parse("@skip(if: $a)")),
+            parse("@include(if: $b)"),
+            "trim @skip(if:) condition"
+        );
+
+        let list = parse("@skip(if: $a)")
+            .merge(parse("@skip(if: $b)"))
+            .merge(parse("@skip(if: $c)"))
+            .merge(parse("@skip(if: $d)"))
+            .merge(parse("@skip(if: $e)"));
+        let handled = parse("@skip(if: $b)").merge(parse("@skip(if: $e)"));
+        assert_eq!(
+            list.update_with(&handled),
+            parse("@skip(if: $a)")
+                .merge(parse("@skip(if: $c)"))
+                .merge(parse("@skip(if: $d)")),
+            "trim multiple conditions"
+        );
+
+        let list = parse("@include(if: $a)")
+            .merge(parse("@include(if: $b)"))
+            .merge(parse("@include(if: $c)"))
+            .merge(parse("@include(if: $d)"))
+            .merge(parse("@include(if: $e)"));
+        let handled = parse("@include(if: $b)").merge(parse("@include(if: $e)"));
+        assert_eq!(
+            list.update_with(&handled),
+            parse("@include(if: $a)")
+                .merge(parse("@include(if: $c)"))
+                .merge(parse("@include(if: $d)")),
+            "trim multiple conditions"
+        );
+
+        let list = parse("@include(if: $a)")
+            .merge(parse("@include(if: $b)"))
+            .merge(parse("@include(if: $c)"))
+            .merge(parse("@include(if: $d)"))
+            .merge(parse("@include(if: $e)"));
+        // It may technically be correct to return `never()` here?
+        // But the result for query planning is the same either way, as these conditions will never
+        // be reached.
+        assert_eq!(
+            list.update_with(&Conditions::never()),
+            list,
+            "update with constant does not affect conditions"
+        );
+
+        let list = parse("@include(if: $a)")
+            .merge(parse("@include(if: $b)"))
+            .merge(parse("@include(if: $c)"))
+            .merge(parse("@include(if: $d)"))
+            .merge(parse("@include(if: $e)"));
+        assert_eq!(
+            list.update_with(&Conditions::always()),
+            list,
+            "update with constant does not affect conditions"
+        );
+    }
+}

apollo-federation/src/query_plan/fetch_dependency_graph.rs

@@ -1779,7 +1779,10 @@ impl FetchDependencyGraph {
             .graph
             .node_weight_mut(node_index)
             .ok_or_else(|| FederationError::internal("Node unexpectedly missing"))?;
-        let conditions = handled_conditions.update_with(&node.selection_set.conditions);
+        let conditions = node
+            .selection_set
+            .conditions
+            .update_with(&handled_conditions);
         let new_handled_conditions = conditions.clone().merge(handled_conditions);
 
         let processed = processor.on_node(