Merge pull request #36506 from kamipo/group_by_with_order_by_virtual_count_attribute

PostgreSQL: Fix GROUP BY with ORDER BY virtual count attribute

5 files changed, 30 insertions, 12 deletions

diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
index 565d5c58b4..8d4b01e995 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 d1bcec9704..87a53966e4 100644
--- a/activerecord/lib/active_record/relation/query_methods.rb
+++ b/activerecord/lib/active_record/relation/query_methods.rb
@@ -1159,8 +1159,9 @@ module ActiveRecord
         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 @@ module ActiveRecord
         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 @@ module ActiveRecord
         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 5dabff1431..525085bb28 100644
--- a/activerecord/test/cases/calculations_test.rb
+++ b/activerecord/test/cases/calculations_test.rb
@@ -767,6 +767,12 @@ class CalculationsTest < ActiveRecord::TestCase
     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 a5c162af33..5df1e3ccf9 100644
--- a/activerecord/test/cases/relations_test.rb
+++ b/activerecord/test/cases/relations_test.rb
@@ -1955,8 +1955,8 @@ class RelationTest < ActiveRecord::TestCase
   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 41920b3719..eed18a7b89 100644
--- a/activerecord/test/schema/schema.rb
+++ b/activerecord/test/schema/schema.rb
@@ -261,6 +261,7 @@ ActiveRecord::Schema.define do
     t.references :developer, index: false
     t.references :company, index: false
     t.string :metadata
+    t.integer :count
   end
 
   create_table :customers, force: true do |t|