Propagate query planner errors as graphql::Error, not Result::Err

[BrynCooke]

Unlike the rest of the data-structures QueryPlannerResponse doesn't have an errors field. This is problematic as the error handling mechanism in tower is only really suitable for unrecoverable pipeline errors.

• Add Response to QueryPlanner
• Ensure that errors are propagated through the chain of responses

[SimonSapin]

I can have a first look tomorrow. Should errors from the query planner end up in the errors JSON key of GraphQL responses? Do we want the same graphql::Response type as everywhere else?

[SimonSapin]

I started at QueryPlannerResponse::error_new which used to ignore its errors parameter and emit a warning. Then propagated those errors values as necessary, and arrived at PR #1358.

It’s only after that that I noticed QueryPlannerResponse::error_new doesn’t have any caller. Maybe it’s intended for plugin authors?

Are there any cases today where the query planner emits errors? (That may not yet be correctly propagated.) Where in the code does that happen?

[SimonSapin]

router/apollo-router/src/query_planner/caching_query_planner.rs

Lines 120 to 152 in b6a2639

	.map_err(|e| {
	let CacheResolverError::RetrievalError(re) = &e;
	if let QueryPlannerError::PlanningErrors(pe) = re.deref() {
	if let Err(inner_e) = request
	.context
	.insert(USAGE_REPORTING, pe.usage_reporting.clone())
	{
	tracing::error!(
	"usage reporting was not serializable to context, {}",
	inner_e
	);
	}
	} else if let QueryPlannerError::SpecError(e) = re.deref() {
	let error_key = match e {
	SpecError::ParsingError(_) => "## GraphQLParseFailure\n",
	_ => "## GraphQLValidationFailure\n",
	};
	if let Err(inner_e) = request.context.insert(
	USAGE_REPORTING,
	UsageReporting {
	stats_report_key: error_key.to_string(),
	referenced_fields_by_type: HashMap::new(),
	},
	) {
	tracing::error!(
	"usage reporting was not serializable to context, {}",
	inner_e
	);
	}
	}
	e.into()
	})

This is where a CacheResolverError (enum whose only variant contains QueryPlannerError) gets converted to tower::BoxError

[SimonSapin]

Based on discussion with @garypen only Rhai errors still need more handling:

• For errors coming the "actual" query planner, the current behavior is already what we want: the service returns a Result whose errors are propagated at [1] with the ? operator so that they short-circuit the rest of the pipeline. If we don’t have a query plan, don’t do anything else

• Rust plugins that provide a query planner service are given a nested query planner service and are responsible for calling it and propagating errors with the ? operator, since they also return a Result.

• When a Rhai plugin throws an error an the query planner stage, QueryPlannerResponse::error_new is called. Currently this ignores the error with a tracing warning. This should be changed to short-circuit the rest of the pipeline and propagate the error.

[1]

router/apollo-router/src/services/router_service.rs

Lines 135 to 143 in b6a2639

	let QueryPlannerResponse { content, context } = planning
	.call(
	QueryPlannerRequest::builder()
	.originating_request(req.originating_request.clone())
	.query_plan_options(QueryPlanOptions::default())
	.context(context)
	.build(),
	)
	.await?;

[SimonSapin]

We’ve discussed this and agreed that query planner errors should not go through Result::Err. Instead we’ll change QueryPlannerResponse to:

• Add a Vec<apollo_router::graphql::Error>
• Make the query plan optional, so that it is not present when the query planner encountered a fatal error. Such an error would be in the Vec

This will make handling of Rhai errors in the query-planner stage the same as in other stages.

[SimonSapin]

With #1552 there are no query-planner-level Rhai plugins anymore. #1504 is still relevant in order to use GraphQL errors instead of Result::Err(BoxError), but it doesn’t affect the public API anymore. So removing the 1.0 label.