Lighthouse has a new layout. Prefer the old one? Return to the old layout, and switch back any time from the link at the top of each page.

This project is archived and is in readonly mode.

accepts_nested_attributes_for creating new associated record when one already exists

#2852

I'm seeing that updating nested attributes is creating additional rows for the nested object:

class Profile < ActiveRecord::Base
  has_one :address
  accepts_nested_attributes_for :address
end







class Address < ActiveRecord::Base
  belongs_to :profile
end

instantiate a profile and save it w/ associated address attributes:

>> p = Profile.new
=> #<Profile id: nil, name: nil, created_at: nil, updated_at: nil>
>> p.update_attributes(:name => "josh", :address_attributes => {:line1 => "my street"})
=> true
>> Profile.count
=> 1
>> Address.count
=> 1

So far this is fine. Now reload the profile and update its attributes again:

>> p = Profile.first
=> #<Profile id: 3, name: "josh", created_at: "2009-06-29 13:23:18", updated_at: "2009-06-29 13:23:18">
>> p.update_attributes(:name => "josh", :address_attributes => {:line1 => "my other street"})
=> true
>> Profile.count
=> 1
>> Address.count
=> 2

Why are there two addresses at this point?

>> Address.first
=> #<Address id: 6, profile_id: nil, line1: "my street", created_at: "2009-06-29 13:23:18", updated_at: "2009-06-29 13:23:50">
>> Address.find(7)
=> #<Address id: 7, profile_id: 3, line1: "my other street", created_at: "2009-06-29 13:23:50", updated_at: "2009-06-29 13:23:50">
>> Address.find(6).profile_id
=> nil

The old address record's foreign key now points to nothing. This would be invalid if Address validated its association.

I'm willing to dig into this if someone can confirm that it is a bug.

Thanks!

Reported by Josh Sharpe · June 29th, 2009 @ 02:20 PM

State: invalid
Milestone: 3.0.2
Assigned to: Eloy Duran Eloy Duran
Importance: none

Activity

  1. Josh Sharpe
    Josh Sharpe
    • Tag changed from 2-3, accepts_nested_attributes_for, has_one, stable to 2-3, accepts_nested_attributes_for, has_one

    June 29th, 2009 @ 02:40 PM

  2. Yehuda Katz (wycats)
    Yehuda Katz (wycats)
    • State changed from new to verified
    • Milestone cleared.

    This does seem to be a bug. Dig away!

    July 2nd, 2009 @ 01:10 AM

  3. Pratik
    Pratik
    • State changed from verified to open

    July 2nd, 2009 @ 01:30 AM

  4. Josh Sharpe
    Josh Sharpe

    There are tests that explicitly test for this behavior making me think this is not a bug.

    See nested_attributes_test.rb:

    def test_should_not_replace_an_existing_record_if_there_is_no_id_and_delete_is_truthy
  @pirate.reload.ship_attributes = { :name => 'Davy Jones Gold Dagger', :_delete => '1' }

  assert_equal @ship, @pirate.ship
  assert_equal 'Nights Dirty Lightning', @pirate.ship.name
end

def test_should_modify_an_existing_record_if_there_is_a_matching_id
  @pirate.reload.ship_attributes = { :id => @ship.id, :name => 'Davy Jones Gold Dagger' }

  assert_equal @ship, @pirate.ship
  assert_equal 'Davy Jones Gold Dagger', @pirate.ship.name
end

    This still seems a bit counterintuitive to me. I would think that the existing record would be used if it exists regardless if an ID is passed. However, maybe there's a really good reason for this behavior that I'm missing.

    July 2nd, 2009 @ 04:31 PM

  5. Carl Lerche
    Carl Lerche
    • State changed from open to invalid

    I am not intimately familiar with the behavior of nested attribute updating, but I can take a shot at this.

    The question specifically would be: When are nested attributes updates vs. created. In the original update_attributes call, in :address_attributes, you do not specify the ID of the record to update, so my guess is that AR decides that it should create a new record.

    p.update_attributes(:name => "josh", :address_attributes => {:line1 => "my street"})

    You also do not specify a :dependent option on your has_one association, so the default is to nullify the foreign key. So, I guess that if you want to update the record, you should specify the ID in the nested Hash. The tests that specify updating nested records all contain the ID of the records to update.

    July 2nd, 2009 @ 06:24 PM

  6. Josh Sharpe
    Josh Sharpe

    I just want to quickly reiterate, to be really clear -- this issue only applies to has_one relations.

    That said, passing :id as the option to decide whether or not to update or create a new record doesn't make much sense. IMO wouldn't it make more sense that the nested attribute ALWAYS reference the existing one. This wouldn't break the ability to pass _delete => true, but it would prevent dangling records being created inadvertently and transparently.

    July 2nd, 2009 @ 08:35 PM

  7. Carl Lerche
    Carl Lerche
    • State changed from invalid to open

    Alright, I checked up on this some more and it seems that you are correct with regards to has_one should work without an ID. I'll mark this back as open.

    July 2nd, 2009 @ 09:40 PM

  8. José Valim
    José Valim

    @Josh, @Carl, For me it makes sense creating a new record when an ID is not specified, even for has_one. It's common that I want to replace the associated object by another one.

    I think the problem here is the dangling record. In such cases, when a record is being replaced, it should check for the :dependent behavior given to the association and invoke it properly.

    July 9th, 2009 @ 08:57 AM

  9. Josh Sharpe
    Josh Sharpe

    @Jose, Would you be strongly opposed if has_one returned the current association and you had to pass _delete => true in order to delete this record?

    I still feel that (per the original example)

    p.update_attributes(:address_attributes => {:id => p.address.id, :line1 => "my other street"})

    is redundant.

    July 9th, 2009 @ 07:42 PM

  10. José Valim
    José Valim

    @Josh, that would be a way to go, but I'm not sure if it works right now. Besides, there would be no way to nullify the record anymore.

    Although the question of when nullify, destroy or delete is another problem yet to be handled by nested attributes. I would prefer if _delete just trigger whatever is configured on the association, instead of destroying it.

    Currently we also have the same behavior to has_one and has_many, giving different behaviors can lead to further confusion. :/

    Have you posted on the mailing list? Maybe we can bring more people to the discussion. :)

    July 9th, 2009 @ 07:59 PM

  11. Michael Koziarski
    Michael Koziarski
    • Assigned user set to Eloy Duran

    July 9th, 2009 @ 11:59 PM

  12. Eloy Duran
    Eloy Duran
    • State changed from open to invalid

    This is not a bug, as José already noted. If you want to update, specify the ID, otherwise a new one is created.

    I also agree that _delete should do what's configured on the association and we might apply the same behaviour on one-to-one associations in the case where a record would be orphaned.

    I just haven't had the need for it yet and thus haven't implemented it yet. If you need it on your app and are willing to write a patch, please do so and re-open the ticket.

    Thanks!

    July 10th, 2009 @ 08:10 AM

  13. Jeremy Kemper
    Jeremy Kemper
    • Milestone set to 3.0.2

    October 15th, 2010 @ 11:01 PM

  14. Andrea Campi
    Andrea Campi
    • Tag changed from 2-3, accepts_nested_attributes_for, has_one to 2-3-stable, accepts_nested_attributes_for, has_one

    October 16th, 2010 @ 11:56 PM

  15. ssupreme11
    ssupreme11

    Its my first time to visit this site and as I was exploring I cant believe that this site was made up of a very informative articles that you should try to have compliment with so as what I am doing now I really love to look forward with more interesting information on this site.. Dissertation Uk

    May 10th, 2011 @ 10:38 PM