From a4c9da7d3f7afea4f1496a03091d4ff2d8bafafb Mon Sep 17 00:00:00 2001 From: Duckki Oe Date: Thu, 22 Aug 2024 19:33:57 -0700 Subject: [PATCH 1/2] feat: enable subgraph query validation - when `subgraph_graphql_validation` configuration option is true. --- apollo-federation/src/operation/mod.rs | 2 +- apollo-federation/src/query_plan/mod.rs | 119 ++++++++++++++++++ .../src/query_plan/query_planner.rs | 32 +++++ 3 files changed, 152 insertions(+), 1 deletion(-) diff --git a/apollo-federation/src/operation/mod.rs b/apollo-federation/src/operation/mod.rs index f0db930fb5..3144576ce1 100644 --- a/apollo-federation/src/operation/mod.rs +++ b/apollo-federation/src/operation/mod.rs @@ -3955,7 +3955,7 @@ impl TryFrom for Valid { document.fragments = fragments; document.operations.insert(operation); coerce_executable_values(value.schema.schema(), &mut document); - Ok(document.validate(value.schema.schema())?) + Ok(Valid::assume_valid(document)) } } diff --git a/apollo-federation/src/query_plan/mod.rs b/apollo-federation/src/query_plan/mod.rs index f620e208b3..5f425b148a 100644 --- a/apollo-federation/src/query_plan/mod.rs +++ b/apollo-federation/src/query_plan/mod.rs @@ -6,6 +6,7 @@ use apollo_compiler::ExecutableDocument; use apollo_compiler::Name; use serde::Serialize; +use crate::error::FederationError; use crate::query_plan::query_planner::QueryPlanningStatistics; pub(crate) mod conditions; @@ -258,3 +259,121 @@ impl QueryPlan { } } } + +mod plan_node_visitors { + use super::*; + + pub(super) fn fetch( + fetch: &FetchNode, + callback: &mut impl FnMut(&FetchNode) -> Result<(), FederationError>, + ) -> Result<(), FederationError> { + callback(fetch) + } + + pub(super) fn flatten( + flatten: &FlattenNode, + callback: &mut impl FnMut(&FetchNode) -> Result<(), FederationError>, + ) -> Result<(), FederationError> { + flatten.node.visit_fetch_nodes(callback) + } + + pub(super) fn sequence( + sequence: &SequenceNode, + callback: &mut impl FnMut(&FetchNode) -> Result<(), FederationError>, + ) -> Result<(), FederationError> { + sequence + .nodes + .iter() + .try_for_each(|node| node.visit_fetch_nodes(callback)) + } + + pub(super) fn parallel( + parallel: &ParallelNode, + callback: &mut impl FnMut(&FetchNode) -> Result<(), FederationError>, + ) -> Result<(), FederationError> { + parallel + .nodes + .iter() + .try_for_each(|node| node.visit_fetch_nodes(callback)) + } + + pub(super) fn condition( + condition: &ConditionNode, + callback: &mut impl FnMut(&FetchNode) -> Result<(), FederationError>, + ) -> Result<(), FederationError> { + let ConditionNode { + if_clause, + else_clause, + .. + } = condition; + if_clause + .iter() + .try_for_each(|node| node.visit_fetch_nodes(callback))?; + else_clause + .iter() + .try_for_each(|node| node.visit_fetch_nodes(callback))?; + Ok(()) + } + + pub(super) fn subscription( + subscription: &SubscriptionNode, + callback: &mut impl FnMut(&FetchNode) -> Result<(), FederationError>, + ) -> Result<(), FederationError> { + let SubscriptionNode { primary, rest } = subscription; + callback(primary)?; + rest.iter() + .try_for_each(|node| node.visit_fetch_nodes(callback))?; + Ok(()) + } + + pub(super) fn defer( + defer: &DeferNode, + callback: &mut impl FnMut(&FetchNode) -> Result<(), FederationError>, + ) -> Result<(), FederationError> { + let DeferNode { primary, deferred } = defer; + primary + .node + .iter() + .try_for_each(|node| node.visit_fetch_nodes(callback))?; + deferred + .iter() + .flat_map(|block| &block.node) + .try_for_each(|node| node.visit_fetch_nodes(callback))?; + Ok(()) + } +} + +impl PlanNode { + pub(crate) fn visit_fetch_nodes( + &self, + callback: &mut impl FnMut(&FetchNode) -> Result<(), FederationError>, + ) -> Result<(), FederationError> { + match self { + Self::Fetch(fetch) => plan_node_visitors::fetch(fetch, callback), + Self::Sequence(sequence) => plan_node_visitors::sequence(sequence, callback), + Self::Parallel(parallel) => plan_node_visitors::parallel(parallel, callback), + Self::Flatten(flatten) => plan_node_visitors::flatten(flatten, callback), + Self::Condition(condition) => plan_node_visitors::condition(condition, callback), + Self::Defer(defer) => plan_node_visitors::defer(defer, callback), + } + } +} + +impl TopLevelPlanNode { + pub(crate) fn visit_fetch_nodes( + &self, + callback: &mut impl FnMut(&FetchNode) -> Result<(), FederationError>, + ) -> Result<(), FederationError> { + match self { + Self::Fetch(fetch) => plan_node_visitors::fetch(fetch, callback), + Self::Sequence(sequence) => plan_node_visitors::sequence(sequence, callback), + Self::Parallel(parallel) => plan_node_visitors::parallel(parallel, callback), + Self::Flatten(flatten) => plan_node_visitors::flatten(flatten, callback), + Self::Condition(condition) => plan_node_visitors::condition(condition, callback), + Self::Defer(defer) => plan_node_visitors::defer(defer, callback), + Self::Subscription(subscription) => { + plan_node_visitors::subscription(subscription, callback) + } + } + } +} diff --git a/apollo-federation/src/query_plan/query_planner.rs b/apollo-federation/src/query_plan/query_planner.rs index 5670c685d9..75bb223799 100644 --- a/apollo-federation/src/query_plan/query_planner.rs +++ b/apollo-federation/src/query_plan/query_planner.rs @@ -525,6 +525,22 @@ impl QueryPlanner { None => None, }; + if self.config.subgraph_graphql_validation { + root_node.iter().try_for_each(|root_node| { + root_node.visit_fetch_nodes(&mut |node| { + self.validate_fetch_node(node).map_err(|err| { + let op_name = node + .operation_name + .as_ref() + .map_or_else(|| "(anonymous)".to_string(), |name| name.to_string()); + FederationError::internal(format!( + "Invalid fetch node operation: {op_name}\n{node}\n{err}", + )) + }) + }) + })?; + } + let plan = QueryPlan { node: root_node, statistics, @@ -540,6 +556,22 @@ impl QueryPlanner { &self.api_schema } + fn validate_fetch_node(&self, node: &FetchNode) -> Result<(), FederationError> { + let subgraph_schema = self + .subgraph_schemas() + .get(&node.subgraph_name) + .ok_or_else(|| { + FederationError::internal(format!( + "subgraph schema not found: {}", + node.subgraph_name + )) + })?; + // TODO: avoid cloning the operation document here + let document = node.operation_document.clone().into_inner(); + document.validate(subgraph_schema.schema())?; + Ok(()) + } + fn check_unsupported_features(supergraph: &Supergraph) -> Result<(), FederationError> { // We have a *progressive* override when `join__field` has a // non-null value for `overrideLabel` field. From c10afff30e6d90c57799d21ccb899d9c7691c8e6 Mon Sep 17 00:00:00 2001 From: Duckki Oe Date: Fri, 23 Aug 2024 09:00:26 -0700 Subject: [PATCH 2/2] addressed reviewer comment --- apollo-federation/src/operation/mod.rs | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/apollo-federation/src/operation/mod.rs b/apollo-federation/src/operation/mod.rs index 3144576ce1..cbf8598815 100644 --- a/apollo-federation/src/operation/mod.rs +++ b/apollo-federation/src/operation/mod.rs @@ -3934,7 +3934,7 @@ impl From<&FragmentSpreadSelection> for executable::FragmentSpread { } } -impl TryFrom for Valid { +impl TryFrom for executable::ExecutableDocument { type Error = FederationError; fn try_from(value: Operation) -> Result { @@ -3955,7 +3955,17 @@ impl TryFrom for Valid { document.fragments = fragments; document.operations.insert(operation); coerce_executable_values(value.schema.schema(), &mut document); - Ok(Valid::assume_valid(document)) + Ok(document) + } +} + +impl TryFrom for Valid { + type Error = FederationError; + + fn try_from(value: Operation) -> Result { + let schema = value.schema.clone(); + let document: executable::ExecutableDocument = value.try_into()?; + Ok(document.validate(schema.schema())?) } }