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 rails#36022.

Fixes rails#36022.

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.

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.

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")

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

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|