From 3b3d823c8d3629d31ad8951d5180e380ab98e205 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9e?= Date: Thu, 3 Oct 2024 15:08:40 +0100 Subject: [PATCH 1/3] fix(federation): do not generate fragments without type condition (#6111) --- apollo-federation/src/operation/optimize.rs | 14 ++++ .../fragment_autogeneration.rs | 47 ++++++++++++ .../supergraphs/same_as_js_router798.graphql | 73 +++++++++++++++++++ 3 files changed, 134 insertions(+) create mode 100644 apollo-federation/tests/query_plan/supergraphs/same_as_js_router798.graphql diff --git a/apollo-federation/src/operation/optimize.rs b/apollo-federation/src/operation/optimize.rs index e9e897304f..bde0b6dfa9 100644 --- a/apollo-federation/src/operation/optimize.rs +++ b/apollo-federation/src/operation/optimize.rs @@ -1684,6 +1684,20 @@ impl FragmentGenerator { SelectionValue::InlineFragment(mut candidate) => { self.visit_selection_set(candidate.get_selection_set_mut())?; + // XXX(@goto-bus-stop): This is temporary to support mismatch testing with JS! + // JS federation does not consider fragments without a type condition. + if candidate + .get() + .inline_fragment + .type_condition_position + .is_none() + { + new_selection_set.add_local_selection(&Selection::InlineFragment( + Arc::clone(candidate.get()), + ))?; + continue; + } + let directives = &candidate.get().inline_fragment.directives; let skip_include = directives .iter() diff --git a/apollo-federation/tests/query_plan/build_query_plan_tests/fragment_autogeneration.rs b/apollo-federation/tests/query_plan/build_query_plan_tests/fragment_autogeneration.rs index 876334fa5d..03b43c6245 100644 --- a/apollo-federation/tests/query_plan/build_query_plan_tests/fragment_autogeneration.rs +++ b/apollo-federation/tests/query_plan/build_query_plan_tests/fragment_autogeneration.rs @@ -177,6 +177,9 @@ fn it_handles_fragments_with_one_non_leaf_field() { ); } +/// XXX(@goto-bus-stop): this test is meant to check that fragments with @skip and @include *are* +/// migrated. But we are currently matching JS behavior, where they are not. This test should be +/// updated when we remove JS compatibility. #[test] fn it_migrates_skip_include() { let planner = planner!( @@ -348,6 +351,50 @@ fn fragments_that_share_a_hash_but_are_not_identical_generate_their_own_fragment ); } +#[test] +fn same_as_js_router798() { + let planner = planner!( + config = QueryPlannerConfig { generate_query_fragments: true, reuse_query_fragments: false, ..Default::default() }, + Subgraph1: r#" + interface Interface { a: Int } + type Y implements Interface { a: Int b: Int } + type Z implements Interface { a: Int c: Int } + + type Query { + interfaces(id: Int!): Interface + } + "#, + ); + assert_plan!( + &planner, + r#" + query($var0: Boolean! = true) { + ... @skip(if: $var0) { + field0: interfaces(id: 0) { + field1: __typename + } + } + } + "#, + @r###" + QueryPlan { + Skip(if: $var0) { + Fetch(service: "Subgraph1") { + { + ... { + field0: interfaces(id: 0) { + __typename + field1: __typename + } + } + } + }, + }, + } + "### + ); +} + #[test] fn works_with_key_chains() { let planner = planner!( diff --git a/apollo-federation/tests/query_plan/supergraphs/same_as_js_router798.graphql b/apollo-federation/tests/query_plan/supergraphs/same_as_js_router798.graphql new file mode 100644 index 0000000000..6895c4bf96 --- /dev/null +++ b/apollo-federation/tests/query_plan/supergraphs/same_as_js_router798.graphql @@ -0,0 +1,73 @@ +# Composed from subgraphs with hash: ee7fce9eb672edf9b036a25bcae0b056ccf5f451 +schema + @link(url: "https://specs.apollo.dev/link/v1.0") + @link(url: "https://specs.apollo.dev/join/v0.4", for: EXECUTION) +{ + query: Query +} + +directive @join__directive(graphs: [join__Graph!], name: String!, args: join__DirectiveArguments) repeatable on SCHEMA | OBJECT | INTERFACE | FIELD_DEFINITION + +directive @join__enumValue(graph: join__Graph!) repeatable on ENUM_VALUE + +directive @join__field(graph: join__Graph, requires: join__FieldSet, provides: join__FieldSet, type: String, external: Boolean, override: String, usedOverridden: Boolean, overrideLabel: String) repeatable on FIELD_DEFINITION | INPUT_FIELD_DEFINITION + +directive @join__graph(name: String!, url: String!) on ENUM_VALUE + +directive @join__implements(graph: join__Graph!, interface: String!) repeatable on OBJECT | INTERFACE + +directive @join__type(graph: join__Graph!, key: join__FieldSet, extension: Boolean! = false, resolvable: Boolean! = true, isInterfaceObject: Boolean! = false) repeatable on OBJECT | INTERFACE | UNION | ENUM | INPUT_OBJECT | SCALAR + +directive @join__unionMember(graph: join__Graph!, member: String!) repeatable on UNION + +directive @link(url: String, as: String, for: link__Purpose, import: [link__Import]) repeatable on SCHEMA + +interface Interface + @join__type(graph: SUBGRAPH1) +{ + a: Int +} + +scalar join__DirectiveArguments + +scalar join__FieldSet + +enum join__Graph { + SUBGRAPH1 @join__graph(name: "Subgraph1", url: "none") +} + +scalar link__Import + +enum link__Purpose { + """ + `SECURITY` features provide metadata necessary to securely resolve fields. + """ + SECURITY + + """ + `EXECUTION` features provide metadata necessary for operation execution. + """ + EXECUTION +} + +type Query + @join__type(graph: SUBGRAPH1) +{ + interfaces(id: Int!): Interface +} + +type Y implements Interface + @join__implements(graph: SUBGRAPH1, interface: "Interface") + @join__type(graph: SUBGRAPH1) +{ + a: Int + b: Int +} + +type Z implements Interface + @join__implements(graph: SUBGRAPH1, interface: "Interface") + @join__type(graph: SUBGRAPH1) +{ + a: Int + c: Int +} From d7329a93c01f9a5a276297928a0e85fc6cf8f4ef Mon Sep 17 00:00:00 2001 From: Duckki Oe Date: Thu, 3 Oct 2024 08:36:53 -0700 Subject: [PATCH 2/3] fix(dual-query-planner): use semantic diff when comparing Defer nodes (#6110) --- .../src/query_planner/dual_query_planner.rs | 49 ++++++++++++++----- 1 file changed, 37 insertions(+), 12 deletions(-) diff --git a/apollo-router/src/query_planner/dual_query_planner.rs b/apollo-router/src/query_planner/dual_query_planner.rs index c62fb621b2..8915253246 100644 --- a/apollo-router/src/query_planner/dual_query_planner.rs +++ b/apollo-router/src/query_planner/dual_query_planner.rs @@ -300,7 +300,12 @@ fn operation_matches( this: &SubgraphOperation, other: &SubgraphOperation, ) -> Result<(), MatchFailure> { - let this_ast = match ast::Document::parse(this.as_serialized(), "this_operation.graphql") { + document_str_matches(this.as_serialized(), other.as_serialized()) +} + +// Compare operation document strings such as query or just selection set. +fn document_str_matches(this: &str, other: &str) -> Result<(), MatchFailure> { + let this_ast = match ast::Document::parse(this, "this_operation.graphql") { Ok(document) => document, Err(_) => { return Err(MatchFailure::new( @@ -308,7 +313,7 @@ fn operation_matches( )); } }; - let other_ast = match ast::Document::parse(other.as_serialized(), "other_operation.graphql") { + let other_ast = match ast::Document::parse(other, "other_operation.graphql") { Ok(document) => document, Err(_) => { return Err(MatchFailure::new( @@ -319,6 +324,20 @@ fn operation_matches( same_ast_document(&this_ast, &other_ast) } +fn opt_document_string_matches( + this: &Option, + other: &Option, +) -> Result<(), MatchFailure> { + match (this, other) { + (None, None) => Ok(()), + (Some(this_sel), Some(other_sel)) => document_str_matches(this_sel, other_sel), + _ => Err(MatchFailure::new(format!( + "mismatched at opt_document_string_matches\nleft: {:?}\nright: {:?}", + this, other + ))), + } +} + // The rest is calling the comparison functions above instead of `PartialEq`, // but otherwise behave just like `PartialEq`: @@ -494,8 +513,8 @@ fn plan_node_matches(this: &PlanNode, other: &PlanNode) -> Result<(), MatchFailu deferred: other_deferred, }, ) => { - check_match!(defer_primary_node_matches(primary, other_primary)); - check_match!(vec_matches(deferred, other_deferred, deferred_node_matches)); + defer_primary_node_matches(primary, other_primary)?; + vec_matches_result(deferred, other_deferred, deferred_node_matches)?; } ( PlanNode::Subscription { primary, rest }, @@ -536,12 +555,15 @@ fn plan_node_matches(this: &PlanNode, other: &PlanNode) -> Result<(), MatchFailu Ok(()) } -fn defer_primary_node_matches(this: &Primary, other: &Primary) -> bool { +fn defer_primary_node_matches(this: &Primary, other: &Primary) -> Result<(), MatchFailure> { let Primary { subselection, node } = this; - *subselection == other.subselection && opt_plan_node_matches(node, &other.node).is_ok() + opt_document_string_matches(subselection, &other.subselection) + .map_err(|err| err.add_description("under defer primary subselection"))?; + opt_plan_node_matches(node, &other.node) + .map_err(|err| err.add_description("under defer primary plan node")) } -fn deferred_node_matches(this: &DeferredNode, other: &DeferredNode) -> bool { +fn deferred_node_matches(this: &DeferredNode, other: &DeferredNode) -> Result<(), MatchFailure> { let DeferredNode { depends, label, @@ -549,11 +571,14 @@ fn deferred_node_matches(this: &DeferredNode, other: &DeferredNode) -> bool { subselection, node, } = this; - *depends == other.depends - && *label == other.label - && *query_path == other.query_path - && *subselection == other.subselection - && opt_plan_node_matches(node, &other.node).is_ok() + + check_match_eq!(*depends, other.depends); + check_match_eq!(*label, other.label); + check_match_eq!(*query_path, other.query_path); + opt_document_string_matches(subselection, &other.subselection) + .map_err(|err| err.add_description("under deferred subselection"))?; + opt_plan_node_matches(node, &other.node) + .map_err(|err| err.add_description("under deferred node")) } fn flatten_node_matches(this: &FlattenNode, other: &FlattenNode) -> Result<(), MatchFailure> { From b90786dfa3923f3741664cfe69eb02c04ca809ac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9e?= Date: Thu, 3 Oct 2024 16:52:22 +0100 Subject: [PATCH 3/3] chore(federation): add macros for internal errors (#6080) --- apollo-federation/src/error/mod.rs | 31 ++++ apollo-federation/src/operation/merging.rs | 148 +++++++++----------- apollo-federation/src/operation/rebase.rs | 9 +- apollo-federation/src/operation/simplify.rs | 10 +- 4 files changed, 105 insertions(+), 93 deletions(-) diff --git a/apollo-federation/src/error/mod.rs b/apollo-federation/src/error/mod.rs index d56c9783a6..97b7d2757d 100644 --- a/apollo-federation/src/error/mod.rs +++ b/apollo-federation/src/error/mod.rs @@ -13,6 +13,37 @@ use lazy_static::lazy_static; use crate::subgraph::spec::FederationSpecError; +/// Break out of the current function, returning an internal error. +#[macro_export] +macro_rules! internal_error { + ( $( $arg:tt )+ ) => { + return Err($crate::error::FederationError::internal(format!( $( $arg )+ )).into()); + } +} + +/// A safe assertion: in debug mode, it panicks on failure, and in production, it returns an +/// internal error. +/// +/// Treat this as an assertion. It must only be used for conditions that *should never happen* +/// in normal operation. +#[macro_export] +macro_rules! ensure { + ( $expr:expr, $( $arg:tt )+ ) => { + #[cfg(debug_assertions)] + { + if false { + return Err($crate::error::FederationError::internal("ensure!() must be used in a function that returns a Result").into()); + } + assert!($expr, $( $arg )+); + } + + #[cfg(not(debug_assertions))] + if !$expr { + $crate::internal_error!( $( $arg )+ ); + } + } +} + // What we really needed here was the string representations in enum form, this isn't meant to // replace AST components. #[derive(Clone, Debug, strum_macros::Display)] diff --git a/apollo-federation/src/operation/merging.rs b/apollo-federation/src/operation/merging.rs index 4c2b31cbd3..c56ff5e66e 100644 --- a/apollo-federation/src/operation/merging.rs +++ b/apollo-federation/src/operation/merging.rs @@ -15,7 +15,9 @@ use super::NamedFragments; use super::Selection; use super::SelectionSet; use super::SelectionValue; +use crate::ensure; use crate::error::FederationError; +use crate::internal_error; impl<'a> FieldSelectionValue<'a> { /// Merges the given field selections into this one. @@ -36,31 +38,29 @@ impl<'a> FieldSelectionValue<'a> { let mut selection_sets = vec![]; for other in others { let other_field = &other.field; - if other_field.schema != self_field.schema { - return Err(FederationError::internal( - "Cannot merge field selections from different schemas", - )); - } - if other_field.field_position != self_field.field_position { - return Err(FederationError::internal(format!( - "Cannot merge field selection for field \"{}\" into a field selection for field \"{}\"", - other_field.field_position, - self_field.field_position, - ))); - } + ensure!( + other_field.schema == self_field.schema, + "Cannot merge field selections from different schemas", + ); + ensure!( + other_field.field_position == self_field.field_position, + "Cannot merge field selection for field \"{}\" into a field selection for field \"{}\"", + other_field.field_position, + self_field.field_position, + ); if self.get().selection_set.is_some() { let Some(other_selection_set) = &other.selection_set else { - return Err(FederationError::internal(format!( + internal_error!( "Field \"{}\" has composite type but not a selection set", other_field.field_position, - ))); + ); }; selection_sets.push(other_selection_set); } else if other.selection_set.is_some() { - return Err(FederationError::internal(format!( + internal_error!( "Field \"{}\" has non-composite type but also has a selection set", other_field.field_position, - ))); + ); } } if let Some(self_selection_set) = self.get_selection_set_mut() { @@ -87,22 +87,16 @@ impl<'a> InlineFragmentSelectionValue<'a> { let mut selection_sets = vec![]; for other in others { let other_inline_fragment = &other.inline_fragment; - if other_inline_fragment.schema != self_inline_fragment.schema { - return Err(FederationError::internal( - "Cannot merge inline fragment from different schemas", - )); - } - if other_inline_fragment.parent_type_position - != self_inline_fragment.parent_type_position - { - return Err(FederationError::internal( - format!( - "Cannot merge inline fragment of parent type \"{}\" into an inline fragment of parent type \"{}\"", - other_inline_fragment.parent_type_position, - self_inline_fragment.parent_type_position, - ), - )); - } + ensure!( + other_inline_fragment.schema == self_inline_fragment.schema, + "Cannot merge inline fragment from different schemas", + ); + ensure!( + other_inline_fragment.parent_type_position == self_inline_fragment.parent_type_position, + "Cannot merge inline fragment of parent type \"{}\" into an inline fragment of parent type \"{}\"", + other_inline_fragment.parent_type_position, + self_inline_fragment.parent_type_position, + ); selection_sets.push(&other.selection_set); } self.get_selection_set_mut() @@ -127,11 +121,10 @@ impl<'a> FragmentSpreadSelectionValue<'a> { let self_fragment_spread = &self.get().spread; for other in others { let other_fragment_spread = &other.spread; - if other_fragment_spread.schema != self_fragment_spread.schema { - return Err(FederationError::internal( - "Cannot merge fragment spread from different schemas", - )); - } + ensure!( + other_fragment_spread.schema == self_fragment_spread.schema, + "Cannot merge fragment spread from different schemas", + ); // Nothing to do since the fragment spread is already part of the selection set. // Fragment spreads are uniquely identified by fragment name and applied directives. // Since there is already an entry for the same fragment spread, there is no point @@ -157,20 +150,16 @@ impl SelectionSet { ) -> Result<(), FederationError> { let mut selections_to_merge = vec![]; for other in others { - if other.schema != self.schema { - return Err(FederationError::internal( - "Cannot merge selection sets from different schemas", - )); - } - if other.type_position != self.type_position { - return Err(FederationError::internal( - format!( - "Cannot merge selection set for type \"{}\" into a selection set for type \"{}\"", - other.type_position, - self.type_position, - ), - )); - } + ensure!( + other.schema == self.schema, + "Cannot merge selection sets from different schemas", + ); + ensure!( + other.type_position == self.type_position, + "Cannot merge selection set for type \"{}\" into a selection set for type \"{}\"", + other.type_position, + self.type_position, + ); selections_to_merge.extend(other.selections.values()); } self.merge_selections_into(selections_to_merge.into_iter()) @@ -198,12 +187,10 @@ impl SelectionSet { selection_map::Entry::Occupied(existing) => match existing.get() { Selection::Field(self_field_selection) => { let Selection::Field(other_field_selection) = other_selection else { - return Err(FederationError::internal( - format!( - "Field selection key for field \"{}\" references non-field selection", - self_field_selection.field.field_position, - ), - )); + internal_error!( + "Field selection key for field \"{}\" references non-field selection", + self_field_selection.field.field_position, + ); }; fields .entry(other_key) @@ -214,12 +201,10 @@ impl SelectionSet { let Selection::FragmentSpread(other_fragment_spread_selection) = other_selection else { - return Err(FederationError::internal( - format!( - "Fragment spread selection key for fragment \"{}\" references non-field selection", - self_fragment_spread_selection.spread.fragment_name, - ), - )); + internal_error!( + "Fragment spread selection key for fragment \"{}\" references non-field selection", + self_fragment_spread_selection.spread.fragment_name, + ); }; fragment_spreads .entry(other_key) @@ -230,17 +215,15 @@ impl SelectionSet { let Selection::InlineFragment(other_inline_fragment_selection) = other_selection else { - return Err(FederationError::internal( - format!( - "Inline fragment selection key under parent type \"{}\" {}references non-field selection", - self_inline_fragment_selection.inline_fragment.parent_type_position, - self_inline_fragment_selection.inline_fragment.type_condition_position.clone() - .map_or_else( - String::new, - |cond| format!("(type condition: {}) ", cond), - ), - ), - )); + internal_error!( + "Inline fragment selection key under parent type \"{}\" {}references non-field selection", + self_inline_fragment_selection.inline_fragment.parent_type_position, + self_inline_fragment_selection.inline_fragment.type_condition_position.clone() + .map_or_else( + String::new, + |cond| format!("(type condition: {}) ", cond), + ), + ); }; inline_fragments .entry(other_key) @@ -306,9 +289,8 @@ impl SelectionSet { &mut self, selection: &Selection, ) -> Result<(), FederationError> { - debug_assert_eq!( - &self.schema, - selection.schema(), + ensure!( + self.schema == *selection.schema(), "In order to add selection it needs to point to the same schema" ); self.merge_selections_into(std::iter::once(selection)) @@ -328,12 +310,12 @@ impl SelectionSet { &mut self, selection_set: &SelectionSet, ) -> Result<(), FederationError> { - debug_assert_eq!( - self.schema, selection_set.schema, + ensure!( + self.schema == selection_set.schema, "In order to add selection set it needs to point to the same schema." ); - debug_assert_eq!( - self.type_position, selection_set.type_position, + ensure!( + self.type_position == selection_set.type_position, "In order to add selection set it needs to point to the same type position" ); self.merge_into(std::iter::once(selection_set)) @@ -386,9 +368,7 @@ pub(crate) fn merge_selection_sets( mut selection_sets: Vec, ) -> Result { let Some((first, remainder)) = selection_sets.split_first_mut() else { - return Err(FederationError::internal( - "merge_selection_sets(): must have at least one selection set", - )); + internal_error!("merge_selection_sets(): must have at least one selection set"); }; first.merge_into(remainder.iter())?; diff --git a/apollo-federation/src/operation/rebase.rs b/apollo-federation/src/operation/rebase.rs index 25ac19ec5a..5e2d29c75d 100644 --- a/apollo-federation/src/operation/rebase.rs +++ b/apollo-federation/src/operation/rebase.rs @@ -22,6 +22,7 @@ use super::Selection; use super::SelectionId; use super::SelectionSet; use super::TYPENAME_FIELD; +use crate::ensure; use crate::error::FederationError; use crate::schema::position::CompositeTypeDefinitionPosition; use crate::schema::position::OutputTypeDefinitionPosition; @@ -376,12 +377,12 @@ impl FragmentSpread { } .into()); }; - debug_assert_eq!( - *schema, self.schema, + ensure!( + *schema == self.schema, "Fragment spread should only be rebased within the same subgraph" ); - debug_assert_eq!( - *schema, named_fragment.schema, + ensure!( + *schema == named_fragment.schema, "Referenced named fragment should've been rebased for the subgraph" ); if runtime_types_intersect( diff --git a/apollo-federation/src/operation/simplify.rs b/apollo-federation/src/operation/simplify.rs index 6ada508a6e..f7c339d210 100644 --- a/apollo-federation/src/operation/simplify.rs +++ b/apollo-federation/src/operation/simplify.rs @@ -14,6 +14,7 @@ use super::NamedFragments; use super::Selection; use super::SelectionMap; use super::SelectionSet; +use crate::ensure; use crate::error::FederationError; use crate::schema::position::CompositeTypeDefinitionPosition; use crate::schema::ValidFederationSchema; @@ -136,11 +137,10 @@ impl FragmentSpreadSelection { // We must update the spread parent type if necessary since we're not going deeper, // or we'll be fundamentally losing context. - if self.spread.schema != *schema { - return Err(FederationError::internal( - "Should not try to flatten_unnecessary_fragments using a type from another schema", - )); - } + ensure!( + self.spread.schema == *schema, + "Should not try to flatten_unnecessary_fragments using a type from another schema", + ); let rebased_fragment_spread = self.rebase_on(parent_type, named_fragments, schema)?; Ok(Some(SelectionOrSet::Selection(rebased_fragment_spread)))