Skip to content

Commit

Permalink
Revert "Reset relations after insert_all/upsert_all"
Browse files Browse the repository at this point in the history
We're reverting this change to avoid using the `.scope` hack. We'll need to
start the work on fixing the callsites with relied on the implicit bug
with `.insert_all`, but in the meanwhile we will revert this commit and the
addition of `.scope` to get the upgrades rolling.
  • Loading branch information
george-ma committed Jan 20, 2025
1 parent 04a950d commit da1d5a6
Show file tree
Hide file tree
Showing 6 changed files with 5 additions and 57 deletions.
6 changes: 0 additions & 6 deletions activerecord/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -54,12 +54,6 @@
*Mike Dalessio*
* Reset relations after `insert_all`/`upsert_all`.
Bulk insert/upsert methods will now call `reset` if used on a relation, matching the behavior of `update_all`.
*Milo Winningham*
* Use `_N` as a parallel tests databases suffixes
Peviously, `-N` was used as a suffix. This can cause problems for RDBMSes
Expand Down
2 changes: 1 addition & 1 deletion activerecord/lib/active_record/association_relation.rb
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ def ==(other)

%w(insert insert_all insert! insert_all! upsert upsert_all).each do |method|
class_eval <<~RUBY, __FILE__, __LINE__ + 1
def #{method}(...)
def #{method}(attributes, **kwargs)
if @association.reflection.through_reflection?
raise ArgumentError, "Bulk insert or upsert is currently not supported for has_many through association"
end
Expand Down
12 changes: 3 additions & 9 deletions activerecord/lib/active_record/associations/collection_proxy.rb
Original file line number Diff line number Diff line change
Expand Up @@ -1125,20 +1125,14 @@ def pretty_print(pp) # :nodoc:
super
end

%w(insert insert_all insert! insert_all! upsert upsert_all).each do |method|
class_eval <<~RUBY, __FILE__, __LINE__ + 1
def #{method}(...)
scope.#{method}(...).tap { reset }
end
RUBY
end

delegate_methods = [
QueryMethods,
SpawnMethods,
].flat_map { |klass|
klass.public_instance_methods(false)
} - self.public_instance_methods(false) - [ :select ] + [ :scoping, :values, :load_async ]
} - self.public_instance_methods(false) - [:select] + [
:scoping, :values, :insert, :insert_all, :insert!, :insert_all!, :upsert, :upsert_all, :load_async
]

delegate(*delegate_methods, to: :scope)

Expand Down
2 changes: 1 addition & 1 deletion activerecord/lib/active_record/insert_all.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ class << self
def execute(relation, ...)
relation.model.with_connection do |c|
new(relation, c, ...).execute
end.tap { relation.reset }
end
end
end

Expand Down
22 changes: 0 additions & 22 deletions activerecord/test/cases/collection_cache_key_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -96,28 +96,6 @@ class CollectionCacheKeyTest < ActiveRecord::TestCase
assert_match(/\Acomments\/query-(\h+)-(\d+)-(\d+)\z/, comments.cache_key)
end

test "insert_all will update cache_key" do
skip unless supports_insert_on_duplicate_skip?

developers = Developer.all
cache_key = developers.cache_key

developers.insert_all([{ name: "Alice" }, { name: "Bob" }])

assert_not_equal cache_key, developers.cache_key
end

test "upsert_all will update cache_key" do
skip unless supports_insert_on_duplicate_update?

developers = Developer.all
cache_key = developers.cache_key

developers.upsert_all([{ id: 1, name: "Alice" }, { id: 2, name: "Bob" }])

assert_not_equal cache_key, developers.cache_key
end

test "update_all will update cache_key" do
developers = Developer.where(name: "David")
cache_key = developers.cache_key
Expand Down
18 changes: 0 additions & 18 deletions activerecord/test/cases/insert_all_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -729,14 +729,6 @@ def test_insert_all_on_relation_precedence
end
end

def test_insert_all_resets_relation
audit_logs = Developer.create!(name: "Alice").audit_logs.load

assert_changes "audit_logs.loaded?", from: true, to: false do
audit_logs.insert_all!([{ message: "event" }])
end
end

def test_insert_all_create_with
assert_difference "Book.where(format: 'X').count", +2 do
Book.create_with(format: "X").insert_all!([ { name: "A" }, { name: "B" } ])
Expand Down Expand Up @@ -769,16 +761,6 @@ def test_upsert_all_on_relation_precedence
end
end

def test_upsert_all_resets_relation
skip unless supports_insert_on_duplicate_update?

audit_logs = Developer.create!(name: "Alice").audit_logs.load

assert_changes "audit_logs.loaded?", from: true, to: false do
audit_logs.upsert_all([{ id: 1, message: "event" }])
end
end

def test_upsert_all_create_with
skip unless supports_insert_on_duplicate_update?

Expand Down

0 comments on commit da1d5a6

Please sign in to comment.