From 18913448ec0700564d040034a24d24a6381ef33e Mon Sep 17 00:00:00 2001 From: Tim Hingston Date: Fri, 6 Sep 2024 14:00:35 +1000 Subject: [PATCH 01/13] Add docs for enhanced OTel tracing in Studio --- docs/source/configuration/overview.mdx | 14 ++++ .../telemetry/apollo-telemetry.mdx | 64 +++++++++++++++++++ 2 files changed, 78 insertions(+) diff --git a/docs/source/configuration/overview.mdx b/docs/source/configuration/overview.mdx index 13e6991a6ae..c96db9d4fc2 100644 --- a/docs/source/configuration/overview.mdx +++ b/docs/source/configuration/overview.mdx @@ -865,6 +865,20 @@ You won't see an immediate change in checks behavior when you first turn on exte + + +### Enhanced tracing in Studio via OpenTelemetry + + + + + +Beginning in v1.49.0, the router supports sending traces to Studio via the more detailed OTel (OpenTelemetry) protocol. +Support for OTel traces has historically only been available for 3rd party APM tools. With this option, +Studio can now provide a much more granular view of Router internals than the legacy Apollo tracing protocol. + +See [Enhanced tracing in Studio via OTel](./telemetry/apollo-telemetry#enhanced-tracing-in-studio-via-opentelemetry). + ### Safelisting with persisted queries You can enhance your graph's security with GraphOS Router by maintaining a persisted query list (PQL), an operation safelist made by your first-party apps. As opposed to automatic persisted queries (APQ) where operations are automatically cached, operations must be preregistered to the PQL. Once configured, the router checks incoming requests against the PQL. diff --git a/docs/source/configuration/telemetry/apollo-telemetry.mdx b/docs/source/configuration/telemetry/apollo-telemetry.mdx index c5f6c0c6095..d4e276118ea 100644 --- a/docs/source/configuration/telemetry/apollo-telemetry.mdx +++ b/docs/source/configuration/telemetry/apollo-telemetry.mdx @@ -72,6 +72,70 @@ telemetry: field_level_instrumentation_sampler: always_off ``` + + +### Enhanced tracing in Studio via OpenTelemetry + + + + + +Beginning in v1.49.0, the router supports sending traces to Studio via the more detailed OTel (OpenTelemetry) protocol. +Support for OTel traces has historically only been available for 3rd party APM tools. With this option, +Studio can now provide a much more granular view of Router internals than the legacy Apollo tracing protocol. + +Benefits include: + +- A comprehensive way to visualize the Router execution path in Studio. +- Additional spans that were previously not included in Studio traces, such as query parsing, planning, execution, and more. +- Additional attributes including HTTP request details, REST connector details, and more. + +It is expected that this will become the default in a future version of Router. + +#### Configuration + +This change adds a new configuration option `telemetry.apollo.experimental_otlp_tracing_sampler`. Use this option to send +a percentage of traces to Studio via OTLP instead of the native Apollo Usage Reporting protocol. Supported values: + +- `always_off` (default): send all traces via the legacy Apollo Usage Reporting protocol. +- `always_on`: send all traces via OTLP. +- `0.0 - 1.0` (used for testing): the ratio of traces to send via OTLP (0.5 = 50 / 50). + +Note that this sampler is only applied _after_ the common tracing sampler, for example: + +#### Sample 1% of traces, send all traces via OTLP: + +```yaml +telemetry: + apollo: + # Send all traces via OTLP + experimental_otlp_tracing_sampler: always_on + + exporters: + tracing: + common: + # Sample traces at 1% of all traffic + sampler: 0.01 +``` + +OTel traces sent to Studio will not necessarily be identical to the ones sent to 3rd Party APM tools via OTLP: + +- Only specific OTLP attributes will be included for parity with what is provided in legacy traces today. This ensures that data privacy + is maintained in an equivalent manner. The existing Router configuration options for Apollo telemetry will continue to function + with OTLP traces, such as forwarding of GraphQL errors, headers, and variables. +- Some features of OTLP traces may only be available in Studio and not in 3rd Party APM tools (e.g. resolver-level timing information from + [Federated Tracing](../../federation/metrics/#enabling-federated-tracing)). + + + +This change results in using a new wire protocol for traces, and some users may experience an increase in tracing traffic +to GraphOS Studio due to the additional detail being captured. In exceptional situations it may be necessary to send fewer traces. +This can be achieved via sending fewer traces (`telemetry.exporters.tracing.common.sampler`) or as a last resort, falling back +to the old protocol via `telemetry.apollo.otlp_tracing_sampler` to send fewer OTLP traces or fully disable them. +Any performance regressions due to the new tracing protocol should also be reported to the Apollo support team. + + + ### Experimental local field metrics Apollo Router can send field-level metrics to GraphOS without using FTV1 tracing. This feature is experimental and is not yet displayable in GraphOS Studio. From 17d90ebc7db6aa8b8cd60256a1d9664e6aeb4c0a Mon Sep 17 00:00:00 2001 From: Taylor Ninesling Date: Fri, 25 Oct 2024 09:01:08 -0700 Subject: [PATCH 02/13] Remove unnecessary warn logs from ResponseVisitor (#6192) --- .../fix_tninesling_remove_demand_control_warnings.md | 5 +++++ apollo-router/src/graphql/visitor.rs | 7 ------- 2 files changed, 5 insertions(+), 7 deletions(-) create mode 100644 .changesets/fix_tninesling_remove_demand_control_warnings.md diff --git a/.changesets/fix_tninesling_remove_demand_control_warnings.md b/.changesets/fix_tninesling_remove_demand_control_warnings.md new file mode 100644 index 00000000000..195ad7d04ce --- /dev/null +++ b/.changesets/fix_tninesling_remove_demand_control_warnings.md @@ -0,0 +1,5 @@ +### Remove noisy demand control logs ([PR #6192](https://github.com/apollographql/router/pull/6192)) + +Demand control no longer logs warnings when a subgraph response is missing a requested field. + +By [@tninesling](https://github.com/tninesling) in https://github.com/apollographql/router/pull/6192 diff --git a/apollo-router/src/graphql/visitor.rs b/apollo-router/src/graphql/visitor.rs index b3443938214..aa901508db3 100644 --- a/apollo-router/src/graphql/visitor.rs +++ b/apollo-router/src/graphql/visitor.rs @@ -92,18 +92,11 @@ pub(crate) trait ResponseVisitor { inner_field.as_ref(), value, ); - } else { - tracing::warn!("The response did not include a field corresponding to query field {:?}", inner_field); } } apollo_compiler::executable::Selection::FragmentSpread(fragment_spread) => { if let Some(fragment) = fragment_spread.fragment_def(request) { self.visit_selections(request, variables, &fragment.selection_set, fields); - } else { - tracing::warn!( - "The fragment {} was not found in the query document.", - fragment_spread.fragment_name - ); } } apollo_compiler::executable::Selection::InlineFragment(inline_fragment) => { From b49acf3e0cca4866e3aa71eeb2cbedaaa4c8ec3e Mon Sep 17 00:00:00 2001 From: Duckki Oe Date: Fri, 25 Oct 2024 11:53:09 -0700 Subject: [PATCH 03/13] test(federation): snapshot logging improvements for `FetchDependencyGraph` and open/closed branches (#6190) --- .../src/query_graph/graph_path.rs | 6 + .../src/query_plan/fetch_dependency_graph.rs | 12 +- .../src/query_plan/query_planner.rs | 31 +++-- .../query_plan/query_planning_traversal.rs | 115 ++++++++++++++---- apollo-federation/src/utils/logging.rs | 91 ++++++++++++-- 5 files changed, 204 insertions(+), 51 deletions(-) diff --git a/apollo-federation/src/query_graph/graph_path.rs b/apollo-federation/src/query_graph/graph_path.rs index 7c9fe758274..da06fffcdf1 100644 --- a/apollo-federation/src/query_graph/graph_path.rs +++ b/apollo-federation/src/query_graph/graph_path.rs @@ -591,6 +591,12 @@ pub(crate) struct SimultaneousPathsWithLazyIndirectPaths { pub(crate) lazily_computed_indirect_paths: Vec>, } +impl Display for SimultaneousPathsWithLazyIndirectPaths { + fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { + write!(f, "{}", self.paths) + } +} + /// A "set" of excluded destinations (i.e. subgraph names). Note that we use a `Vec` instead of set /// because this is used in pretty hot paths (the whole path computation is CPU intensive) and will /// basically always be tiny (it's bounded by the number of distinct key on a given type, so usually diff --git a/apollo-federation/src/query_plan/fetch_dependency_graph.rs b/apollo-federation/src/query_plan/fetch_dependency_graph.rs index f31bd0d2ffc..5629183fd50 100644 --- a/apollo-federation/src/query_plan/fetch_dependency_graph.rs +++ b/apollo-federation/src/query_plan/fetch_dependency_graph.rs @@ -3352,11 +3352,7 @@ pub(crate) fn compute_nodes_for_tree( initial_defer_context: DeferContext, initial_conditions: &OpGraphPathContext, ) -> Result, FederationError> { - snapshot!( - "OpPathTree", - serde_json_bytes::json!(initial_tree.to_string()).to_string(), - "path_tree" - ); + snapshot!("OpPathTree", initial_tree.to_string(), "path_tree"); let mut stack = vec![ComputeNodesStackItem { tree: initial_tree, node_id: initial_node_id, @@ -3443,7 +3439,11 @@ pub(crate) fn compute_nodes_for_tree( } } } - snapshot!(dependency_graph, "updated_dependency_graph"); + snapshot!( + "FetchDependencyGraph", + dependency_graph.to_dot(), + "Fetch dependency graph updated by compute_nodes_for_tree" + ); Ok(created_nodes) } diff --git a/apollo-federation/src/query_plan/query_planner.rs b/apollo-federation/src/query_plan/query_planner.rs index 500076ce4c9..24e09729441 100644 --- a/apollo-federation/src/query_plan/query_planner.rs +++ b/apollo-federation/src/query_plan/query_planner.rs @@ -11,6 +11,7 @@ use apollo_compiler::ExecutableDocument; use apollo_compiler::Name; use itertools::Itertools; use serde::Serialize; +use tracing::trace; use super::fetch_dependency_graph::FetchIdGenerator; use super::ConditionNode; @@ -551,7 +552,15 @@ impl QueryPlanner { statistics, }; - snapshot!(plan, "query plan"); + snapshot!( + "QueryPlan", + plan.to_string(), + "QueryPlan from build_query_plan" + ); + snapshot!( + plan.statistics, + "QueryPlanningStatistics from build_query_plan" + ); Ok(plan) } @@ -716,7 +725,11 @@ pub(crate) fn compute_root_fetch_groups( root_kind, root_type.clone(), )?; - snapshot!(dependency_graph, "tree_with_root_node"); + snapshot!( + "FetchDependencyGraph", + dependency_graph.to_dot(), + "tree_with_root_node" + ); compute_nodes_for_tree( dependency_graph, &child.tree, @@ -737,16 +750,13 @@ fn compute_root_parallel_dependency_graph( parameters: &QueryPlanningParameters, has_defers: bool, ) -> Result { - snapshot!( - "FetchDependencyGraph", - "Empty", - "Starting process to construct a parallel fetch dependency graph" - ); + trace!("Starting process to construct a parallel fetch dependency graph"); let selection_set = parameters.operation.selection_set.clone(); let best_plan = compute_root_parallel_best_plan(parameters, selection_set, has_defers)?; snapshot!( - best_plan.fetch_dependency_graph, - "Plan returned from compute_root_parallel_best_plan" + "FetchDependencyGraph", + best_plan.fetch_dependency_graph.to_dot(), + "Fetch dependency graph returned from compute_root_parallel_best_plan" ); Ok(best_plan.fetch_dependency_graph) } @@ -807,7 +817,8 @@ fn compute_plan_internal( let (main, deferred) = dependency_graph.process(&mut *processor, root_kind)?; snapshot!( - dependency_graph, + "FetchDependencyGraph", + dependency_graph.to_dot(), "Plan after calling FetchDependencyGraph::process" ); // XXX(@goto-bus-stop) Maybe `.defer_tracking` should be on the return value of `process()`..? diff --git a/apollo-federation/src/query_plan/query_planning_traversal.rs b/apollo-federation/src/query_plan/query_planning_traversal.rs index 787013471a9..055498218f1 100644 --- a/apollo-federation/src/query_plan/query_planning_traversal.rs +++ b/apollo-federation/src/query_plan/query_planning_traversal.rs @@ -46,8 +46,17 @@ use crate::schema::position::CompositeTypeDefinitionPosition; use crate::schema::position::ObjectTypeDefinitionPosition; use crate::schema::position::SchemaRootDefinitionKind; use crate::schema::ValidFederationSchema; +use crate::utils::logging::format_open_branch; use crate::utils::logging::snapshot; +#[cfg(feature = "snapshot_tracing")] +mod snapshot_helper { + // A module to import functions only used within `snapshot!(...)` macros. + pub(crate) use crate::utils::logging::closed_branches_to_string; + pub(crate) use crate::utils::logging::open_branch_to_string; + pub(crate) use crate::utils::logging::open_branches_to_string; +} + // PORT_NOTE: Named `PlanningParameters` in the JS codebase, but there was no particular reason to // leave out to the `Query` prefix, so it's been added for consistency. Similar to `GraphPath`, we // don't have a distinguished type for when the head is a root vertex, so we instead check this at @@ -113,13 +122,33 @@ pub(crate) struct QueryPlanningTraversal<'a, 'b> { } #[derive(Debug, Serialize)] -struct OpenBranchAndSelections { +pub(crate) struct OpenBranchAndSelections { /// The options for this open branch. open_branch: OpenBranch, /// A stack of the remaining selections to plan from the node this open branch ends on. selections: Vec, } +impl std::fmt::Display for OpenBranchAndSelections { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + let Some((current_selection, remaining_selections)) = self.selections.split_last() else { + return Ok(()); + }; + format_open_branch(f, &(current_selection, &self.open_branch.0))?; + write!(f, " * Remaining selections:")?; + if remaining_selections.is_empty() { + writeln!(f, " (none)")?; + } else { + // Print in reverse order since remaining selections are processed in that order. + writeln!(f)?; // newline + for selection in remaining_selections.iter().rev() { + writeln!(f, " - {selection}")?; + } + } + Ok(()) + } +} + struct PlanInfo { fetch_dependency_graph: FetchDependencyGraph, path_tree: Arc, @@ -284,7 +313,17 @@ impl<'a: 'b, 'b> QueryPlanningTraversal<'a, 'b> { ) )] fn find_best_plan_inner(&mut self) -> Result, FederationError> { - while let Some(mut current_branch) = self.open_branches.pop() { + while !self.open_branches.is_empty() { + snapshot!( + "OpenBranches", + snapshot_helper::open_branches_to_string(&self.open_branches), + "Query planning open branches" + ); + let Some(mut current_branch) = self.open_branches.pop() else { + return Err(FederationError::internal( + "Branch stack unexpectedly empty during query plan traversal", + )); + }; let Some(current_selection) = current_branch.selections.pop() else { return Err(FederationError::internal( "Sub-stack unexpectedly empty during query plan traversal", @@ -293,7 +332,7 @@ impl<'a: 'b, 'b> QueryPlanningTraversal<'a, 'b> { let (terminate_planning, new_branch) = self.handle_open_branch(¤t_selection, &mut current_branch.open_branch.0)?; if terminate_planning { - trace!("Planning termianted!"); + trace!("Planning terminated!"); // We clear both open branches and closed ones as a means to terminate the plan // computation with no plan. self.open_branches = vec![]; @@ -330,12 +369,10 @@ impl<'a: 'b, 'b> QueryPlanningTraversal<'a, 'b> { let mut new_options = vec![]; let mut no_followups: bool = false; - snapshot!(name = "Options", options, "options"); - snapshot!( - "OperationElement", - operation_element.to_string(), - "operation_element" + "OpenBranch", + snapshot_helper::open_branch_to_string(selection, options), + "open branch" ); for option in options.iter_mut() { @@ -368,7 +405,11 @@ impl<'a: 'b, 'b> QueryPlanningTraversal<'a, 'b> { } } - snapshot!(new_options, "new_options"); + snapshot!( + "OpenBranch", + snapshot_helper::open_branch_to_string(selection, &new_options), + "new_options" + ); if no_followups { // This operation element is valid from this option, but is guarantee to yield no result @@ -610,8 +651,8 @@ impl<'a: 'b, 'b> QueryPlanningTraversal<'a, 'b> { )] fn compute_best_plan_from_closed_branches(&mut self) -> Result<(), FederationError> { snapshot!( - name = "ClosedBranches", - self.closed_branches, + "ClosedBranches", + snapshot_helper::closed_branches_to_string(&self.closed_branches), "closed_branches" ); @@ -622,8 +663,8 @@ impl<'a: 'b, 'b> QueryPlanningTraversal<'a, 'b> { self.reduce_options_if_needed(); snapshot!( - name = "ClosedBranches", - self.closed_branches, + "ClosedBranches", + snapshot_helper::closed_branches_to_string(&self.closed_branches), "closed_branches_after_reduce" ); @@ -653,7 +694,7 @@ impl<'a: 'b, 'b> QueryPlanningTraversal<'a, 'b> { let (first_group, second_group) = self.closed_branches.split_at(sole_path_branch_index); let initial_tree; - snapshot!("FetchDependencyGraph", "", "Generating initial dep graph"); + trace!("Generating initial fetch dependency graph"); let mut initial_dependency_graph = self.new_dependency_graph(); let federated_query_graph = &self.parameters.federated_query_graph; let root = &self.parameters.head; @@ -678,21 +719,32 @@ impl<'a: 'b, 'b> QueryPlanningTraversal<'a, 'b> { self.parameters.config.type_conditioned_fetching, )?; snapshot!( - initial_dependency_graph, + "FetchDependencyGraph", + initial_dependency_graph.to_dot(), "Updated dep graph with initial tree" ); if first_group.is_empty() { // Well, we have the only possible plan; it's also the best. let cost = self.cost(&mut initial_dependency_graph)?; - self.best_plan = BestQueryPlanInfo { + let best_plan = BestQueryPlanInfo { fetch_dependency_graph: initial_dependency_graph, path_tree: initial_tree.into(), cost, - } - .into(); + }; - snapshot!(self.best_plan, "best_plan"); + snapshot!( + "FetchDependencyGraph", + best_plan.fetch_dependency_graph.to_dot(), + "best_plan.fetch_dependency_graph" + ); + snapshot!( + "OpPathTree", + best_plan.path_tree.to_string(), + "best_plan.path_tree" + ); + snapshot!(best_plan.cost, "best_plan.cost"); + self.best_plan = best_plan.into(); return Ok(()); } } @@ -723,14 +775,25 @@ impl<'a: 'b, 'b> QueryPlanningTraversal<'a, 'b> { other_trees, /*plan_builder*/ self, )?; - self.best_plan = BestQueryPlanInfo { + let best_plan = BestQueryPlanInfo { fetch_dependency_graph: best.fetch_dependency_graph, path_tree: best.path_tree, cost, - } - .into(); + }; + + snapshot!( + "FetchDependencyGraph", + best_plan.fetch_dependency_graph.to_dot(), + "best_plan.fetch_dependency_graph" + ); + snapshot!( + "OpPathTree", + best_plan.path_tree.to_string(), + "best_plan.path_tree" + ); + snapshot!(best_plan.cost, "best_plan.cost"); - snapshot!(self.best_plan, "best_plan"); + self.best_plan = best_plan.into(); Ok(()) } @@ -975,7 +1038,11 @@ impl<'a: 'b, 'b> QueryPlanningTraversal<'a, 'b> { )?; } - snapshot!(dependency_graph, "updated_dependency_graph"); + snapshot!( + "FetchDependencyGraph", + dependency_graph.to_dot(), + "updated_dependency_graph" + ); Ok(()) } diff --git a/apollo-federation/src/utils/logging.rs b/apollo-federation/src/utils/logging.rs index c7a07c2ef29..0b247c16981 100644 --- a/apollo-federation/src/utils/logging.rs +++ b/apollo-federation/src/utils/logging.rs @@ -1,3 +1,10 @@ +#![allow(dead_code)] + +use crate::operation::Selection; +use crate::query_graph::graph_path::ClosedBranch; +use crate::query_graph::graph_path::SimultaneousPathsWithLazyIndirectPaths; +use crate::query_plan::query_planning_traversal::OpenBranchAndSelections; + /// This macro is a wrapper around `tracing::trace!` and should not be confused with our snapshot /// testing. This primary goal of this macro is to add the necessary context to logging statements /// so that external tools (like the snapshot log visualizer) can show how various key data @@ -32,17 +39,6 @@ macro_rules! snapshot { $msg ); }; - (name = $name:literal, $value:expr, $msg:literal) => { - #[cfg(feature = "snapshot_tracing")] - tracing::trace!( - snapshot = std::any::type_name_of_val(&$value), - data = ron::ser::to_string(&$value).expect(concat!( - "Could not serialize value for a snapshot with message: ", - $msg - )), - $msg - ); - }; ($name:literal, $value:expr, $msg:literal) => { #[cfg(feature = "snapshot_tracing")] tracing::trace!(snapshot = $name, data = $value, $msg); @@ -50,3 +46,76 @@ macro_rules! snapshot { } pub(crate) use snapshot; + +pub(crate) fn make_string( + data: &T, + writer: fn(&mut std::fmt::Formatter<'_>, &T) -> std::fmt::Result, +) -> String { + // One-off struct to implement `Display` for `data` using `writer`. + struct Stringify<'a, T: ?Sized> { + data: &'a T, + writer: fn(&mut std::fmt::Formatter<'_>, &T) -> std::fmt::Result, + } + + impl<'a, T: ?Sized> std::fmt::Display for Stringify<'a, T> { + fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { + (self.writer)(f, self.data) + } + } + + Stringify { data, writer }.to_string() +} + +// PORT_NOTE: This is a (partial) port of `QueryPlanningTraversal.debugStack` JS method. +pub(crate) fn format_open_branch( + f: &mut std::fmt::Formatter<'_>, + (selection, options): &(&Selection, &[SimultaneousPathsWithLazyIndirectPaths]), +) -> std::fmt::Result { + writeln!(f, "{selection}")?; + writeln!(f, " * Options:")?; + for option in *options { + writeln!(f, " - {option}")?; + } + Ok(()) +} + +pub(crate) fn open_branch_to_string( + selection: &Selection, + options: &[SimultaneousPathsWithLazyIndirectPaths], +) -> String { + make_string(&(selection, options), format_open_branch) +} + +// PORT_NOTE: This is a port of `QueryPlanningTraversal.debugStack` JS method. +pub(crate) fn format_open_branches( + f: &mut std::fmt::Formatter<'_>, + open_branches: &[OpenBranchAndSelections], +) -> std::fmt::Result { + // Print from the stack top to the bottom. + for branch in open_branches.iter().rev() { + writeln!(f, "{branch}")?; + } + Ok(()) +} + +pub(crate) fn open_branches_to_string(open_branches: &[OpenBranchAndSelections]) -> String { + make_string(open_branches, format_open_branches) +} + +pub(crate) fn format_closed_branches( + f: &mut std::fmt::Formatter<'_>, + closed_branches: &[ClosedBranch], +) -> std::fmt::Result { + writeln!(f, "All branches:")?; + for (i, closed_branch) in closed_branches.iter().enumerate() { + writeln!(f, "{i}:")?; + for closed_path in &closed_branch.0 { + writeln!(f, " - {closed_path}")?; + } + } + Ok(()) +} + +pub(crate) fn closed_branches_to_string(closed_branches: &[ClosedBranch]) -> String { + make_string(closed_branches, format_closed_branches) +} From 336c42d401611117da503b1c96c5a541648c11eb Mon Sep 17 00:00:00 2001 From: Geoffroy Couprie Date: Tue, 29 Oct 2024 11:48:07 +0100 Subject: [PATCH 04/13] defer integration tests (#6007) --- apollo-router/tests/common.rs | 1 - .../{basic/query1 => core/defer}/README.md | 0 .../query1 => core/defer}/configuration.yaml | 0 .../tests/samples/core/defer/plan.json | 106 +++++++++++++++ .../query2 => core/defer}/supergraph.graphql | 0 .../{basic/query2 => core/query1}/README.md | 0 .../query2 => core/query1}/configuration.yaml | 0 .../samples/{basic => core}/query1/plan.json | 0 .../{basic => core}/query1/supergraph.graphql | 0 .../tests/samples/core/query2/README.md | 3 + .../samples/core/query2/configuration.yaml | 4 + .../samples/{basic => core}/query2/plan.json | 0 .../samples/core/query2/supergraph.graphql | 125 ++++++++++++++++++ .../enterprise/entity-cache/defer/README.md | 3 + .../entity-cache/defer/configuration.yaml | 23 ++++ .../enterprise/entity-cache/defer/plan.json | 113 ++++++++++++++++ .../entity-cache/defer/supergraph.graphql | 122 +++++++++++++++++ apollo-router/tests/samples_tests.rs | 115 ++++++++++++++-- 18 files changed, 605 insertions(+), 10 deletions(-) rename apollo-router/tests/samples/{basic/query1 => core/defer}/README.md (100%) rename apollo-router/tests/samples/{basic/query1 => core/defer}/configuration.yaml (100%) create mode 100644 apollo-router/tests/samples/core/defer/plan.json rename apollo-router/tests/samples/{basic/query2 => core/defer}/supergraph.graphql (100%) rename apollo-router/tests/samples/{basic/query2 => core/query1}/README.md (100%) rename apollo-router/tests/samples/{basic/query2 => core/query1}/configuration.yaml (100%) rename apollo-router/tests/samples/{basic => core}/query1/plan.json (100%) rename apollo-router/tests/samples/{basic => core}/query1/supergraph.graphql (100%) create mode 100644 apollo-router/tests/samples/core/query2/README.md create mode 100644 apollo-router/tests/samples/core/query2/configuration.yaml rename apollo-router/tests/samples/{basic => core}/query2/plan.json (100%) create mode 100644 apollo-router/tests/samples/core/query2/supergraph.graphql create mode 100644 apollo-router/tests/samples/enterprise/entity-cache/defer/README.md create mode 100644 apollo-router/tests/samples/enterprise/entity-cache/defer/configuration.yaml create mode 100644 apollo-router/tests/samples/enterprise/entity-cache/defer/plan.json create mode 100644 apollo-router/tests/samples/enterprise/entity-cache/defer/supergraph.graphql diff --git a/apollo-router/tests/common.rs b/apollo-router/tests/common.rs index 826a377e042..671c462519d 100644 --- a/apollo-router/tests/common.rs +++ b/apollo-router/tests/common.rs @@ -587,7 +587,6 @@ impl IntegrationTest { let mut request = builder.json(&query).build().unwrap(); telemetry.inject_context(&mut request); - request.headers_mut().remove(ACCEPT); match client.execute(request).await { Ok(response) => (span_id, response), Err(err) => { diff --git a/apollo-router/tests/samples/basic/query1/README.md b/apollo-router/tests/samples/core/defer/README.md similarity index 100% rename from apollo-router/tests/samples/basic/query1/README.md rename to apollo-router/tests/samples/core/defer/README.md diff --git a/apollo-router/tests/samples/basic/query1/configuration.yaml b/apollo-router/tests/samples/core/defer/configuration.yaml similarity index 100% rename from apollo-router/tests/samples/basic/query1/configuration.yaml rename to apollo-router/tests/samples/core/defer/configuration.yaml diff --git a/apollo-router/tests/samples/core/defer/plan.json b/apollo-router/tests/samples/core/defer/plan.json new file mode 100644 index 00000000000..b2499400bd5 --- /dev/null +++ b/apollo-router/tests/samples/core/defer/plan.json @@ -0,0 +1,106 @@ +{ + "actions": [ + { + "type": "Start", + "schema_path": "./supergraph.graphql", + "configuration_path": "./configuration.yaml", + "subgraphs": { + "accounts": { + "requests": [ + { + "request": { + "body": { + "query": "{me{__typename name id}}" + } + }, + "response": { + "body": { + "data": { + "me": { + "__typename": "User", + "name": "test", + "id": "1" + } + } + } + } + } + ] + }, + "reviews": { + "requests": [ + { + "request": { + "body": { + "query": "query($representations:[_Any!]!){_entities(representations:$representations){...on User{reviews{body}}}}", + "variables": { + "representations": [ + { + "__typename": "User", + "id": "1" + } + ] + } + } + }, + "response": { + "body": { + "data": { + "_entities": [ + { + "reviews": [ + { + "body": "Test" + } + ] + } + ] + } + } + } + } + ] + } + } + }, + { + "type": "Request", + "headers": { + "Accept": "multipart/mixed;deferSpec=20220824" + }, + "request": { + "query": "{ me { name ... @defer { reviews { body } } } }" + }, + "expected_response": [ + { + "data": { + "me": { + "name": "test" + } + }, + "hasNext": true + }, + { + "hasNext": false, + "incremental": [ + { + "data": { + "reviews": [ + { + "body": "Test" + } + ] + }, + "path": [ + "me" + ] + } + ] + } + ] + }, + { + "type": "Stop" + } + ] +} \ No newline at end of file diff --git a/apollo-router/tests/samples/basic/query2/supergraph.graphql b/apollo-router/tests/samples/core/defer/supergraph.graphql similarity index 100% rename from apollo-router/tests/samples/basic/query2/supergraph.graphql rename to apollo-router/tests/samples/core/defer/supergraph.graphql diff --git a/apollo-router/tests/samples/basic/query2/README.md b/apollo-router/tests/samples/core/query1/README.md similarity index 100% rename from apollo-router/tests/samples/basic/query2/README.md rename to apollo-router/tests/samples/core/query1/README.md diff --git a/apollo-router/tests/samples/basic/query2/configuration.yaml b/apollo-router/tests/samples/core/query1/configuration.yaml similarity index 100% rename from apollo-router/tests/samples/basic/query2/configuration.yaml rename to apollo-router/tests/samples/core/query1/configuration.yaml diff --git a/apollo-router/tests/samples/basic/query1/plan.json b/apollo-router/tests/samples/core/query1/plan.json similarity index 100% rename from apollo-router/tests/samples/basic/query1/plan.json rename to apollo-router/tests/samples/core/query1/plan.json diff --git a/apollo-router/tests/samples/basic/query1/supergraph.graphql b/apollo-router/tests/samples/core/query1/supergraph.graphql similarity index 100% rename from apollo-router/tests/samples/basic/query1/supergraph.graphql rename to apollo-router/tests/samples/core/query1/supergraph.graphql diff --git a/apollo-router/tests/samples/core/query2/README.md b/apollo-router/tests/samples/core/query2/README.md new file mode 100644 index 00000000000..9386489fb0f --- /dev/null +++ b/apollo-router/tests/samples/core/query2/README.md @@ -0,0 +1,3 @@ +This is an example test + +This file adds some context that will be displayed on test failure \ No newline at end of file diff --git a/apollo-router/tests/samples/core/query2/configuration.yaml b/apollo-router/tests/samples/core/query2/configuration.yaml new file mode 100644 index 00000000000..f7ed04641ee --- /dev/null +++ b/apollo-router/tests/samples/core/query2/configuration.yaml @@ -0,0 +1,4 @@ +override_subgraph_url: + products: http://localhost:4005 +include_subgraph_errors: + all: true diff --git a/apollo-router/tests/samples/basic/query2/plan.json b/apollo-router/tests/samples/core/query2/plan.json similarity index 100% rename from apollo-router/tests/samples/basic/query2/plan.json rename to apollo-router/tests/samples/core/query2/plan.json diff --git a/apollo-router/tests/samples/core/query2/supergraph.graphql b/apollo-router/tests/samples/core/query2/supergraph.graphql new file mode 100644 index 00000000000..1bd9f596ee2 --- /dev/null +++ b/apollo-router/tests/samples/core/query2/supergraph.graphql @@ -0,0 +1,125 @@ +schema + @link(url: "https://specs.apollo.dev/link/v1.0") + @link(url: "https://specs.apollo.dev/inaccessible/v0.2", for: SECURITY) + @link(url: "https://specs.apollo.dev/join/v0.3", for: EXECUTION) { + query: Query + mutation: Mutation +} + +directive @inaccessible on FIELD_DEFINITION | OBJECT | INTERFACE | UNION | ARGUMENT_DEFINITION | SCALAR | ENUM | ENUM_VALUE | INPUT_OBJECT | INPUT_FIELD_DEFINITION + +directive @tag( + name: String! +) repeatable on FIELD_DEFINITION | OBJECT | INTERFACE | UNION | ARGUMENT_DEFINITION | SCALAR | ENUM | ENUM_VALUE | INPUT_OBJECT | INPUT_FIELD_DEFINITION | SCHEMA + +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 +) 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 + +scalar join__FieldSet + +enum join__Graph { + ACCOUNTS + @join__graph(name: "accounts", url: "https://accounts.demo.starstuff.dev/") + INVENTORY + @join__graph( + name: "inventory" + url: "https://inventory.demo.starstuff.dev/" + ) + PRODUCTS + @join__graph(name: "products", url: "https://products.demo.starstuff.dev/") + REVIEWS + @join__graph(name: "reviews", url: "https://reviews.demo.starstuff.dev/") +} + +scalar link__Import + +enum link__Purpose { + SECURITY + EXECUTION +} + +type Mutation @join__type(graph: PRODUCTS) @join__type(graph: REVIEWS) { + createProduct(upc: ID!, name: String): Product @join__field(graph: PRODUCTS) + createReview(upc: ID!, id: ID!, body: String): Review + @join__field(graph: REVIEWS) +} + +type Product + @join__type(graph: INVENTORY, key: "upc") + @join__type(graph: PRODUCTS, key: "upc") + @join__type(graph: REVIEWS, key: "upc") { + inStock: Boolean + @join__field(graph: INVENTORY) + @tag(name: "private") + @inaccessible + name: String @join__field(graph: PRODUCTS) + weight: Int @join__field(graph: INVENTORY, external: true) @join__field(graph: PRODUCTS) + price: Int @join__field(graph: INVENTORY, external: true) @join__field(graph: PRODUCTS) + reviews: [Review] @join__field(graph: REVIEWS) + reviewsForAuthor(authorID: ID!): [Review] @join__field(graph: REVIEWS) + shippingEstimate: Int @join__field(graph: INVENTORY, requires: "price weight") + upc: String! +} + +type Query + @join__type(graph: ACCOUNTS) + @join__type(graph: INVENTORY) + @join__type(graph: PRODUCTS) + @join__type(graph: REVIEWS) { + me: User @join__field(graph: ACCOUNTS) + topProducts(first: Int = 5): [Product] @join__field(graph: PRODUCTS) +} + +type Review @join__type(graph: REVIEWS, key: "id") { + id: ID! + body: String @join__field(graph: REVIEWS) + author: User @join__field(graph: REVIEWS, provides: "username") + product: Product @join__field(graph: REVIEWS) +} + +type User + @join__type(graph: ACCOUNTS, key: "id") + @join__type(graph: REVIEWS, key: "id") { + id: ID! + name: String @join__field(graph: ACCOUNTS) + username: String + @join__field(graph: ACCOUNTS) + @join__field(graph: REVIEWS, external: true) + reviews: [Review] @join__field(graph: REVIEWS) +} diff --git a/apollo-router/tests/samples/enterprise/entity-cache/defer/README.md b/apollo-router/tests/samples/enterprise/entity-cache/defer/README.md new file mode 100644 index 00000000000..a96d350b73a --- /dev/null +++ b/apollo-router/tests/samples/enterprise/entity-cache/defer/README.md @@ -0,0 +1,3 @@ +# Entity cache with @defer + +This tests `Cache-Control` aggregation when using the `@defer` directive. \ No newline at end of file diff --git a/apollo-router/tests/samples/enterprise/entity-cache/defer/configuration.yaml b/apollo-router/tests/samples/enterprise/entity-cache/defer/configuration.yaml new file mode 100644 index 00000000000..fb6b95ecd49 --- /dev/null +++ b/apollo-router/tests/samples/enterprise/entity-cache/defer/configuration.yaml @@ -0,0 +1,23 @@ +override_subgraph_url: + products: http://localhost:4005 +include_subgraph_errors: + all: true + +preview_entity_cache: + enabled: true + redis: + urls: + ["redis://localhost:6379",] + subgraph: + all: + enabled: true + subgraphs: + reviews: + ttl: 120s + enabled: true + +telemetry: + exporters: + logging: + stdout: + format: text \ No newline at end of file diff --git a/apollo-router/tests/samples/enterprise/entity-cache/defer/plan.json b/apollo-router/tests/samples/enterprise/entity-cache/defer/plan.json new file mode 100644 index 00000000000..265e282056f --- /dev/null +++ b/apollo-router/tests/samples/enterprise/entity-cache/defer/plan.json @@ -0,0 +1,113 @@ +{ + "enterprise": true, + "redis": true, + "actions": [ + { + "type": "Start", + "schema_path": "./supergraph.graphql", + "configuration_path": "./configuration.yaml", + "subgraphs": { + "cache-defer-accounts": { + "requests": [ + { + "request": { + "body": { + "query": "query CacheDefer__cache_defer_accounts__0{me{__typename name id}}", + "operationName": "CacheDefer__cache_defer_accounts__0" + } + }, + "response": { + "headers": { + "Cache-Control": "public, max-age=10", + "Content-Type": "application/json" + }, + "body": { + "data": { + "me": { + "__typename": "User", + "name": "test-user", + "id": "1" + } + } + } + } + } + ] + }, + "cache-defer-reviews": { + "requests": [ + { + "request": { + "body": { + "query": "query CacheDefer__cache_defer_reviews__1($representations:[_Any!]!){_entities(representations:$representations){...on User{reviews{body}}}}", + "operationName": "CacheDefer__cache_defer_reviews__1", + "variables": { + "representations": [ + { + "id": "1", + "__typename": "User" + } + ] + } + } + }, + "response": { + "headers": { + "Cache-Control": "public, max-age=100", + "Content-Type": "application/json" + }, + "body": { + "data": { + "reviews": [ + { + "body": "test-review" + } + ] + } + } + } + } + ] + } + } + }, + { + "type": "Request", + "request": { + "query": "query CacheDefer { me { name ... @defer { reviews { body } } } }" + }, + "headers": { + "Accept": "multipart/mixed;deferSpec=20220824" + }, + "expected_response": [ + { + "data": { + "me": { + "name": "test-user" + } + }, + "hasNext": true + }, + { + "hasNext": false, + "incremental": [ + { + "data": { + "reviews": null + }, + "path": [ + "me" + ] + } + ] + } + ], + "expected_headers": { + "Cache-Control": "max-age=10,public" + } + }, + { + "type": "Stop" + } + ] +} \ No newline at end of file diff --git a/apollo-router/tests/samples/enterprise/entity-cache/defer/supergraph.graphql b/apollo-router/tests/samples/enterprise/entity-cache/defer/supergraph.graphql new file mode 100644 index 00000000000..320a9c2a700 --- /dev/null +++ b/apollo-router/tests/samples/enterprise/entity-cache/defer/supergraph.graphql @@ -0,0 +1,122 @@ +schema + @link(url: "https://specs.apollo.dev/link/v1.0") + @link(url: "https://specs.apollo.dev/tag/v0.3") + @link(url: "https://specs.apollo.dev/inaccessible/v0.2") + @link(url: "https://specs.apollo.dev/join/v0.3", for: EXECUTION) { + query: Query + mutation: Mutation +} + +directive @inaccessible on FIELD_DEFINITION | OBJECT | INTERFACE | UNION | ARGUMENT_DEFINITION | SCALAR | ENUM | ENUM_VALUE | INPUT_OBJECT | INPUT_FIELD_DEFINITION +directive @tag( + name: String! +) repeatable on FIELD_DEFINITION | INTERFACE | OBJECT | UNION + +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 +) 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 + +scalar join__FieldSet +scalar link__Import + +enum join__Graph { + ACCOUNTS @join__graph(name: "cache-defer-accounts", url: "https://accounts.demo.starstuff.dev") + INVENTORY @join__graph(name: "inventory", url: "https://inventory.demo.starstuff.dev") + PRODUCTS @join__graph(name: "products", url: "https://products.demo.starstuff.dev") + REVIEWS @join__graph(name: "cache-defer-reviews", url: "https://reviews.demo.starstuff.dev") +} + +enum link__Purpose { + SECURITY + EXECUTION +} + +type Mutation + @join__type(graph: PRODUCTS) + @join__type(graph: REVIEWS) + @join__type(graph: ACCOUNTS) { + updateMyAccount: User @join__field(graph: ACCOUNTS) + createProduct(name: String, upc: ID!): Product @join__field(graph: PRODUCTS) + createReview(body: String, id: ID!, upc: ID!): Review + @join__field(graph: REVIEWS) +} + +type Product + @join__type(graph: ACCOUNTS, key: "upc", extension: true) + @join__type(graph: INVENTORY, key: "upc") + @join__type(graph: PRODUCTS, key: "upc") + @join__type(graph: REVIEWS, key: "upc") { + inStock: Boolean + @join__field(graph: INVENTORY) + @tag(name: "private") + @inaccessible + name: String @join__field(graph: PRODUCTS) + weight: Int @join__field(graph: INVENTORY, external: true) @join__field(graph: PRODUCTS) + price: Int @join__field(graph: INVENTORY, external: true) @join__field(graph: PRODUCTS) + reviews: [Review] @join__field(graph: REVIEWS) + reviewsForAuthor(authorID: ID!): [Review] @join__field(graph: REVIEWS) + shippingEstimate: Int @join__field(graph: INVENTORY, requires: "price weight") + upc: String! +} + +type Query + @join__type(graph: ACCOUNTS) + @join__type(graph: INVENTORY) + @join__type(graph: PRODUCTS) + @join__type(graph: REVIEWS) { + me: User @join__field(graph: ACCOUNTS) + topProducts(first: Int = 5): [Product] @join__field(graph: PRODUCTS) +} + +type Review @join__type(graph: REVIEWS, key: "id") { + id: ID! + author: User @join__field(graph: REVIEWS, provides: "username") + body: String + product: Product +} + +type User + @join__type(graph: ACCOUNTS, key: "id") + @join__type(graph: REVIEWS, key: "id") { + id: ID! + name: String @join__field(graph: ACCOUNTS) + username: String + @join__field(graph: ACCOUNTS) + @join__field(graph: REVIEWS, external: true) + reviews: [Review] @join__field(graph: REVIEWS) +} diff --git a/apollo-router/tests/samples_tests.rs b/apollo-router/tests/samples_tests.rs index 22e3b31e186..4686bd644c1 100644 --- a/apollo-router/tests/samples_tests.rs +++ b/apollo-router/tests/samples_tests.rs @@ -14,6 +14,9 @@ use std::process::ExitCode; use libtest_mimic::Arguments; use libtest_mimic::Failed; use libtest_mimic::Trial; +use mediatype::MediaTypeList; +use mediatype::ReadParams; +use multer::Multipart; use serde::Deserialize; use serde_json::Value; use tokio::runtime::Runtime; @@ -173,12 +176,14 @@ impl TestExecution { query_path, headers, expected_response, + expected_headers, } => { self.request( request.clone(), query_path.as_deref(), headers, expected_response, + expected_headers, path, out, ) @@ -405,12 +410,14 @@ impl TestExecution { } } + #[allow(clippy::too_many_arguments)] async fn request( &mut self, mut request: Value, query_path: Option<&str>, headers: &HashMap, expected_response: &Value, + expected_headers: &HashMap, path: &Path, out: &mut String, ) -> Result<(), Failed> { @@ -434,19 +441,107 @@ impl TestExecution { } writeln!(out, "query: {}\n", serde_json::to_string(&request).unwrap()).unwrap(); + writeln!(out, "header: {:?}\n", headers).unwrap(); + let (_, response) = router .execute_query_with_headers(&request, headers.clone()) .await; - let body = response.bytes().await.map_err(|e| { - writeln!(out, "could not get graphql response data: {e}").unwrap(); - let f: Failed = out.clone().into(); - f - })?; - let graphql_response: Value = serde_json::from_slice(&body).map_err(|e| { - writeln!(out, "could not deserialize graphql response data: {e}").unwrap(); + writeln!(out, "response headers: {:?}", response.headers()).unwrap(); + + let mut failed = false; + for (key, value) in expected_headers { + if !response.headers().contains_key(key) { + failed = true; + writeln!(out, "expected header {} to be present", key).unwrap(); + } else if response.headers().get(key).unwrap() != value { + failed = true; + writeln!( + out, + "expected header {} to be {}, got {:?}", + key, + value, + response.headers().get(key).unwrap() + ) + .unwrap(); + } + } + if failed { let f: Failed = out.clone().into(); - f - })?; + return Err(f); + } + + let content_type = response + .headers() + .get("content-type") + .unwrap() + .to_str() + .unwrap(); + let mut is_multipart = false; + let mut boundary = None; + for mime in MediaTypeList::new(content_type).flatten() { + if mime.ty == mediatype::names::MULTIPART && mime.subty == mediatype::names::MIXED { + is_multipart = true; + boundary = mime.get_param(mediatype::names::BOUNDARY).map(|v| { + // multer does not strip quotes from the boundary: https://github.com/rwf2/multer/issues/64 + let mut s = v.as_str(); + if s.starts_with('\"') && s.ends_with('\"') { + s = &s[1..s.len() - 1]; + } + + s.to_string() + }); + } + } + + let graphql_response: Value = if !is_multipart { + let body = response.bytes().await.map_err(|e| { + writeln!(out, "could not get graphql response data: {e}").unwrap(); + let f: Failed = out.clone().into(); + f + })?; + serde_json::from_slice(&body).map_err(|e| { + writeln!( + out, + "could not deserialize graphql response data: {e}\nfrom:\n{}", + std::str::from_utf8(&body).unwrap() + ) + .unwrap(); + let f: Failed = out.clone().into(); + f + })? + } else { + let mut chunks = Vec::new(); + + let mut multipart = Multipart::new(response.bytes_stream(), boundary.unwrap()); + + // Iterate over the fields, use `next_field()` to get the next field. + while let Some(mut field) = multipart.next_field().await.map_err(|e| { + writeln!(out, "could not get next field from multipart body: {e}",).unwrap(); + let f: Failed = out.clone().into(); + f + })? { + while let Some(chunk) = field.chunk().await.map_err(|e| { + writeln!(out, "could not get next chunk from multipart body: {e}",).unwrap(); + let f: Failed = out.clone().into(); + f + })? { + writeln!(out, "multipart chunk: {:?}\n", std::str::from_utf8(&chunk)).unwrap(); + + let parsed: Value = serde_json::from_slice(&chunk).map_err(|e| { + writeln!( + out, + "could not deserialize graphql response data: {e}\nfrom:\n{}", + std::str::from_utf8(&chunk).unwrap() + ) + .unwrap(); + let f: Failed = out.clone().into(); + f + })?; + chunks.push(parsed); + } + } + Value::Array(chunks) + }; if expected_response != &graphql_response { if let Some(requests) = self @@ -583,6 +678,8 @@ enum Action { #[serde(default)] headers: HashMap, expected_response: Value, + #[serde(default)] + expected_headers: HashMap, }, EndpointRequest { url: url::Url, From 508ed0f10f2386abc51d000ccf70361a7ad2a8a0 Mon Sep 17 00:00:00 2001 From: Geoffroy Couprie Date: Tue, 29 Oct 2024 11:48:40 +0100 Subject: [PATCH 05/13] test interfaceObject (#6072) --- .../samples/basic/interface-object/README.md | 4 + .../basic/interface-object/configuration.yaml | 7 + .../samples/basic/interface-object/plan.json | 254 ++++++++++++++++++ .../basic/interface-object/supergraph.graphql | 115 ++++++++ 4 files changed, 380 insertions(+) create mode 100644 apollo-router/tests/samples/basic/interface-object/README.md create mode 100644 apollo-router/tests/samples/basic/interface-object/configuration.yaml create mode 100644 apollo-router/tests/samples/basic/interface-object/plan.json create mode 100644 apollo-router/tests/samples/basic/interface-object/supergraph.graphql diff --git a/apollo-router/tests/samples/basic/interface-object/README.md b/apollo-router/tests/samples/basic/interface-object/README.md new file mode 100644 index 00000000000..a2e1d7fbb1a --- /dev/null +++ b/apollo-router/tests/samples/basic/interface-object/README.md @@ -0,0 +1,4 @@ + +# @interfaceObject + +Test the [`@interfaceObject`](https://www.apollographql.com/docs/federation/entities/interfaces/) directive. \ No newline at end of file diff --git a/apollo-router/tests/samples/basic/interface-object/configuration.yaml b/apollo-router/tests/samples/basic/interface-object/configuration.yaml new file mode 100644 index 00000000000..d5c60afffae --- /dev/null +++ b/apollo-router/tests/samples/basic/interface-object/configuration.yaml @@ -0,0 +1,7 @@ +override_subgraph_url: + products: http://localhost:4005 +include_subgraph_errors: + all: true + +plugins: + experimental.expose_query_plan: true \ No newline at end of file diff --git a/apollo-router/tests/samples/basic/interface-object/plan.json b/apollo-router/tests/samples/basic/interface-object/plan.json new file mode 100644 index 00000000000..91a5690a0c5 --- /dev/null +++ b/apollo-router/tests/samples/basic/interface-object/plan.json @@ -0,0 +1,254 @@ +{ + "actions": [ + { + "type": "Start", + "schema_path": "./supergraph.graphql", + "configuration_path": "./configuration.yaml", + "subgraphs": { + "accounts": { + "requests": [ + { + "request": { + "body": { + "query": "query TestItf__accounts__0{i{__typename id x ...on A{a}...on B{b}}}", + "operationName": "TestItf__accounts__0" + } + }, + "response": { + "body": { + "data": { + "i": [ + { + "__typename": "A", + "id": "1", + "x": 1, + "a": "a" + }, + null, + { + "__typename": "B", + "id": "2", + "x": 2, + "b": "b" + }, + { + "__typename": "A", + "id": "1", + "x": 1, + "a": "a" + }, + { + "__typename": "B", + "id": "3", + "x": 3, + "b": "c" + } + ] + } + } + } + } + ] + }, + "products": { + "requests": [ + { + "request": { + "body": { + "query": "query TestItf__products__1($representations:[_Any!]!){_entities(representations:$representations){...on I{y}}}", + "operationName": "TestItf__products__1", + "variables": { + "representations": [ + { + "__typename": "I", + "id": "1" + }, + { + "__typename": "I", + "id": "2" + }, + { + "__typename": "I", + "id": "3" + } + ] + } + } + }, + "response": { + "body": { + "data": { + "_entities": [ + { + "y": 1 + }, + { + "y": 2 + }, + null + ] + } + } + } + } + ] + }, + "reviews": { + "requests": [] + } + } + }, + { + "type": "Request", + "request": { + "query": "query TestItf { i { __typename x y ... on A { a } ... on B { b } } }" + }, + "expected_response": { + "data": { + "i": [ + { + "__typename": "A", + "x": 1, + "y": 1, + "a": "a" + }, + null, + { + "__typename": "B", + "x": 2, + "y": 2, + "b": "b" + }, + { + "__typename": "A", + "x": 1, + "y": 1, + "a": "a" + }, + { + "__typename": "B", + "x": 3, + "y": null, + "b": "c" + } + ] + } + } + }, + { + "type": "ReloadSubgraphs", + "subgraphs": { + "accounts": { + "requests": [ + { + "request": { + "body": { + "query": "query TestItf2__accounts__0{req{__typename id i{__typename id x}}}", + "operationName": "TestItf2__accounts__0" + } + }, + "response": { + "body": { + "data": { + "req": { + "__typename": "C", + "id": "1", + "i": { + "__typename": "A", + "id": "1", + "x": 1 + } + } + } + } + } + } + ] + }, + "products": { + "requests": [ + { + "request": { + "body": { + "query": "query TestItf2__products__1($representations:[_Any!]!){_entities(representations:$representations){...on I{y}}}", + "operationName": "TestItf2__products__1", + "variables": { + "representations": [ + { + "__typename": "I", + "id": "1" + } + ] + } + } + }, + "response": { + "body": { + "data": { + "_entities": [ + { + "y": 1 + } + ] + } + } + } + } + ] + }, + "reviews": { + "requests": [ + { + "request": { + "body": { + "query": "query TestItf2__reviews__2($representations:[_Any!]!){_entities(representations:$representations){...on C{c}}}", + "operationName": "TestItf2__reviews__2", + "variables": { + "representations": [ + { + "__typename": "C", + "i": { + "x": 1, + "y": 1 + }, + "id": "1" + } + ] + } + } + }, + "response": { + "body": { + "data": { + "_entities": [ + { + "c": "c" + } + ] + } + } + } + } + ] + } + } + }, + { + "type": "Request", + "request": { + "query": "query TestItf2 { req { id c } }" + }, + "expected_response": { + "data": { + "req": { + "id": "1", + "c": "c" + } + } + } + }, + { + "type": "Stop" + } + ] +} \ No newline at end of file diff --git a/apollo-router/tests/samples/basic/interface-object/supergraph.graphql b/apollo-router/tests/samples/basic/interface-object/supergraph.graphql new file mode 100644 index 00000000000..08783bf70f9 --- /dev/null +++ b/apollo-router/tests/samples/basic/interface-object/supergraph.graphql @@ -0,0 +1,115 @@ +schema + @link(url: "https://specs.apollo.dev/link/v1.0") + @link(url: "https://specs.apollo.dev/inaccessible/v0.2", for: SECURITY) + @link(url: "https://specs.apollo.dev/join/v0.3", for: EXECUTION) { + query: Query +} + +directive @inaccessible on FIELD_DEFINITION | OBJECT | INTERFACE | UNION | ARGUMENT_DEFINITION | SCALAR | ENUM | ENUM_VALUE | INPUT_OBJECT | INPUT_FIELD_DEFINITION + +directive @tag( + name: String! +) repeatable on FIELD_DEFINITION | OBJECT | INTERFACE | UNION | ARGUMENT_DEFINITION | SCALAR | ENUM | ENUM_VALUE | INPUT_OBJECT | INPUT_FIELD_DEFINITION | SCHEMA + +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 +) 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 + +scalar join__FieldSet +scalar link__Import + +enum join__Graph { + ACCOUNTS + @join__graph(name: "accounts", url: "https://accounts.demo.starstuff.dev/") + INVENTORY + @join__graph( + name: "inventory" + url: "https://inventory.demo.starstuff.dev/" + ) + PRODUCTS + @join__graph(name: "products", url: "https://products.demo.starstuff.dev/") + REVIEWS + @join__graph(name: "reviews", url: "https://reviews.demo.starstuff.dev/") +} + +enum link__Purpose { + SECURITY + EXECUTION +} + +type Query + @join__type(graph: ACCOUNTS) + @join__type(graph: REVIEWS) { + i: [I] @join__field(graph: ACCOUNTS) + req: C @join__field(graph: ACCOUNTS) +} + +interface I + @join__type(graph: ACCOUNTS, key: "id") + @join__type(graph: PRODUCTS, key: "id", isInterfaceObject: true) + @join__type(graph: REVIEWS) { + id: ID! + x: Int @join__field(graph: ACCOUNTS) @join__field(graph: REVIEWS) + y: Int @join__field(graph: PRODUCTS) @join__field(graph: REVIEWS) +} + +type A implements I + @join__type(graph: ACCOUNTS, key: "id") + @join__implements(graph: ACCOUNTS, interface: "I") { + id: ID! + x: Int + y: Int @join__field + a: String +} + +type B implements I + @join__type(graph: ACCOUNTS, key: "id") + @join__implements(graph: ACCOUNTS, interface: "I") { + id: ID! + x: Int + y: Int @join__field + b: String +} + +type C + @join__type(graph: ACCOUNTS, key: "id") + @join__type(graph: REVIEWS, key: "id") { + id: ID! + i: I @join__field(graph: ACCOUNTS) @join__field(graph: REVIEWS, external: true) + c: String @join__field(graph: REVIEWS, requires: "i { x y }") +} \ No newline at end of file From 83f57725483f0b195e43f47add0e0cc8f73246ca Mon Sep 17 00:00:00 2001 From: Geoffroy Couprie Date: Tue, 29 Oct 2024 11:49:01 +0100 Subject: [PATCH 06/13] update entity cache samples config (#6044) --- .../entity-cache/invalidation-entity-key/configuration.yaml | 6 +++--- .../invalidation-subgraph-name/configuration.yaml | 6 +++--- .../invalidation-subgraph-type/configuration.yaml | 6 +++--- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/apollo-router/tests/samples/enterprise/entity-cache/invalidation-entity-key/configuration.yaml b/apollo-router/tests/samples/enterprise/entity-cache/invalidation-entity-key/configuration.yaml index e283bbdace8..47f1e99e063 100644 --- a/apollo-router/tests/samples/enterprise/entity-cache/invalidation-entity-key/configuration.yaml +++ b/apollo-router/tests/samples/enterprise/entity-cache/invalidation-entity-key/configuration.yaml @@ -5,12 +5,12 @@ include_subgraph_errors: preview_entity_cache: enabled: true - redis: - urls: - ["redis://localhost:6379",] subgraph: all: enabled: true + redis: + urls: + ["redis://localhost:6379",] subgraphs: invalidation-entity-key-reviews: ttl: 120s diff --git a/apollo-router/tests/samples/enterprise/entity-cache/invalidation-subgraph-name/configuration.yaml b/apollo-router/tests/samples/enterprise/entity-cache/invalidation-subgraph-name/configuration.yaml index 85e106df9fc..7efcac9a817 100644 --- a/apollo-router/tests/samples/enterprise/entity-cache/invalidation-subgraph-name/configuration.yaml +++ b/apollo-router/tests/samples/enterprise/entity-cache/invalidation-subgraph-name/configuration.yaml @@ -8,12 +8,12 @@ preview_entity_cache: invalidation: listen: 127.0.0.1:4000 path: /invalidation - redis: - urls: - ["redis://localhost:6379",] subgraph: all: enabled: true + redis: + urls: + ["redis://localhost:6379",] subgraphs: reviews: ttl: 120s diff --git a/apollo-router/tests/samples/enterprise/entity-cache/invalidation-subgraph-type/configuration.yaml b/apollo-router/tests/samples/enterprise/entity-cache/invalidation-subgraph-type/configuration.yaml index 96577bbb287..8378e3b1272 100644 --- a/apollo-router/tests/samples/enterprise/entity-cache/invalidation-subgraph-type/configuration.yaml +++ b/apollo-router/tests/samples/enterprise/entity-cache/invalidation-subgraph-type/configuration.yaml @@ -5,9 +5,6 @@ include_subgraph_errors: preview_entity_cache: enabled: true - redis: - urls: - ["redis://localhost:6379",] invalidation: # FIXME: right now we cannot configure it to use the same port used for the GraphQL endpoint if it is chosen at random listen: 127.0.0.1:12345 @@ -15,6 +12,9 @@ preview_entity_cache: subgraph: all: enabled: true + redis: + urls: + ["redis://localhost:6379",] invalidation: enabled: true shared_key: "1234" From 39c6c03ed9f15c3872bcad083e1560ddbb1b6d3e Mon Sep 17 00:00:00 2001 From: Geoffroy Couprie Date: Tue, 29 Oct 2024 11:59:47 +0100 Subject: [PATCH 07/13] add errors for response validation (#5787) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Andrew McGivery Co-authored-by: RenĂ©e Kooi --- .../fix_geal_response_validation_errors.md | 5 + apollo-router/src/json_ext.rs | 14 + apollo-router/src/spec/query.rs | 192 +++++++- apollo-router/src/spec/query/tests.rs | 452 ++++++++++++++++++ ...__set_context_unrelated_fetch_failure.snap | 2 +- docs/source/errors.mdx | 9 + 6 files changed, 658 insertions(+), 16 deletions(-) create mode 100644 .changesets/fix_geal_response_validation_errors.md diff --git a/.changesets/fix_geal_response_validation_errors.md b/.changesets/fix_geal_response_validation_errors.md new file mode 100644 index 00000000000..95fc3d60eb4 --- /dev/null +++ b/.changesets/fix_geal_response_validation_errors.md @@ -0,0 +1,5 @@ +### add errors for response validation ([Issue #5372](https://github.com/apollographql/router/issues/5372)) + +When formatting responses, the router is validating the data returned by subgraphs and replacing it with null values as appropriate. That validation phase is now adding errors when encountering the wrong type in a field requested by the client. + +By [@Geal](https://github.com/Geal) in https://github.com/apollographql/router/pull/5787 \ No newline at end of file diff --git a/apollo-router/src/json_ext.rs b/apollo-router/src/json_ext.rs index c9e617635ff..54d1dd0bfe6 100644 --- a/apollo-router/src/json_ext.rs +++ b/apollo-router/src/json_ext.rs @@ -146,6 +146,9 @@ pub(crate) trait ValueExt { #[track_caller] fn is_object_of_type(&self, schema: &Schema, maybe_type: &str) -> bool; + /// value type + fn json_type_name(&self) -> &'static str; + /// Convert this value to an instance of `apollo_compiler::ast::Value` fn to_ast(&self) -> apollo_compiler::ast::Value; @@ -475,6 +478,17 @@ impl ValueExt for Value { }) } + fn json_type_name(&self) -> &'static str { + match self { + Value::Array(_) => "array", + Value::Null => "null", + Value::Bool(_) => "boolean", + Value::Number(_) => "number", + Value::String(_) => "string", + Value::Object(_) => "object", + } + } + fn to_ast(&self) -> apollo_compiler::ast::Value { match self { Value::Null => apollo_compiler::ast::Value::Null, diff --git a/apollo-router/src/spec/query.rs b/apollo-router/src/spec/query.rs index 3ad855870a9..c8bcddec6dd 100644 --- a/apollo-router/src/spec/query.rs +++ b/apollo-router/src/spec/query.rs @@ -30,6 +30,7 @@ use crate::json_ext::Object; use crate::json_ext::Path; use crate::json_ext::ResponsePathElement; use crate::json_ext::Value; +use crate::json_ext::ValueExt; use crate::plugins::authorization::UnauthorizedPaths; use crate::query_planner::fetch::OperationKind; use crate::query_planner::fetch::QueryHash; @@ -51,6 +52,7 @@ pub(crate) mod transform; pub(crate) mod traverse; pub(crate) const TYPENAME: &str = "__typename"; +pub(crate) const RESPONSE_VALIDATION: &str = "RESPONSE_VALIDATION_FAILED"; /// A GraphQL query. #[derive(Derivative, Serialize, Deserialize)] @@ -143,8 +145,9 @@ impl Query { let mut parameters = FormatParameters { variables: &variables, schema, - errors: Vec::new(), + nullification_errors: Vec::new(), nullified: Vec::new(), + validation_errors: Vec::new(), }; response.data = Some( @@ -161,12 +164,18 @@ impl Query { }, ); - if !parameters.errors.is_empty() { - if let Ok(value) = serde_json_bytes::to_value(¶meters.errors) { + if !parameters.nullification_errors.is_empty() { + if let Ok(value) = + serde_json_bytes::to_value(¶meters.nullification_errors) + { response.extensions.insert("valueCompletion", value); } } + if !parameters.validation_errors.is_empty() { + response.errors.append(&mut parameters.validation_errors); + } + return parameters.nullified; } None => { @@ -198,8 +207,9 @@ impl Query { let mut parameters = FormatParameters { variables: &all_variables, schema, - errors: Vec::new(), + nullification_errors: Vec::new(), nullified: Vec::new(), + validation_errors: Vec::new(), }; response.data = Some( @@ -215,12 +225,18 @@ impl Query { Err(InvalidValue) => Value::Null, }, ); - if !parameters.errors.is_empty() { - if let Ok(value) = serde_json_bytes::to_value(¶meters.errors) { + if !parameters.nullification_errors.is_empty() { + if let Ok(value) = + serde_json_bytes::to_value(¶meters.nullification_errors) + { response.extensions.insert("valueCompletion", value); } } + if !parameters.validation_errors.is_empty() { + response.errors.append(&mut parameters.validation_errors); + } + return parameters.nullified; } } @@ -342,6 +358,7 @@ impl Query { output: &mut Value, path: &mut Vec>, parent_type: &executable::Type, + field_or_index: FieldOrIndex<'a>, selection_set: &'a [Selection], ) -> Result<(), InvalidValue> { // for every type, if we have an invalid value, we will replace it with null @@ -362,7 +379,8 @@ impl Query { input, output, path, - field_type, + parent_type, + field_or_index, selection_set, ) { Err(_) => Err(InvalidValue), @@ -377,7 +395,7 @@ impl Query { ), _ => todo!(), }; - parameters.errors.push(Error { + parameters.nullification_errors.push(Error { message, path: Some(Path::from_response_slice(path)), ..Error::default() @@ -417,6 +435,7 @@ impl Query { &mut output_array[i], path, field_type, + FieldOrIndex::Index(i), selection_set, ); path.pop(); @@ -430,7 +449,22 @@ impl Query { Ok(()) => Ok(()), } } - _ => Ok(()), + Value::Null => Ok(()), + v => { + parameters.validation_errors.push( + Error::builder() + .message(format!( + "Invalid non-list value of type {} for list type {field_type}", + v.json_type_name() + )) + .path(Path::from_response_slice(path)) + .extension_code(RESPONSE_VALIDATION) + .build(), + ); + + *output = Value::Null; + Ok(()) + } }, executable::Type::Named(name) if name == "Int" => { let opt = if input.is_i64() { @@ -446,6 +480,19 @@ impl Query { if opt.is_some() { *output = input.clone(); } else { + if !input.is_null() { + parameters.validation_errors.push( + Error::builder() + .message(invalid_value_message( + parent_type, + field_type, + field_or_index, + )) + .path(Path::from_response_slice(path)) + .extension_code(RESPONSE_VALIDATION) + .build(), + ); + } *output = Value::Null; } Ok(()) @@ -454,6 +501,19 @@ impl Query { if input.as_f64().is_some() { *output = input.clone(); } else { + if !input.is_null() { + parameters.validation_errors.push( + Error::builder() + .message(invalid_value_message( + parent_type, + field_type, + field_or_index, + )) + .path(Path::from_response_slice(path)) + .extension_code(RESPONSE_VALIDATION) + .build(), + ); + } *output = Value::Null; } Ok(()) @@ -462,6 +522,19 @@ impl Query { if input.as_bool().is_some() { *output = input.clone(); } else { + if !input.is_null() { + parameters.validation_errors.push( + Error::builder() + .message(invalid_value_message( + parent_type, + field_type, + field_or_index, + )) + .path(Path::from_response_slice(path)) + .extension_code(RESPONSE_VALIDATION) + .build(), + ); + } *output = Value::Null; } Ok(()) @@ -470,6 +543,19 @@ impl Query { if input.as_str().is_some() { *output = input.clone(); } else { + if !input.is_null() { + parameters.validation_errors.push( + Error::builder() + .message(invalid_value_message( + parent_type, + field_type, + field_or_index, + )) + .path(Path::from_response_slice(path)) + .extension_code(RESPONSE_VALIDATION) + .build(), + ); + } *output = Value::Null; } Ok(()) @@ -478,6 +564,19 @@ impl Query { if input.is_string() || input.is_i64() || input.is_u64() || input.is_f64() { *output = input.clone(); } else { + if !input.is_null() { + parameters.validation_errors.push( + Error::builder() + .message(invalid_value_message( + parent_type, + field_type, + field_or_index, + )) + .path(Path::from_response_slice(path)) + .extension_code(RESPONSE_VALIDATION) + .build(), + ); + } *output = Value::Null; } Ok(()) @@ -497,11 +596,31 @@ impl Query { *output = input.clone(); Ok(()) } else { + parameters.validation_errors.push( + Error::builder() + .message(format!( + "Expected a valid enum value for type {}", + enum_type.name + )) + .path(Path::from_response_slice(path)) + .extension_code(RESPONSE_VALIDATION) + .build(), + ); *output = Value::Null; Ok(()) } } None => { + parameters.validation_errors.push( + Error::builder() + .message(format!( + "Expected a valid enum value for type {}", + enum_type.name + )) + .path(Path::from_response_slice(path)) + .extension_code(RESPONSE_VALIDATION) + .build(), + ); *output = Value::Null; Ok(()) } @@ -512,6 +631,9 @@ impl Query { match input { Value::Object(ref mut input_object) => { + // FIXME: we should return an error if __typename is not a string + // but this might cause issues for some production deployments where + // __typename might be missing or invalid (cf https://github.com/apollographql/router/commit/4a592f4933b7b9e46f14c7a98404b9e067687f09 ) if let Some(input_type) = input_object.get(TYPENAME).and_then(|val| val.as_str()) { @@ -564,8 +686,20 @@ impl Query { Ok(()) } - _ => { - parameters.nullified.push(Path::from_response_slice(path)); + Value::Null => { + *output = Value::Null; + Ok(()) + } + v => { + parameters.validation_errors.push( + Error::builder() + .message(format!( + "Invalid non-object value of type {} for composite type {type_name}", v.json_type_name() + )) + .path(Path::from_response_slice(path)) + .extension_code(RESPONSE_VALIDATION) + .build(), + ); *output = Value::Null; Ok(()) } @@ -641,6 +775,7 @@ impl Query { output_value, path, current_type, + FieldOrIndex::Field(field_name.as_str()), selection_set, ); path.pop(); @@ -650,7 +785,7 @@ impl Query { output.insert((*field_name).clone(), Value::Null); } if field_type.is_non_null() { - parameters.errors.push(Error { + parameters.nullification_errors.push(Error { message: format!( "Cannot return null for non-nullable field {current_type}.{}", field_name.as_str() @@ -772,6 +907,11 @@ impl Query { continue; } + let root_type = apollo_compiler::ast::Type::Named( + // Unchecked name instantiation is always safe, and we know the name is + // valid here + apollo_compiler::Name::new_unchecked(root_type_name), + ); let field_name = alias.as_ref().unwrap_or(name); let field_name_str = field_name.as_str(); @@ -800,13 +940,14 @@ impl Query { input_value, output_value, path, - &field_type.0, + &root_type, + FieldOrIndex::Field(field_name_str), selection_set, ); path.pop(); res? } else if field_type.is_non_null() { - parameters.errors.push(Error { + parameters.nullification_errors.push(Error { message: format!( "Cannot return null for non-nullable field {}.{field_name_str}", root_type_name @@ -1014,7 +1155,8 @@ impl Query { /// Intermediate structure for arguments passed through the entire formatting struct FormatParameters<'a> { variables: &'a Object, - errors: Vec, + nullification_errors: Vec, + validation_errors: Vec, nullified: Vec, schema: &'a ApiSchema, } @@ -1034,6 +1176,26 @@ pub(crate) struct Variable { default_value: Option, } +enum FieldOrIndex<'a> { + Field(&'a str), + Index(usize), +} + +fn invalid_value_message( + parent_type: &executable::Type, + field_type: &executable::Type, + field_or_index: FieldOrIndex, +) -> String { + match field_or_index { + FieldOrIndex::Field(field_name) => { + format!("Invalid value found for field {parent_type}.{field_name}") + } + FieldOrIndex::Index(i) => { + format!("Invalid value found for array element of type {field_type} at index {i}") + } + } +} + impl Operation { fn empty() -> Self { Self { diff --git a/apollo-router/src/spec/query/tests.rs b/apollo-router/src/spec/query/tests.rs index 1d979624053..f9af9948744 100644 --- a/apollo-router/src/spec/query/tests.rs +++ b/apollo-router/src/spec/query/tests.rs @@ -40,6 +40,7 @@ macro_rules! assert_eq_and_ordered_json { } #[derive(Default)] +#[must_use = "Must call .test() to run the test"] struct FormatTest { schema: Option<&'static str>, query_type_name: Option<&'static str>, @@ -100,6 +101,11 @@ impl FormatTest { self } + fn expected_errors(mut self, v: serde_json_bytes::Value) -> Self { + self.expected_errors = Some(v); + self + } + fn expected_extensions(mut self, v: serde_json_bytes::Value) -> Self { self.expected_extensions = Some(v); self @@ -1182,6 +1188,452 @@ fn reformat_response_array_of_id_duplicate() { .test(); } +#[test] +// If this test fails, this means you got greedy about allocations, +// beware of aliases! +fn reformat_response_expected_types() { + FormatTest::builder() + .schema( + "type Query { + get: Thing + } + type Thing { + i: Int + s: String + f: Float + b: Boolean + e: E + u: U + id: ID + l: [Int] + } + + enum E { + A + B + } + union U = ObjA | ObjB + type ObjA { + a: String + } + type ObjB { + a: String + } + ", + ) + .query( + r#"{ + get { + i + s + f + ... on Thing { + b + e + u { + ... on ObjA { + a + } + } + id + } + l + } + }"#, + ) + .response(json! {{ + "get": { + "i": "hello", + "s": 1.0, + "f": [1], + "b": 0, + "e": "X", + "u": 1, + "id": { + "test": "test", + }, + "l": "A" + }, + }}) + .expected(json! {{ + "get": { + "i": null, + "s": null, + "f": null, + "b": null, + "e": null, + "u": null, + // FIXME(@goto-bus-stop): this should be null, but we do not + // validate ID values today + "id": { + "test": "test", + }, + "l": null + }, + }}) + .expected_errors(json! ([ + { + "message": "Invalid value found for field Thing.i", + "path": ["get", "i"], + "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } + }, + { + "message": "Invalid value found for field Thing.s", + "path": ["get", "s"], + "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } + }, + { + "message": "Invalid value found for field Thing.f", + "path": ["get", "f"], + "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } + }, + { + "message": "Invalid value found for field Thing.b", + "path": ["get", "b"], + "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } + }, + { + "message": "Expected a valid enum value for type E", + "path": ["get", "e"], + "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } + }, + { + "message": "Invalid non-object value of type number for composite type U", + "path": ["get", "u"], + "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } + }, + { + "message": "Invalid non-list value of type string for list type [Int]", + "path": ["get", "l"], + "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } + } + ])) + .test(); +} + +#[test] +fn reformat_response_expected_int() { + FormatTest::builder() + .schema( + r#" + type Query { + a: Int + b: Int + c: Int + d: Int + e: Int + f: Int + g: Int + } + "#, + ) + .query(r#"{ a b c d e f g }"#) + .response(json!({ + "a": 1, + "b": 1.0, // Should be accepted as Int 1 + "c": 1.2, // Float should not be truncated + "d": "1234", // Optional to be coerced by spec: we do not do so + "e": true, + "f": [1], + "g": { "value": 1 }, + })) + .expected(json!({ + "a": 1, + // FIXME(@goto-bus-stop): we should accept this, and truncate it + // to Int value `1`, but do not do so today + "b": null, + "c": null, + "d": null, + "e": null, + "f": null, + "g": null, + })) + .expected_errors(json!([ + { + "message": "Invalid value found for field Query.b", + "path": ["b"], + "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } + }, + { + "message": "Invalid value found for field Query.c", + "path": ["c"], + "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } + }, + { + "message": "Invalid value found for field Query.d", + "path": ["d"], + "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } + }, + { + "message": "Invalid value found for field Query.e", + "path": ["e"], + "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } + }, + { + "message": "Invalid value found for field Query.f", + "path": ["f"], + "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } + }, + { + "message": "Invalid value found for field Query.g", + "path": ["g"], + "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } + }, + ])) + .test(); +} + +#[test] +fn reformat_response_expected_int_range() { + let schema = "type Query { + me: User + } + + type User { + id: String! + name: String + someNumber: Int + someOtherNumber: Int! + } + "; + + let query = "query { me { id name someNumber } }"; + + FormatTest::builder() + .schema(schema) + .query(query) + .response(json!({ + "me": { + "id": "123", + "name": "Guy Guyson", + "someNumber": 51049694213_i64 + }, + })) + .expected(json!({ + "me": { + "id": "123", + "name": "Guy Guyson", + "someNumber": null, + }, + })) + .expected_errors(json!([ + { + "message": "Invalid value found for field User.someNumber", + "path": ["me", "someNumber"], + "extensions": { "code": "RESPONSE_VALIDATION_FAILED" }, + } + ])) + .test(); + + let query2 = "query { me { id name someOtherNumber } }"; + + FormatTest::builder() + .schema(schema) + .query(query2) + .response(json!({ + "me": { + "id": "123", + "name": "Guy Guyson", + "someOtherNumber": 51049694213_i64 + }, + })) + .expected(json!({ + "me": null, + })) + .expected_errors(json!([ + { + "message": "Invalid value found for field User.someOtherNumber", + "path": ["me", "someOtherNumber"], + "extensions": { "code": "RESPONSE_VALIDATION_FAILED" }, + }, + ])) + .test(); +} + +#[test] +fn reformat_response_expected_float() { + FormatTest::builder() + .schema( + r#" + type Query { + a: Float + b: Float + c: Float + d: Float + e: Float + f: Float + } + "#, + ) + .query(r#"{ a b c d e f }"#) + .response(json!({ + // Note: NaNs and Infinitys are not supported by GraphQL Floats, + // and handily not representable in JSON, so we don't need to handle them. + "a": 1, // Int can be interpreted as Float + "b": 1.2, + "c": "2.2", // Optional to be coerced by spec: we do not do so + "d": true, + "e": [1.234], + "f": { "value": 12.34 }, + })) + .expected(json!({ + "a": 1, // Representing int-valued float without the decimals is okay in JSON + "b": 1.2, + "c": null, + "d": null, + "e": null, + "f": null, + })) + .expected_errors(json!([ + { + "message": "Invalid value found for field Query.c", + "path": ["c"], + "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } + }, + { + "message": "Invalid value found for field Query.d", + "path": ["d"], + "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } + }, + { + "message": "Invalid value found for field Query.e", + "path": ["e"], + "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } + }, + { + "message": "Invalid value found for field Query.f", + "path": ["f"], + "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } + }, + ])) + .test(); +} + +#[test] +fn reformat_response_expected_string() { + FormatTest::builder() + .schema( + r#" + type Query { + a: String + b: String + c: String + d: String + e: String + f: String + } + "#, + ) + .query(r#"{ a b c d e f }"#) + .response(json!({ + "a": "text", + "b": 1, // Optional to be coerced by spec: we do not do so + "c": false, // Optional to be coerced by spec: we do not do so + "d": 1234.5678, // Optional to be coerced by spec: we do not do so + "e": ["s"], + "f": { "text": "text" }, + })) + .expected(json!({ + "a": "text", + "b": null, + "c": null, + "d": null, + "e": null, + "f": null, + })) + .expected_errors(json!([ + { + "message": "Invalid value found for field Query.b", + "path": ["b"], + "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } + }, + { + "message": "Invalid value found for field Query.c", + "path": ["c"], + "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } + }, + { + "message": "Invalid value found for field Query.d", + "path": ["d"], + "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } + }, + { + "message": "Invalid value found for field Query.e", + "path": ["e"], + "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } + }, + { + "message": "Invalid value found for field Query.f", + "path": ["f"], + "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } + }, + ])) + .test(); +} + +#[test] +fn reformat_response_expected_id() { + FormatTest::builder() + .schema( + r#" + type Query { + a: ID + b: ID + c: ID + d: ID + e: ID + f: ID + g: ID + } + "#, + ) + .query(r#"{ a b c d e f g }"#) + .response(json!({ + "a": "1234", + "b": "ABCD", + "c": 1234, + "d": 1234.0, // Integer represented as a float should be coerced + "e": false, + "f": 1234.5678, // Float should not be truncated + "g": ["s"], + })) + .expected(json!({ + // Note technically IDs should always be represented as a String in JSON, + // though the value returned from a field can be either Int or String. + // We do not coerce the acceptable types to strings today. + "a": "1234", + "b": "ABCD", + "c": 1234, + // FIXME(@goto-bus-stop): We should coerce this to string "1234" (without .0), + // but we don't do so today + "d": 1234.0, + // FIXME(@goto-bus-stop): We should null out all these values, + // but we don't validate IDs today + "e": false, + "f": 1234.5678, + "g": ["s"], + })) + .expected_errors(json!([ + // FIXME(@goto-bus-stop): we should expect these errors: + // { + // "message": "Invalid value found for field Query.e", + // "path": ["e"], + // "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } + // }, + // { + // "message": "Invalid value found for field Query.f", + // "path": ["f"], + // "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } + // }, + // { + // "message": "Invalid value found for field Query.g", + // "path": ["g"], + // "extensions": { "code": "RESPONSE_VALIDATION_FAILED" } + // }, + ])) + .test(); +} + #[test] fn solve_query_with_single_typename() { FormatTest::builder() diff --git a/apollo-router/tests/snapshots/set_context__set_context_unrelated_fetch_failure.snap b/apollo-router/tests/snapshots/set_context__set_context_unrelated_fetch_failure.snap index 49dcf6bf9bc..4f28e804193 100644 --- a/apollo-router/tests/snapshots/set_context__set_context_unrelated_fetch_failure.snap +++ b/apollo-router/tests/snapshots/set_context__set_context_unrelated_fetch_failure.snap @@ -160,7 +160,7 @@ expression: response ] }, { - "message": "Cannot return null for non-nullable field T!.t", + "message": "Cannot return null for non-nullable field Query.t", "path": [ "t" ] diff --git a/docs/source/errors.mdx b/docs/source/errors.mdx index 5730dd6fef4..497471d8d93 100644 --- a/docs/source/errors.mdx +++ b/docs/source/errors.mdx @@ -99,4 +99,13 @@ The query could not be parsed. The response from a subgraph did not match the GraphQL schema. + + + + +A subgraph returned a field with a different type that mandated by the GraphQL schema. + + + + From f2dc704d69f11ccbfdbf2e5198547f17ba9e61b9 Mon Sep 17 00:00:00 2001 From: timbotnik Date: Tue, 29 Oct 2024 22:21:10 +1100 Subject: [PATCH 08/13] Apply suggestions from code review Co-authored-by: Edward Huang --- docs/source/configuration/overview.mdx | 8 ++-- .../telemetry/apollo-telemetry.mdx | 47 ++++++++++--------- 2 files changed, 28 insertions(+), 27 deletions(-) diff --git a/docs/source/configuration/overview.mdx b/docs/source/configuration/overview.mdx index c96db9d4fc2..2d0ec9af302 100644 --- a/docs/source/configuration/overview.mdx +++ b/docs/source/configuration/overview.mdx @@ -873,11 +873,11 @@ You won't see an immediate change in checks behavior when you first turn on exte -Beginning in v1.49.0, the router supports sending traces to Studio via the more detailed OTel (OpenTelemetry) protocol. -Support for OTel traces has historically only been available for 3rd party APM tools. With this option, -Studio can now provide a much more granular view of Router internals than the legacy Apollo tracing protocol. +Beginning in v1.49.0, the router supports sending traces to Studio via the OpenTelemetry Protocol (OTLP). +Support for OTLP traces has historically only been available for third-party APM tools. With this option, +Studio can now provide a much more granular and detailed view of router internals than the previous Apollo tracing protocol. -See [Enhanced tracing in Studio via OTel](./telemetry/apollo-telemetry#enhanced-tracing-in-studio-via-opentelemetry). +To learn more, see [Enhanced tracing in Studio via OpenTelemetry](./telemetry/apollo-telemetry#enhanced-tracing-in-studio-via-opentelemetry). ### Safelisting with persisted queries diff --git a/docs/source/configuration/telemetry/apollo-telemetry.mdx b/docs/source/configuration/telemetry/apollo-telemetry.mdx index d4e276118ea..20070898cef 100644 --- a/docs/source/configuration/telemetry/apollo-telemetry.mdx +++ b/docs/source/configuration/telemetry/apollo-telemetry.mdx @@ -80,30 +80,34 @@ telemetry: -Beginning in v1.49.0, the router supports sending traces to Studio via the more detailed OTel (OpenTelemetry) protocol. +Beginning in v1.49.0, the router supports sending traces to Studio via the OpenTelemetry protocol (OTLP). +Support for OTLP traces has historically only been available for third-party APM tools. With this option, +Studio can now provide a much more granular and detailed view of router internals than the previous Apollo tracing protocol. Support for OTel traces has historically only been available for 3rd party APM tools. With this option, Studio can now provide a much more granular view of Router internals than the legacy Apollo tracing protocol. -Benefits include: +Benefits of OTLP traces include: -- A comprehensive way to visualize the Router execution path in Studio. -- Additional spans that were previously not included in Studio traces, such as query parsing, planning, execution, and more. -- Additional attributes including HTTP request details, REST connector details, and more. +- Comprehensive visualization of the router execution path in Studio +- New spans in Studio traces, including query parsing, planning, execution, and more +- New attributes, including HTTP request details, REST connector details, and more -It is expected that this will become the default in a future version of Router. #### Configuration -This change adds a new configuration option `telemetry.apollo.experimental_otlp_tracing_sampler`. Use this option to send +You can configure OTel traces with the `telemetry.apollo.experimental_otlp_tracing_sampler` option. Use this option to send a percentage of traces to Studio via OTLP instead of the native Apollo Usage Reporting protocol. Supported values: -- `always_off` (default): send all traces via the legacy Apollo Usage Reporting protocol. -- `always_on`: send all traces via OTLP. -- `0.0 - 1.0` (used for testing): the ratio of traces to send via OTLP (0.5 = 50 / 50). +- `always_off` (default): send all traces via the legacy Apollo Usage Reporting protocol +- `always_on`: send all traces via OTLP +- `0.0 - 1.0` (used for testing): the ratio of traces to send via OTLP (0.4 = 40% OTLP / 60% legacy) -Note that this sampler is only applied _after_ the common tracing sampler, for example: +This sampler is applied after the common tracing sampler. + +#### Example configuration + +An example configuration that samples 1% of traces and sends all traces via OTLP: -#### Sample 1% of traces, send all traces via OTLP: ```yaml telemetry: @@ -118,21 +122,18 @@ telemetry: sampler: 0.01 ``` -OTel traces sent to Studio will not necessarily be identical to the ones sent to 3rd Party APM tools via OTLP: +OTLP traces sent to Studio aren't necessarily identical to ones sent to third-party APM tools via OTLP: -- Only specific OTLP attributes will be included for parity with what is provided in legacy traces today. This ensures that data privacy - is maintained in an equivalent manner. The existing Router configuration options for Apollo telemetry will continue to function - with OTLP traces, such as forwarding of GraphQL errors, headers, and variables. -- Some features of OTLP traces may only be available in Studio and not in 3rd Party APM tools (e.g. resolver-level timing information from - [Federated Tracing](../../federation/metrics/#enabling-federated-tracing)). +- Only specific OTLP attributes are included for parity with legacy traces today. This ensures that data privacy is maintained in an equivalent manner. Existing router configuration options for Apollo telemetry will continue to function with OTLP traces, including forwarding of GraphQL errors, headers, and variables. +- Some features of OTLP traces are available only in Studio and not in third-party APM tools, such as resolver-level timing information from [Federated Tracing](../../federation/metrics/#enabling-federated-tracing) -This change results in using a new wire protocol for traces, and some users may experience an increase in tracing traffic -to GraphOS Studio due to the additional detail being captured. In exceptional situations it may be necessary to send fewer traces. -This can be achieved via sending fewer traces (`telemetry.exporters.tracing.common.sampler`) or as a last resort, falling back -to the old protocol via `telemetry.apollo.otlp_tracing_sampler` to send fewer OTLP traces or fully disable them. -Any performance regressions due to the new tracing protocol should also be reported to the Apollo support team. +You may experience an increase in tracing traffic sent to GraphOS Studio due to the additional detail captured by the new wire protocol. In exceptional situations, you may need to send fewer traces. + +To send fewer traces, configure `telemetry.exporters.tracing.common.sampler` or revert to the old protocol via `telemetry.apollo.otlp_tracing_sampler` to send fewer OTLP traces or to disable them. + +For performance regressions due to the new tracing protocol, you should report them to the [Apollo support team](https://www.apollographql.com/support). From 442a543e8078eec7e0c0015e8111f5a939c6f00a Mon Sep 17 00:00:00 2001 From: timbotnik Date: Tue, 29 Oct 2024 23:37:19 +1100 Subject: [PATCH 09/13] Update docs/source/configuration/telemetry/apollo-telemetry.mdx Co-authored-by: Coenen Benjamin --- docs/source/configuration/telemetry/apollo-telemetry.mdx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/source/configuration/telemetry/apollo-telemetry.mdx b/docs/source/configuration/telemetry/apollo-telemetry.mdx index 20070898cef..4c2fb8d04fb 100644 --- a/docs/source/configuration/telemetry/apollo-telemetry.mdx +++ b/docs/source/configuration/telemetry/apollo-telemetry.mdx @@ -109,7 +109,7 @@ This sampler is applied after the common tracing sampler. An example configuration that samples 1% of traces and sends all traces via OTLP: -```yaml +```yaml title="router.config.yaml" telemetry: apollo: # Send all traces via OTLP From aa8bdd074e1be3449b62d899652583a5a839c93d Mon Sep 17 00:00:00 2001 From: Coenen Benjamin Date: Wed, 30 Oct 2024 10:18:25 +0100 Subject: [PATCH 10/13] Remove summit ad (#6209) --- README.md | 5 ----- 1 file changed, 5 deletions(-) diff --git a/README.md b/README.md index 0f87d6d343c..df19eb75938 100644 --- a/README.md +++ b/README.md @@ -4,11 +4,6 @@ --- -**Announcement:** -Join 1000+ engineers at GraphQL Summit for talks, workshops, and office hours, Oct 8-10 in NYC. [Get your pass here ->](https://summit.graphql.com/?utm_campaign=github_federation_readme) - ---- - # Apollo Router Core The **Apollo Router Core** is a configurable, high-performance **graph router** written in Rust to run a [federated supergraph](https://www.apollographql.com/docs/federation/) that uses [Apollo Federation 2](https://www.apollographql.com/docs/federation/v2/federation-2/new-in-federation-2). From f8e9a5200e282c0fa872780e75aca88b723cd42b Mon Sep 17 00:00:00 2001 From: Coenen Benjamin Date: Wed, 30 Oct 2024 16:39:42 +0100 Subject: [PATCH 11/13] chore(telemetry): don't create a stub span for supergraph events if it already has a current span (#6096) Signed-off-by: Benjamin <5719034+bnjjj@users.noreply.github.com> --- .changesets/maint_bnjjj_fix_supergraph_events_span.md | 5 +++++ .../src/plugins/telemetry/config_new/events.rs | 10 +++++++--- 2 files changed, 12 insertions(+), 3 deletions(-) create mode 100644 .changesets/maint_bnjjj_fix_supergraph_events_span.md diff --git a/.changesets/maint_bnjjj_fix_supergraph_events_span.md b/.changesets/maint_bnjjj_fix_supergraph_events_span.md new file mode 100644 index 00000000000..00f5cf241df --- /dev/null +++ b/.changesets/maint_bnjjj_fix_supergraph_events_span.md @@ -0,0 +1,5 @@ +### Don't create a stub span for supergraph events if it already has a current span ([PR #6096](https://github.com/apollographql/router/pull/6096)) + +Don't create useless span when we already have a span available to use the span's extensions. + +By [@bnjjj](https://github.com/bnjjj) in https://github.com/apollographql/router/pull/6096 \ No newline at end of file diff --git a/apollo-router/src/plugins/telemetry/config_new/events.rs b/apollo-router/src/plugins/telemetry/config_new/events.rs index a58264bfb78..c5fac133a2f 100644 --- a/apollo-router/src/plugins/telemetry/config_new/events.rs +++ b/apollo-router/src/plugins/telemetry/config_new/events.rs @@ -741,9 +741,13 @@ where } // Stub span to make sure the custom attributes are saved in current span extensions // It won't be extracted or sampled at all - let span = info_span!("supergraph_event_send_event"); - let _entered = span.enter(); - inner.send_event(attributes); + if Span::current().is_none() { + let span = info_span!("supergraph_event_send_event"); + let _entered = span.enter(); + inner.send_event(attributes); + } else { + inner.send_event(attributes); + } } fn on_error(&self, error: &BoxError, ctx: &Context) { From bc7472e06b6d8ace64d4f4fbd6607da9d0e2674a Mon Sep 17 00:00:00 2001 From: Geoffroy Couprie Date: Thu, 31 Oct 2024 15:34:25 +0100 Subject: [PATCH 12/13] do not count the wait time in deduplication as processing time (#6207) --- .changesets/fix_geal_deduplication_processing_time.md | 5 +++++ apollo-router/src/plugins/traffic_shaping/deduplication.rs | 1 + 2 files changed, 6 insertions(+) create mode 100644 .changesets/fix_geal_deduplication_processing_time.md diff --git a/.changesets/fix_geal_deduplication_processing_time.md b/.changesets/fix_geal_deduplication_processing_time.md new file mode 100644 index 00000000000..a3a75c467ef --- /dev/null +++ b/.changesets/fix_geal_deduplication_processing_time.md @@ -0,0 +1,5 @@ +### do not count the wait time in deduplication as processing time ([PR #6207](https://github.com/apollographql/router/pull/6207)) + +waiting for a deduplicated request was incorrectly counted as time spent in the router overhead, while most of it was actually spent waiting for the subgraph response. + +By [@Geal](https://github.com/Geal) in https://github.com/apollographql/router/pull/6207 \ No newline at end of file diff --git a/apollo-router/src/plugins/traffic_shaping/deduplication.rs b/apollo-router/src/plugins/traffic_shaping/deduplication.rs index 0df2574eb41..5c2165f7755 100644 --- a/apollo-router/src/plugins/traffic_shaping/deduplication.rs +++ b/apollo-router/src/plugins/traffic_shaping/deduplication.rs @@ -98,6 +98,7 @@ where let mut receiver = waiter.subscribe(); drop(locked_wait_map); + let _guard = request.context.enter_active_request(); match receiver.recv().await { Ok(value) => { return value From 6f65fa4913e84e9c3113791a358cb00dc2b689e6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9e?= Date: Mon, 4 Nov 2024 08:58:48 +0000 Subject: [PATCH 13/13] Remove redundant clone (#6218) --- apollo-router/src/services/layers/query_analysis.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apollo-router/src/services/layers/query_analysis.rs b/apollo-router/src/services/layers/query_analysis.rs index ce008c0a0f4..4af57a5f382 100644 --- a/apollo-router/src/services/layers/query_analysis.rs +++ b/apollo-router/src/services/layers/query_analysis.rs @@ -205,7 +205,7 @@ impl QueryAnalysisLayer { doc.executable.clone(), op_name, self.schema.api_schema(), - &request.supergraph_request.body().variables.clone(), + &request.supergraph_request.body().variables, )) } else { None