Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Refactor inversing logic of CollectionAssociation #45399

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Refactor inversing logic of CollectionAssociation #45399

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
45 changes: 18 additions & 27 deletions activerecord/lib/active_record/associations/collection_association.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ def ids_writer(ids)
def reset
super
@target = []
@replaced_or_added_targets = Set.new.compare_by_identity
@implicit_target_records = nil
@association_ids = nil
end

Expand Down Expand Up @@ -116,7 +116,7 @@ def build(attributes = nil, &block)
if attributes.is_a?(Array)
attributes.collect { |attr| build(attr, &block) }
else
add_to_target(build_record(attributes, &block), replace: true)
add_to_target(build_record(attributes, &block))
end
end

Expand Down Expand Up @@ -274,21 +274,19 @@ def load_target
target
end

def add_to_target(record, skip_callbacks: false, replace: false, &block)
replace_on_target(record, skip_callbacks, replace: replace || association_scope.distinct_value, &block)
def add_to_target(record, skip_callbacks: false, &block)
add_record(record, replace: association_scope.distinct_value, skip_callbacks: skip_callbacks, &block)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this changes the behavior on build when replace: was being passed as true? Now it will use association_scope.distinct_value to determine if it should replace or not.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand correctly, replace: true was added in 50e4aaa because the following line ends up calling replace_on_target twice:

rails/activerecord/lib/active_record/associations/collection_association.rb

Line 119 in ec4558a

add_to_target(build_record(attributes, &block), replace: true)

The 1st time via target= inside the build_record call, and the 2nd time via add_to_target.

For the 2nd call, replace: true causes the following conditional to compute index:

rails/activerecord/lib/active_record/associations/collection_association.rb

Lines 449 to 452 in ec4558a

def replace_on_target(record, skip_callbacks, replace:, inversing: false)
if replace && (!record.new_record? || @replaced_or_added_targets.include?(record))
index = @target.index(record)
end

However, a "fallback" was also added in 6f6c441 to compute index anyway:

rails/activerecord/lib/active_record/associations/collection_association.rb

Lines 464 to 466 in ec4558a

if !index && @replaced_or_added_targets.include?(record)
index = @target.index(record)
end

In other words, I believe replace: true inside build is no longer necessary even on main.

That said, this PR changes how targets are tracked. With this PR, CollectionAssociation#inversed_from will override Association#inversed_from, so build_record inside build will call add_record with implicit: true instead of target=.

Thus the 2nd call to add_to_target / add_record will find the record in @implicit_target_records and replace it at index:

rails/activerecord/lib/active_record/associations/collection_association.rb

Lines 458 to 462 in ed79453

was_implicit = @implicit_target_records&.delete(record)
(@implicit_target_records ||= []) << record if implicit
if (replace || was_implicit) && index = target.index(record)
target[index] = record

It has been a long time since I looked at this code though, so I could be missing something. A while ago I asked Eileen to test this branch against the Shopify codebase, and she said there were errors but the cause wasn't clear. Unfortunately, I was not able to dig further.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. I just tried this PR again and there were some errors. Some of them is because we were calling add_to_target directly with replace: true. The others were is not clear. But points to me that some behavior is changing in this refactoring, so I was trying to see visually what would be the low hanging fruits. I can dig deeper.

end

def target=(record)
return super unless reflection.klass.has_many_inversing
def inversed_from(record)
# When removing a record from an inverse association, `inversed_from` is
# called with nil. But we need the record itself in order to remove it,
# so instead we ignore.
add_record(record, implicit: true, skip_callbacks: true) if record
end

case record
when nil
# It's not possible to remove the record from the inverse association.
when Array
super
else
replace_on_target(record, true, replace: true, inversing: true)
end
def inversed_from_queries(record)
inversed_from(record) if inversable?(record)
end

def scope
Expand Down Expand Up @@ -395,6 +393,7 @@ def remove_records(existing_records, records, method)

delete_records(existing_records, method) if existing_records.any?
@target -= records
@implicit_target_records -= records if @implicit_target_records
@association_ids = nil

records.each { |record| callback(:after_remove, record) }
Expand Down Expand Up @@ -422,8 +421,7 @@ def replace_records(new_target, original_target)
def replace_common_records_in_memory(new_target, original_target)
common_records = intersection(new_target, original_target)
common_records.each do |record|
skip_callbacks = true
replace_on_target(record, skip_callbacks, replace: true)
add_record(record, replace: true, skip_callbacks: true)
end
end

Expand All @@ -446,11 +444,7 @@ def concat_records(records, raise = false)
records
end

def replace_on_target(record, skip_callbacks, replace:, inversing: false)
if replace && (!record.new_record? || @replaced_or_added_targets.include?(record))
index = @target.index(record)
end

def add_record(record, replace: false, implicit: false, skip_callbacks: false)
catch(:abort) do
callback(:before_add, record)
end || return unless skip_callbacks
Expand All @@ -461,13 +455,10 @@ def replace_on_target(record, skip_callbacks, replace:, inversing: false)

yield(record) if block_given?

if !index && @replaced_or_added_targets.include?(record)
index = @target.index(record)
end

@replaced_or_added_targets << record if inversing || index || record.new_record?
was_implicit = @implicit_target_records&.delete(record)
(@implicit_target_records ||= []) << record if implicit
rafaelfranca marked this conversation as resolved.
Show resolved Hide resolved

if index
if (replace || was_implicit) && index = target.index(record)
target[index] = record
elsif @_was_loaded || !loaded?
@association_ids = nil
Expand Down