Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

support the incremental response field #1551

Merged
merged 9 commits into from
Aug 22, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions NEXT_CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -484,6 +484,13 @@ Introspection queries will now see the `@defer` directive if it was activated in

By [@Geal](https://github.com/Geal) in https://github.com/apollographql/router/pull/1557

### Support the incremental response field ([PR #1551](https://github.com/apollographql/router/pull/1551))

Recent changes in the `@defer` specification now mandate that the deferred responses are transmitted
as an array in the new `incremental` field of the JSON response.

By [@Geal](https://github.com/Geal) in https://github.com/apollographql/router/pull/1551

## 🛠 Maintenance

### Display licenses.html diff in CI if the check failed ([#1524](https://github.com/apollographql/router/issues/1524))
Expand Down
7 changes: 1 addition & 6 deletions apollo-router/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -125,13 +125,8 @@ impl FetchError {
/// Convert the error to an appropriate response.
pub fn to_response(&self) -> Response {
Response {
label: Default::default(),
data: Default::default(),
path: Default::default(),
errors: vec![self.to_graphql_error(None)],
extensions: Default::default(),
subselection: Default::default(),
has_next: Default::default(),
..Response::default()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😅

}
}
}
Expand Down
78 changes: 77 additions & 1 deletion apollo-router/src/response.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,12 +42,16 @@ pub struct Response {

#[serde(skip_serializing)]
pub subselection: Option<String>,

#[serde(skip_serializing_if = "Vec::is_empty", default)]
pub incremental: Vec<IncrementalResponse>,
}

#[buildstructor::buildstructor]
impl Response {
/// Constructor
#[builder(visibility = "pub")]
#[allow(clippy::too_many_arguments)]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is done automagically since 0.5.0

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here the lint was still complaining, unfortunately :/

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

interesting, we should open an issue

fn new(
label: Option<String>,
data: Option<Value>,
Expand All @@ -56,6 +60,7 @@ impl Response {
extensions: Map<ByteString, Value>,
subselection: Option<String>,
has_next: Option<bool>,
incremental: Vec<IncrementalResponse>,
) -> Self {
Self {
label,
Expand All @@ -65,10 +70,11 @@ impl Response {
extensions,
subselection,
has_next,
incremental,
}
}

/// If path is None, this is a primary query.
/// If path is None, this is a primary response.
pub fn is_primary(&self) -> bool {
self.path.is_none()
}
Expand Down Expand Up @@ -128,6 +134,24 @@ impl Response {
service: service_name.to_string(),
reason: err.to_string(),
})?;
let incremental =
extract_key_value_from_object!(object, "incremental", Value::Array(a) => a).map_err(
o0Ignition0o marked this conversation as resolved.
Show resolved Hide resolved
|err| FetchError::SubrequestMalformedResponse {
service: service_name.to_string(),
reason: err.to_string(),
},
)?;
let incremental: Vec<IncrementalResponse> = match incremental {
Some(v) => v
.into_iter()
.map(serde_json_bytes::from_value)
.collect::<Result<Vec<IncrementalResponse>, _>>()
.map_err(|err| FetchError::SubrequestMalformedResponse {
service: service_name.to_string(),
reason: err.to_string(),
})?,
None => vec![],
};

Ok(Response {
label,
Expand All @@ -137,10 +161,62 @@ impl Response {
extensions,
subselection: None,
has_next,
incremental,
})
}
}

/// A graphql incremental response.
/// Used with `@defer`
#[derive(Clone, Debug, Eq, PartialEq, Serialize, Deserialize, Default)]
#[serde(rename_all = "camelCase")]
pub struct IncrementalResponse {
/// The label that was passed to the defer or stream directive for this patch.
#[serde(skip_serializing_if = "Option::is_none", default)]
pub label: Option<String>,

/// The response data.
#[serde(skip_serializing_if = "Option::is_none", default)]
pub data: Option<Value>,

/// The path that the data should be merged at.
#[serde(skip_serializing_if = "Option::is_none", default)]
pub path: Option<Path>,

/// The optional graphql errors encountered.
#[serde(skip_serializing_if = "Vec::is_empty", default)]
pub errors: Vec<Error>,

/// The optional graphql extensions.
#[serde(skip_serializing_if = "Object::is_empty", default)]
pub extensions: Object,
}

#[buildstructor::buildstructor]
impl IncrementalResponse {
/// Constructor
#[builder(visibility = "pub")]
fn new(
label: Option<String>,
data: Option<Value>,
path: Option<Path>,
errors: Vec<Error>,
extensions: Map<ByteString, Value>,
) -> Self {
Self {
label,
data,
path,
errors,
extensions,
}
}

/// append_errors default the errors `path` with the one provided.
pub fn append_errors(&mut self, errors: &mut Vec<Error>) {
self.errors.append(errors)
}
}
#[cfg(test)]
mod tests {
use serde_json::json;
Expand Down
43 changes: 24 additions & 19 deletions apollo-router/src/services/supergraph_service.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,6 @@ use std::task::Poll;

use futures::future::ready;
use futures::future::BoxFuture;
use futures::future::Either;
use futures::stream;
use futures::stream::once;
use futures::stream::BoxStream;
use futures::stream::StreamExt;
Expand Down Expand Up @@ -41,6 +39,7 @@ use crate::plugin::DynPlugin;
use crate::plugin::Handler;
use crate::query_planner::BridgeQueryPlanner;
use crate::query_planner::CachingQueryPlanner;
use crate::response::IncrementalResponse;
use crate::router_factory::SupergraphServiceFactory;
use crate::services::layers::apq::APQLayer;
use crate::services::layers::ensure_query_presence::EnsureQueryPresence;
Expand Down Expand Up @@ -183,7 +182,7 @@ where
response: http::Response::from_parts(
parts,
response_stream
.flat_map(move |mut response: Response| {
.map(move |mut response: Response| {
tracing::debug_span!("format_response").in_scope(|| {
query.format_response(
&mut response,
Expand All @@ -199,7 +198,7 @@ where
response.has_next = Some(true);
}

Either::Left(once(ready(response)))
response
}
// if the deferred response specified a path, we must extract the
//values matched by that path and create a separate response for
Expand All @@ -221,21 +220,27 @@ where
},
);

Either::Right(stream::iter(
sub_responses.into_iter().map(
move |(path, data)| Response {
label: response.label.clone(),
data: Some(data),
path: Some(path),
errors: response.errors.clone(),
extensions: response.extensions.clone(),
has_next: Some(true),
subselection: response
.subselection
.clone(),
},
),
))
Response::builder()
.has_next(true)
.incremental(
sub_responses
.into_iter()
.map(move |(path, data)| {
IncrementalResponse::builder()
.and_label(
response.label.clone(),
)
.data(data)
.path(path)
.errors(response.errors.clone())
.extensions(
response.extensions.clone(),
)
.build()
})
.collect(),
)
.build()
}
}
})
Expand Down
69 changes: 6 additions & 63 deletions apollo-router/tests/integration_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -659,7 +659,7 @@ async fn defer_path() {
r#"{
me {
id
...@defer {
...@defer(label: "name") {
name
}
}
Expand All @@ -678,26 +678,10 @@ async fn defer_path() {
let mut stream = router.oneshot(request.into()).await.unwrap();

let first = stream.next_response().await.unwrap();
println!("first: {:?}", first);
assert_eq!(
first.data.unwrap(),
serde_json_bytes::json! {{
"me":{
"id":"1"
}
}}
);
assert_eq!(first.path, None);
insta::assert_json_snapshot!(first);

let second = stream.next_response().await.unwrap();
println!("second: {:?}", second);
assert_eq!(
second.data.unwrap(),
serde_json_bytes::json! {{
"name": "Ada Lovelace"
}}
);
assert_eq!(second.path.unwrap().to_string(), "/me");
insta::assert_json_snapshot!(second);
}

#[tokio::test(flavor = "multi_thread")]
Expand All @@ -720,7 +704,7 @@ async fn defer_path_in_array() {
id
author {
id
... @defer {
... @defer(label: "author name") {
name
}
}
Expand All @@ -741,51 +725,10 @@ async fn defer_path_in_array() {
let mut stream = router.oneshot(request.into()).await.unwrap();

let first = stream.next_response().await.unwrap();
println!("first: {:?}", first);
assert_eq!(
first.data.unwrap(),
serde_json_bytes::json! {{
"me":{
"reviews":[
{
"id": "1",
"author":
{
"id": "1"
}
},
{
"id": "2",
"author":
{
"id": "1"
}
}
]
}
}}
);
assert_eq!(first.path, None);
insta::assert_json_snapshot!(first);

let second = stream.next_response().await.unwrap();
println!("second: {:?}", second);
assert_eq!(
second.data.unwrap(),
serde_json_bytes::json! {{
"name": "Ada Lovelace"
}}
);
assert_eq!(second.path.unwrap().to_string(), "/me/reviews/0/author");

let third = stream.next_response().await.unwrap();
println!("third: {:?}", third);
assert_eq!(
third.data.unwrap(),
serde_json_bytes::json! {{
"name": "Ada Lovelace"
}}
);
assert_eq!(third.path.unwrap().to_string(), "/me/reviews/1/author");
insta::assert_json_snapshot!(second);
o0Ignition0o marked this conversation as resolved.
Show resolved Hide resolved
}

async fn query_node(
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
---
source: apollo-router/tests/integration_tests.rs
assertion_line: 679
expression: second
---
{
"hasNext": true,
"incremental": [
{
"label": "name",
"data": {
"name": "Ada Lovelace"
},
"path": [
"me"
]
}
]
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
---
source: apollo-router/tests/integration_tests.rs
assertion_line: 686
expression: first
---
{
"data": {
"me": {
"id": "1"
}
},
"hasNext": true
}
Loading