accepts_nested_attributes_for does not work with non id primary_key

[fanantoxa]

Steps to reproduce

Currently accepts_nested_attributes_for when it's trying to find existing records in relation it's hardcoded to take a look on id column.
Problem is that id is not alway a primary key. In our case we have one of tables does not have id column at all but uses uuid is primary key.
We've set self.primary_key = 'uuid' but accepts_nested_attributes_for does not uses this value to use in matching existing records.

# frozen_string_literal: true

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"

  git_source(:github) { |repo| "https://github.com/#{repo}.git" }

  gem "rails", github: "rails/rails", branch: "main"
  gem "sqlite3"
end

require "active_record"
require "minitest/autorun"
require "logger"

# This connection will do for database-independent bug reports.
ActiveRecord::Base.establish_connection(adapter: "sqlite3", database: ":memory:")
ActiveRecord::Base.logger = Logger.new(STDOUT)

ActiveRecord::Schema.define do
  create_table :posts, force: true do |t|
  end

  create_table :comments, force: true, id: false do |t|
    t.integer :uuid, primary_key: true
    t.integer :post_id
    t.string :name
  end
end

class Post < ActiveRecord::Base
  has_many :comments

  accepts_nested_attributes_for :comments, allow_destroy: true
end

class Comment < ActiveRecord::Base
  self.primary_key = 'uuid'

  belongs_to :post
end

class BugTest < Minitest::Test
  def test_association_stuff
    post = Post.create!
    post.comments << Comment.create!(name: 'wonderfull')

    post.reload

    assert_equal 1, post.comments.count
    assert_equal 1, Comment.count
    assert_equal post.id, Comment.first.post.id

    update_posts_data = {
      'id' => post.id,
      'comments_attributes' => [
        {
          'uuid' => Comment.first.uuid,
          'name' => 'Even Better'
        },
        {
          'name' => 'Another comment'
        }
      ]
    }

    post.assign_attributes(update_posts_data)

    post.save

    post.reload
    assert_equal 2, post.comments.count
    assert_equal 2, Comment.count

    assert_equal post.id, Comment.first.post.id
    assert_equal post.id, Comment.last.post.id

    assert_equal 'Even Better', Comment.first.name
    assert_equal 'Another comment', Comment.last.name
  end
end

Expected behavior

Existing records are updated and new created.

Actual behavior

It always trying to create new records and hit violation on primary key:

ActiveRecord::RecordNotUnique: SQLite3::ConstraintException: UNIQUE constraint failed: comments.uuid

System configuration

Rails version: 7.1.0.alpha
But present in any Rails version

Ruby version: 3.1.2
But present in any Ruby version

[fatkodima]

I think, that the 'id' attribute for nested models means that you need to set a primary key value (whether it is named as id or not) under this name. So you should be doing something like:

update_posts_data = {
  'id' => post.id,
  'comments_attributes' => [
    {
      'id' => Comment.first.uuid,
      'name' => 'Even Better'
    },
    {
      'name' => 'Another comment'
    }
  ]
}

To avoid confusion, we can also pretty easily change the logic to accept the actual primary key name as a key in the attributes (uuid in the example). But I am not sure, if this is really necessary.

[fanantoxa]

I was able to get around it with alias_attribute :id, :uuid on a model and setting id to uuid on the front-end.

But ordinal problem is still remain.
ActiveRecord allow to have different names for primary_key column. And there is many reasons not to name it id.

From solution stand point I see that it can be solved:

• accepts_nested_attributes_for would use column defined primary_key if it's present
• add additional attribute to accepts_nested_attributes_for to manually set attribute name that should be used to find existing record
• combination of both above

Maybe other solutions.

[nvasilevski]

I was able to get around it with alias_attribute :id, :uuid on a model

Could you elaborate on the need of this? id has a special meaning in Rails and should already refer to uuid column if uuid is being used as a primary key

So whenever you do object.id it returns the value from the uuid column
Similarly how object.id = "abc-123" populates uuid attribute

I'm curious what alias_attribute provided you with which was not implemented through the default special meaning of id term

[fatkodima]

As I understand, the alias_attribute :id, :uuid is not really needed, as .id and .id= are working as expected. The real problem was that on the frontend the field has the "uuid" name, but Rails expects a field named "id", for example

rails/activerecord/lib/active_record/nested_attributes.rb

Line 510 in 50e3ae0

attribute_ids = attributes_collection.filter_map { |a| a["id"] || a[:id] }

and that it is not working.

The proposed solution is to accept "uuid" (a real name of the primary key) and "id" (for compatibility) as these keys,

[nvasilevski]

As I understand, the alias_attribute :id, :uuid is not really needed

That what I was suspecting as well

accepts_nested_attributes_for would use column defined primary_key if it's present

To be honest this sounds like a valid expectation and I think it should be fixable in a non-breaking way by tweaking assign_nested_attributes_for_collection_association
https://github.com/rails/rails/blob/c373f3cbbf091d2cf8cec58f12a2b68d33fbea08/activerecord/lib/active_record/nested_attributes.rb#L484
and substituting hardcoded "id" and :id references to dynamic primary_key value which most likely needs to be fetched from association(association_name)

I would even say that this line is design-wise wrong
https://github.com/rails/rails/blob/c373f3cbbf091d2cf8cec58f12a2b68d33fbea08/activerecord/lib/active_record/nested_attributes.rb#L524

This line tries to compare record.id value which is a value behind the primary_key column to something that is strictly named as "id". It works for most of the models but not for ones that don't use id column

Changing it to record.id.to_s == attributes[record.class.primary_key].to_s should be a non-breaking change that allows for more flexibility. I think doing such change will be completely justified

[nvasilevski]

the proposed solution is to accept "uuid" (a real name of the primary key) and "id" (for compatibility)

Right, I was slightly wrong in my comment above, we shouldn't be substituting but rather extending the logic to check for the dynamic primary_key key presence in the attributes hash

[fatkodima]

Yes, agreed, so please let me know if you want to open a PR.

[nvasilevski]

Yes, agreed, so please let me know if you want to open a PR.

I don't have spare time to have a look this week but I can try looking into it next week unless you or someone else wants to work on that! I don't mind at all, to me this should be a very low risk change that delivers a quick win

[fanantoxa]

Hey! Any new on that issue? 🙂 I see the PR is opened but looks like dormant at the moment