Merge branch 'dev' into i1g/max_headers

.changesets/fix_renee_limit_errors.md

@@ -0,0 +1,6 @@
+### Limit the amount of GraphQL validation errors returned in the response ([PR #6187](https://github.com/apollographql/router/pull/6187))
+
+When an invalid query is submitted, the router now returns at most 100 GraphQL parsing and validation errors in the response.
+This prevents generating a very large response for nonsense documents.
+
+By [@goto-bus-stop](https://github.com/goto-bus-stop) in https://github.com/apollographql/router/pull/6187

.changesets/helm_host_configuration.md

@@ -0,0 +1,6 @@
+### Allow for configuration of the host via the helm template for virtual service ([PR #5545](https://github.com/apollographql/router/pull/5795))
+
+Using the virtual service template change allows the configuration of the host from a variable when doing helm deploy.
+The default of any host causes issues for those that use different hosts for a single AKS cluster
+
+By [@nicksephora](https://github.com/nicksephora) in https://github.com/apollographql/router/pull/5545

apollo-federation/src/error/mod.rs

@@ -13,11 +13,44 @@ use lazy_static::lazy_static;
 
 use crate::subgraph::spec::FederationSpecError;
 
-/// Break out of the current function, returning an internal error.
+/// Create an internal error.
+///
+/// # Example
+/// ```rust
+/// use apollo_federation::internal_error;
+/// use apollo_federation::error::FederationError;
+/// # fn may_be_none() -> Option<()> { None }
+///
+/// const NAME: &str = "the thing";
+/// let result: Result<(), FederationError> = may_be_none()
+///     .ok_or_else(|| internal_error!("Expected {NAME} to be Some"));
+/// ```
 #[macro_export]
 macro_rules! internal_error {
     ( $( $arg:tt )+ ) => {
-        return Err($crate::error::FederationError::internal(format!( $( $arg )+ )).into());
+        $crate::error::FederationError::internal(format!( $( $arg )+ ))
+    }
+}
+
+/// Break out of the current function, returning an internal error.
+///
+/// # Example
+/// ```rust
+/// use apollo_federation::bail;
+/// use apollo_federation::error::FederationError;
+/// # fn may_be_none() -> Option<()> { None }
+///
+/// fn example() -> Result<(), FederationError> {
+///     bail!("Something went horribly wrong");
+///     unreachable!()
+/// }
+/// #
+/// # _ = example();
+/// ```
+#[macro_export]
+macro_rules! bail {
+    ( $( $arg:tt )+ ) => {
+        return Err($crate::internal_error!( $( $arg )+ ).into());
     }
 }
 
@@ -26,6 +59,18 @@ macro_rules! internal_error {
 ///
 /// Treat this as an assertion. It must only be used for conditions that *should never happen*
 /// in normal operation.
+///
+/// # Example
+/// ```rust,no_run
+/// use apollo_federation::ensure;
+/// use apollo_federation::error::FederationError;
+/// # fn may_be_none() -> Option<()> { None }
+///
+/// fn example() -> Result<(), FederationError> {
+///     ensure!(1 == 0, "Something went horribly wrong");
+///     unreachable!()
+/// }
+/// ```
 #[macro_export]
 macro_rules! ensure {
     ( $expr:expr, $( $arg:tt )+ ) => {

apollo-federation/src/operation/merging.rs

@@ -15,9 +15,9 @@ use super::NamedFragments;
 use super::Selection;
 use super::SelectionSet;
 use super::SelectionValue;
+use crate::bail;
 use crate::ensure;
 use crate::error::FederationError;
-use crate::internal_error;
 
 impl<'a> FieldSelectionValue<'a> {
     /// Merges the given field selections into this one.
@@ -50,14 +50,14 @@ impl<'a> FieldSelectionValue<'a> {
             );
             if self.get().selection_set.is_some() {
                 let Some(other_selection_set) = &other.selection_set else {
-                    internal_error!(
+                    bail!(
                         "Field \"{}\" has composite type but not a selection set",
                         other_field.field_position,
                     );
                 };
                 selection_sets.push(other_selection_set);
             } else if other.selection_set.is_some() {
-                internal_error!(
+                bail!(
                     "Field \"{}\" has non-composite type but also has a selection set",
                     other_field.field_position,
                 );
@@ -187,7 +187,7 @@ impl SelectionSet {
                 selection_map::Entry::Occupied(existing) => match existing.get() {
                     Selection::Field(self_field_selection) => {
                         let Selection::Field(other_field_selection) = other_selection else {
-                            internal_error!(
+                            bail!(
                                 "Field selection key for field \"{}\" references non-field selection",
                                 self_field_selection.field.field_position,
                             );
@@ -201,7 +201,7 @@ impl SelectionSet {
                         let Selection::FragmentSpread(other_fragment_spread_selection) =
                             other_selection
                         else {
-                            internal_error!(
+                            bail!(
                                 "Fragment spread selection key for fragment \"{}\" references non-field selection",
                                 self_fragment_spread_selection.spread.fragment_name,
                             );
@@ -215,7 +215,7 @@ impl SelectionSet {
                         let Selection::InlineFragment(other_inline_fragment_selection) =
                             other_selection
                         else {
-                            internal_error!(
+                            bail!(
                                 "Inline fragment selection key under parent type \"{}\" {}references non-field selection",
                                 self_inline_fragment_selection.inline_fragment.parent_type_position,
                                 self_inline_fragment_selection.inline_fragment.type_condition_position.clone()
@@ -368,7 +368,7 @@ pub(crate) fn merge_selection_sets(
     mut selection_sets: Vec<SelectionSet>,
 ) -> Result<SelectionSet, FederationError> {
     let Some((first, remainder)) = selection_sets.split_first_mut() else {
-        internal_error!("merge_selection_sets(): must have at least one selection set");
+        bail!("merge_selection_sets(): must have at least one selection set");
     };
     first.merge_into(remainder.iter())?;
 

apollo-federation/src/operation/simplify.rs

@@ -348,15 +348,29 @@ impl InlineFragmentSelection {
         } else {
             let rebased_inline_fragment = self.inline_fragment.rebase_on(parent_type, schema)?;
             let rebased_casted_type = rebased_inline_fragment.casted_type();
-            let rebased_selection_set =
-                selection_set.rebase_on(&rebased_casted_type, named_fragments, schema)?;
-            Ok(Some(
-                Selection::InlineFragment(Arc::new(InlineFragmentSelection::new(
-                    rebased_inline_fragment,
-                    rebased_selection_set,
-                )))
-                .into(),
-            ))
+            // Re-flatten with the rebased casted type, which could further flatten away.
+            let selection_set = selection_set.flatten_unnecessary_fragments(
+                &rebased_casted_type,
+                named_fragments,
+                schema,
+            )?;
+            if selection_set.is_empty() {
+                Ok(None)
+            } else {
+                // We need to rebase since the parent type for the selection set could be
+                // changed.
+                // Note: Rebasing after flattening, since rebasing before that can error out.
+                //       Or, `flatten_unnecessary_fragments` could `rebase` at the same time.
+                let rebased_selection_set =
+                    selection_set.rebase_on(&rebased_casted_type, named_fragments, schema)?;
+                Ok(Some(
+                    Selection::InlineFragment(Arc::new(InlineFragmentSelection::new(
+                        rebased_inline_fragment,
+                        rebased_selection_set,
+                    )))
+                    .into(),
+                ))
+            }
         }
     }
 }

apollo-federation/src/query_graph/mod.rs

@@ -16,6 +16,7 @@ use petgraph::Direction;
 
 use crate::error::FederationError;
 use crate::error::SingleFederationError;
+use crate::internal_error;
 use crate::operation::Field;
 use crate::operation::InlineFragment;
 use crate::operation::SelectionSet;
@@ -368,39 +369,27 @@ impl QueryGraph {
     }
 
     pub(crate) fn node_weight(&self, node: NodeIndex) -> Result<&QueryGraphNode, FederationError> {
-        self.graph.node_weight(node).ok_or_else(|| {
-            SingleFederationError::Internal {
-                message: "Node unexpectedly missing".to_owned(),
-            }
-            .into()
-        })
+        self.graph
+            .node_weight(node)
+            .ok_or_else(|| internal_error!("Node unexpectedly missing"))
     }
 
     fn node_weight_mut(&mut self, node: NodeIndex) -> Result<&mut QueryGraphNode, FederationError> {
-        self.graph.node_weight_mut(node).ok_or_else(|| {
-            SingleFederationError::Internal {
-                message: "Node unexpectedly missing".to_owned(),
-            }
-            .into()
-        })
+        self.graph
+            .node_weight_mut(node)
+            .ok_or_else(|| internal_error!("Node unexpectedly missing"))
     }
 
     pub(crate) fn edge_weight(&self, edge: EdgeIndex) -> Result<&QueryGraphEdge, FederationError> {
-        self.graph.edge_weight(edge).ok_or_else(|| {
-            SingleFederationError::Internal {
-                message: "Edge unexpectedly missing".to_owned(),
-            }
-            .into()
-        })
+        self.graph
+            .edge_weight(edge)
+            .ok_or_else(|| internal_error!("Edge unexpectedly missing"))
     }
 
     fn edge_weight_mut(&mut self, edge: EdgeIndex) -> Result<&mut QueryGraphEdge, FederationError> {
-        self.graph.edge_weight_mut(edge).ok_or_else(|| {
-            SingleFederationError::Internal {
-                message: "Edge unexpectedly missing".to_owned(),
-            }
-            .into()
-        })
+        self.graph
+            .edge_weight_mut(edge)
+            .ok_or_else(|| internal_error!("Edge unexpectedly missing"))
     }
 
     pub(crate) fn edge_head_weight(
@@ -415,12 +404,9 @@ impl QueryGraph {
         &self,
         edge: EdgeIndex,
     ) -> Result<(NodeIndex, NodeIndex), FederationError> {
-        self.graph.edge_endpoints(edge).ok_or_else(|| {
-            SingleFederationError::Internal {
-                message: "Edge unexpectedly missing".to_owned(),
-            }
-            .into()
-        })
+        self.graph
+            .edge_endpoints(edge)
+            .ok_or_else(|| internal_error!("Edge unexpectedly missing"))
     }
 
     fn schema(&self) -> Result<&ValidFederationSchema, FederationError> {
@@ -431,12 +417,9 @@ impl QueryGraph {
         &self,
         source: &str,
     ) -> Result<&ValidFederationSchema, FederationError> {
-        self.sources.get(source).ok_or_else(|| {
-            SingleFederationError::Internal {
-                message: "Schema unexpectedly missing".to_owned(),
-            }
-            .into()
-        })
+        self.sources
+            .get(source)
+            .ok_or_else(|| internal_error!(r#"Schema for "{source}" unexpectedly missing"#))
     }
 
     pub(crate) fn subgraph_schemas(&self) -> &IndexMap<Arc<str>, ValidFederationSchema> {
@@ -454,7 +437,7 @@ impl QueryGraph {
     ) -> Result<&IndexSet<NodeIndex>, FederationError> {
         self.types_to_nodes()?
             .get(name)
-            .ok_or_else(|| FederationError::internal("No nodes unexpectedly found for type"))
+            .ok_or_else(|| internal_error!("No nodes unexpectedly found for type"))
     }
 
     pub(crate) fn types_to_nodes(

apollo-federation/src/query_plan/conditions.rs

@@ -8,8 +8,8 @@ use apollo_compiler::Node;
 use indexmap::map::Entry;
 use serde::Serialize;
 
+use crate::bail;
 use crate::error::FederationError;
-use crate::internal_error;
 use crate::operation::DirectiveList;
 use crate::operation::NamedFragments;
 use crate::operation::Selection;
@@ -122,7 +122,7 @@ impl Conditions {
 
         if let Some(skip) = directives.get("skip") {
             let Some(value) = skip.specified_argument_by_name("if") else {
-                internal_error!("missing @skip(if:) argument");
+                bail!("missing @skip(if:) argument");
             };
 
             match value.as_ref() {
@@ -134,14 +134,14 @@ impl Conditions {
                     variables.insert(name.clone(), ConditionKind::Skip);
                 }
                 _ => {
-                    internal_error!("expected boolean or variable `if` argument, got {value}");
+                    bail!("expected boolean or variable `if` argument, got {value}");
                 }
             }
         }
 
         if let Some(include) = directives.get("include") {
             let Some(value) = include.specified_argument_by_name("if") else {
-                internal_error!("missing @include(if:) argument");
+                bail!("missing @include(if:) argument");
             };
 
             match value.as_ref() {
@@ -159,7 +159,7 @@ impl Conditions {
                     }
                 }
                 _ => {
-                    internal_error!("expected boolean or variable `if` argument, got {value}");
+                    bail!("expected boolean or variable `if` argument, got {value}");
                 }
             }
         }

apollo-federation/tests/query_plan/build_query_plan_support.rs

@@ -12,9 +12,9 @@ use apollo_federation::query_plan::TopLevelPlanNode;
 use apollo_federation::schema::ValidFederationSchema;
 use sha1::Digest;
 
-const ROVER_FEDERATION_VERSION: &str = "2.7.4";
+const ROVER_FEDERATION_VERSION: &str = "2.9.0";
 
-const DEFAULT_LINK_DIRECTIVE: &str = r#"@link(url: "https://specs.apollo.dev/federation/v2.7", import: ["@key", "@requires", "@provides", "@external", "@tag", "@extends", "@shareable", "@inaccessible", "@override", "@composeDirective", "@interfaceObject"])"#;
+const DEFAULT_LINK_DIRECTIVE: &str = r#"@link(url: "https://specs.apollo.dev/federation/v2.9", import: ["@key", "@requires", "@provides", "@external", "@tag", "@extends", "@shareable", "@inaccessible", "@override", "@composeDirective", "@interfaceObject", "@context", "@fromContext", "@cost", "@listSize"])"#;
 
 /// Runs composition on the given subgraph schemas and return `(api_schema, query_planner)`
 ///

apollo-federation/tests/query_plan/build_query_plan_tests.rs

@@ -1361,3 +1361,53 @@ fn condition_order_router799() {
     "###
     );
 }
+
+#[test]
+fn rebase_non_intersecting_without_dropping_inline_fragment_due_to_directive() {
+    let planner = planner!(
+        Subgraph1: r#"
+          type Query {
+            test: X
+          }
+
+          interface I {
+            i: Int
+          }
+
+          type X implements I {
+            i: Int
+          }
+
+          type Y implements I {
+            i: Int
+          }
+          "#,
+    );
+    assert_plan!(
+        &planner,
+        r#"
+          {
+            test { # type X
+              ... on I { # upcast to I
+                ... @skip(if: false) { # This fragment can't be dropped due to its directive.
+                  ... on Y { # downcast to Y (non-intersecting)
+                    i
+                  }
+                }
+              }
+            }
+          }
+        "#,
+        @r###"
+    QueryPlan {
+      Fetch(service: "Subgraph1") {
+        {
+          test {
+            __typename @include(if: false)
+          }
+        }
+      },
+    }
+    "###
+    );
+}