This project is archived and is in readonly mode.

#2852 ✓invalid
Josh Sharpe

accepts_nested_attributes_for creating new associated record when one already exists

Reported by Josh Sharpe | June 29th, 2009 @ 02:20 PM | in 3.0.2

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!

Comments and changes to this ticket

  • Josh Sharpe

    Josh Sharpe June 29th, 2009 @ 02:40 PM

    • Tag changed from 2-3, accepts_nested_attributes_for, has_one, stable to 2-3, accepts_nested_attributes_for, has_one
  • Yehuda Katz (wycats)

    Yehuda Katz (wycats) July 2nd, 2009 @ 01:10 AM

    • State changed from “new” to “verified”
    • Milestone cleared.

    This does seem to be a bug. Dig away!

  • Pratik

    Pratik July 2nd, 2009 @ 01:30 AM

    • State changed from “verified” to “open”
  • Josh Sharpe

    Josh Sharpe July 2nd, 2009 @ 04:31 PM

    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.

  • Carl Lerche

    Carl Lerche July 2nd, 2009 @ 06:24 PM

    • 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.

  • Josh Sharpe

    Josh Sharpe July 2nd, 2009 @ 08:35 PM

    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.

  • Carl Lerche

    Carl Lerche July 2nd, 2009 @ 09:40 PM

    • 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.

  • José Valim

    José Valim July 9th, 2009 @ 08:57 AM

    @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.

  • Josh Sharpe

    Josh Sharpe July 9th, 2009 @ 07:42 PM

    @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.

  • José Valim

    José Valim July 9th, 2009 @ 07:59 PM

    @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. :)

  • Michael Koziarski

    Michael Koziarski July 9th, 2009 @ 11:59 PM

    • Assigned user set to “Eloy Duran”
  • Eloy Duran

    Eloy Duran July 10th, 2009 @ 08:10 AM

    • 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!

  • Jeremy Kemper

    Jeremy Kemper October 15th, 2010 @ 11:01 PM

    • Milestone set to 3.0.2
  • Andrea Campi

    Andrea Campi October 16th, 2010 @ 11:56 PM

    • Tag changed from 2-3, accepts_nested_attributes_for, has_one to 2-3-stable, accepts_nested_attributes_for, has_one
  • ssupreme11

    ssupreme11 May 10th, 2011 @ 10:38 PM

    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

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

<h2 style="font-size: 14px">Tickets have moved to Github</h2>

The new ticket tracker is available at <a href="https://github.com/rails/rails/issues">https://github.com/rails/rails/issues</a>

Pages