TEST_CODEOWNERS/rails
3
0
Fork 0
Code Issues Pull Requests 1 Actions Packages Projects Releases Wiki Activity

Fix Relation#delete_all inconsistency

Browse Source 
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
This commit is contained in:
Leandro Facchinetti 2014-05-12 12:06:35 -03:00
parent f25f5336ee
commit 5866437eef
2 changed files with 18 additions and 4 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

16
activerecord/lib/active_record/relation.rb
View File

@ -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

6
activerecord/test/cases/relations_test.rb
View File

@ -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