diff --git a/.changesets/helm_host_configuration.md b/.changesets/helm_host_configuration.md deleted file mode 100644 index e26bdc6dd1..0000000000 --- a/.changesets/helm_host_configuration.md +++ /dev/null @@ -1,5 +0,0 @@ -### Allow configuring host via Helm template for virtual service ([PR #5545](https://github.com/apollographql/router/pull/5795)) - -When deploying via Helm, you can now configure hosts in `virtualservice.yaml` as a single host or a range of hosts. This is helpful when different hosts could be used within a cluster. - -By [@nicksephora](https://github.com/nicksephora) in https://github.com/apollographql/router/pull/5545 diff --git a/.circleci/config.yml b/.circleci/config.yml index 126c85a25d..50d08f1d50 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -1,6 +1,6 @@ version: 2.1 -# Cache key bump: 2 +# Cache key bump: 3 # These "CircleCI Orbs" are reusable bits of configuration that can be shared # across projects. See https://circleci.com/orbs/ for more information. diff --git a/CHANGELOG.md b/CHANGELOG.md index bd46bf9f90..cbab02293c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,30 @@ All notable changes to Router will be documented in this file. This project adheres to [Semantic Versioning v2.0.0](https://semver.org/spec/v2.0.0.html). +# [1.58.1] - 2024-12-05 + +> [!IMPORTANT] +> If you have enabled [Distributed query plan caching](https://www.apollographql.com/docs/router/configuration/distributed-caching/#distributed-query-plan-caching), this release contains changes which necessarily alter the hashing algorithm used for the cache keys. On account of this, you should anticipate additional cache regeneration cost when updating between these versions while the new hashing algorithm comes into service. + +## 🐛 Fixes + +### Particular `supergraph` telemetry customizations using the `query` selector do not error ([PR #6324](https://github.com/apollographql/router/pull/6324)) + +Telemetry customizations like those featured in the [request limits telemetry documentation](https://www.apollographql.com/docs/graphos/routing/security/request-limits#collecting-metrics) now work as intended when using the `query` selector on the `supergraph` layer. Prior to this fix, this was sometimes causing a `this is a bug and should not happen` error, but is now resolved. + +By [@bnjjj](https://github.com/bnjjj) in https://github.com/apollographql/router/pull/6324 + +### Native query planner now receives both "plan" and "path" limits configuration ([PR #6316](https://github.com/apollographql/router/pull/6316)) + +The native query planner now correctly sets two experimental configuration options for limiting query planning complexity. These were previously available in the configuration and observed by the legacy planner, but were not being passed to the new native planner until now: + +- `supergraph.query_planning.experimental_plans_limit` +- `supergraph.query_planning.experimental_paths_limit` + +By [@goto-bus-stop](https://github.com/goto-bus-stop) in https://github.com/apollographql/router/pull/6316 + + + # [1.58.0] - 2024-11-27 > [!IMPORTANT] @@ -133,6 +157,12 @@ experimental_query_planner_mode: new By [@clenfest](https://github.com/clenfest), [@TylerBloom](https://github.com/TylerBloom) in https://github.com/apollographql/router/pull/6310 +### Allow configuring host via Helm template for virtual service ([PR #5545](https://github.com/apollographql/router/pull/5795)) + +When deploying via Helm, you can now configure hosts in `virtualservice.yaml` as a single host or a range of hosts. This is helpful when different hosts could be used within a cluster. + +By [@nicksephora](https://github.com/nicksephora) in https://github.com/apollographql/router/pull/5545 + ## 🐛 Fixes ### Remove noisy demand control logs ([PR #6192](https://github.com/apollographql/router/pull/6192)) diff --git a/Cargo.lock b/Cargo.lock index 57a87514d6..4b1ab30f24 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -178,12 +178,12 @@ dependencies = [ [[package]] name = "apollo-federation" -version = "1.58.0" +version = "1.58.1" dependencies = [ "apollo-compiler", "derive_more", "either", - "hashbrown 0.15.0", + "hashbrown 0.15.2", "hex", "indexmap 2.2.6", "insta", @@ -231,7 +231,7 @@ dependencies = [ [[package]] name = "apollo-router" -version = "1.58.0" +version = "1.58.1" dependencies = [ "access-json", "ahash", @@ -399,7 +399,7 @@ dependencies = [ [[package]] name = "apollo-router-benchmarks" -version = "1.58.0" +version = "1.58.1" dependencies = [ "apollo-parser", "apollo-router", @@ -415,7 +415,7 @@ dependencies = [ [[package]] name = "apollo-router-scaffold" -version = "1.58.0" +version = "1.58.1" dependencies = [ "anyhow", "cargo-scaffold", @@ -3095,9 +3095,9 @@ dependencies = [ [[package]] name = "hashbrown" -version = "0.15.0" +version = "0.15.2" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "1e087f84d4f86bf4b218b927129862374b72199ae7d8657835f1e89000eea4fb" +checksum = "bf151400ff0baff5465007dd2f3e717f3fe502074ca563069ce3a6629d07b289" dependencies = [ "allocator-api2", "equivalent", diff --git a/apollo-federation/Cargo.toml b/apollo-federation/Cargo.toml index c0103f435e..739d379f60 100644 --- a/apollo-federation/Cargo.toml +++ b/apollo-federation/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "apollo-federation" -version = "1.58.0" +version = "1.58.1" authors = ["The Apollo GraphQL Contributors"] edition = "2021" description = "Apollo Federation" @@ -22,7 +22,7 @@ time = { version = "0.3.34", default-features = false, features = [ "local-offset", ] } derive_more = "0.99.17" -hashbrown = "0.15.0" +hashbrown = "0.15.1" indexmap = { version = "2.2.6", features = ["serde"] } itertools = "0.13.0" lazy_static = "1.4.0" diff --git a/apollo-federation/src/lib.rs b/apollo-federation/src/lib.rs index 99d340e881..4e8ba89640 100644 --- a/apollo-federation/src/lib.rs +++ b/apollo-federation/src/lib.rs @@ -42,12 +42,15 @@ pub(crate) mod utils; use apollo_compiler::ast::NamedType; use apollo_compiler::validation::Valid; use apollo_compiler::Schema; +use itertools::Itertools; use link::join_spec_definition::JOIN_VERSIONS; use schema::FederationSchema; pub use crate::api_schema::ApiSchemaOptions; use crate::error::FederationError; use crate::error::SingleFederationError; +use crate::link::context_spec_definition::ContextSpecDefinition; +use crate::link::context_spec_definition::CONTEXT_VERSIONS; use crate::link::join_spec_definition::JoinSpecDefinition; use crate::link::link_spec_definition::LinkSpecDefinition; use crate::link::spec::Identity; @@ -59,18 +62,23 @@ use crate::subgraph::ValidSubgraph; pub use crate::supergraph::ValidFederationSubgraph; pub use crate::supergraph::ValidFederationSubgraphs; -pub(crate) type SupergraphSpecs = (&'static LinkSpecDefinition, &'static JoinSpecDefinition); +pub(crate) type SupergraphSpecs = ( + &'static LinkSpecDefinition, + &'static JoinSpecDefinition, + Option<&'static ContextSpecDefinition>, +); pub(crate) fn validate_supergraph_for_query_planning( supergraph_schema: &FederationSchema, ) -> Result { - validate_supergraph(supergraph_schema, &JOIN_VERSIONS) + validate_supergraph(supergraph_schema, &JOIN_VERSIONS, &CONTEXT_VERSIONS) } /// Checks that required supergraph directives are in the schema, and returns which ones were used. pub(crate) fn validate_supergraph( supergraph_schema: &FederationSchema, join_versions: &'static SpecDefinitions, + context_versions: &'static SpecDefinitions, ) -> Result { let Some(metadata) = supergraph_schema.metadata() else { return Err(SingleFederationError::InvalidFederationSupergraph { @@ -94,7 +102,22 @@ pub(crate) fn validate_supergraph( ), }.into()); }; - Ok((link_spec_definition, join_spec_definition)) + let context_spec_definition = metadata.for_identity(&Identity::context_identity()).map(|context_link| { + context_versions.find(&context_link.url.version).ok_or_else(|| { + SingleFederationError::InvalidFederationSupergraph { + message: format!( + "Invalid supergraph: uses unsupported context spec version {} (supported versions: {})", + context_link.url.version, + context_versions.versions().join(", "), + ), + } + }) + }).transpose()?; + Ok(( + link_spec_definition, + join_spec_definition, + context_spec_definition, + )) } pub struct Supergraph { diff --git a/apollo-federation/src/link/argument.rs b/apollo-federation/src/link/argument.rs index aeac4ccd0e..d8ae1987f2 100644 --- a/apollo-federation/src/link/argument.rs +++ b/apollo-federation/src/link/argument.rs @@ -144,7 +144,7 @@ pub(crate) fn directive_optional_list_argument<'a>( Value::Null => Ok(None), Value::List(values) => Ok(Some(values.as_slice())), _ => bail!( - r#"Argument "{name}" of directive "@{}" must be a boolean."#, + r#"Argument "{name}" of directive "@{}" must be a list."#, application.name ), }, diff --git a/apollo-federation/src/link/context_spec_definition.rs b/apollo-federation/src/link/context_spec_definition.rs index ff4217c2d6..5008bc328c 100644 --- a/apollo-federation/src/link/context_spec_definition.rs +++ b/apollo-federation/src/link/context_spec_definition.rs @@ -1,8 +1,13 @@ +use apollo_compiler::ast::Directive; +use apollo_compiler::ast::DirectiveDefinition; use apollo_compiler::name; use apollo_compiler::Name; +use apollo_compiler::Node; use lazy_static::lazy_static; use crate::error::FederationError; +use crate::internal_error; +use crate::link::argument::directive_required_string_argument; use crate::link::spec::Identity; use crate::link::spec::Url; use crate::link::spec::Version; @@ -11,7 +16,11 @@ use crate::link::spec_definition::SpecDefinitions; use crate::schema::FederationSchema; pub(crate) const CONTEXT_DIRECTIVE_NAME_IN_SPEC: Name = name!("context"); -pub(crate) const CONTEXT_DIRECTIVE_NAME_DEFAULT: Name = name!("federation__context"); +pub(crate) const CONTEXT_NAME_ARGUMENT_NAME: Name = name!("name"); + +pub(crate) struct ContextDirectiveArguments<'doc> { + pub(crate) name: &'doc str, +} #[derive(Clone)] pub(crate) struct ContextSpecDefinition { @@ -30,13 +39,21 @@ impl ContextSpecDefinition { } } - pub(crate) fn context_directive_name_in_schema( + pub(crate) fn context_directive_definition<'schema>( + &self, + schema: &'schema FederationSchema, + ) -> Result<&'schema Node, FederationError> { + self.directive_definition(schema, &CONTEXT_DIRECTIVE_NAME_IN_SPEC)? + .ok_or_else(|| internal_error!("Unexpectedly could not find context spec in schema")) + } + + pub(crate) fn context_directive_arguments<'doc>( &self, - schema: &FederationSchema, - ) -> Result { - Ok(self - .directive_name_in_schema(schema, &CONTEXT_DIRECTIVE_NAME_IN_SPEC)? - .unwrap_or(CONTEXT_DIRECTIVE_NAME_DEFAULT)) + application: &'doc Node, + ) -> Result, FederationError> { + Ok(ContextDirectiveArguments { + name: directive_required_string_argument(application, &CONTEXT_NAME_ARGUMENT_NAME)?, + }) } } diff --git a/apollo-federation/src/link/federation_spec_definition.rs b/apollo-federation/src/link/federation_spec_definition.rs index a773d860c7..5e3e09512c 100644 --- a/apollo-federation/src/link/federation_spec_definition.rs +++ b/apollo-federation/src/link/federation_spec_definition.rs @@ -469,15 +469,16 @@ impl FederationSpecDefinition { }) } - pub(crate) fn context_directive_arguments( - application: &Node, - ) -> Result { + pub(crate) fn context_directive_arguments<'doc>( + &self, + application: &'doc Node, + ) -> Result, FederationError> { Ok(ContextDirectiveArguments { name: directive_required_string_argument(application, &FEDERATION_NAME_ARGUMENT_NAME)?, }) } - // The directive is named `@fromContex`. This is confusing for clippy, as + // The directive is named `@fromContext`. This is confusing for clippy, as // `from` is a conventional prefix used in conversion methods, which do not // take `self` as an argument. This function does **not** perform // conversion, but extracts `@fromContext` directive definition. @@ -495,24 +496,7 @@ impl FederationSpecDefinition { }) } - // The directive is named `@fromContex`. This is confusing for clippy, as - // `from` is a conventional prefix used in conversion methods, which do not - // take `self` as an argument. This function does **not** perform - // conversion, but extracts `@fromContext` directive arguments. - #[allow(clippy::wrong_self_convention)] - pub(crate) fn from_context_directive_arguments<'doc>( - &self, - application: &'doc Node, - ) -> Result, FederationError> { - Ok(FromContextDirectiveArguments { - field: directive_required_string_argument( - application, - &FEDERATION_FIELD_ARGUMENT_NAME, - )?, - }) - } - - // The directive is named `@fromContex`. This is confusing for clippy, as + // The directive is named `@fromContext`. This is confusing for clippy, as // `from` is a conventional prefix used in conversion methods, which do not // take `self` as an argument. This function does **not** perform // conversion, but extracts `@fromContext` directive. @@ -539,6 +523,23 @@ impl FederationSpecDefinition { }) } + // The directive is named `@fromContext`. This is confusing for clippy, as + // `from` is a conventional prefix used in conversion methods, which do not + // take `self` as an argument. This function does **not** perform + // conversion, but extracts `@fromContext` directive arguments. + #[allow(clippy::wrong_self_convention)] + pub(crate) fn from_context_directive_arguments<'doc>( + &self, + application: &'doc Node, + ) -> Result, FederationError> { + Ok(FromContextDirectiveArguments { + field: directive_required_string_argument( + application, + &FEDERATION_FIELD_ARGUMENT_NAME, + )?, + }) + } + pub(crate) fn get_cost_spec_definition( &self, schema: &FederationSchema, diff --git a/apollo-federation/src/link/join_spec_definition.rs b/apollo-federation/src/link/join_spec_definition.rs index 19124f579a..1bffc68b74 100644 --- a/apollo-federation/src/link/join_spec_definition.rs +++ b/apollo-federation/src/link/join_spec_definition.rs @@ -49,7 +49,7 @@ pub(crate) const JOIN_OVERRIDE_LABEL_ARGUMENT_NAME: Name = name!("overrideLabel" pub(crate) const JOIN_USEROVERRIDDEN_ARGUMENT_NAME: Name = name!("usedOverridden"); pub(crate) const JOIN_INTERFACE_ARGUMENT_NAME: Name = name!("interface"); pub(crate) const JOIN_MEMBER_ARGUMENT_NAME: Name = name!("member"); -pub(crate) const JOIN_MEMBER_CONTEXT_ARGUMENTS: Name = name!("contextArguments"); +pub(crate) const JOIN_CONTEXTARGUMENTS_ARGUMENT_NAME: Name = name!("contextArguments"); pub(crate) struct GraphDirectiveArguments<'doc> { pub(crate) name: &'doc str, @@ -81,7 +81,9 @@ impl<'doc> TryFrom<&'doc Value> for ContextArgument<'doc> { value: &'a Value, ) -> Result<(), FederationError> { if let Some(first_value) = field { - bail!("Duplicate contextArgument for '{name}' field: {first_value} and {value}") + bail!( + r#"Input field "{name}" in contextArguments is repeated with value "{value}" (previous value was "{first_value}")"# + ) } let _ = field.insert(value); Ok(()) @@ -94,36 +96,36 @@ impl<'doc> TryFrom<&'doc Value> for ContextArgument<'doc> { field .ok_or_else(|| { FederationError::internal(format!( - "'{field_name}' field was missing from contextArgument" + r#"Input field "{field_name}" is missing from contextArguments"# )) })? .as_str() .ok_or_else(|| { FederationError::internal(format!( - "'{field_name}' field of contextArgument was not a string" + r#"Input field "{field_name}" in contextArguments is not a string"# )) }) } - let Value::Object(names) = value else { - bail!("Item in contextArgument list is not an object {value}") + let Value::Object(input_object) = value else { + bail!(r#"Item "{value}" in contextArguments list is not an object"#) }; let mut name = None; let mut type_ = None; let mut context = None; let mut selection = None; - for (arg_name, value) in names.as_slice() { - match arg_name.as_str() { - "name" => insert_value(arg_name, &mut name, value)?, - "type" => insert_value(arg_name, &mut type_, value)?, - "context" => insert_value(arg_name, &mut context, value)?, - "selection" => insert_value(arg_name, &mut selection, value)?, - _ => bail!("Found unknown contextArgument {arg_name}"), + for (input_field_name, value) in input_object { + match input_field_name.as_str() { + "name" => insert_value(input_field_name, &mut name, value)?, + "type" => insert_value(input_field_name, &mut type_, value)?, + "context" => insert_value(input_field_name, &mut context, value)?, + "selection" => insert_value(input_field_name, &mut selection, value)?, + _ => bail!(r#"Found unknown contextArguments input field "{input_field_name}""#), } } let name = field_or_else("name", name)?; - let type_ = field_or_else("type_", type_)?; + let type_ = field_or_else("type", type_)?; let context = field_or_else("context", context)?; let selection = field_or_else("selection", selection)?; @@ -308,7 +310,7 @@ impl JoinSpecDefinition { )?, context_arguments: directive_optional_list_argument( application, - &JOIN_MEMBER_CONTEXT_ARGUMENTS, + &JOIN_CONTEXTARGUMENTS_ARGUMENT_NAME, )? .map(|values| { values diff --git a/apollo-federation/src/operation/rebase.rs b/apollo-federation/src/operation/rebase.rs index 09b8799067..556a72ca5f 100644 --- a/apollo-federation/src/operation/rebase.rs +++ b/apollo-federation/src/operation/rebase.rs @@ -22,7 +22,6 @@ use super::SelectionSet; use super::TYPENAME_FIELD; use crate::ensure; use crate::error::FederationError; -use crate::link::federation_spec_definition::get_federation_spec_definition_from_subgraph; use crate::schema::position::CompositeTypeDefinitionPosition; use crate::schema::position::OutputTypeDefinitionPosition; use crate::schema::ValidFederationSchema; @@ -276,15 +275,17 @@ impl Field { else { return Ok(None); }; - if let Ok(federation_spec_definition) = get_federation_spec_definition_from_subgraph(schema) + if let Some(federation_spec_definition) = schema + .subgraph_metadata() + .map(|d| d.federation_spec_definition()) { let from_context_directive_definition_name = &federation_spec_definition .from_context_directive_definition(schema)? .name; - // We need to prevent arguments with `@fromContext` from being lost/overwriten. If the - // would-be parent type's field has `@fromContext` and one (or more) of this field's - // arguments doesn't exist in the would-be parent's field, rebasing would loose that - // context. + // We need to ensure that all arguments with `@fromContext` are provided. If the + // would-be parent type's field has an argument with `@fromContext` and that argument + // has no value/data in this field, then we return `None` to indicate the rebase isn't + // possible. if field_definition.arguments.iter().any(|arg_definition| { arg_definition .directives diff --git a/apollo-federation/src/query_graph/build_query_graph.rs b/apollo-federation/src/query_graph/build_query_graph.rs index 81cd361f4f..0c820c5622 100644 --- a/apollo-federation/src/query_graph/build_query_graph.rs +++ b/apollo-federation/src/query_graph/build_query_graph.rs @@ -8,6 +8,7 @@ use apollo_compiler::validation::Valid; use apollo_compiler::Name; use apollo_compiler::Schema; use itertools::Itertools; +use lazy_static::lazy_static; use petgraph::graph::EdgeIndex; use petgraph::graph::NodeIndex; use petgraph::visit::EdgeRef; @@ -18,6 +19,7 @@ use strum::IntoEnumIterator; use crate::bail; use crate::error::FederationError; use crate::error::SingleFederationError; +use crate::internal_error; use crate::link::federation_spec_definition::get_federation_spec_definition_from_subgraph; use crate::link::federation_spec_definition::FederationSpecDefinition; use crate::link::federation_spec_definition::KeyDirectiveArguments; @@ -47,6 +49,7 @@ use crate::schema::position::TypeDefinitionPosition; use crate::schema::position::UnionTypeDefinitionPosition; use crate::schema::ValidFederationSchema; use crate::supergraph::extract_subgraphs_from_supergraph; +use crate::utils::iter_into_single_item; use crate::utils::FallibleIterator; /// Builds a "federated" query graph based on the provided supergraph and API schema. @@ -70,11 +73,11 @@ pub fn build_federated_query_graph( graph: Default::default(), sources: Default::default(), subgraphs_by_name: Default::default(), + supergraph_schema: Default::default(), types_to_nodes_by_source: Default::default(), root_kinds_to_nodes_by_source: Default::default(), non_trivial_followup_edges: Default::default(), - subgraph_to_args: Default::default(), - subgraph_to_arg_indices: Default::default(), + arguments_to_context_ids_by_source: Default::default(), }; let query_graph = extract_subgraphs_from_supergraph(&supergraph_schema, validate_extracted_subgraphs)? @@ -105,11 +108,11 @@ pub fn build_query_graph( graph: Default::default(), sources: Default::default(), subgraphs_by_name: Default::default(), + supergraph_schema: Default::default(), types_to_nodes_by_source: Default::default(), root_kinds_to_nodes_by_source: Default::default(), non_trivial_followup_edges: Default::default(), - subgraph_to_args: Default::default(), - subgraph_to_arg_indices: Default::default(), + arguments_to_context_ids_by_source: Default::default(), }; let builder = SchemaQueryGraphBuilder::new(query_graph, name, schema, None, false)?; query_graph = builder.build()?; @@ -962,9 +965,10 @@ struct FederatedQueryGraphBuilder { impl FederatedQueryGraphBuilder { fn new( - query_graph: QueryGraph, + mut query_graph: QueryGraph, supergraph_schema: ValidFederationSchema, ) -> Result { + query_graph.supergraph_schema = Some(supergraph_schema.clone()); let base = BaseQueryGraphBuilder::new( query_graph, FEDERATED_GRAPH_ROOT_SOURCE.into(), @@ -972,6 +976,7 @@ impl FederatedQueryGraphBuilder { // here (note that empty schemas have no Query type, making them invalid GraphQL). ValidFederationSchema::new(Valid::assume_valid(Schema::new()))?, ); + let subgraphs = FederatedQueryGraphBuilderSubgraphs::new(&base)?; Ok(FederatedQueryGraphBuilder { base, @@ -1478,27 +1483,24 @@ impl FederatedQueryGraphBuilder { fn handle_context(&mut self) -> Result<(), FederationError> { let mut subgraph_to_args: IndexMap, Vec> = - IndexMap::default(); + Default::default(); let mut coordinate_map: IndexMap< Arc, IndexMap>, - > = IndexMap::default(); + > = Default::default(); for (subgraph_name, subgraph) in self.base.query_graph.subgraphs() { let subgraph_data = self.subgraphs.get(subgraph_name)?; - let Some((_, context_refs)) = &subgraph + let Some(context_refs) = &subgraph .referencers() .directives - .iter() - .find(|(dir, _)| **dir == subgraph_data.context_directive_definition_name) + .get(&subgraph_data.context_directive_definition_name) else { continue; }; - - let Some((_, from_context_refs)) = &subgraph + let Some(from_context_refs) = &subgraph .referencers() .directives - .iter() - .find(|(dir, _)| **dir == subgraph_data.from_context_directive_definition_name) + .get(&subgraph_data.from_context_directive_definition_name) else { continue; }; @@ -1514,7 +1516,9 @@ impl FederatedQueryGraphBuilder { .directives .get_all(subgraph_data.context_directive_definition_name.as_str()) { - let application = FederationSpecDefinition::context_directive_arguments(dir)?; + let application = subgraph_data + .federation_spec_definition + .context_directive_arguments(dir)?; context_name_to_types .entry(application.name) .or_default() @@ -1527,7 +1531,9 @@ impl FederatedQueryGraphBuilder { .directives .get_all(subgraph_data.context_directive_definition_name.as_str()) { - let application = FederationSpecDefinition::context_directive_arguments(dir)?; + let application = subgraph_data + .federation_spec_definition + .context_directive_arguments(dir)?; context_name_to_types .entry(application.name) .or_default() @@ -1540,7 +1546,9 @@ impl FederatedQueryGraphBuilder { .directives .get_all(subgraph_data.context_directive_definition_name.as_str()) { - let application = FederationSpecDefinition::context_directive_arguments(dir)?; + let application = subgraph_data + .federation_spec_definition + .context_directive_arguments(dir)?; context_name_to_types .entry(application.name) .or_default() @@ -1557,36 +1565,41 @@ impl FederatedQueryGraphBuilder { .or_default() .push(object_field_arg.clone()); let field_coordinate = object_field_arg.parent(); - if let Some(dir) = input_value.directives.get( + let Some(dir) = input_value.directives.get( subgraph_data .from_context_directive_definition_name .as_str(), - ) { - let application = subgraph_data - .federation_spec_definition - .from_context_directive_arguments(dir)?; - let (context, selection) = parse_context(application.field)?; - - let types_with_context_set = context_name_to_types - .get(context.as_str()) - .into_iter() - .flatten() - .cloned() - .collect(); - let conditions = ContextCondition { + ) else { + bail!( + "Argument {} unexpectedly missing @fromContext directive", + object_field_arg + ); + }; + let application = subgraph_data + .federation_spec_definition + .from_context_directive_arguments(dir)?; + let (context, selection) = parse_context(application.field)?; + let Some(types_with_context_set) = context_name_to_types.get(context.as_str()) + else { + bail!( + r#"Context ${} is never set in subgraph "{}""#, context, - selection, - subgraph_name: subgraph_name.clone(), - argument_coordinate: object_field_arg.clone(), - types_with_context_set, - named_parameter: object_field_arg.argument_name.to_owned(), - arg_type: input_value.ty.clone(), - }; - coordinate_map - .entry(field_coordinate.clone()) - .or_default() - .push(conditions); - } + subgraph_name + ); + }; + let conditions = ContextCondition { + context, + subgraph_name: subgraph_name.clone(), + selection, + types_with_context_set: types_with_context_set.clone(), + argument_name: object_field_arg.argument_name.to_owned(), + argument_coordinate: object_field_arg.clone(), + argument_type: input_value.ty.clone(), + }; + coordinate_map + .entry(field_coordinate.clone()) + .or_default() + .push(conditions); } } @@ -1617,16 +1630,29 @@ impl FederatedQueryGraphBuilder { } // Add the context argument mapping - self.base.query_graph.subgraph_to_arg_indices = self + self.base.query_graph.arguments_to_context_ids_by_source = self .base .query_graph .subgraphs() - .filter_map(|(source, _)| subgraph_to_args.get_full(source)) + .enumerate() + .filter_map(|(index, (source, _))| { + subgraph_to_args + .get_key_value(source) + .map(|(source, args)| (index, source, args)) + }) .map(|(index, source, args)| { Ok::<_, FederationError>(( source.clone(), args.iter() - .sorted() + // TODO: We're manually sorting by the actual GraphQL coordinate string here + // to mimic the behavior of JS code. In the future, we could just sort + // the argument position in the natural tuple-based way. + .sorted_by_key(|arg| { + format!( + "{}.{}({}:)", + arg.type_name, arg.field_name, arg.argument_name + ) + }) .enumerate() .map(|(i, arg)| { Ok::<_, FederationError>(( @@ -1638,7 +1664,6 @@ impl FederatedQueryGraphBuilder { )) }) .process_results(|r| r.collect())?; - self.base.query_graph.subgraph_to_args = subgraph_to_args; Ok(()) } @@ -2332,43 +2357,72 @@ fn resolvable_key_applications<'doc>( Ok(applications) } -fn parse_context(field: &str) -> Result<(String, String), FederationError> { - let pattern = Regex::new( - r#"^(?:[\n\r\t ,]|#[^\n\r]*)*\$(?:[\n\r\t ,]|#[^\n\r]*)*([A-Za-z_]\w*)([\s\S]*)$"#, - ) - .unwrap(); - - let mut iter = pattern.captures_iter(field); - - let Some(captures) = iter.next() else { - bail!("Expected to find the name of a context and a selection inside the field argument to `@fromContext`: {field:?}"); - }; +lazy_static! { + static ref CONTEXT_PARSING_LEADING_PATTERN: Regex = + Regex::new(r#"^(?:[\n\r\t ,]|#[^\n\r]*)*((?s:.)*)$"#).unwrap(); + static ref CONTEXT_PARSING_CONTEXT_PATTERN: Regex = + Regex::new(r#"^([A-Za-z_](?-u:\w)*)((?s:.)*)$"#).unwrap(); +} - if iter.next().is_some() { - bail!("Expected only one context and selection pair inside the field argument to `@fromContext`: {field:?}"); +fn parse_context(field: &str) -> Result<(String, String), FederationError> { + // PORT_NOTE: The original JS regex, as shown below + // /^(?:[\n\r\t ,]|#[^\n\r]*(?![^\n\r]))*\$(?:[\n\r\t ,]|#[^\n\r]*(?![^\n\r]))*([A-Za-z_]\w*(?!\w))([\s\S]*)$/ + // makes use of negative lookaheads, which aren't supported natively by Rust's regex crate. + // There's a fancy_regex crate which does support this, but in the specific case above, the + // negative lookaheads are just used to ensure strict *-greediness for the preceding expression + // (i.e., it guarantees those *-expressions match greedily and won't backtrack). + // + // We can emulate that in this case by matching a series of regexes instead of a single regex, + // where for each regex, the relevant *-expression doesn't backtrack by virtue of the rest of + // the haystack guaranteeing a match. Also note that Rust has (?s:.) to match all characters + // including newlines, which we use in place of JS's common regex workaround of [\s\S]. + fn strip_leading_ignored_tokens(input: &str) -> Result<&str, FederationError> { + iter_into_single_item(CONTEXT_PARSING_LEADING_PATTERN.captures_iter(input)) + .and_then(|c| c.get(1)) + .map(|m| m.as_str()) + .ok_or_else(|| { + internal_error!(r#"Failed to skip any leading ignored tokens in @fromContext substring "{input}""#) + }) } - let (context, selection) = captures - .iter() - // Ignore the first match because it is always the whole matching substring - .skip(1) - .flatten() - .fold(("", ""), |(_, b), group| (b, group.as_str())); - - if context.is_empty() { - bail!("Expected to find the name of a context inside the field argument to `@fromContext`: {field:?}"); + let dollar_start = strip_leading_ignored_tokens(field)?; + let mut dollar_iter = dollar_start.chars(); + if dollar_iter.next() != Some('$') { + bail!(r#"Failed to find leading "$" in @fromContext substring "{dollar_start}""#); } + let after_dollar = dollar_iter.as_str(); - if selection.is_empty() { + let context_start = strip_leading_ignored_tokens(after_dollar)?; + let Some(context_captures) = + iter_into_single_item(CONTEXT_PARSING_CONTEXT_PATTERN.captures_iter(context_start)) + else { bail!( - "Expected to find and a selection inside the field argument to `@fromContext`: {field:?}" + r#"Failed to find context name token and selection in @fromContext substring "{context_start}""# ); - } + }; + + let context = match context_captures.get(1).map(|m| m.as_str()) { + Some(context) if !context.is_empty() => context, + _ => { + bail!( + r#"Expected to find non-empty context name in @fromContext substring "{context_start}""# + ); + } + }; + + let selection = match context_captures.get(2).map(|m| m.as_str()) { + Some(selection) if !selection.is_empty() => selection, + _ => { + bail!( + r#"Expected to find non-empty selection in @fromContext substring "{context_start}""# + ); + } + }; - Ok(( - context.trim_end().to_owned(), - selection.trim_start().to_owned(), - )) + // PORT_NOTE: apollo_compiler's parsing code for field sets requires ignored tokens to be + // pre-stripped if curly braces are missing, so we additionally do that here. + let selection = strip_leading_ignored_tokens(selection)?; + Ok((context.to_owned(), selection.to_owned())) } #[cfg(test)] @@ -2508,6 +2562,9 @@ mod tests { assert_eq!(context, known_context); assert_eq!(selection, known_selection); } + // Ensure we don't backtrack in the comment regex. + assert!(super::parse_context("#comment $fakeContext fakeSelection").is_err()); + assert!(super::parse_context("$ #comment fakeContext fakeSelection").is_err()); } #[test] diff --git a/apollo-federation/src/query_graph/condition_resolver.rs b/apollo-federation/src/query_graph/condition_resolver.rs index c9f3b0da66..95ca4c978c 100644 --- a/apollo-federation/src/query_graph/condition_resolver.rs +++ b/apollo-federation/src/query_graph/condition_resolver.rs @@ -23,9 +23,13 @@ pub(crate) struct ContextMapEntry { pub(crate) levels_in_query_path: usize, pub(crate) path_tree: Option>, pub(crate) selection_set: SelectionSet, - pub(crate) param_name: Name, - pub(crate) arg_type: Node, - pub(crate) id: Name, + // PORT_NOTE: This field was renamed from the JS name (`paramName`) to better align with naming + // in ContextCondition. + pub(crate) argument_name: Name, + // PORT_NOTE: This field was renamed from the JS name (`argType`) to better align with naming in + // ContextCondition. + pub(crate) argument_type: Node, + pub(crate) context_id: Name, } /// Note that `ConditionResolver`s are guaranteed to be only called for edge with conditions. diff --git a/apollo-federation/src/query_graph/graph_path.rs b/apollo-federation/src/query_graph/graph_path.rs index 3bf55adc7d..99fe6aa60c 100644 --- a/apollo-federation/src/query_graph/graph_path.rs +++ b/apollo-federation/src/query_graph/graph_path.rs @@ -4,19 +4,17 @@ use std::fmt::Formatter; use std::fmt::Write; use std::hash::Hash; use std::ops::Deref; -use std::ops::DerefMut; use std::sync::atomic; use std::sync::Arc; -use apollo_compiler::ast::InputValueDefinition; use apollo_compiler::ast::Type; use apollo_compiler::ast::Value; use apollo_compiler::collections::IndexMap; use apollo_compiler::collections::IndexSet; -use apollo_compiler::executable::Argument; use apollo_compiler::Name; use apollo_compiler::Node; use either::Either; +use itertools::izip; use itertools::Itertools; use petgraph::graph::EdgeIndex; use petgraph::graph::EdgeReference; @@ -32,7 +30,6 @@ use crate::display_helpers::DisplayOption; use crate::display_helpers::DisplaySlice; use crate::display_helpers::State as IndentedFormatter; use crate::error::FederationError; -use crate::internal_error; use crate::is_leaf_type; use crate::link::federation_spec_definition::get_federation_spec_definition_from_subgraph; use crate::link::graphql_definition::BooleanOrVariable; @@ -43,10 +40,8 @@ use crate::operation::DirectiveList; use crate::operation::Field; use crate::operation::HasSelectionKey; use crate::operation::InlineFragment; -use crate::operation::NamedFragments; use crate::operation::SelectionId; use crate::operation::SelectionKey; -use crate::operation::SelectionMapperReturn; use crate::operation::SelectionSet; use crate::operation::SiblingTypename; use crate::query_graph::condition_resolver::ConditionResolution; @@ -59,24 +54,25 @@ use crate::query_graph::QueryGraphNodeType; use crate::query_plan::query_planner::EnabledOverrideConditions; use crate::query_plan::FetchDataPathElement; use crate::query_plan::QueryPlanCost; -use crate::schema::field_set::parse_field_set; +use crate::schema::field_set::parse_field_value_without_validation; +use crate::schema::field_set::validate_field_value; use crate::schema::position::AbstractTypeDefinitionPosition; use crate::schema::position::Captures; use crate::schema::position::CompositeTypeDefinitionPosition; -use crate::schema::position::FieldDefinitionPosition; use crate::schema::position::InterfaceFieldDefinitionPosition; use crate::schema::position::ObjectOrInterfaceTypeDefinitionPosition; use crate::schema::position::ObjectTypeDefinitionPosition; use crate::schema::position::OutputTypeDefinitionPosition; use crate::schema::position::TypeDefinitionPosition; use crate::schema::ValidFederationSchema; +use crate::utils::FallibleIterator; #[derive(Clone, serde::Serialize, Debug, Eq, PartialEq)] -pub(crate) struct ContextAtUsageEntry { +pub(crate) struct ContextUsageEntry { pub(crate) context_id: Name, pub(crate) relative_path: Vec, pub(crate) selection_set: SelectionSet, - pub(crate) subgraph_arg_type: Node, + pub(crate) subgraph_argument_type: Node, } /// An immutable path in a query graph. @@ -178,11 +174,15 @@ where // TODO(@TylerBloom): Add in once defer is supported. #[serde(skip)] defer_on_tail: Option, - /// At the point where a `@context` is set, we will have fields to select - context_to_selection: Vec>, - /// Where a context is used (i.e. `@fromContext`) there will exist a ContextAtUsageEntry map - /// (1 for each parameter) - parameter_to_context: Vec>, + // PORT_NOTE: This field was renamed because the JS name (`contextToSelection`) implied it was + // a map to selections, which it isn't. + /// The IDs of contexts that have matched at the edge, for each edge in the path. + matching_context_ids: Vec>, + // PORT_NOTE: This field was renamed because the JS name (`parameterToContext`) left confusion + // to how a parameter was different from an argument. + /// Maps of @fromContext arguments to info about the contexts used in those arguments, for each + /// edge in the path. + arguments_to_context_usages: Vec>, } impl std::fmt::Debug for GraphPath @@ -209,8 +209,8 @@ where runtime_types_of_tail, runtime_types_before_tail_if_last_is_cast, defer_on_tail, - context_to_selection: _, - parameter_to_context: _, + matching_context_ids: _, + arguments_to_context_usages: _, } = self; f.debug_struct("GraphPath") @@ -270,16 +270,16 @@ impl OverrideId { } } -pub(crate) type ContextToSelection = IndexSet; -pub(crate) type ParameterToContext = IndexMap; +pub(crate) type MatchingContextIds = IndexSet; +pub(crate) type ArgumentsToContextUsages = IndexMap; /// The item type for [`GraphPath::iter`] pub(crate) type GraphPathItem<'path, TTrigger, TEdge> = ( TEdge, &'path Arc, &'path Option>, - Option, - Option, + Option<&'path MatchingContextIds>, + Option<&'path ArgumentsToContextUsages>, ); /// A `GraphPath` whose triggers are operation elements (essentially meaning that the path has been @@ -916,11 +916,13 @@ impl TryFrom for Arc { } } +#[derive(derive_more::From)] pub(crate) enum GraphPathTriggerRef<'a> { Op(&'a OpGraphPathTrigger), Transition(&'a QueryGraphEdgeTransition), } +#[derive(derive_more::From)] pub(crate) enum GraphPathTriggerRefMut<'a> { Op(&'a mut OpGraphPathTrigger), Transition(&'a mut QueryGraphEdgeTransition), @@ -935,31 +937,7 @@ impl<'a> From<&'a GraphPathTrigger> for GraphPathTriggerRef<'a> { } } -impl<'a> From<&'a OpGraphPathTrigger> for GraphPathTriggerRef<'a> { - fn from(value: &'a OpGraphPathTrigger) -> Self { - Self::Op(value) - } -} - -impl<'a> From<&'a mut OpGraphPathTrigger> for GraphPathTriggerRefMut<'a> { - fn from(value: &'a mut OpGraphPathTrigger) -> Self { - Self::Op(value) - } -} - -impl<'a> From<&'a QueryGraphEdgeTransition> for GraphPathTriggerRef<'a> { - fn from(value: &'a QueryGraphEdgeTransition) -> Self { - Self::Transition(value) - } -} - -impl<'a> From<&'a mut QueryGraphEdgeTransition> for GraphPathTriggerRefMut<'a> { - fn from(value: &'a mut QueryGraphEdgeTransition) -> Self { - Self::Transition(value) - } -} - -/// `GraphPath` is generic over two type, `TTrigger` and `TEdge`. This trait helps abstract over +/// `GraphPath` is generic over two types, `TTrigger` and `TEdge`. This trait helps abstract over /// the `TTrigger` type bound. A `TTrigger` is one of the two types that make up the variants of /// the `GraphPathTrigger`. Rather than trying to cast into concrete types and cast back (and /// potentially raise errors), this trait provides ways to access the data needed within. @@ -976,14 +954,14 @@ pub(crate) trait GraphPathTriggerVariant: Eq + Hash + std::fmt::Debug { } } - fn get_field_parent_type<'a>(&'a self) -> Option + fn get_field_parent_type<'a>(&'a self) -> Option where &'a Self: Into>, { match self.into() { GraphPathTriggerRef::Op(trigger) => match trigger { OpGraphPathTrigger::OpPathElement(OpPathElement::Field(field)) => { - Some(field.field_position.clone()) + Some(field.field_position.parent()) } _ => None, }, @@ -991,7 +969,7 @@ pub(crate) trait GraphPathTriggerVariant: Eq + Hash + std::fmt::Debug { QueryGraphEdgeTransition::FieldCollection { field_definition_position, .. - } => Some(field_definition_position.clone()), + } => Some(field_definition_position.parent()), _ => None, }, } @@ -1025,8 +1003,8 @@ where runtime_types_of_tail: Arc::new(IndexSet::default()), runtime_types_before_tail_if_last_is_cast: None, defer_on_tail: None, - context_to_selection: Vec::default(), - parameter_to_context: Vec::default(), + matching_context_ids: Vec::default(), + arguments_to_context_usages: Vec::default(), }; path.runtime_types_of_tail = Arc::new(path.head_possible_runtime_types()?); Ok(path) @@ -1069,8 +1047,8 @@ where let mut edges = self.edges.clone(); let mut edge_triggers = self.edge_triggers.clone(); let mut edge_conditions = self.edge_conditions.clone(); - let mut context_to_selection = self.context_to_selection.clone(); - let mut parameter_to_context = self.parameter_to_context.clone(); + let mut matching_context_ids = self.matching_context_ids.clone(); + let mut arguments_to_context_usages = self.arguments_to_context_usages.clone(); let mut last_subgraph_entering_edge_info = if defer.is_none() { self.last_subgraph_entering_edge_info.clone() } else { @@ -1081,8 +1059,8 @@ where edges.push(edge); edge_triggers.push(Arc::new(trigger)); edge_conditions.push(condition_path_tree); - context_to_selection.push(None); - parameter_to_context.push(None); + matching_context_ids.push(None); + arguments_to_context_usages.push(None); return Ok(GraphPath { graph: self.graph.clone(), head: self.head, @@ -1102,8 +1080,8 @@ where ), runtime_types_before_tail_if_last_is_cast: None, defer_on_tail: defer, - context_to_selection, - parameter_to_context, + matching_context_ids, + arguments_to_context_usages, }); }; @@ -1241,8 +1219,10 @@ where } else { self.defer_on_tail.clone() }, - context_to_selection: self.context_to_selection.clone(), - parameter_to_context: self.parameter_to_context.clone(), + matching_context_ids: self.matching_context_ids.clone(), + arguments_to_context_usages: self + .arguments_to_context_usages + .clone(), }); } } @@ -1303,61 +1283,47 @@ where // We know last edge is not a cast. runtime_types_before_tail_if_last_is_cast: None, defer_on_tail: defer, - context_to_selection: self.context_to_selection.clone(), - parameter_to_context: self.parameter_to_context.clone(), + matching_context_ids: self.matching_context_ids.clone(), + arguments_to_context_usages: self.arguments_to_context_usages.clone(), }); } } - let (new_edge_conditions, new_context_to_selection, new_parameter_to_context) = + let (new_edge_conditions, new_matching_context_ids, new_arguments_to_context_usages) = self.merge_edge_conditions_with_resolution(&condition_path_tree, &context_map); - let last_parameter_to_context = new_parameter_to_context.last(); + let last_arguments_to_context_usages = new_arguments_to_context_usages.last(); - if let Some(Some(last_parameter_to_context)) = last_parameter_to_context { + if let Some(Some(last_arguments_to_context_usages)) = last_arguments_to_context_usages { // TODO: Perhaps it is better to explicitly cast this to `GraphPathTriggerRefMut` and // pull out the field from there. if let Some(field) = trigger.get_field_mut() { - let mut schema = field.schema.schema().clone().into_inner(); - let type_name = field.field_position.type_name(); - let field_name = field.field_position.field_name(); - let Some(field_def) = schema.types.get_mut(type_name).and_then(|t| match t { - apollo_compiler::schema::ExtendedType::Scalar(_) => None, - apollo_compiler::schema::ExtendedType::Object(obj) => { - obj.make_mut().fields.get_mut(field_name) - } - apollo_compiler::schema::ExtendedType::Interface(iface) => { - iface.make_mut().fields.get_mut(field_name) - } - apollo_compiler::schema::ExtendedType::Union(_) => None, - apollo_compiler::schema::ExtendedType::Enum(_) => None, - apollo_compiler::schema::ExtendedType::InputObject(_) => None, - }) else { - bail!("Unexpectedly failed to lookup field {type_name}.{field_name}") + // We need to add the extra @fromContext arguments to the trigger, but its likely + // pointing to a schema that doesn't have them, so we update the trigger to use the + // appropriate subgraph schema and position first. + let QueryGraphEdgeTransition::FieldCollection { + source, + field_definition_position, + .. + } = &edge_weight.transition + else { + bail!( + "Unexpectedly found field trigger for non-field edge {}", + edge_weight + ); }; - let field_def = field_def.deref_mut().make_mut(); - let mut updated_field_arguments = vec![]; - let mut updated_field_def_arguments = vec![]; - for (param_name, usage_entry) in last_parameter_to_context { - if !field_def - .arguments + field.schema = self.graph.schema_by_source(source)?.clone(); + field.field_position = field_definition_position.clone(); + + // Now we can append the extra @fromContext arguments. + let updated_field_arguments = + last_arguments_to_context_usages .iter() - .any(|arg| arg.name.as_str() == param_name.as_str()) - { - updated_field_def_arguments.push(Node::new(InputValueDefinition { - name: usage_entry.context_id.clone(), - ty: usage_entry.subgraph_arg_type.clone(), - default_value: None, - description: None, - directives: Default::default(), - })); - updated_field_arguments.push(Node::new(Argument { - name: param_name.clone(), - value: Node::new(Value::Variable(usage_entry.context_id.clone())), - })); - } - } - field_def.arguments.extend(updated_field_def_arguments); - field.schema = ValidFederationSchema::new(schema.validate()?)?; + .map(|(argument_name, usage_entry)| { + Node::new(apollo_compiler::executable::Argument { + name: argument_name.clone(), + value: Node::new(Value::Variable(usage_entry.context_id.clone())), + }) + }); field.arguments = field .arguments .iter() @@ -1413,8 +1379,8 @@ where } else { None }, - context_to_selection: new_context_to_selection, - parameter_to_context: new_parameter_to_context, + matching_context_ids: new_matching_context_ids, + arguments_to_context_usages: new_arguments_to_context_usages, }) } @@ -1426,21 +1392,23 @@ where ) -> ( Vec>>, Vec>>, - Vec>>, + Vec>>, ) { let mut edge_conditions = self.edge_conditions.clone(); - let mut context_to_selection = self.context_to_selection.clone(); - let mut parameter_to_context = self.parameter_to_context.clone(); + let mut matching_context_ids = self.matching_context_ids.clone(); + let mut arguments_to_context_usages = self.arguments_to_context_usages.clone(); edge_conditions.push(condition_path_tree.clone()); + matching_context_ids.push(None); if context_map.is_none() || context_map.as_ref().is_some_and(|m| m.is_empty()) { - context_to_selection.push(None); - parameter_to_context.push(None); - (edge_conditions, context_to_selection, parameter_to_context) + arguments_to_context_usages.push(None); + ( + edge_conditions, + matching_context_ids, + arguments_to_context_usages, + ) } else { - // parameter_to_context.push(Some(Arc::new(IndexMap::default()))); - context_to_selection.push(None); - let mut new_parameter_to_context = IndexMap::default(); + let mut new_arguments_to_context_usages = IndexMap::default(); for (_, entry) in context_map.iter().flat_map(|map| map.iter()) { let idx = edge_conditions.len() - entry.levels_in_query_path - 1; @@ -1450,51 +1418,55 @@ where .map_or_else(|| path_tree.clone(), |condition| condition.merge(path_tree)); edge_conditions[idx] = Some(merged_conditions); } - context_to_selection[idx] + matching_context_ids[idx] .get_or_insert_with(Default::default) - .insert(entry.id.clone()); + .insert(entry.context_id.clone()); - new_parameter_to_context.insert( - entry.param_name.clone(), - ContextAtUsageEntry { - context_id: entry.id.clone(), + new_arguments_to_context_usages.insert( + entry.argument_name.clone(), + ContextUsageEntry { + context_id: entry.context_id.clone(), relative_path: vec![ FetchDataPathElement::Parent; entry.levels_in_data_path ], selection_set: entry.selection_set.clone(), - subgraph_arg_type: entry.arg_type.clone(), + subgraph_argument_type: entry.argument_type.clone(), }, ); } - parameter_to_context.push(Some(new_parameter_to_context)); - (edge_conditions, context_to_selection, parameter_to_context) + arguments_to_context_usages.push(Some(new_arguments_to_context_usages)); + ( + edge_conditions, + matching_context_ids, + arguments_to_context_usages, + ) } } pub(crate) fn iter(&self) -> impl Iterator> { debug_assert_eq!(self.edges.len(), self.edge_triggers.len()); debug_assert_eq!(self.edges.len(), self.edge_conditions.len()); - debug_assert_eq!(self.edges.len(), self.context_to_selection.len()); - debug_assert_eq!(self.edges.len(), self.parameter_to_context.len()); - self.edges - .iter() - .copied() - .zip(&self.edge_triggers) - .zip(&self.edge_conditions) - .zip(&self.context_to_selection) - .zip(&self.parameter_to_context) - .map( - |((((edge, trigger), condition), context_to_selection), parameter_to_context)| { - ( - edge, - trigger, - condition, - context_to_selection.clone(), - parameter_to_context.clone(), - ) - }, - ) + debug_assert_eq!(self.edges.len(), self.matching_context_ids.len()); + debug_assert_eq!(self.edges.len(), self.arguments_to_context_usages.len()); + izip!( + self.edges.iter().copied(), + &self.edge_triggers, + &self.edge_conditions, + &self.matching_context_ids, + &self.arguments_to_context_usages, + ) + .map( + |(edge, trigger, condition, matching_context_ids, arguments_to_context_usages)| { + ( + edge, + trigger, + condition, + matching_context_ids.as_ref(), + arguments_to_context_usages.as_ref(), + ) + }, + ) } pub(crate) fn next_edges( @@ -1686,135 +1658,140 @@ where levels_in_data_path += 1; } let Some(e) = (*e).into() else { continue }; - if !was_unsatisfied && !context_map.contains_key(&ctx.named_parameter) { - if let Some(parent_type) = parent_type { - let parent_type = parent_type.parent(); - let subgraph_schema = - self.graph.schema_by_source(&ctx.subgraph_name)?; - let mut potential_matches: IndexSet = Default::default(); - ctx.types_with_context_set.iter().for_each(|pos| { - if pos.type_name() == parent_type.type_name() { - potential_matches.insert(parent_type.type_name().clone()); + if was_unsatisfied || context_map.contains_key(&ctx.argument_name) { + continue; + } + // There's a context match if in the supergraph schema, the field's parent type + // is equal to or a subtype of one of the @context types. + let Some(parent_type) = parent_type else { + continue; + }; + let supergraph_schema = self.graph.supergraph_schema()?; + let parent_type_in_supergraph: CompositeTypeDefinitionPosition = + supergraph_schema + .get_type(parent_type.type_name().clone())? + .try_into()?; + if !ctx.types_with_context_set.iter().fallible_any(|pos| { + if pos.type_name() == parent_type_in_supergraph.type_name() { + return Ok::<_, FederationError>(true); + } + match &parent_type_in_supergraph { + CompositeTypeDefinitionPosition::Object(parent_type_in_supergraph) => { + if parent_type_in_supergraph + .get(supergraph_schema.schema())? + .implements_interfaces + .iter() + .any(|item| &item.name == pos.type_name()) + { + return Ok(true); } - match &pos { - CompositeTypeDefinitionPosition::Object(obj_pos) => { - if let Ok(obj) = obj_pos.get(subgraph_schema.schema()) { - obj.implements_interfaces - .iter() - .filter(|item| { - &item.name == parent_type.type_name() - }) - .for_each(|item| { - potential_matches.insert(item.name.clone()); - }); - } - } - CompositeTypeDefinitionPosition::Interface(iface_pos) => { - if let Ok(iface) = iface_pos.get(subgraph_schema.schema()) { - iface - .implements_interfaces - .iter() - .filter(|item| { - &item.name == parent_type.type_name() - }) - .for_each(|_| { - potential_matches.insert(iface.name.clone()); - }); - } - } - CompositeTypeDefinitionPosition::Union(union_pos) => { - if let Ok(un) = union_pos.get(subgraph_schema.schema()) { - un.members - .iter() - .filter(|item| { - &item.name == parent_type.type_name() - }) - .for_each(|_| { - potential_matches.insert(un.name.clone()); - }); - } - } + } + CompositeTypeDefinitionPosition::Interface( + parent_type_in_supergraph, + ) => { + if parent_type_in_supergraph + .get(supergraph_schema.schema())? + .implements_interfaces + .iter() + .any(|item| &item.name == pos.type_name()) + { + return Ok(true); } - }); + } + _ => {} + } + let pos_in_supergraph: CompositeTypeDefinitionPosition = supergraph_schema + .get_type(pos.type_name().clone())? + .try_into()?; + if let CompositeTypeDefinitionPosition::Union(pos_in_supergraph) = + &pos_in_supergraph + { + if pos_in_supergraph + .get(supergraph_schema.schema())? + .members + .iter() + .any(|item| &item.name == parent_type_in_supergraph.type_name()) + { + return Ok(true); + } + } + Ok(false) + })? { + continue; + } - // get the selection set for the first match that parses - let selection_set = - potential_matches.iter().find_map(|parent_type_name| { - parse_field_set( - subgraph_schema, - parent_type_name.clone(), - &ctx.selection, - ) - .ok() - }); - - if let Some(selection_set) = selection_set { - selection_set.lazy_map( - &NamedFragments::default(), - |selection| { - if let OpPathElement::InlineFragment(fragment) = - selection.element()? - { - if let Some(CompositeTypeDefinitionPosition::Object( - obj, - )) = &fragment.type_condition_position - { - if !subgraph_schema - .possible_runtime_types(parent_type.clone())? - .contains(obj) - { - return Ok(SelectionMapperReturn::None); - } - } - } - Ok(SelectionMapperReturn::Selection(selection.clone())) - }, - )?; - let resolution = condition_resolver.resolve( - e, - context, - excluded_destinations, - excluded_conditions, - Some(&selection_set), - )?; - let Some(arg_indices) = - self.graph.subgraph_to_arg_indices.get(&ctx.subgraph_name) - else { - bail!("Unknown subgraph, {:?}, in QueryGraph::subgraph_to_arg_indices", ctx.subgraph_name) - }; - let Some(id) = arg_indices.get(&ctx.argument_coordinate).cloned() - else { - bail!("Unknown argument coordiate, {:?}, in QueryGraph::subgraph_to_arg_indices[{}]", ctx.argument_coordinate, ctx.subgraph_name) - }; + // We have a match, so parse the selection set against the field's parent type + // in the supergraph schema. + let mut selection_set = parse_field_value_without_validation( + &supergraph_schema, + parent_type_in_supergraph.type_name().clone(), + &ctx.selection, + )?; - match &resolution { - ConditionResolution::Satisfied { - cost, path_tree, .. - } => { - total_cost += cost; - let entry = ContextMapEntry { - levels_in_data_path, - levels_in_query_path, - path_tree: path_tree.clone(), - selection_set, - param_name: ctx.named_parameter.clone(), - arg_type: ctx.arg_type.clone(), - id, - }; - context_map.insert(ctx.named_parameter.clone(), entry); - } - ConditionResolution::Unsatisfied { .. } => { - was_unsatisfied = true - } - } - } else { - internal_error!( - "Could not parse selection {} over any types {:?}", - &ctx.selection, - potential_matches - ); + // In the current version of @context (v0.1), type conditions (if they appear at + // all) are only allowed at top-level, and are only allowed to reference object + // types. Due to duck-typing semantics, there may be type conditions that have + // an empty intersection in possible runtime types with their parent type, which + // means we need to remove those type conditions for the selection set to be + // considered valid GraphQL. + let possible_runtime_types = + supergraph_schema.possible_runtime_types(parent_type_in_supergraph)?; + selection_set.selection_set.selections = selection_set + .selection_set + .selections + .into_iter() + .map(|selection| { + let apollo_compiler::executable::Selection::InlineFragment( + inline_fragment, + ) = &selection + else { + return Ok::<_, FederationError>(Some(selection)); + }; + let Some(type_condition) = &inline_fragment.type_condition else { + return Ok(Some(selection)); + }; + let type_condition_pos: ObjectTypeDefinitionPosition = + supergraph_schema + .get_type(type_condition.clone())? + .try_into()?; + if possible_runtime_types.contains(&type_condition_pos) { + return Ok(Some(selection)); } + Ok(None) + }) + .process_results(|r| r.flatten().collect())?; + + // The field set should be valid now, so we'll validate and convert to our + // selection set representation. + let selection_set = validate_field_value(&supergraph_schema, selection_set)?; + let resolution = condition_resolver.resolve( + e, + context, + excluded_destinations, + excluded_conditions, + Some(&selection_set), + )?; + let context_id = self.graph.context_id_by_source_and_argument( + &ctx.subgraph_name, + &ctx.argument_coordinate, + )?; + match &resolution { + ConditionResolution::Satisfied { + cost, path_tree, .. + } => { + total_cost += cost; + let entry = ContextMapEntry { + levels_in_data_path, + levels_in_query_path, + path_tree: path_tree.clone(), + selection_set, + argument_name: ctx.argument_name.clone(), + argument_type: ctx.argument_type.clone(), + context_id: context_id.clone(), + }; + context_map.insert(ctx.argument_name.clone(), entry); } + ConditionResolution::Unsatisfied { .. } => was_unsatisfied = true, } } } @@ -1822,7 +1799,7 @@ where if edge_weight .required_contexts .iter() - .any(|ctx| !context_map.contains_key(&ctx.named_parameter)) + .any(|ctx| !context_map.contains_key(&ctx.argument_name)) { debug!("@fromContext requires a context that is not set in graph path"); return Ok(ConditionResolution::Unsatisfied { @@ -1865,6 +1842,9 @@ where excluded_conditions, None, )?; + if matches!(resolution, ConditionResolution::Unsatisfied { .. }) { + return Ok(ConditionResolution::Unsatisfied { reason: None }); + } if let Some(Some(last_edge)) = self.edges.last().map(|e| (*e).into()) { if matches!( edge_weight.transition, @@ -1916,10 +1896,12 @@ where } } if let ConditionResolution::Satisfied { + cost, context_map: ctx_map, .. } = &mut resolution { + *cost += total_cost; *ctx_map = Some(context_map); } debug!("Condition resolution: {resolution:?}"); @@ -2350,6 +2332,12 @@ where ) -> Option, override_conditions: &EnabledOverrideConditions, ) -> Result, FederationError> { + // TODO: Temporary fix to avoid optimization if context exists, a permanent fix is here: + // https://github.com/apollographql/federation/pull/3017#pullrequestreview-2083949094 + if self.graph.is_context_used() { + return Ok(None); + } + let mut current_node = start_node; for index in start_index..self.edges.len() { let trigger = &self.edge_triggers[index]; @@ -2686,8 +2674,11 @@ impl OpGraphPath { runtime_types_before_tail_if_last_is_cast: None, // TODO: The JS codebase copied this from the current path, which seems like a bug. defer_on_tail: self.defer_on_tail.clone(), - context_to_selection: self.context_to_selection.clone(), - parameter_to_context: self.parameter_to_context.clone(), + // PORT_NOTE: The JS codebase doesn't properly truncate these fields, this is a bug + // which we fix here. + matching_context_ids: self.matching_context_ids[0..prefix_length].to_vec(), + arguments_to_context_usages: self.arguments_to_context_usages[0..prefix_length] + .to_vec(), }) } diff --git a/apollo-federation/src/query_graph/mod.rs b/apollo-federation/src/query_graph/mod.rs index cb21e80a95..07f8ceeaff 100644 --- a/apollo-federation/src/query_graph/mod.rs +++ b/apollo-federation/src/query_graph/mod.rs @@ -134,18 +134,24 @@ impl TryFrom for ObjectTypeDefinitionPosition { } } -/// Contains all of the data necessary to connect the object field (`coordinate`) with the -/// `@fromContext` to its (grand)parent types contain a matching selection. +/// Contains all of the data necessary to connect the object field argument (`argument_coordinate`) +/// with the `@fromContext` to its (grand)parent types contain a matching selection. #[derive(Debug, PartialEq, Clone)] pub struct ContextCondition { context: String, subgraph_name: Arc, + // This is purposely left unparsed in query graphs, due to @fromContext selection sets being + // duck-typed. selection: String, types_with_context_set: IndexSet, + // PORT_NOTE: This field was renamed because the JS name (`namedParameter`) left confusion to + // how it was different from the argument name. + argument_name: Name, // PORT_NOTE: This field was renamed because the JS name (`coordinate`) was too vague. argument_coordinate: ObjectFieldArgumentDefinitionPosition, - named_parameter: Name, - arg_type: Node, + // PORT_NOTE: This field was renamed from the JS name (`argType`) for consistency with the rest + // of the naming in this struct. + argument_type: Node, } #[derive(Debug, PartialEq, Clone)] @@ -172,8 +178,8 @@ pub(crate) struct QueryGraphEdge { /// one of them has an @override with a label. If the override condition /// matches the query plan parameters, this edge can be taken. pub(crate) override_condition: Option, - /// All fields with `@fromContext` that need access to parental type with corresponding - /// `@context`. + /// All arguments with `@fromContext` that need to be matched to an upstream graph path field + /// whose parent type has the corresponding `@context`. pub(crate) required_contexts: Vec, } @@ -359,6 +365,8 @@ pub struct QueryGraph { /// same as `sources`, but is missing the dummy source FEDERATED_GRAPH_ROOT_SOURCE which isn't /// really a subgraph. subgraphs_by_name: IndexMap, ValidFederationSchema>, + /// For federated query graphs, this is the supergraph schema; otherwise, this is `None`. + supergraph_schema: Option, /// A map (keyed by source) that associates type names of the underlying schema on which this /// query graph was built to each of the nodes that points to a type of that name. Note that for /// a "federated" query graph source, each type name will only map to a single node. @@ -389,10 +397,13 @@ pub struct QueryGraph { /// lowered composition validation on a big composition (100+ subgraphs) from ~4 minutes to /// ~10 seconds. non_trivial_followup_edges: IndexMap>, - /// Maps each subgraph name to each field argument of `@fromContext`. - pub(crate) subgraph_to_args: IndexMap, Vec>, - /// Like `self.subgraph_to_args` but pairs each field argument with a unique identifier string. - pub(crate) subgraph_to_arg_indices: + // PORT_NOTE: This field was renamed from the JS name (`subgraphToArgIndices`) to better + // align with downstream code. + /// Maps subgraph names to another map, for any subgraph with usages of `@fromContext`. This + /// other map then maps subgraph argument positions/coordinates (for `@fromContext` arguments) + /// to a unique identifier string (specifically, unique across pairs of subgraph names and + /// argument coordinates). This identifier is called the "context ID". + arguments_to_context_ids_by_source: IndexMap, IndexMap>, } @@ -405,6 +416,12 @@ impl QueryGraph { &self.graph } + pub(crate) fn supergraph_schema(&self) -> Result { + self.supergraph_schema + .clone() + .ok_or_else(|| internal_error!("Supergraph schema unexpectedly missing")) + } + pub(crate) fn node_weight(&self, node: NodeIndex) -> Result<&QueryGraphNode, FederationError> { self.graph .node_weight(node) @@ -548,6 +565,23 @@ impl QueryGraph { }) } + pub(crate) fn context_id_by_source_and_argument( + &self, + source: &str, + argument: &ObjectFieldArgumentDefinitionPosition, + ) -> Result<&Name, FederationError> { + self.arguments_to_context_ids_by_source + .get(source) + .and_then(|r| r.get(argument)) + .ok_or_else(|| { + internal_error!("context ID unexpectedly missing for @fromContext argument") + }) + } + + pub(crate) fn is_context_used(&self) -> bool { + !self.arguments_to_context_ids_by_source.is_empty() + } + /// All outward edges from the given node (including self-key and self-root-type-resolution /// edges). Primarily used by `@defer`, when needing to re-enter a subgraph for a deferred /// section. diff --git a/apollo-federation/src/query_graph/path_tree.rs b/apollo-federation/src/query_graph/path_tree.rs index b4f4e820cb..8f3c828e98 100644 --- a/apollo-federation/src/query_graph/path_tree.rs +++ b/apollo-federation/src/query_graph/path_tree.rs @@ -9,8 +9,8 @@ use petgraph::graph::EdgeIndex; use petgraph::graph::NodeIndex; use serde::Serialize; -use super::graph_path::ContextToSelection; -use super::graph_path::ParameterToContext; +use super::graph_path::ArgumentsToContextUsages; +use super::graph_path::MatchingContextIds; use crate::error::FederationError; use crate::operation::SelectionSet; use crate::query_graph::graph_path::GraphPathItem; @@ -94,10 +94,14 @@ where pub(crate) conditions: Option>, /// The child `PathTree` reached by taking the edge. pub(crate) tree: Arc>, - // a list of contexts set at this point in the path tree - pub(crate) context_to_selection: Option, - // a list of contexts used at this point in the path tree - pub(crate) parameter_to_context: Option, + // PORT_NOTE: This field was renamed because the JS name (`contextToSelection`) implied it was + // a map to selections, which it isn't. + /// The IDs of contexts that have matched at the edge. + pub(crate) matching_context_ids: Option, + // PORT_NOTE: This field was renamed because the JS name (`parameterToContext`) left confusion + // to how a parameter was different from an argument. + /// A map of @fromContext arguments to info about the contexts used in those arguments. + pub(crate) arguments_to_context_usages: Option, } impl PartialEq for PathTreeChild @@ -248,8 +252,8 @@ where trigger: &'inputs Arc, conditions: Option>, sub_paths_and_selections: Vec<(GraphPathIter, Option<&'inputs Arc>)>, - context_to_selection: Option, - parameter_to_context: Option, + matching_context_ids: Option, + arguments_to_context_usages: Option, } let mut local_selection_sets = Vec::new(); @@ -259,8 +263,8 @@ where generic_edge, trigger, conditions, - context_to_selection, - parameter_to_context, + matching_context_ids, + arguments_to_context_usages, )) = graph_path_iter.next() else { // End of an input `GraphPath` @@ -289,17 +293,17 @@ where let existing = entry.into_mut(); existing.trigger = trigger; existing.conditions = merge_conditions(&existing.conditions, conditions); - if let Some(other) = context_to_selection { + if let Some(other) = matching_context_ids { existing - .context_to_selection + .matching_context_ids .get_or_insert_with(Default::default) - .extend(other); + .extend(other.iter().cloned()); } - if let Some(other) = parameter_to_context { + if let Some(other) = arguments_to_context_usages { existing - .parameter_to_context - .get_or_insert_with(IndexMap::default) - .extend(other); + .arguments_to_context_usages + .get_or_insert_with(Default::default) + .extend(other.iter().map(|(k, v)| (k.clone(), v.clone()))); } existing .sub_paths_and_selections @@ -311,8 +315,8 @@ where trigger, conditions: conditions.clone(), sub_paths_and_selections: vec![(graph_path_iter, selection)], - context_to_selection, - parameter_to_context, + matching_context_ids: matching_context_ids.cloned(), + arguments_to_context_usages: arguments_to_context_usages.cloned(), }); } } @@ -330,8 +334,8 @@ where by_unique_edge.target_node, child.sub_paths_and_selections, )?), - context_to_selection: child.context_to_selection.clone(), - parameter_to_context: child.parameter_to_context.clone(), + matching_context_ids: child.matching_context_ids.clone(), + arguments_to_context_usages: child.arguments_to_context_usages.clone(), })) } } @@ -365,8 +369,18 @@ where (Some(cond_a), Some(cond_b)) => cond_a.equals_same_root(cond_b), _ => false, } - && a.context_to_selection == b.context_to_selection - && a.parameter_to_context == b.parameter_to_context + && match (&a.matching_context_ids, &b.matching_context_ids) { + (Some(_), Some(_)) => a.matching_context_ids == b.matching_context_ids, + (_, _) => + a.matching_context_ids.as_ref().map(|c| c.is_empty()).unwrap_or(true) && + b.matching_context_ids.as_ref().map(|c| c.is_empty()).unwrap_or(true) + } + && match (&a.arguments_to_context_usages, &b.arguments_to_context_usages) { + (Some(_), Some(_)) => a.arguments_to_context_usages == b.arguments_to_context_usages, + (_, _) => + a.arguments_to_context_usages.as_ref().map(|c| c.is_empty()).unwrap_or(true) && + b.arguments_to_context_usages.as_ref().map(|c| c.is_empty()).unwrap_or(true) + } && a.tree.equals_same_root(&b.tree) }) } @@ -448,13 +462,13 @@ where trigger: child.trigger.clone(), conditions: merge_conditions(&child.conditions, &other_child.conditions), tree: child.tree.merge(&other_child.tree), - context_to_selection: merge_context_to_selection( - &child.context_to_selection, - &other_child.context_to_selection, + matching_context_ids: merge_matching_context_ids( + &child.matching_context_ids, + &other_child.matching_context_ids, ), - parameter_to_context: merge_parameter_to_context( - &child.parameter_to_context, - &other_child.parameter_to_context, + arguments_to_context_usages: merge_arguments_to_context_usages( + &child.arguments_to_context_usages, + &other_child.arguments_to_context_usages, ), }) } else { @@ -477,13 +491,13 @@ where } } -fn merge_context_to_selection( - a: &Option, - b: &Option, -) -> Option { +fn merge_matching_context_ids( + a: &Option, + b: &Option, +) -> Option { match (a, b) { (Some(a), Some(b)) => { - let mut merged: ContextToSelection = Default::default(); + let mut merged: MatchingContextIds = Default::default(); merged.extend(a.iter().cloned()); merged.extend(b.iter().cloned()); Some(merged) @@ -494,13 +508,13 @@ fn merge_context_to_selection( } } -fn merge_parameter_to_context( - a: &Option, - b: &Option, -) -> Option { +fn merge_arguments_to_context_usages( + a: &Option, + b: &Option, +) -> Option { match (a, b) { (Some(a), Some(b)) => { - let mut merged: ParameterToContext = Default::default(); + let mut merged: ArgumentsToContextUsages = Default::default(); merged.extend(a.iter().map(|(k, v)| (k.clone(), v.clone()))); merged.extend(b.iter().map(|(k, v)| (k.clone(), v.clone()))); Some(merged) diff --git a/apollo-federation/src/query_plan/fetch_dependency_graph.rs b/apollo-federation/src/query_plan/fetch_dependency_graph.rs index 4a2da90e95..c2d8caff50 100644 --- a/apollo-federation/src/query_plan/fetch_dependency_graph.rs +++ b/apollo-federation/src/query_plan/fetch_dependency_graph.rs @@ -77,6 +77,7 @@ use crate::subgraph::spec::ANY_SCALAR_NAME; use crate::subgraph::spec::ENTITIES_QUERY; use crate::supergraph::FEDERATION_REPRESENTATIONS_ARGUMENTS_NAME; use crate::supergraph::FEDERATION_REPRESENTATIONS_VAR_NAME; +use crate::utils::iter_into_single_item; use crate::utils::logging::snapshot; /// Represents the value of a `@defer(label:)` argument. @@ -684,16 +685,6 @@ impl FetchDependencyGraphNodePath { } } -/// If the `iter` yields a single element, return it. Else return `None`. -fn iter_into_single_item(mut iter: impl Iterator) -> Option { - let item = iter.next()?; - if iter.next().is_none() { - Some(item) - } else { - None - } -} - impl FetchDependencyGraph { pub(crate) fn new( supergraph_schema: ValidFederationSchema, @@ -3948,7 +3939,7 @@ fn compute_nodes_for_op_path_element<'a>( (stack_item.node_id, &stack_item.node_path), // If setting a context, add __typename to the site where we are retrieving context from // since the context rewrites path will start with a type condition. - if child.context_to_selection.is_some() { + if child.matching_context_ids.is_some() { Some(edge_id) } else { None @@ -3957,12 +3948,12 @@ fn compute_nodes_for_op_path_element<'a>( created_nodes, )?; - if let Some(context_to_selection) = &child.context_to_selection { + if let Some(matching_context_ids) = &child.matching_context_ids { let mut condition_nodes = vec![conditions_node_data.conditions_merge_node_id]; condition_nodes.extend(&conditions_node_data.created_node_ids); let mut context_to_condition_nodes = stack_item.context_to_condition_nodes.deref().clone(); - for context in context_to_selection { + for context in matching_context_ids { context_to_condition_nodes.insert(context.clone(), condition_nodes.clone()); } updated.context_to_condition_nodes = Arc::new(context_to_condition_nodes); @@ -3987,17 +3978,17 @@ fn compute_nodes_for_op_path_element<'a>( // If the edge uses context variables, every context used must be set in a different parent // node or else we need to create a new one. - if let Some(parameter_to_context) = &child.parameter_to_context { + if let Some(arguments_to_context_usages) = &child.arguments_to_context_usages { let mut conditions_nodes: IndexSet = Default::default(); let mut is_subgraph_jump_needed = false; - for context_entry in parameter_to_context.values() { + for context_usage in arguments_to_context_usages.values() { let Some(context_nodes) = updated .context_to_condition_nodes - .get(&context_entry.context_id) + .get(&context_usage.context_id) else { bail!( "Could not find condition nodes for context {}", - context_entry.context_id + context_usage.context_id ); }; conditions_nodes.extend(context_nodes); @@ -4106,14 +4097,14 @@ fn compute_nodes_for_op_path_element<'a>( } // Add context renamers. - for context_entry in parameter_to_context.values() { + for context_entry in arguments_to_context_usages.values() { let updated_node = FetchDependencyGraph::node_weight_mut( &mut dependency_graph.graph, updated.node_id, )?; updated_node.add_input_context( context_entry.context_id.clone(), - context_entry.subgraph_arg_type.clone(), + context_entry.subgraph_argument_type.clone(), )?; updated_node.add_context_renamers_for_selection_set( Some(&context_entry.selection_set), @@ -4139,7 +4130,7 @@ fn compute_nodes_for_op_path_element<'a>( .iter() .filter(|e| matches!((**e).deref(), OpPathElement::Field(_))) .count(); - for context_entry in parameter_to_context.values() { + for context_entry in arguments_to_context_usages.values() { let new_relative_path = &context_entry.relative_path [..(context_entry.relative_path.len() - num_fields)]; let updated_node = FetchDependencyGraph::node_weight_mut( @@ -4148,7 +4139,7 @@ fn compute_nodes_for_op_path_element<'a>( )?; updated_node.add_input_context( context_entry.context_id.clone(), - context_entry.subgraph_arg_type.clone(), + context_entry.subgraph_argument_type.clone(), )?; updated_node.add_context_renamers_for_selection_set( Some(&context_entry.selection_set), diff --git a/apollo-federation/src/schema/field_set.rs b/apollo-federation/src/schema/field_set.rs index a0a9375405..d81349d4a8 100644 --- a/apollo-federation/src/schema/field_set.rs +++ b/apollo-federation/src/schema/field_set.rs @@ -63,23 +63,23 @@ fn check_absence_of_aliases(selection_set: &SelectionSet) -> Result<(), Federati pub(crate) fn parse_field_set( schema: &ValidFederationSchema, parent_type_name: NamedType, - value: &str, + field_set: &str, ) -> Result { // Note this parsing takes care of adding curly braces ("{" and "}") if they aren't in the // string. let field_set = FieldSet::parse_and_validate( schema.schema(), parent_type_name, - value, + field_set, "field_set.graphql", )?; - // field set should not contain any named fragments + // A field set should not contain any named fragments. let named_fragments = NamedFragments::new(&IndexMap::default(), schema); let selection_set = SelectionSet::from_selection_set(&field_set.selection_set, &named_fragments, schema)?; - // Validate the field set has no aliases. + // Validate that the field set has no aliases. check_absence_of_aliases(&selection_set)?; Ok(selection_set) @@ -93,12 +93,12 @@ pub(crate) fn parse_field_set( pub(crate) fn parse_field_set_without_normalization( schema: &Valid, parent_type_name: NamedType, - value: &str, + field_set: &str, ) -> Result { // Note this parsing takes care of adding curly braces ("{" and "}") if they aren't in the // string. let field_set = - FieldSet::parse_and_validate(schema, parent_type_name, value, "field_set.graphql")?; + FieldSet::parse_and_validate(schema, parent_type_name, field_set, "field_set.graphql")?; Ok(field_set.into_inner().selection_set) } @@ -109,12 +109,12 @@ pub(crate) fn parse_field_set_without_normalization( pub(crate) fn collect_target_fields_from_field_set( schema: &Valid, parent_type_name: NamedType, - value: &str, + field_set: &str, ) -> Result, FederationError> { // Note this parsing takes care of adding curly braces ("{" and "}") if they aren't in the // string. let field_set = - FieldSet::parse_and_validate(schema, parent_type_name, value, "field_set.graphql")?; + FieldSet::parse_and_validate(schema, parent_type_name, field_set, "field_set.graphql")?; let mut stack = vec![&field_set.selection_set]; let mut fields = vec![]; while let Some(selection_set) = stack.pop() { @@ -166,6 +166,41 @@ pub(crate) fn collect_target_fields_from_field_set( Ok(fields) } +pub(crate) fn parse_field_value_without_validation( + schema: &ValidFederationSchema, + parent_type_name: NamedType, + field_value: &str, +) -> Result { + // Note this parsing takes care of adding curly braces ("{" and "}") if they aren't in the + // string. + Ok(FieldSet::parse( + schema.schema(), + parent_type_name, + field_value, + "field_set.graphql", + )?) +} + +// Similar to parse_field_set(), we explicitly forbid aliases for field values. In this case though, +// it's because field value evaluation semantics means aliases would be stripped out and have no +// effect. +pub(crate) fn validate_field_value( + schema: &ValidFederationSchema, + field_value: FieldSet, +) -> Result { + field_value.validate(schema.schema())?; + + // A field value should not contain any named fragments. + let named_fragments = NamedFragments::new(&IndexMap::default(), schema); + let selection_set = + SelectionSet::from_selection_set(&field_value.selection_set, &named_fragments, schema)?; + + // Validate that the field value has no aliases. + check_absence_of_aliases(&selection_set)?; + + Ok(selection_set) +} + #[cfg(test)] mod tests { use apollo_compiler::Name; diff --git a/apollo-federation/src/schema/position.rs b/apollo-federation/src/schema/position.rs index 377968542e..916f10aa4f 100644 --- a/apollo-federation/src/schema/position.rs +++ b/apollo-federation/src/schema/position.rs @@ -232,6 +232,21 @@ impl TypeDefinitionPosition { )), } } + + pub(crate) fn insert_directive( + &self, + schema: &mut FederationSchema, + directive: Component, + ) -> Result<(), FederationError> { + match self { + TypeDefinitionPosition::Scalar(type_) => type_.insert_directive(schema, directive), + TypeDefinitionPosition::Object(type_) => type_.insert_directive(schema, directive), + TypeDefinitionPosition::Interface(type_) => type_.insert_directive(schema, directive), + TypeDefinitionPosition::Union(type_) => type_.insert_directive(schema, directive), + TypeDefinitionPosition::Enum(type_) => type_.insert_directive(schema, directive), + TypeDefinitionPosition::InputObject(type_) => type_.insert_directive(schema, directive), + } + } } fallible_conversions!(TypeDefinitionPosition::Scalar -> ScalarTypeDefinitionPosition); @@ -401,6 +416,24 @@ impl CompositeTypeDefinitionPosition { )), } } + + pub(crate) fn insert_directive( + &self, + schema: &mut FederationSchema, + directive: Component, + ) -> Result<(), FederationError> { + match self { + CompositeTypeDefinitionPosition::Object(type_) => { + type_.insert_directive(schema, directive) + } + CompositeTypeDefinitionPosition::Interface(type_) => { + type_.insert_directive(schema, directive) + } + CompositeTypeDefinitionPosition::Union(type_) => { + type_.insert_directive(schema, directive) + } + } + } } fallible_conversions!(CompositeTypeDefinitionPosition::Object -> ObjectTypeDefinitionPosition); diff --git a/apollo-federation/src/subgraph/mod.rs b/apollo-federation/src/subgraph/mod.rs index 4ba138ba8f..d36382e73d 100644 --- a/apollo-federation/src/subgraph/mod.rs +++ b/apollo-federation/src/subgraph/mod.rs @@ -22,6 +22,7 @@ use crate::subgraph::spec::AppliedFederationLink; use crate::subgraph::spec::FederationSpecDefinitions; use crate::subgraph::spec::LinkSpecDefinitions; use crate::subgraph::spec::ANY_SCALAR_NAME; +use crate::subgraph::spec::CONTEXTFIELDVALUE_SCALAR_NAME; use crate::subgraph::spec::ENTITIES_QUERY; use crate::subgraph::spec::ENTITY_UNION_NAME; use crate::subgraph::spec::FEDERATION_V2_DIRECTIVE_NAMES; @@ -175,6 +176,7 @@ impl Subgraph { schema: &mut Schema, fed_definitions: &FederationSpecDefinitions, ) -> Result<(), FederationError> { + // scalar FieldSet let fieldset_scalar_name = &fed_definitions.fieldset_scalar_name; schema .types @@ -185,6 +187,19 @@ impl Subgraph { .into() }); + // scalar ContextFieldValue + let namespaced_contextfieldvalue_scalar_name = + fed_definitions.namespaced_type_name(&CONTEXTFIELDVALUE_SCALAR_NAME, false); + if let Entry::Vacant(entry) = schema + .types + .entry(namespaced_contextfieldvalue_scalar_name.clone()) + { + let type_definition = fed_definitions.contextfieldvalue_scalar_definition(&Some( + namespaced_contextfieldvalue_scalar_name, + )); + entry.insert(type_definition.into()); + } + for directive_name in &FEDERATION_V2_DIRECTIVE_NAMES { let namespaced_directive_name = fed_definitions.namespaced_type_name(directive_name, true); diff --git a/apollo-federation/src/subgraph/spec.rs b/apollo-federation/src/subgraph/spec.rs index 4e8676e498..aa37af70e0 100644 --- a/apollo-federation/src/subgraph/spec.rs +++ b/apollo-federation/src/subgraph/spec.rs @@ -51,6 +51,7 @@ pub const REQUIRES_DIRECTIVE_NAME: Name = name!("requires"); pub const SHAREABLE_DIRECTIVE_NAME: Name = name!("shareable"); pub const TAG_DIRECTIVE_NAME: Name = name!("tag"); pub const FIELDSET_SCALAR_NAME: Name = name!("FieldSet"); +pub const CONTEXTFIELDVALUE_SCALAR_NAME: Name = name!("ContextFieldValue"); // federated types pub const ANY_SCALAR_NAME: Name = name!("_Any"); @@ -320,6 +321,15 @@ impl FederationSpecDefinitions { } } + /// scalar ContextFieldValue + pub fn contextfieldvalue_scalar_definition(&self, alias: &Option) -> ScalarType { + ScalarType { + description: None, + name: alias.clone().unwrap_or(CONTEXTFIELDVALUE_SCALAR_NAME), + directives: Default::default(), + } + } + fn fields_argument_definition(&self) -> Result { Ok(InputValueDefinition { description: None, @@ -425,7 +435,7 @@ impl FederationSpecDefinitions { // `from` is a conventional prefix used in conversion methods, which do not // take `self` as an argument. This function does **not** perform // conversion, but extracts `@fromContext` directive definition. - /// directive @fromContext(field: String!) on ARGUMENT_DEFINITION + /// directive @fromContext(field: ContextFieldValue) on ARGUMENT_DEFINITION #[allow(clippy::wrong_self_convention)] fn from_context_directive_definition(&self, alias: &Option) -> DirectiveDefinition { DirectiveDefinition { @@ -434,7 +444,8 @@ impl FederationSpecDefinitions { arguments: vec![InputValueDefinition { description: None, name: name!("field"), - ty: ty!(String!).into(), + ty: Type::Named(self.namespaced_type_name(&CONTEXTFIELDVALUE_SCALAR_NAME, false)) + .into(), default_value: None, directives: Default::default(), } diff --git a/apollo-federation/src/supergraph/mod.rs b/apollo-federation/src/supergraph/mod.rs index 2ae9173201..a5547c27e6 100644 --- a/apollo-federation/src/supergraph/mod.rs +++ b/apollo-federation/src/supergraph/mod.rs @@ -47,7 +47,7 @@ pub use self::subgraph::ValidFederationSubgraphs; use crate::error::FederationError; use crate::error::MultipleFederationErrors; use crate::error::SingleFederationError; -use crate::link::context_spec_definition::CONTEXT_VERSIONS; +use crate::link::context_spec_definition::ContextSpecDefinition; use crate::link::cost_spec_definition::CostSpecDefinition; use crate::link::federation_spec_definition::get_federation_spec_definition_from_subgraph; use crate::link::federation_spec_definition::FederationSpecDefinition; @@ -94,7 +94,7 @@ pub(crate) fn extract_subgraphs_from_supergraph( validate_extracted_subgraphs: Option, ) -> Result { let validate_extracted_subgraphs = validate_extracted_subgraphs.unwrap_or(true); - let (link_spec_definition, join_spec_definition) = + let (link_spec_definition, join_spec_definition, context_spec_definition) = crate::validate_supergraph_for_query_planning(supergraph_schema)?; let is_fed_1 = *join_spec_definition.version() == Version { major: 0, minor: 1 }; let (mut subgraphs, federation_spec_definitions, graph_enum_value_name_to_subgraph_name) = @@ -126,6 +126,7 @@ pub(crate) fn extract_subgraphs_from_supergraph( &graph_enum_value_name_to_subgraph_name, &federation_spec_definitions, join_spec_definition, + context_spec_definition, &filtered_types, )?; } @@ -213,6 +214,7 @@ fn collect_empty_subgraphs( name: graph_arguments.name.to_owned(), url: graph_arguments.url.to_owned(), schema: new_empty_fed_2_subgraph_schema()?, + graph_enum_value: enum_value_name.clone(), }; let federation_link = &subgraph .schema @@ -260,6 +262,7 @@ fn extract_subgraphs_from_fed_2_supergraph( graph_enum_value_name_to_subgraph_name: &IndexMap>, federation_spec_definitions: &IndexMap, join_spec_definition: &'static JoinSpecDefinition, + context_spec_definition: Option<&'static ContextSpecDefinition>, filtered_types: &Vec, ) -> Result<(), FederationError> { let original_directive_names = get_apollo_directive_names(supergraph_schema)?; @@ -276,6 +279,7 @@ fn extract_subgraphs_from_fed_2_supergraph( graph_enum_value_name_to_subgraph_name, federation_spec_definitions, join_spec_definition, + context_spec_definition, filtered_types, &original_directive_names, )?; @@ -302,7 +306,6 @@ fn extract_subgraphs_from_fed_2_supergraph( supergraph_schema, subgraphs, graph_enum_value_name_to_subgraph_name, - federation_spec_definitions, join_spec_definition, &union_types, )?; @@ -392,12 +395,14 @@ fn extract_subgraphs_from_fed_2_supergraph( Ok(()) } +#[allow(clippy::too_many_arguments)] fn add_all_empty_subgraph_types( supergraph_schema: &FederationSchema, subgraphs: &mut FederationSubgraphs, graph_enum_value_name_to_subgraph_name: &IndexMap>, federation_spec_definitions: &IndexMap, join_spec_definition: &'static JoinSpecDefinition, + context_spec_definition: Option<&'static ContextSpecDefinition>, filtered_types: &Vec, original_directive_names: &IndexMap, ) -> Result { @@ -469,9 +474,12 @@ fn add_all_empty_subgraph_types( types_mut.push(add_empty_type( type_definition_position.clone(), &type_directive_applications, + type_.directives(), + supergraph_schema, subgraphs, graph_enum_value_name_to_subgraph_name, federation_spec_definitions, + context_spec_definition, )?); } } @@ -485,12 +493,16 @@ fn add_all_empty_subgraph_types( }) } +#[allow(clippy::too_many_arguments)] fn add_empty_type( type_definition_position: TypeDefinitionPosition, type_directive_applications: &Vec, + directives: &DirectiveList, + supergraph_schema: &FederationSchema, subgraphs: &mut FederationSubgraphs, graph_enum_value_name_to_subgraph_name: &IndexMap>, federation_spec_definitions: &IndexMap, + context_spec_definition: Option<&'static ContextSpecDefinition>, ) -> Result { // In fed2, we always mark all types with `@join__type` but making sure. if type_directive_applications.is_empty() { @@ -672,25 +684,52 @@ fn add_empty_type( } .into()); } - TypeDefinitionPosition::Object(pos) => { - pos.insert_directive(&mut subgraph.schema, key_directive)?; - } - TypeDefinitionPosition::Interface(pos) => { - pos.insert_directive(&mut subgraph.schema, key_directive)?; - } - TypeDefinitionPosition::Union(pos) => { - pos.insert_directive(&mut subgraph.schema, key_directive)?; - } - TypeDefinitionPosition::Enum(pos) => { - pos.insert_directive(&mut subgraph.schema, key_directive)?; - } - TypeDefinitionPosition::InputObject(pos) => { - pos.insert_directive(&mut subgraph.schema, key_directive)?; + _ => { + subgraph_type_definition_position + .insert_directive(&mut subgraph.schema, key_directive)?; } }; } } + if let Some(context_spec_definition) = context_spec_definition { + let context_directive_definition = + context_spec_definition.context_directive_definition(supergraph_schema)?; + for directive in directives.get_all(&context_directive_definition.name) { + let context_directive_application = + context_spec_definition.context_directive_arguments(directive)?; + let (subgraph_name, context_name) = context_directive_application + .name + .rsplit_once("__") + .ok_or_else(|| SingleFederationError::InvalidFederationSupergraph { + message: format!( + "Invalid context \"{}\" in supergraph schema", + context_directive_application.name + ), + })?; + let subgraph = subgraphs.get_mut(subgraph_name).ok_or_else(|| { + SingleFederationError::Internal { + message: + "All subgraphs should have been created by \"collect_empty_subgraphs()\"" + .to_owned(), + } + })?; + let federation_spec_definition = federation_spec_definitions + .get(&subgraph.graph_enum_value) + .ok_or_else(|| SingleFederationError::InvalidFederationSupergraph { + message: "Subgraph unexpectedly does not use federation spec".to_owned(), + })?; + let context_directive = federation_spec_definition + .context_directive(&subgraph.schema, context_name.to_owned())?; + let subgraph_type_definition_position: CompositeTypeDefinitionPosition = subgraph + .schema + .get_type(type_definition_position.type_name().clone())? + .try_into()?; + subgraph_type_definition_position + .insert_directive(&mut subgraph.schema, Component::new(context_directive))?; + } + } + Ok(type_info) } @@ -713,16 +752,6 @@ fn extract_object_type_content( message: "@join__implements should exist for a fed2 supergraph".to_owned(), })?; - let context = supergraph_schema - .metadata() - .and_then(|metadata| metadata.for_identity(&Identity::context_identity())) - .and_then(|context_link| CONTEXT_VERSIONS.find(&context_link.url.version)) - .and_then(|context_spec_def| { - context_spec_def - .context_directive_name_in_schema(supergraph_schema) - .ok() - .map(|name_in_schema| (context_spec_def, name_in_schema)) - }); for TypeInfo { name: type_name, subgraph_info, @@ -761,17 +790,6 @@ fn extract_object_type_content( )?; } - if let Some((_context_spec_def, name_in_supergraph)) = &context { - apply_context_to_type( - type_, - subgraphs, - graph_enum_value_name_to_subgraph_name, - federation_spec_definitions, - name_in_supergraph, - &CompositeTypeDefinitionPosition::Object(pos.clone()), - )?; - } - for graph_enum_value in subgraph_info.keys() { let subgraph = get_subgraph( subgraphs, @@ -998,27 +1016,6 @@ fn extract_interface_type_content( } } - let context = supergraph_schema - .metadata() - .and_then(|metadata| metadata.for_identity(&Identity::context_identity())) - .and_then(|context_link| CONTEXT_VERSIONS.find(&context_link.url.version)) - .and_then(|context_spec_def| { - context_spec_def - .context_directive_name_in_schema(supergraph_schema) - .ok() - .map(|name_in_schema| (context_spec_def, name_in_schema)) - }); - if let Some((_context_spec_def, name_in_supergraph)) = &context { - apply_context_to_type( - type_, - subgraphs, - graph_enum_value_name_to_subgraph_name, - federation_spec_definitions, - name_in_supergraph, - &CompositeTypeDefinitionPosition::Interface(pos.clone()), - )?; - } - for (field_name, field) in type_.fields.iter() { let mut field_directive_applications = Vec::new(); for directive in field.directives.get_all(&field_directive_definition.name) { @@ -1112,7 +1109,6 @@ fn extract_union_type_content( supergraph_schema: &FederationSchema, subgraphs: &mut FederationSubgraphs, graph_enum_value_name_to_subgraph_name: &IndexMap>, - federation_spec_definitions: &IndexMap, join_spec_definition: &JoinSpecDefinition, info: &[TypeInfo], ) -> Result<(), FederationError> { @@ -1196,28 +1192,6 @@ fn extract_union_type_content( )?; } } - - let context = supergraph_schema - .metadata() - .and_then(|metadata| metadata.for_identity(&Identity::context_identity())) - .and_then(|context_link| CONTEXT_VERSIONS.find(&context_link.url.version)) - .and_then(|context_spec_def| { - context_spec_def - .context_directive_name_in_schema(supergraph_schema) - .ok() - .map(|name_in_schema| (context_spec_def, name_in_schema)) - }); - - if let Some((_context_spec_def, name_in_supergraph)) = &context { - apply_context_to_type( - type_, - subgraphs, - graph_enum_value_name_to_subgraph_name, - federation_spec_definitions, - name_in_supergraph, - &CompositeTypeDefinitionPosition::Union(pos.clone()), - )?; - } } Ok(()) @@ -1554,11 +1528,7 @@ fn add_subgraph_field( } = args; let (_, context_name_in_subgraph) = context.rsplit_once("__").ok_or_else(|| { SingleFederationError::InvalidFederationSupergraph { - message: format!( - "Could not parse context field from supergraph '{}'", - context - ) - .to_owned(), + message: format!(r#"Invalid context "{}" in supergraph schema"#, context), } })?; @@ -1666,16 +1636,6 @@ fn get_subgraph<'subgraph>( }) } -fn get_index_from_subgraph_name<'a>( - graph_enum_value_name_to_subgraph_name: &'a IndexMap>, - subgraph_name: &'a Name, -) -> Option<&'a Name> { - graph_enum_value_name_to_subgraph_name - .iter() - .find(|(_, v)| v.as_ref() == subgraph_name.as_str()) - .map(|(k, _)| k) -} - lazy_static! { static ref EXECUTABLE_DIRECTIVE_LOCATIONS: IndexSet = { [ @@ -1693,103 +1653,6 @@ lazy_static! { }; } -fn insert_directive( - schema: &mut FederationSchema, - pos: &CompositeTypeDefinitionPosition, - directive: Component, -) -> Result<(), FederationError> { - match pos { - CompositeTypeDefinitionPosition::Union(pos) => pos.insert_directive(schema, directive), - CompositeTypeDefinitionPosition::Object(pos) => pos.insert_directive(schema, directive), - CompositeTypeDefinitionPosition::Interface(pos) => pos.insert_directive(schema, directive), - } -} - -trait CompositeType { - fn directives(&self) -> &DirectiveList; -} - -impl CompositeType for UnionType { - fn directives(&self) -> &DirectiveList { - &self.directives - } -} - -impl CompositeType for ObjectType { - fn directives(&self) -> &DirectiveList { - &self.directives - } -} - -impl CompositeType for InterfaceType { - fn directives(&self) -> &DirectiveList { - &self.directives - } -} - -fn apply_context_to_type( - ty: &Node, - subgraphs: &mut FederationSubgraphs, - graph_enum_value_name_to_subgraph_name: &IndexMap>, - federation_spec_definitions: &IndexMap, - context_name_in_supergraph: &Name, - pos: &CompositeTypeDefinitionPosition, -) -> Result<(), FederationError> -where - T: CompositeType, -{ - for directive in ty.directives().get_all(context_name_in_supergraph.as_str()) { - FederationSpecDefinition::context_directive_arguments(directive).and_then( - |context_name| { - let mut arr = context_name.name.split("__"); - let subgraph_name = arr.next().ok_or_else(|| { - SingleFederationError::InvalidFederationSupergraph { - message: format!( - "Could not parse context name from supergraph '{}'", - context_name_in_supergraph - ) - .to_owned(), - } - })?; - let subgraph_name = Name::new_unchecked(subgraph_name); - let subgraph_index = get_index_from_subgraph_name( - graph_enum_value_name_to_subgraph_name, - &subgraph_name, - ) - .ok_or_else(|| SingleFederationError::InvalidSubgraph { - message: format!("Could not look up subgraph by name '{}'", subgraph_name) - .to_owned(), - })?; - let subgraph = get_subgraph( - subgraphs, - graph_enum_value_name_to_subgraph_name, - subgraph_index, - )?; - - let federation_spec_definition = federation_spec_definitions - .get(subgraph_index) - .ok_or_else(|| SingleFederationError::InvalidFederationSupergraph { - message: "Subgraph unexpectedly does not use federation spec".to_owned(), - })?; - let context_in_subgraph = arr.last().ok_or_else(|| { - SingleFederationError::InvalidFederationSupergraph { - message: format!( - "Could not parse context name from supergraph '{}'", - context_name_in_supergraph - ) - .to_owned(), - } - })?; - let context_directive = federation_spec_definition - .context_directive(&subgraph.schema, context_in_subgraph.to_string())?; - insert_directive(&mut subgraph.schema, pos, context_directive.into())?; - Ok(()) - }, - )?; - } - Ok(()) -} - fn remove_unused_types_from_subgraph(schema: &mut FederationSchema) -> Result<(), FederationError> { // We now do an additional path on all types because we sometimes added types to subgraphs // without being sure that the subgraph had the type in the first place (especially with the diff --git a/apollo-federation/src/supergraph/schema.rs b/apollo-federation/src/supergraph/schema.rs index 82154f0d2e..46aa19618e 100644 --- a/apollo-federation/src/supergraph/schema.rs +++ b/apollo-federation/src/supergraph/schema.rs @@ -99,10 +99,12 @@ pub(crate) fn new_empty_fed_2_subgraph_schema() -> Result(mut iter: impl Iterator) -> Option { + let item = iter.next()?; + if iter.next().is_none() { + Some(item) + } else { + None + } +} diff --git a/apollo-federation/tests/query_plan/build_query_plan_tests/context.rs b/apollo-federation/tests/query_plan/build_query_plan_tests/context.rs index 4bdf9f41b1..2a2c03bb3b 100644 --- a/apollo-federation/tests/query_plan/build_query_plan_tests/context.rs +++ b/apollo-federation/tests/query_plan/build_query_plan_tests/context.rs @@ -328,7 +328,7 @@ fn set_context_test_variable_is_already_in_a_different_fetch_group() { } => { ... on U { - field(a: $contextualArgument_1_0) + field(a: $contextualArgument_2_0) } } }, @@ -345,7 +345,7 @@ fn set_context_test_variable_is_already_in_a_different_fetch_group() { node.context_rewrites, vec![Arc::new(FetchDataRewrite::KeyRenamer( FetchDataKeyRenamer { - rename_key_to: Name::new("contextualArgument_1_0").unwrap(), + rename_key_to: Name::new("contextualArgument_2_0").unwrap(), path: vec![ FetchDataPathElement::Parent, FetchDataPathElement::TypenameEquals(Name::new("T").unwrap()), @@ -1308,7 +1308,7 @@ fn set_context_test_before_key_resolution_transition() { } => { ... on Child { - prop(legacyUserId: $contextualArgument_1_0) + prop(legacyUserId: $contextualArgument_2_0) } } }, @@ -1440,7 +1440,7 @@ fn set_context_test_efficiently_merge_fetch_groups() { { ... on Customer { accounts { - foo(ctx_id5: $contextualArgument_1_0, ctx_mid: $contextualArgument_1_1) { + foo(ctx_id5: $contextualArgument_3_0, ctx_mid: $contextualArgument_3_1) { id } } @@ -1460,7 +1460,7 @@ fn set_context_test_efficiently_merge_fetch_groups() { node.context_rewrites, vec![ Arc::new(FetchDataRewrite::KeyRenamer(FetchDataKeyRenamer { - rename_key_to: Name::new("contextualArgument_1_0").unwrap(), + rename_key_to: Name::new("contextualArgument_3_0").unwrap(), path: vec![ FetchDataPathElement::Key( Name::new_unchecked("identifiers"), @@ -1473,7 +1473,7 @@ fn set_context_test_efficiently_merge_fetch_groups() { ], })), Arc::new(FetchDataRewrite::KeyRenamer(FetchDataKeyRenamer { - rename_key_to: Name::new("contextualArgument_1_1").unwrap(), + rename_key_to: Name::new("contextualArgument_3_1").unwrap(), path: vec![FetchDataPathElement::Key( Name::new_unchecked("mid"), Default::default() diff --git a/apollo-federation/tests/snapshots/main__extract_subgraphs__can_extract_subgraph.snap b/apollo-federation/tests/snapshots/main__extract_subgraphs__can_extract_subgraph.snap index 50474161ef..94b28cefb7 100644 --- a/apollo-federation/tests/snapshots/main__extract_subgraphs__can_extract_subgraph.snap +++ b/apollo-federation/tests/snapshots/main__extract_subgraphs__can_extract_subgraph.snap @@ -42,9 +42,9 @@ directive @federation__cost(weight: Int!) on ARGUMENT_DEFINITION | ENUM | FIELD_ directive @federation__listSize(assumedSize: Int, slicingArguments: [String!], sizedFields: [String!], requireOneSlicingArgument: Boolean = true) on FIELD_DEFINITION -directive @federation__fromContext(field: String) on ARGUMENT_DEFINITION +directive @federation__fromContext(field: federation__ContextFieldValue) on ARGUMENT_DEFINITION -directive @federation__context(name: String) repeatable on INTERFACE | OBJECT | UNION +directive @federation__context(name: String!) repeatable on INTERFACE | OBJECT | UNION scalar link__Import @@ -59,6 +59,8 @@ enum link__Purpose { EXECUTION } +scalar federation__ContextFieldValue + scalar federation__FieldSet scalar federation__Scope @@ -127,9 +129,9 @@ directive @federation__cost(weight: Int!) on ARGUMENT_DEFINITION | ENUM | FIELD_ directive @federation__listSize(assumedSize: Int, slicingArguments: [String!], sizedFields: [String!], requireOneSlicingArgument: Boolean = true) on FIELD_DEFINITION -directive @federation__fromContext(field: String) on ARGUMENT_DEFINITION +directive @federation__fromContext(field: federation__ContextFieldValue) on ARGUMENT_DEFINITION -directive @federation__context(name: String) repeatable on INTERFACE | OBJECT | UNION +directive @federation__context(name: String!) repeatable on INTERFACE | OBJECT | UNION scalar link__Import @@ -144,6 +146,8 @@ enum link__Purpose { EXECUTION } +scalar federation__ContextFieldValue + scalar federation__FieldSet scalar federation__Scope diff --git a/apollo-federation/tests/snapshots/main__extract_subgraphs__does_not_extract_demand_control_directive_name_conflicts.snap b/apollo-federation/tests/snapshots/main__extract_subgraphs__does_not_extract_demand_control_directive_name_conflicts.snap index a20ce0e66d..29489ed89b 100644 --- a/apollo-federation/tests/snapshots/main__extract_subgraphs__does_not_extract_demand_control_directive_name_conflicts.snap +++ b/apollo-federation/tests/snapshots/main__extract_subgraphs__does_not_extract_demand_control_directive_name_conflicts.snap @@ -42,9 +42,9 @@ directive @federation__cost(weight: Int!) on ARGUMENT_DEFINITION | ENUM | FIELD_ directive @federation__listSize(assumedSize: Int, slicingArguments: [String!], sizedFields: [String!], requireOneSlicingArgument: Boolean = true) on FIELD_DEFINITION -directive @federation__fromContext(field: String) on ARGUMENT_DEFINITION +directive @federation__fromContext(field: federation__ContextFieldValue) on ARGUMENT_DEFINITION -directive @federation__context(name: String) repeatable on INTERFACE | OBJECT | UNION +directive @federation__context(name: String!) repeatable on INTERFACE | OBJECT | UNION scalar link__Import @@ -59,6 +59,8 @@ enum link__Purpose { EXECUTION } +scalar federation__ContextFieldValue + scalar federation__FieldSet scalar federation__Scope @@ -116,9 +118,9 @@ directive @federation__cost(weight: Int!) on ARGUMENT_DEFINITION | ENUM | FIELD_ directive @federation__listSize(assumedSize: Int, slicingArguments: [String!], sizedFields: [String!], requireOneSlicingArgument: Boolean = true) on FIELD_DEFINITION -directive @federation__fromContext(field: String) on ARGUMENT_DEFINITION +directive @federation__fromContext(field: federation__ContextFieldValue) on ARGUMENT_DEFINITION -directive @federation__context(name: String) repeatable on INTERFACE | OBJECT | UNION +directive @federation__context(name: String!) repeatable on INTERFACE | OBJECT | UNION scalar link__Import @@ -133,6 +135,8 @@ enum link__Purpose { EXECUTION } +scalar federation__ContextFieldValue + scalar federation__FieldSet scalar federation__Scope diff --git a/apollo-federation/tests/snapshots/main__extract_subgraphs__does_not_extract_renamed_demand_control_directive_name_conflicts.snap b/apollo-federation/tests/snapshots/main__extract_subgraphs__does_not_extract_renamed_demand_control_directive_name_conflicts.snap index a20ce0e66d..29489ed89b 100644 --- a/apollo-federation/tests/snapshots/main__extract_subgraphs__does_not_extract_renamed_demand_control_directive_name_conflicts.snap +++ b/apollo-federation/tests/snapshots/main__extract_subgraphs__does_not_extract_renamed_demand_control_directive_name_conflicts.snap @@ -42,9 +42,9 @@ directive @federation__cost(weight: Int!) on ARGUMENT_DEFINITION | ENUM | FIELD_ directive @federation__listSize(assumedSize: Int, slicingArguments: [String!], sizedFields: [String!], requireOneSlicingArgument: Boolean = true) on FIELD_DEFINITION -directive @federation__fromContext(field: String) on ARGUMENT_DEFINITION +directive @federation__fromContext(field: federation__ContextFieldValue) on ARGUMENT_DEFINITION -directive @federation__context(name: String) repeatable on INTERFACE | OBJECT | UNION +directive @federation__context(name: String!) repeatable on INTERFACE | OBJECT | UNION scalar link__Import @@ -59,6 +59,8 @@ enum link__Purpose { EXECUTION } +scalar federation__ContextFieldValue + scalar federation__FieldSet scalar federation__Scope @@ -116,9 +118,9 @@ directive @federation__cost(weight: Int!) on ARGUMENT_DEFINITION | ENUM | FIELD_ directive @federation__listSize(assumedSize: Int, slicingArguments: [String!], sizedFields: [String!], requireOneSlicingArgument: Boolean = true) on FIELD_DEFINITION -directive @federation__fromContext(field: String) on ARGUMENT_DEFINITION +directive @federation__fromContext(field: federation__ContextFieldValue) on ARGUMENT_DEFINITION -directive @federation__context(name: String) repeatable on INTERFACE | OBJECT | UNION +directive @federation__context(name: String!) repeatable on INTERFACE | OBJECT | UNION scalar link__Import @@ -133,6 +135,8 @@ enum link__Purpose { EXECUTION } +scalar federation__ContextFieldValue + scalar federation__FieldSet scalar federation__Scope diff --git a/apollo-federation/tests/snapshots/main__extract_subgraphs__extracts_demand_control_directives.snap b/apollo-federation/tests/snapshots/main__extract_subgraphs__extracts_demand_control_directives.snap index c32e8c73a9..328f7ad473 100644 --- a/apollo-federation/tests/snapshots/main__extract_subgraphs__extracts_demand_control_directives.snap +++ b/apollo-federation/tests/snapshots/main__extract_subgraphs__extracts_demand_control_directives.snap @@ -42,9 +42,9 @@ directive @federation__cost(weight: Int!) on ARGUMENT_DEFINITION | ENUM | FIELD_ directive @federation__listSize(assumedSize: Int, slicingArguments: [String!], sizedFields: [String!], requireOneSlicingArgument: Boolean = true) on FIELD_DEFINITION -directive @federation__fromContext(field: String) on ARGUMENT_DEFINITION +directive @federation__fromContext(field: federation__ContextFieldValue) on ARGUMENT_DEFINITION -directive @federation__context(name: String) repeatable on INTERFACE | OBJECT | UNION +directive @federation__context(name: String!) repeatable on INTERFACE | OBJECT | UNION scalar link__Import @@ -59,6 +59,8 @@ enum link__Purpose { EXECUTION } +scalar federation__ContextFieldValue + scalar federation__FieldSet scalar federation__Scope @@ -136,9 +138,9 @@ directive @federation__cost(weight: Int!) on ARGUMENT_DEFINITION | ENUM | FIELD_ directive @federation__listSize(assumedSize: Int, slicingArguments: [String!], sizedFields: [String!], requireOneSlicingArgument: Boolean = true) on FIELD_DEFINITION -directive @federation__fromContext(field: String) on ARGUMENT_DEFINITION +directive @federation__fromContext(field: federation__ContextFieldValue) on ARGUMENT_DEFINITION -directive @federation__context(name: String) repeatable on INTERFACE | OBJECT | UNION +directive @federation__context(name: String!) repeatable on INTERFACE | OBJECT | UNION scalar link__Import @@ -153,6 +155,8 @@ enum link__Purpose { EXECUTION } +scalar federation__ContextFieldValue + scalar federation__FieldSet scalar federation__Scope diff --git a/apollo-federation/tests/snapshots/main__extract_subgraphs__extracts_renamed_demand_control_directives.snap b/apollo-federation/tests/snapshots/main__extract_subgraphs__extracts_renamed_demand_control_directives.snap index c32e8c73a9..328f7ad473 100644 --- a/apollo-federation/tests/snapshots/main__extract_subgraphs__extracts_renamed_demand_control_directives.snap +++ b/apollo-federation/tests/snapshots/main__extract_subgraphs__extracts_renamed_demand_control_directives.snap @@ -42,9 +42,9 @@ directive @federation__cost(weight: Int!) on ARGUMENT_DEFINITION | ENUM | FIELD_ directive @federation__listSize(assumedSize: Int, slicingArguments: [String!], sizedFields: [String!], requireOneSlicingArgument: Boolean = true) on FIELD_DEFINITION -directive @federation__fromContext(field: String) on ARGUMENT_DEFINITION +directive @federation__fromContext(field: federation__ContextFieldValue) on ARGUMENT_DEFINITION -directive @federation__context(name: String) repeatable on INTERFACE | OBJECT | UNION +directive @federation__context(name: String!) repeatable on INTERFACE | OBJECT | UNION scalar link__Import @@ -59,6 +59,8 @@ enum link__Purpose { EXECUTION } +scalar federation__ContextFieldValue + scalar federation__FieldSet scalar federation__Scope @@ -136,9 +138,9 @@ directive @federation__cost(weight: Int!) on ARGUMENT_DEFINITION | ENUM | FIELD_ directive @federation__listSize(assumedSize: Int, slicingArguments: [String!], sizedFields: [String!], requireOneSlicingArgument: Boolean = true) on FIELD_DEFINITION -directive @federation__fromContext(field: String) on ARGUMENT_DEFINITION +directive @federation__fromContext(field: federation__ContextFieldValue) on ARGUMENT_DEFINITION -directive @federation__context(name: String) repeatable on INTERFACE | OBJECT | UNION +directive @federation__context(name: String!) repeatable on INTERFACE | OBJECT | UNION scalar link__Import @@ -153,6 +155,8 @@ enum link__Purpose { EXECUTION } +scalar federation__ContextFieldValue + scalar federation__FieldSet scalar federation__Scope diff --git a/apollo-federation/tests/snapshots/main__extract_subgraphs__extracts_set_context_directives.snap b/apollo-federation/tests/snapshots/main__extract_subgraphs__extracts_set_context_directives.snap index fd7c25bff1..01c3de21d0 100644 --- a/apollo-federation/tests/snapshots/main__extract_subgraphs__extracts_set_context_directives.snap +++ b/apollo-federation/tests/snapshots/main__extract_subgraphs__extracts_set_context_directives.snap @@ -42,9 +42,9 @@ directive @federation__cost(weight: Int!) on ARGUMENT_DEFINITION | ENUM | FIELD_ directive @federation__listSize(assumedSize: Int, slicingArguments: [String!], sizedFields: [String!], requireOneSlicingArgument: Boolean = true) on FIELD_DEFINITION -directive @federation__fromContext(field: String) on ARGUMENT_DEFINITION +directive @federation__fromContext(field: federation__ContextFieldValue) on ARGUMENT_DEFINITION -directive @federation__context(name: String) repeatable on INTERFACE | OBJECT | UNION +directive @federation__context(name: String!) repeatable on INTERFACE | OBJECT | UNION scalar link__Import @@ -59,6 +59,8 @@ enum link__Purpose { EXECUTION } +scalar federation__ContextFieldValue + scalar federation__FieldSet scalar federation__Scope @@ -130,9 +132,9 @@ directive @federation__cost(weight: Int!) on ARGUMENT_DEFINITION | ENUM | FIELD_ directive @federation__listSize(assumedSize: Int, slicingArguments: [String!], sizedFields: [String!], requireOneSlicingArgument: Boolean = true) on FIELD_DEFINITION -directive @federation__fromContext(field: String) on ARGUMENT_DEFINITION +directive @federation__fromContext(field: federation__ContextFieldValue) on ARGUMENT_DEFINITION -directive @federation__context(name: String) repeatable on INTERFACE | OBJECT | UNION +directive @federation__context(name: String!) repeatable on INTERFACE | OBJECT | UNION scalar link__Import @@ -147,6 +149,8 @@ enum link__Purpose { EXECUTION } +scalar federation__ContextFieldValue + scalar federation__FieldSet scalar federation__Scope diff --git a/apollo-router-benchmarks/Cargo.toml b/apollo-router-benchmarks/Cargo.toml index 359391b4a7..0a0f627142 100644 --- a/apollo-router-benchmarks/Cargo.toml +++ b/apollo-router-benchmarks/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "apollo-router-benchmarks" -version = "1.58.0" +version = "1.58.1" authors = ["Apollo Graph, Inc. "] edition = "2021" license = "Elastic-2.0" diff --git a/apollo-router-scaffold/Cargo.toml b/apollo-router-scaffold/Cargo.toml index 8b68e56a12..a4d9414020 100644 --- a/apollo-router-scaffold/Cargo.toml +++ b/apollo-router-scaffold/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "apollo-router-scaffold" -version = "1.58.0" +version = "1.58.1" authors = ["Apollo Graph, Inc. "] edition = "2021" license = "Elastic-2.0" diff --git a/apollo-router-scaffold/templates/base/Cargo.template.toml b/apollo-router-scaffold/templates/base/Cargo.template.toml index 00b111b264..3c6cbb58e9 100644 --- a/apollo-router-scaffold/templates/base/Cargo.template.toml +++ b/apollo-router-scaffold/templates/base/Cargo.template.toml @@ -22,7 +22,7 @@ apollo-router = { path ="{{integration_test}}apollo-router" } apollo-router = { git="https://github.com/apollographql/router.git", branch="{{branch}}" } {{else}} # Note if you update these dependencies then also update xtask/Cargo.toml -apollo-router = "1.58.0" +apollo-router = "1.58.1" {{/if}} {{/if}} async-trait = "0.1.52" diff --git a/apollo-router-scaffold/templates/base/xtask/Cargo.template.toml b/apollo-router-scaffold/templates/base/xtask/Cargo.template.toml index db2a939c2a..2d695c9daf 100644 --- a/apollo-router-scaffold/templates/base/xtask/Cargo.template.toml +++ b/apollo-router-scaffold/templates/base/xtask/Cargo.template.toml @@ -13,7 +13,7 @@ apollo-router-scaffold = { path ="{{integration_test}}apollo-router-scaffold" } {{#if branch}} apollo-router-scaffold = { git="https://github.com/apollographql/router.git", branch="{{branch}}" } {{else}} -apollo-router-scaffold = { git = "https://github.com/apollographql/router.git", tag = "v1.58.0" } +apollo-router-scaffold = { git = "https://github.com/apollographql/router.git", tag = "v1.58.1" } {{/if}} {{/if}} anyhow = "1.0.58" diff --git a/apollo-router/Cargo.toml b/apollo-router/Cargo.toml index f2cedca917..2cdb3fa8f6 100644 --- a/apollo-router/Cargo.toml +++ b/apollo-router/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "apollo-router" -version = "1.58.0" +version = "1.58.1" authors = ["Apollo Graph, Inc. "] repository = "https://github.com/apollographql/router/" documentation = "https://docs.rs/apollo-router" @@ -66,7 +66,7 @@ features = ["docs_rs"] access-json = "0.1.0" anyhow = "1.0.86" apollo-compiler.workspace = true -apollo-federation = { path = "../apollo-federation", version = "=1.58.0" } +apollo-federation = { path = "../apollo-federation", version = "=1.58.1" } arc-swap = "1.6.0" async-channel = "1.9.0" async-compression = { version = "0.4.6", features = [ diff --git a/apollo-router/src/configuration/mod.rs b/apollo-router/src/configuration/mod.rs index 2df5ad8c62..50371fd5dd 100644 --- a/apollo-router/src/configuration/mod.rs +++ b/apollo-router/src/configuration/mod.rs @@ -5,6 +5,7 @@ use std::io::BufReader; use std::iter; use std::net::IpAddr; use std::net::SocketAddr; +use std::num::NonZeroU32; use std::num::NonZeroUsize; use std::str::FromStr; use std::sync::Arc; @@ -416,16 +417,31 @@ impl Configuration { pub(crate) fn rust_query_planner_config( &self, ) -> apollo_federation::query_plan::query_planner::QueryPlannerConfig { - apollo_federation::query_plan::query_planner::QueryPlannerConfig { + use apollo_federation::query_plan::query_planner::QueryPlanIncrementalDeliveryConfig; + use apollo_federation::query_plan::query_planner::QueryPlannerConfig; + use apollo_federation::query_plan::query_planner::QueryPlannerDebugConfig; + + let max_evaluated_plans = self + .supergraph + .query_planning + .experimental_plans_limit + // Fails if experimental_plans_limit is zero; use our default. + .and_then(NonZeroU32::new) + .unwrap_or(NonZeroU32::new(10_000).expect("it is not zero")); + + QueryPlannerConfig { reuse_query_fragments: self.supergraph.reuse_query_fragments.unwrap_or(true), subgraph_graphql_validation: false, generate_query_fragments: self.supergraph.generate_query_fragments, - incremental_delivery: - apollo_federation::query_plan::query_planner::QueryPlanIncrementalDeliveryConfig { - enable_defer: self.supergraph.defer_support, - }, + incremental_delivery: QueryPlanIncrementalDeliveryConfig { + enable_defer: self.supergraph.defer_support, + }, type_conditioned_fetching: self.experimental_type_conditioned_fetching, - debug: Default::default(), + debug: QueryPlannerDebugConfig { + bypass_planner_for_single_subgraph: false, + max_evaluated_plans, + paths_limit: self.supergraph.query_planning.experimental_paths_limit, + }, } } } diff --git a/apollo-router/src/plugins/telemetry/config_new/instruments.rs b/apollo-router/src/plugins/telemetry/config_new/instruments.rs index 341f84ad35..88760f1f1c 100644 --- a/apollo-router/src/plugins/telemetry/config_new/instruments.rs +++ b/apollo-router/src/plugins/telemetry/config_new/instruments.rs @@ -1446,7 +1446,7 @@ where }) } None => { - ::tracing::error!("cannot convert static instrument into a counter, this is an error; please fill an issue on GitHub"); + failfast_debug!("cannot convert static instrument into a counter, this is an error; please fill an issue on GitHub"); } } } @@ -1501,7 +1501,7 @@ where }); } None => { - ::tracing::error!("cannot convert static instrument into a histogram, this is an error; please fill an issue on GitHub"); + failfast_debug!("cannot convert static instrument into a histogram, this is an error; please fill an issue on GitHub"); } } } diff --git a/apollo-router/src/plugins/telemetry/config_new/selectors.rs b/apollo-router/src/plugins/telemetry/config_new/selectors.rs index 1b49d9548c..3324047b2f 100644 --- a/apollo-router/src/plugins/telemetry/config_new/selectors.rs +++ b/apollo-router/src/plugins/telemetry/config_new/selectors.rs @@ -1069,25 +1069,6 @@ impl Selector for SupergraphSelector { ctx: &Context, ) -> Option { match self { - SupergraphSelector::Query { query, .. } => { - let limits_opt = ctx - .extensions() - .with_lock(|lock| lock.get::>().cloned()); - match query { - Query::Aliases => { - limits_opt.map(|limits| opentelemetry::Value::I64(limits.aliases as i64)) - } - Query::Depth => { - limits_opt.map(|limits| opentelemetry::Value::I64(limits.depth as i64)) - } - Query::Height => { - limits_opt.map(|limits| opentelemetry::Value::I64(limits.height as i64)) - } - Query::RootFields => limits_opt - .map(|limits| opentelemetry::Value::I64(limits.root_fields as i64)), - Query::String => None, - } - } SupergraphSelector::ResponseData { response_data, default, @@ -3289,12 +3270,6 @@ mod test { .unwrap(), 4.into() ); - assert_eq!( - selector - .on_response_event(&crate::graphql::Response::builder().build(), &context) - .unwrap(), - 4.into() - ); } #[test] diff --git a/apollo-router/src/query_planner/bridge_query_planner.rs b/apollo-router/src/query_planner/bridge_query_planner.rs index cc058b5555..dac65efe45 100644 --- a/apollo-router/src/query_planner/bridge_query_planner.rs +++ b/apollo-router/src/query_planner/bridge_query_planner.rs @@ -166,20 +166,7 @@ impl PlannerMode { schema: &Schema, configuration: &Configuration, ) -> Result, ServiceBuildError> { - let config = apollo_federation::query_plan::query_planner::QueryPlannerConfig { - reuse_query_fragments: configuration - .supergraph - .reuse_query_fragments - .unwrap_or(true), - subgraph_graphql_validation: false, - generate_query_fragments: configuration.supergraph.generate_query_fragments, - incremental_delivery: - apollo_federation::query_plan::query_planner::QueryPlanIncrementalDeliveryConfig { - enable_defer: configuration.supergraph.defer_support, - }, - type_conditioned_fetching: configuration.experimental_type_conditioned_fetching, - debug: Default::default(), - }; + let config = configuration.rust_query_planner_config(); let result = QueryPlanner::new(schema.federation_supergraph(), config); match &result { diff --git a/apollo-router/src/query_planner/dual_query_planner.rs b/apollo-router/src/query_planner/dual_query_planner.rs index 04d393c2cf..9e5250a447 100644 --- a/apollo-router/src/query_planner/dual_query_planner.rs +++ b/apollo-router/src/query_planner/dual_query_planner.rs @@ -14,6 +14,7 @@ use apollo_compiler::ast; use apollo_compiler::validation::Valid; use apollo_compiler::ExecutableDocument; use apollo_compiler::Name; +use apollo_compiler::Node; use apollo_federation::query_plan::query_planner::QueryPlanOptions; use apollo_federation::query_plan::query_planner::QueryPlanner; use apollo_federation::query_plan::QueryPlan; @@ -1003,6 +1004,24 @@ fn same_ast_argument(x: &ast::Argument, y: &ast::Argument) -> bool { x.name == y.name && same_ast_argument_value(&x.value, &y.value) } +fn same_ast_arguments(x: &[Node], y: &[Node]) -> bool { + vec_matches_sorted_by( + x, + y, + |a, b| a.name.cmp(&b.name), + |a, b| same_ast_argument(a, b), + ) +} + +fn same_directives(x: &ast::DirectiveList, y: &ast::DirectiveList) -> bool { + vec_matches_sorted_by( + x, + y, + |a, b| a.name.cmp(&b.name), + |a, b| a.name == b.name && same_ast_arguments(&a.arguments, &b.arguments), + ) +} + fn get_ast_selection_key( selection: &ast::Selection, fragment_map: &HashMap, @@ -1035,24 +1054,20 @@ fn same_ast_selection( (ast::Selection::Field(x), ast::Selection::Field(y)) => { x.name == y.name && x.alias == y.alias - && vec_matches_sorted_by( - &x.arguments, - &y.arguments, - |a, b| a.name.cmp(&b.name), - |a, b| same_ast_argument(a, b), - ) - && x.directives == y.directives + && same_ast_arguments(&x.arguments, &y.arguments) + && same_directives(&x.directives, &y.directives) && same_ast_selection_set_sorted(&x.selection_set, &y.selection_set, fragment_map) } (ast::Selection::FragmentSpread(x), ast::Selection::FragmentSpread(y)) => { let mapped_fragment_name = fragment_map .get(&x.fragment_name) .unwrap_or(&x.fragment_name); - *mapped_fragment_name == y.fragment_name && x.directives == y.directives + *mapped_fragment_name == y.fragment_name + && same_directives(&x.directives, &y.directives) } (ast::Selection::InlineFragment(x), ast::Selection::InlineFragment(y)) => { x.type_condition == y.type_condition - && x.directives == y.directives + && same_directives(&x.directives, &y.directives) && same_ast_selection_set_sorted(&x.selection_set, &y.selection_set, fragment_map) } _ => false, @@ -1200,6 +1215,15 @@ mod ast_comparison_tests { assert!(super::same_ast_document(&ast_x, &ast_y).is_ok()); } + #[test] + fn test_selection_directive_order() { + let op_x = r#"{ x @include(if:true) @skip(if:false) }"#; + let op_y = r#"{ x @skip(if:false) @include(if:true) }"#; + let ast_x = ast::Document::parse(op_x, "op_x").unwrap(); + let ast_y = ast::Document::parse(op_y, "op_y").unwrap(); + assert!(super::same_ast_document(&ast_x, &ast_y).is_ok()); + } + #[test] fn test_string_to_id_coercion_difference() { // JS QP coerces strings into integer for ID type, while Rust QP doesn't. diff --git a/apollo-router/tests/integration/fixtures/query_planner_max_evaluated_plans.graphql b/apollo-router/tests/integration/fixtures/query_planner_max_evaluated_plans.graphql new file mode 100644 index 0000000000..a1f388ed61 --- /dev/null +++ b/apollo-router/tests/integration/fixtures/query_planner_max_evaluated_plans.graphql @@ -0,0 +1,73 @@ +# Composed from subgraphs with hash: a9236eee956ed7fc219b2212696478159ced7eea +schema + @link(url: "https://specs.apollo.dev/link/v1.0") + @link(url: "https://specs.apollo.dev/join/v0.5", for: EXECUTION) +{ + query: Query +} + +directive @join__directive(graphs: [join__Graph!], name: String!, args: join__DirectiveArguments) repeatable on SCHEMA | OBJECT | INTERFACE | FIELD_DEFINITION + +directive @join__enumValue(graph: join__Graph!) repeatable on ENUM_VALUE + +directive @join__field(graph: join__Graph, requires: join__FieldSet, provides: join__FieldSet, type: String, external: Boolean, override: String, usedOverridden: Boolean, overrideLabel: String, contextArguments: [join__ContextArgument!]) repeatable on FIELD_DEFINITION | INPUT_FIELD_DEFINITION + +directive @join__graph(name: String!, url: String!) on ENUM_VALUE + +directive @join__implements(graph: join__Graph!, interface: String!) repeatable on OBJECT | INTERFACE + +directive @join__type(graph: join__Graph!, key: join__FieldSet, extension: Boolean! = false, resolvable: Boolean! = true, isInterfaceObject: Boolean! = false) repeatable on OBJECT | INTERFACE | UNION | ENUM | INPUT_OBJECT | SCALAR + +directive @join__unionMember(graph: join__Graph!, member: String!) repeatable on UNION + +directive @link(url: String, as: String, for: link__Purpose, import: [link__Import]) repeatable on SCHEMA + +input join__ContextArgument { + name: String! + type: String! + context: String! + selection: join__FieldValue! +} + +scalar join__DirectiveArguments + +scalar join__FieldSet + +scalar join__FieldValue + +enum join__Graph { + SUBGRAPH1 @join__graph(name: "Subgraph1", url: "none") + SUBGRAPH2 @join__graph(name: "Subgraph2", url: "none") +} + +scalar link__Import + +enum link__Purpose { + """ + `SECURITY` features provide metadata necessary to securely resolve fields. + """ + SECURITY + + """ + `EXECUTION` features provide metadata necessary for operation execution. + """ + EXECUTION +} + +type Query + @join__type(graph: SUBGRAPH1) + @join__type(graph: SUBGRAPH2) +{ + t: T +} + +type T + @join__type(graph: SUBGRAPH1, key: "id") + @join__type(graph: SUBGRAPH2, key: "id") +{ + id: ID! + v1: Int + v2: Int + v3: Int + v4: Int +} diff --git a/apollo-router/tests/integration/query_planner.rs b/apollo-router/tests/integration/query_planner.rs index 4b1c1ab70e..5bf0ca799c 100644 --- a/apollo-router/tests/integration/query_planner.rs +++ b/apollo-router/tests/integration/query_planner.rs @@ -3,6 +3,8 @@ use std::path::PathBuf; use crate::integration::common::graph_os_enabled; use crate::integration::IntegrationTest; +mod max_evaluated_plans; + const PROMETHEUS_METRICS_CONFIG: &str = include_str!("telemetry/fixtures/prometheus.router.yaml"); const LEGACY_QP: &str = "experimental_query_planner_mode: legacy"; const NEW_QP: &str = "experimental_query_planner_mode: new"; diff --git a/apollo-router/tests/integration/query_planner/max_evaluated_plans.rs b/apollo-router/tests/integration/query_planner/max_evaluated_plans.rs new file mode 100644 index 0000000000..d6474aa30b --- /dev/null +++ b/apollo-router/tests/integration/query_planner/max_evaluated_plans.rs @@ -0,0 +1,130 @@ +use serde_json::json; + +use crate::integration::IntegrationTest; + +fn assert_evaluated_plans(prom: &str, expected: u64) { + let line = prom + .lines() + .find(|line| line.starts_with("apollo_router_query_planning_plan_evaluated_plans_sum")) + .expect("apollo.router.query_planning.plan.evaluated_plans metric is missing"); + let (_, num) = line + .split_once(' ') + .expect("apollo.router.query_planning.plan.evaluated_plans metric has unexpected shape"); + assert_eq!(num, format!("{expected}")); +} + +#[tokio::test(flavor = "multi_thread")] +async fn reports_evaluated_plans() { + let mut router = IntegrationTest::builder() + .config( + r#" + telemetry: + exporters: + metrics: + prometheus: + enabled: true + "#, + ) + .supergraph("tests/integration/fixtures/query_planner_max_evaluated_plans.graphql") + .build() + .await; + router.start().await; + router.assert_started().await; + router + .execute_query(&json!({ + "query": r#"{ t { v1 v2 v3 v4 } }"#, + "variables": {}, + })) + .await; + + let metrics = router + .get_metrics_response() + .await + .expect("failed to fetch metrics") + .text() + .await + .expect("metrics are not text?!"); + assert_evaluated_plans(&metrics, 16); + + router.graceful_shutdown().await; +} + +#[tokio::test(flavor = "multi_thread")] +async fn does_not_exceed_max_evaluated_plans_legacy() { + let mut router = IntegrationTest::builder() + .config( + r#" + experimental_query_planner_mode: legacy + telemetry: + exporters: + metrics: + prometheus: + enabled: true + supergraph: + query_planning: + experimental_plans_limit: 4 + "#, + ) + .supergraph("tests/integration/fixtures/query_planner_max_evaluated_plans.graphql") + .build() + .await; + router.start().await; + router.assert_started().await; + router + .execute_query(&json!({ + "query": r#"{ t { v1 v2 v3 v4 } }"#, + "variables": {}, + })) + .await; + + let metrics = router + .get_metrics_response() + .await + .expect("failed to fetch metrics") + .text() + .await + .expect("metrics are not text?!"); + assert_evaluated_plans(&metrics, 4); + + router.graceful_shutdown().await; +} + +#[tokio::test(flavor = "multi_thread")] +async fn does_not_exceed_max_evaluated_plans() { + let mut router = IntegrationTest::builder() + .config( + r#" + experimental_query_planner_mode: new + telemetry: + exporters: + metrics: + prometheus: + enabled: true + supergraph: + query_planning: + experimental_plans_limit: 4 + "#, + ) + .supergraph("tests/integration/fixtures/query_planner_max_evaluated_plans.graphql") + .build() + .await; + router.start().await; + router.assert_started().await; + router + .execute_query(&json!({ + "query": r#"{ t { v1 v2 v3 v4 } }"#, + "variables": {}, + })) + .await; + + let metrics = router + .get_metrics_response() + .await + .expect("failed to fetch metrics") + .text() + .await + .expect("metrics are not text?!"); + assert_evaluated_plans(&metrics, 4); + + router.graceful_shutdown().await; +} diff --git a/apollo-router/tests/integration/telemetry/fixtures/graphql.router.yaml b/apollo-router/tests/integration/telemetry/fixtures/graphql.router.yaml index 24002e9be0..e1ff03b1c7 100644 --- a/apollo-router/tests/integration/telemetry/fixtures/graphql.router.yaml +++ b/apollo-router/tests/integration/telemetry/fixtures/graphql.router.yaml @@ -9,6 +9,31 @@ telemetry: instrumentation: instruments: + supergraph: + oplimits.aliases: + value: + query: aliases + type: histogram + unit: number + description: "Aliases for an operation" + oplimits.depth: + value: + query: depth + type: histogram + unit: number + description: "Depth for an operation" + oplimits.height: + value: + query: height + type: histogram + unit: number + description: "Height for an operation" + oplimits.root_fields: + value: + query: root_fields + type: histogram + unit: number + description: "Root fields for an operation" graphql: field.execution: true list.length: true @@ -38,7 +63,4 @@ telemetry: condition: eq: - field_name: string - - "topProducts" - - - + - "topProducts" \ No newline at end of file diff --git a/apollo-router/tests/integration/telemetry/metrics.rs b/apollo-router/tests/integration/telemetry/metrics.rs index 1ae93f2d4f..cbce509c13 100644 --- a/apollo-router/tests/integration/telemetry/metrics.rs +++ b/apollo-router/tests/integration/telemetry/metrics.rs @@ -201,6 +201,27 @@ async fn test_graphql_metrics() { router.start().await; router.assert_started().await; router.execute_default_query().await; + router + .assert_log_not_contains("this is a bug and should not happen") + .await; + router + .assert_metrics_contains( + r#"oplimits_aliases_sum{otel_scope_name="apollo/router"} 0"#, + None, + ) + .await; + router + .assert_metrics_contains( + r#"oplimits_root_fields_sum{otel_scope_name="apollo/router"} 1"#, + None, + ) + .await; + router + .assert_metrics_contains( + r#"oplimits_depth_sum{otel_scope_name="apollo/router"} 2"#, + None, + ) + .await; router .assert_metrics_contains(r#"graphql_field_list_length_sum{graphql_field_name="topProducts",graphql_field_type="Product",graphql_type_name="Query",otel_scope_name="apollo/router"} 3"#, None) .await; diff --git a/apollo-router/tests/snapshots/set_context__set_context_union_rust_qp.snap b/apollo-router/tests/snapshots/set_context__set_context_union_rust_qp.snap index 74e41f53c9..650849369c 100644 --- a/apollo-router/tests/snapshots/set_context__set_context_union_rust_qp.snap +++ b/apollo-router/tests/snapshots/set_context__set_context_union_rust_qp.snap @@ -47,15 +47,6 @@ expression: response "scopes": [] }, "contextRewrites": [ - { - "kind": "KeyRenamer", - "path": [ - "..", - "... on A", - "prop" - ], - "renameKeyTo": "contextualArgument_1_1" - }, { "kind": "KeyRenamer", "path": [ @@ -117,15 +108,6 @@ expression: response "prop" ], "renameKeyTo": "contextualArgument_1_1" - }, - { - "kind": "KeyRenamer", - "path": [ - "..", - "... on B", - "prop" - ], - "renameKeyTo": "contextualArgument_1_1" } ], "id": null, diff --git a/dockerfiles/tracing/docker-compose.datadog.yml b/dockerfiles/tracing/docker-compose.datadog.yml index e69604eb8c..adb40c2e47 100644 --- a/dockerfiles/tracing/docker-compose.datadog.yml +++ b/dockerfiles/tracing/docker-compose.datadog.yml @@ -3,7 +3,7 @@ services: apollo-router: container_name: apollo-router - image: ghcr.io/apollographql/router:v1.58.0 + image: ghcr.io/apollographql/router:v1.58.1 volumes: - ./supergraph.graphql:/etc/config/supergraph.graphql - ./router/datadog.router.yaml:/etc/config/configuration.yaml diff --git a/dockerfiles/tracing/docker-compose.jaeger.yml b/dockerfiles/tracing/docker-compose.jaeger.yml index 8225d7799a..9ea992be09 100644 --- a/dockerfiles/tracing/docker-compose.jaeger.yml +++ b/dockerfiles/tracing/docker-compose.jaeger.yml @@ -4,7 +4,7 @@ services: apollo-router: container_name: apollo-router #build: ./router - image: ghcr.io/apollographql/router:v1.58.0 + image: ghcr.io/apollographql/router:v1.58.1 volumes: - ./supergraph.graphql:/etc/config/supergraph.graphql - ./router/jaeger.router.yaml:/etc/config/configuration.yaml diff --git a/dockerfiles/tracing/docker-compose.zipkin.yml b/dockerfiles/tracing/docker-compose.zipkin.yml index a84a7f1fcf..fd60819066 100644 --- a/dockerfiles/tracing/docker-compose.zipkin.yml +++ b/dockerfiles/tracing/docker-compose.zipkin.yml @@ -4,7 +4,7 @@ services: apollo-router: container_name: apollo-router build: ./router - image: ghcr.io/apollographql/router:v1.58.0 + image: ghcr.io/apollographql/router:v1.58.1 volumes: - ./supergraph.graphql:/etc/config/supergraph.graphql - ./router/zipkin.router.yaml:/etc/config/configuration.yaml diff --git a/helm/chart/router/Chart.yaml b/helm/chart/router/Chart.yaml index 51cf64fa63..cbdd08e61b 100644 --- a/helm/chart/router/Chart.yaml +++ b/helm/chart/router/Chart.yaml @@ -20,10 +20,10 @@ type: application # so it matches the shape of our release process and release automation. # By proxy of that decision, this version uses SemVer 2.0.0, though the prefix # of "v" is not included. -version: 1.58.0 +version: 1.58.1 # This is the version number of the application being deployed. This version number should be # incremented each time you make changes to the application. Versions are not expected to # follow Semantic Versioning. They should reflect the version the application is using. # It is recommended to use it with quotes. -appVersion: "v1.58.0" +appVersion: "v1.58.1" diff --git a/helm/chart/router/README.md b/helm/chart/router/README.md index c68ab5cd8b..f26ac4d052 100644 --- a/helm/chart/router/README.md +++ b/helm/chart/router/README.md @@ -2,7 +2,7 @@ [router](https://github.com/apollographql/router) Rust Graph Routing runtime for Apollo Federation -![Version: 1.58.0](https://img.shields.io/badge/Version-1.58.0-informational?style=flat-square) ![Type: application](https://img.shields.io/badge/Type-application-informational?style=flat-square) ![AppVersion: v1.58.0](https://img.shields.io/badge/AppVersion-v1.58.0-informational?style=flat-square) +![Version: 1.58.1](https://img.shields.io/badge/Version-1.58.1-informational?style=flat-square) ![Type: application](https://img.shields.io/badge/Type-application-informational?style=flat-square) ![AppVersion: v1.58.1](https://img.shields.io/badge/AppVersion-v1.58.1-informational?style=flat-square) ## Prerequisites @@ -11,7 +11,7 @@ ## Get Repo Info ```console -helm pull oci://ghcr.io/apollographql/helm-charts/router --version 1.58.0 +helm pull oci://ghcr.io/apollographql/helm-charts/router --version 1.58.1 ``` ## Install Chart @@ -19,7 +19,7 @@ helm pull oci://ghcr.io/apollographql/helm-charts/router --version 1.58.0 **Important:** only helm3 is supported ```console -helm upgrade --install [RELEASE_NAME] oci://ghcr.io/apollographql/helm-charts/router --version 1.58.0 --values my-values.yaml +helm upgrade --install [RELEASE_NAME] oci://ghcr.io/apollographql/helm-charts/router --version 1.58.1 --values my-values.yaml ``` _See [configuration](#configuration) below._ diff --git a/licenses.html b/licenses.html index 37ceccad5d..ee38302396 100644 --- a/licenses.html +++ b/licenses.html @@ -44,7 +44,7 @@

Third Party Licenses

Overview of licenses: