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 3 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"
}
}
]
}
]
}
}
]
26 changes: 17 additions & 9 deletions apollo-router/src/services/router_service.rs
Original file line number Diff line number Diff line change
Expand Up @@ -402,7 +402,7 @@ 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 first = results.remove(0);
garypen marked this conversation as resolved.
Show resolved Hide resolved
let (parts, body) = first.response.into_parts();
let context = first.context;
let mut bytes = BytesMut::new();
Expand Down Expand Up @@ -601,9 +601,7 @@ impl RouterService {

let mut 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 first = ok_results.remove(0);
let sg = http::Request::from_parts(parts, first);

if !ok_results.is_empty() {
Expand Down Expand Up @@ -1055,13 +1053,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