Skip to content

Commit

Permalink
Added test cases and a fix for rails#10865.
Browse files Browse the repository at this point in the history
Assuming Topic has_many Replies and Reply belongs_to Topic, the counter cache
was incremented twice if reply is saved after reassignment, as follows:

    reply.topic = topic  # triggers BelongsToAssocation#replace
    reply.save           # triggers belongs_to_counter_cache_after_update

The problem is caused by duplicate work.

This commit adds a flag to the reply if it has already triggered an increment,
but has not been saved yet. Thus, when the reply is actually saved, the
superfluous increment does not occur.

Note the following edge case:

    reply.topic = t1    # not saved, but counter updated
    t2.replies << t2    # save! called, previous change needs to be undone
  • Loading branch information
Jiunn Haur Lim committed Jul 25, 2013
1 parent 7af0ae9 commit 52e30db
Show file tree
Hide file tree
Showing 3 changed files with 71 additions and 15 deletions.
Expand Up @@ -38,20 +38,26 @@ def update_counters(record)
counter_cache_name = reflection.counter_cache_column

if counter_cache_name && owner.persisted? && different_target?(record)
dirty_flag = { from: nil, to: nil }

if record
record.class.increment_counter(counter_cache_name, record.id)
dirty_flag[:to] = record.id
end

if foreign_key_present?
klass.decrement_counter(counter_cache_name, target_id)
dirty_flag[:from] = target_id
end

owner.instance_variable_set(:@_dirty_but_updated_counter_cache, dirty_flag)
end
end

# Checks whether record is different to the current target, without loading it
def different_target?(record)
if record.nil?
owner[reflection.foreign_key]
if record.nil?
owner[reflection.foreign_key]
else
record.id != owner[reflection.foreign_key]
end
Expand Down
40 changes: 27 additions & 13 deletions activerecord/lib/active_record/associations/builder/belongs_to.rb
Expand Up @@ -49,21 +49,35 @@ def belongs_to_counter_cache_before_destroy(association, reflection)
end

def belongs_to_counter_cache_after_update(association, reflection)
foreign_key = reflection.foreign_key
cache_column = reflection.counter_cache_column

if (@_after_create_counter_called ||= false)
foreign_key = reflection.foreign_key
foreign_key_was = attribute_was foreign_key
foreign_key_now = attribute foreign_key
model = reflection.klass
dirty = (@_dirty_but_updated_counter_cache ||= false)

case
when (@_after_create_counter_called ||= false)
@_after_create_counter_called = false
elsif attribute_changed?(foreign_key) && !new_record? && association.constructable?
model = reflection.klass
foreign_key_was = attribute_was foreign_key
foreign_key = attribute foreign_key
return
when !attribute_changed?(foreign_key) || new_record? || !association.constructable?
return
when dirty && dirty == { from: foreign_key_was, to: foreign_key_now }
@_dirty_but_updated_counter_cache = false
return
when !model.respond_to?(:increment_counter) || !model.respond_to?(:decrement_counter)
return
else
cache_column = reflection.counter_cache_column

if foreign_key && model.respond_to?(:increment_counter)
model.increment_counter(cache_column, foreign_key)
end
if foreign_key_was && model.respond_to?(:decrement_counter)
model.decrement_counter(cache_column, foreign_key_was)
model.increment_counter(cache_column, foreign_key_now) if foreign_key_now
model.decrement_counter(cache_column, foreign_key_was) if foreign_key_was

if dirty # undo
undo_from = dirty[:from]
undo_to = dirty[:to]
model.increment_counter(cache_column, undo_from) if undo_from
model.decrement_counter(cache_column, undo_to) if undo_to
@_dirty_but_updated_counter_cache = false
end
end
end
Expand Down
Expand Up @@ -264,6 +264,42 @@ def test_belongs_to_with_primary_key_counter
assert_equal 0, debate2.reload.replies_count
end

def test_belongs_to_counter_with_append_after_create
topic = Topic.create("title" => "t1")
reply = Reply.create("title" => "r1", "content" => "r1")

topic.replies << reply
assert_equal 1, topic.reload.replies_count

assert reply.save
assert_equal 1, topic.reload.replies_count
end

def test_belongs_to_counter_with_reassigning_after_find
topic = Topic.create("title" => "t1")
reply = Reply.create("title" => "r1", "content" => "r1")
reply = Reply.find(reply.id)

reply.topic = topic

assert reply.save
assert_equal 1, topic.reload.replies_count
end

def test_belongs_to_counter_with_append_after_reassigning
topic1 = Topic.create("title" => "t1")
topic2 = Topic.create("title" => "t2")
reply = Reply.create("title" => "r1", "content" => "r1")
reply = Reply.find(reply.id)

reply.topic = topic1
topic2.replies << reply

assert reply.save
assert_equal 0, topic1.reload.replies_count
assert_equal 1, topic2.reload.replies_count
end

def test_belongs_to_counter_with_reassigning
topic1 = Topic.create("title" => "t1")
topic2 = Topic.create("title" => "t2")
Expand Down

0 comments on commit 52e30db

Please sign in to comment.