Merge pull request #36506 from kamipo/group_by_with_order_by_virtual_…

…count_attribute

PostgreSQL: Fix GROUP BY with ORDER BY virtual count attribute

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.

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

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

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|