From 6b6d98cc292e480e2ca75a73ba9a351ddb5b07a0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9e=20Kooi?= Date: Fri, 22 Nov 2024 11:42:06 +0100 Subject: [PATCH 1/6] chore(federation): add unit tests for conditions handling --- .../src/query_plan/conditions.rs | 224 +++++++++++++++++- .../src/query_plan/fetch_dependency_graph.rs | 2 +- 2 files changed, 215 insertions(+), 11 deletions(-) diff --git a/apollo-federation/src/query_plan/conditions.rs b/apollo-federation/src/query_plan/conditions.rs index f202fd4058..df49993a35 100644 --- a/apollo-federation/src/query_plan/conditions.rs +++ b/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,6 +36,12 @@ impl ConditionKind { } } +impl Display for ConditionKind { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + self.as_str().fmt(f) + } +} + /// 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 @@ -46,6 +53,28 @@ pub(crate) enum Conditions { 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. @@ -107,12 +136,22 @@ impl Conditions { /// condition that always evaluates to true. fn from_variables(map: IndexMap) -> 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()); } } _ => { @@ -168,9 +207,9 @@ impl Conditions { } // 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(), + 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 +218,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 +237,7 @@ impl Conditions { match (self, other) { // Absorbing element (Conditions::Boolean(false), _) | (_, Conditions::Boolean(false)) => { - Conditions::Boolean(false) + Conditions::never() } // Neutral element @@ -207,7 +246,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 +405,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" + ); + } +} diff --git a/apollo-federation/src/query_plan/fetch_dependency_graph.rs b/apollo-federation/src/query_plan/fetch_dependency_graph.rs index 4a2da90e95..cac62cad27 100644 --- a/apollo-federation/src/query_plan/fetch_dependency_graph.rs +++ b/apollo-federation/src/query_plan/fetch_dependency_graph.rs @@ -1779,7 +1779,7 @@ 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( From 9857a742269825666edbcebc16c6dd2895158b8e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9e=20Kooi?= Date: Fri, 22 Nov 2024 11:44:18 +0100 Subject: [PATCH 2/6] document update_with --- .../src/query_plan/conditions.rs | 27 ++++++++++++++++--- 1 file changed, 24 insertions(+), 3 deletions(-) diff --git a/apollo-federation/src/query_plan/conditions.rs b/apollo-federation/src/query_plan/conditions.rs index df49993a35..3d3b033763 100644 --- a/apollo-federation/src/query_plan/conditions.rs +++ b/apollo-federation/src/query_plan/conditions.rs @@ -132,8 +132,9 @@ pub(crate) struct VariableCondition { } 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) -> Self { if map.is_empty() { Self::always() @@ -206,7 +207,27 @@ impl Conditions { Ok(Self::from_variables(variables)) } - // TODO(@goto-bus-stop): what exactly is the difference between this and `Self::merge`? + /// 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(), From 74d6e9bfa040fae6bf389c4646fb6c572f59c425 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9e=20Kooi?= Date: Fri, 22 Nov 2024 11:55:33 +0100 Subject: [PATCH 3/6] fmt docs for Conditions struct --- apollo-federation/src/query_plan/conditions.rs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/apollo-federation/src/query_plan/conditions.rs b/apollo-federation/src/query_plan/conditions.rs index 3d3b033763..d31f0ab682 100644 --- a/apollo-federation/src/query_plan/conditions.rs +++ b/apollo-federation/src/query_plan/conditions.rs @@ -42,11 +42,16 @@ impl Display for ConditionKind { } } -/// This struct is meant for tracking whether a selection set in a `FetchDependencyGraphNode` needs +/// 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), From fb7a1d47682abf0eaf53cd60dc44f74d0193e410 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9e=20Kooi?= Date: Fri, 22 Nov 2024 11:55:40 +0100 Subject: [PATCH 4/6] Remove unused struct --- apollo-federation/src/query_plan/conditions.rs | 6 ------ 1 file changed, 6 deletions(-) diff --git a/apollo-federation/src/query_plan/conditions.rs b/apollo-federation/src/query_plan/conditions.rs index d31f0ab682..c1827b461e 100644 --- a/apollo-federation/src/query_plan/conditions.rs +++ b/apollo-federation/src/query_plan/conditions.rs @@ -130,12 +130,6 @@ impl VariableConditions { } } -#[derive(Debug, Clone, PartialEq)] -pub(crate) struct VariableCondition { - variable: Name, - kind: ConditionKind, -} - impl Conditions { /// Create conditions from a map of variable conditions. /// From d01483f38fe58ad4eaa27f25679898ffb5871f35 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9e=20Kooi?= Date: Fri, 22 Nov 2024 11:55:51 +0100 Subject: [PATCH 5/6] fmt --- apollo-federation/src/query_plan/fetch_dependency_graph.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/apollo-federation/src/query_plan/fetch_dependency_graph.rs b/apollo-federation/src/query_plan/fetch_dependency_graph.rs index cac62cad27..91482c2495 100644 --- a/apollo-federation/src/query_plan/fetch_dependency_graph.rs +++ b/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 = node.selection_set.conditions.update_with(&handled_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( From aa9c8e5d85895b6f1f3382ac011c19b32abcc82f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9e=20Kooi?= Date: Fri, 22 Nov 2024 12:07:02 +0100 Subject: [PATCH 6/6] make the graphql syntax highlight properly in hover preview --- apollo-federation/src/query_plan/conditions.rs | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/apollo-federation/src/query_plan/conditions.rs b/apollo-federation/src/query_plan/conditions.rs index c1827b461e..9d61ab76ea 100644 --- a/apollo-federation/src/query_plan/conditions.rs +++ b/apollo-federation/src/query_plan/conditions.rs @@ -211,17 +211,21 @@ impl Conditions { /// /// For example, if we have a selection set like so: /// ```graphql - /// a @skip(if: $a) { - /// b @skip(if: $a) @include(if: $b) { - /// c + /// { + /// 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 + /// { + /// a @skip(if: $a) { + /// b @include(if: $b) { + /// c + /// } /// } /// } /// ```