From 852f129eeab43b593902647b1ab04751d3cf0333 Mon Sep 17 00:00:00 2001 From: Aaron Arinder Date: Tue, 10 Mar 2026 11:52:24 -0400 Subject: [PATCH] feat(orphan-hoist-config): adds config for hoisting orphaned errors --- .changesets/exp_hoist_orphan_errors.md | 36 +++++ apollo-router/src/configuration/mod.rs | 26 ++++ ...nfiguration__tests__schema_generation.snap | 51 +++++++ apollo-router/src/configuration/tests.rs | 103 ++++++++++++++ apollo-router/src/executable.rs | 2 + apollo-router/src/query_planner/fetch.rs | 133 +++++++++++------- apollo-router/src/query_planner/tests.rs | 8 ++ apollo-router/src/services/fetch_service.rs | 22 ++- .../src/services/supergraph/service.rs | 1 + ...eferred_fragment_bounds_nullability-2.snap | 9 +- ...r_paths__multi_level_response_failure.snap | 51 ++++++- ...or_paths__nested_response_failure_404.snap | 84 ++++++++++- ...hs__nested_response_failure_malformed.snap | 36 ++++- ...ond_level_response_failure_empty_path.snap | 13 +- ...cond_level_response_failure_malformed.snap | 15 +- ...ror_selector__nested_response_failure.snap | 13 +- ...are_accepted@federated_ships_required.snap | 31 +++- 17 files changed, 567 insertions(+), 67 deletions(-) create mode 100644 .changesets/exp_hoist_orphan_errors.md diff --git a/.changesets/exp_hoist_orphan_errors.md b/.changesets/exp_hoist_orphan_errors.md new file mode 100644 index 0000000000..47fcd4c65e --- /dev/null +++ b/.changesets/exp_hoist_orphan_errors.md @@ -0,0 +1,36 @@ +### Add `experimental_hoist_orphan_errors` configuration for controlling orphan error path assignment + +Adds a new `experimental_hoist_orphan_errors` configuration that controls how entity-less ("orphan") errors from subgraphs are assigned paths in the response. When enabled for a subgraph, orphan errors are assigned to the nearest non-array ancestor in the response path, preventing them from being duplicated across every element in an array. This can be enabled globally via `all` or per-subgraph via the `subgraphs` map. Per-subgraph settings override `all`. + +Here's an example when targeting a specific subgraph, `my_subgraph`: + +```yaml +experimental_hoist_orphan_errors: + subgraphs: + my_subgraph: + enabled: true +``` + +An example when targeting all subgraphs: + +```yaml +experimental_hoist_orphan_errors: + all: + enabled: true +``` + +And an example enabling for all subgraphs except one: + +```yaml +experimental_hoist_orphan_errors: + all: + enabled: true + subgraphs: + noisy_one: + enabled: false +``` + +Using this feature should only happen if you know you have subgraphs that don't respond with the correct paths when making entity calls. If you're unsure, you probably don't need this! + + +By [@aaronArinder](https://github.com/aaronArinder) in https://github.com/apollographql/router/pull/8998 diff --git a/apollo-router/src/configuration/mod.rs b/apollo-router/src/configuration/mod.rs index 675876f3d5..e15d75bdfe 100644 --- a/apollo-router/src/configuration/mod.rs +++ b/apollo-router/src/configuration/mod.rs @@ -223,6 +223,13 @@ pub struct Configuration { /// Type conditioned fetching configuration. #[serde(default)] pub(crate) experimental_type_conditioned_fetching: bool, + + /// When enabled for specific subgraphs, orphan errors (those without a valid + /// `_entities` path) are assigned to the nearest non-array ancestor in the + /// response path, preventing them from being duplicated across every array + /// element. + #[serde(default)] + pub(crate) experimental_hoist_orphan_errors: SubgraphConfiguration, } impl PartialEq for Configuration { @@ -257,6 +264,7 @@ impl<'de> serde::Deserialize<'de> for Configuration { experimental_chaos: chaos::Config, batching: Batching, experimental_type_conditioned_fetching: bool, + experimental_hoist_orphan_errors: SubgraphConfiguration, } let mut ad_hoc: AdHocConfiguration = serde::Deserialize::deserialize(deserializer)?; @@ -288,6 +296,7 @@ impl<'de> serde::Deserialize<'de> for Configuration { limits: ad_hoc.limits, experimental_chaos: ad_hoc.experimental_chaos, experimental_type_conditioned_fetching: ad_hoc.experimental_type_conditioned_fetching, + experimental_hoist_orphan_errors: ad_hoc.experimental_hoist_orphan_errors, plugins: ad_hoc.plugins, apollo_plugins: ad_hoc.apollo_plugins, batching: ad_hoc.batching, @@ -328,6 +337,7 @@ impl Configuration { chaos: Option, uplink: Option, experimental_type_conditioned_fetching: Option, + experimental_hoist_orphan_errors: Option>, batching: Option, server: Option, ) -> Result { @@ -357,6 +367,7 @@ impl Configuration { batching: batching.unwrap_or_default(), experimental_type_conditioned_fetching: experimental_type_conditioned_fetching .unwrap_or_default(), + experimental_hoist_orphan_errors: experimental_hoist_orphan_errors.unwrap_or_default(), notify, }; @@ -501,6 +512,7 @@ impl Configuration { uplink, experimental_type_conditioned_fetching: experimental_type_conditioned_fetching .unwrap_or_default(), + experimental_hoist_orphan_errors: Default::default(), batching: batching.unwrap_or_default(), raw_yaml: None, }; @@ -1599,3 +1611,17 @@ impl Batching { } } } + +/// Per-subgraph configuration for hoisting orphan errors. +/// +/// "Orphan errors" are errors from entity fetches that lack a valid `_entities` path. +/// When hoisting is enabled, these errors are assigned to the nearest non-array +/// ancestor in the response path, preventing them from being duplicated across +/// every element in an array. +#[derive(Debug, Clone, Default, Deserialize, Serialize, JsonSchema)] +#[serde(deny_unknown_fields)] +pub(crate) struct HoistOrphanErrors { + /// Enable hoisting of orphan errors for this subgraph. + #[serde(default)] + pub(crate) enabled: bool, +} diff --git a/apollo-router/src/configuration/snapshots/apollo_router__configuration__tests__schema_generation.snap b/apollo-router/src/configuration/snapshots/apollo_router__configuration__tests__schema_generation.snap index 09ccbf5957..b4dea9c73c 100644 --- a/apollo-router/src/configuration/snapshots/apollo_router__configuration__tests__schema_generation.snap +++ b/apollo-router/src/configuration/snapshots/apollo_router__configuration__tests__schema_generation.snap @@ -5667,6 +5667,18 @@ expression: "&schema" } ] }, + "HoistOrphanErrors": { + "additionalProperties": false, + "description": "Per-subgraph configuration for hoisting orphan errors.\n\n\"Orphan errors\" are errors from entity fetches that lack a valid `_entities` path.\nWhen hoisting is enabled, these errors are assigned to the nearest non-array\nancestor in the response path, preventing them from being duplicated across\nevery element in an array.", + "properties": { + "enabled": { + "default": false, + "description": "Enable hoisting of orphan errors for this subgraph.", + "type": "boolean" + } + }, + "type": "object" + }, "Homepage": { "additionalProperties": false, "description": "Configuration options pertaining to the home page.", @@ -9698,6 +9710,31 @@ expression: "&schema" }, "type": "object" }, + "SubgraphHoistOrphanErrorsConfiguration": { + "description": "Configuration options pertaining to the subgraph server component.", + "properties": { + "all": { + "allOf": [ + { + "$ref": "#/definitions/HoistOrphanErrors" + } + ], + "default": { + "enabled": false + }, + "description": "options applying to all subgraphs" + }, + "subgraphs": { + "additionalProperties": { + "$ref": "#/definitions/HoistOrphanErrors" + }, + "default": {}, + "description": "per subgraph options", + "type": "object" + } + }, + "type": "object" + }, "SubgraphInvalidationConfig": { "additionalProperties": false, "properties": { @@ -12350,6 +12387,20 @@ expression: "&schema" "experimental_diagnostics": { "$ref": "#/definitions/Config5" }, + "experimental_hoist_orphan_errors": { + "allOf": [ + { + "$ref": "#/definitions/SubgraphHoistOrphanErrorsConfiguration" + } + ], + "default": { + "all": { + "enabled": false + }, + "subgraphs": {} + }, + "description": "When enabled for specific subgraphs, orphan errors (those without a valid\n`_entities` path) are assigned to the nearest non-array ancestor in the\nresponse path, preventing them from being duplicated across every array\nelement." + }, "experimental_type_conditioned_fetching": { "default": false, "description": "Type conditioned fetching configuration.", diff --git a/apollo-router/src/configuration/tests.rs b/apollo-router/src/configuration/tests.rs index d92de4d10b..addbc96130 100644 --- a/apollo-router/src/configuration/tests.rs +++ b/apollo-router/src/configuration/tests.rs @@ -1339,3 +1339,106 @@ fn it_prevents_enablement_of_both_subgraph_caching_plugins() { serde_json::from_value(make_config(Some(true), Some(true))); config_result.expect_err("both plugins configured"); } + +#[rstest::rstest] +#[case::all_enabled("some_subgraph_name", true, &[], true)] +#[case::all_enabled_unknown("unknown", true, &[], true)] +#[case::subgraph_enabled("some_subgraph_name", false, &[("some_subgraph_name", true)], true)] +#[case::subgraph_disabled("disabled", false, &[("disabled", false)], false)] +#[case::subgraph_unknown_falls_back_to_all("unknown", false, &[("some_subgraph_name", true)], false)] +#[case::default_hoists_nothing("anything", false, &[], false)] +#[case::all_with_subgraph_override("overridden", true, &[("overridden", false)], false)] +fn hoist_orphan_errors_get( + #[case] query_subgraph: &str, + #[case] all_enabled: bool, + #[case] subgraph_entries: &[(&str, bool)], + #[case] expected: bool, +) { + let config = super::subgraph::SubgraphConfiguration { + all: super::HoistOrphanErrors { + enabled: all_enabled, + }, + subgraphs: subgraph_entries + .iter() + .map(|(k, v)| (k.to_string(), super::HoistOrphanErrors { enabled: *v })) + .collect(), + }; + assert_eq!(config.get(query_subgraph).enabled, expected); +} + +#[test] +fn hoist_orphan_errors_deserializes_with_subgraphs() { + let config: Configuration = serde_json::from_value(json!({ + "experimental_hoist_orphan_errors": { + "subgraphs": { + "some_subgraph_name": { + "enabled": true + }, + "other": { + "enabled": false + } + } + } + })) + .expect("valid config"); + assert!( + config + .experimental_hoist_orphan_errors + .get("some_subgraph_name") + .enabled + ); + assert!(!config.experimental_hoist_orphan_errors.get("other").enabled); + assert!( + !config + .experimental_hoist_orphan_errors + .get("unknown") + .enabled + ); +} + +#[test] +fn hoist_orphan_errors_deserializes_with_all() { + let config: Configuration = serde_json::from_value(json!({ + "experimental_hoist_orphan_errors": { + "all": { + "enabled": true + } + } + })) + .expect("valid config"); + assert!( + config + .experimental_hoist_orphan_errors + .get("anything") + .enabled + ); +} + +#[test] +fn hoist_orphan_errors_all_with_subgraph_override() { + let config: Configuration = serde_json::from_value(json!({ + "experimental_hoist_orphan_errors": { + "all": { + "enabled": true + }, + "subgraphs": { + "noisy_one": { + "enabled": false + } + } + } + })) + .expect("valid config"); + assert!( + config + .experimental_hoist_orphan_errors + .get("anything") + .enabled + ); + assert!( + !config + .experimental_hoist_orphan_errors + .get("noisy_one") + .enabled + ); +} diff --git a/apollo-router/src/executable.rs b/apollo-router/src/executable.rs index 02d0f6c0f8..513a1f9671 100644 --- a/apollo-router/src/executable.rs +++ b/apollo-router/src/executable.rs @@ -919,6 +919,7 @@ mod tests { experimental_chaos: Default::default(), batching: Default::default(), experimental_type_conditioned_fetching: false, + experimental_hoist_orphan_errors: Default::default(), plugins: Default::default(), apollo_plugins: Default::default(), notify: Default::default(), @@ -1000,6 +1001,7 @@ mod tests { experimental_chaos: Default::default(), batching: Default::default(), experimental_type_conditioned_fetching: false, + experimental_hoist_orphan_errors: Default::default(), plugins: Default::default(), apollo_plugins: Default::default(), notify: Default::default(), diff --git a/apollo-router/src/query_planner/fetch.rs b/apollo-router/src/query_planner/fetch.rs index 7ef3cb75e2..83083067b8 100644 --- a/apollo-router/src/query_planner/fetch.rs +++ b/apollo-router/src/query_planner/fetch.rs @@ -274,6 +274,7 @@ impl FetchNode { paths: Vec>, operation_str: &str, variables: Map, + hoist_orphan_errors: bool, ) -> (Value, Vec) { let (_parts, response) = match service .oneshot(subgraph_request) @@ -301,7 +302,8 @@ impl FetchNode { ); } - let (value, errors) = self.response_at_path(schema, current_dir, paths, response); + let (value, errors) = + self.response_at_path(schema, current_dir, paths, response, hoist_orphan_errors); (value, errors) } @@ -337,9 +339,9 @@ impl FetchNode { /// into the right slots for the supergraph response, and it does that by a bit of path /// handling and manipulation /// - /// Importantly, it makes a decision about entity-less errors. When we have such an error, it's - /// unclear where it should be applied so we apply them to the most immediate parent of the - /// current_dir + /// When `hoist_orphan_errors` is true, entity-less errors are assigned to the nearest + /// non-array ancestor of `current_dir`, preventing error multiplication across array + /// elements. When false, they are assigned to `current_dir` as-is. #[instrument(skip_all, level = "debug", name = "response_insert")] pub(crate) fn response_at_path<'a>( &'a self, @@ -347,6 +349,7 @@ impl FetchNode { current_dir: &'a Path, inverted_paths: Vec>, response: graphql::Response, + hoist_orphan_errors: bool, ) -> (Value, Vec) { if !self.requires.is_empty() { let entities_path = Path(vec![json_ext::PathElement::Key( @@ -354,38 +357,21 @@ impl FetchNode { None, )]); - // the fallback_dir is the immediate parent of the current_dir when the current_dir is - // wildcarded (ie, @, which is PathElement::Flatten--conceptually, flattening is the - // application of some behavior to every index in an array) - // - // this is important because sometimes we get paths that don't start with _entities and - // we're unsure where to apply any errors returned by subgraphs; previously we applied - // them to the wildcarded current_dir, which means we applied them to _everything_ at - // that path; if we were handling an array, we'd apply each error to every element of - // that array + // when hoist_orphan_errors is enabled, the fallback_dir is the immediate parent of + // the current_dir when the current_dir is wildcarded (ie, @, which is PathElement::Flatten) // - // you can guess what that might do: if you have a query returning some large number of - // errors and every one of those errors is applied to every index represented in the - // array of data that we're building for the response, you get an explosion of errors - // (and downstream of this function, an explosion of memory allocations when we clone - // each error in FlattenNode--worse, we add them to a vec and that vec will get resized - // when its capacity is hit, creating a significant memory burden that almost certainly - // leads to an OOMKill) - // - // the moral of the story is that we need to be very careful in this function when we - // apply wildcards for paths - let fallback_dir = { - // if we have a wildcard (@, Flatten) + // this prevents error multiplication across array elements + let error_dir = if hoist_orphan_errors { let pos = current_dir .0 .iter() .position(|e| matches!(e, json_ext::PathElement::Flatten(_))); - // then take the most immediate parent match pos { Some(i) => Path(current_dir.0[..i].to_vec()), - // otherwise, use the current_dir None => current_dir.clone(), } + } else { + current_dir.clone() }; let mut errors: Vec = vec![]; @@ -422,16 +408,16 @@ impl FetchNode { } } _ => { - error.path = Some(fallback_dir.clone()); + error.path = Some(error_dir.clone()); errors.push(error) } } } else { - error.path = Some(fallback_dir.clone()); + error.path = Some(error_dir.clone()); errors.push(error); } } else { - error.path = Some(fallback_dir.clone()); + error.path = Some(error_dir.clone()); errors.push(error); } } @@ -695,7 +681,7 @@ mod tests { data, ..Default::default() }; - let (value, errors) = node.response_at_path(&schema, ¤t_dir, vec![], response); + let (value, errors) = node.response_at_path(&schema, ¤t_dir, vec![], response, false); assert!(errors.is_empty()); assert_eq!(value, expected); @@ -744,7 +730,7 @@ mod tests { .error(make_error(error_path)) .build(); - let (_, errors) = node.response_at_path(&schema, ¤t_dir, vec![], response); + let (_, errors) = node.response_at_path(&schema, ¤t_dir, vec![], response, false); assert_eq!(errors.len(), 1); assert_eq!(errors[0].path.as_ref().unwrap(), &expected_path); @@ -771,7 +757,7 @@ mod tests { .error(graphql::Error::builder().message("error 3").build()) .build(); - let (_, errors) = node.response_at_path(&schema, ¤t_dir, vec![], response); + let (_, errors) = node.response_at_path(&schema, ¤t_dir, vec![], response, false); assert_eq!(errors.len(), 3); assert_eq!( @@ -800,7 +786,7 @@ mod tests { ) .build(); - let (_, errors) = node.response_at_path(&schema, ¤t_dir, vec![], response); + let (_, errors) = node.response_at_path(&schema, ¤t_dir, vec![], response, false); assert_eq!(errors.len(), 1); assert_eq!(errors[0].extension_code().as_deref(), Some("UNAUTHORIZED")); @@ -850,7 +836,42 @@ mod tests { .error(make_error(error_path)) .build(); - let (_, errors) = node.response_at_path(&schema, ¤t_dir, vec![], response); + let (_, errors) = node.response_at_path(&schema, ¤t_dir, vec![], response, true); + + assert_eq!(errors.len(), 1); + assert_eq!(errors[0].path.as_ref().unwrap(), &expected_path); + } + + #[rstest] + #[case::flatten_preserved_when_disabled( + vec![key("users"), flatten()], + None, + Path(vec![key("users"), flatten()]) + )] + #[case::nested_flatten_preserved_when_disabled( + vec![key("a"), flatten(), key("b"), flatten()], + None, + Path(vec![key("a"), flatten(), key("b"), flatten()]) + )] + #[case::non_entities_path_gets_current_dir_when_disabled( + vec![key("items"), flatten()], + Some(Path(vec![key("something")])), + Path(vec![key("items"), flatten()]) + )] + fn entity_error_uses_current_dir_when_hoist_disabled( + #[case] dir_elements: Vec, + #[case] error_path: Option, + #[case] expected_path: Path, + ) { + let schema = test_schema(); + let node = make_fetch_node(make_requires()); + let current_dir = Path(dir_elements); + let response = graphql::Response::builder() + .data(json!({"_entities": []})) + .error(make_error(error_path)) + .build(); + + let (_, errors) = node.response_at_path(&schema, ¤t_dir, vec![], response, false); assert_eq!(errors.len(), 1); assert_eq!(errors[0].path.as_ref().unwrap(), &expected_path); @@ -875,7 +896,7 @@ mod tests { .build(); let (value, errors) = - node.response_at_path(&schema, ¤t_dir, inverted_paths, response); + node.response_at_path(&schema, ¤t_dir, inverted_paths, response, false); assert!(errors.is_empty()); let top = value.as_object().unwrap().get("topField").unwrap(); @@ -900,7 +921,7 @@ mod tests { .build(); let (value, errors) = - node.response_at_path(&schema, ¤t_dir, inverted_paths, response); + node.response_at_path(&schema, ¤t_dir, inverted_paths, response, false); assert!(errors.is_empty()); let arr = value @@ -923,7 +944,7 @@ mod tests { .data(json!({"_entities": []})) .build(); - let (value, errors) = node.response_at_path(&schema, ¤t_dir, vec![], response); + let (value, errors) = node.response_at_path(&schema, ¤t_dir, vec![], response, false); assert!(errors.is_empty()); assert_eq!(value, Value::default()); @@ -946,7 +967,7 @@ mod tests { .build(); let (value, errors) = - node.response_at_path(&schema, ¤t_dir, inverted_paths, response); + node.response_at_path(&schema, ¤t_dir, inverted_paths, response, false); assert!(errors.is_empty()); let arr = value @@ -978,7 +999,8 @@ mod tests { ) .build(); - let (_, errors) = node.response_at_path(&schema, ¤t_dir, inverted_paths, response); + let (_, errors) = + node.response_at_path(&schema, ¤t_dir, inverted_paths, response, false); assert_eq!(errors.len(), 1); assert_eq!( @@ -1009,6 +1031,7 @@ mod tests { ¤t_dir, vec![vec![Path(vec![key("data"), index(0)])]], response, + false, ); assert_eq!(errors.len(), 1); @@ -1034,7 +1057,8 @@ mod tests { ) .build(); - let (_, errors) = node.response_at_path(&schema, ¤t_dir, inverted_paths, response); + let (_, errors) = + node.response_at_path(&schema, ¤t_dir, inverted_paths, response, false); assert_eq!(errors.len(), 2); assert_eq!( @@ -1062,7 +1086,7 @@ mod tests { ) .build(); - let (_, errors) = node.response_at_path(&schema, ¤t_dir, vec![], response); + let (_, errors) = node.response_at_path(&schema, ¤t_dir, vec![], response, false); assert!(errors.is_empty()); } @@ -1084,7 +1108,8 @@ mod tests { ) .build(); - let (_, errors) = node.response_at_path(&schema, ¤t_dir, inverted_paths, response); + let (_, errors) = + node.response_at_path(&schema, ¤t_dir, inverted_paths, response, false); assert_eq!(errors.len(), 1); assert_eq!(errors[0].extension_code().as_deref(), Some("FORBIDDEN")); @@ -1115,7 +1140,8 @@ mod tests { ) .build(); - let (_, errors) = node.response_at_path(&schema, ¤t_dir, inverted_paths, response); + let (_, errors) = + node.response_at_path(&schema, ¤t_dir, inverted_paths, response, false); assert_eq!(errors.len(), 1); assert_eq!( @@ -1138,7 +1164,7 @@ mod tests { ) .build(); - let (value, errors) = node.response_at_path(&schema, ¤t_dir, vec![], response); + let (value, errors) = node.response_at_path(&schema, ¤t_dir, vec![], response, false); assert_eq!(value, Value::Null); assert_eq!(errors.len(), 1); @@ -1154,7 +1180,7 @@ mod tests { .data(json!({"something": "else"})) .build(); - let (value, errors) = node.response_at_path(&schema, ¤t_dir, vec![], response); + let (value, errors) = node.response_at_path(&schema, ¤t_dir, vec![], response, false); assert_eq!(value, Value::Null); assert!(errors.is_empty()); @@ -1169,7 +1195,7 @@ mod tests { .error(graphql::Error::builder().message("subgraph error").build()) .build(); - let (value, errors) = node.response_at_path(&schema, ¤t_dir, vec![], response); + let (value, errors) = node.response_at_path(&schema, ¤t_dir, vec![], response, false); assert_eq!(value, Value::Null); assert_eq!(errors.len(), 1); @@ -1197,7 +1223,7 @@ mod tests { ) .build(); - let (value, errors) = node.response_at_path(&schema, ¤t_dir, vec![], response); + let (value, errors) = node.response_at_path(&schema, ¤t_dir, vec![], response, true); assert_eq!(value, Value::Null); assert_eq!(errors.len(), 3); @@ -1232,7 +1258,7 @@ mod tests { ) .build(); - let (value, errors) = node.response_at_path(&schema, ¤t_dir, vec![], response); + let (value, errors) = node.response_at_path(&schema, ¤t_dir, vec![], response, true); assert_eq!(value, Value::Null); assert_eq!(errors.len(), 2); @@ -1255,7 +1281,7 @@ mod tests { .data(json!({"_entities": "not_an_array"})) .build(); - let (value, errors) = node.response_at_path(&schema, ¤t_dir, vec![], response); + let (value, errors) = node.response_at_path(&schema, ¤t_dir, vec![], response, false); assert_eq!(value, Value::Null); assert!(errors.is_empty()); @@ -1272,7 +1298,7 @@ mod tests { .error(graphql::Error::builder().message("bad entities").build()) .build(); - let (value, errors) = node.response_at_path(&schema, ¤t_dir, vec![], response); + let (value, errors) = node.response_at_path(&schema, ¤t_dir, vec![], response, true); assert_eq!(value, Value::Null); assert_eq!(errors.len(), 1); @@ -1291,7 +1317,7 @@ mod tests { ..Default::default() }; - let (value, errors) = node.response_at_path(&schema, ¤t_dir, vec![], response); + let (value, errors) = node.response_at_path(&schema, ¤t_dir, vec![], response, true); assert_eq!(value, Value::Null); assert_eq!(errors.len(), 1); @@ -1324,7 +1350,8 @@ mod tests { .error(graphql::Error::builder().message("pathless").build()) .build(); - let (_, errors) = node.response_at_path(&schema, ¤t_dir, inverted_paths, response); + let (_, errors) = + node.response_at_path(&schema, ¤t_dir, inverted_paths, response, true); assert_eq!(errors.len(), 3); assert_eq!( diff --git a/apollo-router/src/query_planner/tests.rs b/apollo-router/src/query_planner/tests.rs index 25431e4748..af28576267 100644 --- a/apollo-router/src/query_planner/tests.rs +++ b/apollo-router/src/query_planner/tests.rs @@ -24,6 +24,8 @@ use crate::Context; use crate::MockedSubgraphs; use crate::TestHarness; use crate::apollo_studio_interop::UsageReporting; +use crate::configuration::HoistOrphanErrors; +use crate::configuration::subgraph::SubgraphConfiguration; use crate::graphql; use crate::json_ext::Path; use crate::json_ext::PathElement; @@ -123,6 +125,7 @@ async fn mock_subgraph_service_with_panics_should_be_reported_as_service_closed( Arc::new(ssf), None, Arc::new(ConnectorServiceFactory::empty(schema.clone())), + Arc::new(SubgraphConfiguration::::default()), )); let result = query_plan @@ -191,6 +194,7 @@ async fn fetch_includes_operation_name() { Arc::new(ssf), None, Arc::new(ConnectorServiceFactory::empty(schema.clone())), + Arc::new(SubgraphConfiguration::::default()), )); let _response = query_plan @@ -256,6 +260,7 @@ async fn fetch_makes_post_requests() { Arc::new(ssf), None, Arc::new(ConnectorServiceFactory::empty(schema.clone())), + Arc::new(SubgraphConfiguration::::default()), )); let _response = query_plan @@ -418,6 +423,7 @@ async fn defer() { Arc::new(ssf), None, Arc::new(ConnectorServiceFactory::empty(schema.clone())), + Arc::new(SubgraphConfiguration::::default()), )); let response = query_plan @@ -521,6 +527,7 @@ async fn defer_if_condition() { Arc::new(ssf), None, Arc::new(ConnectorServiceFactory::empty(schema.clone())), + Arc::new(SubgraphConfiguration::::default()), )); let defer_primary_response = query_plan @@ -687,6 +694,7 @@ async fn dependent_mutations() { Arc::new(ssf), None, Arc::new(ConnectorServiceFactory::empty(schema.clone())), + Arc::new(SubgraphConfiguration::::default()), )); let (sender, _) = tokio::sync::mpsc::channel(10); diff --git a/apollo-router/src/services/fetch_service.rs b/apollo-router/src/services/fetch_service.rs index 66119ad94c..3d63ebc22e 100644 --- a/apollo-router/src/services/fetch_service.rs +++ b/apollo-router/src/services/fetch_service.rs @@ -17,6 +17,8 @@ use super::connector_service::ConnectorServiceFactory; use super::fetch::AddSubgraphNameExt; use super::fetch::BoxService; use super::new_service::ServiceFactory; +use crate::configuration::HoistOrphanErrors; +use crate::configuration::subgraph::SubgraphConfiguration; use crate::graphql::Request as GraphQLRequest; use crate::http_ext; use crate::plugins::subscription::SubscriptionConfig; @@ -40,6 +42,7 @@ pub(crate) struct FetchService { pub(crate) subgraph_schemas: Arc, pub(crate) _subscription_config: Option, // TODO: add subscription support to FetchService pub(crate) connector_service_factory: Arc, + pub(crate) hoist_orphan_errors: Arc>, } impl tower::Service for FetchService { @@ -77,6 +80,7 @@ impl FetchService { } = request; let service_name = service_name.clone(); let fetch_time_offset = context.created_at.elapsed().as_nanos() as i64; + let hoist_orphan_errors = self.hoist_orphan_errors.get(&service_name).enabled; if let Some(connector) = self .connector_service_factory @@ -88,6 +92,7 @@ impl FetchService { self.connector_service_factory.clone(), connector.id.subgraph_name.clone(), request, + hoist_orphan_errors, ) .instrument(tracing::info_span!( FETCH_SPAN_NAME, @@ -101,6 +106,7 @@ impl FetchService { self.subgraph_service_factory.clone(), self.subgraph_schemas.clone(), request, + hoist_orphan_errors, ) .instrument(tracing::info_span!( FETCH_SPAN_NAME, @@ -116,6 +122,7 @@ impl FetchService { connector_service_factory: Arc, subgraph_name: String, request: FetchRequest, + hoist_orphan_errors: bool, ) -> BoxFuture<'static, Result> { let FetchRequest { fetch_node, @@ -160,8 +167,13 @@ impl FetchService { Ok(res) => res.response.into_parts(), }; - let (value, errors) = - fetch_node.response_at_path(&schema, ¤t_dir, paths, response); + let (value, errors) = fetch_node.response_at_path( + &schema, + ¤t_dir, + paths, + response, + hoist_orphan_errors, + ); Ok((value, errors)) }) } @@ -171,6 +183,7 @@ impl FetchService { subgraph_service_factory: Arc, subgraph_schemas: Arc, request: FetchRequest, + hoist_orphan_errors: bool, ) -> BoxFuture<'static, Result> { let FetchRequest { fetch_node, @@ -261,6 +274,7 @@ impl FetchService { variables.inverted_paths, &aqs, variables.variables, + hoist_orphan_errors, ) .await) }) @@ -274,6 +288,7 @@ pub(crate) struct FetchServiceFactory { pub(crate) subgraph_service_factory: Arc, pub(crate) subscription_config: Option, pub(crate) connector_service_factory: Arc, + pub(crate) hoist_orphan_errors: Arc>, } impl FetchServiceFactory { @@ -283,6 +298,7 @@ impl FetchServiceFactory { subgraph_service_factory: Arc, subscription_config: Option, connector_service_factory: Arc, + hoist_orphan_errors: Arc>, ) -> Self { Self { subgraph_service_factory, @@ -290,6 +306,7 @@ impl FetchServiceFactory { schema, subscription_config, connector_service_factory, + hoist_orphan_errors, } } } @@ -304,6 +321,7 @@ impl ServiceFactory for FetchServiceFactory { subgraph_schemas: self.subgraph_schemas.clone(), _subscription_config: self.subscription_config.clone(), connector_service_factory: self.connector_service_factory.clone(), + hoist_orphan_errors: self.hoist_orphan_errors.clone(), } .boxed() } diff --git a/apollo-router/src/services/supergraph/service.rs b/apollo-router/src/services/supergraph/service.rs index 1eb40ba84d..83f47c7486 100644 --- a/apollo-router/src/services/supergraph/service.rs +++ b/apollo-router/src/services/supergraph/service.rs @@ -572,6 +572,7 @@ impl PluggableSupergraphServiceBuilder { connector_sources, )), )), + Arc::new(configuration.experimental_hoist_orphan_errors.clone()), )); let execution_service_factory = ExecutionServiceFactory { diff --git a/apollo-router/src/services/supergraph/snapshots/apollo_router__services__supergraph__tests__deferred_fragment_bounds_nullability-2.snap b/apollo-router/src/services/supergraph/snapshots/apollo_router__services__supergraph__tests__deferred_fragment_bounds_nullability-2.snap index 894e59df42..9a2f8f48b1 100644 --- a/apollo-router/src/services/supergraph/snapshots/apollo_router__services__supergraph__tests__deferred_fragment_bounds_nullability-2.snap +++ b/apollo-router/src/services/supergraph/snapshots/apollo_router__services__supergraph__tests__deferred_fragment_bounds_nullability-2.snap @@ -19,7 +19,8 @@ expression: stream.next_response().await.unwrap() "path": [ "currentUser", "activeOrganization", - "suborga" + "suborga", + 0 ], "extensions": { "code": "FETCH_ERROR", @@ -55,7 +56,8 @@ expression: stream.next_response().await.unwrap() "path": [ "currentUser", "activeOrganization", - "suborga" + "suborga", + 1 ], "extensions": { "code": "FETCH_ERROR", @@ -91,7 +93,8 @@ expression: stream.next_response().await.unwrap() "path": [ "currentUser", "activeOrganization", - "suborga" + "suborga", + 2 ], "extensions": { "code": "FETCH_ERROR", diff --git a/apollo-router/tests/integration/query_planner/snapshots/integration_tests__integration__query_planner__error_paths__multi_level_response_failure.snap b/apollo-router/tests/integration/query_planner/snapshots/integration_tests__integration__query_planner__error_paths__multi_level_response_failure.snap index b64956d9fe..dfae1d9262 100644 --- a/apollo-router/tests/integration/query_planner/snapshots/integration_tests__integration__query_planner__error_paths__multi_level_response_failure.snap +++ b/apollo-router/tests/integration/query_planner/snapshots/integration_tests__integration__query_planner__error_paths__multi_level_response_failure.snap @@ -44,7 +44,8 @@ expression: response { "message": "service 'inventory' response was malformed: graphql response without data must contain at least one error", "path": [ - "topProducts" + "topProducts", + 0 ], "extensions": { "service": "inventory", @@ -52,10 +53,56 @@ expression: response "code": "SUBREQUEST_MALFORMED_RESPONSE" } }, + { + "message": "service 'inventory' response was malformed: graphql response without data must contain at least one error", + "path": [ + "topProducts", + 1 + ], + "extensions": { + "service": "inventory", + "reason": "graphql response without data must contain at least one error", + "code": "SUBREQUEST_MALFORMED_RESPONSE" + } + }, + { + "message": "service 'accounts' response was malformed: graphql response without data must contain at least one error", + "path": [ + "topProducts", + 0, + "reviews", + 0, + "author" + ], + "extensions": { + "service": "accounts", + "reason": "graphql response without data must contain at least one error", + "code": "SUBREQUEST_MALFORMED_RESPONSE" + } + }, + { + "message": "service 'accounts' response was malformed: graphql response without data must contain at least one error", + "path": [ + "topProducts", + 0, + "reviews", + 1, + "author" + ], + "extensions": { + "service": "accounts", + "reason": "graphql response without data must contain at least one error", + "code": "SUBREQUEST_MALFORMED_RESPONSE" + } + }, { "message": "service 'accounts' response was malformed: graphql response without data must contain at least one error", "path": [ - "topProducts" + "topProducts", + 1, + "reviews", + 0, + "author" ], "extensions": { "service": "accounts", diff --git a/apollo-router/tests/integration/query_planner/snapshots/integration_tests__integration__query_planner__error_paths__nested_response_failure_404.snap b/apollo-router/tests/integration/query_planner/snapshots/integration_tests__integration__query_planner__error_paths__nested_response_failure_404.snap index 8d3c801da9..13ccbc5089 100644 --- a/apollo-router/tests/integration/query_planner/snapshots/integration_tests__integration__query_planner__error_paths__nested_response_failure_404.snap +++ b/apollo-router/tests/integration/query_planner/snapshots/integration_tests__integration__query_planner__error_paths__nested_response_failure_404.snap @@ -44,7 +44,11 @@ expression: response { "message": "HTTP fetch failed from 'accounts': 404: Not Found", "path": [ - "topProducts" + "topProducts", + 0, + "reviews", + 0, + "author" ], "extensions": { "code": "SUBREQUEST_HTTP_ERROR", @@ -55,10 +59,86 @@ expression: response } } }, + { + "message": "HTTP fetch failed from 'accounts': 404: Not Found", + "path": [ + "topProducts", + 0, + "reviews", + 1, + "author" + ], + "extensions": { + "code": "SUBREQUEST_HTTP_ERROR", + "service": "accounts", + "reason": "404: Not Found", + "http": { + "status": 404 + } + } + }, + { + "message": "HTTP fetch failed from 'accounts': 404: Not Found", + "path": [ + "topProducts", + 1, + "reviews", + 0, + "author" + ], + "extensions": { + "code": "SUBREQUEST_HTTP_ERROR", + "service": "accounts", + "reason": "404: Not Found", + "http": { + "status": 404 + } + } + }, + { + "message": "HTTP fetch failed from 'accounts': subgraph response does not contain 'content-type' header; expected content-type: application/json or content-type: application/graphql-response+json", + "path": [ + "topProducts", + 0, + "reviews", + 0, + "author" + ], + "extensions": { + "code": "SUBREQUEST_HTTP_ERROR", + "service": "accounts", + "reason": "subgraph response does not contain 'content-type' header; expected content-type: application/json or content-type: application/graphql-response+json", + "http": { + "status": 404 + } + } + }, + { + "message": "HTTP fetch failed from 'accounts': subgraph response does not contain 'content-type' header; expected content-type: application/json or content-type: application/graphql-response+json", + "path": [ + "topProducts", + 0, + "reviews", + 1, + "author" + ], + "extensions": { + "code": "SUBREQUEST_HTTP_ERROR", + "service": "accounts", + "reason": "subgraph response does not contain 'content-type' header; expected content-type: application/json or content-type: application/graphql-response+json", + "http": { + "status": 404 + } + } + }, { "message": "HTTP fetch failed from 'accounts': subgraph response does not contain 'content-type' header; expected content-type: application/json or content-type: application/graphql-response+json", "path": [ - "topProducts" + "topProducts", + 1, + "reviews", + 0, + "author" ], "extensions": { "code": "SUBREQUEST_HTTP_ERROR", diff --git a/apollo-router/tests/integration/query_planner/snapshots/integration_tests__integration__query_planner__error_paths__nested_response_failure_malformed.snap b/apollo-router/tests/integration/query_planner/snapshots/integration_tests__integration__query_planner__error_paths__nested_response_failure_malformed.snap index 6124b3a4cf..31b679c697 100644 --- a/apollo-router/tests/integration/query_planner/snapshots/integration_tests__integration__query_planner__error_paths__nested_response_failure_malformed.snap +++ b/apollo-router/tests/integration/query_planner/snapshots/integration_tests__integration__query_planner__error_paths__nested_response_failure_malformed.snap @@ -44,7 +44,41 @@ expression: response { "message": "service 'accounts' response was malformed: graphql response without data must contain at least one error", "path": [ - "topProducts" + "topProducts", + 0, + "reviews", + 0, + "author" + ], + "extensions": { + "service": "accounts", + "reason": "graphql response without data must contain at least one error", + "code": "SUBREQUEST_MALFORMED_RESPONSE" + } + }, + { + "message": "service 'accounts' response was malformed: graphql response without data must contain at least one error", + "path": [ + "topProducts", + 0, + "reviews", + 1, + "author" + ], + "extensions": { + "service": "accounts", + "reason": "graphql response without data must contain at least one error", + "code": "SUBREQUEST_MALFORMED_RESPONSE" + } + }, + { + "message": "service 'accounts' response was malformed: graphql response without data must contain at least one error", + "path": [ + "topProducts", + 1, + "reviews", + 0, + "author" ], "extensions": { "service": "accounts", diff --git a/apollo-router/tests/integration/query_planner/snapshots/integration_tests__integration__query_planner__error_paths__second_level_response_failure_empty_path.snap b/apollo-router/tests/integration/query_planner/snapshots/integration_tests__integration__query_planner__error_paths__second_level_response_failure_empty_path.snap index 155e18fb17..983a56dbe1 100644 --- a/apollo-router/tests/integration/query_planner/snapshots/integration_tests__integration__query_planner__error_paths__second_level_response_failure_empty_path.snap +++ b/apollo-router/tests/integration/query_planner/snapshots/integration_tests__integration__query_planner__error_paths__second_level_response_failure_empty_path.snap @@ -44,7 +44,18 @@ expression: response { "message": "inventory error", "path": [ - "topProducts" + "topProducts", + 0 + ], + "extensions": { + "service": "inventory" + } + }, + { + "message": "inventory error", + "path": [ + "topProducts", + 1 ], "extensions": { "service": "inventory" diff --git a/apollo-router/tests/integration/query_planner/snapshots/integration_tests__integration__query_planner__error_paths__second_level_response_failure_malformed.snap b/apollo-router/tests/integration/query_planner/snapshots/integration_tests__integration__query_planner__error_paths__second_level_response_failure_malformed.snap index 7a984a7757..6f155d1399 100644 --- a/apollo-router/tests/integration/query_planner/snapshots/integration_tests__integration__query_planner__error_paths__second_level_response_failure_malformed.snap +++ b/apollo-router/tests/integration/query_planner/snapshots/integration_tests__integration__query_planner__error_paths__second_level_response_failure_malformed.snap @@ -44,7 +44,20 @@ expression: response { "message": "service 'inventory' response was malformed: graphql response without data must contain at least one error", "path": [ - "topProducts" + "topProducts", + 0 + ], + "extensions": { + "service": "inventory", + "reason": "graphql response without data must contain at least one error", + "code": "SUBREQUEST_MALFORMED_RESPONSE" + } + }, + { + "message": "service 'inventory' response was malformed: graphql response without data must contain at least one error", + "path": [ + "topProducts", + 1 ], "extensions": { "service": "inventory", diff --git a/apollo-router/tests/integration/snapshots/integration_tests__integration__coprocessor__on_graphql_error_selector__nested_response_failure.snap b/apollo-router/tests/integration/snapshots/integration_tests__integration__coprocessor__on_graphql_error_selector__nested_response_failure.snap index bd10cf48aa..131e577b4e 100644 --- a/apollo-router/tests/integration/snapshots/integration_tests__integration__coprocessor__on_graphql_error_selector__nested_response_failure.snap +++ b/apollo-router/tests/integration/snapshots/integration_tests__integration__coprocessor__on_graphql_error_selector__nested_response_failure.snap @@ -6,7 +6,18 @@ expression: errors { "message": "inventory error", "path": [ - "topProducts" + "topProducts", + 0 + ], + "extensions": { + "service": "inventory" + } + }, + { + "message": "inventory error", + "path": [ + "topProducts", + 1 ], "extensions": { "service": "inventory" diff --git a/apollo-router/tests/integration/snapshots/integration_tests__integration__demand_control__requests_exceeding_one_subgraph_cost_are_accepted@federated_ships_required.snap b/apollo-router/tests/integration/snapshots/integration_tests__integration__demand_control__requests_exceeding_one_subgraph_cost_are_accepted@federated_ships_required.snap index d2dcbb9814..15fb908bea 100644 --- a/apollo-router/tests/integration/snapshots/integration_tests__integration__demand_control__requests_exceeding_one_subgraph_cost_are_accepted@federated_ships_required.snap +++ b/apollo-router/tests/integration/snapshots/integration_tests__integration__demand_control__requests_exceeding_one_subgraph_cost_are_accepted@federated_ships_required.snap @@ -49,7 +49,36 @@ expression: parse_result_for_snapshot(response).await }, "message": "query estimated cost 110 exceeded configured maximum 1 for subgraph users", "path": [ - "ships" + "ships", + 0 + ] + }, + { + "extensions": { + "code": "SUBGRAPH_COST_ESTIMATED_TOO_EXPENSIVE", + "cost.subgraph": "users", + "cost.subgraph.estimated": 110.0, + "cost.subgraph.max": 1.0, + "service": "users" + }, + "message": "query estimated cost 110 exceeded configured maximum 1 for subgraph users", + "path": [ + "ships", + 1 + ] + }, + { + "extensions": { + "code": "SUBGRAPH_COST_ESTIMATED_TOO_EXPENSIVE", + "cost.subgraph": "users", + "cost.subgraph.estimated": 110.0, + "cost.subgraph.max": 1.0, + "service": "users" + }, + "message": "query estimated cost 110 exceeded configured maximum 1 for subgraph users", + "path": [ + "ships", + 2 ] } ]