Skip to content

Commit

Permalink
Fix GraphQL pagination ordering
Browse files Browse the repository at this point in the history
This resolved an issue in the GraphQL API where the ordering of edges
was inconsistent when using `last`/`before` pagination arguments.
According to the GraphQL Cursor Connections Specification, the order of
edges should remain the same whether using `first`/`after` or
`last`/`before`, provided all other arguments are equal. Previously, our
API returned edges in reverse order when `last`/`before` was used, which
was contrary to the specification.
  • Loading branch information
msk committed Jan 25, 2024
1 parent bf3a5cd commit d82d4de
Show file tree
Hide file tree
Showing 3 changed files with 21 additions and 8 deletions.
12 changes: 12 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,17 @@ file is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and
this project adheres to [Semantic
Versioning](https://semver.org/spec/v2.0.0.html).

## [Unreleased]

### Fixed

- We've resolved an issue in the GraphQL API where the ordering of edges was
inconsistent when using `last`/`before` pagination arguments. According to the
GraphQL Cursor Connections Specification, the order of edges should remain the
same whether using `first`/`after` or `last`/`before`, provided all other
arguments are equal. Previously, our API returned edges in reverse order when
`last`/`before` was used, which was contrary to the specification.

## [0.17.0] - 2024-01-19

### Added
Expand Down Expand Up @@ -371,6 +382,7 @@ across our system.

- An initial version.

[Unreleased]: https://github.com/aicers/review-web/compare/0.17.0...main
[0.17.0]: https://github.com/aicers/review-web/compare/0.16.0...0.17.0
[0.16.0]: https://github.com/aicers/review-web/compare/0.15.0...0.16.0
[0.15.0]: https://github.com/aicers/review-web/compare/0.14.5...0.15.0
Expand Down
3 changes: 2 additions & 1 deletion src/graphql.rs
Original file line number Diff line number Diff line change
Expand Up @@ -280,12 +280,13 @@ where
map.iter_backward()?
};

let (nodes, has_more) = if let Some(after) = after {
let (mut nodes, has_more) = if let Some(after) = after {
let to = earliest_key(&after)?;
iter_to_nodes_with_filter(iter, &to, cmp::Ordering::is_ge, filter, last)
} else {
iter_to_nodes_with_filter(iter, &[], always_true, filter, last)
}?;
nodes.reverse();
Ok((nodes, has_more, false))
} else {
let first = first.unwrap_or(DEFAULT_CONNECTION_SIZE);
Expand Down
14 changes: 7 additions & 7 deletions src/graphql/account.rs
Original file line number Diff line number Diff line change
Expand Up @@ -755,11 +755,6 @@ mod tests {
};
assert_eq!(username, "u2");

// Record the cursor of the first edge.
let Some(Value::String(cursor)) = edge.get("cursor") else {
panic!("unexpected response: {:?}", edge);
};

// The last edge should be "u4".
let Some(Value::Object(edge)) = edges.get(2) else {
panic!("unexpected response: {:?}", edges);
Expand All @@ -772,11 +767,16 @@ mod tests {
};
assert_eq!(username, "u4");

// Record the cursor of the last edge.
let Some(Value::String(cursor)) = edge.get("cursor") else {
panic!("unexpected response: {:?}", edge);
};

// Retrieve backward.
let res = schema
.execute(&format!(
"query {{
accountList(last: 1, before: \"{cursor}\") {{
accountList(last: 3, before: \"{cursor}\") {{
edges {{
node {{
username
Expand All @@ -802,7 +802,7 @@ mod tests {
let Some(Value::List(edges)) = account_list.get("edges") else {
panic!("unexpected response: {:?}", account_list);
};
assert_eq!(edges.len(), 1);
assert_eq!(edges.len(), 3);
let Some(Value::Object(page_info)) = account_list.get("pageInfo") else {
panic!("unexpected response: {:?}", account_list);
};
Expand Down

0 comments on commit d82d4de

Please sign in to comment.