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.

Setting the id of a belongs_to object does not update the reference to the object

#2989

Sorry for the confusing title. Here's the most concise explanation:

require 'test_helper'

class Article < ActiveRecord::Base
  has_many :comments
end

class Comment < ActiveRecord::Base
  belongs_to :article
end

class CommentTest < ActiveSupport::TestCase
  test "assigning an associated object id replaces the reference to the associated object" do
    article1 = Article.create! :title => "First"
    article2 = Article.create! :title => "Second"
    
    comment = Comment.new
    comment.article_id = article1.id
    assert_equal article1, comment.article
    
    comment.article_id = article2.id
    assert_equal article2, comment.article # FAILS!
  end
end

This is with the following schema, on a brand new edge rails project with no other plugins or gems installed:

ActiveRecord::Schema.define(:version => 20090803184410) do
  create_table "articles", :force => true do |t|
    t.string   "title"
    t.datetime "created_at"
    t.datetime "updated_at"
  end

  create_table "comments", :force => true do |t|
    t.integer  "article_id"
    t.text     "body"
    t.datetime "created_at"
    t.datetime "updated_at"
  end
end

Reported by Jeff Dean · August 3rd, 2009 @ 07:54 PM

State: resolved
Milestone: 3.1
Assigned to: Aaron Patterson Aaron Patterson
Importance: none

Activity

  1. Mike Dalessio
    Mike Dalessio

    This seems to be unexpected behavior to me. When updating association ids via form parameters, the associated object can't be used in, e.g., before_save callbacks.

    +1

    August 3rd, 2009 @ 08:53 PM

  2. Adam Milligan
    Adam Milligan

    I run into this bug about once a month, and it drives me up the wall. I patched it at one point, which created an infinite loop at validation time due to the fact that ActiveRecords now implicitly validates_associated their associations by default.

    A valid fix is a pain in the ass and a half that I haven't inflicted on myself yet.

    August 3rd, 2009 @ 09:28 PM

  3. CancelProfileIsBroken
    CancelProfileIsBroken
    • Tag changed from active_record, associations, belongs_to to active_record, associations, belongs_to, bugmash

    September 25th, 2009 @ 01:00 PM

  4. Elad Meidar
    Elad Meidar

    -1 current situation is expected and concurrent with other attribute behaviors. The reason for that is that there is no actual save upon the new association id assignment, i find it very confusing if there were some attributes (association ids for ex.) that will be saved immediately upon change, and some aren't, current state where you have to save / update_attributeis what i would expect.

    September 25th, 2009 @ 11:04 PM

  5. Peter Jaros
    Peter Jaros

    @Elad: This isn't about saving to the DB, this is just about updating the associations in memory. Just as setting the article_id the first time sets the article association to article1, setting the article_id the second time should set the article to article2.

    This isn't expected to be reflected in the database until save, but I do expect it to happen in memory, and I get quite confused when it doesn't. There should never be a time when comment.article.id != comment.article_id.

    September 26th, 2009 @ 02:23 AM

  6. Elad Meidar
    Elad Meidar

    @Peter: mmm, do you see anywhere else it needs to be applied except for a foreign key assignment in a :belongs_to association?

    September 26th, 2009 @ 02:36 AM

  7. Peter Jaros
    Peter Jaros

    I can't think of another place.

    September 26th, 2009 @ 11:56 PM

  8. Joe Van Dyk
    Joe Van Dyk

    I've attached a patch that contains a failing test.

    September 27th, 2009 @ 01:18 AM

  9. Elad Meidar
    Elad Meidar

    @Joe: Master? 2-3-stable?

    The way i see it, is that the way to solve it is to create a custom getter for any foreign key on a :belongs_to model..(child) and reload the parent association after assignment.

    any other idea?

    September 27th, 2009 @ 01:25 AM

  10. Elad Meidar
    Elad Meidar

    +1 on the patch, applies cleanly on master and 2-3-stable.

    September 27th, 2009 @ 01:30 AM

  11. Joe Van Dyk
    Joe Van Dyk
    • Tag changed from active_record, associations, belongs_to, bugmash to 2-3-stable, active_record, associations, belongs_to, bugmash

    2.3 stable, sorry.

    Elad, that was my thinking. Also, the association should only be reloaded if the foreign key was changed.

    September 27th, 2009 @ 01:34 AM

  12. Elad Meidar
    Elad Meidar

    @Joe: i think there's a logical error in your test.

    You wrote:

    
client.firm_id = other_firm.id

    to update the existing firm, but the Client class indicates:

    
