Skip to content

Commit

Permalink
Improve error messaging when QP encounters unmergeable fields (#6420)
Browse files Browse the repository at this point in the history
This PR updates the error messaging when query planner encounters unmergeable fields (indicating a known bug in query planner), and increments an internal metric when this error kind is encountered. Note that query planner does not check all such cases of this, as doing so is computationally expensive. The bug itself is not fixed in this PR, as it requires substantial effort to fix.
  • Loading branch information
sachindshinde authored Dec 9, 2024
1 parent cf7a42d commit 89432b6
Show file tree
Hide file tree
Showing 3 changed files with 35 additions and 8 deletions.
4 changes: 4 additions & 0 deletions apollo-federation/src/error/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,9 @@ pub enum SingleFederationError {
#[error("An internal error has occurred, please report this bug to Apollo. Details: {0}")]
#[allow(private_interfaces)] // users should not inspect this.
InternalRebaseError(#[from] crate::operation::RebaseError),
// This is a known bug that will take time to fix, and does not require reporting.
#[error("{message}")]
InternalUnmergeableFields { message: String },
#[error("{diagnostics}")]
InvalidGraphQL { diagnostics: DiagnosticList },
#[error(transparent)]
Expand Down Expand Up @@ -301,6 +304,7 @@ impl SingleFederationError {
match self {
SingleFederationError::Internal { .. } => ErrorCode::Internal,
SingleFederationError::InternalRebaseError { .. } => ErrorCode::Internal,
SingleFederationError::InternalUnmergeableFields { .. } => ErrorCode::Internal,
SingleFederationError::InvalidGraphQL { .. }
| SingleFederationError::InvalidGraphQLName(_) => ErrorCode::InvalidGraphQL,
SingleFederationError::InvalidSubgraph { .. } => ErrorCode::InvalidGraphQL,
Expand Down
24 changes: 18 additions & 6 deletions apollo-federation/src/operation/merging.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ use super::SelectionValue;
use crate::bail;
use crate::ensure;
use crate::error::FederationError;
use crate::error::SingleFederationError;

impl<'a> FieldSelectionValue<'a> {
/// Merges the given field selections into this one.
Expand All @@ -42,12 +43,23 @@ impl<'a> FieldSelectionValue<'a> {
other_field.schema == self_field.schema,
"Cannot merge field selections from different schemas",
);
ensure!(
other_field.field_position == self_field.field_position,
"Cannot merge field selection for field \"{}\" into a field selection for field \"{}\"",
other_field.field_position,
self_field.field_position,
);
if other_field.field_position != self_field.field_position {
return Err(SingleFederationError::InternalUnmergeableFields {
message: format!(
"Cannot merge field selection for field \"{}\" into a field selection for \
field \"{}\". This is a known query planning bug in the old Javascript \
query planner that was silently ignored. The Rust-native query planner \
does not address this bug at this time, but in some cases does catch when \
this bug occurs. If you're seeing this message, this bug was likely \
triggered by one of the field selections mentioned previously having an \
alias that was the same name as the field in the other field selection. \
The recommended workaround is to change this alias to a different one in \
your operation.",
other_field.field_position, self_field.field_position,
),
}
.into());
}
if self.get().selection_set.is_some() {
let Some(other_selection_set) = &other.selection_set else {
bail!(
Expand Down
15 changes: 13 additions & 2 deletions apollo-router/src/query_planner/bridge_query_planner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -269,8 +269,19 @@ impl PlannerMode {
operation,
query_plan_options,
)
})
.map_err(|e| QueryPlannerError::FederationError(e.to_string()));
});
if let Err(FederationError::SingleFederationError(
SingleFederationError::InternalUnmergeableFields { .. },
)) = &result
{
u64_counter!(
"apollo.router.operations.query_planner.unmergeable_fields",
"Query planner caught attempting to merge unmergeable fields",
1
);
}
let result =
result.map_err(|e| QueryPlannerError::FederationError(e.to_string()));

let elapsed = start.elapsed().as_secs_f64();
metric_query_planning_plan_duration(RUST_QP_MODE, elapsed);
Expand Down

0 comments on commit 89432b6

Please sign in to comment.