From e2f9227bea44367e7d4136734b81c88ce0fa60b3 Mon Sep 17 00:00:00 2001 From: Ben Dilley Date: Wed, 13 May 2020 15:35:35 +0100 Subject: [PATCH 1/5] Fix 'STI total_count with GROUP BY clause' test This test's data was built using a hash keyed by a name which was included twice, so it would always pass as long as total_count builds a valid query. Fixed by using a 2D array instead. --- .../active_record_relation_methods_test.rb | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) 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 68febbacb..5f7d3cece 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 @@ -105,13 +105,13 @@ class ActiveRecordRelationMethodsTest < ActiveSupport::TestCase 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) } + [ + ['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 From 2e6f61aa856484470929c800fee58388ab47652d Mon Sep 17 00:00:00 2001 From: Ben Dilley Date: Wed, 13 May 2020 15:36:24 +0100 Subject: [PATCH 2/5] Grouped query performance of total_count using SQL Improved performance for total_count on grouped ActiveRecord::Relation. This re-implements #979 using plain SQL to avoid issues #1012 and #1015 --- .../active_record_relation_methods.rb | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 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 ed35fbe10..cfd98cad2 100644 --- a/kaminari-activerecord/lib/kaminari/activerecord/active_record_relation_methods.rb +++ b/kaminari-activerecord/lib/kaminari/activerecord/active_record_relation_methods.rb @@ -32,15 +32,14 @@ def total_count(column_name = :all, _options = nil) #:nodoc: c = c.limit(max_pages * limit_value) if max_pages && max_pages.respond_to?(:*) - # .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 + # Handle grouping with a subquery + @total_count = if c.group_values.any? + c.model.connection.select_value <<~SQL + SELECT count(*) FROM (#{c.except(:select).select("1").to_sql}) subquery + SQL + else + c.count(column_name) + end end # Turn this Relation to a "without count mode" Relation. From 72e30e1b5d29229e59cf314cdbe477d6039b06e7 Mon Sep 17 00:00:00 2001 From: Ben Dilley Date: Fri, 30 Oct 2020 13:59:37 +0000 Subject: [PATCH 3/5] Attempt to accommodate total_count(column_name) Only count non-null values of column_name if supplied. This meets the semantics of the SQL count function without requiring that column_name is one of the grouped columns. Also use the older-style heredoc syntax for backwards-compat. --- .../activerecord/active_record_relation_methods.rb | 7 +++++-- 1 file changed, 5 insertions(+), 2 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 cfd98cad2..1f41d79ad 100644 --- a/kaminari-activerecord/lib/kaminari/activerecord/active_record_relation_methods.rb +++ b/kaminari-activerecord/lib/kaminari/activerecord/active_record_relation_methods.rb @@ -34,8 +34,11 @@ def total_count(column_name = :all, _options = nil) #:nodoc: # Handle grouping with a subquery @total_count = if c.group_values.any? - c.model.connection.select_value <<~SQL - SELECT count(*) FROM (#{c.except(:select).select("1").to_sql}) subquery + # Only count non-null values of column_name if supplied + c = c.where.not column_name => nil unless column_name.nil? || column_name == :all + + c.connection.select_value <<-SQL.strip! + SELECT count(*) FROM (#{c.except(:select).select(1).to_sql}) subquery SQL else c.count(column_name) From 149ff7e289e1e4d9021a57415b85216ea43bd34e Mon Sep 17 00:00:00 2001 From: Ben Dilley Date: Fri, 30 Oct 2020 14:47:51 +0000 Subject: [PATCH 4/5] Heredocs are frozen strings by default ...and it looks like activerecord 4.1 returns string representations from `select_value` --- .../lib/kaminari/activerecord/active_record_relation_methods.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 1f41d79ad..8a70e282b 100644 --- a/kaminari-activerecord/lib/kaminari/activerecord/active_record_relation_methods.rb +++ b/kaminari-activerecord/lib/kaminari/activerecord/active_record_relation_methods.rb @@ -37,7 +37,7 @@ def total_count(column_name = :all, _options = nil) #:nodoc: # Only count non-null values of column_name if supplied c = c.where.not column_name => nil unless column_name.nil? || column_name == :all - c.connection.select_value <<-SQL.strip! + c.connection.select_value(<<-SQL.strip).to_i SELECT count(*) FROM (#{c.except(:select).select(1).to_sql}) subquery SQL else From 343c870f9184e6a613116ff5e124ea1109595d91 Mon Sep 17 00:00:00 2001 From: Ben Dilley Date: Mon, 2 Nov 2020 09:17:25 +0000 Subject: [PATCH 5/5] Fix for activerecord 4.1 activerecord 4.1 doesn't like the use of a numeric value for `select` --- .../lib/kaminari/activerecord/active_record_relation_methods.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 8a70e282b..ce27b2ea1 100644 --- a/kaminari-activerecord/lib/kaminari/activerecord/active_record_relation_methods.rb +++ b/kaminari-activerecord/lib/kaminari/activerecord/active_record_relation_methods.rb @@ -38,7 +38,7 @@ def total_count(column_name = :all, _options = nil) #:nodoc: c = c.where.not column_name => nil unless column_name.nil? || column_name == :all c.connection.select_value(<<-SQL.strip).to_i - SELECT count(*) FROM (#{c.except(:select).select(1).to_sql}) subquery + SELECT count(*) FROM (#{c.except(:select).select("1").to_sql}) subquery SQL else c.count(column_name)