Fix: Preloader should no longer overwrites built records

[MaxLap]

In short, this change makes it so that using ActiveRecord::Associations::Preloader doesn't overwrite / lose state.

Right now, here is how things are:

post = Post.last
post.comments.build
ActiveRecord::Associations::Preloader.new(records: [post], associations: :comment)
post.comments.to_a

The last line would not return the comment that was built. This is unintuitive and was changed in Rails 6.0. In Rails 5.2, this code behave as one would expect and still contains the persisted comment.

Considering that the commit that introduced this behaviors was to fix a problem that, it turns out, was also fixed differently, this change is just a partial revert of said commit with no ill effects.

It would be nice to backport this to at least 7.1 since it's so new. The code hasn't changed since Rails 7.0 so that would be easy too. I'm willing to do it if there is openness to such a backport.

Investigation

In effect, this PR undoes changes made in 5f9e050 which introduced this bug.

Here is the story:

Starting after Rails 5.2

(Bug1) If you have 2 distinct but equal records given to preload, only one of them will receive the preloaded records.

(BugfixA) 9897f50
This fixes Bug1. The #uniq was removing the extra distinct but equal records, so only one of them would receive sub-records in its associations.
But there is a new bug!

(Bug2) If you have a single instance twice in the records to preload on, it receives the sub-records multiple time.

(BugfixB): 5f9e050
This fixes Bug2. And is what I am reverting.
Instead of adding the preloaded data to what was already in the association, the received data overwrites the data that was already there.
But there is a new bug!

(Bug3) If you had unpersisted data in the association being preloaded (using #build for example), it will be gone!
This is the bug this PR wants to fix.

(BugfixC) a4b20e4
This fix doesn't resolve Bug3.
But it also fixes Bug2, and does it more correctly. It does a better #uniq than the one that was removed by BugfixA due to Bug1.
Unlike BugfixB, BuugfixC doesn't introduce Bug3.

So at this point, it's safe to undo BugfixB, at least the part that causes Bug3:

• Bug2 doesn't get brought back because BugfixC also fixes it.

This is what this PR is doing, while also adding tests.

I'd like to thank git bisect for making this investigation so much simpler than it would have been otherwise xD

[MaxLap]

@kamipo Could you take a look? You made BugfixB (which I'm partially reverting) and you merged the PRs for BugfixA and BugfixC which interact with this. Thank you

[MaxLap]

I did not notice that kamipo was not really active anymore. Could someone else provide feedback?

[skipkayhil]

The best way to get feedback would be to bring it up on Discord

[tenderlove]

At first glance, this seems fine to me. My only hesitation is that the preloader isn't supposed to be public API. Is there a way to tickle this bug without directly calling the preloader? IOW is it possible for this bug to occur via public APIs?

[MaxLap]

Thank you for the feedback!

I don't think it's possible with only public API to trigger this bug.

Having an array of records that need some form of mass loading of an association is a super frequent need when dealing with a complex application. I encompass this into a #preload_now function on Array. Just in my codebase, I have 80 uses of it.

As #39156 mentions, Preloader is not public API but is popular anyways. In my opinion, the only reason it's not a public API is that you don't get much requests for it mainly because it's pretty easy to use even as a private API. If it was painful, there would be more pressure for it to be a public API.

I can't think of a situation where the current behavior makes sense other than a "it will never happen". Should Preloader ever become a public API, then this would become an obvious problem.

[MaxLap]

Any chance this can be cherry-picked back to 7-1-stable? 7.2 is a long way to go. I just tested it and it's a clean cherry-pick without conflicts. Thank!

[jhawthorn]

Sorry, this isn't the type of change we should be backporting. Even if the API was public it could change expectations of users (and I could see an argument that the previous behaviour wasn't a bug)