diff --git a/app/services/user_stat_count_updater.rb b/app/services/user_stat_count_updater.rb index 53995d3f328..dac3e086b40 100644 --- a/app/services/user_stat_count_updater.rb +++ b/app/services/user_stat_count_updater.rb @@ -3,16 +3,29 @@ class UserStatCountUpdater class << self def increment!(post, user_stat: nil) - update!(post, user_stat: user_stat) + update_using_operator!(post, user_stat: user_stat, action: :increment!) end def decrement!(post, user_stat: nil) - update!(post, user_stat: user_stat, action: :decrement!) + update_using_operator!(post, user_stat: user_stat, action: :decrement!) + end + + def set!(user_stat:, count:, count_column:) + return if user_stat.blank? + return if ![:post_count, :topic_count].include?(count_column) + + if SiteSetting.verbose_user_stat_count_logging && count < 0 + Rails.logger.warn( + "Attempted to insert negative count into UserStat##{count_column} for user #{user_stat.user_id}, using 0 instead. Caller:\n #{caller[0..10].join("\n")}" + ) + end + + user_stat.update_column(count_column, [count, 0].max) end private - def update!(post, user_stat: nil, action: :increment!) + def update_using_operator!(post, user_stat: nil, action: :increment!) return if !post&.topic return if action == :increment! && post.topic.private_message? stat = user_stat || post.user&.user_stat @@ -33,7 +46,9 @@ class UserStatCountUpdater # to trigger an error. if action == :decrement! && stat.public_send(column) < 1 if SiteSetting.verbose_user_stat_count_logging - Rails.logger.warn("Attempted to insert negative count into UserStat##{column} for post with id '#{post.id}'") + Rails.logger.warn( + "Attempted to insert negative count into UserStat##{column} for post with id '#{post.id}'. Caller:\n #{caller[0..10].join("\n")}" + ) end return diff --git a/db/post_migrate/20220215015538_drop_user_stat_count_constraints.rb b/db/post_migrate/20220215015538_drop_user_stat_count_constraints.rb new file mode 100644 index 00000000000..2d30f885623 --- /dev/null +++ b/db/post_migrate/20220215015538_drop_user_stat_count_constraints.rb @@ -0,0 +1,25 @@ +# frozen_string_literal: true + +class DropUserStatCountConstraints < ActiveRecord::Migration[6.1] + def up + execute "ALTER TABLE user_stats DROP CONSTRAINT topic_count_positive" + execute "ALTER TABLE user_stats DROP CONSTRAINT post_count_positive" + end + + def down + execute(<<~SQL) + UPDATE user_stats + SET post_count = 0 + WHERE post_count < 0 + SQL + + execute(<<~SQL) + UPDATE user_stats + SET topic_count = 0 + WHERE topic_count < 0 + SQL + + execute "ALTER TABLE user_stats ADD CONSTRAINT topic_count_positive CHECK (topic_count >= 0)" + execute "ALTER TABLE user_stats ADD CONSTRAINT post_count_positive CHECK (post_count >= 0)" + end +end diff --git a/lib/post_destroyer.rb b/lib/post_destroyer.rb index 8921a715122..f29acb7acf8 100644 --- a/lib/post_destroyer.rb +++ b/lib/post_destroyer.rb @@ -5,7 +5,6 @@ # this class contains the logic to delete it. # class PostDestroyer - def self.destroy_old_hidden_posts Post.where(deleted_at: nil, hidden: true) .where("hidden_at < ?", 30.days.ago) @@ -132,12 +131,7 @@ class PostDestroyer if @post.is_first_post? # Update stats of all people who replied - counts = Post.where(post_type: Post.types[:regular], topic_id: @post.topic_id).where('post_number > 1').group(:user_id).count - counts.each do |user_id, count| - if user_stat = UserStat.where(user_id: user_id).first - user_stat.update(post_count: user_stat.post_count + count) - end - end + update_post_counts(:increment) end end @@ -400,13 +394,7 @@ class PostDestroyer if @post.is_first_post? && @post.topic && !@post.topic.private_message? # Update stats of all people who replied - counts = Post.where(post_type: Post.types[:regular], topic_id: @post.topic_id).where('post_number > 1').group(:user_id).count - - counts.each do |user_id, count| - if user_stat = UserStat.where(user_id: user_id).first - user_stat.update(post_count: user_stat.post_count - count) - end - end + update_post_counts(:decrement) end end @@ -417,4 +405,19 @@ class PostDestroyer incoming.update(imap_sync: sync) end + def update_post_counts(operator) + counts = Post.where( + post_type: Post.types[:regular], topic_id: @post.topic_id + ).where('post_number > 1').group(:user_id).count + + counts.each do |user_id, count| + if user_stat = UserStat.where(user_id: user_id).first + if operator == :decrement + UserStatCountUpdater.set!(user_stat: user_stat, count: user_stat.post_count - count, count_column: :post_count) + else + UserStatCountUpdater.set!(user_stat: user_stat, count: user_stat.post_count + count, count_column: :post_count) + end + end + end + end end diff --git a/spec/components/post_destroyer_spec.rb b/spec/components/post_destroyer_spec.rb index d510e6cc5f5..7b8426e4193 100644 --- a/spec/components/post_destroyer_spec.rb +++ b/spec/components/post_destroyer_spec.rb @@ -442,6 +442,21 @@ describe PostDestroyer do expect(user2.user_stat.post_count).to eq(0) end + it "does not update post_count or topic_count to a negative number" do + user1 = post.user + reply2 = create_post(topic_id: post.topic_id, user: user1) + expect(user1.user_stat.topic_count).to eq(1) + expect(user1.user_stat.post_count).to eq(1) + + user1.user_stat.update!(topic_count: 0) + user1.user_stat.update!(post_count: 0) + + PostDestroyer.new(admin, post).destroy + user1.reload + expect(user1.user_stat.topic_count).to eq(0) + expect(user1.user_stat.post_count).to eq(0) + end + it 'deletes the published page associated with the topic' do slug = 'my-published-page' publish_result = PublishedPage.publish!(admin, post.topic, slug) diff --git a/spec/services/user_stat_count_updater_spec.rb b/spec/services/user_stat_count_updater_spec.rb index 35fbbf05ff1..e3a7bbb8caa 100644 --- a/spec/services/user_stat_count_updater_spec.rb +++ b/spec/services/user_stat_count_updater_spec.rb @@ -29,4 +29,13 @@ describe UserStatCountUpdater do expect(@fake_logger.warnings.last).to match("post_count") expect(@fake_logger.warnings.last).to match(post_2.id.to_s) end + + it 'should log the exception when a negative count will be inserted but 0 is used instead' do + UserStatCountUpdater.set!(user_stat: user_stat, count: -10, count_column: :post_count) + + expect(@fake_logger.warnings.last).to match("post_count") + expect(@fake_logger.warnings.last).to match("using 0") + expect(@fake_logger.warnings.last).to match("user #{user_stat.user_id}") + expect(user_stat.reload.post_count).to eq(0) + end end