From 12cba2ce2481286de8051941c97d3053e3597246 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?R=C3=A9gis=20Hanol?= <regis@hanol.fr>
Date: Tue, 7 May 2024 18:06:16 +0200
Subject: [PATCH] PERF: bail out of expensive post validations

Whenever a post already failed "lightweight" validations, we skip all the expensive validations (that cooks the post or run SQL queries) so that we reply as soon as possible.

Also skip validating polls when there's no "[/poll]" in the raw.

Internal ref - t/115890
---
 lib/validators/post_validator.rb            | 124 ++++-----
 lib/validators/stripped_length_validator.rb |   2 +-
 plugins/poll/lib/poll.rb                    |   3 +
 spec/lib/validators/post_validator_spec.rb  | 276 ++++++++++++--------
 4 files changed, 223 insertions(+), 182 deletions(-)

diff --git a/lib/validators/post_validator.rb b/lib/validators/post_validator.rb
index 0386a4331ef..2977ae5764a 100644
--- a/lib/validators/post_validator.rb
+++ b/lib/validators/post_validator.rb
@@ -4,27 +4,27 @@ class PostValidator < ActiveModel::Validator
   def validate(record)
     presence(record)
 
-    return if record.acting_user.try(:staged?)
-    if record.acting_user.try(:admin?) && Discourse.static_doc_topic_ids.include?(record.topic_id)
-      return
-    end
+    return if record.acting_user&.staged?
+    return if record.acting_user&.admin? && Discourse.static_doc_topic_ids.include?(record.topic_id)
 
     post_body_validator(record)
     max_posts_validator(record)
-    max_mention_validator(record)
-    max_embedded_media_validator(record)
-    max_attachments_validator(record)
-    can_post_links_validator(record)
     unique_post_validator(record)
-    force_edit_last_validator(record)
+
+    # These validators might cook the post or do SQL queries.
+    # So we only run them if the post is otherwise valid.
+    if record.errors.empty?
+      max_mention_validator(record)
+      max_embedded_media_validator(record)
+      max_attachments_validator(record)
+      max_links_validator(record)
+      force_edit_last_validator(record)
+    end
   end
 
   def presence(post)
-    unless options[:skip_topic]
-      post.errors.add(:topic_id, :blank, **options) if post.topic_id.blank?
-    end
-
-    post.errors.add(:user_id, :blank, **options) if post.new_record? && post.user_id.nil?
+    post.errors.add(:topic_id, :blank, **options) if !options[:skip_topic] && post.topic_id.blank?
+    post.errors.add(:user_id, :blank, **options) if post.new_record? && post.user_id.blank?
   end
 
   def post_body_validator(post)
@@ -37,31 +37,40 @@ class PostValidator < ActiveModel::Validator
   def stripped_length(post)
     range =
       if private_message?(post)
-        # private message
         SiteSetting.private_message_post_length
       elsif post.is_first_post? || (post.topic.present? && post.topic.posts_count == 0)
-        # creating/editing first post
         if post.topic&.featured_link&.present?
           (0..SiteSetting.max_post_length)
         else
           SiteSetting.first_post_length
         end
       else
-        # regular post
         SiteSetting.post_length
       end
 
     StrippedLengthValidator.validate(post, :raw, post.raw, range)
   end
 
-  def raw_quality(post)
-    sentinel = TextSentinel.body_sentinel(post.raw, private_message: private_message?(post))
-    post.errors.add(:raw, I18n.t(:is_invalid)) unless sentinel.valid?
+  def max_posts_validator(post)
+    if post.new_record? && post.acting_user&.posted_too_much_in_topic?(post.topic_id)
+      post.errors.add(
+        :base,
+        I18n.t(:too_many_replies, count: SiteSetting.newuser_max_replies_per_topic),
+      )
+    end
+  end
+
+  def unique_post_validator(post)
+    return if SiteSetting.unique_posts_mins == 0
+    return if post.skip_unique_check
+    return if post.acting_user&.staff?
+    return if post.raw.blank?
+
+    post.errors.add(:raw, I18n.t(:just_posted_that)) if post.matches_recent_post?
   end
 
-  # Ensure maximum amount of mentions in a post
   def max_mention_validator(post)