class Client < Company
  belongs_to :firm, :foreign_key => "client_of"

    so i am changing it. to client.client_of = other_firm.id

    September 27th, 2009 @ 02:21 AM

  13. Elad Meidar
    Elad Meidar

    +1 verified the test fails on 2-3-stable, iv'e attached a patch that reloads all the associations that rely on a specific column, once it's changed. THERE ARE FAILING TESTS and i kept them for a reason, it seems to put on some extra overhead in terms of extra SQL queries.

    As you can see, all failing tests are tests that assert a certain number of queries to be preformed. note that the actual overload is about 300%.

    I think that this requires some more discussion, or at least a better way to let the developer decide.

    
  1) Failure:
test_create_without_loading_association(HasManyAssociationsTest)
    [./test/cases/../../lib/active_record/test_case.rb:31:in `assert_queries'
     ./test/cases/associations/has_many_associations_test.rb:456:in `test_create_without_loading_association'
     ./test/cases/../../../activesupport/lib/active_support/testing/setup_and_teardown.rb:62:in `__send__'
     ./test/cases/../../../activesupport/lib/active_support/testing/setup_and_teardown.rb:62:in `run']:
7 instead of 1 queries were executed.
Queries:
SELECT * FROM `companies` WHERE (`companies`.`id` = 1 AND (1 = 1)) AND ( (`companies`.`type` = 'Firm' ) ) 
SELECT * FROM `companies` WHERE (`companies`.`id` = 1 AND (1 = 1)) AND ( (`companies`.`type` = 'Firm' ) ) 
SELECT * FROM `companies` WHERE (`companies`.`id` = 1) AND ( (`companies`.`type` = 'Firm' ) ) 
SELECT * FROM `companies` WHERE (`companies`.`id` = 1) AND ( (`companies`.`type` = 'Firm' ) ) 
SELECT * FROM `companies` WHERE (`companies`.`id` = 1) AND ( (`companies`.`type` = 'Firm' ) ) 
SELECT * FROM `companies` WHERE (`companies`.`id` = 1) AND ( (`companies`.`type` = 'Firm' ) ) 
INSERT INTO `companies` (`name`, `client_of`, `rating`, `firm_name`, `ruby_type`, `type`, `firm_id`) VALUES('Superstars', 1, 1, NULL, NULL, 'Client', NULL).
<1> expected but was
<7>.

  2) Failure:
test_associate_existing(HasManyThroughAssociationsTest)
    [./test/cases/../../lib/active_record/test_case.rb:31:in `assert_queries'
     ./test/cases/associations/has_many_through_associations_test.rb:27:in `test_associate_existing'
     ./test/cases/../../../activesupport/lib/active_support/testing/setup_and_teardown.rb:62:in `__send__'
     ./test/cases/../../../activesupport/lib/active_support/testing/setup_and_teardown.rb:62:in `run']:
5 instead of 1 queries were executed.
Queries:
SELECT * FROM `posts` WHERE (`posts`.`id` = 2) 
SELECT * FROM `posts` WHERE (`posts`.`id` = 2) 
SELECT * FROM `people` WHERE (`people`.`id` = 2) 
SELECT * FROM `people` WHERE (`people`.`id` = 2) 
INSERT INTO `readers` (`post_id`, `person_id`) VALUES(2, 2).
<1> expected but was
<5>.

  3) Failure:
test_associate_new_by_building(HasManyThroughAssociationsTest)
    [./test/cases/../../lib/active_record/test_case.rb:31:in `assert_queries'
     ./test/cases/associations/has_many_through_associations_test.rb:76:in `test_associate_new_by_building'
     ./test/cases/../../../activesupport/lib/active_support/testing/setup_and_teardown.rb:62:in `__send__'
     ./test/cases/../../../activesupport/lib/active_support/testing/setup_and_teardown.rb:62:in `run']:
13 instead of 5 queries were executed.
Queries:
UPDATE `posts` SET `body` = 'Like I hopefully always am-changed' WHERE `id` = 2
INSERT INTO `people` (`primary_contact_id`, `lock_version`, `gender`, `first_name`) VALUES(NULL, 0, NULL, 'Bob')
SELECT * FROM `posts` WHERE (`posts`.`id` = 2) 
SELECT * FROM `posts` WHERE (`posts`.`id` = 2) 
SELECT * FROM `people` WHERE (`people`.`id` = 7) 
SELECT * FROM `people` WHERE (`people`.`id` = 7) 
INSERT INTO `readers` (`post_id`, `person_id`) VALUES(2, 7)
INSERT INTO `people` (`primary_contact_id`, `lock_version`, `gender`, `first_name`) VALUES(NULL, 0, NULL, 'Ted')
SELECT * FROM `posts` WHERE (`posts`.`id` = 2) 
SELECT * FROM `posts` WHERE (`posts`.`id` = 2) 
SELECT * FROM `people` WHERE (`people`.`id` = 8) 
SELECT * FROM `people` WHERE (`people`.`id` = 8) 
INSERT INTO `readers` (`post_id`, `person_id`) VALUES(2, 8).
<5> expected but was
<13>.

  4) Failure:
