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

Update the pager object with fresh dataset before returning #403

Open
wants to merge 2 commits into
base: release-3.5
Choose a base branch
from

Conversation

katafrakt
Copy link

@katafrakt katafrakt commented Oct 13, 2021

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.

Pager object was keeping reference to old dataset in situations when
some additionap predicated were called after using #page or #per_page
methods, e.g.:

```
users.page(2).where(active: true)
```

In the example above, pager was still holding reference to a dataset not
having active=true condition set. This resulted in a correct dataset
being returned, but the pager methods like #total_pages returning wrong
results.

In this commit, a new public method #pager is created which shadows
regular usage from dry-initialize by firstly update the pager with fresh
dataset, so the calculations are correct.
@katafrakt katafrakt changed the title Update the pager object with fresh dataset before return Update the pager object with fresh dataset before returning Oct 13, 2021
@flash-gordon
Copy link
Member

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

@katafrakt
Copy link
Author

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

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.

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

Successfully merging this pull request may close these issues.

2 participants