-    return if post.acting_user.try(:staff?)
+    return if post.acting_user&.staff?
 
     if acting_user_is_trusted?(post) || private_message?(post)
       add_error_if_count_exceeded(
@@ -82,19 +91,9 @@ class PostValidator < ActiveModel::Validator
     end
   end
 
-  def max_posts_validator(post)
-    if post.new_record? && post.acting_user.present? &&
-         post.acting_user.posted_too_much_in_topic?(post.topic_id)
-      post.errors.add(
-        :base,
-        I18n.t(:too_many_replies, count: SiteSetting.newuser_max_replies_per_topic),
-      )
-    end
-  end
-
-  # Ensure new users can not put too many media embeds (images, video, audio) in a post
   def max_embedded_media_validator(post)
-    return if post.acting_user.blank? || post.acting_user&.staff?
+    return if post.acting_user.nil?
+    return if post.acting_user.staff?
 
     if !post.acting_user.in_any_groups?(SiteSetting.embedded_media_post_allowed_groups_map)
       add_error_if_count_exceeded(
@@ -115,9 +114,9 @@ class PostValidator < ActiveModel::Validator
     end
   end
 
-  # Ensure new users can not put too many attachments in a post
   def max_attachments_validator(post)
     return if acting_user_is_trusted?(post) || private_message?(post)
+
     add_error_if_count_exceeded(
       post,
       :no_attachments_allowed,
@@ -127,7 +126,7 @@ class PostValidator < ActiveModel::Validator
     )
   end
 
-  def can_post_links_validator(post)
+  def max_links_validator(post)
     if (post.link_count == 0 && !post.has_oneboxes?) || private_message?(post)
       return newuser_links_validator(post)
     end
@@ -140,35 +139,11 @@ class PostValidator < ActiveModel::Validator
     post.errors.add(:base, I18n.t(:links_require_trust))
   end
 
-  # Ensure new users can not put too many links in a post
-  def newuser_links_validator(post)
-    return if acting_user_is_trusted?(post) || private_message?(post)
-    add_error_if_count_exceeded(
-      post,
-      :no_links_allowed,
-      :too_many_links,
-      post.link_count,
-      SiteSetting.newuser_max_links,
-    )
-  end
-
-  # Stop us from posting the same thing too quickly
-  def unique_post_validator(post)
-    return if SiteSetting.unique_posts_mins == 0
-    return if post.skip_unique_check
-    return if post.acting_user.try(:staff?)
-
-    # If the post is empty, default to the validates_presence_of
-    return if post.raw.blank?
-
-    post.errors.add(:raw, I18n.t(:just_posted_that)) if post.matches_recent_post?
-  end
-
   def force_edit_last_validator(post)
-    if SiteSetting.max_consecutive_replies == 0 || post.id || post.acting_user&.staff? ||
-         private_message?(post)
-      return
-    end
+    return if post.id
+    return if private_message?(post)
+    return if post.acting_user&.staff?
+    return if SiteSetting.max_consecutive_replies == 0
 
     topic = post.topic
     return if topic&.ordered_posts&.first&.user == post.user
@@ -207,12 +182,29 @@ class PostValidator < ActiveModel::Validator
 
   private
 
+  def raw_quality(post)
+    return if TextSentinel.body_sentinel(post.raw, private_message: private_message?(post)).valid?
+    post.errors.add(:raw, I18n.t(:is_invalid))
+  end
+
   def acting_user_is_trusted?(post, level = 1)
-    post.acting_user.present? && post.acting_user.has_trust_level?(TrustLevel[level])
+    post.acting_user&.has_trust_level?(TrustLevel[level])
   end
 
   def private_message?(post)
-    post.topic.try(:private_message?) || options[:private_message]
+    post.topic&.private_message? || options[:private_message]
+  end
+
+  def newuser_links_validator(post)
+    return if acting_user_is_trusted?(post) || private_message?(post)
+
+    add_error_if_count_exceeded(
+      post,
+      :no_links_allowed,
+      :too_many_links,
+      post.link_count,
+      SiteSetting.newuser_max_links,
+    )
   end
 
   def add_error_if_count_exceeded(
diff --git a/lib/validators/stripped_length_validator.rb b/lib/validators/stripped_length_validator.rb
index 2cef231b18f..984f0e37da7 100644
--- a/lib/validators/stripped_length_validator.rb
+++ b/lib/validators/stripped_length_validator.rb
@@ -2,7 +2,7 @@
 
 class StrippedLengthValidator < ActiveModel::EachValidator
   def self.validate(record, attribute, value, range)
-    if value.nil?
+    if value.blank?
       record.errors.add attribute, I18n.t("errors.messages.blank")
     elsif value.length > range.end
       record.errors.add attribute,
diff --git a/plugins/poll/lib/poll.rb b/plugins/poll/lib/poll.rb
index 37c370c2b5b..f51f43cf9c1 100644
--- a/plugins/poll/lib/poll.rb
+++ b/plugins/poll/lib/poll.rb
@@ -318,6 +318,9 @@ class DiscoursePoll::Poll
     # creation process. `raw` could be nil here.
     return [] if raw.blank?
 
+    # bail-out early if the post does not contain a poll
+    return [] if !raw.include?("[/poll]")
+
     # TODO: we should fix the callback mess so that the cooked version is available
     # in the validators instead of cooking twice
     raw = raw.sub(%r{\[quote.+/quote\]}m, "")
diff --git a/spec/lib/validators/post_validator_spec.rb b/spec/lib/validators/post_validator_spec.rb
index 75962203d7a..256d75d9c74 100644
--- a/spec/lib/validators/post_validator_spec.rb
+++ b/spec/lib/validators/post_validator_spec.rb
@@ -5,7 +5,7 @@ RSpec.describe PostValidator do
   let(:post) { build(:post, topic: topic) }
   let(:validator) { PostValidator.new({}) }
 
-  describe "#post_body_validator" do
+  describe "post_body_validator" do
     it "should not allow a post with an empty raw" do
       post.raw = ""
       validator.post_body_validator(post)
@@ -105,7 +105,7 @@ RSpec.describe PostValidator do
     end
   end
 
-  describe "too_many_posts" do
+  describe "max_posts_validator" do
     it "should be invalid when the user has posted too much" do
       post.user.expects(:posted_too_much_in_topic?).returns(true)
       validator.max_posts_validator(post)
@@ -126,115 +126,6 @@ RSpec.describe PostValidator do
     end
   end
 
-  describe "too_many_mentions" do
-    before do
-      SiteSetting.newuser_max_mentions_per_post = 2
-      SiteSetting.max_mentions_per_post = 3
-    end
-
-    it "should be invalid when new user exceeds max mentions limit" do
-      post.acting_user = build(:newuser)
-      post.expects(:raw_mentions).returns(%w[jake finn jake_old])
-      validator.max_mention_validator(post)
-      expect(post.errors.count).to be > 0
-    end
-
-    it "should be invalid when leader user exceeds max mentions limit" do
-      post.acting_user = build(:trust_level_4)
-      post.expects(:raw_mentions).returns(%w[jake finn jake_old jake_new])
-      validator.max_mention_validator(post)
-      expect(post.errors.count).to be > 0
-    end
-
-    it "should be valid when new user does not exceed max mentions limit" do
-      post.acting_user = build(:newuser)
-      post.expects(:raw_mentions).returns(%w[jake finn])
-      validator.max_mention_validator(post)
-      expect(post.errors.count).to be(0)
-    end
-
-    it "should be valid when new user exceeds max mentions limit in PM" do
-      post.acting_user = build(:newuser)
-      post.topic.expects(:private_message?).returns(true)
-      post.expects(:raw_mentions).returns(%w[jake finn jake_old])
-      validator.max_mention_validator(post)
-      expect(post.errors.count).to be(0)
-    end
-
-    it "should be valid when leader user does not exceed max mentions limit" do
-      post.acting_user = build(:trust_level_4)
-      post.expects(:raw_mentions).returns(%w[jake finn jake_old])
-      validator.max_mention_validator(post)
-      expect(post.errors.count).to be(0)
-    end
-
-    it "should be valid for moderator in all cases" do
-      post.acting_user = build(:moderator)
-      post.expects(:raw_mentions).never
-      validator.max_mention_validator(post)
-      expect(post.errors.count).to be(0)
-    end
-
-    it "should be valid for admin in all cases" do
-      post.acting_user = build(:admin)
-      post.expects(:raw_mentions).never
-      validator.max_mention_validator(post)
-      expect(post.errors.count).to be(0)
-    end
-  end
-
-  describe "too_many_embedded_media" do
-    fab!(:new_user) { Fabricate(:newuser, refresh_auto_groups: true) }
-
-    before do
-      SiteSetting.embedded_media_post_allowed_groups = Group::AUTO_GROUPS[:trust_level_0]
-      SiteSetting.newuser_max_embedded_media = 2
-    end
-
-    it "should be invalid when new user exceeds max mentions limit" do
-      post.acting_user = new_user
-      post.expects(:embedded_media_count).returns(3)
-      validator.max_embedded_media_validator(post)
-      expect(post.errors.count).to be > 0
-    end
-
-    it "should be valid when new user does not exceed max mentions limit" do
-      post.acting_user = new_user
-      post.expects(:embedded_media_count).returns(2)
-      validator.max_embedded_media_validator(post)
-      expect(post.errors.count).to be(0)
-    end
-
-    it "should be invalid when user trust level is not sufficient" do
-      SiteSetting.embedded_media_post_allowed_groups = Group::AUTO_GROUPS[:trust_level_4]
-      post.acting_user = Fabricate(:leader, groups: [])
-      post.expects(:embedded_media_count).returns(2)
-      validator.max_embedded_media_validator(post)
-      expect(post.errors.count).to be > 0
-    end
-
-    it "should be valid for moderator in all cases" do
-      post.acting_user = Fabricate(:moderator)
-      post.expects(:embedded_media_count).never
-      validator.max_embedded_media_validator(post)
-      expect(post.errors.count).to be(0)
-    end
-
-    it "should be valid for admin in all cases" do
-      post.acting_user = Fabricate(:admin)
-      post.expects(:embedded_media_count).never
-      validator.max_embedded_media_validator(post)
-      expect(post.errors.count).to be(0)
-    end
-  end
-
-  describe "invalid post" do
-    it "should be invalid" do
-      validator.validate(post)
-      expect(post.errors.count).to be > 0
-    end
-  end
-
   describe "unique_post_validator" do
     fab!(:user)
     fab!(:post) { Fabricate(:post, raw: "Non PM topic body", user: user, topic: topic) }
@@ -319,6 +210,162 @@ RSpec.describe PostValidator do
     end
   end
 
+  describe "max_mention_validator" do
+    before do
+      SiteSetting.newuser_max_mentions_per_post = 2
+      SiteSetting.max_mentions_per_post = 3
+    end
+
+    it "should be invalid when new user exceeds max mentions limit" do
+      post.acting_user = build(:newuser)
+      post.expects(:raw_mentions).returns(%w[jake finn jake_old])
+      validator.max_mention_validator(post)
+      expect(post.errors.count).to be > 0
+    end
+
+    it "should be invalid when leader user exceeds max mentions limit" do
+      post.acting_user = build(:trust_level_4)
+      post.expects(:raw_mentions).returns(%w[jake finn jake_old jake_new])
+      validator.max_mention_validator(post)
+      expect(post.errors.count).to be > 0
+    end
+
+    it "should be valid when new user does not exceed max mentions limit" do
+      post.acting_user = build(:newuser)
+      post.expects(:raw_mentions).returns(%w[jake finn])
+      validator.max_mention_validator(post)
+      expect(post.errors.count).to be(0)
+    end
+
+    it "should be valid when new user exceeds max mentions limit in PM" do
+      post.acting_user = build(:newuser)
+      post.topic.expects(:private_message?).returns(true)
+      post.expects(:raw_mentions).returns(%w[jake finn jake_old])
+      validator.max_mention_validator(post)
+      expect(post.errors.count).to be(0)
+    end
+
+    it "should be valid when leader user does not exceed max mentions limit" do
+      post.acting_user = build(:trust_level_4)
+      post.expects(:raw_mentions).returns(%w[jake finn jake_old])
+      validator.max_mention_validator(post)
+      expect(post.errors.count).to be(0)
+    end
+
+    it "should be valid for moderator in all cases" do
+      post.acting_user = build(:moderator)
+      post.expects(:raw_mentions).never
+      validator.max_mention_validator(post)
+      expect(post.errors.count).to be(0)
+    end
+
+    it "should be valid for admin in all cases" do
+      post.acting_user = build(:admin)
+      post.expects(:raw_mentions).never
+      validator.max_mention_validator(post)
+      expect(post.errors.count).to be(0)
+    end
+  end
+
+  describe "max_embedded_media_validator" do
+    fab!(:new_user) { Fabricate(:newuser, refresh_auto_groups: true) }
+
+    before do
+      SiteSetting.embedded_media_post_allowed_groups = Group::AUTO_GROUPS[:trust_level_0]
+      SiteSetting.newuser_max_embedded_media = 2
+    end
+
+    it "should be invalid when new user exceeds max mentions limit" do
+      post.acting_user = new_user
+      post.expects(:embedded_media_count).returns(3)
+      validator.max_embedded_media_validator(post)
+      expect(post.errors.count).to be > 0
+    end
+
+    it "should be valid when new user does not exceed max mentions limit" do
+      post.acting_user = new_user
+      post.expects(:embedded_media_count).returns(2)
+      validator.max_embedded_media_validator(post)
+      expect(post.errors.count).to be(0)
+    end
+
+    it "should be invalid when user trust level is not sufficient" do
+      SiteSetting.embedded_media_post_allowed_groups = Group::AUTO_GROUPS[:trust_level_4]
+      post.acting_user = Fabricate(:leader, groups: [])
+      post.expects(:embedded_media_count).returns(2)
+      validator.max_embedded_media_validator(post)
+      expect(post.errors.count).to be > 0
+    end
+
+    it "should be valid for moderator in all cases" do
+      post.acting_user = Fabricate(:moderator)
+      post.expects(:embedded_media_count).never
+      validator.max_embedded_media_validator(post)
+      expect(post.errors.count).to be(0)
+    end
+
+    it "should be valid for admin in all cases" do
+      post.acting_user = Fabricate(:admin)
+      post.expects(:embedded_media_count).never
+      validator.max_embedded_media_validator(post)
+      expect(post.errors.count).to be(0)
+    end
+  end
+
+  describe "max_attachments_validator" do
+    fab!(:new_user) { Fabricate(:newuser) }
+
+    before { SiteSetting.newuser_max_attachments = 2 }
+
+    it "should be invalid when new user exceeds max attachments limit" do
+      post.acting_user = new_user
+      post.expects(:attachment_count).returns(3)
+      validator.max_attachments_validator(post)
+      expect(post.errors.count).to be > 0
+    end
+
+    it "should be valid when new user does not exceed max attachments limit" do
+      post.acting_user = new_user
+      post.expects(:attachment_count).returns(2)
+      validator.max_attachments_validator(post)
+      expect(post.errors.count).to be(0)
+    end
+
+    it "should be valid for moderator in all cases" do
+      post.acting_user = Fabricate(:moderator)
+      post.expects(:attachment_count).never
+      validator.max_attachments_validator(post)
+      expect(post.errors.count).to be(0)
+    end
+
+    it "should be valid for admin in all cases" do
+      post.acting_user = Fabricate(:admin)
+      post.expects(:attachment_count).never
+      validator.max_attachments_validator(post)
+      expect(post.errors.count).to be(0)
+    end
+  end
+
+  describe "max_links_validator" do
+    fab!(:new_user) { Fabricate(:newuser) }
+
+    before { SiteSetting.newuser_max_links = 2 }
+
+    it "should be invalid when new user exceeds max links limit" do
+      post.acting_user = new_user
+      post.stubs(:link_count).returns(3)
+      validator.max_links_validator(post)
+      expect(post.errors.count).to be > 0
+    end
+
+    it "should be valid when new user does not exceed max links limit" do
+      post.acting_user = new_user
+      post.stubs(:link_count).returns(2)
+      validator.max_links_validator(post)
+      expect(post.errors.count).to be(0)
+    end
+  end
+
   describe "force_edit_last_validator" do
     fab!(:user) { Fabricate(:user, refresh_auto_groups: true) }
     fab!(:other_user) { Fabricate(:user) }
@@ -381,14 +428,13 @@ RSpec.describe PostValidator do
 
   shared_examples "almost no validations" do
     it "skips most validations" do
-      validator.expects(:stripped_length).never
-      validator.expects(:raw_quality).never
+      validator.expects(:post_body_validator).never
       validator.expects(:max_posts_validator).never
+      validator.expects(:unique_post_validator).never
       validator.expects(:max_mention_validator).never
       validator.expects(:max_embedded_media_validator).never
       validator.expects(:max_attachments_validator).never
-      validator.expects(:newuser_links_validator).never
-      validator.expects(:unique_post_validator).never
+      validator.expects(:max_links_validator).never
       validator.expects(:force_edit_last_validator).never
       validator.validate(post)
     end