From cb0299c9eba6463085130debda6347abf9683463 Mon Sep 17 00:00:00 2001 From: Ryuta Kamizono Date: Mon, 17 Jun 2019 21:49:28 +0900 Subject: [PATCH] PostgreSQL: Fix GROUP BY with ORDER BY virtual count attribute GROUP BY with virtual count attribute is invalid for almost all databases, but it is valid for PostgreSQL, and it had worked until Rails 5.2.2, so it is a regression for Rails 5.2.3 (caused by 311f001). I can't find perfectly solution for fixing this for now, but I would not like to break existing apps, so I decided to allow referencing virtual count attribute in ORDER BY clause when GROUP BY aggrigation (it partly revert the effect of 311f001) to fix the regression #36022. Fixes #36022. --- activerecord/CHANGELOG.md | 6 +++++ .../active_record/relation/query_methods.rb | 25 +++++++++++-------- activerecord/test/cases/calculations_test.rb | 6 +++++ activerecord/test/cases/relations_test.rb | 4 +-- activerecord/test/schema/schema.rb | 1 + 5 files changed, 30 insertions(+), 12 deletions(-) diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index 565d5c58b4eee..8d4b01e995f32 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,3 +1,9 @@ +* PostgreSQL: Fix GROUP BY with ORDER BY virtual count attribute. + + Fixes #36022. + + *Ryuta Kamizono* + * Make ActiveRecord `ConnectionPool.connections` method thread-safe. Fixes #36465. diff --git a/activerecord/lib/active_record/relation/query_methods.rb b/activerecord/lib/active_record/relation/query_methods.rb index d1bcec9704762..87a53966e4404 100644 --- a/activerecord/lib/active_record/relation/query_methods.rb +++ b/activerecord/lib/active_record/relation/query_methods.rb @@ -1159,8 +1159,9 @@ def arel_columns(columns) columns.flat_map do |field| case field when Symbol - field = field.to_s - arel_column(field, &connection.method(:quote_table_name)) + arel_column(field.to_s) do |attr_name| + connection.quote_table_name(attr_name) + end when String arel_column(field, &:itself) when Proc @@ -1268,20 +1269,14 @@ def preprocess_order_args(order_args) order_args.map! do |arg| case arg when Symbol - arg = arg.to_s - arel_column(arg) { - Arel.sql(connection.quote_table_name(arg)) - }.asc + order_column(arg.to_s).asc when Hash arg.map { |field, dir| case field when Arel::Nodes::SqlLiteral field.send(dir.downcase) else - field = field.to_s - arel_column(field) { - Arel.sql(connection.quote_table_name(field)) - }.send(dir.downcase) + order_column(field.to_s).send(dir.downcase) end } else @@ -1290,6 +1285,16 @@ def preprocess_order_args(order_args) end.flatten! end + def order_column(field) + arel_column(field) do |attr_name| + if attr_name == "count" && !group_values.empty? + arel_attribute(attr_name) + else + Arel.sql(connection.quote_table_name(attr_name)) + end + end + end + # Checks to make sure that the arguments are not blank. Note that if some # blank-like object were initially passed into the query method, then this # method will not raise an error. diff --git a/activerecord/test/cases/calculations_test.rb b/activerecord/test/cases/calculations_test.rb index 5dabff1431e64..525085bb28859 100644 --- a/activerecord/test/cases/calculations_test.rb +++ b/activerecord/test/cases/calculations_test.rb @@ -767,6 +767,12 @@ def test_pluck_with_join assert_equal [[2, 2], [4, 4]], Reply.includes(:topic).pluck(:id, :"topics.id") end + def test_group_by_with_order_by_virtual_count_attribute + expected = { "SpecialPost" => 1, "StiPost" => 2 } + actual = Post.group(:type).order(:count).limit(2).maximum(:comments_count) + assert_equal expected, actual + end if current_adapter?(:PostgreSQLAdapter) + def test_group_by_with_limit expected = { "Post" => 8, "SpecialPost" => 1 } actual = Post.includes(:comments).group(:type).order(:type).limit(2).count("comments.id") diff --git a/activerecord/test/cases/relations_test.rb b/activerecord/test/cases/relations_test.rb index a5c162af33d29..5df1e3ccf9155 100644 --- a/activerecord/test/cases/relations_test.rb +++ b/activerecord/test/cases/relations_test.rb @@ -1955,8 +1955,8 @@ def test_destroy_by test "joins with order by custom attribute" do companies = Company.create!([{ name: "test1" }, { name: "test2" }]) companies.each { |company| company.contracts.create! } - assert_equal companies, Company.joins(:contracts).order(:metadata) - assert_equal companies.reverse, Company.joins(:contracts).order(metadata: :desc) + assert_equal companies, Company.joins(:contracts).order(:metadata, :count) + assert_equal companies.reverse, Company.joins(:contracts).order(metadata: :desc, count: :desc) end test "delegations do not leak to other classes" do diff --git a/activerecord/test/schema/schema.rb b/activerecord/test/schema/schema.rb index 41920b3719e32..eed18a7b894ca 100644 --- a/activerecord/test/schema/schema.rb +++ b/activerecord/test/schema/schema.rb @@ -261,6 +261,7 @@ t.references :developer, index: false t.references :company, index: false t.string :metadata + t.integer :count end create_table :customers, force: true do |t|