Skip to content

Commit

Permalink
fix(core): fix value of hasPrev[/Next]Page when paginating backwards (
Browse files Browse the repository at this point in the history
#5320)

When using cursor pagination, the `hasPrevPage` and `hasNextPage` has
inconsistency behavior that display the wrong value when query to the
last page of last/before (which is the first page of the first/after).

### Changes

```ts
const { first, last, before, after, orderBy, overfetch } = options;
const limit = first || last;
const isLast = !first && !!last;
const hasMorePages = !!overfetch && limit != null && items.length > limit;
// this.hasPrevPage = before || after ? true : (isLast && hasMorePages);
// this.hasNextPage = !(isLast && !before && !after) && hasMorePages;
this.hasPrevPage = isLast ? hasMorePages : !!after; 
this.hasNextPage = isLast ? !!before : hasMorePages;
```
The old `hasPrevPage` is true if either `before` or `after` exists,
which is wrong because having `before` doesn't mean it wouldn't have
more page beyond the result, `hasMorePages` decides that if a backward
pagination has a previous page.

Similar to `hasNextPage`, it fails when `isLast` is true and `before`
exists because it relies on hasMorePages, in a backward pagination
`hasMorePages` can not determine if it has the next page which is the
page beyond the `before`.

### Fixing the test cases

You can see the inconsistency in the test cases here, I have fixed to be
aligned with the changes.

```ts 
// These ones are correct with the following hasPrevPage/hasNextPage order: 
// true/false => true/true => false/true

// 1. page
const cursor1 = await orm.em.findByCursor(User, {}, {
  first: 3,
  orderBy: { id: 'asc' },
});
expect(cursor1.hasNextPage).toBe(true);
expect(cursor1.hasPrevPage).toBe(false);

// 2. page
const cursor2 = await orm.em.findByCursor(User, {}, {
  first: 3,
  after: cursor1,
  orderBy: { id: 'asc' },
});
expect(cursor2.hasNextPage).toBe(true);
expect(cursor2.hasPrevPage).toBe(true);

....

// 5. page (last)
const cursor5 = await orm.em.findByCursor(User, {}, {
  first: 40,
  after: cursor4,
  orderBy: { id: 'asc' },
});
expect(cursor5.hasNextPage).toBe(false);
expect(cursor5.hasPrevPage).toBe(true);
```

This is where I made the changes
```ts
// These ones are incorrect with the following hasPrevPage/hasNextPage order: 
// false/true => true/true => false/true
// I changed the last one into true/false

// 1. page
const cursor1 = await orm.em.findByCursor(User, {}, {
  last: 3,
  orderBy: { id: 'asc' },
});
expect(cursor1.hasNextPage).toBe(false);
expect(cursor1.hasPrevPage).toBe(true);

// 2. page
const cursor2 = await orm.em.findByCursor(User, {}, {
  last: 3,
  before: cursor1,
  orderBy: { id: 'asc' },
});
expect(cursor2.hasNextPage).toBe(true);
expect(cursor2.hasPrevPage).toBe(true);

// 5. page (last)
const cursor5 = await orm.em.findByCursor(User, {}, {
  last: 40,
  before: cursor4,
  orderBy: { id: 'asc' },
});
// expect(cursor5.hasNextPage).toBe(false);
// expect(cursor5.hasPrevPage).toBe(true);
expect(cursor5.hasNextPage).toBe(true); // new change
expect(cursor5.hasPrevPage).toBe(false); // new change
```

### Additional tests

I added a new `bidirectional-cursor.test.ts`, in which it will test the
cursor moving forwards using `first/after` from the first page to the
last page and then backward using `last/before` from the last cursor to
the first page, and vice versa.

Also, fixing the word simple to complex in the `complex-cursor.test.ts`

Co-authored-by: linhtv <linhtv@salefronts.com>
  • Loading branch information
linkthai and linhtv-salefronts committed Mar 14, 2024
1 parent 5cb5387 commit 00239eb
Show file tree
Hide file tree
Showing 6 changed files with 1,245 additions and 31 deletions.
4 changes: 2 additions & 2 deletions packages/core/src/utils/Cursor.ts
Expand Up @@ -77,8 +77,8 @@ export class Cursor<
const limit = first || last;
const isLast = !first && !!last;
const hasMorePages = !!overfetch && limit != null && items.length > limit;
this.hasPrevPage = before || after ? true : (isLast && hasMorePages);
this.hasNextPage = !(isLast && !before && !after) && hasMorePages;
this.hasPrevPage = isLast ? hasMorePages : !!after;
this.hasNextPage = isLast ? !!before : hasMorePages;

if (hasMorePages) {
if (isLast) {
Expand Down

0 comments on commit 00239eb

Please sign in to comment.