aboutsummaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorRyuta Kamizono <kamipo@gmail.com>2019-03-28 09:56:40 +0900
committerRyuta Kamizono <kamipo@gmail.com>2019-04-04 16:36:49 +0900
commit060e09dfb3637b03ef026c5e6b69432c08995c47 (patch)
tree24318e4f244c28480a936d8c76a282faf1b4ac24
parent39d42dbde2571a9a713007c1a8e9f202e004cdeb (diff)
downloadrails-060e09dfb3637b03ef026c5e6b69432c08995c47.tar.gz
rails-060e09dfb3637b03ef026c5e6b69432c08995c47.tar.bz2
rails-060e09dfb3637b03ef026c5e6b69432c08995c47.zip
Fix `count(:all)` with eager loading and explicit select and order
This follows up ebc09ed9ad9a04338138739226a1a92c7a2707ee. We've still experienced a regression for `size` (`count(:all)`) with eager loading and explicit select and order when upgrading Rails to 5.1. In that case, the eager loading enforces `distinct` to subselect but still keep the custom select, it would cause the ORDER BY with DISTINCT issue. ``` % ARCONN=postgresql bundle exec ruby -w -Itest test/cases/relations_test.rb -n test_size_with_eager_loading_and_custom_select_and_order Using postgresql Run options: -n test_size_with_eager_loading_and_custom_select_and_order --seed 8356 # Running: E Error: RelationTest#test_size_with_eager_loading_and_custom_select_and_order: ActiveRecord::StatementInvalid: PG::InvalidColumnReference: ERROR: for SELECT DISTINCT, ORDER BY expressions must appear in select list LINE 1: ..." ON "comments"."post_id" = "posts"."id" ORDER BY comments.i... ^ ``` As another problem on `distinct` is enforced, the result of `count` becomes fewer than expected if `select` is given explicitly. e.g. ```ruby Post.select(:type).count # => 11 Post.select(:type).distinct.count # => 3 ``` As long as `distinct` is enforced, we need to care to keep the result of `count`. This fixes both the `count` with eager loading problems.
-rw-r--r--activerecord/lib/active_record/relation/calculations.rb9
-rw-r--r--activerecord/test/cases/calculations_test.rb6
-rw-r--r--activerecord/test/cases/relations_test.rb6
3 files changed, 17 insertions, 4 deletions
diff --git a/activerecord/lib/active_record/relation/calculations.rb b/activerecord/lib/active_record/relation/calculations.rb
index 0384023a17..29b1dc97b7 100644
--- a/activerecord/lib/active_record/relation/calculations.rb
+++ b/activerecord/lib/active_record/relation/calculations.rb
@@ -129,11 +129,12 @@ module ActiveRecord
relation = apply_join_dependency
if operation.to_s.downcase == "count"
- relation.distinct!
- # PostgreSQL: ORDER BY expressions must appear in SELECT list when using DISTINCT
- if (column_name == :all || column_name.nil?) && select_values.empty?
- relation.order_values = []
+ unless distinct_value || distinct_select?(column_name || select_for_count)
+ relation.distinct!
+ relation.select_values = [ klass.primary_key || table[Arel.star] ]
end
+ # PostgreSQL: ORDER BY expressions must appear in SELECT list when using DISTINCT
+ relation.order_values = []
end
relation.calculate(operation, column_name)
diff --git a/activerecord/test/cases/calculations_test.rb b/activerecord/test/cases/calculations_test.rb
index f1e35d6ab9..c4070fc341 100644
--- a/activerecord/test/cases/calculations_test.rb
+++ b/activerecord/test/cases/calculations_test.rb
@@ -243,6 +243,12 @@ class CalculationsTest < ActiveRecord::TestCase
assert_queries(1) { assert_equal 11, posts.count(:all) }
end
+ def test_count_with_eager_loading_and_custom_select_and_order
+ posts = Post.includes(:comments).order("comments.id").select(:type)
+ assert_queries(1) { assert_equal 11, posts.count }
+ assert_queries(1) { assert_equal 11, posts.count(:all) }
+ end
+
def test_count_with_eager_loading_and_custom_order_and_distinct
posts = Post.includes(:comments).order("comments.id").distinct
assert_queries(1) { assert_equal 11, posts.count }
diff --git a/activerecord/test/cases/relations_test.rb b/activerecord/test/cases/relations_test.rb
index 131e034c66..2417775ef1 100644
--- a/activerecord/test/cases/relations_test.rb
+++ b/activerecord/test/cases/relations_test.rb
@@ -985,6 +985,12 @@ class RelationTest < ActiveRecord::TestCase
assert_queries(1) { assert_equal 11, posts.load.size }
end
+ def test_size_with_eager_loading_and_custom_select_and_order
+ posts = Post.includes(:comments).order("comments.id").select(:type)
+ assert_queries(1) { assert_equal 11, posts.size }
+ assert_queries(1) { assert_equal 11, posts.load.size }
+ end
+
def test_size_with_eager_loading_and_custom_order_and_distinct
posts = Post.includes(:comments).order("comments.id").distinct
assert_queries(1) { assert_equal 11, posts.size }