Skip to content

Commit

Permalink
Don't wrap updates in counter_culture_active
Browse files Browse the repository at this point in the history
This is an artifact of updates running after commit by default, which is
no longer the default, or in fact even tested. It causes to problems as
demonstrated by the new (formerly failing) test case.

This also deprecates execute_after_commit, as it is currently untested
and there is no real reason to use it anymore.
  • Loading branch information
magnusvk committed Jun 12, 2018
1 parent f00e305 commit 81dfbf5
Show file tree
Hide file tree
Showing 2 changed files with 58 additions and 43 deletions.
73 changes: 30 additions & 43 deletions lib/counter_culture/extensions.rb
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,12 @@ def counter_culture(relation, options = {})
raise ":column_names must be a Hash of conditions and column names"
end

if options[:execute_after_commit]
ActiveSupport::Deprecation.warn(
'execute_after_commit is deprecated and will be removed from '\
'counter_culture 2.0')
end

# add the counter to our collection
@after_commit_counter_cache << Counter.new(self, relation, options)
end
Expand Down Expand Up @@ -89,61 +95,42 @@ def counter_culture_fix_counts(options = {})
end

private
# need to make sure counter_culture is only activated once
# per commit; otherwise, if we do an update in an after_create,
# we would be triggered twice within the same transaction -- once
# for the create, once for the update
def _wrap_in_counter_culture_active(&block)
if @_counter_culture_active
# don't do anything; we are already active for this transaction
else
block.call
execute_after_commit { @_counter_culture_active = false}
end
end

# called by after_create callback
def _update_counts_after_create
_wrap_in_counter_culture_active do
@_counter_culture_active = true
self.class.after_commit_counter_cache.each do |counter|
# increment counter cache
counter.change_counter_cache(self, :increment => true)
end
@_counter_culture_active = true
self.class.after_commit_counter_cache.each do |counter|
# increment counter cache
counter.change_counter_cache(self, :increment => true)
end
end

# called by after_destroy callback
def _update_counts_after_destroy
_wrap_in_counter_culture_active do
@_counter_culture_active = true
self.class.after_commit_counter_cache.each do |counter|
# decrement counter cache
counter.change_counter_cache(self, :increment => false)
end
@_counter_culture_active = true
self.class.after_commit_counter_cache.each do |counter|
# decrement counter cache
counter.change_counter_cache(self, :increment => false)
end
end

# called by after_update callback
def _update_counts_after_update
_wrap_in_counter_culture_active do
self.class.after_commit_counter_cache.each do |counter|
# figure out whether the applicable counter cache changed (this can happen
# with dynamic column names)
counter_cache_name_was = counter.counter_cache_name_for(counter.previous_model(self))
counter_cache_name = counter.counter_cache_name_for(self)

if counter.first_level_relation_changed?(self) ||
(counter.delta_column && counter.attribute_changed?(self, counter.delta_column)) ||
counter_cache_name != counter_cache_name_was

@_counter_culture_active = true

# increment the counter cache of the new value
counter.change_counter_cache(self, :increment => true, :counter_column => counter_cache_name)
# decrement the counter cache of the old value
counter.change_counter_cache(self, :increment => false, :was => true, :counter_column => counter_cache_name_was)
end
self.class.after_commit_counter_cache.each do |counter|
# figure out whether the applicable counter cache changed (this can happen
# with dynamic column names)
counter_cache_name_was = counter.counter_cache_name_for(counter.previous_model(self))
counter_cache_name = counter.counter_cache_name_for(self)

if counter.first_level_relation_changed?(self) ||
(counter.delta_column && counter.attribute_changed?(self, counter.delta_column)) ||
counter_cache_name != counter_cache_name_was

@_counter_culture_active = true

# increment the counter cache of the new value
counter.change_counter_cache(self, :increment => true, :counter_column => counter_cache_name)
# decrement the counter cache of the old value
counter.change_counter_cache(self, :increment => false, :was => true, :counter_column => counter_cache_name_was)
end
end
end
Expand Down
28 changes: 28 additions & 0 deletions spec/counter_culture_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,34 @@
expect(user2.reload.review_approvals_count).to eq(69)
end

it "works with multiple saves in one transcation" do
user = User.create
product = Product.create

expect(user.reviews_count).to eq(0)
expect(user.review_approvals_count).to eq(0)

Review.transaction do
review1 = Review.create!(user_id: user.id, product_id: product.id, approvals: 0)

user.reload
expect(user.reviews_count).to eq(1)
expect(user.review_approvals_count).to eq(0)

review1.update_attributes!(approvals: 42)

user.reload
expect(user.reviews_count).to eq(1)
expect(user.review_approvals_count).to eq(42)

review2 = Review.create!(user_id: user.id, product_id: product.id, approvals: 1)

user.reload
expect(user.reviews_count).to eq(2)
expect(user.review_approvals_count).to eq(43)
end
end

it "treats null delta column values as 0" do
user = User.create
product = Product.create
Expand Down

0 comments on commit 81dfbf5

Please sign in to comment.