Bug: counter_cache update twice #13304

Closed
ghost opened this Issue Dec 13, 2013 · 18 comments

Comments

Projects
None yet
@ghost

ghost commented Dec 13, 2013

Example

# topic model
  belongs_to :node,       counter_cache: true
topic = Topic.find(1)  # node_id = 2
node = Node.find(1)

topic.node = node
topic.save

when you do that, active record update both node(id = 2) and node(id=1) topics_count twice. which make one add 2 and another minus 2

but if you do:

topic.node_id = node.id
topic.save
#or
topic.update_attributes(node_id: node.id)

you will be fine

Contributor

gregmolnar commented Dec 13, 2013

https://gist.github.com/gregmolnar/7940834 It looks like this is a bug indeed.

Contributor

gregmolnar commented Dec 13, 2013

This is a tricky one. When we do topic.node = node we build the relation, save it in the db and update the counter cache. The problem at that point of the code we don't know if there is any instance of the parent element loaded so I don't see a way to update the counter field in the instances of that model. Maybe someone smarter than me( @tenderlove) will have an idea to come around this. Or else we need to document this behavior.

Which Rails version are you using? Can you check if Rails master still has the described issue?

Contributor

gregmolnar commented Dec 13, 2013

I tried with master. I can submit a PR with a failing testcase if that helps. The gist above also uses master.
What I discovered is when the relation is built rails increments the counter field on the another model in the db. But if you have an instance of that model it won't know about this change and if you save it later it will increment the field again and causes the issue.

Member

senny commented Dec 13, 2013

@gregmolnar no need for a PR with test-case only. The gist is handy enough to reproduce.

Contributor

gregmolnar commented Dec 13, 2013

@senny great. My explanation might not be the best but hopefully you guys get what I mean :).

Owner

tenderlove commented Dec 13, 2013

It's kinda weird that we mutate the database from the writer method. Why would calling the writer have side effects like that? Seems like you'd want to update the counter on the call to save. Anyway, I am poking at the code.

Owner

tenderlove commented Dec 13, 2013

This seems unexpected:

require 'active_record'
require 'minitest/autorun'
require 'logger'

# This connection will do for database-independent bug reports.
ActiveRecord::Base.establish_connection 'sqlite3:///:memory:'

ActiveRecord::Schema.define do
  create_table(:posts)    { |t| t.integer :comments_count }
  create_table(:comments) { |t| t.integer :post_id }
end

class Post < ActiveRecord::Base
  has_many :comments
end

class Comment < ActiveRecord::Base
  belongs_to :post, counter_cache: true
end

class BugTest < Minitest::Test
  def test_counter_cache
    post = Post.create!
    comment = Comment.create!

    assert_equal 0, post.reload.comments_count.to_i
    comment.post = post
    assert_equal 0, post.reload.comments_count # fails here
  end
end
Contributor

gregmolnar commented Dec 13, 2013

I guess it is updating the counter cache here: https://github.com/rails/rails/blob/master/activerecord/lib/active_record/associations/belongs_to_association.rb#L13
And thanks for looking into this!

Owner

tenderlove commented Dec 13, 2013

Need to think up a better, more permanent solution, but this super hack fixes the test and AR runs clean:

diff --git a/activerecord/lib/active_record/associations/belongs_to_association.rb b/activerecord/lib/active_record/associations/belongs_to_association.rb
index 8272a55..195dc7b 100644
--- a/activerecord/lib/active_record/associations/belongs_to_association.rb
+++ b/activerecord/lib/active_record/associations/belongs_to_association.rb
@@ -10,6 +10,7 @@ module ActiveRecord
       def replace(record)
         if record
           raise_on_type_mismatch!(record)
+          owner.instance_eval { @ZOMG = true }
           update_counters(record)
           replace_keys(record)
           set_inverse_instance(record)
diff --git a/activerecord/lib/active_record/associations/builder/belongs_to.rb b/activerecord/lib/active_record/associations/builder/belongs_to.rb
index 62cc1e3..e408cfc 100644
--- a/activerecord/lib/active_record/associations/builder/belongs_to.rb
+++ b/activerecord/lib/active_record/associations/builder/belongs_to.rb
@@ -55,6 +55,7 @@ module ActiveRecord::Associations::Builder
           if (@_after_create_counter_called ||= false)
             @_after_create_counter_called = false
           elsif attribute_changed?(foreign_key) && !new_record? && reflection.constructable?
+            return if @ZOMG
             model           = reflection.klass
             foreign_key_was = attribute_was foreign_key
             foreign_key     = attribute foreign_key
Member

arthurnn commented Dec 18, 2013

IMO, comment.post = post should not increment the counter, even though this being the default behaviour on rails for a while. On rails 4 the after_update code was also introduced https://github.com/rails/rails/blob/master/activerecord/lib/active_record/associations/builder/belongs_to.rb#L85 leading to this bug that now we call increment, on the association = and on save.

I think, on rails 4.1 we should change this behaviour and only increment the counter when the save happens, and not on the replace.

thoughts @tenderlove @carlosantoniodasilva ?

Agreed with @arthurnn. It should not update counter cache since no model has been persisted yet.
A related PR: #11594

👍 for not updating the counter until the model is persisted. If it helps at all, I discovered that this bug doesn't happen, if the association is updated from something other than nil. I.e. this test passes

def test_update_from_other_post
  post = Post.create!
  post2 = Post.create!
  comment = Comment.create!(post: post)
  assert_equal 1, post.reload.comments_count
  assert_equal nil, post2.reload.comments_count

  comment.post = post2
  comment.save!

  assert_equal 0, post.reload.comments_count
  assert_equal 1, post2.reload.comments_count
end

To me this makes it look like there may be a solution that doesn't require breaking changes to rails.

I just closed my duplicate issue #14500

synth commented Apr 13, 2014

+1 Just upgraded my rails 3.2.x app to rails 4.1 and my test suite picked up this bug. is there a workaround?

Contributor

jasl commented Apr 18, 2014

I have a PR #9236 about 1 year ago, but seems no one mention this

Member

byroot commented Apr 18, 2014

@jasl 👍 I think that's the way to go, I was about to implement that.

Contributor

jasl commented Apr 18, 2014

@byroot 👍

Owner

jeremy commented May 5, 2014

@jeremy jeremy closed this May 5, 2014

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