PageInfo spec has bugs that require `false` to be returned when there are previous or next pages

[myronmarston]

It appears that the formal algorithm for hasPreviousPage and hasNextPage, as defined in the Relay Connections Cursor Spec, has a couple bugs. That is, if I implement my server to follow the spec exactly as written, there are some situations where false will be returned for hasPreviousPage when there in fact is a previous page, and false will be returned for hasNextPage when there is in fact a next page.

Here is the current spec I am working off of:

HasPreviousPage(allEdges, before, after, first, last):
  * If {last} is set:
    * Let {edges} be the result of calling {ApplyCursorsToEdges(allEdges, before, after)}.
    * If {edges} contains more than {last} elements return {true}, otherwise {false}.
  * If {after} is set:
    * If the server can efficiently determine that elements exist prior to {after}, return {true}.
  * Return {false}.

HasNextPage(allEdges, before, after, first, last):
  * If {first} is set:
    * Let {edges} be the result of calling {ApplyCursorsToEdges(allEdges, before, after)}.
    * If {edges} contains more than {first} elements return {true}, otherwise {false}.
  * If {before} is set:
    * If the server can efficiently determine that elements exist following {before}, return {true}.
  * Return {false}.

For the bugs listed below, assume we're working with a list of edges like [a, b, c, d, e] (where the letters are the cursors for each edge).

Bug # 1

If my server is called with just after: a (no first, last or before arguments), then [b, c, d, e] will be returned as the page of edges. The algorithm requires that false be returned for hasPreviousPage, even though there is the previous page of [a]. Specifically, last is not set but after is, so this part of the algorithm applies:

* If {after} is set:
    * If the server can efficiently determine that elements exist prior to {after}, return {true}.
* Return {false}.

We can efficiently determine that no elements exist prior to the after cursor value of a (it is the first edge, after all) so we fall through to the Return {false} bit and return false.

In general, it seems like there's a mismatch between the algorithm for ApplyCursorsToEdges removing elements on or before after while hasPreviousPage only considers if any elements come before after (rather than also considering the element identified by after itself).

The same bug also exists for hasNextPage; if my server is called with just before: e, the spec tells me I have to return false for hasNextPage even though a next page of [e] does in fact exist.

Bug # 2

If my server is called with last: 3, after: a, before: e, then [b, c, d] will be returned as the page of edges, but the algorithm requires that false be returned for hasPreviousPage, even though there is a previous page ([a]). Since last is set, this part of the algorithm applies:

* If {last} is set:
    * Let {edges} be the result of calling {ApplyCursorsToEdges(allEdges, before, after)}.
    * If {edges} contains more than {last} elements return {true}, otherwise {false}.

Calling ApplyCursorsToEdges(allEdges, before, after) when allEdges = [a, b, c, d, e] and before = e and after = a results in [b, c, d]. edges contains 3 elements, which is the value of last; therefore, according to If {edges} contains more than {last} elements return {true}, otherwise {false} we must return false.

The same bug also exists for hasNextPage; if my server is called with just first: 3, after: a, before: 3, the spec tells me I have to return false for hasNextPage even though a next page of [e] does in fact exist.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[simnalamburt]

Could someone from the Relay team confirm whether this is an accurate interpretation or a misunderstanding, and whether it’s a bug or intended behavior? I’m implementing a GraphQL backend and unsure how to proceed. There's already a PR to fix this issue (#4365), but it would be great to have confirmation from the Relay team.