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

Remove the first element from the Vec, not the last #4144

Merged
merged 5 commits into from
Nov 9, 2023
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
5 changes: 5 additions & 0 deletions .changesets/fix_garypen_batch_ordering_fix.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
### Maintain query ordering within a batch ([Issue #4143](https://github.com/apollographql/router/issues/4143))

A bug in batch manipulation meant that the last element in a batch was treated as the first element. Ordering should be maintained and there is now an updated unit test to ensure this.

By [@garypen](https://github.com/garypen) in https://github.com/apollographql/router/pull/4144
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
"data": {
"topProducts": [
{
"upc": "1",
"name": "Table",
"reviews": [
{
Expand All @@ -29,7 +28,6 @@
]
},
{
"upc": "2",
"name": "Couch",
"reviews": [
{
Expand Down Expand Up @@ -94,5 +92,50 @@
}
]
}
},
{
"data": {
"topProducts": [
{
"upc": "1",
"name": "Table",
"reviews": [
{
"id": "1",
"product": {
"name": "Table"
},
"author": {
"name": "Ada Lovelace"
}
},
{
"id": "4",
"product": {
"name": "Table"
},
"author": {
"name": "Alan Turing"
}
}
]
},
{
"upc": "2",
"name": "Couch",
"reviews": [
{
"id": "2",
"product": {
"name": "Couch"
},
"author": {
"name": "Ada Lovelace"
}
}
]
}
]
}
}
]
44 changes: 30 additions & 14 deletions apollo-router/src/services/router_service.rs
Original file line number Diff line number Diff line change
Expand Up @@ -402,13 +402,16 @@ impl RouterService {
if results.len() == 1 {
Ok(results.pop().expect("we should have at least one response"))
} else {
let first = results.pop().expect("we should have at least one response");
let mut results_it = results.into_iter();
let first = results_it
.next()
.expect("we should have at least one response");
let (parts, body) = first.response.into_parts();
let context = first.context;
let mut bytes = BytesMut::new();
bytes.put_u8(b'[');
bytes.extend_from_slice(&hyper::body::to_bytes(body).await?);
for result in results {
for result in results_it {
bytes.put(&b", "[..]);
bytes.extend_from_slice(&hyper::body::to_bytes(result.response.into_body()).await?);
}
Expand Down Expand Up @@ -599,19 +602,22 @@ impl RouterService {
}
};

let mut ok_results = graphql_requests?;
let ok_results = graphql_requests?;
let mut results = Vec::with_capacity(ok_results.len());
let first = ok_results
.pop()
.expect("We must have at least one response");
let sg = http::Request::from_parts(parts, first);

if !ok_results.is_empty() {
if ok_results.len() > 1 {
context
.private_entries
.lock()
.insert(self.experimental_batching.clone());
}

let mut ok_results_it = ok_results.into_iter();
let first = ok_results_it
.next()
.expect("we should have at least one request");
let sg = http::Request::from_parts(parts, first);

// Building up the batch of supergraph requests is tricky.
// Firstly note that any http extensions are only propagated for the first request sent
// through the pipeline. This is because there is simply no way to clone http
Expand All @@ -626,7 +632,7 @@ impl RouterService {
// would mean all the requests in a batch shared the same set of private entries and review
// comments expressed the sentiment that this may be a bad thing...)
//
for graphql_request in ok_results {
for graphql_request in ok_results_it {
// XXX Lose http extensions, is that ok?
let mut new = http_ext::clone_http_request(&sg);
*new.body_mut() = graphql_request;
Expand Down Expand Up @@ -1058,13 +1064,23 @@ mod tests {
.build()
.unwrap()
.supergraph_request
.map(|req: crate::request::Request| {
// Modify the request so that it is a valid array of requests.
let mut json_bytes = serde_json::to_vec(&req).unwrap();
.map(|req_2: crate::request::Request| {
// Create clones of our standard query and update it to have 3 unique queries
let mut req_1 = req_2.clone();
let mut req_3 = req_2.clone();
req_1.query = req_2.query.clone().map(|x| x.replace("upc\n", ""));
req_3.query = req_2.query.clone().map(|x| x.replace("id name", "name"));

// Modify the request so that it is a valid array of 3 requests.
let mut json_bytes_1 = serde_json::to_vec(&req_1).unwrap();
let mut json_bytes_2 = serde_json::to_vec(&req_2).unwrap();
let mut json_bytes_3 = serde_json::to_vec(&req_3).unwrap();
let mut result = vec![b'['];
result.append(&mut json_bytes.clone());
result.append(&mut json_bytes_1);
result.push(b',');
result.append(&mut json_bytes);
result.append(&mut json_bytes_2);
result.push(b',');
result.append(&mut json_bytes_3);
result.push(b']');
hyper::Body::from(result)
});
Expand Down