search

rom-rb / rom-sql

SQL support for rom-rb
https://rom-rb.org
MIT License
217 stars 93 forks source link

Pagination plugin returns incorect number of pages #366

Open flash-gordon opened 4 years ago

flash-gordon commented 4 years ago

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

solnic commented 4 years ago

@flash-gordon it calls unlimited, not unfiltered

flash-gordon commented 4 years ago

@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

solnic commented 4 years ago

@flash-gordon oh wow that is puzzling

flash-gordon commented 4 years ago

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

katafrakt commented 2 years ago

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.

solnic commented 2 years ago

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