From 62b4a7556b4200b7b2232ed7754c5b13d5960f14 Mon Sep 17 00:00:00 2001 From: Tyler Bloom Date: Thu, 21 Nov 2024 16:58:04 -0500 Subject: [PATCH 1/4] Context test rewriter assertion (#6319) --- .../build_query_plan_tests/context.rs | 427 +++++------------- 1 file changed, 111 insertions(+), 316 deletions(-) diff --git a/apollo-federation/tests/query_plan/build_query_plan_tests/context.rs b/apollo-federation/tests/query_plan/build_query_plan_tests/context.rs index 4bdf9f41b1..624678dc7a 100644 --- a/apollo-federation/tests/query_plan/build_query_plan_tests/context.rs +++ b/apollo-federation/tests/query_plan/build_query_plan_tests/context.rs @@ -36,6 +36,48 @@ use apollo_federation::query_plan::FetchDataRewrite; use apollo_federation::query_plan::PlanNode; use apollo_federation::query_plan::TopLevelPlanNode; +fn parse_fetch_data_path_element(value: &str) -> FetchDataPathElement { + if value == ".." { + FetchDataPathElement::Parent + } else if let Some(("", ty)) = value.split_once("... on ") { + FetchDataPathElement::TypenameEquals(Name::new(ty).unwrap()) + } else { + FetchDataPathElement::Key(Name::new(value).unwrap(), Default::default()) + } +} + +macro_rules! node_assert { + ($plan: ident, $index: literal, $($rename_key_to: literal, $path: expr),+$(,)?) => { + let Some(TopLevelPlanNode::Sequence(node)) = $plan.node else { + panic!("failed to get sequence node"); + }; + let Some(PlanNode::Flatten(node)) = node.nodes.get($index) else { + panic!("failed to get fetch node"); + }; + let PlanNode::Fetch(node) = &*node.node else { + panic!("failed to get flatten node"); + }; + let expected_rewrites = &[ $( $rename_key_to ),+ ]; + let expected_paths = &[ $( $path.into_iter().map(parse_fetch_data_path_element).collect::>() ),+ ]; + assert_eq!(expected_rewrites.len(), expected_paths.len()); + assert_eq!(node.context_rewrites.len(), expected_rewrites.len()); + node + .context_rewrites + .iter() + .map(|rewriter| { + let FetchDataRewrite::KeyRenamer(renamer) = &**rewriter else { + panic!("Expected KeyRenamer"); + }; + renamer + }) + .zip(expected_rewrites.iter().zip(expected_paths)) + .for_each(|(actual, (rename_key_to, path))|{ + assert_eq!(&actual.rename_key_to.as_str(), rename_key_to); + assert_eq!(&actual.path, path); + }); + }; +} + #[test] fn set_context_test_variable_is_from_same_subgraph() { let planner = planner!( @@ -110,33 +152,12 @@ fn set_context_test_variable_is_from_same_subgraph() { } "### ); - match plan.node { - Some(TopLevelPlanNode::Sequence(node)) => match node.nodes.get(1) { - Some(PlanNode::Flatten(node)) => match &*node.node { - PlanNode::Fetch(node) => { - assert_eq!( - node.context_rewrites, - vec![Arc::new(FetchDataRewrite::KeyRenamer( - FetchDataKeyRenamer { - rename_key_to: Name::new("contextualArgument_1_0").unwrap(), - path: vec![ - FetchDataPathElement::Parent, - FetchDataPathElement::TypenameEquals(Name::new("T").unwrap()), - FetchDataPathElement::Key( - Name::new("prop").unwrap(), - Default::default() - ), - ], - } - )),] - ); - } - _ => panic!("failed to get fetch node"), - }, - _ => panic!("failed to get flatten node"), - }, - _ => panic!("failed to get sequence node"), - } + node_assert!( + plan, + 1, + "contextualArgument_1_0", + ["..", "... on T", "prop"] + ); } #[test] @@ -230,33 +251,12 @@ fn set_context_test_variable_is_from_different_subgraph() { } "###); - match plan.node { - Some(TopLevelPlanNode::Sequence(node)) => match node.nodes.get(2) { - Some(PlanNode::Flatten(node)) => match &*node.node { - PlanNode::Fetch(node) => { - assert_eq!( - node.context_rewrites, - vec![Arc::new(FetchDataRewrite::KeyRenamer( - FetchDataKeyRenamer { - rename_key_to: Name::new("contextualArgument_1_0").unwrap(), - path: vec![ - FetchDataPathElement::Parent, - FetchDataPathElement::TypenameEquals(Name::new("T").unwrap()), - FetchDataPathElement::Key( - Name::new("prop").unwrap(), - Default::default() - ), - ], - } - )),] - ); - } - _ => panic!("failed to get fetch node"), - }, - _ => panic!("failed to get flatten node"), - }, - _ => panic!("failed to get sequence node"), - } + node_assert!( + plan, + 2, + "contextualArgument_1_0", + ["..", "... on T", "prop"] + ); } #[test] @@ -337,33 +337,13 @@ fn set_context_test_variable_is_already_in_a_different_fetch_group() { } "### ); - match plan.node { - Some(TopLevelPlanNode::Sequence(node)) => match node.nodes.get(1) { - Some(PlanNode::Flatten(node)) => match &*node.node { - PlanNode::Fetch(node) => { - assert_eq!( - node.context_rewrites, - vec![Arc::new(FetchDataRewrite::KeyRenamer( - FetchDataKeyRenamer { - rename_key_to: Name::new("contextualArgument_1_0").unwrap(), - path: vec![ - FetchDataPathElement::Parent, - FetchDataPathElement::TypenameEquals(Name::new("T").unwrap()), - FetchDataPathElement::Key( - Name::new("prop").unwrap(), - Default::default() - ), - ], - } - )),] - ); - } - _ => panic!("failed to get fetch node"), - }, - _ => panic!("failed to get flatten node"), - }, - _ => panic!("failed to get sequence node"), - } + + node_assert!( + plan, + 1, + "contextualArgument_1_0", + ["..", "... on T", "prop"] + ); } #[test] @@ -540,33 +520,13 @@ fn set_context_test_fetched_as_a_list() { } "### ); - match plan.node { - Some(TopLevelPlanNode::Sequence(node)) => match node.nodes.get(1) { - Some(PlanNode::Flatten(node)) => match &*node.node { - PlanNode::Fetch(node) => { - assert_eq!( - node.context_rewrites, - vec![Arc::new(FetchDataRewrite::KeyRenamer( - FetchDataKeyRenamer { - rename_key_to: Name::new("contextualArgument_1_0").unwrap(), - path: vec![ - FetchDataPathElement::Parent, - FetchDataPathElement::TypenameEquals(Name::new("T").unwrap()), - FetchDataPathElement::Key( - Name::new("prop").unwrap(), - Default::default() - ), - ], - } - )),] - ); - } - _ => panic!("failed to get fetch node"), - }, - _ => panic!("failed to get flatten node"), - }, - _ => panic!("failed to get sequence node"), - } + + node_assert!( + plan, + 1, + "contextualArgument_1_0", + ["..", "... on T", "prop"] + ); } #[test] @@ -657,44 +617,15 @@ fn set_context_test_impacts_on_query_planning() { } "### ); - match plan.node { - Some(TopLevelPlanNode::Sequence(node)) => match node.nodes.get(1) { - Some(PlanNode::Flatten(node)) => match &*node.node { - PlanNode::Fetch(node) => { - assert_eq!( - node.context_rewrites, - vec![ - Arc::new(FetchDataRewrite::KeyRenamer(FetchDataKeyRenamer { - rename_key_to: Name::new("contextualArgument_1_0").unwrap(), - path: vec![ - FetchDataPathElement::Parent, - FetchDataPathElement::TypenameEquals(Name::new("A").unwrap()), - FetchDataPathElement::Key( - Name::new("prop").unwrap(), - Default::default() - ), - ], - })), - Arc::new(FetchDataRewrite::KeyRenamer(FetchDataKeyRenamer { - rename_key_to: Name::new("contextualArgument_1_0").unwrap(), - path: vec![ - FetchDataPathElement::Parent, - FetchDataPathElement::TypenameEquals(Name::new("B").unwrap()), - FetchDataPathElement::Key( - Name::new("prop").unwrap(), - Default::default() - ), - ], - })), - ] - ); - } - _ => panic!("failed to get fetch node"), - }, - _ => panic!("failed to get flatten node"), - }, - _ => panic!("failed to get sequence node"), - } + + node_assert!( + plan, + 1, + "contextualArgument_1_0", + ["..", "... on A", "prop"], + "contextualArgument_1_0", + ["..", "... on B", "prop"] + ); } #[test] @@ -806,44 +737,15 @@ fn set_context_test_with_type_conditions_for_union() { } "### ); - match plan.node { - Some(TopLevelPlanNode::Sequence(node)) => match node.nodes.get(1) { - Some(PlanNode::Flatten(node)) => match &*node.node { - PlanNode::Fetch(node) => { - assert_eq!( - node.context_rewrites, - vec![ - Arc::new(FetchDataRewrite::KeyRenamer(FetchDataKeyRenamer { - rename_key_to: Name::new("contextualArgument_1_0").unwrap(), - path: vec![ - FetchDataPathElement::Parent, - FetchDataPathElement::TypenameEquals(Name::new("A").unwrap()), - FetchDataPathElement::Key( - Name::new("prop").unwrap(), - Default::default() - ), - ], - })), - Arc::new(FetchDataRewrite::KeyRenamer(FetchDataKeyRenamer { - rename_key_to: Name::new("contextualArgument_1_0").unwrap(), - path: vec![ - FetchDataPathElement::Parent, - FetchDataPathElement::TypenameEquals(Name::new("B").unwrap()), - FetchDataPathElement::Key( - Name::new("prop").unwrap(), - Default::default() - ), - ], - })), - ] - ); - } - _ => panic!("failed to get fetch node"), - }, - _ => panic!("failed to get flatten node"), - }, - _ => panic!("failed to get sequence node"), - } + + node_assert!( + plan, + 1, + "contextualArgument_1_0", + ["..", "... on A", "prop"], + "contextualArgument_1_0", + ["..", "... on B", "prop"] + ); } #[test] @@ -921,36 +823,8 @@ fn set_context_test_accesses_a_different_top_level_query() { } "### ); - match plan.node { - Some(TopLevelPlanNode::Sequence(node)) => match node.nodes.get(1) { - Some(PlanNode::Flatten(node)) => match &*node.node { - PlanNode::Fetch(node) => { - assert_eq!( - node.context_rewrites, - vec![Arc::new(FetchDataRewrite::KeyRenamer( - FetchDataKeyRenamer { - rename_key_to: Name::new("contextualArgument_1_0").unwrap(), - path: vec![ - FetchDataPathElement::Parent, - FetchDataPathElement::Key( - Name::new("me").unwrap(), - Default::default() - ), - FetchDataPathElement::Key( - Name::new("locale").unwrap(), - Default::default() - ), - ], - } - )),] - ); - } - _ => panic!("failed to get fetch node"), - }, - _ => panic!("failed to get flatten node"), - }, - _ => panic!("failed to get sequence node"), - } + + node_assert!(plan, 1, "contextualArgument_1_0", ["..", "me", "locale"]); } #[test] @@ -1022,33 +896,13 @@ fn set_context_one_subgraph() { } "### ); - match plan.node { - Some(TopLevelPlanNode::Sequence(node)) => match node.nodes.get(1) { - Some(PlanNode::Flatten(node)) => match &*node.node { - PlanNode::Fetch(node) => { - assert_eq!( - node.context_rewrites, - vec![Arc::new(FetchDataRewrite::KeyRenamer( - FetchDataKeyRenamer { - rename_key_to: Name::new("contextualArgument_1_0").unwrap(), - path: vec![ - FetchDataPathElement::Parent, - FetchDataPathElement::TypenameEquals(Name::new("T").unwrap()), - FetchDataPathElement::Key( - Name::new("prop").unwrap(), - Default::default() - ), - ], - } - )),] - ); - } - _ => panic!("failed to get fetch node"), - }, - _ => panic!("failed to get flatten node"), - }, - _ => panic!("failed to get sequence node"), - } + + node_assert!( + plan, + 1, + "contextualArgument_1_0", + ["..", "... on T", "prop"] + ); } #[test] @@ -1185,45 +1039,13 @@ fn set_context_required_field_is_several_levels_deep_going_back_and_forth_betwee } "### ); - match plan.node { - Some(TopLevelPlanNode::Sequence(node)) => match node.nodes.get(3) { - Some(PlanNode::Flatten(node)) => match &*node.node { - PlanNode::Fetch(node) => { - assert_eq!( - node.context_rewrites, - vec![Arc::new(FetchDataRewrite::KeyRenamer( - FetchDataKeyRenamer { - rename_key_to: Name::new("contextualArgument_1_0").unwrap(), - path: vec![ - FetchDataPathElement::Parent, - FetchDataPathElement::TypenameEquals(Name::new("T").unwrap()), - FetchDataPathElement::Key( - Name::new("a").unwrap(), - Default::default() - ), - FetchDataPathElement::Key( - Name::new("b").unwrap(), - Default::default() - ), - FetchDataPathElement::Key( - Name::new("c").unwrap(), - Default::default() - ), - FetchDataPathElement::Key( - Name::new("prop").unwrap(), - Default::default() - ), - ], - } - )),] - ); - } - _ => panic!("failed to get fetch node"), - }, - _ => panic!("failed to get flatten node"), - }, - _ => panic!("failed to get sequence node"), - } + + node_assert!( + plan, + 3, + "contextualArgument_1_0", + ["..", "... on T", "a", "b", "c", "prop"] + ); } #[test] @@ -1452,40 +1274,13 @@ fn set_context_test_efficiently_merge_fetch_groups() { } "### ); - match plan.node { - Some(TopLevelPlanNode::Sequence(node)) => match node.nodes.get(1) { - Some(PlanNode::Flatten(node)) => match &*node.node { - PlanNode::Fetch(node) => { - assert_eq!( - node.context_rewrites, - vec![ - Arc::new(FetchDataRewrite::KeyRenamer(FetchDataKeyRenamer { - rename_key_to: Name::new("contextualArgument_1_0").unwrap(), - path: vec![ - FetchDataPathElement::Key( - Name::new_unchecked("identifiers"), - Default::default() - ), - FetchDataPathElement::Key( - Name::new_unchecked("id5"), - Default::default() - ), - ], - })), - Arc::new(FetchDataRewrite::KeyRenamer(FetchDataKeyRenamer { - rename_key_to: Name::new("contextualArgument_1_1").unwrap(), - path: vec![FetchDataPathElement::Key( - Name::new_unchecked("mid"), - Default::default() - ),], - })), - ] - ); - } - _ => panic!("failed to get fetch node"), - }, - _ => panic!("failed to get flatten node"), - }, - _ => panic!("failed to get sequence node"), - } + + node_assert!( + plan, + 1, + "contextualArgument_1_0", + ["identifiers", "id5"], + "contextualArgument_1_1", + ["mid"] + ); } From bd6c283c2f969d85cfe4378dcab6ecbf7c348357 Mon Sep 17 00:00:00 2001 From: Duckki Oe Date: Thu, 21 Nov 2024 19:59:49 -0800 Subject: [PATCH 2/4] added a progression test --- .../query_plan/build_query_plan_tests.rs | 40 +++++++++++++++++++ 1 file changed, 40 insertions(+) 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 c0d85c7b62..2ee5850475 100644 --- a/apollo-federation/tests/query_plan/build_query_plan_tests.rs +++ b/apollo-federation/tests/query_plan/build_query_plan_tests.rs @@ -1412,3 +1412,43 @@ fn rebase_non_intersecting_without_dropping_inline_fragment_due_to_directive() { "### ); } + +#[test] +fn field_condition_propagation_to_parent_node() { + let planner = planner!( + Subgraph1: r#" + type Query { + test: T! + } + + type T { + id: ID! + name: String! + x: Int! + } + "#, + ); + assert_plan!( + &planner, + r#" + query($v1: Boolean!) { + test @include(if: $v1) { + id @include(if: false) + } + } + "#, + @r###" + QueryPlan { + Include(if: $v1) { + Fetch(service: "Subgraph1") { + { + test { + id + } + } + }, + }, + } + "### + ); +} From ad9330af0cf5d428ec0f70ad5f0a4a742e8ebfd2 Mon Sep 17 00:00:00 2001 From: Duckki Oe Date: Thu, 21 Nov 2024 20:00:06 -0800 Subject: [PATCH 3/4] fixed and updated the test --- apollo-federation/src/operation/mod.rs | 22 ++++++++++++------- .../query_plan/build_query_plan_tests.rs | 14 +----------- 2 files changed, 15 insertions(+), 21 deletions(-) diff --git a/apollo-federation/src/operation/mod.rs b/apollo-federation/src/operation/mod.rs index 1fe1f16287..452fb5f896 100644 --- a/apollo-federation/src/operation/mod.rs +++ b/apollo-federation/src/operation/mod.rs @@ -424,14 +424,20 @@ impl Selection { Ok(Conditions::Boolean(false)) } else { match self { - Selection::Field(_) => { - // The sub-selections of this field don't affect whether we should query this - // field, so we explicitly do not merge them in. - // - // PORT_NOTE: The JS codebase merges the sub-selections' conditions in with the - // field's conditions when field's selections are non-boolean. This is arguably - // a bug, so we've fixed it here. - Ok(self_conditions) + Selection::Field(field) => { + // If it's `true`, then it means that element is included. If it is a field, + // then we should also stop and return `true`, because no matter what the + // sub-selection is, we need to get that field. + // Note: The `Boolean(false)` case has been already checked above. Thus, this + // case is really checking for `Boolean(true)`. + if matches!(self_conditions, Conditions::Boolean(_)) { + return Ok(self_conditions); + } + let Some(ref selection_set) = field.selection_set else { + // No subselection set => condition won't change. + return Ok(self_conditions); + }; + Ok(self_conditions.merge(selection_set.conditions()?)) } Selection::InlineFragment(inline) => { Ok(self_conditions.merge(inline.selection_set.conditions()?)) 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 2ee5850475..9c6d35958a 100644 --- a/apollo-federation/tests/query_plan/build_query_plan_tests.rs +++ b/apollo-federation/tests/query_plan/build_query_plan_tests.rs @@ -1437,18 +1437,6 @@ fn field_condition_propagation_to_parent_node() { } } "#, - @r###" - QueryPlan { - Include(if: $v1) { - Fetch(service: "Subgraph1") { - { - test { - id - } - } - }, - }, - } - "### + @"QueryPlan {}" ); } From e79870d3a4ba133eff819c12680964b0fe4cfdb4 Mon Sep 17 00:00:00 2001 From: Duckki Oe Date: Thu, 21 Nov 2024 20:09:42 -0800 Subject: [PATCH 4/4] added the test's supergraph --- ...ndition_propagation_to_parent_node.graphql | 68 +++++++++++++++++++ 1 file changed, 68 insertions(+) create mode 100644 apollo-federation/tests/query_plan/supergraphs/field_condition_propagation_to_parent_node.graphql diff --git a/apollo-federation/tests/query_plan/supergraphs/field_condition_propagation_to_parent_node.graphql b/apollo-federation/tests/query_plan/supergraphs/field_condition_propagation_to_parent_node.graphql new file mode 100644 index 0000000000..b9de590d03 --- /dev/null +++ b/apollo-federation/tests/query_plan/supergraphs/field_condition_propagation_to_parent_node.graphql @@ -0,0 +1,68 @@ +# Composed from subgraphs with hash: 8c47dd900c78b744b6de87136fc52a6b07fd5a81 +schema + @link(url: "https://specs.apollo.dev/link/v1.0") + @link(url: "https://specs.apollo.dev/join/v0.5", 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, contextArguments: [join__ContextArgument!]) 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 + +input join__ContextArgument { + name: String! + type: String! + context: String! + selection: join__FieldValue! +} + +scalar join__DirectiveArguments + +scalar join__FieldSet + +scalar join__FieldValue + +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) +{ + test: T! +} + +type T + @join__type(graph: SUBGRAPH1) +{ + id: ID! + name: String! + x: Int! +}