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

page.per producing wrong result set #813

Closed
dudo opened this issue Aug 9, 2016 · 12 comments
Closed

page.per producing wrong result set #813

dudo opened this issue Aug 9, 2016 · 12 comments

Comments

@dudo
Copy link

dudo commented Aug 9, 2016

[9] pry(#<SmartListing::Base>)> @collection.class
=> Loan::ActiveRecord_Relation
[10] pry(#<SmartListing::Base>)> @collection.size
=> 16
[11] pry(#<SmartListing::Base>)> @page
=> ""
[12] pry(#<SmartListing::Base>)> @per_page
=> 10
[13] pry(#<SmartListing::Base>)> @collection.page(@page).per(@per_page).size
2016-08-09 11:33:21,392|54722|DEBUG|development| -   CACHE (0.0ms)  SELECT  DISTINCT `loans`.`id`, statistics.created_at AS alias_0 FROM `loans` LEFT OUTER JOIN `lenders` ON `lenders`.`id` = `loans`.`lender_id` LEFT OUTER JOIN `comments` ON `comments`.`commentable_id` = `loans`.`id` AND `comments`.`commentable_type` = 'Loan' LEFT OUTER JOIN `admins` ON `admins`.`id` = `loans`.`admin_id` LEFT OUTER JOIN `partners` ON `partners`.`id` = `loans`.`partner_id` LEFT OUTER JOIN `promotions` ON `promotions`.`id` = `loans`.`promotion_id` LEFT OUTER JOIN `businesses` ON `businesses`.`loan_id` = `loans`.`id` LEFT OUTER JOIN `users` ON `users`.`id` = `loans`.`user_id` LEFT OUTER JOIN `statistics` ON `statistics`.`loan_id` = `loans`.`id` LEFT OUTER JOIN `decisions` ON `decisions`.`loan_id` = `loans`.`id` WHERE `loans`.`lender_id` IN (6, 2, 3, 5, 1, 4, 7) AND `loans`.`state` = 'funded' AND `lenders`.`info_key` IN ('gpb', 'gpb_large')  ORDER BY statistics.created_at desc LIMIT 10 OFFSET 0  [["commentable_type", "Loan"]]
2016-08-09 11:33:21,398|54722|DEBUG|development| -   CACHE (0.0ms)  SELECT DISTINCT COUNT(DISTINCT `loans`.`id`) FROM `loans` LEFT OUTER JOIN `lenders` ON `lenders`.`id` = `loans`.`lender_id` LEFT OUTER JOIN `comments` ON `comments`.`commentable_id` = `loans`.`id` AND `comments`.`commentable_type` = 'Loan' LEFT OUTER JOIN `admins` ON `admins`.`id` = `loans`.`admin_id` LEFT OUTER JOIN `partners` ON `partners`.`id` = `loans`.`partner_id` LEFT OUTER JOIN `promotions` ON `promotions`.`id` = `loans`.`promotion_id` LEFT OUTER JOIN `businesses` ON `businesses`.`loan_id` = `loans`.`id` LEFT OUTER JOIN `users` ON `users`.`id` = `loans`.`user_id` LEFT OUTER JOIN `statistics` ON `statistics`.`loan_id` = `loans`.`id` LEFT OUTER JOIN `decisions` ON `decisions`.`loan_id` = `loans`.`id` WHERE `loans`.`lender_id` IN (6, 2, 3, 5, 1, 4, 7) AND `loans`.`state` = 'funded' AND `lenders`.`info_key` IN ('gpb', 'gpb_large') AND `loans`.`id` IN (124, 20, 9, 653, 1652, 1652, 1652, 1652, 1652, 1652)  [["commentable_type", "Loan"]]
=> 5

The concerning part is this...

AND `loans`.`id` IN (124, 20, 9, 653, 1652, 1652, 1652, 1652, 1652, 1652)

@collection is table with a handful of other tables included, and I can't figure out for the life of me why paginating this collection botches the results. This behavior is in the https://github.com/Sology/smart_listing gem, but they use kaminari gem for pagination, so I hope you don't mind me asking this here.

Any thoughts or ideas on this mystery?

@yuki24
Copy link
Member

yuki24 commented Oct 12, 2016

If I remember correctly, there is a problem that occurs when DISTINCT is used with kaminari. Could you provide us with a simpler SQL example so we could reproduce the problem on our side?

@chrise86
Copy link

chrise86 commented Oct 17, 2016

Also having this issue, the produced SQL is:

SELECT  DISTINCT "events"."id", performances.date AS alias_0 FROM "events" LEFT OUTER JOIN "performances" ON "performances"."event_id" = "events"."id" ORDER BY performances.date ASC LIMIT $1 OFFSET $2  [["LIMIT", 24], ["OFFSET", 0]]

Caused by doing:

# app/models/event.rb
scope :by_date, -> { includes(:performances).order('performances.date ASC') }

And then in controller:

Event.by_date.page(params[:page])

How can I go about fixing this? 😕

@yuki24
Copy link
Member

yuki24 commented Oct 19, 2016

@chrise86 Thanks for the info. This is really helpful. Could you also tell us the numbers of event/performance records in the DB, the number of the elements you would expect to see on a page and the number of elements you actually saw?

@chrise86
Copy link

Hi @yuki24, sure, there are 260 events, 1300 performances - each event has 5 performances. I tried using the default 25 (I believe), and specifically setting paginates_per 24 in the Event model, so I would have expected to see 25 and 24 respectively.

@chrise86
Copy link

chrise86 commented Oct 19, 2016

Oddly, I can't replicate the issue in the console this morning. A lot has changed in the application but I don't think there has been any alterations around this part. I'll try and investigate further and report back anything I find. In the mean time, feel free to close, unless @dudo has any input? 👍

@merqlove
Copy link

merqlove commented Jan 11, 2017

@amatsuda, @yuki24 , hi, i have similar issue.
Now (1.0) if we run .per with nil argument, it is trying use max_per_page instead of default_per_page

@merqlove
Copy link

Ok, i found new method: max_paginates_per.

@amatsuda
Copy link
Member

@merqlove Ugh, confirmed that Model.page(1).per(nil).count returns the max_paginates_per value when both default_per_page (or paginates_per) and max_paginates_per were set.
This used to return the default_per_page value, and this was unintentionally changed in 1.0.0.
I mean, it's a regression.

BTW I'm curious, in what situation do you call per(nil)?

amatsuda added a commit that referenced this issue Jan 12, 2017
…fault

Fixes a regression reported at #813 (comment)

> Now (1.0) if we run .per with nil argument, it is trying use max_per_page instead of default_per_page
@merqlove
Copy link

@amatsuda, yep just changed in current app all per for max_paginates_per. Seems it works well.
Amm, i'm added per_page parameter for controllers action, with limit by max_per_page :) It is nil by default because Hash.

@yuki24
Copy link
Member

yuki24 commented Jan 12, 2017

@merqlove Thank you for reporting. Although your issue was an actual issue, I believe what @dudo observed was a different issue. Please double check next time you file an issue.

@dudo Could you provide an example with a failing test using the bug report template?

@merqlove
Copy link

Seems that it is fixed.

@ryanhageman
Copy link

Is this still an issue?

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

No branches or pull requests

6 participants