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

@@ -4,27 +4,27 @@ class PostValidator < ActiveModel::Validator
   def validate(record)
     presence(record)
 
-    return if record.acting_user.try(:staged?)
+    return if record.acting_user&.staged?
-    if record.acting_user.try(:admin?) && Discourse.static_doc_topic_ids.include?(record.topic_id)
+    return if record.acting_user&.admin? && Discourse.static_doc_topic_ids.include?(record.topic_id)
-      return
-    end
 
     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 !options[:skip_topic] && post.topic_id.blank?
-      post.errors.add(:topic_id, :blank, **options) if post.topic_id.blank?
+    post.errors.add(:user_id, :blank, **options) if post.new_record? && post.user_id.blank?
-    end
-
-    post.errors.add(:user_id, :blank, **options) if post.new_record? && post.user_id.nil?
   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)
+  def max_posts_validator(post)
-    sentinel = TextSentinel.body_sentinel(post.raw, private_message: private_message?(post))
+    if post.new_record? && post.acting_user&.posted_too_much_in_topic?(post.topic_id)
-    post.errors.add(:raw, I18n.t(:is_invalid)) unless sentinel.valid?
+      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? ||
+    return if post.id
-         private_message?(post)
+    return if private_message?(post)
-      return
+    return if post.acting_user&.staff?
-    end
+    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(

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,

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, "")

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(:post_body_validator).never
-      validator.expects(:raw_quality).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(:max_links_validator).never
-      validator.expects(:unique_post_validator).never
       validator.expects(:force_edit_last_validator).never
       validator.validate(post)
     end