Skip to content

Commit d7fad4b

Browse files
Edouard-chinmrpasquini
authored andcommitted
Fix inconsistency between delete_all & update_all allowed methods:
- At the moment, `delete_all` doesn't support `WITH`, `WITH RECURSIVE` and `DISTINCT` statement. Calling `Post.with(ex: Post.where(title: "")).delete_all` raises an error. However calling `Post.with(ex: Post.where(title: "")).update_all` executes the following SQL `UPDATE "posts" SET "title" = blabla`, which can be surprising for users. This commit adds a deprecation message to warn users that those statements have no effect, with the intention of raising the same error as when using `delete_all` in a future Rails release.
1 parent ad7bd38 commit d7fad4b

File tree

3 files changed

+35
-2
lines changed

3 files changed

+35
-2
lines changed

activerecord/CHANGELOG.md

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,14 @@
1+
* Deprecate usage of unsupported methods in conjunction with `update_all`:
2+
3+
`update_all` will now print a deprecation message if a query includes either `WITH`,
4+
`WITH RECURSIVE` or `DISTINCT` statements. Those were never supported and were ignored
5+
when generating the SQL query.
6+
7+
An error will be raised in a future Rails release. This behaviour will be consistent
8+
with `delete_all` which currently raises an error for unsupported statements.
9+
10+
*Edouard Chin*
11+
112
* The table columns inside `schema.rb` are now sorted alphabetically.
213

314
Previously they'd be sorted by creation order, which can cause merge conflicts when two

activerecord/lib/active_record/relation.rb

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -60,7 +60,7 @@ def exec_explain(&block)
6060
:reverse_order, :distinct, :create_with, :skip_query_cache]
6161

6262
CLAUSE_METHODS = [:where, :having, :from]
63-
INVALID_METHODS_FOR_DELETE_ALL = [:distinct, :with, :with_recursive]
63+
INVALID_METHODS_FOR_UPDATE_AND_DELETE_ALL = [:distinct, :with, :with_recursive]
6464

6565
VALUE_METHODS = MULTI_VALUE_METHODS + SINGLE_VALUE_METHODS + CLAUSE_METHODS
6666

@@ -590,6 +590,18 @@ def update_all(updates)
590590

591591
return 0 if @none
592592

593+
invalid_methods = INVALID_METHODS_FOR_UPDATE_AND_DELETE_ALL.select do |method|
594+
value = @values[method]
595+
method == :distinct ? value : value&.any?
596+
end
597+
if invalid_methods.any?
598+
ActiveRecord.deprecator.warn <<~MESSAGE
599+
`#{invalid_methods.join(', ')}` is not supported by `update_all` and was never included in the generated query.
600+
601+
Calling `#{invalid_methods.join(', ')}` with `update_all` will raise an error in Rails 8.2.
602+
MESSAGE
603+
end
604+
593605
if updates.is_a?(Hash)
594606
if model.locking_enabled? &&
595607
!updates.key?(model.locking_column) &&
@@ -1011,7 +1023,7 @@ def destroy_all
10111023
def delete_all
10121024
return 0 if @none
10131025

1014-
invalid_methods = INVALID_METHODS_FOR_DELETE_ALL.select do |method|
1026+
invalid_methods = INVALID_METHODS_FOR_UPDATE_AND_DELETE_ALL.select do |method|
10151027
value = @values[method]
10161028
method == :distinct ? value : value&.any?
10171029
end

activerecord/test/cases/relation/update_all_test.rb

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -75,6 +75,16 @@ def test_update_all_with_joins
7575
end
7676
end
7777

78+
def test_update_all_with_unpermitted_relation_raises_error
79+
assert_deprecated("`distinct` is not supported by `update_all`", ActiveRecord.deprecator) do
80+
Author.distinct.update_all(name: "Bob")
81+
end
82+
83+
assert_deprecated("`with` is not supported by `update_all`", ActiveRecord.deprecator) do
84+
Author.with(limited: Author.where(name: "")).update_all(name: "Bob")
85+
end
86+
end
87+
7888
def test_update_all_with_left_joins
7989
pets = Pet.left_joins(:toys).where(toys: { name: "Bone" })
8090

0 commit comments

Comments
 (0)