Update the pager object with fresh dataset before returning#403
Open
katafrakt wants to merge 2 commits intorom-rb:release-3.5from
Open
Update the pager object with fresh dataset before returning#403katafrakt wants to merge 2 commits intorom-rb:release-3.5from
katafrakt wants to merge 2 commits intorom-rb:release-3.5from
Conversation
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.
Member
|
why do we need |
Author
|
|
Author
|
I pushed another version, where option is renamed to |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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:It calculates the new dataset and creates a new
Pagerholding this new dataset - and return newRelationwith new dataset and pager object. Now, if something else is called after it, for exampleusers.page(2).where(active: true), a newRelationis constructed with a new dataset containing anactive=truecondition, but the pager still keep reference to old dataset, without this condition. As a result, when calculating#total_pagesetc., 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 holdcurrent_pageandper_pageand only create "real"Pagerobject (with methods liketotal_pagesetc.) from the newly introducedpagermethod onRelation. But I'm not sure if this is not an overkill.