Allow `accepts_nested_attributes_for` to accept associated record attributes with custom primary keys #48733

fatkodima · 2023-07-13T14:38:31Z

Currently, when we have an accepts_nested_attributes_for for a child relation with a custom primary key, when assigning child attributes we must use "id" key with a value to provide a primary key, which can be confusing (as in the linked issue).

Example:

class Owner < ApplicationRecord
  has_many :pets
  accepts_nested_attributes_for :pets
end

class Pet < ApplicationRecord
  self.primary_key = :pet_id # custom primary key name
  belongs_to :owner
end

Before:

owner.update(pets_attributes: { id: 2, name: "parrot" }) # works, because we used "id" as a key
owner.update(pets_attributes: { pet_id: 2, name: "parrot" }) # does not work

After - both variants are working

nvasilevski

I just wanted to mention that changes in this PR treat primary_key value as a single column though technically Rails main allows primary_key to be derived and configured as an Array.
However, I don't think we need to overcomplicate this PR with any changes related to composite primary keys since, we can always do a follow up

nvasilevski · 2023-07-13T15:34:10Z

activerecord/lib/active_record/nested_attributes.rb

+        nested_attributes_possible_id_names(klass).each do |primary_key|
+          return attributes[primary_key] if attributes[primary_key].present?
+        end
+        nil


For some reason I really want to use find here but not sure if it looks any better

Suggested change

nested_attributes_possible_id_names(klass).each do |primary_key|

return attributes[primary_key] if attributes[primary_key].present?

end

nil

pk = nested_attributes_possible_id_names(klass).find do |primary_key|

attributes[primary_key]

end

attributes[pk] if pk

nvasilevski · 2023-07-13T15:38:18Z

activerecord/lib/active_record/nested_attributes.rb

          assign_to_or_mark_for_destruction(existing_record, attributes, options[:allow_destroy]) unless call_reject_if(association_name, attributes)

-        elsif attributes["id"].present?
-          raise_nested_attributes_record_not_found!(association_name, attributes["id"])
+        elsif id = get_nested_attributes_id(attributes, association.klass)


Unlikely it will be noticeable in the big picture but perhaps we could avoid calling get_nested_attributes_id more than once and just derive it before all the if statements and reuse it

nvasilevski · 2023-07-13T15:41:52Z

activerecord/lib/active_record/nested_attributes.rb

            unless call_reject_if(association_name, attributes)
              # Make sure we are operating on the actual object which is in the association's
              # proxy_target array (either by finding it, or adding it if not found)
              # Take into account that the proxy_target may have changed due to callbacks
-              target_record = association.target.detect { |record| record.id.to_s == attributes["id"].to_s }
+              target_record = association.target.detect { |record| record.id.to_s == get_nested_attributes_id(attributes, association.klass).to_s }


Same as the other comment. It look like we should be able to perform get_nested_attributes_id(attributes, association.klass).to_s once and reuse it

Calling it multiple times makes me a little uncomfortable as it calls into nested_attributes_possible_id_names which allocates the same ["id", :id] array

nvasilevski · 2023-07-13T15:51:14Z

activerecord/lib/active_record/nested_attributes.rb

+        nil
+      end
+
+      def nested_attributes_possible_id_names(klass)


If we won't be able to avoid multiple calls to get_nested_attributes_id perhaps we should consider memoizing the value

…ributes with custom primary keys

fatkodima · 2023-07-13T18:02:16Z

Thanks! Updated with the suggestion for each -> find and extracted getting id in both methods.

nvasilevski

👍 Let's have someone from the core team to have a look

One thing I didn't mention in my previous review is that in this PR we treat primary_key value as a single column while on main Active Record allows primary_key to be derived as an array. However, I don't think it is a concern as we shouldn't be doing two things (composite primary keys support & bug fix) in the same PR. So to me it's okay to ship it as is and we will address the composite primary keys support later

Thank you for the fix!

westonganger · 2023-12-06T02:10:14Z

Seems like this fix is similar/interrelated to the changes in another PR #48390

rails-bot bot added the activerecord label Jul 13, 2023

nvasilevski reviewed Jul 13, 2023

View reviewed changes

Allow accepts_nested_attributes_for to accept associated record att…

be7d40c

…ributes with custom primary keys

fatkodima force-pushed the accepts_nested_attributes_for-custom-pk branch from 253ae65 to be7d40c Compare July 13, 2023 18:01

nvasilevski approved these changes Jul 20, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow `accepts_nested_attributes_for` to accept associated record attributes with custom primary keys #48733

Allow `accepts_nested_attributes_for` to accept associated record attributes with custom primary keys #48733

fatkodima commented Jul 13, 2023

nvasilevski left a comment

nvasilevski Jul 13, 2023

nvasilevski Jul 13, 2023

nvasilevski Jul 13, 2023

nvasilevski Jul 13, 2023

fatkodima commented Jul 13, 2023

nvasilevski left a comment

westonganger commented Dec 6, 2023 •

edited

Allow accepts_nested_attributes_for to accept associated record attributes with custom primary keys #48733

Are you sure you want to change the base?

Allow accepts_nested_attributes_for to accept associated record attributes with custom primary keys #48733

Conversation

fatkodima commented Jul 13, 2023

nvasilevski left a comment

Choose a reason for hiding this comment

nvasilevski Jul 13, 2023

Choose a reason for hiding this comment

nvasilevski Jul 13, 2023

Choose a reason for hiding this comment

nvasilevski Jul 13, 2023

Choose a reason for hiding this comment

nvasilevski Jul 13, 2023

Choose a reason for hiding this comment

fatkodima commented Jul 13, 2023

nvasilevski left a comment

Choose a reason for hiding this comment

westonganger commented Dec 6, 2023 • edited

Allow `accepts_nested_attributes_for` to accept associated record attributes with custom primary keys #48733

Allow `accepts_nested_attributes_for` to accept associated record attributes with custom primary keys #48733

westonganger commented Dec 6, 2023 •

edited