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 with GROUPED STI relation #1015

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Expand Up @@ -34,7 +34,8 @@ def total_count(column_name = :all, _options = nil) #:nodoc:

# Handle grouping with a subquery
@total_count = if c.group_values.any?
c.model.from(c.except(:select).select("1")).count
# STI can place a 'hard scope' on the model which doesn't work on the outer query here
c.model.unscoped.unscope(where: :type).from(c.except(:select).select("1")).count
Copy link

@guigs guigs Nov 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better to use unscoped.unscope(:where) because one can use different column for STI (using inheritance_column)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Further down @yuki24's "rabbit hole"!

Here's another idea which you could argue reads better than "unscope it and then unscope it some more":

Suggested change
c.model.unscoped.unscope(where: :type).from(c.except(:select).select("1")).count
c.model.base_class.unscoped.from(c.except(:select).select("1")).count

else
c.count(column_name)
end
Expand Down
15 changes: 15 additions & 0 deletions kaminari-core/test/fake_app/active_record/models.rb
Expand Up @@ -51,6 +51,20 @@ class Product < ActiveRecord::Base
class Device < Product
end

# STI table: animals
class Animal < ActiveRecord::Base
end
class Mammal < Animal
end
class Dog < Mammal
end
class Cat < Mammal
end
class Insect < Animal
end
class Grasshopper < Insect
end

# migrations
ActiveRecord::Migration.verbose = false
ActiveRecord::Tasks::DatabaseTasks.root = Dir.pwd
Expand All @@ -66,6 +80,7 @@ def self.up
create_table(:authorships) {|t| t.integer :user_id; t.integer :book_id }
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 @@ -14,6 +14,11 @@ class ActiveRecordRelationMethodsTest < ActiveSupport::TestCase
@books3 = 4.times.map {|i| @author3.books_authored.create!(title: "subject%03d" % i) }
@readers = 4.times.map { User.create! name: 'reader' }
@books.each {|book| book.readers << @readers }
[
['Fenton', Dog], ['Bob', Dog],
['Garfield', Cat], ['Bob', Cat],
['Caine', Grasshopper]
].each { |name, type| type.create! name: name }
end
teardown do
Book.delete_all
Expand Down Expand Up @@ -100,6 +105,11 @@ class ActiveRecordRelationMethodsTest < ActiveSupport::TestCase
assert_equal 2, Authorship.group(:user_id).having("COUNT(book_id) >= 3").page(1).total_count
end

test 'calculating STI total_count with GROUP BY clause' do
# A type-based restriction places a scope on the model
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