From 6c559adf2ca742946b7af8eff7aefc329d95a5f3 Mon Sep 17 00:00:00 2001 From: Chidimma O Date: Fri, 19 Sep 2025 14:14:49 -0400 Subject: [PATCH 01/19] Port test for directiveArgumentMergeStrategies --- .../schema/argument_composition_strategies.rs | 4 +- apollo-federation/src/schema/mod.rs | 2 +- .../directive_argument_merge_strategies.rs | 440 ++++++++++++++++++ apollo-federation/tests/composition/mod.rs | 15 + 4 files changed, 458 insertions(+), 3 deletions(-) create mode 100644 apollo-federation/tests/composition/directive_argument_merge_strategies.rs diff --git a/apollo-federation/src/schema/argument_composition_strategies.rs b/apollo-federation/src/schema/argument_composition_strategies.rs index 31a28429d0..60a3a26e11 100644 --- a/apollo-federation/src/schema/argument_composition_strategies.rs +++ b/apollo-federation/src/schema/argument_composition_strategies.rs @@ -10,7 +10,7 @@ use crate::schema::FederationSchema; #[allow(dead_code)] #[derive(Clone, Copy)] -pub(crate) enum ArgumentCompositionStrategy { +pub enum ArgumentCompositionStrategy { Max, Min, // Sum, @@ -52,7 +52,7 @@ impl ArgumentCompositionStrategy { } } - pub(crate) fn name(&self) -> &str { + pub fn name(&self) -> &str { self.get_impl().name() } diff --git a/apollo-federation/src/schema/mod.rs b/apollo-federation/src/schema/mod.rs index 99d479f610..17ea3dc875 100644 --- a/apollo-federation/src/schema/mod.rs +++ b/apollo-federation/src/schema/mod.rs @@ -64,7 +64,7 @@ use crate::schema::position::TypeDefinitionPosition; use crate::schema::position::UnionTypeDefinitionPosition; use crate::schema::subgraph_metadata::SubgraphMetadata; -pub(crate) mod argument_composition_strategies; +pub mod argument_composition_strategies; pub(crate) mod blueprint; pub(crate) mod definitions; pub(crate) mod directive_location; diff --git a/apollo-federation/tests/composition/directive_argument_merge_strategies.rs b/apollo-federation/tests/composition/directive_argument_merge_strategies.rs new file mode 100644 index 0000000000..dc169f41b2 --- /dev/null +++ b/apollo-federation/tests/composition/directive_argument_merge_strategies.rs @@ -0,0 +1,440 @@ +use std::iter::zip; + +use apollo_compiler::ast; +use apollo_compiler::schema; +use apollo_federation::error::CompositionError; +use apollo_federation::schema::argument_composition_strategies::ArgumentCompositionStrategy; +use apollo_federation::supergraph::CompositionHint; +use apollo_federation::supergraph::Supergraph; + +use super::ServiceDefinition; +use super::compose_as_fed2_subgraphs; +use super::errors; + +fn assert_composition_success( + result: Result, Vec>, +) -> Supergraph { + result.expect("Expected successful composition, but got errors") +} + +// Helper function to create directive strings from applied directives +// Note: This function is currently unused but kept for future implementation +fn directive_strings_schema(directives: &schema::DirectiveList, target: &str) -> Vec { + directives + .iter() + .map(|dir| dir.to_string()) + .filter(|s| s.contains(target)) + .collect() +} + +fn directive_strings_ast(directives: &ast::DirectiveList, target: &str) -> Vec { + directives + .iter() + .map(|dir| dir.to_string()) + .filter(|s| s.contains(target)) + .collect() +} + +fn assert_hints_equal(actual_hints: &Vec, expected_hints: &Vec) { + if actual_hints.len() != expected_hints.len() { + panic!("Mismatched number of hints") + } + let zipped = zip(actual_hints, expected_hints); + zipped + .for_each(|(ch1, ch2)| assert!(ch1.code() == ch2.code() && ch1.message() == ch2.message())); +} + +#[cfg(test)] +mod tests { + use core::str; + use std::collections::HashMap; + use std::sync::LazyLock; + + use super::*; + + // Test cases for different argument composition strategies + struct CompositionStrategyTestCase { + name: &'static str, + composition_strategy: ArgumentCompositionStrategy, + arg_values_s1: serde_json::Value, + arg_values_s2: serde_json::Value, + result_values: serde_json::Value, + } + + static TEST_CASES: LazyLock> = LazyLock::new(|| { + HashMap::from([ + ( + "max", + CompositionStrategyTestCase { + name: "max", + composition_strategy: ArgumentCompositionStrategy::Max, + arg_values_s1: serde_json::json!({ + "t": "3", + "k": "1" + }), + arg_values_s2: serde_json::json!({ + "t": "2", + "k": "5", + "b": "4" + }), + result_values: serde_json::json!({ + "t": "3", + "k": "5", + "b": "4" + }), + }, + ), + ( + "min", + CompositionStrategyTestCase { + name: "min", + composition_strategy: ArgumentCompositionStrategy::Min, + arg_values_s1: serde_json::json!({ + "t": "3", + "k": "1" + }), + arg_values_s2: serde_json::json!({ + "t": "2", + "k": "5", + "b": "4" + }), + result_values: serde_json::json!({ + "t": "2", + "k": "1", + "b": "4" + }), + }, + ), + ( + "intersection", + CompositionStrategyTestCase { + name: "intersection", + composition_strategy: ArgumentCompositionStrategy::Intersection, + arg_values_s1: serde_json::json!({ + "t": r#"["foo", "bar"]"#, + "k": r#"[]"# + }), + arg_values_s2: serde_json::json!({ + "t": r#"["foo"]"#, + "k": r#"["v1", "v2"]"#, + "b": r#"["x"]"# + }), + result_values: serde_json::json!({ + "t": r#"["foo"]"#, + "k": r#"[]"#, + "b": r#"["x"]"# + }), + }, + ), + ( + "union", + CompositionStrategyTestCase { + name: "union", + composition_strategy: ArgumentCompositionStrategy::Union, + arg_values_s1: serde_json::json!({ + "t": r#"["foo", "bar"]"#, + "k": r#"[]"# + }), + arg_values_s2: serde_json::json!({ + "t": r#"["foo"]"#, + "k": r#"["v1", "v2"]"#, + "b": r#"["x"]"# + }), + result_values: serde_json::json!({ + "t": r#"["foo", "bar"]"#, + "k": r#"["v1", "v2"]"#, + "b": r#"["x"]"# + }), + }, + ), + ( + "nullable_and", + CompositionStrategyTestCase { + name: "nullable_and", + composition_strategy: ArgumentCompositionStrategy::NullableAnd, + arg_values_s1: serde_json::json!({ + "t": "true", + "k": "true" + }), + arg_values_s2: serde_json::json!({ + "t": "null", + "k": "false", + "b": "false" + }), + result_values: serde_json::json!({ + "t": "true", + "k": "false", + "b": "false" + }), + }, + ), + ( + "nullable_max", + CompositionStrategyTestCase { + name: "nullable_max", + composition_strategy: ArgumentCompositionStrategy::NullableMax, + arg_values_s1: serde_json::json!({ + "t": "3", + "k": "1" + }), + arg_values_s2: serde_json::json!({ + "t": "2", + "k": "null", + "b": "null" + }), + result_values: serde_json::json!({ + "t": "3", + "k": "1", + "b": "null" + }), + }, + ), + ( + "nullable_union", + CompositionStrategyTestCase { + name: "nullable_union", + composition_strategy: ArgumentCompositionStrategy::NullableUnion, + arg_values_s1: serde_json::json!({ + "t": r#"["foo", "bar"]"#, + "k": r#"[]"# + }), + arg_values_s2: serde_json::json!({ + "t": r#"["foo"]"#, + "k": r#"["v1", "v2"]"#, + "b": r#"["x"]"# + }), + result_values: serde_json::json!({ + "t": r#"["foo", "bar"]"#, + "k": r#"["v1", "v2"]"#, + "b": r#"["x"]"# + }), + }, + ), + ]) + }); + + // Helper function to convert JSON values to GraphQL value strings + fn value_to_string(value: &serde_json::Value) -> String { + match value { + serde_json::Value::Null => "null".to_string(), + serde_json::Value::Bool(b) => b.to_string(), + serde_json::Value::Number(n) => n.to_string(), + serde_json::Value::String(s) => format!("\"{}\"", s), + serde_json::Value::Array(arr) => { + let items: Vec = arr.iter().map(value_to_string).collect(); + format!("[{}]", items.join(", ")) + } + serde_json::Value::Object(_) => { + // For objects, we'll need to handle this differently based on the test case + // For now, return a placeholder + "{}".to_string() + } + } + } + + fn test_composition_of_directive_with_non_trivial_argument_strategies( + test_case: &CompositionStrategyTestCase, + ) { + let subgraph1 = ServiceDefinition { + name: "Subgraph1", + type_defs: &format!( + r#" + extend schema @link(url: "https://specs.apollo.dev/{}/v0.1") + + type Query {{ + t: T + }} + + type T + @key(fields: "k") + @{}(value: {}) + {{ + k: ID @{}(value: {}) + }} + "#, + test_case.name, + test_case.name, + value_to_string(&test_case.arg_values_s1["t"]), + test_case.name, + value_to_string(&test_case.arg_values_s1["k"]) + ), + }; + + let subgraph2 = ServiceDefinition { + name: "Subgraph2", + type_defs: &format!( + r#" + extend schema @link(url: "https://specs.apollo.dev/{}/v0.1") + + type T + @key(fields: "k") + @{}(value: {}) + {{ + k: ID @{}(value: {}) + a: Int + b: String @{}(value: {}) + }} + "#, + test_case.name, + test_case.name, + value_to_string(&test_case.arg_values_s2["t"]), + test_case.name, + value_to_string(&test_case.arg_values_s2["k"]), + test_case.name, + value_to_string(&test_case.arg_values_s2["b"]) + ), + }; + + let result = compose_as_fed2_subgraphs(&[subgraph1, subgraph2]); + let result_sg = assert_composition_success(result); + + // Check expected hints + let expected_hints = vec![ + CompositionHint { + code: String::from("MERGED_NON_REPEATABLE_DIRECTIVE_ARGUMENTS"), + message: format!( + "Directive @{} is applied to \"T\" in multiple subgraphs with different arguments. Merging strategies used by arguments: {{ \"value\": {} }}", + test_case.name, + test_case.composition_strategy.name() + ), + locations: Vec::new(), + }, + CompositionHint { + code: String::from("MERGED_NON_REPEATABLE_DIRECTIVE_ARGUMENTS"), + message: format!( + "Directive @{} is applied to \"T.k\" in multiple subgraphs with different arguments. Merging strategies used by arguments: {{ \"value\": {} }}", + test_case.name, + test_case.composition_strategy.name() + ), + locations: Vec::new(), + }, + ]; + assert_hints_equal(result_sg.hints(), &expected_hints); + + // Check expected directive strings + let schema = result_sg.schema().schema(); + assert_eq!( + directive_strings_schema(&schema.schema_definition.directives, test_case.name), + vec![format!( + r#"@link(url: "https://specs.apollo.dev/{}/v0.1")"#, + test_case.name + )] + ); + + let t = schema.get_object("T").unwrap(); + assert_eq!( + directive_strings_schema(&t.directives, test_case.name), + [format!( + r#"@{}(value: {})"#, + test_case.name, + value_to_string(test_case.result_values.get("t").unwrap()) + )] + ); + assert_eq!( + directive_strings_ast(&t.fields.get("k").unwrap().directives, test_case.name), + [format!( + r#"@{}(value: {})"#, + test_case.name, + value_to_string(test_case.result_values.get("k").unwrap()) + )] + ); + assert_eq!( + directive_strings_ast(&t.fields.get("b").unwrap().directives, test_case.name), + [format!( + r#"@{}(value: {})"#, + test_case.name, + value_to_string(test_case.result_values.get("b").unwrap()) + )] + ); + } + + #[test] + #[ignore = "Directive argument merge strategies not yet implemented"] + fn works_for_max() { + let test_case = TEST_CASES.get("max").expect("Test case not found"); + test_composition_of_directive_with_non_trivial_argument_strategies(test_case); + } + + #[test] + #[ignore = "Directive argument merge strategies not yet implemented"] + fn works_for_min() { + let test_case = TEST_CASES.get("min").expect("Test case not found"); + test_composition_of_directive_with_non_trivial_argument_strategies(test_case); + } + + #[test] + #[ignore = "Directive argument merge strategies not yet implemented"] + fn works_for_intersection() { + let test_case = TEST_CASES.get("intersection").expect("Test case not found"); + test_composition_of_directive_with_non_trivial_argument_strategies(test_case); + } + + #[test] + #[ignore = "Directive argument merge strategies not yet implemented"] + fn works_for_union() { + let test_case = TEST_CASES.get("union").expect("Test case not found"); + test_composition_of_directive_with_non_trivial_argument_strategies(test_case); + } + + #[test] + #[ignore = "Directive argument merge strategies not yet implemented"] + fn works_for_nullable_and() { + let test_case = TEST_CASES.get("nullable_and").expect("Test case not found"); + test_composition_of_directive_with_non_trivial_argument_strategies(test_case); + } + + #[test] + #[ignore = "Directive argument merge strategies not yet implemented"] + fn works_for_nullable_max() { + let test_case = TEST_CASES.get("nullable_max").expect("Test case not found"); + test_composition_of_directive_with_non_trivial_argument_strategies(test_case); + } + + #[test] + #[ignore = "Directive argument merge strategies not yet implemented"] + fn works_for_nullable_union() { + let test_case = TEST_CASES + .get("nullable_union") + .expect("Test case not found"); + test_composition_of_directive_with_non_trivial_argument_strategies(test_case); + } + + #[test] + #[ignore = "Directive argument merge strategies not yet implemented"] + fn errors_when_declaring_strategy_that_does_not_match_the_argument_type() { + let subgraph1 = ServiceDefinition { + name: "Subgraph1", + type_defs: r#" + extend schema @link(url: "https://specs.apollo.dev/foo/v0.1") + + type Query { + t: T + } + + type T { + v: String @foo(value: "bar") + } + "#, + }; + + let subgraph2 = ServiceDefinition { + name: "Subgraph2", + type_defs: r#" + extend schema @link(url: "https://specs.apollo.dev/foo/v0.1") + + type T { + v: String @foo(value: "bar") + } + "#, + }; + + let result = compose_as_fed2_subgraphs(&[subgraph1, subgraph2]); + let errs = errors(&result); + assert_eq!( + errs.iter().map(|(_, message)| message).collect::>(), + [ + r#"Invalid composition strategy MAX for argument @foo(value:) of type String; MAX only supports type(s) Int!"# + ] + ); + } +} diff --git a/apollo-federation/tests/composition/mod.rs b/apollo-federation/tests/composition/mod.rs index e8ef8d620e..9489c07fd8 100644 --- a/apollo-federation/tests/composition/mod.rs +++ b/apollo-federation/tests/composition/mod.rs @@ -1,5 +1,6 @@ mod compose_directive; mod demand_control; +mod directive_argument_merge_strategies; // TODO: remove #[ignore] from tests once all fns called by Merger::merge() are implemented mod external; mod override_directive; @@ -60,7 +61,21 @@ pub(crate) mod test_helpers { compose(fed2_subgraphs) } + + // Return a vec of error codes and error messages as strings + pub(crate) fn errors( + result: &Result, Vec>, + ) -> Vec<(String, String)> { + match result { + Ok(_) => panic!("Expected an error, but got a successful composition"), + Err(err) => err + .iter() + .map(|e| (e.code().definition().code().to_string(), e.to_string())) + .collect(), + } + } } pub(crate) use test_helpers::ServiceDefinition; pub(crate) use test_helpers::compose_as_fed2_subgraphs; +pub(crate) use test_helpers::errors; From d85bd3258028e6e4af57ee6dc4551444e7ef1d8a Mon Sep 17 00:00:00 2001 From: Chidimma O Date: Fri, 19 Sep 2025 14:41:37 -0400 Subject: [PATCH 02/19] Omit json to simplify test case struct --- .../directive_argument_merge_strategies.rs | 243 ++++++++---------- 1 file changed, 112 insertions(+), 131 deletions(-) diff --git a/apollo-federation/tests/composition/directive_argument_merge_strategies.rs b/apollo-federation/tests/composition/directive_argument_merge_strategies.rs index dc169f41b2..c2e7f19091 100644 --- a/apollo-federation/tests/composition/directive_argument_merge_strategies.rs +++ b/apollo-federation/tests/composition/directive_argument_merge_strategies.rs @@ -53,14 +53,14 @@ mod tests { use super::*; // Test cases for different argument composition strategies - struct CompositionStrategyTestCase { - name: &'static str, + struct CompositionStrategyTestCase<'a> { + name: &'a str, composition_strategy: ArgumentCompositionStrategy, - arg_values_s1: serde_json::Value, - arg_values_s2: serde_json::Value, - result_values: serde_json::Value, + arg_values_s1: HashMap<&'a str, &'a str>, + arg_values_s2: HashMap<&'a str, &'a str>, + result_values: HashMap<&'a str, &'a str>, } - + static TEST_CASES: LazyLock> = LazyLock::new(|| { HashMap::from([ ( @@ -68,20 +68,20 @@ mod tests { CompositionStrategyTestCase { name: "max", composition_strategy: ArgumentCompositionStrategy::Max, - arg_values_s1: serde_json::json!({ - "t": "3", - "k": "1" - }), - arg_values_s2: serde_json::json!({ - "t": "2", - "k": "5", - "b": "4" - }), - result_values: serde_json::json!({ - "t": "3", - "k": "5", - "b": "4" - }), + arg_values_s1: HashMap::from([ + ("t", "3"), + ("k", "1") + ]), + arg_values_s2: HashMap::from([ + ("t", "2"), + ("k", "5"), + ("b", "4") + ]), + result_values: HashMap::from([ + ("t", "3"), + ("k", "5"), + ("b", "4") + ]), }, ), ( @@ -89,20 +89,20 @@ mod tests { CompositionStrategyTestCase { name: "min", composition_strategy: ArgumentCompositionStrategy::Min, - arg_values_s1: serde_json::json!({ - "t": "3", - "k": "1" - }), - arg_values_s2: serde_json::json!({ - "t": "2", - "k": "5", - "b": "4" - }), - result_values: serde_json::json!({ - "t": "2", - "k": "1", - "b": "4" - }), + arg_values_s1: HashMap::from([ + ("t", "3"), + ("k", "1") + ]), + arg_values_s2: HashMap::from([ + ("t", "2"), + ("k", "5"), + ("b", "4") + ]), + result_values: HashMap::from([ + ("t", "2"), + ("k", "1"), + ("b", "4") + ]), }, ), ( @@ -110,20 +110,20 @@ mod tests { CompositionStrategyTestCase { name: "intersection", composition_strategy: ArgumentCompositionStrategy::Intersection, - arg_values_s1: serde_json::json!({ - "t": r#"["foo", "bar"]"#, - "k": r#"[]"# - }), - arg_values_s2: serde_json::json!({ - "t": r#"["foo"]"#, - "k": r#"["v1", "v2"]"#, - "b": r#"["x"]"# - }), - result_values: serde_json::json!({ - "t": r#"["foo"]"#, - "k": r#"[]"#, - "b": r#"["x"]"# - }), + arg_values_s1: HashMap::from([ + ("t", r#"["foo", "bar"]"#), + ("k", r#"[]"#) + ]), + arg_values_s2: HashMap::from([ + ("t", r#"["foo"]"#), + ("k", r#"["v1", "v2"]"#), + ("b", r#"["x"]"#) + ]), + result_values: HashMap::from([ + ("t", r#"["foo"]"#), + ("k", r#"[]"#), + ("b", r#"["x"]"#) + ]), }, ), ( @@ -131,20 +131,20 @@ mod tests { CompositionStrategyTestCase { name: "union", composition_strategy: ArgumentCompositionStrategy::Union, - arg_values_s1: serde_json::json!({ - "t": r#"["foo", "bar"]"#, - "k": r#"[]"# - }), - arg_values_s2: serde_json::json!({ - "t": r#"["foo"]"#, - "k": r#"["v1", "v2"]"#, - "b": r#"["x"]"# - }), - result_values: serde_json::json!({ - "t": r#"["foo", "bar"]"#, - "k": r#"["v1", "v2"]"#, - "b": r#"["x"]"# - }), + arg_values_s1: HashMap::from([ + ("t", r#"["foo", "bar"]"#), + ("k", r#"[]"#) + ]), + arg_values_s2: HashMap::from([ + ("t", r#"["foo"]"#), + ("k", r#"["v1", "v2"]"#), + ("b", r#"["x"]"#) + ]), + result_values: HashMap::from([ + ("t", r#"["foo", "bar"]"#), + ("k", r#"["v1", "v2"]"#), + ("b", r#"["x"]"#) + ]), }, ), ( @@ -152,20 +152,20 @@ mod tests { CompositionStrategyTestCase { name: "nullable_and", composition_strategy: ArgumentCompositionStrategy::NullableAnd, - arg_values_s1: serde_json::json!({ - "t": "true", - "k": "true" - }), - arg_values_s2: serde_json::json!({ - "t": "null", - "k": "false", - "b": "false" - }), - result_values: serde_json::json!({ - "t": "true", - "k": "false", - "b": "false" - }), + arg_values_s1: HashMap::from([ + ("t", "true"), + ("k", "true") + ]), + arg_values_s2: HashMap::from([ + ("t", "null"), + ("k", "false"), + ("b", "false") + ]), + result_values: HashMap::from([ + ("t", "true"), + ("k", "false"), + ("b", "false") + ]), }, ), ( @@ -173,20 +173,20 @@ mod tests { CompositionStrategyTestCase { name: "nullable_max", composition_strategy: ArgumentCompositionStrategy::NullableMax, - arg_values_s1: serde_json::json!({ - "t": "3", - "k": "1" - }), - arg_values_s2: serde_json::json!({ - "t": "2", - "k": "null", - "b": "null" - }), - result_values: serde_json::json!({ - "t": "3", - "k": "1", - "b": "null" - }), + arg_values_s1: HashMap::from([ + ("t", "3"), + ("k", "1") + ]), + arg_values_s2: HashMap::from([ + ("t", "2"), + ("k", "null"), + ("b", "null") + ]), + result_values: HashMap::from([ + ("t", "3"), + ("k", "1"), + ("b", "null") + ]), }, ), ( @@ -194,44 +194,25 @@ mod tests { CompositionStrategyTestCase { name: "nullable_union", composition_strategy: ArgumentCompositionStrategy::NullableUnion, - arg_values_s1: serde_json::json!({ - "t": r#"["foo", "bar"]"#, - "k": r#"[]"# - }), - arg_values_s2: serde_json::json!({ - "t": r#"["foo"]"#, - "k": r#"["v1", "v2"]"#, - "b": r#"["x"]"# - }), - result_values: serde_json::json!({ - "t": r#"["foo", "bar"]"#, - "k": r#"["v1", "v2"]"#, - "b": r#"["x"]"# - }), + arg_values_s1: HashMap::from([ + ("t", r#"["foo", "bar"]"#), + ("k", r#"[]"#) + ]), + arg_values_s2: HashMap::from([ + ("t", r#"["foo"]"#), + ("k", r#"["v1", "v2"]"#), + ("b", r#"["x"]"#) + ]), + result_values: HashMap::from([ + ("t", r#"["foo", "bar"]"#), + ("k", r#"["v1", "v2"]"#), + ("b", r#"["x"]"#) + ]), }, ), ]) }); - // Helper function to convert JSON values to GraphQL value strings - fn value_to_string(value: &serde_json::Value) -> String { - match value { - serde_json::Value::Null => "null".to_string(), - serde_json::Value::Bool(b) => b.to_string(), - serde_json::Value::Number(n) => n.to_string(), - serde_json::Value::String(s) => format!("\"{}\"", s), - serde_json::Value::Array(arr) => { - let items: Vec = arr.iter().map(value_to_string).collect(); - format!("[{}]", items.join(", ")) - } - serde_json::Value::Object(_) => { - // For objects, we'll need to handle this differently based on the test case - // For now, return a placeholder - "{}".to_string() - } - } - } - fn test_composition_of_directive_with_non_trivial_argument_strategies( test_case: &CompositionStrategyTestCase, ) { @@ -254,9 +235,9 @@ mod tests { "#, test_case.name, test_case.name, - value_to_string(&test_case.arg_values_s1["t"]), + test_case.arg_values_s1["t"], test_case.name, - value_to_string(&test_case.arg_values_s1["k"]) + test_case.arg_values_s1["k"] ), }; @@ -277,11 +258,11 @@ mod tests { "#, test_case.name, test_case.name, - value_to_string(&test_case.arg_values_s2["t"]), + test_case.arg_values_s2["t"], test_case.name, - value_to_string(&test_case.arg_values_s2["k"]), + test_case.arg_values_s2["k"], test_case.name, - value_to_string(&test_case.arg_values_s2["b"]) + test_case.arg_values_s2["b"] ), }; @@ -327,7 +308,7 @@ mod tests { [format!( r#"@{}(value: {})"#, test_case.name, - value_to_string(test_case.result_values.get("t").unwrap()) + test_case.result_values["t"] )] ); assert_eq!( @@ -335,7 +316,7 @@ mod tests { [format!( r#"@{}(value: {})"#, test_case.name, - value_to_string(test_case.result_values.get("k").unwrap()) + test_case.result_values["k"] )] ); assert_eq!( @@ -343,7 +324,7 @@ mod tests { [format!( r#"@{}(value: {})"#, test_case.name, - value_to_string(test_case.result_values.get("b").unwrap()) + test_case.result_values["b"] )] ); } From 230b350c0d50982c70459d0c7b574aa9c2854f97 Mon Sep 17 00:00:00 2001 From: Chidimma O Date: Fri, 19 Sep 2025 14:57:54 -0400 Subject: [PATCH 03/19] Make assert_composition_success() more in line with TS version --- .../directive_argument_merge_strategies.rs | 11 ++--------- apollo-federation/tests/composition/mod.rs | 19 +++++++++++++++++++ 2 files changed, 21 insertions(+), 9 deletions(-) diff --git a/apollo-federation/tests/composition/directive_argument_merge_strategies.rs b/apollo-federation/tests/composition/directive_argument_merge_strategies.rs index c2e7f19091..27aabb9c68 100644 --- a/apollo-federation/tests/composition/directive_argument_merge_strategies.rs +++ b/apollo-federation/tests/composition/directive_argument_merge_strategies.rs @@ -2,20 +2,13 @@ use std::iter::zip; use apollo_compiler::ast; use apollo_compiler::schema; -use apollo_federation::error::CompositionError; use apollo_federation::schema::argument_composition_strategies::ArgumentCompositionStrategy; use apollo_federation::supergraph::CompositionHint; -use apollo_federation::supergraph::Supergraph; use super::ServiceDefinition; use super::compose_as_fed2_subgraphs; use super::errors; - -fn assert_composition_success( - result: Result, Vec>, -) -> Supergraph { - result.expect("Expected successful composition, but got errors") -} +use super::assert_composition_success; // Helper function to create directive strings from applied directives // Note: This function is currently unused but kept for future implementation @@ -60,7 +53,7 @@ mod tests { arg_values_s2: HashMap<&'a str, &'a str>, result_values: HashMap<&'a str, &'a str>, } - + static TEST_CASES: LazyLock> = LazyLock::new(|| { HashMap::from([ ( diff --git a/apollo-federation/tests/composition/mod.rs b/apollo-federation/tests/composition/mod.rs index 9489c07fd8..2b2b47e1b0 100644 --- a/apollo-federation/tests/composition/mod.rs +++ b/apollo-federation/tests/composition/mod.rs @@ -62,6 +62,24 @@ pub(crate) mod test_helpers { compose(fed2_subgraphs) } + pub(crate) fn assert_composition_success( + result: Result, Vec>, + ) -> Supergraph { + match result { + Ok(supergraph) => supergraph, + Err(errors) => { + panic!( + "Expected successful composition, but got errors: {:?}", + errors + .iter() + .map(|e| e.to_string()) + .collect::>() + .join("\n\n") + ); + } + } + } + // Return a vec of error codes and error messages as strings pub(crate) fn errors( result: &Result, Vec>, @@ -79,3 +97,4 @@ pub(crate) mod test_helpers { pub(crate) use test_helpers::ServiceDefinition; pub(crate) use test_helpers::compose_as_fed2_subgraphs; pub(crate) use test_helpers::errors; +pub(crate) use test_helpers::assert_composition_success; From 98eb3022a63d16d35d6294f31c49f8abbc423ed8 Mon Sep 17 00:00:00 2001 From: Chidimma O Date: Fri, 19 Sep 2025 15:29:26 -0400 Subject: [PATCH 04/19] Run formatter --- .../directive_argument_merge_strategies.rs | 118 +++++------------- apollo-federation/tests/composition/mod.rs | 2 +- 2 files changed, 32 insertions(+), 88 deletions(-) diff --git a/apollo-federation/tests/composition/directive_argument_merge_strategies.rs b/apollo-federation/tests/composition/directive_argument_merge_strategies.rs index 27aabb9c68..33d212b613 100644 --- a/apollo-federation/tests/composition/directive_argument_merge_strategies.rs +++ b/apollo-federation/tests/composition/directive_argument_merge_strategies.rs @@ -6,9 +6,9 @@ use apollo_federation::schema::argument_composition_strategies::ArgumentComposit use apollo_federation::supergraph::CompositionHint; use super::ServiceDefinition; +use super::assert_composition_success; use super::compose_as_fed2_subgraphs; use super::errors; -use super::assert_composition_success; // Helper function to create directive strings from applied directives // Note: This function is currently unused but kept for future implementation @@ -61,20 +61,9 @@ mod tests { CompositionStrategyTestCase { name: "max", composition_strategy: ArgumentCompositionStrategy::Max, - arg_values_s1: HashMap::from([ - ("t", "3"), - ("k", "1") - ]), - arg_values_s2: HashMap::from([ - ("t", "2"), - ("k", "5"), - ("b", "4") - ]), - result_values: HashMap::from([ - ("t", "3"), - ("k", "5"), - ("b", "4") - ]), + arg_values_s1: HashMap::from([("t", "3"), ("k", "1")]), + arg_values_s2: HashMap::from([("t", "2"), ("k", "5"), ("b", "4")]), + result_values: HashMap::from([("t", "3"), ("k", "5"), ("b", "4")]), }, ), ( @@ -82,20 +71,9 @@ mod tests { CompositionStrategyTestCase { name: "min", composition_strategy: ArgumentCompositionStrategy::Min, - arg_values_s1: HashMap::from([ - ("t", "3"), - ("k", "1") - ]), - arg_values_s2: HashMap::from([ - ("t", "2"), - ("k", "5"), - ("b", "4") - ]), - result_values: HashMap::from([ - ("t", "2"), - ("k", "1"), - ("b", "4") - ]), + arg_values_s1: HashMap::from([("t", "3"), ("k", "1")]), + arg_values_s2: HashMap::from([("t", "2"), ("k", "5"), ("b", "4")]), + result_values: HashMap::from([("t", "2"), ("k", "1"), ("b", "4")]), }, ), ( @@ -103,19 +81,16 @@ mod tests { CompositionStrategyTestCase { name: "intersection", composition_strategy: ArgumentCompositionStrategy::Intersection, - arg_values_s1: HashMap::from([ - ("t", r#"["foo", "bar"]"#), - ("k", r#"[]"#) - ]), - arg_values_s2: HashMap::from([ + arg_values_s1: HashMap::from([("t", r#"["foo", "bar"]"#), ("k", r#"[]"#)]), + arg_values_s2: HashMap::from([ ("t", r#"["foo"]"#), ("k", r#"["v1", "v2"]"#), - ("b", r#"["x"]"#) + ("b", r#"["x"]"#), ]), - result_values: HashMap::from([ + result_values: HashMap::from([ ("t", r#"["foo"]"#), ("k", r#"[]"#), - ("b", r#"["x"]"#) + ("b", r#"["x"]"#), ]), }, ), @@ -124,19 +99,16 @@ mod tests { CompositionStrategyTestCase { name: "union", composition_strategy: ArgumentCompositionStrategy::Union, - arg_values_s1: HashMap::from([ - ("t", r#"["foo", "bar"]"#), - ("k", r#"[]"#) - ]), - arg_values_s2: HashMap::from([ + arg_values_s1: HashMap::from([("t", r#"["foo", "bar"]"#), ("k", r#"[]"#)]), + arg_values_s2: HashMap::from([ ("t", r#"["foo"]"#), ("k", r#"["v1", "v2"]"#), - ("b", r#"["x"]"#) + ("b", r#"["x"]"#), ]), - result_values: HashMap::from([ + result_values: HashMap::from([ ("t", r#"["foo", "bar"]"#), ("k", r#"["v1", "v2"]"#), - ("b", r#"["x"]"#) + ("b", r#"["x"]"#), ]), }, ), @@ -145,20 +117,9 @@ mod tests { CompositionStrategyTestCase { name: "nullable_and", composition_strategy: ArgumentCompositionStrategy::NullableAnd, - arg_values_s1: HashMap::from([ - ("t", "true"), - ("k", "true") - ]), - arg_values_s2: HashMap::from([ - ("t", "null"), - ("k", "false"), - ("b", "false") - ]), - result_values: HashMap::from([ - ("t", "true"), - ("k", "false"), - ("b", "false") - ]), + arg_values_s1: HashMap::from([("t", "true"), ("k", "true")]), + arg_values_s2: HashMap::from([("t", "null"), ("k", "false"), ("b", "false")]), + result_values: HashMap::from([("t", "true"), ("k", "false"), ("b", "false")]), }, ), ( @@ -166,20 +127,9 @@ mod tests { CompositionStrategyTestCase { name: "nullable_max", composition_strategy: ArgumentCompositionStrategy::NullableMax, - arg_values_s1: HashMap::from([ - ("t", "3"), - ("k", "1") - ]), - arg_values_s2: HashMap::from([ - ("t", "2"), - ("k", "null"), - ("b", "null") - ]), - result_values: HashMap::from([ - ("t", "3"), - ("k", "1"), - ("b", "null") - ]), + arg_values_s1: HashMap::from([("t", "3"), ("k", "1")]), + arg_values_s2: HashMap::from([("t", "2"), ("k", "null"), ("b", "null")]), + result_values: HashMap::from([("t", "3"), ("k", "1"), ("b", "null")]), }, ), ( @@ -187,19 +137,16 @@ mod tests { CompositionStrategyTestCase { name: "nullable_union", composition_strategy: ArgumentCompositionStrategy::NullableUnion, - arg_values_s1: HashMap::from([ - ("t", r#"["foo", "bar"]"#), - ("k", r#"[]"#) - ]), - arg_values_s2: HashMap::from([ + arg_values_s1: HashMap::from([("t", r#"["foo", "bar"]"#), ("k", r#"[]"#)]), + arg_values_s2: HashMap::from([ ("t", r#"["foo"]"#), ("k", r#"["v1", "v2"]"#), - ("b", r#"["x"]"#) + ("b", r#"["x"]"#), ]), - result_values: HashMap::from([ + result_values: HashMap::from([ ("t", r#"["foo", "bar"]"#), ("k", r#"["v1", "v2"]"#), - ("b", r#"["x"]"#) + ("b", r#"["x"]"#), ]), }, ), @@ -300,24 +247,21 @@ mod tests { directive_strings_schema(&t.directives, test_case.name), [format!( r#"@{}(value: {})"#, - test_case.name, - test_case.result_values["t"] + test_case.name, test_case.result_values["t"] )] ); assert_eq!( directive_strings_ast(&t.fields.get("k").unwrap().directives, test_case.name), [format!( r#"@{}(value: {})"#, - test_case.name, - test_case.result_values["k"] + test_case.name, test_case.result_values["k"] )] ); assert_eq!( directive_strings_ast(&t.fields.get("b").unwrap().directives, test_case.name), [format!( r#"@{}(value: {})"#, - test_case.name, - test_case.result_values["b"] + test_case.name, test_case.result_values["b"] )] ); } diff --git a/apollo-federation/tests/composition/mod.rs b/apollo-federation/tests/composition/mod.rs index 2b2b47e1b0..72816bd570 100644 --- a/apollo-federation/tests/composition/mod.rs +++ b/apollo-federation/tests/composition/mod.rs @@ -95,6 +95,6 @@ pub(crate) mod test_helpers { } pub(crate) use test_helpers::ServiceDefinition; +pub(crate) use test_helpers::assert_composition_success; pub(crate) use test_helpers::compose_as_fed2_subgraphs; pub(crate) use test_helpers::errors; -pub(crate) use test_helpers::assert_composition_success; From 60ec5b38ff6024803c53ffd5fe20fa41ea973021 Mon Sep 17 00:00:00 2001 From: Chidimma O Date: Fri, 19 Sep 2025 16:29:46 -0400 Subject: [PATCH 05/19] Minor comment updates --- .../tests/composition/directive_argument_merge_strategies.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/apollo-federation/tests/composition/directive_argument_merge_strategies.rs b/apollo-federation/tests/composition/directive_argument_merge_strategies.rs index 33d212b613..cea20b7e2d 100644 --- a/apollo-federation/tests/composition/directive_argument_merge_strategies.rs +++ b/apollo-federation/tests/composition/directive_argument_merge_strategies.rs @@ -10,8 +10,7 @@ use super::assert_composition_success; use super::compose_as_fed2_subgraphs; use super::errors; -// Helper function to create directive strings from applied directives -// Note: This function is currently unused but kept for future implementation +// Helper function to create directive strings from applied directives using schema::DirectiveList fn directive_strings_schema(directives: &schema::DirectiveList, target: &str) -> Vec { directives .iter() @@ -20,6 +19,7 @@ fn directive_strings_schema(directives: &schema::DirectiveList, target: &str) -> .collect() } +// Helper function to create directive strings from applied directives using ast::DirectiveList fn directive_strings_ast(directives: &ast::DirectiveList, target: &str) -> Vec { directives .iter() From 05d5a99fe9d088ade708c6eb28044af30ee0917a Mon Sep 17 00:00:00 2001 From: Chidimma O Date: Thu, 23 Oct 2025 15:45:44 -0400 Subject: [PATCH 06/19] Move assert_hints_equal() up to mod level --- apollo-federation/tests/composition/mod.rs | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/apollo-federation/tests/composition/mod.rs b/apollo-federation/tests/composition/mod.rs index 72816bd570..a549af68e5 100644 --- a/apollo-federation/tests/composition/mod.rs +++ b/apollo-federation/tests/composition/mod.rs @@ -14,6 +14,8 @@ pub(crate) mod test_helpers { use apollo_federation::subgraph::typestate::Subgraph; use apollo_federation::supergraph::Satisfiable; use apollo_federation::supergraph::Supergraph; + use apollo_federation::supergraph::CompositionHint; + use std::iter::zip; pub(crate) struct ServiceDefinition<'a> { pub(crate) name: &'a str, @@ -62,6 +64,15 @@ pub(crate) mod test_helpers { compose(fed2_subgraphs) } + pub(crate) fn assert_hints_equal(actual_hints: &Vec, expected_hints: &Vec) { + if actual_hints.len() != expected_hints.len() { + panic!("Mismatched number of hints") + } + let zipped = zip(actual_hints, expected_hints); + zipped + .for_each(|(ch1, ch2)| assert!(ch1.code() == ch2.code() && ch1.message() == ch2.message())); + } + pub(crate) fn assert_composition_success( result: Result, Vec>, ) -> Supergraph { @@ -96,5 +107,6 @@ pub(crate) mod test_helpers { pub(crate) use test_helpers::ServiceDefinition; pub(crate) use test_helpers::assert_composition_success; +pub(crate) use test_helpers::assert_hints_equal; pub(crate) use test_helpers::compose_as_fed2_subgraphs; -pub(crate) use test_helpers::errors; +pub(crate) use test_helpers::errors; \ No newline at end of file From e4fd7ce323ef54719a2d2677fa84dd36550645bf Mon Sep 17 00:00:00 2001 From: Chidimma O Date: Mon, 27 Oct 2025 12:30:09 -0400 Subject: [PATCH 07/19] Reset visibility of argument_composition_strategies mod --- .../src/schema/argument_composition_strategies.rs | 4 ++-- apollo-federation/src/schema/mod.rs | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/apollo-federation/src/schema/argument_composition_strategies.rs b/apollo-federation/src/schema/argument_composition_strategies.rs index 60a3a26e11..31a28429d0 100644 --- a/apollo-federation/src/schema/argument_composition_strategies.rs +++ b/apollo-federation/src/schema/argument_composition_strategies.rs @@ -10,7 +10,7 @@ use crate::schema::FederationSchema; #[allow(dead_code)] #[derive(Clone, Copy)] -pub enum ArgumentCompositionStrategy { +pub(crate) enum ArgumentCompositionStrategy { Max, Min, // Sum, @@ -52,7 +52,7 @@ impl ArgumentCompositionStrategy { } } - pub fn name(&self) -> &str { + pub(crate) fn name(&self) -> &str { self.get_impl().name() } diff --git a/apollo-federation/src/schema/mod.rs b/apollo-federation/src/schema/mod.rs index ed887e9317..a020738f9d 100644 --- a/apollo-federation/src/schema/mod.rs +++ b/apollo-federation/src/schema/mod.rs @@ -64,7 +64,7 @@ use crate::schema::position::TypeDefinitionPosition; use crate::schema::position::UnionTypeDefinitionPosition; use crate::schema::subgraph_metadata::SubgraphMetadata; -pub mod argument_composition_strategies; +pub(crate) mod argument_composition_strategies; pub(crate) mod blueprint; pub(crate) mod definitions; pub(crate) mod directive_location; From da5c360bf87f52082041e3be70911ed76dad9a6d Mon Sep 17 00:00:00 2001 From: Taylor Ninesling Date: Mon, 27 Oct 2025 10:47:42 -0500 Subject: [PATCH 08/19] fix(composition): Give correct argument name to merger when merging directive arguments (#8487) --- .../src/merger/merge_directive.rs | 2 +- .../schema/argument_composition_strategies.rs | 2 +- .../type_and_directive_specification.rs | 3 +++ .../tests/composition/demand_control.rs | 19 ++++++++++++++++--- 4 files changed, 21 insertions(+), 5 deletions(-) diff --git a/apollo-federation/src/merger/merge_directive.rs b/apollo-federation/src/merger/merge_directive.rs index e6aab2927e..63fd917e58 100644 --- a/apollo-federation/src/merger/merge_directive.rs +++ b/apollo-federation/src/merger/merge_directive.rs @@ -181,7 +181,7 @@ impl Merger { }) .cloned() .collect_vec(); - if let Some(merged_value) = (merger.merge)(name, &values)? { + if let Some(merged_value) = (merger.merge)(&arg_def.name, &values)? { let merged_arg = Argument { name: arg_def.name.clone(), value: Node::new(merged_value), diff --git a/apollo-federation/src/schema/argument_composition_strategies.rs b/apollo-federation/src/schema/argument_composition_strategies.rs index 31a28429d0..50cb57f485 100644 --- a/apollo-federation/src/schema/argument_composition_strategies.rs +++ b/apollo-federation/src/schema/argument_composition_strategies.rs @@ -9,7 +9,7 @@ use apollo_compiler::ty; use crate::schema::FederationSchema; #[allow(dead_code)] -#[derive(Clone, Copy)] +#[derive(Clone, Copy, Debug)] pub(crate) enum ArgumentCompositionStrategy { Max, Min, diff --git a/apollo-federation/src/schema/type_and_directive_specification.rs b/apollo-federation/src/schema/type_and_directive_specification.rs index f871f4b5e4..857b44a46f 100644 --- a/apollo-federation/src/schema/type_and_directive_specification.rs +++ b/apollo-federation/src/schema/type_and_directive_specification.rs @@ -587,6 +587,9 @@ impl DirectiveSpecification { arg_strategies.len() == args.len(), "Invalid directive specification for @{name}: not all arguments define a composition strategy" ); + tracing::trace!( + "Creating argument merger for directive @{name} with strategies: {arg_strategies:?}", + ); argument_merger = Some(directive_argument_merger( name.clone(), args.to_vec(), diff --git a/apollo-federation/tests/composition/demand_control.rs b/apollo-federation/tests/composition/demand_control.rs index e0406b364e..e27be22883 100644 --- a/apollo-federation/tests/composition/demand_control.rs +++ b/apollo-federation/tests/composition/demand_control.rs @@ -1,7 +1,9 @@ +use apollo_compiler::coord; use apollo_federation::composition::compose; use apollo_federation::error::ErrorCode; use apollo_federation::subgraph::typestate::Initial; use apollo_federation::subgraph::typestate::Subgraph; +use test_log::test; fn subgraph_with_cost() -> Subgraph { Subgraph::parse( @@ -355,7 +357,6 @@ fn errors_when_subgraphs_use_different_names() { ) } -#[ignore = "until merge implementation completed"] #[test] fn hints_when_merging_cost_arguments() { let subgraph_a = Subgraph::parse( @@ -364,7 +365,7 @@ fn hints_when_merging_cost_arguments() { r#" extend schema @link(url: "https://specs.apollo.dev/link/v1.0") - @link(url: "https://specs.apollo.dev/cost/v0.1", import: ["@cost"]) + @link(url: "https://specs.apollo.dev/federation/v2.9", import: ["@cost", "@shareable"]) type Query { sharedWithCost: Int @shareable @cost(weight: 5) @@ -378,7 +379,7 @@ fn hints_when_merging_cost_arguments() { r#" extend schema @link(url: "https://specs.apollo.dev/link/v1.0") - @link(url: "https://specs.apollo.dev/cost/v0.1", import: ["@cost"]) + @link(url: "https://specs.apollo.dev/federation/v2.9", import: ["@cost", "@shareable"]) type Query { sharedWithCost: Int @shareable @cost(weight: 10) @@ -388,6 +389,7 @@ fn hints_when_merging_cost_arguments() { .unwrap(); let result = compose(vec![subgraph_a, subgraph_b]).unwrap(); + /* TODO: Re-enable once FED-693 is merged assert_eq!(result.hints().len(), 1); let hint = result.hints().first().unwrap(); assert_eq!(hint.code(), "MERGED_NON_REPEATABLE_DIRECTIVE_ARGUMENTS"); @@ -395,6 +397,17 @@ fn hints_when_merging_cost_arguments() { hint.message(), r#"Directive @cost is applied to "Query.sharedWithCost" in multiple subgraphs with different arguments. Merging strategies used by arguments: { "weight": MAX }""# ); + */ + + let shared_with_cost = coord!(Query.sharedWithCost) + .lookup_field(result.schema().schema()) + .unwrap(); + let cost_directive = shared_with_cost + .directives + .iter() + .find(|d| d.name == "cost") + .expect("Expected @cost directive to be present on Query.sharedWithCost"); + assert_eq!(cost_directive.to_string(), r#"@cost(weight: 10)"#); } #[ignore = "until merge implementation completed"] From 21b883fc7838ba1de02f6278749df314d6e7ba7f Mon Sep 17 00:00:00 2001 From: Chidimma O Date: Tue, 28 Oct 2025 17:16:50 -0400 Subject: [PATCH 09/19] Update tests to use public-facing directives. Most argument merging strategies are not used by public-facing directives, so the following strategies have been skipped: - min - intersection - nullable_and - nullable_max - nullable_union --- .../directive_argument_merge_strategies.rs | 446 +++++++----------- apollo-federation/tests/composition/mod.rs | 92 ++-- 2 files changed, 213 insertions(+), 325 deletions(-) diff --git a/apollo-federation/tests/composition/directive_argument_merge_strategies.rs b/apollo-federation/tests/composition/directive_argument_merge_strategies.rs index cea20b7e2d..4ef342a592 100644 --- a/apollo-federation/tests/composition/directive_argument_merge_strategies.rs +++ b/apollo-federation/tests/composition/directive_argument_merge_strategies.rs @@ -1,324 +1,224 @@ -use std::iter::zip; - -use apollo_compiler::ast; -use apollo_compiler::schema; -use apollo_federation::schema::argument_composition_strategies::ArgumentCompositionStrategy; -use apollo_federation::supergraph::CompositionHint; - use super::ServiceDefinition; -use super::assert_composition_success; +use super::assert_hints_equal; use super::compose_as_fed2_subgraphs; -use super::errors; - -// Helper function to create directive strings from applied directives using schema::DirectiveList -fn directive_strings_schema(directives: &schema::DirectiveList, target: &str) -> Vec { - directives - .iter() - .map(|dir| dir.to_string()) - .filter(|s| s.contains(target)) - .collect() -} - -// Helper function to create directive strings from applied directives using ast::DirectiveList -fn directive_strings_ast(directives: &ast::DirectiveList, target: &str) -> Vec { - directives - .iter() - .map(|dir| dir.to_string()) - .filter(|s| s.contains(target)) - .collect() -} - -fn assert_hints_equal(actual_hints: &Vec, expected_hints: &Vec) { - if actual_hints.len() != expected_hints.len() { - panic!("Mismatched number of hints") - } - let zipped = zip(actual_hints, expected_hints); - zipped - .for_each(|(ch1, ch2)| assert!(ch1.code() == ch2.code() && ch1.message() == ch2.message())); -} +use super::print_sdl; #[cfg(test)] mod tests { - use core::str; - use std::collections::HashMap; - use std::sync::LazyLock; + use apollo_compiler::coord; + use apollo_federation::supergraph::CompositionHint; + use test_log::test; use super::*; + use crate::composition::assert_composition_errors; - // Test cases for different argument composition strategies - struct CompositionStrategyTestCase<'a> { - name: &'a str, - composition_strategy: ArgumentCompositionStrategy, - arg_values_s1: HashMap<&'a str, &'a str>, - arg_values_s2: HashMap<&'a str, &'a str>, - result_values: HashMap<&'a str, &'a str>, - } + /* The following argument merging strategies are not currently being used by any + public-facing directives and thus are not represented in this set of tests: + - min + - intersection + - nullable_and + - nullable_max + - nullable_union + */ - static TEST_CASES: LazyLock> = LazyLock::new(|| { - HashMap::from([ - ( - "max", - CompositionStrategyTestCase { - name: "max", - composition_strategy: ArgumentCompositionStrategy::Max, - arg_values_s1: HashMap::from([("t", "3"), ("k", "1")]), - arg_values_s2: HashMap::from([("t", "2"), ("k", "5"), ("b", "4")]), - result_values: HashMap::from([("t", "3"), ("k", "5"), ("b", "4")]), - }, - ), - ( - "min", - CompositionStrategyTestCase { - name: "min", - composition_strategy: ArgumentCompositionStrategy::Min, - arg_values_s1: HashMap::from([("t", "3"), ("k", "1")]), - arg_values_s2: HashMap::from([("t", "2"), ("k", "5"), ("b", "4")]), - result_values: HashMap::from([("t", "2"), ("k", "1"), ("b", "4")]), - }, - ), - ( - "intersection", - CompositionStrategyTestCase { - name: "intersection", - composition_strategy: ArgumentCompositionStrategy::Intersection, - arg_values_s1: HashMap::from([("t", r#"["foo", "bar"]"#), ("k", r#"[]"#)]), - arg_values_s2: HashMap::from([ - ("t", r#"["foo"]"#), - ("k", r#"["v1", "v2"]"#), - ("b", r#"["x"]"#), - ]), - result_values: HashMap::from([ - ("t", r#"["foo"]"#), - ("k", r#"[]"#), - ("b", r#"["x"]"#), - ]), - }, - ), - ( - "union", - CompositionStrategyTestCase { - name: "union", - composition_strategy: ArgumentCompositionStrategy::Union, - arg_values_s1: HashMap::from([("t", r#"["foo", "bar"]"#), ("k", r#"[]"#)]), - arg_values_s2: HashMap::from([ - ("t", r#"["foo"]"#), - ("k", r#"["v1", "v2"]"#), - ("b", r#"["x"]"#), - ]), - result_values: HashMap::from([ - ("t", r#"["foo", "bar"]"#), - ("k", r#"["v1", "v2"]"#), - ("b", r#"["x"]"#), - ]), - }, - ), - ( - "nullable_and", - CompositionStrategyTestCase { - name: "nullable_and", - composition_strategy: ArgumentCompositionStrategy::NullableAnd, - arg_values_s1: HashMap::from([("t", "true"), ("k", "true")]), - arg_values_s2: HashMap::from([("t", "null"), ("k", "false"), ("b", "false")]), - result_values: HashMap::from([("t", "true"), ("k", "false"), ("b", "false")]), - }, - ), - ( - "nullable_max", - CompositionStrategyTestCase { - name: "nullable_max", - composition_strategy: ArgumentCompositionStrategy::NullableMax, - arg_values_s1: HashMap::from([("t", "3"), ("k", "1")]), - arg_values_s2: HashMap::from([("t", "2"), ("k", "null"), ("b", "null")]), - result_values: HashMap::from([("t", "3"), ("k", "1"), ("b", "null")]), - }, - ), - ( - "nullable_union", - CompositionStrategyTestCase { - name: "nullable_union", - composition_strategy: ArgumentCompositionStrategy::NullableUnion, - arg_values_s1: HashMap::from([("t", r#"["foo", "bar"]"#), ("k", r#"[]"#)]), - arg_values_s2: HashMap::from([ - ("t", r#"["foo"]"#), - ("k", r#"["v1", "v2"]"#), - ("b", r#"["x"]"#), - ]), - result_values: HashMap::from([ - ("t", r#"["foo", "bar"]"#), - ("k", r#"["v1", "v2"]"#), - ("b", r#"["x"]"#), - ]), - }, - ), - ]) - }); + #[test] + fn works_for_max_argument_merge_strategy() { + // NOTE: @cost uses the MAX strategy for merging arguments, + // So we'll use it as a proxy for the private @max directive - fn test_composition_of_directive_with_non_trivial_argument_strategies( - test_case: &CompositionStrategyTestCase, - ) { let subgraph1 = ServiceDefinition { name: "Subgraph1", - type_defs: &format!( - r#" - extend schema @link(url: "https://specs.apollo.dev/{}/v0.1") - - type Query {{ + type_defs: r#" + type Query { t: T - }} - + } + type T - @key(fields: "k") - @{}(value: {}) - {{ - k: ID @{}(value: {}) - }} + @key(fields: "k") + @cost(weight: 3) + { + k: ID @cost(weight: 1) + } "#, - test_case.name, - test_case.name, - test_case.arg_values_s1["t"], - test_case.name, - test_case.arg_values_s1["k"] - ), }; let subgraph2 = ServiceDefinition { name: "Subgraph2", - type_defs: &format!( - r#" - extend schema @link(url: "https://specs.apollo.dev/{}/v0.1") - - type T - @key(fields: "k") - @{}(value: {}) - {{ - k: ID @{}(value: {}) + type_defs: r#" + type T + @key(fields: "k") + @cost(weight: 2) + { + k: ID @cost(weight: 5) a: Int - b: String @{}(value: {}) - }} + b: String @cost(weight: 4) + } "#, - test_case.name, - test_case.name, - test_case.arg_values_s2["t"], - test_case.name, - test_case.arg_values_s2["k"], - test_case.name, - test_case.arg_values_s2["b"] - ), }; let result = compose_as_fed2_subgraphs(&[subgraph1, subgraph2]); - let result_sg = assert_composition_success(result); + let result_sg = result.expect("Composition should succeed"); // Check expected hints let expected_hints = vec![ CompositionHint { code: String::from("MERGED_NON_REPEATABLE_DIRECTIVE_ARGUMENTS"), - message: format!( - "Directive @{} is applied to \"T\" in multiple subgraphs with different arguments. Merging strategies used by arguments: {{ \"value\": {} }}", - test_case.name, - test_case.composition_strategy.name() + message: String::from( + r#"Directive @cost is applied to "T" in multiple subgraphs with different arguments. Merging strategies used by arguments: { weight: MAX }"#, ), locations: Vec::new(), }, CompositionHint { code: String::from("MERGED_NON_REPEATABLE_DIRECTIVE_ARGUMENTS"), - message: format!( - "Directive @{} is applied to \"T.k\" in multiple subgraphs with different arguments. Merging strategies used by arguments: {{ \"value\": {} }}", - test_case.name, - test_case.composition_strategy.name() + message: String::from( + r#"Directive @cost is applied to "T.k" in multiple subgraphs with different arguments. Merging strategies used by arguments: { weight: MAX }"#, ), locations: Vec::new(), }, ]; assert_hints_equal(result_sg.hints(), &expected_hints); - // Check expected directive strings + // Check merging using MAX succeeded let schema = result_sg.schema().schema(); - assert_eq!( - directive_strings_schema(&schema.schema_definition.directives, test_case.name), - vec![format!( - r#"@link(url: "https://specs.apollo.dev/{}/v0.1")"#, - test_case.name - )] - ); + let sdl = print_sdl(schema); + assert!(sdl.contains(r#"@link(url: "https://specs.apollo.dev/cost/v0.1")"#)); - let t = schema.get_object("T").unwrap(); - assert_eq!( - directive_strings_schema(&t.directives, test_case.name), - [format!( - r#"@{}(value: {})"#, - test_case.name, test_case.result_values["t"] - )] - ); - assert_eq!( - directive_strings_ast(&t.fields.get("k").unwrap().directives, test_case.name), - [format!( - r#"@{}(value: {})"#, - test_case.name, test_case.result_values["k"] - )] - ); - assert_eq!( - directive_strings_ast(&t.fields.get("b").unwrap().directives, test_case.name), - [format!( - r#"@{}(value: {})"#, - test_case.name, test_case.result_values["b"] - )] - ); - } + let t = coord!(T) + .lookup(schema) + .expect("T should be defined on the supergraph"); + let t_cost_directive = t + .directives() + .iter() + .find(|d| d.name == "cost") + .expect("@cost directive should be present on T"); + assert_eq!(t_cost_directive.to_string(), r#"@cost(weight: 3)"#); - #[test] - #[ignore = "Directive argument merge strategies not yet implemented"] - fn works_for_max() { - let test_case = TEST_CASES.get("max").expect("Test case not found"); - test_composition_of_directive_with_non_trivial_argument_strategies(test_case); - } + let k = coord!(T.k) + .lookup_field(schema) + .expect("T.k should be defined on the supergraph"); + let k_cost_directive = k + .directives + .iter() + .find(|d| d.name == "cost") + .expect("@cost directive should be present on T.k"); + assert_eq!(k_cost_directive.to_string(), r#"@cost(weight: 5)"#); - #[test] - #[ignore = "Directive argument merge strategies not yet implemented"] - fn works_for_min() { - let test_case = TEST_CASES.get("min").expect("Test case not found"); - test_composition_of_directive_with_non_trivial_argument_strategies(test_case); + let b = coord!(T.b) + .lookup_field(schema) + .expect("T.b should be defined on the supergraph"); + let b_cost_directive = b + .directives + .iter() + .find(|d| d.name == "cost") + .expect("@cost directive should be present on T.b"); + assert_eq!(b_cost_directive.to_string(), r#"@cost(weight: 4)"#); } #[test] - #[ignore = "Directive argument merge strategies not yet implemented"] - fn works_for_intersection() { - let test_case = TEST_CASES.get("intersection").expect("Test case not found"); - test_composition_of_directive_with_non_trivial_argument_strategies(test_case); - } + fn works_for_union_argument_merge_strategy() { + // NOTE: @requiresScopes uses the UNION strategy for merging arguments, + // So we'll use it as a proxy for the private @union directive - #[test] - #[ignore = "Directive argument merge strategies not yet implemented"] - fn works_for_union() { - let test_case = TEST_CASES.get("union").expect("Test case not found"); - test_composition_of_directive_with_non_trivial_argument_strategies(test_case); - } + let subgraph1 = ServiceDefinition { + name: "Subgraph1", + type_defs: r#" + type Query { + t: T! + } + + type T + @key(fields: "k") + @requiresScopes(scopes: ["foo", "bar"]) + { + k: ID @requiresScopes(scopes: []) + } + "#, + }; - #[test] - #[ignore = "Directive argument merge strategies not yet implemented"] - fn works_for_nullable_and() { - let test_case = TEST_CASES.get("nullable_and").expect("Test case not found"); - test_composition_of_directive_with_non_trivial_argument_strategies(test_case); - } + let subgraph2 = ServiceDefinition { + name: "Subgraph2", + type_defs: r#" + type T + @key(fields: "k") + @requiresScopes(scopes: ["foo"]) + { + k: ID @requiresScopes(scopes: ["v1", "v2"]) + a: Int + b: String @requiresScopes(scopes: ["x"]) + } + "#, + }; - #[test] - #[ignore = "Directive argument merge strategies not yet implemented"] - fn works_for_nullable_max() { - let test_case = TEST_CASES.get("nullable_max").expect("Test case not found"); - test_composition_of_directive_with_non_trivial_argument_strategies(test_case); - } + let result = compose_as_fed2_subgraphs(&[subgraph1, subgraph2]); + let result_sg = result.expect("Composition should succeed"); - #[test] - #[ignore = "Directive argument merge strategies not yet implemented"] - fn works_for_nullable_union() { - let test_case = TEST_CASES - .get("nullable_union") - .expect("Test case not found"); - test_composition_of_directive_with_non_trivial_argument_strategies(test_case); + // Check expected hints + let expected_hints = vec![ + CompositionHint { + code: String::from("MERGED_NON_REPEATABLE_DIRECTIVE_ARGUMENTS"), + message: String::from( + r#"Directive @requiresScopes is applied to "T" in multiple subgraphs with different arguments. Merging strategies used by arguments: { scopes: UNION }"#, + ), + locations: Vec::new(), + }, + CompositionHint { + code: String::from("MERGED_NON_REPEATABLE_DIRECTIVE_ARGUMENTS"), + message: String::from( + r#"Directive @requiresScopes is applied to "T.k" in multiple subgraphs with different arguments. Merging strategies used by arguments: { scopes: UNION }"#, + ), + locations: Vec::new(), + }, + ]; + assert_hints_equal(result_sg.hints(), &expected_hints); + + // Check merging using UNION succeeded + let schema = result_sg.schema().schema(); + let sdl = print_sdl(schema); + assert!(sdl.contains( + r#"@link(url: "https://specs.apollo.dev/requiresScopes/v0.1", for: SECURITY)"# + )); + + let t = coord!(T) + .lookup(schema) + .expect("T should be defined on the supergraph"); + let t_requires_scopes_directive = t + .directives() + .iter() + .find(|d| d.name == "requiresScopes") + .expect("@requiresScopes directive should be present on T"); + assert_eq!( + t_requires_scopes_directive.to_string(), + r#"@requiresScopes(scopes: ["foo", "bar"])"# + ); + + let k = coord!(T.k) + .lookup_field(schema) + .expect("T.k should be defined on the supergraph"); + let k_requires_scopes_directive = k + .directives + .iter() + .find(|d| d.name == "requiresScopes") + .expect("@requiresScopes directive should be present on T.k"); + assert_eq!( + k_requires_scopes_directive.to_string(), + r#"@requiresScopes(scopes: ["v1", "v2"])"# + ); + + let b = coord!(T.b) + .lookup_field(schema) + .expect("T.b should be defined on the supergraph"); + let b_requires_scopes_directive = b + .directives + .iter() + .find(|d| d.name == "requiresScopes") + .expect("@requiresScopes directive should be present on T.b"); + assert_eq!( + b_requires_scopes_directive.to_string(), + r#"@requiresScopes(scopes: ["x"])"# + ) } #[test] - #[ignore = "Directive argument merge strategies not yet implemented"] fn errors_when_declaring_strategy_that_does_not_match_the_argument_type() { let subgraph1 = ServiceDefinition { name: "Subgraph1", @@ -347,12 +247,10 @@ mod tests { }; let result = compose_as_fed2_subgraphs(&[subgraph1, subgraph2]); - let errs = errors(&result); - assert_eq!( - errs.iter().map(|(_, message)| message).collect::>(), - [ - r#"Invalid composition strategy MAX for argument @foo(value:) of type String; MAX only supports type(s) Int!"# - ] - ); + let expected_errors = [( + "DIRECTIVE_DEFINITION_INVALID", + r#"Invalid composition strategy MAX for argument @foo(value:) of type String; MAX only supports type(s) Int!"#, + )]; + assert_composition_errors(&result, &expected_errors); } } diff --git a/apollo-federation/tests/composition/mod.rs b/apollo-federation/tests/composition/mod.rs index b24f918896..725594f6d3 100644 --- a/apollo-federation/tests/composition/mod.rs +++ b/apollo-federation/tests/composition/mod.rs @@ -14,22 +14,22 @@ mod demand_control; mod directive_argument_merge_strategies; // TODO: remove #[ignore] from tests once all fns called by Merger::merge() are implemented mod external; -mod hints; mod override_directive; mod subscription; mod supergraph_reversibility; mod validation_errors; pub(crate) mod test_helpers { + use std::iter::zip; + use apollo_federation::ValidFederationSubgraphs; use apollo_federation::composition::compose; use apollo_federation::error::CompositionError; use apollo_federation::error::FederationError; use apollo_federation::subgraph::typestate::Subgraph; + use apollo_federation::supergraph::CompositionHint; use apollo_federation::supergraph::Satisfiable; use apollo_federation::supergraph::Supergraph; - use apollo_federation::supergraph::CompositionHint; - use std::iter::zip; pub(crate) struct ServiceDefinition<'a> { pub(crate) name: &'a str, @@ -92,33 +92,42 @@ pub(crate) mod test_helpers { expected_errors: &[(&str, &str)], ) { let errors = result.as_ref().expect_err("Expected composition to fail"); - let error_strings: Vec = errors.iter().map(|e| e.to_string()).collect(); + let error_strings: Vec<(String, String)> = errors + .iter() + .map(|e| (e.code().definition().code().to_string(), e.to_string())) + .collect(); // Verify error count matches expectations assert_eq!( - errors.len(), expected_errors.len(), - "Expected {} errors but got {}: {:?}", + errors.len(), + "Expected {} errors but got {}:\nEXPECTED:\n{:?}\nACTUAL:\n{:?}", expected_errors.len(), errors.len(), + expected_errors, error_strings ); // Verify each expected error code and message - for (i, (_expected_code, expected_message)) in expected_errors.iter().enumerate() { - let error = &errors[i]; + for (i, (expected_code, expected_message)) in expected_errors.iter().enumerate() { + let (error_code, error_str) = &error_strings[i]; - // Check error code (assuming CompositionError has a code method or field) - // This will need to be implemented based on the actual CompositionError structure - // For now, we'll validate the error message contains the expected content - let error_str = error.to_string(); + // Check error message assert!( error_str.contains(expected_message), - "Error {} does not contain expected message.\nExpected: {}\nActual: {}", + "Error at index {} does not contain expected message.\nEXPECTED:\n{}\nACTUAL:\n{}", i, expected_message, error_str ); + // Check error code + assert!( + error_code.contains(expected_code), + "Error at index {} does not contain expected code.\nEXPECTED:\n{}\nACTUAL:\n{}", + i, + expected_code, + error_code + ); } } @@ -133,51 +142,32 @@ pub(crate) mod test_helpers { api_supergraph.extract_subgraphs() } - pub(crate) fn assert_hints_equal(actual_hints: &Vec, expected_hints: &Vec) { + pub(crate) fn assert_hints_equal( + actual_hints: &Vec, + expected_hints: &Vec, + ) { if actual_hints.len() != expected_hints.len() { - panic!("Mismatched number of hints") + panic!( + "Mismatched number of hints: expected {} hint(s) but got {} hint(s)\nEXPECTED:\n{expected_hints:#?}\nACTUAL:\n{actual_hints:#?}", + expected_hints.len(), + actual_hints.len() + ) } let zipped = zip(actual_hints, expected_hints); - zipped - .for_each(|(ch1, ch2)| assert!(ch1.code() == ch2.code() && ch1.message() == ch2.message())); - } - - pub(crate) fn assert_composition_success( - result: Result, Vec>, - ) -> Supergraph { - match result { - Ok(supergraph) => supergraph, - Err(errors) => { - panic!( - "Expected successful composition, but got errors: {:?}", - errors - .iter() - .map(|e| e.to_string()) - .collect::>() - .join("\n\n") - ); - } - } - } - - // Return a vec of error codes and error messages as strings - pub(crate) fn errors( - result: &Result, Vec>, - ) -> Vec<(String, String)> { - match result { - Ok(_) => panic!("Expected an error, but got a successful composition"), - Err(err) => err - .iter() - .map(|e| (e.code().definition().code().to_string(), e.to_string())) - .collect(), - } + zipped.for_each(|(ch1, ch2)| { + assert!( + ch1.code() == ch2.code() && ch1.message() == ch2.message(), + "EXPECTED:\n{:#?}\nACTUAL:\n{:#?}", + expected_hints, + actual_hints + ) + }); } } pub(crate) use test_helpers::ServiceDefinition; -pub(crate) use test_helpers::assert_composition_success; -pub(crate) use test_helpers::assert_hints_equal; pub(crate) use test_helpers::assert_composition_errors; +pub(crate) use test_helpers::assert_hints_equal; pub(crate) use test_helpers::compose_as_fed2_subgraphs; -pub(crate) use test_helpers::errors;pub(crate) use test_helpers::extract_subgraphs_from_supergraph_result; +pub(crate) use test_helpers::extract_subgraphs_from_supergraph_result; pub(crate) use test_helpers::print_sdl; From d5da1fd3929cb9aad3fffaa4a293f88f5d997afa Mon Sep 17 00:00:00 2001 From: Chidimma O Date: Wed, 29 Oct 2025 15:24:32 -0400 Subject: [PATCH 10/19] Remove test for mismatched composition strategy and argument type Doing this in favor of converting that test into a unit test, as it is not trivial in Rust to define a custom directive from an integration test. This work will be associated with a new ticket: [FED-879](https://apollographql.atlassian.net/browse/FED-879) --- .../directive_argument_merge_strategies.rs | 37 ------------------- 1 file changed, 37 deletions(-) diff --git a/apollo-federation/tests/composition/directive_argument_merge_strategies.rs b/apollo-federation/tests/composition/directive_argument_merge_strategies.rs index 4ef342a592..d59abe70dc 100644 --- a/apollo-federation/tests/composition/directive_argument_merge_strategies.rs +++ b/apollo-federation/tests/composition/directive_argument_merge_strategies.rs @@ -10,7 +10,6 @@ mod tests { use test_log::test; use super::*; - use crate::composition::assert_composition_errors; /* The following argument merging strategies are not currently being used by any public-facing directives and thus are not represented in this set of tests: @@ -217,40 +216,4 @@ mod tests { r#"@requiresScopes(scopes: ["x"])"# ) } - - #[test] - fn errors_when_declaring_strategy_that_does_not_match_the_argument_type() { - let subgraph1 = ServiceDefinition { - name: "Subgraph1", - type_defs: r#" - extend schema @link(url: "https://specs.apollo.dev/foo/v0.1") - - type Query { - t: T - } - - type T { - v: String @foo(value: "bar") - } - "#, - }; - - let subgraph2 = ServiceDefinition { - name: "Subgraph2", - type_defs: r#" - extend schema @link(url: "https://specs.apollo.dev/foo/v0.1") - - type T { - v: String @foo(value: "bar") - } - "#, - }; - - let result = compose_as_fed2_subgraphs(&[subgraph1, subgraph2]); - let expected_errors = [( - "DIRECTIVE_DEFINITION_INVALID", - r#"Invalid composition strategy MAX for argument @foo(value:) of type String; MAX only supports type(s) Int!"#, - )]; - assert_composition_errors(&result, &expected_errors); - } } From 7c414fa20ddb5804cb3bea4511eecd00e58f75cc Mon Sep 17 00:00:00 2001 From: Chidimma O Date: Thu, 30 Oct 2025 16:23:32 -0400 Subject: [PATCH 11/19] Add test for nullable_max strategy --- .../directive_argument_merge_strategies.rs | 109 +++++++++++++++++- 1 file changed, 106 insertions(+), 3 deletions(-) diff --git a/apollo-federation/tests/composition/directive_argument_merge_strategies.rs b/apollo-federation/tests/composition/directive_argument_merge_strategies.rs index d59abe70dc..8325ee47a0 100644 --- a/apollo-federation/tests/composition/directive_argument_merge_strategies.rs +++ b/apollo-federation/tests/composition/directive_argument_merge_strategies.rs @@ -16,14 +16,13 @@ mod tests { - min - intersection - nullable_and - - nullable_max - nullable_union */ #[test] fn works_for_max_argument_merge_strategy() { // NOTE: @cost uses the MAX strategy for merging arguments, - // So we'll use it as a proxy for the private @max directive + // So we'll use it as a proxy for the @max directive let subgraph1 = ServiceDefinition { name: "Subgraph1", @@ -116,7 +115,7 @@ mod tests { #[test] fn works_for_union_argument_merge_strategy() { // NOTE: @requiresScopes uses the UNION strategy for merging arguments, - // So we'll use it as a proxy for the private @union directive + // So we'll use it as a proxy for the @union directive let subgraph1 = ServiceDefinition { name: "Subgraph1", @@ -216,4 +215,108 @@ mod tests { r#"@requiresScopes(scopes: ["x"])"# ) } + + #[test] + fn works_for_nullable_max_argument_merge_strategy() { + // NOTE: @listSize(assumedSize:) uses the NULLABLE_MAX strategy for merging arguments, + // So we'll use it as a proxy for the @nullable_max directive + + let subgraph1 = ServiceDefinition { + name: "Subgraph1", + type_defs: r#" + type Query { + t: [T!] @shareable @listSize(assumedSize: 20) + } + + type T @key(fields: "k") { + k: [ID] @listSize(assumedSize: 1) + } + "#, + }; + + let subgraph2 = ServiceDefinition { + name: "Subgraph2", + type_defs: r#" + type Query { + t: [T!] @shareable @listSize(assumedSize: 10) + } + + type T @key(fields: "k") { + k: [ID] @listSize(assumedSize: 3) + a: String + b: [Int] @listSize(assumedSize: null) + } + "#, + }; + + let result = compose_as_fed2_subgraphs(&[subgraph1, subgraph2]); + let result_sg = result.expect("Composition should succeed"); + + // Check expected hints + let expected_hints = vec![ + CompositionHint { + code: String::from("MERGED_NON_REPEATABLE_DIRECTIVE_ARGUMENTS"), + message: String::from( + r#"Directive @listSize is applied to "Query.t" in multiple subgraphs with different arguments. Merging strategies used by arguments: { assumedSize: NULLABLE_MAX, slicingArguments: NULLABLE_UNION, sizedFields: NULLABLE_UNION, requireOneSlicingArgument: NULLABLE_AND }"#, + ), + locations: Vec::new(), + }, + CompositionHint { + code: String::from("MERGED_NON_REPEATABLE_DIRECTIVE_ARGUMENTS"), + message: String::from( + r#"Directive @listSize is applied to "T.k" in multiple subgraphs with different arguments. Merging strategies used by arguments: { assumedSize: NULLABLE_MAX, slicingArguments: NULLABLE_UNION, sizedFields: NULLABLE_UNION, requireOneSlicingArgument: NULLABLE_AND }"#, + ), + locations: Vec::new(), + }, + ]; + assert_hints_equal(result_sg.hints(), &expected_hints); + + // Check merging using NULLABLE_AND succeeded + let schema = result_sg.schema().schema(); + let sdl = print_sdl(schema); + assert!( + sdl.contains( + r#"@link(url: "https://specs.apollo.dev/cost/v0.1", import: ["listSize"])"# + ) + ); + + let t = coord!(Query.t) + .lookup_field(schema) + .expect("Query.t should be defined on the supergraph"); + let t_requires_scopes_directive = t + .directives + .iter() + .find(|d| d.name == "listSize") + .expect("@listSize directive should be present on T"); + assert_eq!( + t_requires_scopes_directive.to_string(), + r#"@listSize(assumedSize: 20, requireOneSlicingArgument: true)"# + ); + + let k = coord!(T.k) + .lookup_field(schema) + .expect("T.k should be defined on the supergraph"); + let k_requires_scopes_directive = k + .directives + .iter() + .find(|d| d.name == "listSize") + .expect("@listSize directive should be present on T.k"); + assert_eq!( + k_requires_scopes_directive.to_string(), + r#"@listSize(assumedSize: 3, requireOneSlicingArgument: true)"# + ); + + let b = coord!(T.b) + .lookup_field(schema) + .expect("T.b should be defined on the supergraph"); + let b_requires_scopes_directive = b + .directives + .iter() + .find(|d| d.name == "listSize") + .expect("@listSize directive should be present on T.b"); + assert_eq!( + b_requires_scopes_directive.to_string(), + r#"@listSize(assumedSize: null, requireOneSlicingArgument: true)"# + ); + } } From 36961f6a6e742d0dae100c9210bee74302e503b5 Mon Sep 17 00:00:00 2001 From: Chidimma O Date: Thu, 30 Oct 2025 16:55:57 -0400 Subject: [PATCH 12/19] Add test for nullable_and strategy --- .../directive_argument_merge_strategies.rs | 146 ++++++++++++++++-- 1 file changed, 130 insertions(+), 16 deletions(-) diff --git a/apollo-federation/tests/composition/directive_argument_merge_strategies.rs b/apollo-federation/tests/composition/directive_argument_merge_strategies.rs index 8325ee47a0..74ba27e8dd 100644 --- a/apollo-federation/tests/composition/directive_argument_merge_strategies.rs +++ b/apollo-federation/tests/composition/directive_argument_merge_strategies.rs @@ -15,14 +15,12 @@ mod tests { public-facing directives and thus are not represented in this set of tests: - min - intersection - - nullable_and - nullable_union */ #[test] fn works_for_max_argument_merge_strategy() { - // NOTE: @cost uses the MAX strategy for merging arguments, - // So we'll use it as a proxy for the @max directive + // NOTE: @cost uses the MAX strategy for merging arguments let subgraph1 = ServiceDefinition { name: "Subgraph1", @@ -30,7 +28,7 @@ mod tests { type Query { t: T } - + type T @key(fields: "k") @cost(weight: 3) @@ -43,7 +41,7 @@ mod tests { let subgraph2 = ServiceDefinition { name: "Subgraph2", type_defs: r#" - type T + type T @key(fields: "k") @cost(weight: 2) { @@ -114,8 +112,7 @@ mod tests { #[test] fn works_for_union_argument_merge_strategy() { - // NOTE: @requiresScopes uses the UNION strategy for merging arguments, - // So we'll use it as a proxy for the @union directive + // NOTE: @requiresScopes uses the UNION strategy for merging arguments let subgraph1 = ServiceDefinition { name: "Subgraph1", @@ -123,10 +120,10 @@ mod tests { type Query { t: T! } - - type T + + type T @key(fields: "k") - @requiresScopes(scopes: ["foo", "bar"]) + @requiresScopes(scopes: ["foo", "bar"]) { k: ID @requiresScopes(scopes: []) } @@ -135,10 +132,10 @@ mod tests { let subgraph2 = ServiceDefinition { name: "Subgraph2", - type_defs: r#" - type T + type_defs: r#" + type T @key(fields: "k") - @requiresScopes(scopes: ["foo"]) + @requiresScopes(scopes: ["foo"]) { k: ID @requiresScopes(scopes: ["v1", "v2"]) a: Int @@ -216,10 +213,127 @@ mod tests { ) } + #[test] + fn works_for_nullable_and_argument_merge_strategy() { + // NOTE: @listSize(requireOneSlicingArgument:) is merged using the NULLABLE_AND strategy + + let subgraph1 = ServiceDefinition { + name: "Subgraph1", + type_defs: r#" + type Query { + t: [T!] @shareable @listSize(requireOneSlicingArgument: false) + } + + type T @key(fields: "k") { + k: [ID] @listSize(requireOneSlicingArgument: true) + c: [Int]! @shareable @listSize(requireOneSlicingArgument: null) + } + "#, + }; + + let subgraph2 = ServiceDefinition { + name: "Subgraph2", + type_defs: r#" + type Query { + t: [T!] @shareable @listSize(requireOneSlicingArgument: true) + } + + type T @key(fields: "k") { + k: [ID] @listSize(requireOneSlicingArgument: null) + a: String + b: [Int] @listSize(requireOneSlicingArgument: false) + c: [Int]! @shareable @listSize(requireOneSlicingArgument: null) + } + "#, + }; + + let result = compose_as_fed2_subgraphs(&[subgraph1, subgraph2]); + let result_sg = result.expect("Composition should succeed"); + + // Check expected hints + let expected_hints = vec![ + CompositionHint { + code: String::from("MERGED_NON_REPEATABLE_DIRECTIVE_ARGUMENTS"), + message: String::from( + r#"Directive @listSize is applied to "Query.t" in multiple subgraphs with different arguments. Merging strategies used by arguments: { assumedSize: NULLABLE_MAX, slicingArguments: NULLABLE_UNION, sizedFields: NULLABLE_UNION, requireOneSlicingArgument: NULLABLE_AND }"#, + ), + locations: Vec::new(), + }, + CompositionHint { + code: String::from("MERGED_NON_REPEATABLE_DIRECTIVE_ARGUMENTS"), + message: String::from( + r#"Directive @listSize is applied to "T.k" in multiple subgraphs with different arguments. Merging strategies used by arguments: { assumedSize: NULLABLE_MAX, slicingArguments: NULLABLE_UNION, sizedFields: NULLABLE_UNION, requireOneSlicingArgument: NULLABLE_AND }"#, + ), + locations: Vec::new(), + }, + ]; + assert_hints_equal(result_sg.hints(), &expected_hints); + + // Check merging using NULLABLE_AND succeeded + let schema = result_sg.schema().schema(); + let sdl = print_sdl(schema); + assert!( + sdl.contains( + r#"@link(url: "https://specs.apollo.dev/cost/v0.1", import: ["listSize"])"# + ) + ); + + let t = coord!(Query.t) + .lookup_field(schema) + .expect("Query.t should be defined on the supergraph"); + let t_requires_scopes_directive = t + .directives + .iter() + .find(|d| d.name == "listSize") + .expect("@listSize directive should be present on T"); + assert_eq!( + t_requires_scopes_directive.to_string(), + r#"@listSize(requireOneSlicingArgument: false)"# + ); + + let k = coord!(T.k) + .lookup_field(schema) + .expect("T.k should be defined on the supergraph"); + let k_requires_scopes_directive = k + .directives + .iter() + .find(|d| d.name == "listSize") + .expect("@listSize directive should be present on T.k"); + assert_eq!( + k_requires_scopes_directive.to_string(), + r#"@listSize(requireOneSlicingArgument: true)"# + ); + + let b = coord!(T.b) + .lookup_field(schema) + .expect("T.b should be defined on the supergraph"); + let b_requires_scopes_directive = b + .directives + .iter() + .find(|d| d.name == "listSize") + .expect("@listSize directive should be present on T.b"); + assert_eq!( + b_requires_scopes_directive.to_string(), + r#"@listSize(requireOneSlicingArgument: false)"# + ); + + let c = coord!(T.c) + .lookup_field(schema) + .expect("T.b should be defined on the supergraph"); + let b_requires_scopes_directive = c + .directives + .iter() + .find(|d| d.name == "listSize") + .expect("@listSize directive should be present on T.c"); + assert_eq!( + b_requires_scopes_directive.to_string(), + r#"@listSize(requireOneSlicingArgument: null)"# + ); + } + #[test] fn works_for_nullable_max_argument_merge_strategy() { - // NOTE: @listSize(assumedSize:) uses the NULLABLE_MAX strategy for merging arguments, - // So we'll use it as a proxy for the @nullable_max directive + // NOTE: @listSize(assumedSize:) is merged using the NULLABLE_MAX strategy let subgraph1 = ServiceDefinition { name: "Subgraph1", @@ -271,7 +385,7 @@ mod tests { ]; assert_hints_equal(result_sg.hints(), &expected_hints); - // Check merging using NULLABLE_AND succeeded + // Check merging using NULLABLE_MAX succeeded let schema = result_sg.schema().schema(); let sdl = print_sdl(schema); assert!( From cf8522df3a025759fc70bb5bd5874127057b7c63 Mon Sep 17 00:00:00 2001 From: Chidimma O Date: Thu, 30 Oct 2025 18:23:50 -0400 Subject: [PATCH 13/19] Add test for nullable_union strategy --- .../directive_argument_merge_strategies.rs | 143 +++++++++++++++++- 1 file changed, 140 insertions(+), 3 deletions(-) diff --git a/apollo-federation/tests/composition/directive_argument_merge_strategies.rs b/apollo-federation/tests/composition/directive_argument_merge_strategies.rs index 74ba27e8dd..ad860ef801 100644 --- a/apollo-federation/tests/composition/directive_argument_merge_strategies.rs +++ b/apollo-federation/tests/composition/directive_argument_merge_strategies.rs @@ -15,7 +15,6 @@ mod tests { public-facing directives and thus are not represented in this set of tests: - min - intersection - - nullable_union */ #[test] @@ -320,13 +319,13 @@ mod tests { let c = coord!(T.c) .lookup_field(schema) .expect("T.b should be defined on the supergraph"); - let b_requires_scopes_directive = c + let c_requires_scopes_directive = c .directives .iter() .find(|d| d.name == "listSize") .expect("@listSize directive should be present on T.c"); assert_eq!( - b_requires_scopes_directive.to_string(), + c_requires_scopes_directive.to_string(), r#"@listSize(requireOneSlicingArgument: null)"# ); } @@ -433,4 +432,142 @@ mod tests { r#"@listSize(assumedSize: null, requireOneSlicingArgument: true)"# ); } + + #[test] + fn works_for_nullable_union_argument_merge_strategy() { + // NOTE: @listSize(slicingArguments: & sizedFields:) are merged using the NULLABLE_UNION strategy + + let subgraph1 = ServiceDefinition { + name: "Subgraph1", + type_defs: r#" + type Query { + t: [T!] @shareable + } + + type Cursor @shareable { + page: [Item!] + nextPageToken: [String] + } + + type Item @key(fields: "id") { + id: ID + } + + type T @key(fields: "a") @shareable { + k(first: Int): [ID] @listSize(slicingArguments: ["first"]) + a: Int + b: Cursor! @listSize(sizedFields: ["page"]) + } + "#, + }; + + let subgraph2 = ServiceDefinition { + name: "Subgraph2", + type_defs: r#" + type Cursor @shareable { + page: [Item!] + nextPageToken: [String] + } + + type Item @key(fields: "id") { + id: ID + } + + type T @key(fields: "a") @shareable { + k(last: Int): [ID] @listSize(slicingArguments: ["last"]) + a: Int + b: Cursor! @listSize(sizedFields: ["nextPageToken"]) + c: Cursor! @listSize(sizedFields: ["nextPageToken"]) + } + "#, + }; + + let result = compose_as_fed2_subgraphs(&[subgraph1, subgraph2]); + let result_sg = result.expect("Composition should succeed"); + + // Check expected hints + let expected_hints = vec![ + CompositionHint { + code: String::from("INCONSISTENT_ARGUMENT_PRESENCE"), + message: String::from( + r#"Optional argument "T.k(first:)" will not be included in the supergraph as it does not appear in all subgraphs: it is defined in subgraph "Subgraph1" but not in subgraph "Subgraph2"."#, + ), + locations: Vec::new(), + }, + CompositionHint { + code: String::from("INCONSISTENT_ARGUMENT_PRESENCE"), + message: String::from( + r#"Optional argument "T.k(last:)" will not be included in the supergraph as it does not appear in all subgraphs: it is defined in subgraph "Subgraph2" but not in subgraph "Subgraph1"."#, + ), + locations: Vec::new(), + }, + CompositionHint { + code: String::from("MERGED_NON_REPEATABLE_DIRECTIVE_ARGUMENTS"), + message: String::from( + r#"Directive @listSize is applied to "T.k" in multiple subgraphs with different arguments. Merging strategies used by arguments: { assumedSize: NULLABLE_MAX, slicingArguments: NULLABLE_UNION, sizedFields: NULLABLE_UNION, requireOneSlicingArgument: NULLABLE_AND }"#, + ), + locations: Vec::new(), + }, + CompositionHint { + code: String::from("MERGED_NON_REPEATABLE_DIRECTIVE_ARGUMENTS"), + message: String::from( + r#"Directive @listSize is applied to "T.b" in multiple subgraphs with different arguments. Merging strategies used by arguments: { assumedSize: NULLABLE_MAX, slicingArguments: NULLABLE_UNION, sizedFields: NULLABLE_UNION, requireOneSlicingArgument: NULLABLE_AND }"#, + ), + locations: Vec::new(), + }, + ]; + assert_hints_equal(result_sg.hints(), &expected_hints); + + // Check merging using NULLABLE_UNION succeeded + let schema = result_sg.schema().schema(); + let sdl = print_sdl(schema); + assert!( + sdl.contains( + r#"@link(url: "https://specs.apollo.dev/cost/v0.1", import: ["listSize"])"# + ) + ); + + coord!(Query.t) + .lookup_field(schema) + .expect("Query.t should be defined on the supergraph"); + + let k = coord!(T.k) + .lookup_field(schema) + .expect("T.k should be defined on the supergraph"); + let k_requires_scopes_directive = k + .directives + .iter() + .find(|d| d.name == "listSize") + .expect("@listSize directive should be present on T.k"); + assert_eq!( + k_requires_scopes_directive.to_string(), + r#"@listSize(slicingArguments: ["first", "last"], requireOneSlicingArgument: true)"# + ); + + let b = coord!(T.b) + .lookup_field(schema) + .expect("T.b should be defined on the supergraph"); + let b_requires_scopes_directive = b + .directives + .iter() + .find(|d| d.name == "listSize") + .expect("@listSize directive should be present on T.b"); + assert_eq!( + b_requires_scopes_directive.to_string(), + r#"@listSize(sizedFields: ["page", "nextPageToken"], requireOneSlicingArgument: true)"# + ); + + let c = coord!(T.c) + .lookup_field(schema) + .expect("T.b should be defined on the supergraph"); + let c_requires_scopes_directive = c + .directives + .iter() + .find(|d| d.name == "listSize") + .expect("@listSize directive should be present on T.c"); + assert_eq!( + c_requires_scopes_directive.to_string(), + r#"@listSize(sizedFields: ["nextPageToken"], requireOneSlicingArgument: true)"# + ); + } } From 25a5ec08d4db1b82b42c0476b5626f1d652a248f Mon Sep 17 00:00:00 2001 From: Chidimma O Date: Thu, 30 Oct 2025 19:04:51 -0400 Subject: [PATCH 14/19] Fix incorrect error code in compose_validation test --- apollo-federation/tests/composition/compose_validation.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apollo-federation/tests/composition/compose_validation.rs b/apollo-federation/tests/composition/compose_validation.rs index 39fd09d1df..b43905dadb 100644 --- a/apollo-federation/tests/composition/compose_validation.rs +++ b/apollo-federation/tests/composition/compose_validation.rs @@ -166,7 +166,7 @@ fn merge_validations_reject_if_no_subgraphs_have_query() { assert_composition_errors( &result, &[( - "NO_QUERIES", + "QUERY_ROOT_MISSING", "No queries found in any subgraph: a supergraph must have a query root type.", )], ); From 5c168b508a69a6f671b8a108af56c3ad81f9e259 Mon Sep 17 00:00:00 2001 From: Chidimma O Date: Thu, 30 Oct 2025 19:42:08 -0400 Subject: [PATCH 15/19] Fix incorrect error code in compose_inacessible test --- apollo-federation/tests/composition/compose_inaccessible.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apollo-federation/tests/composition/compose_inaccessible.rs b/apollo-federation/tests/composition/compose_inaccessible.rs index 88c234c51a..73e07cb598 100644 --- a/apollo-federation/tests/composition/compose_inaccessible.rs +++ b/apollo-federation/tests/composition/compose_inaccessible.rs @@ -290,7 +290,7 @@ fn inaccessible_errors_if_subgraph_misuses_inaccessible() { assert_composition_errors( &result, &[( - "REFERENCED_INACCESSIBLE", + "INTERNAL", r#"The following errors occurred: - Type `A` is @inaccessible but is referenced by `Query.q2`, which is in the API schema."#, )], From 754cb6961575f27de210a21f096ade9cee94e95b Mon Sep 17 00:00:00 2001 From: Chidimma O Date: Thu, 30 Oct 2025 22:48:48 -0400 Subject: [PATCH 16/19] Revert change in error code for compose_inaccessible test [PR#8520](https://github.com/apollographql/router/pull/8520) ports a behavior that propagates the correct error code. --- apollo-federation/tests/composition/compose_inaccessible.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apollo-federation/tests/composition/compose_inaccessible.rs b/apollo-federation/tests/composition/compose_inaccessible.rs index 73e07cb598..88c234c51a 100644 --- a/apollo-federation/tests/composition/compose_inaccessible.rs +++ b/apollo-federation/tests/composition/compose_inaccessible.rs @@ -290,7 +290,7 @@ fn inaccessible_errors_if_subgraph_misuses_inaccessible() { assert_composition_errors( &result, &[( - "INTERNAL", + "REFERENCED_INACCESSIBLE", r#"The following errors occurred: - Type `A` is @inaccessible but is referenced by `Query.q2`, which is in the API schema."#, )], From e7b355e7b8d96421434797b53e3ccd6ae966df09 Mon Sep 17 00:00:00 2001 From: Chidimma O Date: Thu, 30 Oct 2025 23:31:47 -0400 Subject: [PATCH 17/19] Check error code before checking error msg --- apollo-federation/tests/composition/mod.rs | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/apollo-federation/tests/composition/mod.rs b/apollo-federation/tests/composition/mod.rs index 725594f6d3..d954d7c13f 100644 --- a/apollo-federation/tests/composition/mod.rs +++ b/apollo-federation/tests/composition/mod.rs @@ -112,14 +112,6 @@ pub(crate) mod test_helpers { for (i, (expected_code, expected_message)) in expected_errors.iter().enumerate() { let (error_code, error_str) = &error_strings[i]; - // Check error message - assert!( - error_str.contains(expected_message), - "Error at index {} does not contain expected message.\nEXPECTED:\n{}\nACTUAL:\n{}", - i, - expected_message, - error_str - ); // Check error code assert!( error_code.contains(expected_code), @@ -128,6 +120,14 @@ pub(crate) mod test_helpers { expected_code, error_code ); + // Check error message + assert!( + error_str.contains(expected_message), + "Error at index {} does not contain expected message.\nEXPECTED:\n{}\nACTUAL:\n{}", + i, + expected_message, + error_str + ); } } From db90254ea4673ad032a3e22bd7ef33488b00428d Mon Sep 17 00:00:00 2001 From: Chidimma O Date: Thu, 30 Oct 2025 23:53:38 -0400 Subject: [PATCH 18/19] Add useful context to composition errors --- apollo-federation/tests/composition/mod.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/apollo-federation/tests/composition/mod.rs b/apollo-federation/tests/composition/mod.rs index d954d7c13f..1dcda509e8 100644 --- a/apollo-federation/tests/composition/mod.rs +++ b/apollo-federation/tests/composition/mod.rs @@ -115,18 +115,18 @@ pub(crate) mod test_helpers { // Check error code assert!( error_code.contains(expected_code), - "Error at index {} does not contain expected code.\nEXPECTED:\n{}\nACTUAL:\n{}", + "Error at index {} does not contain expected code.\n\nEXPECTED:\n{}\nACTUAL:\n{}", i, - expected_code, - error_code + format!("code: {}\nmessage: {}\n", expected_code, expected_message), + format!("code: {}\nmessage: {}\n", error_code, error_str) ); // Check error message assert!( error_str.contains(expected_message), - "Error at index {} does not contain expected message.\nEXPECTED:\n{}\nACTUAL:\n{}", + "Error at index {} does not contain expected message.\n\nEXPECTED:\n{}\nACTUAL:\n{}", i, - expected_message, - error_str + format!("code: {}\nmessage: {}\n", expected_code, expected_message), + format!("code: {}\nmessage: {}\n", error_code, error_str) ); } } From 081019d1582de85f25c50f762820e481d14f86e1 Mon Sep 17 00:00:00 2001 From: Chidimma O Date: Fri, 31 Oct 2025 00:12:15 -0400 Subject: [PATCH 19/19] Fix lint errors --- apollo-federation/tests/composition/mod.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/apollo-federation/tests/composition/mod.rs b/apollo-federation/tests/composition/mod.rs index 1dcda509e8..8223f3338b 100644 --- a/apollo-federation/tests/composition/mod.rs +++ b/apollo-federation/tests/composition/mod.rs @@ -117,16 +117,16 @@ pub(crate) mod test_helpers { error_code.contains(expected_code), "Error at index {} does not contain expected code.\n\nEXPECTED:\n{}\nACTUAL:\n{}", i, - format!("code: {}\nmessage: {}\n", expected_code, expected_message), - format!("code: {}\nmessage: {}\n", error_code, error_str) + format_args!("code: {}\nmessage: {}\n", expected_code, expected_message), + format_args!("code: {}\nmessage: {}\n", error_code, error_str) ); // Check error message assert!( error_str.contains(expected_message), "Error at index {} does not contain expected message.\n\nEXPECTED:\n{}\nACTUAL:\n{}", i, - format!("code: {}\nmessage: {}\n", expected_code, expected_message), - format!("code: {}\nmessage: {}\n", error_code, error_str) + format_args!("code: {}\nmessage: {}\n", expected_code, expected_message), + format_args!("code: {}\nmessage: {}\n", error_code, error_str) ); } }