diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index 997afa6d91776..6ee86de8362b2 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,4 +1,9 @@ -<<<<<<< HEAD +* 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 33475e2e099ec..4943e17f1b873 100644 --- a/activerecord/test/cases/calculations_test.rb +++ b/activerecord/test/cases/calculations_test.rb @@ -774,6 +774,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 7bc5c1a7166de..2937f47c1d001 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|