Merge pull request #31966 from kg8m/fix_limited_ids_for

Use column alias of primary_key in limited_ids_for

6 files changed, 30 insertions, 17 deletions

diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
index 36bb5e1daf..92c20ec79f 100644
--- a/activerecord/CHANGELOG.md
+++ b/activerecord/CHANGELOG.md
@@ -1,5 +1,13 @@
 ## Rails 6.0.0.alpha (Unreleased) ##
 
+*   Fix `#columsn_for_distinct` of MySQL and PostgreSQL to make
+    `ActiveRecord::FinderMethods#limited_ids_for` use correct primary key values
+    even if `ORDER BY` columns include other table's primary key.
+
+    Fixes #28364.
+
+    *Takumi Kagiyama*
+
 *   Make `reflection.klass` raise if `polymorphic?` not to be misused.
 
     Fixes #31876.
diff --git a/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb b/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb
index 608258d05c..fbedddd7f9 100644
--- a/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb
+++ b/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb
@@ -513,7 +513,7 @@ module ActiveRecord
           s.gsub(/\s+(?:ASC|DESC)\b/i, "")
         }.reject(&:blank?).map.with_index { |column, i| "#{column} AS alias_#{i}" }
 
-        [super, *order_columns].join(", ")
+        (order_columns << super).join(", ")
       end
 
       def strict_mode?
diff --git a/activerecord/lib/active_record/connection_adapters/postgresql/schema_statements.rb b/activerecord/lib/active_record/connection_adapters/postgresql/schema_statements.rb
index 6f3db772cd..45b230f0f9 100644
--- a/activerecord/lib/active_record/connection_adapters/postgresql/schema_statements.rb
+++ b/activerecord/lib/active_record/connection_adapters/postgresql/schema_statements.rb
@@ -583,7 +583,7 @@ module ActiveRecord
                .gsub(/\s+NULLS\s+(?:FIRST|LAST)\b/i, "")
             }.reject(&:blank?).map.with_index { |column, i| "#{column} AS alias_#{i}" }
 
-          [super, *order_columns].join(", ")
+          (order_columns << super).join(", ")
         end
 
         def update_table_definition(table_name, base) # :nodoc:
diff --git a/activerecord/test/cases/adapters/mysql2/mysql2_adapter_test.rb b/activerecord/test/cases/adapters/mysql2/mysql2_adapter_test.rb
index d18fb97e05..0719baaa23 100644
--- a/activerecord/test/cases/adapters/mysql2/mysql2_adapter_test.rb
+++ b/activerecord/test/cases/adapters/mysql2/mysql2_adapter_test.rb
@@ -25,25 +25,25 @@ class Mysql2AdapterTest < ActiveRecord::Mysql2TestCase
   end
 
   def test_columns_for_distinct_one_order
-    assert_equal "posts.id, posts.created_at AS alias_0",
+    assert_equal "posts.created_at AS alias_0, posts.id",
       @conn.columns_for_distinct("posts.id", ["posts.created_at desc"])
   end
 
   def test_columns_for_distinct_few_orders
-    assert_equal "posts.id, posts.created_at AS alias_0, posts.position AS alias_1",
+    assert_equal "posts.created_at AS alias_0, posts.position AS alias_1, posts.id",
       @conn.columns_for_distinct("posts.id", ["posts.created_at desc", "posts.position asc"])
   end
 
   def test_columns_for_distinct_with_case
     assert_equal(
-      "posts.id, CASE WHEN author.is_active THEN UPPER(author.name) ELSE UPPER(author.email) END AS alias_0",
+      "CASE WHEN author.is_active THEN UPPER(author.name) ELSE UPPER(author.email) END AS alias_0, posts.id",
       @conn.columns_for_distinct("posts.id",
         ["CASE WHEN author.is_active THEN UPPER(author.name) ELSE UPPER(author.email) END"])
     )
   end
 
   def test_columns_for_distinct_blank_not_nil_orders
-    assert_equal "posts.id, posts.created_at AS alias_0",
+    assert_equal "posts.created_at AS alias_0, posts.id",
       @conn.columns_for_distinct("posts.id", ["posts.created_at desc", "", "   "])
   end
 
@@ -52,7 +52,7 @@ class Mysql2AdapterTest < ActiveRecord::Mysql2TestCase
     def order.to_sql
       "posts.created_at desc"
     end
-    assert_equal "posts.id, posts.created_at AS alias_0",
+    assert_equal "posts.created_at AS alias_0, posts.id",
       @conn.columns_for_distinct("posts.id", [order])
   end
 
