From 04d86ed3f2537aff620941413e5fca254e87aebe Mon Sep 17 00:00:00 2001 From: Yuki Nishijima Date: Sat, 29 Feb 2020 18:24:45 -0500 Subject: [PATCH] Revert "Merge pull request #979 from MmKolodziej/better_total_count_for_grouped_queries" This reverts commit b5e2e07234caf015ca802e647a44512ebe45244a, reversing changes made to d1c26fd2fcd16f29b00bb076f47445ccbc550b89. We merged #979 as a performance improvement, but it turns out it introduced a couple of bugs. The solution seems to be a little tricky as it has to call `.unscoped` and `unscope(where: :type)`, so let's revert it until we find a better solution. closes #1012, #1015. --- .../active_record_relation_methods.rb | 15 +++++++++------ .../test/fake_app/active_record/models.rb | 14 +++++++++++++- .../active_record_relation_methods_test.rb | 16 ++++++++++++++++ 3 files changed, 38 insertions(+), 7 deletions(-) diff --git a/kaminari-activerecord/lib/kaminari/activerecord/active_record_relation_methods.rb b/kaminari-activerecord/lib/kaminari/activerecord/active_record_relation_methods.rb index 6c3c15ed0..ed35fbe10 100644 --- a/kaminari-activerecord/lib/kaminari/activerecord/active_record_relation_methods.rb +++ b/kaminari-activerecord/lib/kaminari/activerecord/active_record_relation_methods.rb @@ -32,12 +32,15 @@ def total_count(column_name = :all, _options = nil) #:nodoc: c = c.limit(max_pages * limit_value) if max_pages && max_pages.respond_to?(:*) - # Handle grouping with a subquery - @total_count = if c.group_values.any? - c.model.from(c.except(:select).select("1")).count - else - c.count(column_name) - end + # .group returns an OrderedHash that responds to #count + c = c.count(column_name) + @total_count = if c.is_a?(Hash) || c.is_a?(ActiveSupport::OrderedHash) + c.count + elsif c.respond_to? :count + c.count(column_name) + else + c + end end # Turn this Relation to a "without count mode" Relation. diff --git a/kaminari-core/test/fake_app/active_record/models.rb b/kaminari-core/test/fake_app/active_record/models.rb index 374ac18c9..56cd32853 100644 --- a/kaminari-core/test/fake_app/active_record/models.rb +++ b/kaminari-core/test/fake_app/active_record/models.rb @@ -26,6 +26,12 @@ class Authorship < ActiveRecord::Base belongs_to :user belongs_to :book end +class CurrentAuthorship < ActiveRecord::Base + self.table_name = 'authorships' + belongs_to :user + belongs_to :book + default_scope -> { where(deleted_at: nil) } +end class Readership < ActiveRecord::Base belongs_to :user belongs_to :book @@ -43,6 +49,11 @@ class Admin < User class User::Address < ActiveRecord::Base belongs_to :user end +class Animal < ActiveRecord::Base; end +class Mammal < Animal; end +class Dog < Mammal; end +class Cat < Mammal; end +class Insect < Animal; end # a class that uses abstract class class Product < ActiveRecord::Base @@ -63,9 +74,10 @@ def self.up create_table(:users) {|t| t.string :name; t.integer :age} create_table(:books) {|t| t.string :title} create_table(:readerships) {|t| t.integer :user_id; t.integer :book_id } - create_table(:authorships) {|t| t.integer :user_id; t.integer :book_id } + create_table(:authorships) {|t| t.integer :user_id; t.integer :book_id; t.datetime :deleted_at } create_table(:user_addresses) {|t| t.string :street; t.integer :user_id } create_table(:devices) {|t| t.string :name; t.integer :age} + create_table(:animals) {|t| t.string :type; t.string :name} end end CreateAllTables.up diff --git a/kaminari-core/test/models/active_record/active_record_relation_methods_test.rb b/kaminari-core/test/models/active_record/active_record_relation_methods_test.rb index 8093961e3..68febbacb 100644 --- a/kaminari-core/test/models/active_record/active_record_relation_methods_test.rb +++ b/kaminari-core/test/models/active_record/active_record_relation_methods_test.rb @@ -100,6 +100,22 @@ class ActiveRecordRelationMethodsTest < ActiveSupport::TestCase assert_equal 2, Authorship.group(:user_id).having("COUNT(book_id) >= 3").page(1).total_count end + test 'calculating total_count with GROUP BY ... HAVING clause with model that has default scope' do + assert_equal 2, CurrentAuthorship.group(:user_id).having("COUNT(book_id) >= 3").page(1).total_count + end + + test 'calculating STI total_count with GROUP BY clause' do + { + 'Fenton' => Dog, + 'Bob' => Dog, + 'Garfield' => Cat, + 'Bob' => Cat, + 'Caine' => Insect + }.each { |name, type| type.create!(name: name) } + + assert_equal 3, Mammal.group(:name).page(1).total_count + end + test 'total_count with max_pages does not add LIMIT' do begin subscriber = ActiveSupport::Notifications.subscribe 'sql.active_record' do |_, __, ___, ____, payload|