rom-sql icon indicating copy to clipboard operation
rom-sql copied to clipboard

Pagination plugin returns incorect number of pages

Open flash-gordon opened this issue 5 years ago • 6 comments

Describe the bug

If you paginate over a filtered relation (say, users.active) then rel.total will return incorrect results since it calls dataset.unfiltered which discard all the filtering.

To Reproduce

user.active.pager.total returns the total number of users

Expected behavior

user.active.pager.total returns the number of active users

Your environment

  • Affects my production application: NO (or maybe? 😅)

flash-gordon avatar Feb 16 '20 11:02 flash-gordon

@flash-gordon it calls unlimited, not unfiltered

solnic avatar Feb 16 '20 11:02 solnic

@solnic 🤦‍♂ ok I'm still getting invalid results from .pager.dataset.unlimited.count, Sequel issues queries for the default dataset which is not expected. I'll take a deeper look

flash-gordon avatar Feb 16 '20 11:02 flash-gordon

@flash-gordon oh wow that is puzzling

solnic avatar Feb 16 '20 11:02 solnic

Got it, https://github.com/rom-rb/rom-sql/blob/bde6843526a4f5e2508932e9050e59a55dcf800e/lib/rom/sql/plugin/pagination.rb#L115-L121 It happens because pager is eagerly evaluated and passed around with options when you chain relation calls. It means it always sticks to the default dataset. This is 💯 a bug, we should use memoization instead

flash-gordon avatar Feb 16 '20 11:02 flash-gordon

So, I just was bitten by this. And I discovered a funny thing, maybe it'd help someone:

relation.where { ... }.per_page(per_page).page(page) # this works correctly
relation.per_page(per_page).page(page).where { ... } # this does not

Anyway, I thought I could try to fix it and make a PR, but it seems that tests are currently not passing on master, so I'm not sure how to proceed.

katafrakt avatar Oct 11 '21 23:10 katafrakt

@katafrakt please target release-3.5 branch and then I could cherry-pick your fix into master

solnic avatar Oct 12 '21 06:10 solnic