simplify response streams

[Geal]

The code is organized around the ResponseStream type, that represents a response made from one or more GraphQL responses. That type complicates the code because we end up manipulating futures of response streams all the time.

It even goes up to the HTTP server implementation, where it requires transforming that stream of responses to a stream of Bytes.

AFAIK that was designed to prepare for @defer and @stream support, where multiple responses can be sent. That is not enough though, because those features will require multipart responses, and those are handled differently.

Proposal

• remove ResponseStream for now, as it is not useful
• make the Router and PreparedQuery traits return a hyper Response. This will be easier to integrate on the web server side and make it more reusable
• update the code to build only one response
• once we get started on defer and stream, that will require specific support from the query planner, so we can have a different execution mode that builds responses in a stream. By returning a hyper Response, we abstract those details from the outside

[Geal]

another issue I just encountered: Fetcher::stream returns a ResponseStream, so http_subgraph transforms its errors to a ResponseStream:

router/apollo-router/src/http_subgraph.rs

Lines 45 to 59 in ba03d47

	let response = self
	.http_client
	.post(self.url.clone())
	.json(&request)
	.send()
	.instrument(tracing::trace_span!("http-subgraph-request"))
	.await
	.map_err(|err| {
	tracing::error!(fetch_error = format!("{:?}", err).as_str());
	graphql::FetchError::SubrequestHttpError {
	service: self.service.to_owned(),
	reason: err.to_string(),
	}
	})?;

router/apollo-router/src/http_subgraph.rs

Lines 61 to 73 in ba03d47

	response
	.bytes()
	.instrument(tracing::debug_span!("aggregate_response_data"))
	.await
	.map_err(|err| {
	tracing::error!(fetch_error = format!("{:?}", err).as_str());
	graphql::FetchError::SubrequestHttpError {
	service: self.service.to_owned(),
	reason: err.to_string(),
	}
	})
	}

router/apollo-router/src/http_subgraph.rs

Lines 82 to 97 in ba03d47

	match response {
	Err(e) => e.to_response(is_primary),
	Ok(bytes) => tracing::debug_span!("parse_subgraph_response").in_scope(|| {
	serde_json::from_slice::<graphql::Response>(&bytes).unwrap_or_else(
	|error| {
	graphql::FetchError::SubrequestMalformedResponse {
	service: service_name.clone(),
	reason: error.to_string(),
	}
	.to_response(is_primary)
	},
	)
	}),
	}
	}
	.into_stream(),

but then in the query plan execution, we do not handle those errors properly:

router/apollo-router-core/src/query_planner/mod.rs

Lines 352 to 364 in ba03d47

	let subgraph_response = match res {
	Some(response) if !response.is_primary() => {
	return Err(FetchError::SubrequestUnexpectedPatchResponse {
	service: service_name.to_owned(),
	});
	}
	Some(subgraph_response) => subgraph_response,
	None => {
	return Err(FetchError::SubrequestNoResponse {
	service: service_name.to_string(),
	})
	}
	};

Example: if we had a network error talking to the backend, we will get this error:

router/apollo-router-core/src/query_planner/mod.rs

Lines 400 to 402 in ba03d47

	Err(FetchError::ExecutionInvalidContent {
	reason: "Missing key `_entities`!".to_string(),
	})

But in fact we should not even try to handle the response content

It would make more sense to have the fetcher return its content or an error, then convert that to a response in the query plan execution

[Geal]

fixed by #306