diff --git a/apollo-federation/src/compat.rs b/apollo-federation/src/compat.rs index fcb1061730..ee80a2d32a 100644 --- a/apollo-federation/src/compat.rs +++ b/apollo-federation/src/compat.rs @@ -11,9 +11,11 @@ use apollo_compiler::ExecutableDocument; use apollo_compiler::Name; use apollo_compiler::Node; use apollo_compiler::Schema; +use apollo_compiler::ast::DirectiveDefinition; use apollo_compiler::ast::Value; use apollo_compiler::collections::IndexMap; use apollo_compiler::executable; +use apollo_compiler::schema; use apollo_compiler::schema::Directive; use apollo_compiler::schema::ExtendedType; use apollo_compiler::schema::InputValueDefinition; @@ -53,7 +55,7 @@ fn standardize_deprecated(directive: &mut Directive) { } /// Retain only semantic directives in a directive list from the high-level schema representation. -fn retain_semantic_directives(directives: &mut apollo_compiler::schema::DirectiveList) { +fn retain_semantic_directives(directives: &mut schema::DirectiveList) { directives .0 .retain(|directive| is_semantic_directive_application(directive)); @@ -178,6 +180,13 @@ fn coerce_value( coerce_value(types, element, ty.item_type())?; } } + // Coerce single-element list to a non-list type. + // This is the reverse of the scalar-to-list coercion below. + (Value::List(list), Some(_)) if !ty.is_list() && list.len() == 1 => { + let element = list.pop().unwrap(); + *target.make_mut() = element.as_ref().clone(); + coerce_value(types, target, ty)?; + } // Coerce single values (except null) to a list. ( Value::Object(_) @@ -287,6 +296,63 @@ pub(crate) fn coerce_schema_default_values(schema: &mut Schema) { } } +pub(crate) fn coerce_schema_values(schema: &mut Schema) { + // Keep a copy of the types in the schema so we can mutate the schema while walking it. + let types = schema.types.clone(); + + let directive_definitions = schema.directive_definitions.clone(); + + for ty in schema.types.values_mut() { + match ty { + ExtendedType::Object(object) => { + let object = object.make_mut(); + coerce_directive_application_values_schema( + &directive_definitions, + &types, + &mut object.directives, + ); + for field in object.fields.values_mut() { + let field = field.make_mut(); + coerce_arguments_default_values(&types, &mut field.arguments); + coerce_directive_application_values_ast( + &directive_definitions, + &types, + &mut field.directives, + ); + } + } + ExtendedType::Interface(interface) => { + let interface = interface.make_mut(); + for field in interface.fields.values_mut() { + let field = field.make_mut(); + coerce_arguments_default_values(&types, &mut field.arguments); + } + } + ExtendedType::InputObject(input_object) => { + let input_object = input_object.make_mut(); + for field in input_object.fields.values_mut() { + let field = field.make_mut(); + let Some(default_value) = &mut field.default_value else { + continue; + }; + + if coerce_value(&types, default_value, &field.ty).is_err() { + field.default_value = None; + } + } + } + ExtendedType::Union(_) | ExtendedType::Scalar(_) | ExtendedType::Enum(_) => { + // Nothing to do + } + } + } + + for directive in schema.directive_definitions.values_mut() { + let directive = directive.make_mut(); + coerce_arguments_default_values(&types, &mut directive.arguments); + } +} + fn coerce_directive_application_values( schema: &Valid, directives: &mut executable::DirectiveList, @@ -306,6 +372,46 @@ fn coerce_directive_application_values( } } +fn coerce_directive_application_values_schema( + directive_definitions: &IndexMap>, + type_definitions: &IndexMap, + directives: &mut schema::DirectiveList, +) { + for directive in directives { + let Some(definition) = directive_definitions.get(&directive.name) else { + continue; + }; + let directive = directive.make_mut(); + for arg in &mut directive.arguments { + let Some(definition) = definition.argument_by_name(&arg.name) else { + continue; + }; + let arg = arg.make_mut(); + _ = coerce_value(type_definitions, &mut arg.value, &definition.ty); + } + } +} + +fn coerce_directive_application_values_ast( + directive_definitions: &IndexMap>, + type_definitions: &IndexMap, + directives: &mut apollo_compiler::ast::DirectiveList, +) { + for directive in directives { + let Some(definition) = directive_definitions.get(&directive.name) else { + continue; + }; + let directive = directive.make_mut(); + for arg in &mut directive.arguments { + let Some(definition) = definition.argument_by_name(&arg.name) else { + continue; + }; + let arg = arg.make_mut(); + _ = coerce_value(type_definitions, &mut arg.value, &definition.ty); + } + } +} + fn coerce_selection_set_values( schema: &Valid, selection_set: &mut executable::SelectionSet, diff --git a/apollo-federation/src/subgraph/typestate.rs b/apollo-federation/src/subgraph/typestate.rs index 687eab193c..589d995b09 100644 --- a/apollo-federation/src/subgraph/typestate.rs +++ b/apollo-federation/src/subgraph/typestate.rs @@ -16,6 +16,7 @@ use tracing::trace; use crate::LinkSpecDefinition; use crate::ValidFederationSchema; use crate::bail; +use crate::compat::coerce_schema_values; use crate::ensure; use crate::error::FederationError; use crate::error::Locations; @@ -224,6 +225,9 @@ impl Subgraph { // Simulate graphql-js behavior accepting duplicate argument definitions. parser_backward_compatibility::remove_duplicate_arguments(&mut schema); + // Coerce directive argument values based on directive definitions. + coerce_schema_values(&mut schema); + Self::new(name, url, schema, orphan_extension_types) } diff --git a/apollo-federation/tests/composition/compose_directive.rs b/apollo-federation/tests/composition/compose_directive.rs index 8edf117645..d7c226f1fa 100644 --- a/apollo-federation/tests/composition/compose_directive.rs +++ b/apollo-federation/tests/composition/compose_directive.rs @@ -1089,7 +1089,7 @@ mod composition { directive @auth(scope: [String!]) repeatable on FIELD_DEFINITION type Query { - shared: String @shareable @auth(scope: "VIEWER") + shared: String @shareable @auth(scope: ["VIEWER"]) } "#).unwrap(); let subgraph_b = Subgraph::parse("subgraphB", "", r#" @@ -1119,7 +1119,10 @@ mod composition { "Expected 2 @auth directives on Query.shared" ); - assert_eq!(auth_directives[0].to_string(), r#"@auth(scope: "VIEWER")"#); + assert_eq!( + auth_directives[0].to_string(), + r#"@auth(scope: ["VIEWER"])"# + ); assert_eq!(auth_directives[1].to_string(), "@auth"); } } diff --git a/apollo-federation/tests/composition/directive_argument_merge_strategies.rs b/apollo-federation/tests/composition/directive_argument_merge_strategies.rs index 73c46816d8..c1a7a855eb 100644 --- a/apollo-federation/tests/composition/directive_argument_merge_strategies.rs +++ b/apollo-federation/tests/composition/directive_argument_merge_strategies.rs @@ -122,7 +122,7 @@ mod tests { type T @key(fields: "k") - @requiresScopes(scopes: ["foo", "bar"]) + @requiresScopes(scopes: [["foo"], ["bar"]]) { k: ID @requiresScopes(scopes: []) } @@ -134,11 +134,11 @@ mod tests { type_defs: r#" type T @key(fields: "k") - @requiresScopes(scopes: ["foo"]) + @requiresScopes(scopes: [["foo"]]) { - k: ID @requiresScopes(scopes: ["v1", "v2"]) + k: ID @requiresScopes(scopes: [["v1"], ["v2"]]) a: Int - b: String @requiresScopes(scopes: ["x"]) + b: String @requiresScopes(scopes: [["x"]]) } "#, }; @@ -182,7 +182,7 @@ mod tests { .expect("@requiresScopes directive should be present on T"); assert_eq!( t_requires_scopes_directive.to_string(), - r#"@requiresScopes(scopes: ["foo", "bar"])"# + r#"@requiresScopes(scopes: [["foo"], ["bar"]])"# ); let k = coord!(T.k) @@ -195,7 +195,7 @@ mod tests { .expect("@requiresScopes directive should be present on T.k"); assert_eq!( k_requires_scopes_directive.to_string(), - r#"@requiresScopes(scopes: ["v1", "v2"])"# + r#"@requiresScopes(scopes: [["v1"], ["v2"]])"# ); let b = coord!(T.b) @@ -208,7 +208,7 @@ mod tests { .expect("@requiresScopes directive should be present on T.b"); assert_eq!( b_requires_scopes_directive.to_string(), - r#"@requiresScopes(scopes: ["x"])"# + r#"@requiresScopes(scopes: [["x"]])"# ) } diff --git a/apollo-federation/tests/subgraph/coercion_tests.rs b/apollo-federation/tests/subgraph/coercion_tests.rs new file mode 100644 index 0000000000..3cf6344c3d --- /dev/null +++ b/apollo-federation/tests/subgraph/coercion_tests.rs @@ -0,0 +1,65 @@ +use apollo_federation::subgraph::test_utils::build_and_validate; + +#[test] +fn coerces_directive_argument_values() { + // Test that directive argument values are coerced correctly. + let schema = r#" + extend schema @link(url: "https://specs.apollo.dev/federation/v2.0", import: ["@key"]) + + type Query { + test: T! + } + + type T @key(fields: "id") { + id: ID! + x: Int! + } + "#; + + let _subgraph = build_and_validate(schema); + // Success: schema validated after coercion +} + +#[test] +fn coerces_field_argument_default_values() { + // Test that field argument default values are coerced correctly. + // The field argument expects String! but the default is a list ["id"] + // which should be coerced to "id". + let schema = r#" + extend schema @link(url: "https://specs.apollo.dev/federation/v2.0", import: ["@key"]) + + type Query { + test: T! + } + + type T @key(fields: "id") { + id: ID! + name(arg: String! = ["id"]): String! + x: Int! + } + "#; + + let _subgraph = build_and_validate(schema); + // Success: schema validated after coercion +} + +#[test] +fn coerces_input_field_default_values() { + // Test that input object field default values are coerced correctly. + // - `name` has an enum-like default value `Anonymous` which should be coerced for custom scalars + // - `age` expects Int but the default is a list [18] which should be coerced + let schema = r#" + extend schema @link(url: "https://specs.apollo.dev/federation/v2.0", import: ["@key"]) + + type Query { + test(input: UserInput): String + } + + input UserInput { + name: String = Anonymous + age: Int = [18] + } + "#; + let _subgraph = build_and_validate(schema); + // Success: schema validated after coercion +} diff --git a/apollo-federation/tests/subgraph/mod.rs b/apollo-federation/tests/subgraph/mod.rs index 3b4f779a79..a76e696635 100644 --- a/apollo-federation/tests/subgraph/mod.rs +++ b/apollo-federation/tests/subgraph/mod.rs @@ -1,2 +1,3 @@ +mod coercion_tests; mod parse_expand_tests; mod subgraph_validation_tests; diff --git a/apollo-federation/tests/subgraph/subgraph_validation_tests.rs b/apollo-federation/tests/subgraph/subgraph_validation_tests.rs index a065e69e5a..caedb2e197 100644 --- a/apollo-federation/tests/subgraph/subgraph_validation_tests.rs +++ b/apollo-federation/tests/subgraph/subgraph_validation_tests.rs @@ -225,13 +225,15 @@ mod fieldset_based_directives { #[test] fn rejects_non_string_argument_to_key() { + // Use a multi-element list which cannot be coerced to a single string let schema_str = r#" type Query { t: T } - type T @key(fields: ["f"]) { + type T @key(fields: ["f", "g"]) { f: Int + g: Int } "#; let err = build_for_errors(schema_str); @@ -240,20 +242,22 @@ mod fieldset_based_directives { err, [( "KEY_INVALID_FIELDS_TYPE", - r#"[S] On type "T", for @key(fields: ["f"]): Invalid value for argument "fields": must be a string."#, + r#"[S] On type "T", for @key(fields: ["f", "g"]): Invalid value for argument "fields": must be a string."#, )] ); } #[test] fn rejects_non_string_argument_to_provides() { + // Use a multi-element list which cannot be coerced to a single string let schema_str = r#" type Query { - t: T @provides(fields: ["f"]) + t: T @provides(fields: ["f", "g"]) } type T { f: Int @external + g: Int @external } "#; let err = build_for_errors(schema_str); @@ -266,18 +270,23 @@ mod fieldset_based_directives { [ ( "PROVIDES_INVALID_FIELDS_TYPE", - r#"[S] On field "Query.t", for @provides(fields: ["f"]): Invalid value for argument "fields": must be a string."#, + r#"[S] On field "Query.t", for @provides(fields: ["f", "g"]): Invalid value for argument "fields": must be a string."#, ), ( "EXTERNAL_UNUSED", r#"[S] Field "T.f" is marked @external but is not used in any federation directive (@key, @provides, @requires) or to satisfy an interface; the field declaration has no use and should be removed (or the field should not be @external)."#, ), + ( + "EXTERNAL_UNUSED", + r#"[S] Field "T.g" is marked @external but is not used in any federation directive (@key, @provides, @requires) or to satisfy an interface; the field declaration has no use and should be removed (or the field should not be @external)."#, + ), ] ); } #[test] fn rejects_non_string_argument_to_requires() { + // Use a multi-element list which cannot be coerced to a single string let schema_str = r#" type Query { t: T @@ -285,7 +294,8 @@ mod fieldset_based_directives { type T { f: Int @external - g: Int @requires(fields: ["f"]) + h: Int @external + g: Int @requires(fields: ["f", "h"]) } "#; let err = build_for_errors(schema_str); @@ -298,12 +308,16 @@ mod fieldset_based_directives { [ ( "REQUIRES_INVALID_FIELDS_TYPE", - r#"[S] On field "T.g", for @requires(fields: ["f"]): Invalid value for argument "fields": must be a string."#, + r#"[S] On field "T.g", for @requires(fields: ["f", "h"]): Invalid value for argument "fields": must be a string."#, ), ( "EXTERNAL_UNUSED", r#"[S] Field "T.f" is marked @external but is not used in any federation directive (@key, @provides, @requires) or to satisfy an interface; the field declaration has no use and should be removed (or the field should not be @external)."#, ), + ( + "EXTERNAL_UNUSED", + r#"[S] Field "T.h" is marked @external but is not used in any federation directive (@key, @provides, @requires) or to satisfy an interface; the field declaration has no use and should be removed (or the field should not be @external)."#, + ), ] ); }