Pagination first/last/before/after can't go both ways

[intellix]

Originally I was having issues with last, but decided to try first because it's simpler to understand and found that both are broken. I haven't noticed this before now because I've just been doing infinite scrolling to append rows which means you only go one way either forwards/backwards.

Now I've got to create an actual previous/next pagination and discovered:

Consider these 10 rows: a, b, c, d, e, f, g, h, i, j

First

✓ first 2 (forward)

expected: a, b actual: a, b expected limit: ASC LIMIT 2 actual limit: ASC LIMIT 2

✓ first 2 after b (forward)

expected: c, d actual: c, d expected limit: ASC LIMIT 2, 2 actual limit: ASC LIMIT 2, 2

✗first 2 before c (back)

expected: a, b actual: d, e expected limit: ASC LIMIT 2 actual limit: ASC LIMIT 3, 2

Last

✓ last 2 (forward)

expected: j, i actual: j, i expected limit: DESC LIMIT 2 actual limit: DESC LIMIT 2

✓ last 2 before i (forward)

expected: h, g actual: h, g expected limit: DESC LIMIT 2, 2 actual limit: DESC LIMIT 2, 2

✗last 2 after h (back)

expected: j, i actual: f, e expected limit: DESC LIMIT 0, 2 actual limit: DESC LIMIT 3, 2

Investigated somewhat and so far I found that the cursors are these: base64(PREFIX + id + SEPERATOR + index)

So in the first set, the cursors look like:

• arrayconnection$1$0
• arrayconnection$2$1
• arrayconnection$3$2
• arrayconnection$4$3

Code for the limits and offsets generated are: https://github.com/mickhansen/graphql-sequelize/blob/d39d0ce48ea248831480161f1aeff68f240f7b5c/src/relay.js#L217-L219 https://github.com/mickhansen/graphql-sequelize/blob/d39d0ce48ea248831480161f1aeff68f240f7b5c/src/relay.js#L265-L270

Maybe that should be (need to test):

options.offset = args.after ? startIndex + 1 : options.offset - args.limit;

[mickhansen]

Combining first/before and last/after was probably never adequately tested as it's generally not the way relay would query it.

[intellix]

It's supported by the official SWAPI test implementation: http://graphql.org/swapi-graphql/?query=%7B%0A%20%20allFilms(first%3A%202%2C%20before%3A%20%22YXJyYXljb25uZWN0aW9uOjI%3D%22)%20%7B%0A%20%20%20%20pageInfo%20%7B%0A%20%20%20%20%20%20startCursor%0A%20%20%20%20%20%20endCursor%0A%20%20%20%20%7D%0A%20%20%20%20edges%20%7B%0A%20%20%20%20%20%20cursor%0A%20%20%20%20%20%20node%20%7B%0A%20%20%20%20%20%20%20%20id%0A%20%20%20%20%20%20%20%20title%0A%20%20%20%20%20%20%7D%0A%20%20%20%20%7D%0A%20%20%7D%0A%7D

{
  allFilms(first: 2, after: "YXJyYXljb25uZWN0aW9uOjE=") {
    pageInfo {
      startCursor
      endCursor
    }
    edges {
      node {
        id
        title
      }
    }
  }
}

{
  allFilms(first: 2, before: "YXJyYXljb25uZWN0aW9uOjI=") {
    pageInfo {
      startCursor
      endCursor
    }
    edges {
      node {
        id
        title
      }
    }
  }
}

I'm not sure how you'd do previous/next pagination without it

[mickhansen]

It might be, but the relay connection stuff was made primarily for relay and Relay doesn't query like that so first/before and last/after were never really considered :)

first/before doesn't really make semantic sense to me either, but that's not relevant.

[mickhansen]

In any case i'd welcome a PR fixing this with tests, no reason not to have it :)

[rosshadden]

@mickhansen If you don't use first, before, last, or after, how are you doing previous/next style pagination? I would like to know what your alternative method is so I can use it to work around this issue as well.

[mickhansen]

@rosshadden That's not what i said exactly, i said i wasn't using the combination of first/before and last/after - react-relay uses the combination of first/after and last/before usually

[mickhansen]

as far as i can tell first: 2, before: "cursor" should behave the same way as first: 2

[intellix]

I'm working on this. Been on holiday so only poked at it on the plane :) I've added unit tests for Relay which pass as expected and now sorting out the connections from sequelize

[jedwards1211]

@intellix I think I could easily implement this in my PR #607. I'm wondering what your use case is though -- when do you need to use first: 2, before: "cursor"? As far as I understand, first, last, first/after and last/before are sufficient for doing pagination, and those are already supported. I don't see how first/before and last/after are necessary for doing pagination.

To get the next page one gets the first N items after the endCursor of the current page. To get the previous page one gets the last N items before the startCursor of the current page. Are you trying to do something different?

[jedwards1211]

@mickhansen I think I realized where @intellix' confusion is coming from. It's common to have a table where clicking on column headers toggles between ascending and descending sort on that column. I think relay connections, or at least sequelizeConnection, were designed more for a situation where there's only a fixed sort direction that can't be toggled in the UI. Because of this (the fact that last flips the sort order), there end up being two ways to support both sort orders with sequelizeConnection. Let's imagine we have a table of users that can be sorted on firstName and lastName in ascending or descending order.

Option 1

{
  FIRST_NAME: ['firstName', 'ASC'],
  LAST_NAME: ['lastName', 'ASC'],
}

Option 2

{
  FIRST_NAME_ASC: ['firstName', 'ASC'],
  FIRST_NAME_DESC: ['firstName', 'DESC'],
  LAST_NAME_ASC: ['lastName', 'ASC'],
  LAST_NAME_DESC: ['lastName', 'DESC'],
}

