Merge pull request rails#46676 from Shopify/ordered-relation-respects-query-constraints

`#last` and `#first` finders should use `query_constraints` for ordering

Author: eileencodes

activerecord/lib/active_record/persistence.rb

@@ -456,13 +456,14 @@ def update!(id = :all, attributes)
       end
 
       # Accepts a list of attribute names to be used in the WHERE clause
-      # of SELECT / UPDATE / DELETE queries.
+      # of SELECT / UPDATE / DELETE queries and in the ORDER BY clause for `#first` and `#last` finder methods.
       #
       #   class Developer < ActiveRecord::Base
       #     query_constraints :company_id, :id
       #   end
       #
       #   developer = Developer.first
+      #   SELECT "developers".* FROM "developers" ORDER BY "developers"."company_id" ASC, "developers"."id" ASC LIMIT 1
       #   developer.inspect # => #<Developer id: 1, company_id: 1, ...>
       #
       #   developer.update!(name: "Nikita")

activerecord/lib/active_record/relation/finder_methods.rb

@@ -576,12 +576,12 @@ def find_last(limit)
       end
 
       def ordered_relation
-        if order_values.empty? && (implicit_order_column || primary_key)
-          if implicit_order_column && primary_key && implicit_order_column != primary_key
-            order(table[implicit_order_column].asc, table[primary_key].asc)
-          else
-            order(table[implicit_order_column || primary_key].asc)
-          end
+        if order_values.empty? && (implicit_order_column || !query_constraints_list.empty?)
+          # use query_constraints_list as the order clause if there is no implicit_order_column
+          # otherwise remove the implicit order column from the query constraints list if it's there
+          # and prepend it to the beginning of the list
+          order_columns = implicit_order_column.nil? ? query_constraints_list : ([implicit_order_column] | query_constraints_list)
+          order(*order_columns.map { |column| table[column].asc })
         else
           self
         end

activerecord/test/cases/finder_test.rb

@@ -23,13 +23,16 @@
 require "models/tyre"
 require "models/subscriber"
 require "models/non_primary_key"
+require "models/clothing_item"
 require "support/stubs/strong_parameters"
 require "support/async_helper"
 
 class FinderTest < ActiveRecord::TestCase
   include AsyncHelper
 
-  fixtures :companies, :topics, :entrants, :developers, :developers_projects, :posts, :comments, :accounts, :authors, :author_addresses, :customers, :categories, :categorizations, :cars
+  fixtures :companies, :topics, :entrants, :developers, :developers_projects,
+    :posts, :comments, :accounts, :authors, :author_addresses, :customers,
+    :categories, :categorizations, :cars, :clothing_items
 
   def test_find_by_id_with_hash
     assert_nothing_raised do
@@ -1033,6 +1036,33 @@ def test_implicit_order_for_model_without_primary_key
     NonPrimaryKey.implicit_order_column = old_implicit_order_column
   end
 
+  def test_implicit_order_column_reorders_query_constraints
+    c = ClothingItem.connection
+    ClothingItem.implicit_order_column = "color"
+    quoted_type = Regexp.escape(c.quote_table_name("clothing_items.clothing_type"))
+    quoted_color = Regexp.escape(c.quote_table_name("clothing_items.color"))
+
+    assert_sql(/ORDER BY #{quoted_color} ASC, #{quoted_type} ASC LIMIT/i) do
+      assert_kind_of ClothingItem, ClothingItem.first
+    end
+  ensure
+    ClothingItem.implicit_order_column = nil
+  end
+
+  def test_implicit_order_column_prepends_query_constraints
+    c = ClothingItem.connection
+    ClothingItem.implicit_order_column = "description"
+    quoted_type = Regexp.escape(c.quote_table_name("clothing_items.clothing_type"))
+    quoted_color = Regexp.escape(c.quote_table_name("clothing_items.color"))
+    quoted_descrption = Regexp.escape(c.quote_table_name("clothing_items.description"))
+
+    assert_sql(/ORDER BY #{quoted_descrption} ASC, #{quoted_type} ASC, #{quoted_color} ASC LIMIT/i) do
+      assert_kind_of ClothingItem, ClothingItem.first
+    end
+  ensure
+    ClothingItem.implicit_order_column = nil
+  end
+
   def test_take_and_first_and_last_with_integer_should_return_an_array
     assert_kind_of Array, Topic.take(5)
     assert_kind_of Array, Topic.first(5)
@@ -1731,6 +1761,26 @@ def test_finder_with_offset_string
     end
   end
 
+  test "#last for a model with composite query constraints" do
+    c = ClothingItem.connection
+    quoted_type = Regexp.escape(c.quote_table_name("clothing_items.clothing_type"))
+    quoted_color = Regexp.escape(c.quote_table_name("clothing_items.color"))
+
+    assert_sql(/ORDER BY #{quoted_type} DESC, #{quoted_color} DESC LIMIT/i) do
+      assert_kind_of ClothingItem, ClothingItem.last
+    end
+  end
+
+  test "#first for a model with composite query constraints" do
+    c = ClothingItem.connection
+    quoted_type = Regexp.escape(c.quote_table_name("clothing_items.clothing_type"))
+    quoted_color = Regexp.escape(c.quote_table_name("clothing_items.color"))
+
+    assert_sql(/ORDER BY #{quoted_type} ASC, #{quoted_color} ASC LIMIT/i) do
+      assert_kind_of ClothingItem, ClothingItem.first
+    end
+  end
+
   private
     def table_with_custom_primary_key
       yield(Class.new(Toy) do