rom-sql
rom-sql copied to clipboard
Update the pager object with fresh dataset before returning
Addresses #366
In the issue @flash-gordon identified root cause as eager evaluation of the pager, but my investigation showed that the problem is different. See what calling users.page(2)
does:
def page(num)
next_pager = pager.at(dataset, num)
new(next_pager.dataset, pager: next_pager)
end
It calculates the new dataset and creates a new Pager
holding this new dataset - and return new Relation
with new dataset and pager object. Now, if something else is called after it, for example users.page(2).where(active: true)
, a new Relation
is constructed with a new dataset containing an active=true
condition, but the pager still keep reference to old dataset, without this condition. As a result, when calculating #total_pages
etc., it uses stale dataset, without all the conditions - and this results in wrong numbers being returned.
Now, I'm not very good at naming, so I'm sure some things might be improved here, but I believe this is the simplest (or easiest?) way to fix the bug.
As an alternative approach, I thought about creating additional private API class, say, PagerConfig
, which would only hold current_page
and per_page
and only create "real" Pager
object (with methods like total_pages
etc.) from the newly introduced pager
method on Relation
. But I'm not sure if this is not an overkill.
why do we need pager
as an option in the first place? What's the problem with having it as a method instead? Using instance variables beyond constructor is not desirable because the relation can be frozen
pager
as an option keeps current_page
and per_page
values, which otherwise would be lost. On the other hand, these two could be options directly, not the whole pager.
I pushed another version, where option is renamed to _pager
to suggest it's internal (well...) and actual public pager is created in the end via public pager
method of the relation. No more instance variables, but still not as clean as I would wish it to be.