Merge pull request #11958 from jetthoughts/extract_pre_process_orders_args

Re-use order arguments pre-processing for reorder

5 files changed, 52 insertions, 15 deletions

diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
index c7928e503b..b79b9a87c6 100644
--- a/activerecord/CHANGELOG.md
+++ b/activerecord/CHANGELOG.md
@@ -1,3 +1,7 @@
+*   Re-use `order` argument pre-processing for `reorder`.
+
+    *Paul Nikitochkin*
+
 *   Fix PredicateBuilder so polymorhic association keys in `where` clause can
     also accept not only `ActiveRecord::Base` direct descendances (decorated
     models, for example).
diff --git a/activerecord/lib/active_record/relation/query_methods.rb b/activerecord/lib/active_record/relation/query_methods.rb
index 9f2a039d94..1c6ea94c0b 100644
--- a/activerecord/lib/active_record/relation/query_methods.rb
+++ b/activerecord/lib/active_record/relation/query_methods.rb
@@ -289,17 +289,7 @@ module ActiveRecord
     end
 
     def order!(*args) # :nodoc:
-      args.flatten!
-      validate_order_args(args)
-
-      references = args.grep(String)
-      references.map! { |arg| arg =~ /^([a-zA-Z]\w*)\.(\w+)/ && $1 }.compact!
-      references!(references) if references.any?
-
-      # if a symbol is given we prepend the quoted table name
-      args.map! do |arg|
-        arg.is_a?(Symbol) ? Arel::Nodes::Ascending.new(klass.arel_table[arg]) : arg
-      end
+      preprocess_order_args(args)
 
       self.order_values += args
       self
@@ -320,8 +310,7 @@ module ActiveRecord
     end
 
     def reorder!(*args) # :nodoc:
-      args.flatten!
-      validate_order_args(args)
+      preprocess_order_args(args)
 
       self.reordering_value = true
       self.order_values = args
@@ -1036,6 +1025,20 @@ module ActiveRecord
       end
     end
 
+    def preprocess_order_args(order_args)
+      order_args.flatten!
+      validate_order_args(order_args)
+
+      references = order_args.grep(String)
+      references.map! { |arg| arg =~ /^([a-zA-Z]\w*)\.(\w+)/ && $1 }.compact!
+      references!(references) if references.any?
+
+      # if a symbol is given we prepend the quoted table name
+      order_args.map! do |arg|
+        arg.is_a?(Symbol) ? Arel::Nodes::Ascending.new(klass.arel_table[arg]) : arg
+      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/associations/eager_test.rb b/activerecord/test/cases/associations/eager_test.rb
index 28bf48f4fd..8d3d6962fc 100644
--- a/activerecord/test/cases/associations/eager_test.rb
+++ b/activerecord/test/cases/associations/eager_test.rb
@@ -1172,8 +1172,11 @@ class EagerAssociationTest < ActiveRecord::TestCase
     }
   end
 
-  test "works in combination with order(:symbol)" do
-    author = Author.includes(:posts).references(:posts).order(:name).where('posts.title IS NOT NULL').first
+  test "works in combination with order(:symbol) and reorder(:symbol)" do
+    author = Author.includes(:posts).references(:posts).order(:name).find_by('posts.title IS NOT NULL')
+    assert_equal authors(:bob), author
+
+    author = Author.includes(:posts).references(:posts).reorder(:name).find_by('posts.title IS NOT NULL')
     assert_equal authors(:bob), author
   end
 end
diff --git a/activerecord/test/cases/relation_test.rb b/activerecord/test/cases/relation_test.rb
index a695087426..a327b0d3e5 100644
--- a/activerecord/test/cases/relation_test.rb
+++ b/activerecord/test/cases/relation_test.rb
@@ -311,6 +311,15 @@ module ActiveRecord
       assert relation.reordering_value
     end
 
+    test '#reorder! with symbol prepends the table name' do
+      assert relation.reorder!(:name).equal?(relation)
+      node = relation.order_values.first
+
+      assert node.ascending?
+      assert_equal :name, node.expr.name
+      assert_equal "posts", node.expr.relation.name
+    end
+
     test 'reverse_order!' do
       assert relation.reverse_order!.equal?(relation)
       assert relation.reverse_order_value
diff --git a/activerecord/test/cases/relations_test.rb b/activerecord/test/cases/relations_test.rb
index e1a760d240..b3ca45c4c8 100644
--- a/activerecord/test/cases/relations_test.rb
+++ b/activerecord/test/cases/relations_test.rb
@@ -1344,6 +1344,24 @@ class RelationTest < ActiveRecord::TestCase
     assert_equal [], scope.references_values
   end
 
+  def test_automatically_added_reorder_references
+    scope = Post.reorder('comments.body')
+    assert_equal %w(comments), scope.references_values
+
+    scope = Post.reorder('comments.body', 'yaks.body')
+    assert_equal %w(comments yaks), scope.references_values
+
+    # Don't infer yaks, let's not go down that road again...
+    scope = Post.reorder('comments.body, yaks.body')
+    assert_equal %w(comments), scope.references_values
+
+    scope = Post.reorder('comments.body asc')
+    assert_equal %w(comments), scope.references_values
+
+    scope = Post.reorder('foo(comments.body)')
+    assert_equal [], scope.references_values
+  end
+
   def test_presence
     topics = Topic.all