Merge pull request #2251 from thedarkone/update-all-order

Bring back the ability to provide :order for update_all

2 files changed, 25 insertions, 9 deletions

diff --git a/activerecord/lib/active_record/relation.rb b/activerecord/lib/active_record/relation.rb
index 2d0861d5c9..fff0ad1b83 100644
--- a/activerecord/lib/active_record/relation.rb
+++ b/activerecord/lib/active_record/relation.rb
@@ -216,17 +216,13 @@ module ActiveRecord
       if conditions || options.present?
         where(conditions).apply_finder_options(options.slice(:limit, :order)).update_all(updates)
       else
-        limit = nil
-        order = []
-        # Apply limit and order only if they're both present
-        if @limit_value.present? == @order_values.present?
-          limit = arel.limit
-          order = arel.orders
+        stmt = arel.compile_update(Arel.sql(@klass.send(:sanitize_sql_for_assignment, updates)))
+
+        if limit = arel.limit
+          stmt.take limit
         end
 
-        stmt = arel.compile_update(Arel.sql(@klass.send(:sanitize_sql_for_assignment, updates)))
-        stmt.take limit if limit
-        stmt.order(*order)
+        stmt.order(*arel.orders)
         stmt.key = table[primary_key]
         @klass.connection.update stmt.to_sql, 'SQL', bind_values
       end
diff --git a/activerecord/test/cases/persistence_test.rb b/activerecord/test/cases/persistence_test.rb
index 57d1441128..9cd07fa8a5 100644
--- a/activerecord/test/cases/persistence_test.rb
+++ b/activerecord/test/cases/persistence_test.rb
@@ -29,6 +29,26 @@ class PersistencesTest < ActiveRecord::TestCase
       end
     end
 
+    def test_update_all_doesnt_ignore_order
+      assert_equal authors(:david).id + 1, authors(:mary).id # make sure there is going to be a duplicate PK error
+      test_update_with_order_succeeds = lambda do |order|
+        begin
+          Author.order(order).update_all('id = id + 1')
+        rescue ActiveRecord::ActiveRecordError
+          false
+        end
+      end
+
+      if test_update_with_order_succeeds.call('id DESC')
+        assert !test_update_with_order_succeeds.call('id ASC') # test that this wasn't a fluke and using an incorrect order results in an exception
+      else
+        # test that we're failing because the current Arel's engine doesn't support UPDATE ORDER BY queries is using subselects instead
+        assert_sql(/\AUPDATE .+ \(SELECT .* ORDER BY id DESC\)\Z/i) do
+          test_update_with_order_succeeds.call('id DESC')
+        end
+      end
+    end
+
     def test_update_all_with_order_and_limit_updates_subset_only
       author = authors(:david)
       assert_nothing_raised do