test_associate_with_create(HasManyThroughAssociationsTest)
    [./test/cases/../../lib/active_record/test_case.rb:31:in `assert_queries'
     ./test/cases/associations/has_many_through_associations_test.rb:140:in `test_associate_with_create'
     ./test/cases/../../../activesupport/lib/active_support/testing/setup_and_teardown.rb:62:in `__send__'
     ./test/cases/../../../activesupport/lib/active_support/testing/setup_and_teardown.rb:62:in `run']:
6 instead of 2 queries were executed.
Queries:
INSERT INTO `people` (`primary_contact_id`, `lock_version`, `gender`, `first_name`) VALUES(NULL, 0, NULL, 'Jeb')
SELECT * FROM `posts` WHERE (`posts`.`id` = 2) 
SELECT * FROM `posts` WHERE (`posts`.`id` = 2) 
SELECT * FROM `people` WHERE (`people`.`id` = 9) 
SELECT * FROM `people` WHERE (`people`.`id` = 9) 
INSERT INTO `readers` (`post_id`, `person_id`) VALUES(2, 9).
<2> expected but was
<6>.

  5) Failure:
test_associating_new(HasManyThroughAssociationsTest)
    [./test/cases/../../lib/active_record/test_case.rb:31:in `assert_queries'
     ./test/cases/associations/has_many_through_associations_test.rb:48:in `test_associating_new'
     ./test/cases/../../../activesupport/lib/active_support/testing/setup_and_teardown.rb:62:in `__send__'
     ./test/cases/../../../activesupport/lib/active_support/testing/setup_and_teardown.rb:62:in `run']:
6 instead of 2 queries were executed.
Queries:
INSERT INTO `people` (`primary_contact_id`, `lock_version`, `gender`, `first_name`) VALUES(NULL, 0, NULL, 'bob')
SELECT * FROM `posts` WHERE (`posts`.`id` = 2) 
SELECT * FROM `posts` WHERE (`posts`.`id` = 2) 
SELECT * FROM `people` WHERE (`people`.`id` = 12) 
SELECT * FROM `people` WHERE (`people`.`id` = 12) 
INSERT INTO `readers` (`post_id`, `person_id`) VALUES(2, 12).
<2> expected but was
<6>.

    September 27th, 2009 @ 03:03 AM

  14. Joe Van Dyk
    Joe Van Dyk

    Elad, good catch about the foreign key.

    September 27th, 2009 @ 03:33 AM

  15. Elad Meidar
    Elad Meidar

    Thanks, but i think there must be a better way to do it (i use eval for god sake... :) ) and further more considerations are required as for the extra queries.

    September 27th, 2009 @ 03:43 AM

  16. Zach Brock
    Zach Brock

    Any progress made on this?

    November 30th, 2009 @ 06:15 AM

  17. Rizwan Reza
    Rizwan Reza
    • Tag changed from 2-3-stable, active_record, associations, belongs_to, bugmash to 2-3-stable, active_record, associations, belongs_to

    February 12th, 2010 @ 12:46 PM

  18. Jeff Dean
    Jeff Dean
    • Tag changed from 2-3-stable, active_record, associations, belongs_to to active_record, associations, belongs_to, patch

    This issue is far more complex than I realized, but luckily the fix was simple and clean. Take this example:

    class Client < Company
  belongs_to :firm, :foreign_key => "client_of"
  belongs_to :firm_with_other_name, :class_name => "Firm", :foreign_key => "client_of"
  belongs_to :firm_with_condition, :class_name => "Firm", :foreign_key => "client_of", :conditions => ["1 = ?", 1]

  belongs_to :sponsor, :polymorphic => true
end

firm1 = Firm.create
client = Client.new :client_of => firm1.id
client.firm == client.firm_with_other_name == client.firm_with_condition == firm1 # => true

firm2 = Firm.create
client.client_of => firm2.id
client.firm == client.firm_with_other_name == client.firm_with_condition == firm2 # => false on master, true in this patch

