From a760b7bf3dc91a082b59e6cf2c4ea9e00b8c7563 Mon Sep 17 00:00:00 2001 From: TylerBloom Date: Tue, 19 Nov 2024 21:00:26 -0500 Subject: [PATCH 1/3] Refactored rewriter node check in @context tests --- .../build_query_plan_tests/context.rs | 82 +++++++++++++++++++ 1 file changed, 82 insertions(+) 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..e72ad8e9ad 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,78 @@ 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() + ) + } +} + +// TODO: This should check the whole renamer not just the name. +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.iter().map(parse_fetch_data_path_element).collect::>() ),+ ]; + assert_eq!(node.context_rewrites.len(), expected_rewrite.len()); + node + .context_rewrites + .iter() + .map(|rewriter| { + let FetchDataRewrite::KeyRenamer(renamer) = &**rewriter else { + panic!("Expected KeyRenamer"); + }; + renamer + }) + .zip(expected_rewrite.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); + }); + }; +} +/* +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() + ), + ], + } +)),] +*/ + #[test] fn set_context_test_variable_is_from_same_subgraph() { let planner = planner!( @@ -137,6 +209,7 @@ fn set_context_test_variable_is_from_same_subgraph() { }, _ => panic!("failed to get sequence node"), } + node_assert!(plan, 1, "contextualArgument__Subgraph1_0"); } #[test] @@ -257,6 +330,7 @@ fn set_context_test_variable_is_from_different_subgraph() { }, _ => panic!("failed to get sequence node"), } + node_assert!(plan, 2, "contextualArgument__Subgraph1_0"); } #[test] @@ -364,6 +438,8 @@ fn set_context_test_variable_is_already_in_a_different_fetch_group() { }, _ => panic!("failed to get sequence node"), } + + node_assert!(plan, 1, "contextualArgument__Subgraph2_0"); } #[test] @@ -567,6 +643,8 @@ fn set_context_test_fetched_as_a_list() { }, _ => panic!("failed to get sequence node"), } + + node_assert!(plan, 1, "contextualArgument__Subgraph1_0"); } #[test] @@ -951,6 +1029,8 @@ fn set_context_test_accesses_a_different_top_level_query() { }, _ => panic!("failed to get sequence node"), } + + node_assert!(plan, 1, "contextualArgument__Subgraph1_0"); } #[test] @@ -1049,6 +1129,8 @@ fn set_context_one_subgraph() { }, _ => panic!("failed to get sequence node"), } + + node_assert!(plan, 1, "contextualArgument__Subgraph1_0"); } #[test] From 0427dd8980d57e06dd08ad2641113058a452b393 Mon Sep 17 00:00:00 2001 From: TylerBloom Date: Thu, 21 Nov 2024 16:26:17 -0500 Subject: [PATCH 2/3] Added path assertion to node_assert! macro in context tests --- .../build_query_plan_tests/context.rs | 400 +++--------------- 1 file changed, 70 insertions(+), 330 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 e72ad8e9ad..67601a941a 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 @@ -42,14 +42,10 @@ fn parse_fetch_data_path_element(value: &str) -> FetchDataPathElement { } else if let Some(("", ty)) = value.split_once("... on ") { FetchDataPathElement::TypenameEquals(Name::new(ty).unwrap()) } else { - FetchDataPathElement::Key( - Name::new(value).unwrap(), - Default::default() - ) + FetchDataPathElement::Key(Name::new(value).unwrap(), Default::default()) } } -// TODO: This should check the whole renamer not just the name. macro_rules! node_assert { ($plan: ident, $index: literal, $($rename_key_to: literal, $path: expr),+$(,)?) => { let Some(TopLevelPlanNode::Sequence(node)) = $plan.node else { @@ -62,8 +58,9 @@ macro_rules! node_assert { panic!("failed to get flatten node"); }; let expected_rewrites = &[ $( $rename_key_to ),+ ]; - let expected_paths = &[ $( $path.iter().map(parse_fetch_data_path_element).collect::>() ),+ ]; - assert_eq!(node.context_rewrites.len(), expected_rewrite.len()); + 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() @@ -73,7 +70,7 @@ macro_rules! node_assert { }; renamer }) - .zip(expected_rewrite.iter().zip(expected_paths)) + .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); @@ -182,34 +179,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__Subgraph1_0"); + node_assert!( + plan, + 1, + "contextualArgument_1_0", + ["..", "... on T", "prop"] + ); } #[test] @@ -303,34 +278,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__Subgraph1_0"); + node_assert!( + plan, + 2, + "contextualArgument_1_0", + ["..", "... on T", "prop"] + ); } #[test] @@ -411,35 +364,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__Subgraph2_0"); + node_assert!( + plan, + 1, + "contextualArgument_1_0", + ["..", "... on T", "prop"] + ); } #[test] @@ -616,35 +547,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__Subgraph1_0"); + node_assert!( + plan, + 1, + "contextualArgument_1_0", + ["..", "... on T", "prop"] + ); } #[test] @@ -735,44 +644,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] @@ -884,44 +764,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] @@ -999,38 +850,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__Subgraph1_0"); + node_assert!(plan, 1, "contextualArgument_1_0", ["..", "me", "locale"]); } #[test] @@ -1102,35 +923,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__Subgraph1_0"); + node_assert!( + plan, + 1, + "contextualArgument_1_0", + ["..", "... on T", "prop"] + ); } #[test] @@ -1267,45 +1066,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] @@ -1534,40 +1301,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 0c91a2aff84d5a88dd06d0849a41579681fef34e Mon Sep 17 00:00:00 2001 From: TylerBloom Date: Thu, 21 Nov 2024 16:28:22 -0500 Subject: [PATCH 3/3] Cleanup --- .../build_query_plan_tests/context.rs | 27 ------------------- 1 file changed, 27 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 67601a941a..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 @@ -77,33 +77,6 @@ macro_rules! node_assert { }); }; } -/* -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() - ), - ], - } -)),] -*/ #[test] fn set_context_test_variable_is_from_same_subgraph() {