DEV: Drop user_stats count column constraints (#15949)

We added this constraint in 5bd55acf835e79ba8f8b655c684035c3f2293652
but it is causing problems in hosted sites and is catching the
issue too far down the line. This commit removes the constraint
for now, and also fixes an issue found with PostDestroyer
which wasn't using the UserStatCountUpdater when updating post_count
and thus was causing negative numbers to occur.

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

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

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

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)

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