diff --git a/activerecord/test/cases/adapters/postgresql/postgresql_adapter_test.rb b/activerecord/test/cases/adapters/postgresql/postgresql_adapter_test.rb
index 1951230c8a..cbb6cd42b5 100644
--- a/activerecord/test/cases/adapters/postgresql/postgresql_adapter_test.rb
+++ b/activerecord/test/cases/adapters/postgresql/postgresql_adapter_test.rb
@@ -263,25 +263,25 @@ module ActiveRecord
       end
 
       def test_columns_for_distinct_one_order
-        assert_equal "posts.id, posts.created_at AS alias_0",
+        assert_equal "posts.created_at AS alias_0, posts.id",
           @connection.columns_for_distinct("posts.id", ["posts.created_at desc"])
       end
 
       def test_columns_for_distinct_few_orders
-        assert_equal "posts.id, posts.created_at AS alias_0, posts.position AS alias_1",
+        assert_equal "posts.created_at AS alias_0, posts.position AS alias_1, posts.id",
           @connection.columns_for_distinct("posts.id", ["posts.created_at desc", "posts.position asc"])
       end
 
       def test_columns_for_distinct_with_case
         assert_equal(
-          "posts.id, CASE WHEN author.is_active THEN UPPER(author.name) ELSE UPPER(author.email) END AS alias_0",
+          "CASE WHEN author.is_active THEN UPPER(author.name) ELSE UPPER(author.email) END AS alias_0, posts.id",
           @connection.columns_for_distinct("posts.id",
             ["CASE WHEN author.is_active THEN UPPER(author.name) ELSE UPPER(author.email) END"])
         )
       end
 
       def test_columns_for_distinct_blank_not_nil_orders
-        assert_equal "posts.id, posts.created_at AS alias_0",
+        assert_equal "posts.created_at AS alias_0, posts.id",
           @connection.columns_for_distinct("posts.id", ["posts.created_at desc", "", "   "])
       end
 
@@ -290,23 +290,23 @@ module ActiveRecord
         def order.to_sql
           "posts.created_at desc"
         end
-        assert_equal "posts.id, posts.created_at AS alias_0",
+        assert_equal "posts.created_at AS alias_0, posts.id",
           @connection.columns_for_distinct("posts.id", [order])
       end
 
       def test_columns_for_distinct_with_nulls
-        assert_equal "posts.title, posts.updater_id AS alias_0", @connection.columns_for_distinct("posts.title", ["posts.updater_id desc nulls first"])
-        assert_equal "posts.title, posts.updater_id AS alias_0", @connection.columns_for_distinct("posts.title", ["posts.updater_id desc nulls last"])
+        assert_equal "posts.updater_id AS alias_0, posts.title", @connection.columns_for_distinct("posts.title", ["posts.updater_id desc nulls first"])
+        assert_equal "posts.updater_id AS alias_0, posts.title", @connection.columns_for_distinct("posts.title", ["posts.updater_id desc nulls last"])
       end
 
       def test_columns_for_distinct_without_order_specifiers
-        assert_equal "posts.title, posts.updater_id AS alias_0",
+        assert_equal "posts.updater_id AS alias_0, posts.title",
           @connection.columns_for_distinct("posts.title", ["posts.updater_id"])
 
-        assert_equal "posts.title, posts.updater_id AS alias_0",
+        assert_equal "posts.updater_id AS alias_0, posts.title",
           @connection.columns_for_distinct("posts.title", ["posts.updater_id nulls last"])
 
-        assert_equal "posts.title, posts.updater_id AS alias_0",
+        assert_equal "posts.updater_id AS alias_0, posts.title",
           @connection.columns_for_distinct("posts.title", ["posts.updater_id nulls first"])
       end
 
diff --git a/activerecord/test/cases/finder_test.rb b/activerecord/test/cases/finder_test.rb
index ebddf81449..d85910d9c6 100644
--- a/activerecord/test/cases/finder_test.rb
+++ b/activerecord/test/cases/finder_test.rb
@@ -1189,6 +1189,11 @@ class FinderTest < ActiveRecord::TestCase
       order("author_addresses_authors.id DESC").limit(3).to_a.size
   end
 
+  def test_find_with_eager_loading_collection_and_ordering_by_collection_primary_key
+    assert_equal Post.first, Post.eager_load(comments: :ratings).
+      order("posts.id, ratings.id, comments.id").first
+  end
+
   def test_find_with_nil_inside_set_passed_for_one_attribute
     client_of = Company.
       where(client_of: [2, 1, nil],