Change to association id does not change cached referenced to association #5744

Closed
crystalneth opened this Issue Apr 4, 2012 · 11 comments

Comments

Projects
None yet
5 participants

If Book belongs_to Author

book = Book.new
book.author => nil
book.author_id = 1
book.author => Author(:id => 1)
book.author_id = 2
book.author => Author(:id => 1) # WTF?
book.save!
book.author => Author(:id => 1) # WTF?
book.author_id => 2 # OK, but WTF?
book.reload
book.author => Author(:id => 2) # OK

book.author = Author.find(2)
book.author_id => 2 # OK

This is particularly problematic in web forms which update the id directly because accessing the new association will access the old object unless the old object was nil.

book = Book.create!(:author => nil)
# Web form:
book.author_id = 3
# Some after save callback:
puts "Our new author is #{book.author.name}" # GREAT!

book = Book.create!(:author => Author.find(2))
# Web form:
book.author_id = 3
# Some after save callback:
puts "Our new author is #{book.author.name}" # FAIL! This points to author 2 even though author_id_changed? and author_id = 3! Yikes@
# Seems a call to reload is necessary

Seems duplicated of #5563, and PR #5663.

@kennyj could you please take a look and confirm if it's the same issue? Thanks.

Contributor

kennyj commented Apr 4, 2012

I also think this issue is the same issue.

Appears so. Is this a regression? Seems rather core to active_record and very surprised there is no test in place to catch this!

After looking again, it appears this is a different issue. Note that this is in Rails 3.0.10.

Here is a test that fails when added to activerecord/..../belongs_to_association_test.rb

def test_association_id_assignment_updates_association
  author1, author2 = Author.limit(2)
  post = Post.new(:title => "foo", :body=> "bar")
    post.author    = author1

  post.author_id = author2.id

  assert_equal author2.id, post.author.id 
end

This seems to be a critical and insidious bug. I find it hard to believe a but this core and this bad has survived into a released version of Rails, but I don't see any other way of looking at it.

I've verified that, at least in Rails 2.2.2 (yes I still have something running from back then), assignment of an _id property for an association changes the association.

One serious consequence: it means association references are almost always wrong in validations triggered by rails forms.

def validate_parent
    errors.add :parent, "can't be yourself" if parent.id == id
end

This validation will NOT catch an update changing the parent from a non-nil value to passed ID. (It will also not allow you to CHANGE the id from the invalid one pointing to the object's ID.)

OUCH!

Contributor

sevenseacat commented Apr 5, 2012

Not saying this isn't a real issue, but with the usual post/redirect/get mechanism you'd never see it because you redirect and reload the object.

@karpah Not true for validations or save callbacks, which happen on the post.

@Allix I agree this is a real issue, and may probably an unintentional breakage. But I don't see your validation example happening so often because you usually don't set both the association and the id at the same time in a normal action. Anyway, we'll be looking into that, thanks for reporting.

This has happened to me quite a few times when I was unable to uncover the
root cause and I resorted to ugly calls to reload. I had a discussion in
IRC last night and heard from a few people who simply saw association
caching causing ids to be out of sync as a given. Apparently this has been
broken since 2.3. One person mentioned it was fixed in 3.2.

To me it seems the most basic of active_recod functionality.

I think this could be a significant cause of obscure bugs and ugly hacks.
For nontrivial situations it is quite common to access associations in
validations and callbacks.

On Thursday, April 5, 2012, Carlos Antonio da Silva wrote:

@Allix I agree this is a real issue, and may probably an unintentional
breakage. But I don't see your validation example happening so often
because you usually don't set both the association and the id at the same
time in a normal action. Anyway, we'll be looking into that, thanks for
reporting.


Reply to this email directly or view it on GitHub:
#5744 (comment)

Sent from a device that makes me spell badly and write short emails
Alex Neth
remindem
http://remindem.com/remind/alex
+1 206 499 4995
+86 13761577188

Contributor

kennyj commented Apr 5, 2012

@aliix
FYI: When I added your testcase and executed rake test, but it was passed on master (#5744 (comment) with replacing post.author2.id to post.author.id).

Member

jonleighton commented Apr 25, 2012

This bug is fixed in master as far as we know. If you disagree please submit a script to reproduce. If you want stuff backported to 3-2-stable, please submit a PR for the backport.

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