fix: correct pagination behavior for standard pagination with descending ordering clauses

[wschurman]

Why

This bug was subtle but important, and can be demonstrated best by looking at the new test case added performs forward pagination with descending order.

The bug was that cursor comparison must be derived from both direction but also ASC/DESC of the order bys.

Example:

obj1(t=1)
obj2(t=2)
obj3(t=3)
obj4(t=4)

> loadPage(first: 2, orderBy: t DESC)
obj4
obj3

> loadPage(first: 2, orderBy: t DESC, after: obj3)

the issue is that the cursor comparison for this second load would be:

(t) > (obj3.t)

which yields only obj4, which is incorrect.

How

The fix is to determine cursor comparator by both the order by direction and the pagination direction, thus making the cursor from the example above:

(t) < (obj3.t)

The second part of this is to require all orderBys to have the same ordering for pagination, otherwise this new assumption breaks since cursor comparison then would be unable to be done with a single comparator. While there are ways we can handle different order by directions, we'd have to not use a cursor which adds a massive amount of complexity.

A small other fix was to set the id tiebreaker column order to the same as the other orders.

Test Plan

Add new test case.

[wschurman]

• fix: correct pagination behavior for standard pagination with descending ordering clauses #477 👈 (View in Graphite)
• fix: correct precedence parentheses for SQLFragment joins #476
• feat: add method to get pagination cursor for single entity #475
• fix: correct discriminated union type for forward/backward pagination #474
• feat: support nullable search fields with postgres transforms #470
• fix: Apply same viewer context subclass type mechanism to knexLoader and related methods #472
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (2c4c649) to head (139816d).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##              main      #477    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files          110       110            
  Lines        16198     16228    +30     
  Branches      1456       878   -578     
==========================================
+ Hits         16198     16228    +30     

Flag	Coverage Δ
integration	24.31% <100.00%> (+0.14%)	⬆️
unittest	94.89% <32.87%> (-0.12%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[wschurman]

Merge activity

• Feb 27, 2:15 AM UTC: A user started a stack merge that includes this pull request via Graphite.
• Feb 27, 2:32 AM UTC: Graphite rebased this pull request as part of a merge.
• Feb 27, 2:35 AM UTC: @wschurman merged this pull request with Graphite.