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

Do not consolidate persisted in-memory records #43517

Merged
merged 1 commit into from
Nov 25, 2021
Merged

Do not consolidate persisted in-memory records #43517

Show file tree
Hide file tree
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
4 changes: 4 additions & 0 deletions activerecord/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
* Ignore persisted in-memory records when merging target lists.

*Kevin Sjöberg*

* Add nested_attributes_for support for `delegated_type`

```ruby
Expand Down
2 changes: 1 addition & 1 deletion 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 @@ -335,7 +335,7 @@ def merge_target_lists(persisted, memory)
end
end

persisted + memory
persisted + memory.reject(&:persisted?)
Copy link
Member

Choose a reason for hiding this comment

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

If the issue is that persisted and memory have the same objects, is this a better fix?

Suggested change
persisted + memory.reject(&:persisted?)
[persisted + memory].flatten.uniq

Copy link

Choose a reason for hiding this comment

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

why don't just do

(persisted + memory).uniq

Copy link
Member

Choose a reason for hiding this comment

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

🤦 yeah that

Copy link
Contributor Author

@hovsater hovsater Oct 26, 2021 •
edited

Choose a reason for hiding this comment

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

The issue isn't that persisted and memory contains the same records. In fact, that is to be expected. merge_target_lists already handles duplicates. The issue is that the memory collection may contain persisted records that are not present in the persisted collection. These records are currently being included even though they shouldn't. The persisted collection is already up-to-date, so if there are records in memory that are persisted but not part of persisted they are invalid (e.g., violating the association scope).

Copy link

Choose a reason for hiding this comment

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

@kevinsjoberg I think the documentation at the beginning of this method should also be updated to reflect this change.

end

def _create_record(attributes, raise = false, &block)
Expand Down
10 changes: 10 additions & 0 deletions activerecord/test/cases/associations_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -289,6 +289,16 @@ def test_reset_unloads_target
assert_not_predicate david.posts, :loaded?
assert_not_predicate david.posts, :loaded
end

def test_target_merging_ignores_persisted_in_memory_records
david = authors(:david)
assert david.thinking_posts.include?(posts(:thinking))

david.thinking_posts.create!(title: "Something else entirely", body: "Does not matter.")

assert_equal 1, david.thinking_posts.size
assert_equal 1, david.thinking_posts.to_a.size
end
Comment on lines +293 to +301
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def test_target_merging_ignores_persisted_in_memory_records
david = authors(:david)
assert david.thinking_posts.include?(posts(:thinking))
david.thinking_posts.create!(title: "Something else entirely", body: "Does not matter.")
assert_equal 1, david.thinking_posts.size
assert_equal 1, david.thinking_posts.to_a.size
end
def test_target_merging_ignores_persisted_in_memory_records
david = authors(:david)
assert david.thinking_posts.include?(posts(:thinking))
david.thinking_posts.create!(title: "Something else entirely", body: "Does not matter.")
assert_equal 1, david.thinking_posts.size
assert_equal 1, david.thinking_posts.to_a.size
end
def test_target_merging_does_not_ignores_persisted_in_memory_records_that_match_scope
david = authors(:david)
assert david.thinking_posts.include?(posts(:thinking))
david.thinking_posts.create!(title: "So I was thinking", body: "This should be included in the scope.")
assert_equal 2, david.thinking_posts.size
assert_equal 2, david.thinking_posts.to_a.size
end

Does this also work? (Mostly just want to make sure I've understood the issue correctly.)

Copy link
Contributor Author

@hovsater hovsater Oct 26, 2021 •
edited

Choose a reason for hiding this comment

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

Yes, correct. That test case would pass as well. 🙂

Copy link
Member

Choose a reason for hiding this comment

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

Is it worth adding it as an additional test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think so. That behaviour was already present prior to my change, so I would expect things to blow up left and right if that part broke. But if anyone feel strongly about it, I don't mind adding another test case.

end

class OverridingAssociationsTest < ActiveRecord::TestCase
Expand Down