fix(federation): correct order of @Skip and @include conditions on th…

…e same selection (#6137)

apollo-federation/src/operation/mod.rs

@@ -822,6 +822,9 @@ impl Selection {
         }
     }
 
+    /// # Errors
+    /// Returns an error if the selection contains a fragment spread, or if any of the
+    /// @skip/@include directives are invalid (per GraphQL validation rules).
     pub(crate) fn conditions(&self) -> Result<Conditions, FederationError> {
         let self_conditions = Conditions::from_directives(self.directives())?;
         if let Conditions::Boolean(false) = self_conditions {
@@ -2188,6 +2191,9 @@ impl SelectionSet {
         }
     }
 
+    /// # Errors
+    /// Returns an error if the selection set contains a fragment spread, or if any of the
+    /// @skip/@include directives are invalid (per GraphQL validation rules).
     pub(crate) fn conditions(&self) -> Result<Conditions, FederationError> {
         // If the conditions of all the selections within the set are the same,
         // then those are conditions of the whole set and we return it.

apollo-federation/src/query_plan/conditions.rs

@@ -9,6 +9,7 @@ use indexmap::map::Entry;
 use serde::Serialize;
 
 use crate::error::FederationError;
+use crate::internal_error;
 use crate::operation::DirectiveList;
 use crate::operation::NamedFragments;
 use crate::operation::Selection;
@@ -17,6 +18,23 @@ use crate::operation::SelectionMapperReturn;
 use crate::operation::SelectionSet;
 use crate::query_graph::graph_path::OpPathElement;
 
+#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize)]
+pub(crate) enum ConditionKind {
+    /// A `@skip(if:)` condition.
+    Skip,
+    /// An `@include(if:)` condition.
+    Include,
+}
+
+impl ConditionKind {
+    fn as_str(self) -> &'static str {
+        match self {
+            Self::Skip => "skip",
+            Self::Include => "include",
+        }
+    }
+}
+
 /// 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
@@ -32,99 +50,140 @@ pub(crate) enum Conditions {
 /// 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.
 #[derive(Debug, Clone, PartialEq, Serialize)]
-pub(crate) struct VariableConditions(Arc<IndexMap<Name, bool>>);
+pub(crate) struct VariableConditions(
+    // TODO(@goto-bus-stop): does it really make sense for this to be an indexmap? we normally only
+    // have 1 or 2. Can we ever get so many conditions on the same node that it makes sense to use
+    // a map over a vec?
+    Arc<IndexMap<Name, ConditionKind>>,
+);
 
 impl VariableConditions {
     /// Construct VariableConditions from a non-empty map of variable names.
     ///
     /// In release builds, this does not check if the map is empty.
-    fn new_unchecked(map: IndexMap<Name, bool>) -> Self {
+    fn new_unchecked(map: IndexMap<Name, ConditionKind>) -> Self {
         debug_assert!(!map.is_empty());
         Self(Arc::new(map))
     }
 
-    /// Returns whether a variable condition is negated, or None if there is no condition for the variable name.
-    pub(crate) fn is_negated(&self, name: &str) -> Option<bool> {
+    /// Returns the condition kind of a variable, or None if there is no condition for the variable name.
+    fn condition_kind(&self, name: &str) -> Option<ConditionKind> {
         self.0.get(name).copied()
     }
 
-    pub(crate) fn iter(&self) -> impl Iterator<Item = (&Name, bool)> {
-        self.0.iter().map(|(name, &negated)| (name, negated))
+    /// Iterate all variable conditions and their kinds.
+    pub(crate) fn iter(&self) -> impl Iterator<Item = (&Name, ConditionKind)> {
+        self.0.iter().map(|(name, &kind)| (name, kind))
+    }
+
+    /// Merge with another set of variable conditions. If the conditions conflict, returns `None`.
+    fn merge(mut self, other: Self) -> Option<Self> {
+        let vars = Arc::make_mut(&mut self.0);
+        for (name, other_kind) in other.0.iter() {
+            match vars.entry(name.clone()) {
+                // `@skip(if: $var)` and `@include(if: $var)` on the same selection always means
+                // it's not included.
+                Entry::Occupied(self_kind) if self_kind.get() != other_kind => {
+                    return None;
+                }
+                Entry::Occupied(_entry) => {}
+                Entry::Vacant(entry) => {
+                    entry.insert(*other_kind);
+                }
+            }
+        }
+        Some(self)
     }
 }
 
 #[derive(Debug, Clone, PartialEq)]
 pub(crate) struct VariableCondition {
     variable: Name,
-    negated: bool,
+    kind: ConditionKind,
 }
 
 impl Conditions {
     /// 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, bool>) -> Self {
+    fn from_variables(map: IndexMap<Name, ConditionKind>) -> Self {
         if map.is_empty() {
             Self::Boolean(true)
         } else {
             Self::Variables(VariableConditions::new_unchecked(map))
         }
     }
 
+    /// Parse @skip and @include conditions from a directive list.
+    ///
+    /// # Errors
+    /// Returns an error if a @skip/@include directive is invalid (per GraphQL validation rules).
     pub(crate) fn from_directives(directives: &DirectiveList) -> Result<Self, FederationError> {
         let mut variables = IndexMap::default();
-        for directive in directives.iter() {
-            let negated = match directive.name.as_str() {
-                "include" => false,
-                "skip" => true,
-                _ => continue,
+
+        if let Some(skip) = directives.get("skip") {
+            let Some(value) = skip.specified_argument_by_name("if") else {
+                internal_error!("missing @skip(if:) argument");
             };
-            let value = directive.specified_argument_by_name("if").ok_or_else(|| {
-                FederationError::internal(format!(
-                    "missing if argument on @{}",
-                    if negated { "skip" } else { "include" },
-                ))
-            })?;
-            match &**value {
-                Value::Boolean(false) if !negated => return Ok(Self::Boolean(false)),
-                Value::Boolean(true) if negated => return Ok(Self::Boolean(false)),
+
+            match value.as_ref() {
+                // Constant @skip(if: true) can never match
+                Value::Boolean(true) => return Ok(Self::Boolean(false)),
+                // Constant @skip(if: false) always matches
                 Value::Boolean(_) => {}
-                Value::Variable(name) => match variables.entry(name.clone()) {
-                    Entry::Occupied(entry) => {
-                        let previous_negated = *entry.get();
-                        if previous_negated != negated {
-                            return Ok(Self::Boolean(false));
-                        }
-                    }
-                    Entry::Vacant(entry) => {
-                        entry.insert(negated);
+                Value::Variable(name) => {
+                    variables.insert(name.clone(), ConditionKind::Skip);
+                }
+                _ => {
+                    internal_error!("expected boolean or variable `if` argument, got {value}");
+                }
+            }
+        }
+
+        if let Some(include) = directives.get("include") {
+            let Some(value) = include.specified_argument_by_name("if") else {
+                internal_error!("missing @include(if:) argument");
+            };
+
+            match value.as_ref() {
+                // Constant @include(if: false) can never match
+                Value::Boolean(false) => return Ok(Self::Boolean(false)),
+                // Constant @include(if: true) always matches
+                Value::Boolean(true) => {}
+                // If both @skip(if: $var) and @include(if: $var) exist, the condition can also
+                // never match
+                Value::Variable(name) => {
+                    if variables.insert(name.clone(), ConditionKind::Include)
+                        == Some(ConditionKind::Skip)
+                    {
+                        return Ok(Self::Boolean(false));
                     }
-                },
+                }
                 _ => {
-                    return Err(FederationError::internal(format!(
-                        "expected boolean or variable `if` argument, got {value}",
-                    )))
+                    internal_error!("expected boolean or variable `if` argument, got {value}");
                 }
             }
         }
+
         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(),
             (Conditions::Variables(new_conditions), Conditions::Variables(handled_conditions)) => {
                 let mut filtered = IndexMap::default();
-                for (cond_name, &cond_negated) in new_conditions.0.iter() {
-                    match handled_conditions.is_negated(cond_name) {
-                        Some(handled_cond) if cond_negated != handled_cond => {
+                for (cond_name, &cond_kind) in new_conditions.0.iter() {
+                    match handled_conditions.condition_kind(cond_name) {
+                        Some(handled_cond_kind) if cond_kind != handled_cond_kind => {
                             // 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);
                         }
                         Some(_) => {}
                         None => {
-                            filtered.insert(cond_name.clone(), cond_negated);
+                            filtered.insert(cond_name.clone(), cond_kind);
                         }
                     }
                 }
@@ -133,6 +192,8 @@ impl Conditions {
         }
     }
 
+    /// Merge two sets of conditions. The new conditions evaluate to true only if both input
+    /// conditions evaluate to true.
     pub(crate) fn merge(self, other: Self) -> Self {
         match (self, other) {
             // Absorbing element
@@ -143,22 +204,11 @@ impl Conditions {
             // Neutral element
             (Conditions::Boolean(true), x) | (x, Conditions::Boolean(true)) => x,
 
-            (Conditions::Variables(mut self_vars), Conditions::Variables(other_vars)) => {
-                let vars = Arc::make_mut(&mut self_vars.0);
-                for (name, other_negated) in other_vars.0.iter() {
-                    match vars.entry(name.clone()) {
-                        Entry::Occupied(entry) => {
-                            let self_negated = entry.get();
-                            if self_negated != other_negated {
-                                return Conditions::Boolean(false);
-                            }
-                        }
-                        Entry::Vacant(entry) => {
-                            entry.insert(*other_negated);
-                        }
-                    }
+            (Conditions::Variables(self_vars), Conditions::Variables(other_vars)) => {
+                match self_vars.merge(other_vars) {
+                    Some(vars) => Conditions::Variables(vars),
+                    None => Conditions::Boolean(false),
                 }
-                Conditions::Variables(self_vars)
             }
         }
     }
@@ -298,39 +348,21 @@ fn remove_conditions_of_element(
     }
 }
 
-#[derive(PartialEq)]
-enum ConditionKind {
-    Include,
-    Skip,
-}
-
 fn matches_condition_for_kind(
     directive: &Directive,
     conditions: &VariableConditions,
     kind: ConditionKind,
 ) -> bool {
-    let kind_str = match kind {
-        ConditionKind::Include => "include",
-        ConditionKind::Skip => "skip",
-    };
-
-    if directive.name != kind_str {
+    if directive.name != kind.as_str() {
         return false;
     }
 
-    let value = directive.specified_argument_by_name("if");
-
-    let matches_if_negated = match kind {
-        ConditionKind::Include => false,
-        ConditionKind::Skip => true,
-    };
-    match value {
-        None => false,
+    match directive.specified_argument_by_name("if") {
         Some(v) => match v.as_variable() {
-            Some(directive_var) => conditions.0.iter().any(|(cond_name, cond_is_negated)| {
-                cond_name == directive_var && *cond_is_negated == matches_if_negated
-            }),
+            Some(directive_var) => conditions.condition_kind(directive_var) == Some(kind),
             None => true,
         },
+        // Directive without argument: unreachable in a valid document.
+        None => false,
     }
 }

apollo-federation/src/query_plan/fetch_dependency_graph_processor.rs

@@ -5,6 +5,7 @@ use apollo_compiler::executable::VariableDefinition;
 use apollo_compiler::Name;
 use apollo_compiler::Node;
 
+use super::conditions::ConditionKind;
 use super::query_planner::SubgraphOperationCompression;
 use super::QueryPathElement;
 use crate::error::FederationError;
@@ -304,11 +305,10 @@ impl FetchDependencyGraphProcessor<Option<PlanNode>, DeferredDeferBlock>
                 condition.then_some(value)
             }
             Conditions::Variables(variables) => {
-                for (name, negated) in variables.iter() {
-                    let (if_clause, else_clause) = if negated {
-                        (None, Some(Box::new(value)))
-                    } else {
-                        (Some(Box::new(value)), None)
+                for (name, kind) in variables.iter() {
+                    let (if_clause, else_clause) = match kind {
+                        ConditionKind::Skip => (None, Some(Box::new(value))),
+                        ConditionKind::Include => (Some(Box::new(value)), None),
                     };
                     value = PlanNode::from(ConditionNode {
                         condition_variable: name.clone(),

apollo-federation/tests/query_plan/build_query_plan_tests.rs

@@ -1333,4 +1333,31 @@ fn condition_order_router799() {
     }
     "###
     );
+
+    // Reordering @skip/@include should produce the same plan.
+    assert_plan!(
+        &planner,
+        r#"
+          mutation($var0: Boolean! = true, $var1: Boolean!) {
+            ... on Mutation @include(if: $var1) @skip(if: $var0) {
+              field0: __typename
+            }
+          }
+        "#,
+        @r###"
+    QueryPlan {
+      Include(if: $var1) {
+        Skip(if: $var0) {
+          Fetch(service: "books") {
+            {
+              ... on Mutation {
+                field0: __typename
+              }
+            }
+          },
+        },
+      },
+    }
+    "###
+    );
 }