Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pagination plugin returns incorect number of pages #366

Open
flash-gordon opened this issue Feb 16, 2020 · 6 comments
Open

Pagination plugin returns incorect number of pages #366

flash-gordon opened this issue Feb 16, 2020 · 6 comments
Labels

Comments

@flash-gordon
Copy link
Member

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? 😅)
@solnic
Copy link
Member

solnic commented Feb 16, 2020

@flash-gordon it calls unlimited, not unfiltered

@flash-gordon
Copy link
Member Author

@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
Copy link
Member

solnic commented Feb 16, 2020

@flash-gordon oh wow that is puzzling

@flash-gordon
Copy link
Member Author

Got it,

klass.class_eval do
defines :per_page
option :pager, default: -> {
Pager.new(dataset, per_page: self.class.per_page)
}
end

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

@solnic solnic added this to the 3.2.1 milestone May 29, 2020
@solnic solnic removed this from the 3.2.1 milestone Jan 6, 2021
@katafrakt
Copy link

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
Copy link
Member

solnic commented Oct 12, 2021

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants