From 871c3c759d9beb9a06c251c148c83ba48ad85203 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9e?= Date: Fri, 11 Oct 2024 01:34:04 +0100 Subject: [PATCH] fix(federation): correct order of @skip and @include conditions on the same selection (#6137) --- apollo-federation/src/operation/mod.rs | 6 + .../src/query_plan/conditions.rs | 186 ++++++++++-------- .../fetch_dependency_graph_processor.rs | 10 +- .../query_plan/build_query_plan_tests.rs | 27 +++ 4 files changed, 147 insertions(+), 82 deletions(-) diff --git a/apollo-federation/src/operation/mod.rs b/apollo-federation/src/operation/mod.rs index 43f88d5c51..23833a65e2 100644 --- a/apollo-federation/src/operation/mod.rs +++ b/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 { 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 { // 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. diff --git a/apollo-federation/src/query_plan/conditions.rs b/apollo-federation/src/query_plan/conditions.rs index a1d5d6f6a3..cf248c8b61 100644 --- a/apollo-federation/src/query_plan/conditions.rs +++ b/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,37 +50,62 @@ 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>); +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>, +); 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) -> Self { + fn new_unchecked(map: IndexMap) -> 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 { + /// 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 { self.0.get(name).copied() } - pub(crate) fn iter(&self) -> impl Iterator { - self.0.iter().map(|(name, &negated)| (name, negated)) + /// Iterate all variable conditions and their kinds. + pub(crate) fn iter(&self) -> impl Iterator { + 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 { + 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) -> Self { + fn from_variables(map: IndexMap) -> Self { if map.is_empty() { Self::Boolean(true) } else { @@ -70,53 +113,69 @@ impl Conditions { } } + /// 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 { 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. @@ -124,7 +183,7 @@ impl Conditions { } 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, } } diff --git a/apollo-federation/src/query_plan/fetch_dependency_graph_processor.rs b/apollo-federation/src/query_plan/fetch_dependency_graph_processor.rs index 37982b3ee2..0a3c6465d3 100644 --- a/apollo-federation/src/query_plan/fetch_dependency_graph_processor.rs +++ b/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, 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(), diff --git a/apollo-federation/tests/query_plan/build_query_plan_tests.rs b/apollo-federation/tests/query_plan/build_query_plan_tests.rs index b99cb1ba30..0e93f5a229 100644 --- a/apollo-federation/tests/query_plan/build_query_plan_tests.rs +++ b/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 + } + } + }, + }, + }, + } + "### + ); }