Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
Using subselect for delete_all with limit or offset
Arel doesn't support subselect generation for DELETE unlike UPDATE yet,
but we already have that generation in connection adapters. We can
simply use the subselect generated by that one.
  • Loading branch information
kamipo committed Dec 19, 2017
1 parent 1118e2c commit 9e7260d
Show file tree
Hide file tree
Showing 4 changed files with 29 additions and 6 deletions.
5 changes: 5 additions & 0 deletions activerecord/CHANGELOG.md
@@ -1,3 +1,7 @@
* Using subselect for `delete_all` with `limit` or `offset`.

*Ryuta Kamizono*

* Undefine attribute methods on descendants when resetting column
information.

Expand Down Expand Up @@ -553,4 +557,5 @@

*Kevin McPhillips*


Please check [5-1-stable](https://github.com/rails/rails/blob/5-1-stable/activerecord/CHANGELOG.md) for previous changes.
8 changes: 4 additions & 4 deletions activerecord/lib/active_record/relation.rb
Expand Up @@ -10,7 +10,7 @@ class Relation
SINGLE_VALUE_METHODS = [:limit, :offset, :lock, :readonly, :reordering,
:reverse_order, :distinct, :create_with, :skip_query_cache]
CLAUSE_METHODS = [:where, :having, :from]
INVALID_METHODS_FOR_DELETE_ALL = [:limit, :distinct, :offset, :group, :having]
INVALID_METHODS_FOR_DELETE_ALL = [:distinct, :group, :having]

VALUE_METHODS = MULTI_VALUE_METHODS + SINGLE_VALUE_METHODS + CLAUSE_METHODS

Expand Down Expand Up @@ -365,8 +365,8 @@ def destroy_all
#
# If an invalid method is supplied, #delete_all raises an ActiveRecordError:
#
# Post.limit(100).delete_all
# # => ActiveRecord::ActiveRecordError: delete_all doesn't support limit
# Post.distinct.delete_all
# # => ActiveRecord::ActiveRecordError: delete_all doesn't support distinct
def delete_all
invalid_methods = INVALID_METHODS_FOR_DELETE_ALL.select do |method|
value = get_value(method)
Expand All @@ -384,7 +384,7 @@ def delete_all
stmt = Arel::DeleteManager.new
stmt.from(table)

if has_join_values?
if has_join_values? || has_limit_or_offset?
@klass.connection.join_to_delete(stmt, arel, arel_attribute(primary_key))
else
stmt.wheres = arel.constraints
Expand Down
20 changes: 20 additions & 0 deletions activerecord/test/cases/persistence_test.rb
Expand Up @@ -76,6 +76,26 @@ def test_update_all_with_order_and_limit_and_offset_updates_subset_only
assert_equal "bulk update!", posts(:thinking).body
assert_not_equal "bulk update!", posts(:welcome).body
end

def test_delete_all_with_order_and_limit_deletes_subset_only
author = authors(:david)
limited_posts = Post.where(author: author).order(:id).limit(1)
assert_equal 1, limited_posts.size
assert_equal 2, limited_posts.limit(2).size
assert_equal 1, limited_posts.delete_all
assert_raise(ActiveRecord::RecordNotFound) { posts(:welcome) }
assert posts(:thinking)
end

def test_delete_all_with_order_and_limit_and_offset_deletes_subset_only
author = authors(:david)
limited_posts = Post.where(author: author).order(:id).limit(1).offset(1)
assert_equal 1, limited_posts.size
assert_equal 2, limited_posts.limit(2).size
assert_equal 1, limited_posts.delete_all
assert_raise(ActiveRecord::RecordNotFound) { posts(:thinking) }
assert posts(:welcome)
end
end

def test_update_many
Expand Down
2 changes: 0 additions & 2 deletions activerecord/test/cases/relations_test.rb
Expand Up @@ -896,11 +896,9 @@ def test_delete_all_loaded
end

def test_delete_all_with_unpermitted_relation_raises_error
assert_raises(ActiveRecord::ActiveRecordError) { Author.limit(10).delete_all }
assert_raises(ActiveRecord::ActiveRecordError) { Author.distinct.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
Expand Down

0 comments on commit 9e7260d

Please sign in to comment.