Skip to content

Commit

Permalink
Fix inconsistency between delete_all & update_all allowed methods:
Browse files Browse the repository at this point in the history
- 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.
  • Loading branch information
Edouard-chin committed Jan 14, 2025
1 parent e7c0592 commit 1585064
Show file tree
Hide file tree
Showing 3 changed files with 35 additions and 2 deletions.
11 changes: 11 additions & 0 deletions activerecord/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,14 @@
* Deprecate usage of unsupported methods in conjunction with `update_all`:

`update_all` will now print a deprecation message if a query includes either `WITH`,
`WITH RECURSIVE` or `DISTINCT` statements. Those were never supported and were ignored
when generating the SQL query.

An error will be raised in a future Rails release. This behaviour will be consistent
with `delete_all` which currently raises an error for unsupported statements.

*Edouard Chin*

* The table columns inside `schema.rb` are now sorted alphabetically.

Previously they'd be sorted by creation order, which can cause merge conflicts when two
Expand Down
16 changes: 14 additions & 2 deletions activerecord/lib/active_record/relation.rb
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ def exec_explain(&block)
:reverse_order, :distinct, :create_with, :skip_query_cache]

CLAUSE_METHODS = [:where, :having, :from]
INVALID_METHODS_FOR_DELETE_ALL = [:distinct, :with, :with_recursive]
INVALID_METHODS_FOR_UPDATE_AND_DELETE_ALL = [:distinct, :with, :with_recursive]

VALUE_METHODS = MULTI_VALUE_METHODS + SINGLE_VALUE_METHODS + CLAUSE_METHODS

Expand Down Expand Up @@ -590,6 +590,18 @@ def update_all(updates)

return 0 if @none

invalid_methods = INVALID_METHODS_FOR_UPDATE_AND_DELETE_ALL.select do |method|
value = @values[method]
method == :distinct ? value : value&.any?
end
if invalid_methods.any?
ActiveRecord.deprecator.warn <<~MESSAGE
`#{invalid_methods.join(', ')}` is not supported by `update_all` and was never included in the generated query.
Calling `#{invalid_methods.join(', ')}` with `update_all` will raise an error in Rails 8.2.
MESSAGE
end

if updates.is_a?(Hash)
if model.locking_enabled? &&
!updates.key?(model.locking_column) &&
Expand Down Expand Up @@ -1011,7 +1023,7 @@ def destroy_all
def delete_all
return 0 if @none

invalid_methods = INVALID_METHODS_FOR_DELETE_ALL.select do |method|
invalid_methods = INVALID_METHODS_FOR_UPDATE_AND_DELETE_ALL.select do |method|
value = @values[method]
method == :distinct ? value : value&.any?
end
Expand Down
10 changes: 10 additions & 0 deletions activerecord/test/cases/relation/update_all_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,16 @@ def test_update_all_with_joins
end
end

def test_update_all_with_unpermitted_relation_raises_error
assert_deprecated("`distinct` is not supported by `update_all`", ActiveRecord.deprecator) do
Author.distinct.update_all(name: "Bob")
end

assert_deprecated("`with` is not supported by `update_all`", ActiveRecord.deprecator) do
Author.with(limited: Author.where(name: "")).update_all(name: "Bob")
end
end

def test_update_all_with_left_joins
pets = Pet.left_joins(:toys).where(toys: { name: "Bone" })

Expand Down

0 comments on commit 1585064

Please sign in to comment.