Option 1 in detail

With Option 1, paging is straightforward in the ASC direction. But paging in the DESC direction is funky, because when the user clicks the "Next" button, the client actually has to fetch the previous page (orderBy: [LAST_NAME], last: 10, before: result.pageInfo.startCursor), which also means that result.pageInfo.hasPreviousPage determines if the "Next" button is enabled. In other words:

# ascending sort
orderBy: [LAST_NAME], first: 10
# user clicks next
orderBy: [LAST_NAME], first: 10, after: result.pageInfo.endCursor
# user clicks previous
orderBy: [LAST_NAME], last: 10, before: result.pageInfo.startCursor
# NOTE: the edges returned for this page will be in descending order!

# descending sort
orderBy: [LAST_NAME], last: 10
# user clicks next
orderBy: [LAST_NAME], last: 10, before: result.pageInfo.startCursor
# user clicks previous
orderBy: [LAST_NAME], first: 10, after: result.pageInfo.endCursor
# NOTE: the edges returned for this page will be in ascending order!

Option 2 in detail

Option 2 doesn't have this awkward switcheroo but requires the client code to change the orderBy enum constant. So ascending paging looks like

orderBy: [LAST_NAME_ASC], first: 10
# user clicks next
orderBy: [LAST_NAME_ASC], first: 10, after: result.pageInfo.endCursor
# user clicks previous
orderBy: [LAST_NAME_ASC], last: 10, before: result.pageInfo.startCursor
# NOTE: the edges returned for this page will be descending order!

And descending paging looks like

orderBy: [LAST_NAME_DESC], first: 10
# user clicks next
orderBy: [LAST_NAME_DESC], first: 10, after: result.pageInfo.endCursor
# user clicks previous
orderBy: [LAST_NAME_DESC], last: 10, after: result.pageInfo.startCursor
# NOTE: the edges returned for this page will be in ascending order!

@intellix I think you probably want to use Option 2?

[jedwards1211]

@mickhansen also, when sorting ascending and fetching the previous page, sequelizeConnection returns the nodes in descending order, even though they will need to be displayed on screen in ascending order. Is that what relay pagination containers expect, or do the items end up getting displayed in the wrong order onscreen? I couldn't find anything in the relay cursor connections specification about the order of the nodes within edges, so I don't think they intended for the order when using last/before to be different from the order when using first/after; I'm thinking this might be a bug in sequelizeConnection.

[mickhansen]

@jedwards1211 Relay doesnt cover ordering in the spec as that is generally user supplied i believe, graphql-sequelize has the magic of changing the order when you change the query direction. That's a specific feature to ease the querying, there's an argument to be made for not having that i suppose, and letting the user change the ordering if they so wish. The SQL would still need to reverse the order but the results could be reversed again before being returned.

[jedwards1211]

Yes, for a last/before request I understand that requesting the reverse order from SQL is necessary so that the limit can be applied, but I think it would be best to reverse again before returning, so that the edges are in the same order as first/after results. If a dev wants to get the reverse order in the GraphQL results they could define a separate order by constant with the reverse order, as in my option 2 example. And I could help write more docs to clarify this

[jedwards1211]

@intellix has this helped clear up your confusion yet?

[jedwards1211]

I added some comments to my examples to better illustrate how the order of the edges is a bit surprising

[jedwards1211]

@mickhansen if you look closely at the reverse pagination test at https://github.com/mickhansen/graphql-sequelize/blob/master/test/integration/relay/connection.test.js#L961, you'll see that it fetches the previous page by doing last: 3, before: pageInfo.endCursor. I'm pretty sure the spec intends for us to use before: pageInfo.startCursor for fetching the previous page. (I've now fixed all of this in my PR)

[intellix]

Hey, kept getting sidetracked whilst writing a response and trying to write tests :)

After messing around with the tests for a while I did find what you said true about the reverse ordering. My original examples were wrong above.

If results are not automatically swapped or you force the ordering then the correct way to paginate is:

items: [0, 1, 2, 3, 4, 5]

first page: first 2: [0, 1]
next page:  first 2 after index 1: [2, 3]
next page:  first 2 after index 3: [4, 5]
prev page:  last 2 before index 4: [2, 3]
prev page:  last 2 before index 2: [0, 1]

Problem is as you said, it gets reversed and becomes a total mind game :S I found the reversing convenient until I was trying to do bi-directional pagination and then I just couldn't wrap my head around it

I suppose whatever I do will get superseded by the windowed queries which rewrites it right

[jedwards1211]

@intellix yeah, I figured that's how you got confused! Hopefully my PR will get merged soon and you'll find it easy to do bi-directional pagination with it.

I've filed an issue in Relay about adding missing specifications about ordering to the cursor connections specification: https://github.com/facebook/relay/issues/2466

Hopefully that will prevent misinterpretations in how the spec is implemented in the future.

[mickhansen]

@jedwards1211 That should atleast definitely be startCursor, but that's "just" a miswritten test, no need to ask for clarification in https://github.com/facebook/relay/issues/2466 on that as the code does adhere to the spec on that.

The proposed change is a major (as in semver major) change and should be seperate from any other PR so it can be discussed on it's own - i'm extremely open to it though as the logic does make sense.

[jedwards1211]

@mickhansen it's not just a miswritten test; it passes because the startCursor and endCursor end up swapped when last is used. I believe that this is a gap in the specification and while graphql-sequelize does adhere to what's written, that miswritten test would fail on a correct reverse pagination implementation (whereas it passes here).

[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.

[jedwards1211]

@mickhansen I kind of gave up on any hopes of getting my PRs merged...I just made my own fork and released it as a scoped package.

[mickhansen]

@jedwards1211 Understandable, i haven't been the most attentive maintainer at times