Do not consolidate persisted in-memory records

[hovsater]

Summary

merge_target_lists is called with two arguments. A collection of
persisted records and a collection of in-memory records. If the
in-memory collection contains any persisted records we can safely
reject them as the persisted collection is already up-to-date.

Fixes #43516.

[ghiculescu]

Could you add a test and a changelog entry?

[hovsater]

@ghiculescu sure thing!

[hovsater]

@ghiculescu I've now added a test and a changelog entry as requested. 🙂

[ghiculescu]

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.)

[hovsater]

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

[ghiculescu]

Is it worth adding it as an additional test?

[hovsater]

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.

[ghiculescu]

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

[nafaabout]

why don't just do

(persisted + memory).uniq

[ghiculescu]

🤦 yeah that

[hovsater]

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).

[tiendo1011]

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

[tiendo1011]

@kevinsjoberg I feel like the root cause is the record being added to memory without respecting the association condition. Do you think it makes more sense if we fix it at the moment it tries to add the record to memory?

I think it's this line:

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

Line 469 in aa0c418

target << record

[hovsater]

@kevinsjoberg I feel like the root cause is the record being added to memory without respecting the association condition. Do you think it makes more sense if we fix it at the moment it tries to add the record to memory?

I think it's this line:

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

Line 469 in aa0c418

target << record

I was thinking the same thing initially but came to the conclusion that it can't be fixed at that point. I might be wrong here, but to my understanding scopes are not enforced at that level.

[hovsater]

I've rebased with latest main.