FIX: Avoid errors when updating post and topic count user stats. ()

In ab5361d69ae4b55a7635676eef7d80dd96d8460c, we rescue from the PG error
but the transaction is already aborted causing any DB query after to
fail. As such, we avoid triggering the error in the first place by
checking that we would not be insertin a negative number into the
counter cache.

Follow-up to ab5361d69ae4b55a7635676eef7d80dd96d8460c
This commit is contained in:
Alan Guo Xiang Tan 2022-02-09 21:48:18 +08:00 committed by GitHub
parent 1fb97f8bba
commit ae0625323a
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
2 changed files with 22 additions and 16 deletions

@ -17,23 +17,24 @@ class UserStatCountUpdater
return if post.topic.private_message? return if post.topic.private_message?
stat = user_stat || post.user.user_stat stat = user_stat || post.user.user_stat
column =
if post.is_first_post? if post.is_first_post?
stat.public_send(action, :topic_count) :topic_count
elsif post.post_type == Post.types[:regular] elsif post.post_type == Post.types[:regular]
stat.public_send(action, :post_count) :post_count
end end
rescue ActiveRecord::StatementInvalid => e
if e.cause.is_a?(PG::CheckViolation) return if column.blank?
if action == :decrement! && stat.public_send(column) < 1
# There are still spots in the code base which results in the counter cache going out of sync. However, # There are still spots in the code base which results in the counter cache going out of sync. However,
# we have a job that runs on a daily basis which will correct the count. Therefore, avoid raising an error for now # we have a job that runs on a daily basis which will correct the count. Therefore, we always check that we
# and log the exception instead. # wouldn't end up with a negative count first before inserting.
Discourse.warn_exception( Rails.logger.warn("Attempted to insert negative count into UserStat##{column}\n#{caller.join('\n')}")
e, return
message: "Attempted to insert negative count into UserStat#post_count or UserStat#topic_count"
)
else
raise
end end
stat.public_send(action, column)
end end
end end
end end

@ -6,6 +6,7 @@ describe UserStatCountUpdater do
fab!(:user) { Fabricate(:user) } fab!(:user) { Fabricate(:user) }
fab!(:user_stat) { user.user_stat } fab!(:user_stat) { user.user_stat }
fab!(:post) { Fabricate(:post) } fab!(:post) { Fabricate(:post) }
fab!(:post_2) { Fabricate(:post, topic: post.topic) }
before do before do
@orig_logger = Rails.logger @orig_logger = Rails.logger
@ -19,6 +20,10 @@ describe UserStatCountUpdater do
it 'should log the exception when a negative count is inserted' do it 'should log the exception when a negative count is inserted' do
UserStatCountUpdater.decrement!(post, user_stat: user_stat) UserStatCountUpdater.decrement!(post, user_stat: user_stat)
expect(@fake_logger.warnings.first).to include("PG::CheckViolation") expect(@fake_logger.warnings.last).to match("topic_count")
UserStatCountUpdater.decrement!(post_2, user_stat: user_stat)
expect(@fake_logger.warnings.last).to match("post_count")
end end
end end