This project is archived and is in readonly mode.

#2989 ✓resolved
Jeff Dean

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

Reported by Jeff Dean | August 3rd, 2009 @ 07:54 PM | in 3.1

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

Comments and changes to this ticket

  • Mike Dalessio

    Mike Dalessio August 3rd, 2009 @ 08:53 PM

    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

  • Adam Milligan

    Adam Milligan August 3rd, 2009 @ 09:28 PM

    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.

  • CancelProfileIsBroken

    CancelProfileIsBroken September 25th, 2009 @ 01:00 PM

    • Tag changed from “active_record, associations, belongs_to” to “active_record, associations, belongs_to, bugmash”
  • Elad Meidar

    Elad Meidar September 25th, 2009 @ 11:04 PM

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

  • Peter Jaros

    Peter Jaros September 26th, 2009 @ 02:23 AM

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

  • Elad Meidar

    Elad Meidar September 26th, 2009 @ 02:36 AM

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

  • Peter Jaros

    Peter Jaros September 26th, 2009 @ 11:56 PM

    I can't think of another place.

  • Joe Van Dyk

    Joe Van Dyk September 27th, 2009 @ 01:18 AM

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

  • Elad Meidar

    Elad Meidar September 27th, 2009 @ 01:25 AM

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

  • Elad Meidar

    Elad Meidar September 27th, 2009 @ 01:30 AM

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

  • Joe Van Dyk

    Joe Van Dyk September 27th, 2009 @ 01:34 AM

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

  • Elad Meidar

    Elad Meidar September 27th, 2009 @ 02:21 AM

    @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

  • Elad Meidar

    Elad Meidar September 27th, 2009 @ 03:03 AM

    +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>.
  • Joe Van Dyk

    Joe Van Dyk September 27th, 2009 @ 03:33 AM

    Elad, good catch about the foreign key.

  • Elad Meidar

    Elad Meidar September 27th, 2009 @ 03:43 AM

    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.

  • Zach Brock

    Zach Brock November 30th, 2009 @ 06:15 AM

    Any progress made on this?

  • Rizwan Reza

    Rizwan Reza February 12th, 2010 @ 12:46 PM

    • Tag changed from “2-3-stable, active_record, associations, belongs_to, bugmash” to “2-3-stable, active_record, associations, belongs_to”
  • Jeff Dean

    Jeff Dean June 19th, 2010 @ 07:38 AM

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

  • Michael Koziarski

    Michael Koziarski June 23rd, 2010 @ 05:19 AM

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

  • Jeff Dean

    Jeff Dean June 23rd, 2010 @ 06:13 AM

    Thank you for the feedback.

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

  • Jeff Dean

    Jeff Dean July 4th, 2010 @ 05:08 AM

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

  • Neeraj Singh

    Neeraj Singh July 7th, 2010 @ 06:44 PM

    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.

  • Ryan Bigg

    Ryan Bigg October 9th, 2010 @ 10:05 PM

    • Tag cleared.

    Automatic cleanup of spam.

  • Aditya Sanghi

    Aditya Sanghi October 9th, 2010 @ 10:05 PM

    Automatic cleanup of spam.

  • Jonas Schneider

    Jonas Schneider October 10th, 2010 @ 10:08 AM

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

  • Santiago Pastorino

    Santiago Pastorino October 10th, 2010 @ 05:33 PM

    • Assigned user set to “Aaron Patterson”
  • Ryan Bigg

    Ryan Bigg October 21st, 2010 @ 03:35 AM

    Automatic cleanup of spam.

  • Jeff Dean

    Jeff Dean December 3rd, 2010 @ 09:01 PM

    Any progress on this?

  • Jeff Dean

    Jeff Dean December 4th, 2010 @ 06:25 AM

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

  • Repository

    Repository December 8th, 2010 @ 11:40 PM

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

  • Andrew Assarattanakul

    Andrew Assarattanakul December 10th, 2010 @ 07:24 PM

    Still an issue with 2.3 stable

  • Jeff Dean

    Jeff Dean December 11th, 2010 @ 08:44 PM

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

  • Jon Leighton

    Jon Leighton December 23rd, 2010 @ 01:41 PM

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

  • Jon Leighton

    Jon Leighton December 23rd, 2010 @ 11:55 PM

    • State changed from “open” to “resolved”

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>

People watching this ticket

Attachments

Referenced by

Pages