fix: duplicate active record objects on inverse_of #43445
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fixes a bug that causes duplicate references to the same object to be added to the collection association on an active_record object when inverse_of is used. In cases where has_many_inversing is set to true, the set_inverse_instance function calls target= on collection_association during concat resulting in multiple appends to target. This only occurs for new records. This PR introduces changes that sets the index so the duplicate object replaces the original. Closes issue rails#43222. Co-authored-by: Terence Li <terence.li@shopify.com> Co-authored-by: Dave Rose <dave.rose@shopify.com>
rafaelfranca
pushed a commit
that referenced
this pull request
Oct 13, 2021
fix: duplicate active record objects on inverse_of
jonathanhefner
added a commit
to jonathanhefner/rails
that referenced
this pull request
Jun 18, 2022
Follow-up to rails#40379, rails#42524, and rails#43445. The logic to associate records with a `CollectionAssociation` when using `has_many` inversing has become increasingly complex. Prior to this commit, we defined an "add record" method (`add_to_target`) that can accept a `replace: true` option, and a "replace record" method (`replace_on_target`) that can accept a `replace: false` option. And regardless of the value of the `:replace` option, the record may be added if it cannot be replaced (i.e. does not exist), or replaced if a complex set of conditions dictate it should not be simply added. This commit aims to simplify this logic by: * Removing the `:replace` option from `add_to_target`. * Renaming `replace_on_target` to a low-level `add_record` method, supporting a `:replace` option. The name `add_record` was chosen for parity with the similarly low-level `remove_records` method. * Replacing the complex conditions of `replace_on_target` with an "implicit target records" concept. Implicit target records are those that have been associated via inversing but not explicitly added. Implicit target records exist in the `@target` array, and are also tracked via an `@implicit_target_records` array. Whenever an implicit target record is explicitly added, it is instead removed from the `@implicit_target_records` array, to prevent a double occurrence in the `@target` array. (Though if the record is explicitly added a 2nd time, a 2nd occurrence *will* be added to the `@target` array. We have tests that expect this.) Additionally, this commit provides a very small speed increase, according to the following benchmark: ```ruby require "benchmark" ActiveRecord::Base.logger = nil ActiveRecord::Schema.define do create_table :authors, force: true do |t| end create_table :posts, force: true do |t| t.references :author end end class Author < ActiveRecord::Base has_many :posts end class Post < ActiveRecord::Base belongs_to :author end POST_COUNTS = [1, 10, 100, 1000] ITERATIONS = 100 CALLS_PER_ITERATION = 100 eval <<~RUBY def ms_per_build(posts) Benchmark.ms { #{"posts.build;" * CALLS_PER_ITERATION} }.to_f / CALLS_PER_ITERATION end def ms_per_shovel(posts, post) Benchmark.ms { #{"posts << post;" * CALLS_PER_ITERATION} }.to_f / CALLS_PER_ITERATION end RUBY def report(label, &block) [false, true].each do |has_many_inversing| ActiveRecord::Base.has_many_inversing = has_many_inversing POST_COUNTS.each do |post_count| Post.delete_all Post.insert_all(post_count.times.map { { author_id: 1 } }) block.call # warmup ms = ITERATIONS.times.sum(&block).to_f / ITERATIONS puts "has_many_inversing == #{has_many_inversing}, posts.count == #{post_count}".ljust(50) + "=> `#{label}` @ #{"%.3f" % ms}ms" end end end author = Author.create!(id: 1) report("posts.build") do posts = author.reload.posts.tap(&:load_target) ms_per_build(posts) end report("posts << post") do posts = author.reload.posts.tap(&:load_target) post = posts[posts.length / 2] ms_per_shovel(posts, post) end ``` Results with `main` branch: ``` has_many_inversing == false, posts.count == 1 => `posts.build` @ 0.154ms has_many_inversing == false, posts.count == 10 => `posts.build` @ 0.142ms has_many_inversing == false, posts.count == 100 => `posts.build` @ 0.141ms has_many_inversing == false, posts.count == 1000 => `posts.build` @ 0.144ms has_many_inversing == true, posts.count == 1 => `posts.build` @ 0.143ms has_many_inversing == true, posts.count == 10 => `posts.build` @ 0.142ms has_many_inversing == true, posts.count == 100 => `posts.build` @ 0.142ms has_many_inversing == true, posts.count == 1000 => `posts.build` @ 0.147ms has_many_inversing == false, posts.count == 1 => `posts << post` @ 0.192ms has_many_inversing == false, posts.count == 10 => `posts << post` @ 0.192ms has_many_inversing == false, posts.count == 100 => `posts << post` @ 0.202ms has_many_inversing == false, posts.count == 1000 => `posts << post` @ 0.196ms has_many_inversing == true, posts.count == 1 => `posts << post` @ 0.209ms has_many_inversing == true, posts.count == 10 => `posts << post` @ 0.204ms has_many_inversing == true, posts.count == 100 => `posts << post` @ 0.221ms has_many_inversing == true, posts.count == 1000 => `posts << post` @ 0.195ms ``` Results with this branch: ``` has_many_inversing == false, posts.count == 1 => `posts.build` @ 0.144ms has_many_inversing == false, posts.count == 10 => `posts.build` @ 0.132ms has_many_inversing == false, posts.count == 100 => `posts.build` @ 0.133ms has_many_inversing == false, posts.count == 1000 => `posts.build` @ 0.135ms has_many_inversing == true, posts.count == 1 => `posts.build` @ 0.132ms has_many_inversing == true, posts.count == 10 => `posts.build` @ 0.133ms has_many_inversing == true, posts.count == 100 => `posts.build` @ 0.132ms has_many_inversing == true, posts.count == 1000 => `posts.build` @ 0.140ms has_many_inversing == false, posts.count == 1 => `posts << post` @ 0.200ms has_many_inversing == false, posts.count == 10 => `posts << post` @ 0.187ms has_many_inversing == false, posts.count == 100 => `posts << post` @ 0.188ms has_many_inversing == false, posts.count == 1000 => `posts << post` @ 0.191ms has_many_inversing == true, posts.count == 1 => `posts << post` @ 0.187ms has_many_inversing == true, posts.count == 10 => `posts << post` @ 0.188ms has_many_inversing == true, posts.count == 100 => `posts << post` @ 0.197ms has_many_inversing == true, posts.count == 1000 => `posts << post` @ 0.189ms ```
jonathanhefner
added a commit
to jonathanhefner/rails
that referenced
this pull request
Sep 23, 2022
Follow-up to rails#40379, rails#42524, and rails#43445. The logic to associate records with a `CollectionAssociation` when using `has_many` inversing has become increasingly complex. Prior to this commit, we defined an "add record" method (`add_to_target`) that can accept a `replace: true` option, and a "replace record" method (`replace_on_target`) that can accept a `replace: false` option. And regardless of the value of the `:replace` option, the record may be added if it cannot be replaced (i.e. does not exist), or replaced if a complex set of conditions dictate it should not be simply added. This commit aims to simplify this logic by: * Removing the `:replace` option from `add_to_target`. * Renaming `replace_on_target` to a low-level `add_record` method, supporting a `:replace` option. The name `add_record` was chosen for parity with the similarly low-level `remove_records` method. * Replacing the complex conditions of `replace_on_target` with an "implicit target records" concept. Implicit target records are those that have been associated via inversing but not explicitly added. Implicit target records exist in the `@target` array, and are also tracked via an `@implicit_target_records` array. Whenever an implicit target record is explicitly added, it is always replaced in the `@target` array to prevent a double occurrence, and then removed from the `@implicit_target_records` array. (Thus if the record is explicitly added a 2nd time, a 2nd occurrence *will* be added to the `@target` array. We have tests that expect this.) Additionally, this commit provides a very small speed increase, according to the following benchmark: ```ruby require "benchmark" ActiveRecord::Base.logger = nil ActiveRecord::Schema.define do create_table :authors, force: true do |t| end create_table :posts, force: true do |t| t.references :author end end class Author < ActiveRecord::Base has_many :posts end class Post < ActiveRecord::Base belongs_to :author end POST_COUNTS = [1, 10, 100, 1000] ITERATIONS = 100 CALLS_PER_ITERATION = 100 eval <<~RUBY def ms_per_build(posts) Benchmark.ms { #{"posts.build;" * CALLS_PER_ITERATION} }.to_f / CALLS_PER_ITERATION end def ms_per_shovel(posts, post) Benchmark.ms { #{"posts << post;" * CALLS_PER_ITERATION} }.to_f / CALLS_PER_ITERATION end RUBY def report(label, &block) [false, true].each do |has_many_inversing| ActiveRecord::Base.has_many_inversing = has_many_inversing POST_COUNTS.each do |post_count| Post.delete_all Post.insert_all(post_count.times.map { { author_id: 1 } }) block.call # warmup ms = ITERATIONS.times.sum(&block).to_f / ITERATIONS puts "has_many_inversing == #{has_many_inversing}, posts.count == #{post_count}".ljust(50) + "=> `#{label}` @ #{"%.3f" % ms}ms" end end end author = Author.create!(id: 1) report("posts.build") do posts = author.reload.posts.tap(&:load_target) ms_per_build(posts) end report("posts << post") do posts = author.reload.posts.tap(&:load_target) post = posts[posts.length / 2] ms_per_shovel(posts, post) end ``` Results with `main` branch: ``` has_many_inversing == false, posts.count == 1 => `posts.build` @ 0.154ms has_many_inversing == false, posts.count == 10 => `posts.build` @ 0.142ms has_many_inversing == false, posts.count == 100 => `posts.build` @ 0.141ms has_many_inversing == false, posts.count == 1000 => `posts.build` @ 0.144ms has_many_inversing == true, posts.count == 1 => `posts.build` @ 0.143ms has_many_inversing == true, posts.count == 10 => `posts.build` @ 0.142ms has_many_inversing == true, posts.count == 100 => `posts.build` @ 0.142ms has_many_inversing == true, posts.count == 1000 => `posts.build` @ 0.147ms has_many_inversing == false, posts.count == 1 => `posts << post` @ 0.192ms has_many_inversing == false, posts.count == 10 => `posts << post` @ 0.192ms has_many_inversing == false, posts.count == 100 => `posts << post` @ 0.202ms has_many_inversing == false, posts.count == 1000 => `posts << post` @ 0.196ms has_many_inversing == true, posts.count == 1 => `posts << post` @ 0.209ms has_many_inversing == true, posts.count == 10 => `posts << post` @ 0.204ms has_many_inversing == true, posts.count == 100 => `posts << post` @ 0.221ms has_many_inversing == true, posts.count == 1000 => `posts << post` @ 0.195ms ``` Results with this branch: ``` has_many_inversing == false, posts.count == 1 => `posts.build` @ 0.144ms has_many_inversing == false, posts.count == 10 => `posts.build` @ 0.132ms has_many_inversing == false, posts.count == 100 => `posts.build` @ 0.133ms has_many_inversing == false, posts.count == 1000 => `posts.build` @ 0.135ms has_many_inversing == true, posts.count == 1 => `posts.build` @ 0.132ms has_many_inversing == true, posts.count == 10 => `posts.build` @ 0.133ms has_many_inversing == true, posts.count == 100 => `posts.build` @ 0.132ms has_many_inversing == true, posts.count == 1000 => `posts.build` @ 0.140ms has_many_inversing == false, posts.count == 1 => `posts << post` @ 0.200ms has_many_inversing == false, posts.count == 10 => `posts << post` @ 0.187ms has_many_inversing == false, posts.count == 100 => `posts << post` @ 0.188ms has_many_inversing == false, posts.count == 1000 => `posts << post` @ 0.191ms has_many_inversing == true, posts.count == 1 => `posts << post` @ 0.187ms has_many_inversing == true, posts.count == 10 => `posts << post` @ 0.188ms has_many_inversing == true, posts.count == 100 => `posts << post` @ 0.197ms has_many_inversing == true, posts.count == 1000 => `posts << post` @ 0.189ms ```
rafaelfranca
pushed a commit
to jonathanhefner/rails
that referenced
this pull request
Apr 22, 2024
Follow-up to rails#40379, rails#42524, and rails#43445. The logic to associate records with a `CollectionAssociation` when using `has_many` inversing has become increasingly complex. Prior to this commit, we defined an "add record" method (`add_to_target`) that can accept a `replace: true` option, and a "replace record" method (`replace_on_target`) that can accept a `replace: false` option. And regardless of the value of the `:replace` option, the record may be added if it cannot be replaced (i.e. does not exist), or replaced if a complex set of conditions dictate it should not be simply added. This commit aims to simplify this logic by: * Removing the `:replace` option from `add_to_target`. * Renaming `replace_on_target` to a low-level `add_record` method, supporting a `:replace` option. The name `add_record` was chosen for parity with the similarly low-level `remove_records` method. * Replacing the complex conditions of `replace_on_target` with an "implicit target records" concept. Implicit target records are those that have been associated via inversing but not explicitly added. Implicit target records exist in the `@target` array, and are also tracked via an `@implicit_target_records` array. Whenever an implicit target record is explicitly added, it is always replaced in the `@target` array to prevent a double occurrence, and then removed from the `@implicit_target_records` array. (Thus if the record is explicitly added a 2nd time, a 2nd occurrence *will* be added to the `@target` array. We have tests that expect this.) Additionally, this commit provides a very small speed increase, according to the following benchmark: ```ruby require "benchmark" ActiveRecord::Base.logger = nil ActiveRecord::Schema.define do create_table :authors, force: true do |t| end create_table :posts, force: true do |t| t.references :author end end class Author < ActiveRecord::Base has_many :posts end class Post < ActiveRecord::Base belongs_to :author end POST_COUNTS = [1, 10, 100, 1000] ITERATIONS = 100 CALLS_PER_ITERATION = 100 eval <<~RUBY def ms_per_build(posts) Benchmark.ms { #{"posts.build;" * CALLS_PER_ITERATION} }.to_f / CALLS_PER_ITERATION end def ms_per_shovel(posts, post) Benchmark.ms { #{"posts << post;" * CALLS_PER_ITERATION} }.to_f / CALLS_PER_ITERATION end RUBY def report(label, &block) [false, true].each do |has_many_inversing| ActiveRecord::Base.has_many_inversing = has_many_inversing POST_COUNTS.each do |post_count| Post.delete_all Post.insert_all(post_count.times.map { { author_id: 1 } }) block.call # warmup ms = ITERATIONS.times.sum(&block).to_f / ITERATIONS puts "has_many_inversing == #{has_many_inversing}, posts.count == #{post_count}".ljust(50) + "=> `#{label}` @ #{"%.3f" % ms}ms" end end end author = Author.create!(id: 1) report("posts.build") do posts = author.reload.posts.tap(&:load_target) ms_per_build(posts) end report("posts << post") do posts = author.reload.posts.tap(&:load_target) post = posts[posts.length / 2] ms_per_shovel(posts, post) end ``` Results with `main` branch: ``` has_many_inversing == false, posts.count == 1 => `posts.build` @ 0.154ms has_many_inversing == false, posts.count == 10 => `posts.build` @ 0.142ms has_many_inversing == false, posts.count == 100 => `posts.build` @ 0.141ms has_many_inversing == false, posts.count == 1000 => `posts.build` @ 0.144ms has_many_inversing == true, posts.count == 1 => `posts.build` @ 0.143ms has_many_inversing == true, posts.count == 10 => `posts.build` @ 0.142ms has_many_inversing == true, posts.count == 100 => `posts.build` @ 0.142ms has_many_inversing == true, posts.count == 1000 => `posts.build` @ 0.147ms has_many_inversing == false, posts.count == 1 => `posts << post` @ 0.192ms has_many_inversing == false, posts.count == 10 => `posts << post` @ 0.192ms has_many_inversing == false, posts.count == 100 => `posts << post` @ 0.202ms has_many_inversing == false, posts.count == 1000 => `posts << post` @ 0.196ms has_many_inversing == true, posts.count == 1 => `posts << post` @ 0.209ms has_many_inversing == true, posts.count == 10 => `posts << post` @ 0.204ms has_many_inversing == true, posts.count == 100 => `posts << post` @ 0.221ms has_many_inversing == true, posts.count == 1000 => `posts << post` @ 0.195ms ``` Results with this branch: ``` has_many_inversing == false, posts.count == 1 => `posts.build` @ 0.144ms has_many_inversing == false, posts.count == 10 => `posts.build` @ 0.132ms has_many_inversing == false, posts.count == 100 => `posts.build` @ 0.133ms has_many_inversing == false, posts.count == 1000 => `posts.build` @ 0.135ms has_many_inversing == true, posts.count == 1 => `posts.build` @ 0.132ms has_many_inversing == true, posts.count == 10 => `posts.build` @ 0.133ms has_many_inversing == true, posts.count == 100 => `posts.build` @ 0.132ms has_many_inversing == true, posts.count == 1000 => `posts.build` @ 0.140ms has_many_inversing == false, posts.count == 1 => `posts << post` @ 0.200ms has_many_inversing == false, posts.count == 10 => `posts << post` @ 0.187ms has_many_inversing == false, posts.count == 100 => `posts << post` @ 0.188ms has_many_inversing == false, posts.count == 1000 => `posts << post` @ 0.191ms has_many_inversing == true, posts.count == 1 => `posts << post` @ 0.187ms has_many_inversing == true, posts.count == 10 => `posts << post` @ 0.188ms has_many_inversing == true, posts.count == 100 => `posts << post` @ 0.197ms has_many_inversing == true, posts.count == 1000 => `posts << post` @ 0.189ms ```
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes issue #43222.
Summary
Fixes a bug that causes duplicate references to the same object to be
added to the collection association on an active_record object when
inverse_of is used.
In cases where has_many_inversing is set to true, the
set_inverse_instance function calls target= on collection_association during concat
resulting in multiple appends to target. This only occurs for new records. This
PR introduces changes that sets the index so the duplicate object
replaces the original.
Other Information