Double save with record.association.create! in Rails 3.1.0.rc1 #1360

Closed
GBH opened this Issue May 27, 2011 · 9 comments

Comments

Projects
None yet
5 participants
Contributor

GBH commented May 27, 2011

Here's a fun one I found this morning. Took about 4 hours to pinpoint this sucker.
This problem exists in both rc1 and the master branch.

class Foo < ActiveRecord::Base
  has_many :bars
end

class Bar < ActiveRecord::Base
  belongs_to :foo

  before_save :hi
  after_save :bye

  def hi
    p "HI #{self.id}"
  end

  def bye
    p "BYE #{self.id}"
  end
end

Here's little test to run:

class BarTest < ActiveSupport::TestCase
  def setup
    @foo = Foo.create!
  end

  def test_direct_create
    Bar.create!
  end

  def test_save
    bar = @foo.bars.new
    bar.save!
  end

  def test_create_via_association
    @foo.bars.create
  end

  def test_create_via_association_gone_bad
    @foo.bars.create!
  end
end

Few sanity checks, but the meat of the problem is at the last two. When you run the test_create_via_association this is the debug output:

"HI "
"BYE 980190963"

That's expected. Next test should have exactly the same output. But:

"HI "
"BYE 980190963"
"HI 980190963"
"BYE 980190963"

For some reason create! will trigger save twice, but only via association. Any thoughts how this is happening?

Contributor

GBH commented May 27, 2011

Wanted to add that I arrived at this problem when object.attributes = { :parent_id => id } in my application started triggering errors in belongs_to_association.rb, def different_target?(record) method.

From what I could tell record was nil and so was the owner[reflection.foreign_key], then it would just blow up on record.id. Since that code at least 3 months old, it's probably not the problem. Problem is that mere assignment of attributes would trigger counter cache update. Something is iffy.

Contributor

masterkain commented May 29, 2011

+1 on patch

foeken commented May 29, 2011

+1 I encountered this too

Contributor

GBH commented May 30, 2011

Thanks for the patch. Hopefully it will get merged soon.
Also, wouldn't it be great to be able to tag issues when creating them? Seems only the repo owner can do it.

Contributor

farleyknight commented May 30, 2011

No problem. Glad to help. My pull request is here: #1389 . Maybe a +1 over there could raise it's awareness, not sure.

BTW, I realised, literally 5 seconds ago, that my patch was somewhat incomplete. I just submitted a second patch, so it should pass all the (unbroken) tests now.

@ghost ghost assigned jonleighton May 30, 2011

Member

jonleighton commented May 30, 2011

Hiya, Just to say that I have seen this and will look at it when I have a chance. I agree this should be fixed before 3.1 goes out.

Contributor

farleyknight commented May 31, 2011

Okay, just merged my two patches, closing my original pull request.

jonleighton added a commit that referenced this issue May 31, 2011

jonleighton added a commit that referenced this issue May 31, 2011

Member

jonleighton commented May 31, 2011

Hiya,

Thanks for the patch. I worked on the implementation a bit as I wanted to reduce the duplication, but I've given you full credit for the test.

Cheers!

Member

jonleighton commented May 31, 2011

Oh, and BTW, your patch had lots of trailing white space, which really muddies up diffs. You can get plugins for various text editors which will automatically remove trailing white space.

martinploeger pushed a commit to martinploeger/rails that referenced this issue May 31, 2011

jjwright55 pushed a commit to jjwright55/comfortable-mexican-sofa that referenced this issue Apr 1, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment