Skip to content

Commit

Permalink
Revert "Merge pull request #979 from MmKolodziej/better_total_count_f…
Browse files Browse the repository at this point in the history
…or_grouped_queries"

This reverts commit b5e2e07, reversing
changes made to d1c26fd.

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.
  • Loading branch information
yuki24 committed Feb 29, 2020
1 parent 7347292 commit 04d86ed
Show file tree
Hide file tree
Showing 3 changed files with 38 additions and 7 deletions.
Expand Up @@ -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.
Expand Down
14 changes: 13 additions & 1 deletion kaminari-core/test/fake_app/active_record/models.rb
Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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
Expand Up @@ -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|
Expand Down

0 comments on commit 04d86ed

Please sign in to comment.