Permalink
Browse files

Fix `Relation#delete_all` inconsistency

When relation scopes include one of `uniq`, `group`, `having` or
`offset`, the generated query ignores them and that causes unintended
records to be deleted. This solves the issue by restricting the deletion
when those scopes are present.

rails/rails#11985
  • Loading branch information...
1 parent f25f533 commit 5866437eefa19a38010c6045b9015b9508dd8a0d Leandro Facchinetti committed May 12, 2014
Showing with 18 additions and 4 deletions.
  1. +13 −3 activerecord/lib/active_record/relation.rb
  2. +5 −1 activerecord/test/cases/relations_test.rb
@@ -12,6 +12,7 @@ class Relation
SINGLE_VALUE_METHODS = [:limit, :offset, :lock, :readonly, :from, :reordering,
:reverse_order, :distinct, :create_with, :uniq]
+ INVALID_METHODS_FOR_DELETE_ALL = [:limit, :distinct, :offset, :group, :having]
VALUE_METHODS = MULTI_VALUE_METHODS + SINGLE_VALUE_METHODS
@@ -430,12 +431,21 @@ def destroy(id)
# If you need to destroy dependent associations or call your <tt>before_*</tt> or
# +after_destroy+ callbacks, use the +destroy_all+ method instead.
#
- # If a limit scope is supplied, +delete_all+ raises an ActiveRecord error:
+ # If an invalid method is supplied, +delete_all+ raises an ActiveRecord error:
#
# Post.limit(100).delete_all
- # # => ActiveRecord::ActiveRecordError: delete_all doesn't support limit scope
+ # # => ActiveRecord::ActiveRecordError: delete_all doesn't support limit
def delete_all(conditions = nil)
- raise ActiveRecordError.new("delete_all doesn't support limit scope") if self.limit_value
+ invalid_methods = INVALID_METHODS_FOR_DELETE_ALL.select { |method|
+ if MULTI_VALUE_METHODS.include?(method)
+ send("#{method}_values").any?
+ else
+ send("#{method}_value")
+ end
+ }
+ if invalid_methods.any?
+ raise ActiveRecordError.new("delete_all doesn't support #{invalid_methods.join(', ')}")
+ end
if conditions
where(conditions).delete_all
@@ -831,8 +831,12 @@ def test_delete_all_loaded
assert davids.loaded?
end
- def test_delete_all_limit_error
+ def test_delete_all_with_unpermitted_relation_raises_error
assert_raises(ActiveRecord::ActiveRecordError) { Author.limit(10).delete_all }
+ assert_raises(ActiveRecord::ActiveRecordError) { Author.uniq.delete_all }
+ assert_raises(ActiveRecord::ActiveRecordError) { Author.group(:name).delete_all }
+ assert_raises(ActiveRecord::ActiveRecordError) { Author.having('SUM(id) < 3').delete_all }
+ assert_raises(ActiveRecord::ActiveRecordError) { Author.offset(10).delete_all }
end
def test_select_with_aggregates

0 comments on commit 5866437

Please sign in to comment.