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

Update the pager object with fresh dataset before returning

Open katafrakt opened this issue 3 years ago • 3 comments

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.

katafrakt avatar Oct 13 '21 22:10 katafrakt

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

flash-gordon avatar Oct 14 '21 11:10 flash-gordon

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.

katafrakt avatar Oct 14 '21 14:10 katafrakt

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.

katafrakt avatar Oct 14 '21 18:10 katafrakt