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

Fix total_count when grouping model has default scope #1012

Conversation

eitoball
Copy link
Contributor

This PR should fix #total_count when grouping model has default_scope.

Currently, if you have model like:

class User < ApplicationRecord
  default_scope -> { where(deleted_at: nil) }
end

and have statement like User.group(:email).having("COUNT(name) >= 3").page(1).total_count, you will get error like "Mysql2::Error: Unknown column 'users.deleted_at' in 'where clause'".

#total_count in 1.2.0 uses subquery (here). This remove table reference from FROM clause, and WHERE clause created by default_scope loses table to reference.

This PR removes default_scope by calling unscoped to solve this issue.

@ChrisBAshton
Copy link

This PR also fixes our issue: alphagov/whitehall#5300

In our case, we're not using a default scope but a custom one:

def unfiltered_scope
    StatisticsAnnouncement.includes(:current_release_date, statistics_announcement_topics: :topic, publication: :translations, organisations: :translations)
                          .joins("INNER JOIN statistics_announcement_dates
                            ON (statistics_announcement_dates.statistics_announcement_id = statistics_announcements.id)")
                          .joins("LEFT OUTER JOIN statistics_announcement_dates sd2
                            ON (sd2.statistics_announcement_id = statistics_announcements.id
                            AND statistics_announcement_dates.created_at > sd2.created_at)")
                          .group("statistics_announcement_dates.statistics_announcement_id")
                          .page(options[:page])
end

bendilley added a commit to skillstream/kaminari that referenced this pull request Feb 20, 2020
Incorporating solution kaminari#1012 which doesn't cover the
special case of STI type restriction.
@bendilley
Copy link
Contributor

This is a good catch and should definitely be merged IMO. I've opened #1015 which covers a variation of the same problem (when STI subtypes get involved) and which needed an additional unscope(where: :type). I've incorporated this fix so both PRs can be merged with a more obvious conflict resolution.

@yuki24
Copy link
Member

yuki24 commented Feb 20, 2020

Hmm, I think calling unscope would be very surprising. I think we should just revert #979. Also the tests in this PR and #1015 are very useful to catch failures early, we might as well want to do that once we revert that.

@bendilley
Copy link
Contributor

This is unscoping c.model in order to say from(subquery_with_all_the_scoping).count so within the remit of acquiring the correct count for a grouped query, I'd say the unscoping is not only ok, but the only way to be sure of deriving the correct total.

@yuki24
Copy link
Member

yuki24 commented Feb 29, 2020

I have reverted #979 and picked up the test from this PR. Thanks for reporting this issue!

bendilley added a commit to skillstream/kaminari that referenced this pull request May 13, 2020
Improved performance for total_count on grouped ActiveRecord::Relation.

This re-implements kaminari#979 using Arel to avoid issues kaminari#1012 and kaminari#1015
bendilley added a commit to skillstream/kaminari that referenced this pull request May 13, 2020
Improved performance for total_count on grouped ActiveRecord::Relation.

This re-implements kaminari#979 using plain SQL to avoid issues kaminari#1012 and kaminari#1015
bendilley added a commit to skillstream/kaminari that referenced this pull request May 13, 2020
Improved performance for total_count on grouped ActiveRecord::Relation.

This re-implements kaminari#979 using Arel to avoid issues kaminari#1012 and kaminari#1015
bendilley added a commit to skillstream/kaminari that referenced this pull request May 29, 2020
Improved performance for total_count on grouped ActiveRecord::Relation.

This re-implements kaminari#979 using plain SQL to avoid issues kaminari#1012 and kaminari#1015
bendilley added a commit to bendilley/kaminari that referenced this pull request Oct 29, 2020
Improved performance for total_count on grouped ActiveRecord::Relation.

This re-implements kaminari#979 using Arel to avoid issues kaminari#1012 and kaminari#1015
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.

None yet

4 participants