Skip to content

Commit

Permalink
Bring back the ability to provide :order for update_all.
Browse files Browse the repository at this point in the history
  • Loading branch information
@thedarkone
thedarkone committed Jul 25, 2011
1 parent 4b1b9ac commit 787194e
Show file tree
Hide file tree
Showing 2 changed files with 25 additions and 9 deletions.
14 changes: 5 additions & 9 deletions activerecord/lib/active_record/relation.rb
View file
Original file line number Original file line Diff line number Diff line change
Expand Up @@ -216,17 +216,13 @@ def update_all(updates, conditions = nil, options = {})
if conditions || options.present? if conditions || options.present?
where(conditions).apply_finder_options(options.slice(:limit, :order)).update_all(updates) where(conditions).apply_finder_options(options.slice(:limit, :order)).update_all(updates)
else else
limit = nil stmt = arel.compile_update(Arel.sql(@klass.send(:sanitize_sql_for_assignment, updates)))
order = []
# Apply limit and order only if they're both present if limit = arel.limit
if @limit_value.present? == @order_values.present? stmt.take limit
limit = arel.limit
order = arel.orders
end end


stmt = arel.compile_update(Arel.sql(@klass.send(:sanitize_sql_for_assignment, updates))) stmt.order(*arel.orders)
stmt.take limit if limit
stmt.order(*order)
stmt.key = table[primary_key] stmt.key = table[primary_key]
@klass.connection.update stmt.to_sql, 'SQL', bind_values @klass.connection.update stmt.to_sql, 'SQL', bind_values
end end
Expand Down
20 changes: 20 additions & 0 deletions activerecord/test/cases/persistence_test.rb
View file
Original file line number Original file line Diff line number Diff line change
Expand Up @@ -29,6 +29,26 @@ def test_update_all_ignores_order_without_limit_from_association
end end
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 def test_update_all_with_order_and_limit_updates_subset_only
author = authors(:david) author = authors(:david)
assert_nothing_raised do assert_nothing_raised do
Expand Down

6 comments on commit 787194e

@rsim
Copy link
Contributor

@rsim rsim commented on 787194e Jul 26, 2011

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This commit broke tests on Oracle as Oracle does not allow ORDER BY in subselect of UPDATE, see http://ci.rayapps.com/job/rails-3-1-stable-activerecord-oracle/171/build_ruby=1.8.7/console

As this example represents very specific case where such ordered update would be necessary I suggest that in this case manually written SQL UPDATE statement would be used (in databases which allow such syntax) without using Arel. For example, in Oracle you could just do "UPDATE orders SET id = id + 1" and it will succeed as Oracle will validate uniqueness constraint just at the end of all UPDATE statement.

Therefore I propose that Arel should not generate SQL which semantics is too specific for one database. SQL updates are set operations and we should not think of them as sequential procedural programs :)

@thedarkone
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The success of UPDATE does depend on the ORDER BY in DBs that do support it. Maybe Arel's Oracle engine should just ignore ORDER BYs for UPDATE queries? /cc @tenderlove

@thedarkone
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Therefore I propose that Arel should not generate SQL which semantics is too specific for one database. SQL updates are set operations and we should not think of them as sequential procedural programs :)

ActiveRecord is perfectly fine generating SQL that is going to fail on Oracle, for example see the next test.

I've created a pull request for Arel rails/arel#69.

@rsim
Copy link
Contributor

@rsim rsim commented on 787194e Jul 26, 2011

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix :) Will verify if this fixes faling tests on Oracle.

@metaskills
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, this broke a few things in SQL Server too. Always finding @rsim is my partner in crime too :)

@metaskills
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well I am passing all tests again but putting this in the SQL Server adapter's included visitor.

def visit_Arel_Nodes_UpdateStatement(o)
  if o.orders.any? && o.limit.nil?
    o.limit = Nodes::Limit.new(2147483647)
  end
  super
end

Basically we are OK with orders in a sub-select/derived table if you include a top, so I just shim that in. It does feel that we are working against the grain tho. Then again, I think it is in our camp to make our visitors cope with the stuff ActiveRecord is doing for the mainstream. This is one of the reasons I have never considered working and submitting patches to Arel's mssql visitor vs building and including our own sqlserver one our adapter. Too much of our visitor is coping with what ActiveRecord does.

Anyways, thanks @thedarkone for the purposed work around.

Please sign in to comment.