Skip to content

Commit

Permalink
Don't rely on the counter_cache to destroy childrens:
Browse files Browse the repository at this point in the history
- rails#35127 introduced a new change to
  not make a query when the counter cache value of the parent is 0.

  The implementation was overwriting the `find_target?` method from
  `AR::Associaton`.
  This had the side effect of making `Topic.comments.destroy_all` to
  not destroy anything in some circumstances.

  The bug was mainly happening during test because the counter_cache
  value isn't set when fixtures load. This resulted in the
  `find_target` method to return the in memory target (empty array)
  instead of getting it from the DB.

  This PR removes the `find_target?` overwritten method in
  HasManyAssociation and inline the condition inside methods that
  needs it (such as `size` and `load_target` which is used by
  `to_ary`).

  Even though the root cause of the problem was mainly happening in
  test, I decided to opt-out the `destroy_all` from checking the
  counter cache value has this could cause potential issue if fonr any
  reason the counter cacher get desync.
  • Loading branch information
Edouard-chin committed Feb 13, 2019
1 parent 1bbf08b commit 2884503
Show file tree
Hide file tree
Showing 3 changed files with 18 additions and 12 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,9 @@ def delete_all(dependent = nil)
#
# See destroy for more info.
def destroy_all
destroy(load_target).tap do
@target = merge_target_lists(find_target, target) if find_target?

destroy(target).tap do
reset
loaded!
end
Expand Down Expand Up @@ -209,7 +211,7 @@ def destroy(*records)
# This method is abstract in the sense that it relies on
# +count_records+, which is a method descendants have to provide.
def size
if !find_target?
if !find_target? && !counter_cache_value&.zero?
loaded! unless loaded?
target.size
elsif @association_ids
Expand Down Expand Up @@ -271,7 +273,7 @@ def include?(record)
end

def load_target
if find_target?
if find_target? && !counter_cache_value&.zero?
@target = merge_target_lists(find_target, target)
end

Expand Down Expand Up @@ -494,6 +496,10 @@ def find_by_scan(*args)
load_target.select { |r| ids.include?(r.id.to_s) }
end
end

def counter_cache_value
reflection.has_cached_counter? ? owner._read_attribute(reflection.counter_cache_column).to_i : nil
end
end
end
end
Original file line number Diff line number Diff line change
Expand Up @@ -62,14 +62,6 @@ def count_records
[association_scope.limit_value, count].compact.min
end

def counter_cache_value
reflection.has_cached_counter? ? owner._read_attribute(reflection.counter_cache_column).to_i : nil
end

def find_target?
super && !counter_cache_value&.zero?
end

def update_counter(difference, reflection = reflection())
if reflection.has_cached_counter?
owner.increment!(reflection.counter_cache_column, difference)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1755,6 +1755,14 @@ def test_destroy_all_on_association_clears_scope
assert_nil posts.first
end

def test_destroy_all_on_desynced_counter_cache_association
category = categories(:general)
assert_operator category.categorizations.count, :>, 0

category.categorizations.destroy_all
assert_equal 0, category.categorizations.count
end

def test_destroy_on_association_clears_scope
author = Author.create!(name: "Gannon")
posts = author.posts
Expand Down Expand Up @@ -2021,7 +2029,7 @@ def test_ids_reader_cache_should_be_cleared_when_collection_is_deleted

def test_zero_counter_cache_usage_on_unloaded_association
car = Car.create!(name: "My AppliCar")
assert_no_queries do
assert_no_queries(ignore_none: false) do
assert_equal car.engines.size, 0
assert_predicate car.engines, :loaded?
end
Expand Down

0 comments on commit 2884503

Please sign in to comment.