From 9e7260da1bdc0770cf4ac547120c85ab93ff3d48 Mon Sep 17 00:00:00 2001 From: Ryuta Kamizono Date: Tue, 19 Dec 2017 20:10:51 +0900 Subject: 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. --- activerecord/CHANGELOG.md | 5 +++++ activerecord/lib/active_record/relation.rb | 8 ++++---- activerecord/test/cases/persistence_test.rb | 20 ++++++++++++++++++++ activerecord/test/cases/relations_test.rb | 2 -- 4 files changed, 29 insertions(+), 6 deletions(-) diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index c1a6d7fd3a..73970c99d0 100644 --- a/activerecord/CHANGELOG.md +++ b/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. @@ -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. diff --git a/activerecord/lib/active_record/relation.rb b/activerecord/lib/active_record/relation.rb index 66fa98f172..4df3864d07 100644 --- a/activerecord/lib/active_record/relation.rb +++ b/activerecord/lib/active_record/relation.rb @@ -10,7 +10,7 @@ module ActiveRecord 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 @@ -365,8 +365,8 @@ module ActiveRecord # # 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) @@ -384,7 +384,7 @@ module ActiveRecord 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 diff --git a/activerecord/test/cases/persistence_test.rb b/activerecord/test/cases/persistence_test.rb index 3440781fee..07c4a17fb1 100644 --- a/activerecord/test/cases/persistence_test.rb +++ b/activerecord/test/cases/persistence_test.rb @@ -76,6 +76,26 @@ class PersistenceTest < ActiveRecord::TestCase 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 diff --git a/activerecord/test/cases/relations_test.rb b/activerecord/test/cases/relations_test.rb index 675aafabda..915ff03c2c 100644 --- a/activerecord/test/cases/relations_test.rb +++ b/activerecord/test/cases/relations_test.rb @@ -896,11 +896,9 @@ class RelationTest < ActiveRecord::TestCase 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 -- cgit v1.2.3