From 0189a169ff61a520528fb91bee5bd43a560cf10f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9e?= Date: Thu, 7 Nov 2024 10:16:58 +0000 Subject: [PATCH] chore(federation): add format-only error macro (#6236) --- apollo-federation/src/error/mod.rs | 49 +++++++++++++++- apollo-federation/src/operation/merging.rs | 14 ++--- apollo-federation/src/query_graph/mod.rs | 57 +++++++------------ .../src/query_plan/conditions.rs | 10 ++-- 4 files changed, 79 insertions(+), 51 deletions(-) diff --git a/apollo-federation/src/error/mod.rs b/apollo-federation/src/error/mod.rs index 97b7d2757d..cd163ecdab 100644 --- a/apollo-federation/src/error/mod.rs +++ b/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 )+ ) => { diff --git a/apollo-federation/src/operation/merging.rs b/apollo-federation/src/operation/merging.rs index c56ff5e66e..e7ebb8b63b 100644 --- a/apollo-federation/src/operation/merging.rs +++ b/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, ) -> Result { 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())?; diff --git a/apollo-federation/src/query_graph/mod.rs b/apollo-federation/src/query_graph/mod.rs index 344d9dc01d..736f7af567 100644 --- a/apollo-federation/src/query_graph/mod.rs +++ b/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, ValidFederationSchema> { @@ -454,7 +437,7 @@ impl QueryGraph { ) -> Result<&IndexSet, 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( diff --git a/apollo-federation/src/query_plan/conditions.rs b/apollo-federation/src/query_plan/conditions.rs index cf248c8b61..f202fd4058 100644 --- a/apollo-federation/src/query_plan/conditions.rs +++ b/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}"); } } }