member = Member.create
client.sponsor_id = member.id
client.sponsor_type = "Member"
client.sponsor == member # => true
client.sponsor_type = "Firm"
client.sponsor == nil # => false on master, true in this patch

    In addition, for a belongs_to :polymorphic association, you'll want all associated objects to reload whenever you change the id or the type field.

    To make this work, every time the foreign key changes, we have to loop through the belongs_to associations and make the appropriate objects nil.

    The attached patch makes this work, and I've included test coverage for both regular and polymorphic belongs_to associations, and this patch does not break any tests on master.

    June 19th, 2010 @ 07:38 AM

  19. Michael Koziarski
    Michael Koziarski
    • Milestone set to 3.1

    Looping through all associations just to call a setter seems rather expensive, I'd like to defer this one till after 3.0 ships.

    However yes, this is a problem and this fix appears to be the right way to do it.

    June 23rd, 2010 @ 05:19 AM

  20. Jeff Dean
    Jeff Dean

    Thank you for the feedback.

    I'll find a good way to remove that loop and resubmit.

    June 23rd, 2010 @ 06:13 AM

  21. Jeff Dean
    Jeff Dean
    • Importance changed from to

    Here's a patch that moves the loop from the setter method. The code looks like this:

    def association_foreign_key_setter_method(reflection)
  setters = reflect_on_all_associations(:belongs_to).map do |belongs_to_reflection|
    if belongs_to_reflection.primary_key_name == reflection.primary_key_name
      "association_instance_set(:#{belongs_to_reflection.name}, nil);"
    end
  end.compact.join

  class_eval <<-FILE, __FILE__, __LINE__  1
    def #{reflection.primary_key_name}=(new_id)
      write_attribute :#{reflection.primary_key_name}, new_id
      if #{reflection.primary_key_name}_changed?
        #{ setters }
      end
    end
  FILE
end

    The idea is to rewrite the setter method every time you define a new belongs_to. Consider the following:

    class Comment < ActiveRecord::Base
  belongs_to :user
end

    Calling belongs_to would create a method like this:

    def user_id=(new_id)
  write_attribute :user_id, new_id
  if user_id_changed?
    association_instance_set(:user, nil);
  end
end

    In the case that 2 associations point to user_id:

    class Comment < ActiveRecord::Base
  belongs_to :user
  belongs_to :creator, :class_name => "User", :foreign_key => "user_id"
end

    The second time you called belongs_to it would rewrite the user_id= method to this:

    def user_id=(new_id)
  write_attribute :user_id, new_id
  if user_id_changed?
    association_instance_set(:user, nil)
    association_instance_set(:creator, nil)
  end
end

    Let me know if there are any issues with this technique that might cause a problem.

    Thank you.

    July 4th, 2010 @ 05:08 AM

  22. Neeraj Singh
    Neeraj Singh

    The code looks good to me.

    Given the complexity of the code I am wondering if this problem is worth solving. How about blowing up if someone uses

    client.sponsor_id = member.id

    Just tell user to not to assign to id directly. Just something to think about.

    July 7th, 2010 @ 06:44 PM

  23. Ryan Bigg
    Ryan Bigg
    • Tag cleared.

    Automatic cleanup of spam.

    October 9th, 2010 @ 10:05 PM

  24. Aditya Sanghi
    Aditya Sanghi

    Automatic cleanup of spam.

    October 9th, 2010 @ 10:05 PM

  25. Jonas Schneider
    Jonas Schneider

    +1 I run into this bug frequently and it's causing headaches with before_save callbacks etc.

    October 10th, 2010 @ 10:08 AM

  26. Santiago Pastorino
    Santiago Pastorino
    • Assigned user set to Aaron Patterson

    October 10th, 2010 @ 05:33 PM

  27. Ryan Bigg
    Ryan Bigg

    Automatic cleanup of spam.

    October 21st, 2010 @ 03:35 AM

  28. Jeff Dean
    Jeff Dean

    Any progress on this?

    December 3rd, 2010 @ 09:01 PM

  29. Jeff Dean
    Jeff Dean

    I've confirmed that this is still an issue and recreated a patch that should apply cleanly.

    December 4th, 2010 @ 06:25 AM

  30. Repository
    Repository
    • State changed from new to resolved

    (from [7ecee054a322e214e4f285b1a8327751bd79a418]) Setting the id of a belongs_to object updates all referenced objects [#2989 state:resolved] https://github.com/rails/rails/commit/7ecee054a322e214e4f285b1a8327...

    December 8th, 2010 @ 11:40 PM

  31. Andrew Assarattanakul
    Andrew Assarattanakul

    Still an issue with 2.3 stable

    December 10th, 2010 @ 07:24 PM

  32. Jeff Dean
    Jeff Dean

    Here's a patch that should apply cleanly to the 2-3-stable branch.

    December 11th, 2010 @ 08:44 PM

  33. Jon Leighton
    Jon Leighton
    • State changed from resolved to open

    This bug was actually originally reported at #142. A fix similar to the above checked in, but then reverted on the basis that there were problems with it. I've tried to take this forward by addressing the problems which were talked about in #142:

    • Instead of overwriting the foreign key accessor, check its value against the id column on the target when the association is next accessed. (E.g. lazily check whether an association is stale when accessing it.)
    • Building on this, I added tests and an implementation which extends this patch to support :through associations which go through a belongs_to which has its foreign key changed.

    Patch is attached.

    December 23rd, 2010 @ 01:41 PM

  34. Jon Leighton
    Jon Leighton
    • State changed from open to resolved

    December 23rd, 2010 @ 11:55 PM