Reversed attribute precedence in insert_all

[kobsy]

Summary

Rails 6.1 (via #38899) introduced incorporating relationship scopes into the attributes inserted when using insert_all/upsert_all. This allowed author.books.insert_all({ title: "Out of the Silent Planet" }) to work as expected. Neat!

However, the attributes from the scope were combined with attributes passed in as arguments with merge!. This meant that attributes on the scope would overwrite those passed in, which can be surprising in some cases, especially when using default scopes.

The example in the test for attribute precedence prior to this PR was a bit contrived:

assert_difference "author.books.count", +1 do
  author.books.insert_all({ title: "Out of the Silent Planet", isbn: "XXX", author_id: second_author.id })
end

... but one could perhaps see the logic of it: author.books as the scope is a strong contender for what should win out when inserting the new record.

However, I believe it's more surprising that the attributes explicitly passed in to insert_all don't ultimately take precedence; these seem to be the stronger indicator of intent, more even than the scope. A different example illustrates what I mean:

class Book < ActiveRecord
  default_scope where(archived_at: nil)

  def archived?
    archived_at.nil?
  end

end

book = Book.insert_all({ title: "Perelandra", archived_at: Time.now })
book.archived?
# => false

In this example, books may be archived using a timestamp. By default, we only ever want to work with un-archived books, and so we've set up a default_scope to accomplish this (it could just as easily be a scope on a relationship as well, e.g., an Author has_many :books, ->{ where(archived_at: nil) }). But should we want to insert a book as already archived, passing in a timestamp for archived_at is not enough! We also have to remember to also call unscope(where: :archived_at) before insert_all. That's surprising.

What I suggest is reversing the precedence and deferring to the attributes passed into the method over what we get from the scope (i.e., using reverse_merge! instead of merge!); the attributes passed in are stronger signals of intent than the scope on which one calls insert_all.

This would be a reversal from current behavior, though I don't believe the current behavior is noted in the documentation (cf. #41639). It would, however, make other attributes more consistent with the automatically-added timestamps, which already use reverse_merge! to defer to the passed-in attributes over the default values (cf. #43003).

Thanks for your thoughts and consideration!

[boblail]

I believe it's more surprising that the attributes explicitly passed in to insert_all don't ultimately take precedence; these seem to be the stronger indicator of intent, more even than the scope.

✅ I would agree!

[rails-bot]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

[kobsy]

Bump. Any further thoughts on this change? 😅

[skipkayhil]

I think I agree that it makes sense to be able to override the scope, however if we were to change insert_all we would definitely need to make sure all of the other query methods work the same.

See #35638 (review), currently insert_all and create both work the same way and there may be others as well

Because this would be a breaking change I'm sure it would have to go through a deprecation cycle as well (probably a config/framework default)

[kobsy]

Thanks for the great feedback, @skipkayhil. I took a closer look at how create works to pull attributes in from the scope. By my reading, neither the current behavior of a blanket merge! of scope_attributes nor my originally-proposed reverse_merge! fully matches create. Instead, create (via initialize_attributes in association.rb) effectively merges the association keys, regardless of the attributes passed in. Otherwise it reverse merges the remaining scope attributes into the passed-in attributes.

So author.books.published.create(author_id: another_author.id, status: :proposed) (to use the models in the test schema) would associate the newly-created book with author but preserve the :proposed status passed in.

I've modified the PR to refine the existing logic to duplicate create's behavior when calling insert_all. This would still be a breaking change for non-association scopes and attributes, but it's no longer a reversal of what #38899 was trying to accomplish for bulk insertions via associations. Given the revised logic, would this still be something that would need to have a deprecation flag, etc.?

Thanks again for the feedback. 🙂 Let me know if I'd need to tweak anything else with this PR.