diff --git a/apollo-federation/src/query_graph/build_query_graph.rs b/apollo-federation/src/query_graph/build_query_graph.rs index 27c17a3a57..bfd39e2c62 100644 --- a/apollo-federation/src/query_graph/build_query_graph.rs +++ b/apollo-federation/src/query_graph/build_query_graph.rs @@ -2009,6 +2009,7 @@ struct FederatedQueryGraphBuilderSubgraphData { interface_object_directive_definition_name: Name, } +#[derive(Debug)] struct QueryGraphEdgeData { head: NodeIndex, tail: NodeIndex, diff --git a/apollo-federation/src/query_graph/graph_path.rs b/apollo-federation/src/query_graph/graph_path.rs index 9f44d3b847..8127b3dfc8 100644 --- a/apollo-federation/src/query_graph/graph_path.rs +++ b/apollo-federation/src/query_graph/graph_path.rs @@ -794,10 +794,12 @@ impl std::fmt::Display for ClosedPath { /// A list of the options generated during query planning for a specific "closed branch", which is a /// full/closed path in a GraphQL operation (i.e. one that ends in a leaf field). +#[derive(Debug)] pub(crate) struct ClosedBranch(pub(crate) Vec>); /// A list of the options generated during query planning for a specific "open branch", which is a /// partial/open path in a GraphQL operation (i.e. one that does not end in a leaf field). +#[derive(Debug)] pub(crate) struct OpenBranch(pub(crate) Vec); impl GraphPath @@ -1169,6 +1171,7 @@ where return Ok(Box::new( self.graph .out_edges_with_federation_self_edges(self.tail) + .into_iter() .map(|edge_ref| edge_ref.id()), )); } @@ -1194,6 +1197,7 @@ where Ok(Box::new( self.graph .out_edges(self.tail) + .into_iter() .map(|edge_ref| edge_ref.id()), )) } @@ -1988,14 +1992,11 @@ impl OpGraphPath { else { return Ok(path); }; - let typename_field = Field::new(FieldData { - schema: self.graph.schema_by_source(&tail_weight.source)?.clone(), - field_position: tail_type_pos.introspection_typename_field(), - alias: None, - arguments: Arc::new(vec![]), - directives: Arc::new(Default::default()), - sibling_typename: None, - }); + let typename_field = Field::new_introspection_typename( + self.graph.schema_by_source(&tail_weight.source)?, + &tail_type_pos, + None, + ); let Some(edge) = self.graph.edge_for_field(path.tail, &typename_field) else { return Err(FederationError::internal( "Unexpectedly missing edge for __typename field", @@ -3438,9 +3439,7 @@ impl ClosedBranch { } // Keep track of which options should be kept. - let mut keep_options = std::iter::repeat_with(|| true) - .take(self.0.len()) - .collect::>(); + let mut keep_options = vec![true; self.0.len()]; for option_index in 0..(self.0.len()) { if !keep_options[option_index] { continue; diff --git a/apollo-federation/src/query_graph/mod.rs b/apollo-federation/src/query_graph/mod.rs index cc294e0337..dd29e5acff 100644 --- a/apollo-federation/src/query_graph/mod.rs +++ b/apollo-federation/src/query_graph/mod.rs @@ -458,26 +458,42 @@ impl QueryGraph { pub(crate) fn out_edges_with_federation_self_edges( &self, node: NodeIndex, - ) -> impl Iterator> { - self.graph.edges_directed(node, Direction::Outgoing) + ) -> Vec> { + Self::sorted_edges(self.graph.edges_directed(node, Direction::Outgoing)) } /// The outward edges from the given node, minus self-key and self-root-type-resolution edges, /// as they're rarely useful (currently only used by `@defer`). - pub(crate) fn out_edges( - &self, - node: NodeIndex, - ) -> impl Iterator> { - self.graph - .edges_directed(node, Direction::Outgoing) - .filter(|edge_ref| { + pub(crate) fn out_edges(&self, node: NodeIndex) -> Vec> { + Self::sorted_edges(self.graph.edges_directed(node, Direction::Outgoing).filter( + |edge_ref| { !(edge_ref.source() == edge_ref.target() && matches!( edge_ref.weight().transition, QueryGraphEdgeTransition::KeyResolution | QueryGraphEdgeTransition::RootTypeResolution { .. } )) - }) + }, + )) + } + + /// Edge iteration order is unspecified in petgraph, but appears to be + /// *reverse* insertion order in practice. + /// This can affect generated query plans, such as when two options have the same cost. + /// To match the JS code base, we want to iterate in insertion order. + /// + /// Sorting by edge indices relies on documented behavior: + /// + /// + /// As of this writing, edges of the query graph are removed + /// in `FederatedQueryGraphBuilder::update_edge_tail` which specifically preserves indices + /// by pairing with an insertion. + fn sorted_edges<'graph>( + edges: impl Iterator>, + ) -> Vec> { + let mut edges: Vec<_> = edges.collect(); + edges.sort_by_key(|e| -> EdgeIndex { e.id() }); + edges } pub(crate) fn is_self_key_or_root_edge( @@ -538,7 +554,7 @@ impl QueryGraph { } pub(crate) fn edge_for_field(&self, node: NodeIndex, field: &Field) -> Option { - let mut candidates = self.out_edges(node).filter_map(|edge_ref| { + let mut candidates = self.out_edges(node).into_iter().filter_map(|edge_ref| { let edge_weight = edge_ref.weight(); let QueryGraphEdgeTransition::FieldCollection { field_definition_position, @@ -578,7 +594,7 @@ impl QueryGraph { // No type condition means the type hasn't changed, meaning there is no edge to take. return None; }; - let mut candidates = self.out_edges(node).filter_map(|edge_ref| { + let mut candidates = self.out_edges(node).into_iter().filter_map(|edge_ref| { let edge_weight = edge_ref.weight(); let QueryGraphEdgeTransition::Downcast { to_type_position, .. diff --git a/apollo-federation/src/query_graph/path_tree.rs b/apollo-federation/src/query_graph/path_tree.rs index 27796e67ee..2cbe28bc6d 100644 --- a/apollo-federation/src/query_graph/path_tree.rs +++ b/apollo-federation/src/query_graph/path_tree.rs @@ -24,7 +24,7 @@ use crate::query_plan::operation::SelectionSet; // Typescript doesn't have a native way of associating equality/hash functions with types, so they // were passed around manually. This isn't the case with Rust, where we instead implement trigger // equality via `PartialEq` and `Hash`. -#[derive(Debug, Clone)] +#[derive(Clone)] pub(crate) struct PathTree where TTrigger: Eq + Hash, @@ -361,6 +361,27 @@ fn merge_conditions( } } +impl std::fmt::Debug + for PathTree +where + TTrigger: Eq + Hash, + TEdge: Copy + Into>, +{ + fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { + let Self { + graph: _, // skip + node, + local_selection_sets, + childs, + } = self; + f.debug_struct("PathTree") + .field("node", node) + .field("local_selection_sets", local_selection_sets) + .field("childs", childs) + .finish() + } +} + #[cfg(test)] mod tests { use std::sync::Arc; @@ -418,6 +439,7 @@ mod tests { // find the edge that matches `field_name` let (edge_ref, field_def) = query_graph .out_edges(curr_node_idx) + .into_iter() .find_map(|e_ref| { let edge = e_ref.weight(); match &edge.transition { diff --git a/apollo-federation/src/query_plan/display.rs b/apollo-federation/src/query_plan/display.rs index a0c1134e9b..da81fd057d 100644 --- a/apollo-federation/src/query_plan/display.rs +++ b/apollo-federation/src/query_plan/display.rs @@ -57,17 +57,17 @@ impl SubscriptionNode { state.write("Primary: {")?; primary.write_indented(state)?; - state.write("}")?; + state.write("},")?; if let Some(rest) = rest { state.new_line()?; state.write("Rest: {")?; rest.write_indented(state)?; - state.write("}")?; + state.write("},")?; } state.dedent()?; - state.write("}") + state.write("},") } } @@ -94,13 +94,14 @@ impl FetchNode { if let Some(v) = requires.as_ref() { if !v.is_empty() { write_selections(state, v)?; - state.write(" => ")?; + state.write(" =>")?; + state.new_line()?; } } write_operation(state, operation_document)?; state.dedent()?; - state.write("}") + state.write("},") } } @@ -111,7 +112,7 @@ impl SequenceNode { write_indented_lines(state, nodes, |state, node| node.write_indented(state))?; - state.write("}") + state.write("},") } } @@ -122,7 +123,7 @@ impl ParallelNode { write_indented_lines(state, nodes, |state, node| node.write_indented(state))?; - state.write("}") + state.write("},") } } @@ -143,7 +144,7 @@ impl FlattenNode { node.write_indented(state)?; state.dedent()?; - state.write("}") + state.write("},") } } @@ -163,16 +164,16 @@ impl ConditionNode { state.indent()?; if_clause.write_indented(state)?; state.dedent()?; - state.write("}")?; + state.write("},")?; state.write("Else {")?; state.indent()?; else_clause.write_indented(state)?; state.dedent()?; - state.write("}")?; + state.write("},")?; state.dedent()?; - state.write("}") + state.write("},") } (Some(if_clause), None) => { @@ -182,7 +183,7 @@ impl ConditionNode { if_clause.write_indented(state)?; state.dedent()?; - state.write("}") + state.write("},") } (None, Some(else_clause)) => { @@ -192,7 +193,7 @@ impl ConditionNode { else_clause.write_indented(state)?; state.dedent()?; - state.write("}") + state.write("},") } // Shouldn’t happen? @@ -217,7 +218,7 @@ impl DeferNode { } state.dedent()?; - state.write("}") + state.write("},") } } @@ -248,7 +249,7 @@ impl PrimaryDeferBlock { state.dedent()?; } - state.write("}") + state.write("},") } } @@ -296,7 +297,7 @@ impl DeferredDeferBlock { state.dedent()?; } - state.write("}") + state.write("},") } } diff --git a/apollo-federation/src/query_plan/fetch_dependency_graph.rs b/apollo-federation/src/query_plan/fetch_dependency_graph.rs index c4e93e6bfd..4710c26ad2 100644 --- a/apollo-federation/src/query_plan/fetch_dependency_graph.rs +++ b/apollo-federation/src/query_plan/fetch_dependency_graph.rs @@ -2016,7 +2016,7 @@ fn compute_nodes_for_key_resolution<'a>( // (and so no one subgraph has a type definition with all the proper fields, // only the supergraph does). let input_type = dependency_graph.type_for_fetch_inputs(source_type.type_name())?; - let mut input_selections = SelectionSet::empty( + let mut input_selections = SelectionSet::for_composite_type( dependency_graph.supergraph_schema.clone(), input_type.clone(), ); @@ -2057,14 +2057,11 @@ fn compute_nodes_for_key_resolution<'a>( // We also ensure to get the __typename of the current type in the "original" node. let node = FetchDependencyGraph::node_weight_mut(&mut dependency_graph.graph, stack_item.node_id)?; - let typename_field = Arc::new(OpPathElement::Field(Field::new(FieldData { - schema: dependency_graph.supergraph_schema.clone(), - field_position: source_type.introspection_typename_field(), - alias: None, - arguments: Default::default(), - directives: Default::default(), - sibling_typename: None, - }))); + let typename_field = Arc::new(OpPathElement::Field(Field::new_introspection_typename( + &dependency_graph.supergraph_schema, + &source_type, + None, + ))); let typename_path = stack_item .node_path .path_in_node @@ -2131,14 +2128,11 @@ fn compute_nodes_for_root_type_resolution<'a>( let node = FetchDependencyGraph::node_weight_mut(&mut dependency_graph.graph, stack_item.node_id)?; if !stack_item.node_path.path_in_node.is_empty() { - let typename_field = Arc::new(OpPathElement::Field(Field::new(FieldData { - schema: dependency_graph.supergraph_schema.clone(), - field_position: source_type.introspection_typename_field().into(), - alias: None, - arguments: Default::default(), - directives: Default::default(), - sibling_typename: None, - }))); + let typename_field = Arc::new(OpPathElement::Field(Field::new_introspection_typename( + &dependency_graph.supergraph_schema, + &source_type.into(), + None, + ))); let typename_path = stack_item .node_path .path_in_node @@ -2239,16 +2233,11 @@ fn compute_nodes_for_op_path_element<'a>( } else { Some(name.clone()) }; - let typename_field = Arc::new(OpPathElement::Field(Field::new(FieldData { - schema: dependency_graph.supergraph_schema.clone(), - field_position: operation - .parent_type_position() - .introspection_typename_field(), + let typename_field = Arc::new(OpPathElement::Field(Field::new_introspection_typename( + &dependency_graph.supergraph_schema, + &operation.parent_type_position(), alias, - arguments: Default::default(), - directives: Default::default(), - sibling_typename: None, - }))); + ))); let typename_path = stack_item .node_path .path_in_node diff --git a/apollo-federation/src/query_plan/generate.rs b/apollo-federation/src/query_plan/generate.rs index 2d3e8a6249..68d2d798df 100644 --- a/apollo-federation/src/query_plan/generate.rs +++ b/apollo-federation/src/query_plan/generate.rs @@ -107,6 +107,7 @@ where Element: Clone, // Uncomment to use `dbg!()` // Element: std::fmt::Debug, + // Plan: std::fmt::Debug, { if to_add.is_empty() { let cost = plan_builder.compute_plan_cost(&mut initial)?; diff --git a/apollo-federation/src/query_plan/operation.rs b/apollo-federation/src/query_plan/operation.rs index 8dd7b1bbc4..02ce57356e 100644 --- a/apollo-federation/src/query_plan/operation.rs +++ b/apollo-federation/src/query_plan/operation.rs @@ -694,11 +694,7 @@ pub(crate) enum Selection { impl Selection { pub(crate) fn from_field(field: Field, sub_selections: Option) -> Self { - let field_selection = FieldSelection { - field, - selection_set: sub_selections, - }; - Self::Field(Arc::new(field_selection)) + Self::Field(Arc::new(field.with_subselection(sub_selections))) } pub(crate) fn from_inline_fragment( @@ -1067,6 +1063,7 @@ mod normalized_field_selection { use crate::query_plan::operation::SelectionKey; use crate::query_plan::operation::SelectionSet; use crate::query_plan::FetchDataPathElement; + use crate::schema::position::CompositeTypeDefinitionPosition; use crate::schema::position::FieldDefinitionPosition; use crate::schema::position::TypeDefinitionPosition; use crate::schema::ValidFederationSchema; @@ -1127,12 +1124,18 @@ mod normalized_field_selection { /// The non-selection-set data of `FieldSelection`, used with operation paths and graph /// paths. - #[derive(Debug, Clone, PartialEq, Eq, Hash)] + #[derive(Clone, PartialEq, Eq, Hash)] pub(crate) struct Field { data: FieldData, key: SelectionKey, } + impl std::fmt::Debug for Field { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + self.data.fmt(f) + } + } + impl Field { pub(crate) fn new(data: FieldData) -> Self { Self { @@ -1149,6 +1152,21 @@ mod normalized_field_selection { Self::new(FieldData::from_position(schema, field_position)) } + pub(crate) fn new_introspection_typename( + schema: &ValidFederationSchema, + parent_type: &CompositeTypeDefinitionPosition, + alias: Option, + ) -> Self { + Self::new(FieldData { + schema: schema.clone(), + field_position: parent_type.introspection_typename_field(), + alias, + arguments: Default::default(), + directives: Default::default(), + sibling_typename: None, + }) + } + /// Turn this `Field` into a `FieldSelection` with the given sub-selection. If this is /// meant to be a leaf selection, use `None`. pub(crate) fn with_subselection( @@ -1320,12 +1338,18 @@ mod normalized_fragment_spread_selection { /// An analogue of the apollo-compiler type `FragmentSpread` with these changes: /// - Stores the schema (may be useful for directives). /// - Encloses collection types in `Arc`s to facilitate cheaper cloning. - #[derive(Debug, Clone, PartialEq, Eq)] + #[derive(Clone, PartialEq, Eq)] pub(crate) struct FragmentSpread { data: FragmentSpreadData, key: SelectionKey, } + impl std::fmt::Debug for FragmentSpread { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + self.data.fmt(f) + } + } + impl FragmentSpread { pub(crate) fn new(data: FragmentSpreadData) -> Self { Self { @@ -1653,12 +1677,18 @@ mod normalized_inline_fragment_selection { /// The non-selection-set data of `InlineFragmentSelection`, used with operation paths and /// graph paths. - #[derive(Debug, Clone, PartialEq, Eq, Hash)] + #[derive(Clone, PartialEq, Eq, Hash)] pub(crate) struct InlineFragment { data: InlineFragmentData, key: SelectionKey, } + impl std::fmt::Debug for InlineFragment { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + self.data.fmt(f) + } + } + impl InlineFragment { pub(crate) fn new(data: InlineFragmentData) -> Self { Self { @@ -1800,6 +1830,8 @@ pub(crate) use normalized_inline_fragment_selection::InlineFragment; pub(crate) use normalized_inline_fragment_selection::InlineFragmentData; pub(crate) use normalized_inline_fragment_selection::InlineFragmentSelection; +use crate::schema::position::INTROSPECTION_TYPENAME_FIELD_NAME; + // TODO(@goto-bus-stop): merge this with the other Operation impl block. impl Operation { pub(crate) fn optimize( @@ -1881,6 +1913,20 @@ impl SelectionSet { } } + /// PORT_NOTE: JS calls this `newCompositeTypeSelectionSet` + pub(crate) fn for_composite_type( + schema: ValidFederationSchema, + type_position: CompositeTypeDefinitionPosition, + ) -> Self { + let typename_field = Field::new_introspection_typename(&schema, &type_position, None) + .with_subselection(None); + Self { + schema, + type_position, + selections: Arc::new(std::iter::once(typename_field).collect()), + } + } + /// Build a selection set from a single selection. pub(crate) fn from_selection( type_position: CompositeTypeDefinitionPosition, @@ -2581,18 +2627,11 @@ impl SelectionSet { } else { Some(sibling_typename.clone()) }; - let field_position = selection - .element()? - .parent_type_position() - .introspection_typename_field(); - let field_element = Field::new(FieldData { - schema: self.schema.clone(), - field_position, + let field_element = Field::new_introspection_typename( + &self.schema, + &selection.element()?.parent_type_position(), alias, - arguments: Default::default(), - directives: Default::default(), - sibling_typename: None, - }); + ); let typename_selection = Selection::from_element(field_element.into(), /*subselection*/ None)?; Ok([typename_selection, updated].into_iter().collect()) @@ -2607,9 +2646,8 @@ impl SelectionSet { let mut selection_map = SelectionMap::new(); if let Some(parent) = parent_type_if_abstract { if !self.has_top_level_typename_field() { - let field_position = parent.introspection_typename_field(); let typename_selection = Selection::from_field( - Field::new(FieldData::from_position(&self.schema, field_position)), + Field::new_introspection_typename(&self.schema, &parent.into(), None), None, ); selection_map.insert(typename_selection); @@ -4768,6 +4806,15 @@ impl TryFrom<&SelectionSet> for executable::SelectionSet { let mut flattened = vec![]; for normalized_selection in val.selections.values() { let selection: executable::Selection = normalized_selection.try_into()?; + if let executable::Selection::Field(field) = &selection { + if field.name == *INTROSPECTION_TYPENAME_FIELD_NAME && field.alias.is_none() { + // Move unaliased __typename to the start of the selection set. + // This looks nicer, and matches existing tests. + // PORT_NOTE: JS does this in `selectionsInPrintOrder` + flattened.insert(0, selection); + continue; + } + } flattened.push(selection); } Ok(Self { @@ -4989,11 +5036,7 @@ impl Display for Field { // serializing it when empty. Note we're implicitly relying on the lack of type-checking // in both `FieldSelection` and `Field` display logic (specifically, we rely on // them not checking whether it is valid for the selection set to be empty). - let selection = FieldSelection { - field: self.clone(), - selection_set: None, - }; - selection.fmt(f) + self.clone().with_subselection(None).fmt(f) } } @@ -7186,15 +7229,8 @@ type T { let parent_type_pos = s.element()?.parent_type_position(); // "__typename" field - let field_position = parent_type_pos.introspection_typename_field(); - let field_element = Field::new(FieldData { - schema: s.schema().clone(), - field_position, - alias: None, - arguments: Default::default(), - directives: Default::default(), - sibling_typename: None, - }); + let field_element = + Field::new_introspection_typename(s.schema(), &parent_type_pos, None); let typename_selection = Selection::from_element(field_element.into(), /*subselection*/ None)?; // return `updated` and `typename_selection` diff --git a/apollo-federation/src/query_plan/query_planner.rs b/apollo-federation/src/query_plan/query_planner.rs index c25ef5a869..55d6f225c7 100644 --- a/apollo-federation/src/query_plan/query_planner.rs +++ b/apollo-federation/src/query_plan/query_planner.rs @@ -753,7 +753,7 @@ type User email } } - } + }, } "###); } @@ -784,81 +784,89 @@ type User { bestRatedProducts { ... on Book { - id __typename + id } ... on Movie { - id __typename + id } } } - } + }, Parallel { Sequence { Flatten(path: "bestRatedProducts.*") { Fetch(service: "products") { { ... on Movie { + __typename id } - } => { + } => + { ... on Movie { vendor { - id __typename + id } } } - } - } + }, + }, Flatten(path: "bestRatedProducts.*.vendor") { Fetch(service: "accounts") { { ... on User { + __typename id } - } => { + } => + { ... on User { name } } - } - } - } + }, + }, + }, Sequence { Flatten(path: "bestRatedProducts.*") { Fetch(service: "products") { { ... on Book { + __typename id } - } => { + } => + { ... on Book { vendor { - id __typename + id } } } - } - } + }, + }, Flatten(path: "bestRatedProducts.*.vendor") { Fetch(service: "accounts") { { ... on User { + __typename id } - } => { + } => + { ... on User { name } } - } - } - } - } - } + }, + }, + }, + }, + }, } "###); } @@ -967,7 +975,7 @@ type User } } } - } + }, } "###); } diff --git a/apollo-federation/src/query_plan/query_planning_traversal.rs b/apollo-federation/src/query_plan/query_planning_traversal.rs index 70a614fb4c..5fe3f3b125 100644 --- a/apollo-federation/src/query_plan/query_planning_traversal.rs +++ b/apollo-federation/src/query_plan/query_planning_traversal.rs @@ -106,6 +106,7 @@ pub(crate) struct QueryPlanningTraversal<'a> { resolver_cache: ConditionResolverCache, } +#[derive(Debug)] struct OpenBranchAndSelections { /// The options for this open branch. open_branch: OpenBranch, @@ -113,6 +114,17 @@ struct OpenBranchAndSelections { selections: Vec, } +struct PlanInfo { + fetch_dependency_graph: FetchDependencyGraph, + path_tree: Arc, +} + +impl std::fmt::Debug for PlanInfo { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + std::fmt::Display::fmt(&self.path_tree, f) + } +} + pub(crate) struct BestQueryPlanInfo { /// The fetch dependency graph for this query plan. pub fetch_dependency_graph: FetchDependencyGraph, @@ -603,13 +615,16 @@ impl<'a> QueryPlanningTraversal<'a> { .collect(); let (best, cost) = generate_all_plans_and_find_best( - (initial_dependency_graph, Arc::new(initial_tree)), + PlanInfo { + fetch_dependency_graph: initial_dependency_graph, + path_tree: Arc::new(initial_tree), + }, other_trees, /*plan_builder*/ self, )?; self.best_plan = BestQueryPlanInfo { - fetch_dependency_graph: best.0, - path_tree: best.1, + fetch_dependency_graph: best.fetch_dependency_graph, + path_tree: best.path_tree, cost, } .into(); @@ -948,35 +963,34 @@ impl<'a> QueryPlanningTraversal<'a> { } } -impl PlanBuilder<(FetchDependencyGraph, Arc), Arc> - for QueryPlanningTraversal<'_> -{ - fn add_to_plan( - &mut self, - (plan_graph, plan_tree): &(FetchDependencyGraph, Arc), - tree: Arc, - ) -> (FetchDependencyGraph, Arc) { - let mut updated_graph = plan_graph.clone(); +impl PlanBuilder> for QueryPlanningTraversal<'_> { + fn add_to_plan(&mut self, plan_info: &PlanInfo, tree: Arc) -> PlanInfo { + let mut updated_graph = plan_info.fetch_dependency_graph.clone(); let result = self.updated_dependency_graph(&mut updated_graph, &tree); if result.is_ok() { - let updated_tree = plan_tree.merge(&tree); - (updated_graph, updated_tree) + PlanInfo { + fetch_dependency_graph: updated_graph, + path_tree: plan_info.path_tree.merge(&tree), + } } else { // Failed to update. Return the original plan. - (updated_graph, plan_tree.clone()) + PlanInfo { + fetch_dependency_graph: updated_graph, + path_tree: plan_info.path_tree.clone(), + } } } fn compute_plan_cost( &mut self, - (plan_graph, _): &mut (FetchDependencyGraph, Arc), + plan_info: &mut PlanInfo, ) -> Result { - self.cost(plan_graph) + self.cost(&mut plan_info.fetch_dependency_graph) } fn on_plan_generated( &self, - (_, _plan_tree): &(FetchDependencyGraph, Arc), + _plan_info: &PlanInfo, _cost: QueryPlanCost, _prev_cost: Option, ) { diff --git a/apollo-federation/src/schema/definitions.rs b/apollo-federation/src/schema/definitions.rs index 05e6ca9675..b35d5c2a1b 100644 --- a/apollo-federation/src/schema/definitions.rs +++ b/apollo-federation/src/schema/definitions.rs @@ -4,7 +4,7 @@ use apollo_compiler::Schema; use crate::error::FederationError; use crate::error::SingleFederationError; -use crate::schema::position::FieldDefinitionPosition; +use crate::schema::position::CompositeTypeDefinitionPosition; use crate::schema::position::InterfaceTypeDefinitionPosition; use crate::schema::position::TypeDefinitionPosition; use crate::schema::position::UnionTypeDefinitionPosition; @@ -14,15 +14,11 @@ pub(crate) enum AbstractType { Union(UnionTypeDefinitionPosition), } -impl AbstractType { - pub(crate) fn introspection_typename_field(&self) -> FieldDefinitionPosition { - match self { - AbstractType::Interface(interface) => { - FieldDefinitionPosition::Interface(interface.introspection_typename_field()) - } - AbstractType::Union(union) => { - FieldDefinitionPosition::Union(union.introspection_typename_field()) - } +impl From for CompositeTypeDefinitionPosition { + fn from(value: AbstractType) -> Self { + match value { + AbstractType::Interface(x) => Self::Interface(x), + AbstractType::Union(x) => Self::Union(x), } } } diff --git a/apollo-federation/src/schema/mod.rs b/apollo-federation/src/schema/mod.rs index 2f4ed33879..9f3dd35867 100644 --- a/apollo-federation/src/schema/mod.rs +++ b/apollo-federation/src/schema/mod.rs @@ -218,7 +218,7 @@ impl FederationSchema { } /// A GraphQL schema with federation data that is known to be valid, and cheap to clone. -#[derive(Debug, Clone)] +#[derive(Clone)] pub struct ValidFederationSchema { schema: Arc>, } @@ -306,3 +306,9 @@ impl Hash for ValidFederationSchema { Arc::as_ptr(&self.schema).hash(state); } } + +impl std::fmt::Debug for ValidFederationSchema { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "ValidFederationSchema @ {:?}", Arc::as_ptr(&self.schema)) + } +} diff --git a/apollo-federation/src/schema/position.rs b/apollo-federation/src/schema/position.rs index c88ab459b2..e61d227614 100644 --- a/apollo-federation/src/schema/position.rs +++ b/apollo-federation/src/schema/position.rs @@ -6343,7 +6343,7 @@ lazy_static! { }; // This is static so that UnionTypenameFieldDefinitionPosition.field_name() can return `&Name`, // like the other field_name() methods in this file. - static ref INTROSPECTION_TYPENAME_FIELD_NAME: Name = name!("__typename"); + pub(crate) static ref INTROSPECTION_TYPENAME_FIELD_NAME: Name = name!("__typename"); } fn validate_component_directives( diff --git a/apollo-federation/tests/query_plan/build_query_plan_support.rs b/apollo-federation/tests/query_plan/build_query_plan_support.rs index 828ddd9939..87439fa18a 100644 --- a/apollo-federation/tests/query_plan/build_query_plan_support.rs +++ b/apollo-federation/tests/query_plan/build_query_plan_support.rs @@ -1,4 +1,7 @@ +use std::collections::HashSet; use std::io::Read; +use std::sync::Mutex; +use std::sync::OnceLock; use apollo_federation::query_plan::query_planner::QueryPlanner; use apollo_federation::query_plan::query_planner::QueryPlannerConfig; @@ -55,11 +58,11 @@ macro_rules! assert_plan { #[track_caller] pub(crate) fn api_schema_and_planner( - test_name: &str, + function_path: &'static str, config: QueryPlannerConfig, subgraph_names_and_schemas: &[(&str, &str)], ) -> (ValidFederationSchema, QueryPlanner) { - let supergraph = compose(test_name, subgraph_names_and_schemas); + let supergraph = compose(function_path, subgraph_names_and_schemas); let supergraph = apollo_federation::Supergraph::new(&supergraph).unwrap(); let planner = QueryPlanner::new(&supergraph, config).unwrap(); let api_schema_config = apollo_federation::ApiSchemaOptions { @@ -71,7 +74,10 @@ pub(crate) fn api_schema_and_planner( } #[track_caller] -pub(crate) fn compose(test_name: &str, subgraph_names_and_schemas: &[(&str, &str)]) -> String { +pub(crate) fn compose( + function_path: &'static str, + subgraph_names_and_schemas: &[(&str, &str)], +) -> String { let unique_names: std::collections::HashSet<_> = subgraph_names_and_schemas .iter() .map(|(name, _)| name) @@ -102,11 +108,22 @@ pub(crate) fn compose(test_name: &str, subgraph_names_and_schemas: &[(&str, &str let expected_hash = hex::encode(hasher.finalize()); let prefix = "# Composed from subgraphs with hash: "; + let test_name = function_path.rsplit("::").next().unwrap(); + static SEEN_TEST_NAMES: OnceLock>> = OnceLock::new(); + let new = SEEN_TEST_NAMES + .get_or_init(Default::default) + .lock() + .unwrap() + .insert(test_name); + assert!( + new, + "planner!() can only be used once in test(s) named '{test_name}'" + ); let supergraph_path = std::path::PathBuf::from(std::env::var_os("CARGO_MANIFEST_DIR").unwrap()) .join("tests") .join("query_plan") .join("supergraphs") - .join(format!("{}.graphql", test_name.replace("::", "__"))); + .join(format!("{test_name}.graphql",)); let supergraph = match std::fs::read_to_string(&supergraph_path) { Ok(contents) => { if let Some(hash) = contents 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 d6066a6916..121d99fb73 100644 --- a/apollo-federation/tests/query_plan/build_query_plan_tests.rs +++ b/apollo-federation/tests/query_plan/build_query_plan_tests.rs @@ -1,25 +1,17 @@ -/// can use same root operation from multiple subgraphs in parallel +/* +Template to copy-paste: + #[test] -fn shareable_root_fields() { +fn some_name() { let planner = planner!( Subgraph1: r#" type Query { - me: User! @shareable - } - - type User @key(fields: "id") { - id: ID! - prop1: String + ... } "#, Subgraph2: r#" type Query { - me: User! @shareable - } - - type User @key(fields: "id") { - id: ID! - prop2: String + ... } "#, ); @@ -27,33 +19,18 @@ fn shareable_root_fields() { &planner, r#" { - me { - prop1 - prop2 - } + ... } "#, @r###" QueryPlan { - Parallel { - Fetch(service: "Subgraph1") { - { - me { - prop1 - } - } - } - Fetch(service: "Subgraph2") { - { - me { - prop2 - } - } - } - } + ... } "### ); } +*/ + +mod shareable_root_fields; // TODO: port the rest of query-planner-js/src/__tests__/buildPlan.test.ts diff --git a/apollo-federation/tests/query_plan/build_query_plan_tests/shareable_root_fields.rs b/apollo-federation/tests/query_plan/build_query_plan_tests/shareable_root_fields.rs new file mode 100644 index 0000000000..5a56842358 --- /dev/null +++ b/apollo-federation/tests/query_plan/build_query_plan_tests/shareable_root_fields.rs @@ -0,0 +1,134 @@ +#[test] +fn can_use_same_root_operation_from_multiple_subgraphs_in_parallel() { + let planner = planner!( + Subgraph1: r#" + type Query { + me: User! @shareable + } + + type User @key(fields: "id") { + id: ID! + prop1: String + } + "#, + Subgraph2: r#" + type Query { + me: User! @shareable + } + + type User @key(fields: "id") { + id: ID! + prop2: String + } + "#, + ); + assert_plan!( + &planner, + r#" + { + me { + prop1 + prop2 + } + } + "#, + @r###" + QueryPlan { + Parallel { + Fetch(service: "Subgraph1") { + { + me { + prop1 + } + } + }, + Fetch(service: "Subgraph2") { + { + me { + prop2 + } + } + }, + }, + } + "### + ); +} + +#[test] +fn handles_root_operation_shareable_in_many_subgraphs() { + let planner = planner!( + Subgraph1: r#" + type User @key(fields: "id") { + id: ID! + f0: Int + f1: Int + f2: Int + f3: Int + } + "#, + Subgraph2: r#" + type Query { + me: User! @shareable + } + + type User @key(fields: "id") { + id: ID! + } + "#, + Subgraph3: r#" + type Query { + me: User! @shareable + } + + type User @key(fields: "id") { + id: ID! + } + "#, + ); + assert_plan!( + &planner, + r#" + { + me { + f0 + f1 + f2 + f3 + } + } + "#, + @r###" + QueryPlan { + Sequence { + Fetch(service: "Subgraph2") { + { + me { + __typename + id + } + } + }, + Flatten(path: "me") { + Fetch(service: "Subgraph1") { + { + ... on User { + __typename + id + } + } => + { + ... on User { + f0 + f1 + f2 + f3 + } + } + }, + }, + }, + } + "### + ); +} diff --git a/apollo-federation/tests/query_plan/supergraphs/main__query_plan__build_query_plan_tests__shareable_root_fields.graphql b/apollo-federation/tests/query_plan/supergraphs/can_use_same_root_operation_from_multiple_subgraphs_in_parallel.graphql similarity index 100% rename from apollo-federation/tests/query_plan/supergraphs/main__query_plan__build_query_plan_tests__shareable_root_fields.graphql rename to apollo-federation/tests/query_plan/supergraphs/can_use_same_root_operation_from_multiple_subgraphs_in_parallel.graphql diff --git a/apollo-federation/tests/query_plan/supergraphs/handles_root_operation_shareable_in_many_subgraphs.graphql b/apollo-federation/tests/query_plan/supergraphs/handles_root_operation_shareable_in_many_subgraphs.graphql new file mode 100644 index 0000000000..76ccd6875f --- /dev/null +++ b/apollo-federation/tests/query_plan/supergraphs/handles_root_operation_shareable_in_many_subgraphs.graphql @@ -0,0 +1,63 @@ +# Composed from subgraphs with hash: 23796fabaf5b788d4a3df5cd4043263f2d825828 +schema + @link(url: "https://specs.apollo.dev/link/v1.0") + @link(url: "https://specs.apollo.dev/join/v0.3", for: EXECUTION) +{ + query: Query +} + +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) 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 + +scalar join__FieldSet + +enum join__Graph { + SUBGRAPH1 @join__graph(name: "Subgraph1", url: "none") + SUBGRAPH2 @join__graph(name: "Subgraph2", url: "none") + SUBGRAPH3 @join__graph(name: "Subgraph3", 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) + @join__type(graph: SUBGRAPH2) + @join__type(graph: SUBGRAPH3) +{ + me: User! @join__field(graph: SUBGRAPH2) @join__field(graph: SUBGRAPH3) +} + +type User + @join__type(graph: SUBGRAPH1, key: "id") + @join__type(graph: SUBGRAPH2, key: "id") + @join__type(graph: SUBGRAPH3, key: "id") +{ + id: ID! + f0: Int @join__field(graph: SUBGRAPH1) + f1: Int @join__field(graph: SUBGRAPH1) + f2: Int @join__field(graph: SUBGRAPH1) + f3: Int @join__field(graph: SUBGRAPH1) +}