PERF: Reduce overhead from chat message excerpt (#26712)

This change moves the chat message excerpt into a new database column (string) on the chat_messages table. As part of this change, we will now set the excerpt within the `Chat::CreateMessage` service, and update it within the `Chat::UpdateMessage` service.
2024-04-25 20:29:00 +08:00 · 2024-04-25 20:29:00 +08:00 · c62d3610c6
parent 14fc029a30
commit c62d3610c6
19 changed files with 84 additions and 41 deletions
--- a/plugins/chat/app/models/chat/message.rb
+++ b/plugins/chat/app/models/chat/message.rb
@ -8,6 +8,7 @@ module Chat
    self.table_name = "chat_messages"

    BAKED_VERSION = 2
+    EXCERPT_LENGTH = 150

    attribute :has_oneboxes, default: false

@ -122,7 +123,7 @@ module Chat
      end
    end

-    def excerpt(max_length: 100)
+    def build_excerpt
      # just show the URL if the whole message is a URL, because we cannot excerpt oneboxes
      return message if UrlHelper.relaxed_parse(message).is_a?(URI)

@ -130,11 +131,7 @@ module Chat
      return uploads.first.original_filename if cooked.blank? && uploads.present?

      # this may return blank for some complex things like quotes, that is acceptable
-      PrettyText.excerpt(cooked, max_length, strip_links: true, keep_mentions: true)
-    end
-
-    def censored_excerpt(max_length: 100)
-      WordWatcher.censor(excerpt(max_length: max_length))
+      PrettyText.excerpt(cooked, EXCERPT_LENGTH, strip_links: true, keep_mentions: true)
    end

    def cooked_for_excerpt
--- a/plugins/chat/app/models/chat/thread.rb
+++ b/plugins/chat/app/models/chat/thread.rb
@ -2,7 +2,6 @@

 module Chat
  class Thread < ActiveRecord::Base
-    EXCERPT_LENGTH = 150
    MAX_TITLE_LENGTH = 100

    include Chat::ThreadCache
@ -68,7 +67,7 @@ module Chat
    end

    def excerpt
-      original_message.excerpt(max_length: EXCERPT_LENGTH)
+      original_message.excerpt
    end

    def update_last_message_id!
--- a/plugins/chat/app/serializers/chat/in_reply_to_serializer.rb
+++ b/plugins/chat/app/serializers/chat/in_reply_to_serializer.rb
@ -7,10 +7,6 @@ module Chat

    attributes :id, :cooked, :excerpt

-    def excerpt
-      object.censored_excerpt
-    end
-
    def user
      object.user || Chat::NullUser.new
    end
--- a/plugins/chat/app/serializers/chat/message_serializer.rb
+++ b/plugins/chat/app/serializers/chat/message_serializer.rb
@ -57,7 +57,7 @@ module Chat
    end

    def excerpt
-      object.censored_excerpt
+      object.excerpt || object.build_excerpt
    end

    def reactions
--- a/plugins/chat/app/serializers/chat/thread_original_message_serializer.rb
+++ b/plugins/chat/app/serializers/chat/thread_original_message_serializer.rb
@ -12,10 +12,6 @@ module Chat
               :mentioned_users,
               :user

-    def excerpt
-      object.censored_excerpt
-    end
-
    def mentioned_users
      object
        .user_mentions
--- a/plugins/chat/app/serializers/chat/thread_preview_serializer.rb
+++ b/plugins/chat/app/serializers/chat/thread_preview_serializer.rb
@ -28,7 +28,7 @@ module Chat
    end

    def last_reply_excerpt
-      object.last_message.excerpt(max_length: Chat::Thread::EXCERPT_LENGTH)
+      object.last_message.excerpt
    end

    def last_reply_user
--- a/plugins/chat/app/services/chat/create_message.rb
+++ b/plugins/chat/app/services/chat/create_message.rb
@ -37,6 +37,7 @@ module Chat
    model :message_instance, :instantiate_message

    transaction do
+      step :create_excerpt
      step :save_message
      step :delete_drafts
      step :post_process_thread
@ -222,6 +223,10 @@ module Chat
      end
    end

+    def create_excerpt(message_instance:)
+      message_instance.excerpt = message_instance.build_excerpt
+    end
+
    def publish_user_tracking_state(message_instance:, channel:, channel_membership:, guardian:)
      message_to_publish = message_instance
      message_to_publish =
--- a/plugins/chat/app/services/chat/update_message.rb
+++ b/plugins/chat/app/services/chat/update_message.rb
@ -24,6 +24,7 @@ module Chat

    transaction do
      step :modify_message
+      step :update_excerpt
      step :save_message
      step :save_revision
      step :publish
@ -95,6 +96,10 @@ module Chat
      message.upload_ids = new_upload_ids
    end

+    def update_excerpt(message:)
+      message.excerpt = message.build_excerpt
+    end
+
    def save_message(message:)
      message.save!
    end
--- a/plugins/chat/db/migrate/20240422042830_add_excerpt_to_chat_messages.rb
+++ b/plugins/chat/db/migrate/20240422042830_add_excerpt_to_chat_messages.rb
@ -0,0 +1,7 @@
+# frozen_string_literal: true
+
+class AddExcerptToChatMessages < ActiveRecord::Migration[7.0]
+  def change
+    add_column :chat_messages, :excerpt, :string, limit: 1000, null: true
+  end
+end
--- a/plugins/chat/spec/integration/outgoing_web_hooks_spec.rb
+++ b/plugins/chat/spec/integration/outgoing_web_hooks_spec.rb
@ -102,20 +102,12 @@ RSpec.describe "Outgoing chat webhooks" do
    context "for a category channel" do
      fab!(:category)
      fab!(:chat_channel) { Fabricate(:category_channel, chatable: category) }
-      fab!(:chat_message) { Fabricate(:chat_message, chat_channel: chat_channel, user: user1) }
-
-      before do
-        [user1, user2].each do |user|
-          Chat::UserChatChannelMembership.create(
-            user: user,
-            chat_channel: chat_channel,
-            following: true,
-          )
-        end
-
-        sign_in(user1)
+      fab!(:chat_message) do
+        Fabricate(:chat_message, use_service: true, chat_channel: chat_channel, user: user1)
      end

+      before { sign_in(user1) }
+
      it "triggers a webhook when a chat message is created" do
        post "/chat/#{chat_channel.id}.json", params: { message: message_content }

@ -175,7 +167,12 @@ RSpec.describe "Outgoing chat webhooks" do
      fab!(:direct_message) { Fabricate(:direct_message, users: [user1, user2]) }
      fab!(:direct_message_channel) { Fabricate(:direct_message_channel, chatable: direct_message) }
      fab!(:chat_message) do
-        Fabricate(:chat_message, chat_channel: direct_message_channel, user: user1)
+        Fabricate(
+          :chat_message,
+          use_service: true,
+          chat_channel: direct_message_channel,
+          user: user1,
+        )
      end

      before { sign_in(user1) }
--- a/plugins/chat/spec/models/chat/message_spec.rb
+++ b/plugins/chat/spec/models/chat/message_spec.rb
@ -277,7 +277,9 @@ describe Chat::Message do
          :chat_message,
          message: "https://twitter.com/EffinBirds/status/1518743508378697729",
        )
-      expect(message.excerpt).to eq("https://twitter.com/EffinBirds/status/1518743508378697729")
+      expect(message.build_excerpt).to eq(
+        "https://twitter.com/EffinBirds/status/1518743508378697729",
+      )
      message =
        Fabricate.build(
          :chat_message,
@ -286,7 +288,9 @@ describe Chat::Message do
          <aside class=\"onebox twitterstatus\" data-onebox-src=\"https://twitter.com/EffinBirds/status/1518743508378697729\">\n  <header class=\"source\">\n\n      <a href=\"https://twitter.com/EffinBirds/status/1518743508378697729\" target=\"_blank\" rel=\"nofollow ugc noopener\">twitter.com</a>\n  </header>\n\n  <article class=\"onebox-body\">\n    \n<h4><a href=\"https://twitter.com/EffinBirds/status/1518743508378697729\" target=\"_blank\" rel=\"nofollow ugc noopener\">Effin' Birds</a></h4>\n<div class=\"twitter-screen-name\"><a href=\"https://twitter.com/EffinBirds/status/1518743508378697729\" target=\"_blank\" rel=\"nofollow ugc noopener\">@EffinBirds</a></div>\n\n<div class=\"tweet\">\n  <span class=\"tweet-description\">https://t.co/LjlqMm9lck</span>\n</div>\n\n<div class=\"date\">\n  <a href=\"https://twitter.com/EffinBirds/status/1518743508378697729\" class=\"timestamp\" target=\"_blank\" rel=\"nofollow ugc noopener\">5:07 PM - 25 Apr 2022</a>\n\n    <span class=\"like\">\n      <svg viewbox=\"0 0 512 512\" width=\"14px\" height=\"16px\" aria-hidden=\"true\">\n        <path d=\"M462.3 62.6C407.5 15.9 326 24.3 275.7 76.2L256 96.5l-19.7-20.3C186.1 24.3 104.5 15.9 49.7 62.6c-62.8 53.6-66.1 149.8-9.9 207.9l193.5 199.8c12.5 12.9 32.8 12.9 45.3 0l193.5-199.8c56.3-58.1 53-154.3-9.8-207.9z\"></path>\n      </svg>\n      2.5K\n    </span>\n\n    <span class=\"retweet\">\n      <svg viewbox=\"0 0 640 512\" width=\"14px\" height=\"16px\" aria-hidden=\"true\">\n        <path d=\"M629.657 343.598L528.971 444.284c-9.373 9.372-24.568 9.372-33.941 0L394.343 343.598c-9.373-9.373-9.373-24.569 0-33.941l10.823-10.823c9.562-9.562 25.133-9.34 34.419.492L480 342.118V160H292.451a24.005 24.005 0 0 1-16.971-7.029l-16-16C244.361 121.851 255.069 96 276.451 96H520c13.255 0 24 10.745 24 24v222.118l40.416-42.792c9.285-9.831 24.856-10.054 34.419-.492l10.823 10.823c9.372 9.372 9.372 24.569-.001 33.941zm-265.138 15.431A23.999 23.999 0 0 0 347.548 352H160V169.881l40.416 42.792c9.286 9.831 24.856 10.054 34.419.491l10.822-10.822c9.373-9.373 9.373-24.569 0-33.941L144.971 67.716c-9.373-9.373-24.569-9.373-33.941 0L10.343 168.402c-9.373 9.373-9.373 24.569 0 33.941l10.822 10.822c9.562 9.562 25.133 9.34 34.419-.491L96 169.881V392c0 13.255 10.745 24 24 24h243.549c21.382 0 32.09-25.851 16.971-40.971l-16.001-16z\"></path>\n      </svg>\n      499\n    </span>\n</div>\n\n  </article>\n\n  <div class=\"onebox-metadata\">\n    \n    \n  </div>\n\n  <div style=\"clear: both\"></div>\n</aside>\n
        COOKED
        )
-      expect(message.excerpt).to eq("https://twitter.com/EffinBirds/status/1518743508378697729")
+      expect(message.build_excerpt).to eq(
+        "https://twitter.com/EffinBirds/status/1518743508378697729",
+      )
    end

    it "excerpts upload file name if message is empty" do
@ -294,7 +298,7 @@ describe Chat::Message do
        Fabricate(:upload, original_filename: "cat.gif", width: 400, height: 300, extension: "gif")
      message = Fabricate(:chat_message, message: "", uploads: [gif])

-      expect(message.excerpt).to eq "cat.gif"
+      expect(message.build_excerpt).to eq "cat.gif"
    end

    it "supports autolink with <>" do
--- a/plugins/chat/spec/plugin_helper.rb
+++ b/plugins/chat/spec/plugin_helper.rb
@ -55,9 +55,7 @@ module ChatSystemHelpers
  end

  def thread_excerpt(message)
-    CGI.escapeHTML(
-      message.censored_excerpt(max_length: ::Chat::Thread::EXCERPT_LENGTH).gsub("&hellip;", "…"),
-    )
+    message.excerpt
  end
 end

--- a/plugins/chat/spec/serializer/chat/chat_message_serializer_spec.rb
+++ b/plugins/chat/spec/serializer/chat/chat_message_serializer_spec.rb
@ -44,7 +44,7 @@ describe Chat::MessageSerializer do
  describe "#excerpt" do
    it "censors words" do
      watched_word = Fabricate(:watched_word, action: WatchedWord.actions[:censor])
-      message = Fabricate(:chat_message, message: "ok #{watched_word.word}")
+      message = Fabricate(:chat_message, use_service: true, message: "ok #{watched_word.word}")
      serializer = described_class.new(message, scope: guardian, root: nil)

      expect(serializer.as_json[:excerpt]).to eq("ok ■■■■■")
--- a/plugins/chat/spec/serializer/chat/in_reply_to_serializer_spec.rb
+++ b/plugins/chat/spec/serializer/chat/in_reply_to_serializer_spec.rb
@ -23,7 +23,9 @@ RSpec.describe Chat::InReplyToSerializer do

  describe "#excerpt" do
    let(:watched_word) { Fabricate(:watched_word, action: WatchedWord.actions[:censor]) }
-    let(:message) { Fabricate(:chat_message, message: "ok #{watched_word.word}") }
+    let(:message) do
+      Fabricate(:chat_message, use_service: true, message: "ok #{watched_word.word}")
+    end

    it "censors words" do
      expect(serializer.as_json[:excerpt]).to eq("ok ■■■■■")
--- a/plugins/chat/spec/services/chat/create_message_spec.rb
+++ b/plugins/chat/spec/services/chat/create_message_spec.rb
@ -57,6 +57,10 @@ RSpec.describe Chat::CreateMessage do
        expect(message).to be_cooked
      end

+      it "creates the excerpt" do
+        expect(message).to have_attributes(excerpt: content)
+      end
+
      it "creates mentions" do
        Jobs.run_immediately!
        expect { result }.to change { Chat::Mention.count }.by(1)
--- a/plugins/chat/spec/services/chat/update_message_spec.rb
+++ b/plugins/chat/spec/services/chat/update_message_spec.rb
@ -145,6 +145,17 @@ RSpec.describe Chat::UpdateMessage do
      expect(chat_message.reload.cooked).to eq("<p>Change <strong>to</strong> this!</p>")
    end

+    it "updates the excerpt" do
+      chat_message = create_chat_message(user1, "This is a message", public_chat_channel)
+
+      described_class.call(
+        guardian: guardian,
+        message_id: chat_message.id,
+        message: "Change to this!",
+      )
+      expect(chat_message.reload.excerpt).to eq("Change to this!")
+    end
+
    it "publishes a DiscourseEvent for updated messages" do
      chat_message = create_chat_message(user1, "This will be changed", public_chat_channel)
      events =
@ -740,6 +751,9 @@ RSpec.describe Chat::UpdateMessage do

    describe "watched words" do
      fab!(:watched_word)
+      let!(:censored_word) do
+        Fabricate(:watched_word, word: "test", action: WatchedWord.actions[:censor])
+      end

      it "errors when a blocked word is present" do
        chat_message = create_chat_message(user1, "something", public_chat_channel)
@ -755,6 +769,18 @@ RSpec.describe Chat::UpdateMessage do

        expect(chat_message.reload.message).not_to eq("bad word - #{watched_word.word}")
      end
+
+      it "hides censored word within the excerpt" do
+        chat_message = create_chat_message(user1, "something", public_chat_channel)
+
+        described_class.call(
+          guardian: guardian,
+          message_id: chat_message.id,
+          message: "bad word - #{censored_word.word}",
+        )
+
+        expect(chat_message.reload.excerpt).to eq("bad word - ■■■■")
+      end
    end

    describe "channel statuses" do
--- a/plugins/chat/spec/system/chat/composer/channel_spec.rb
+++ b/plugins/chat/spec/system/chat/composer/channel_spec.rb
@ -18,7 +18,12 @@ RSpec.describe "Chat | composer | channel", type: :system do
  describe "reply to message" do
    context "when raw contains html" do
      fab!(:message_1) do
-        Fabricate(:chat_message, chat_channel: channel_1, message: "<mark>not marked</mark>")
+        Fabricate(
+          :chat_message,
+          use_service: true,
+          chat_channel: channel_1,
+          message: "<mark>not marked</mark>",
+        )
      end

      it "renders text in the details" do
--- a/plugins/chat/spec/system/chat_channel_spec.rb
+++ b/plugins/chat/spec/system/chat_channel_spec.rb
@ -3,7 +3,7 @@
 RSpec.describe "Chat channel", type: :system do
  fab!(:current_user) { Fabricate(:user) }
  fab!(:channel_1) { Fabricate(:chat_channel) }
-  fab!(:message_1) { Fabricate(:chat_message, chat_channel: channel_1) }
+  fab!(:message_1) { Fabricate(:chat_message, use_service: true, chat_channel: channel_1) }

  let(:chat_page) { PageObjects::Pages::Chat.new }
  let(:channel_page) { PageObjects::Pages::ChatChannel.new }
@ -278,6 +278,7 @@ RSpec.describe "Chat channel", type: :system do
        :chat_message,
        user: other_user,
        chat_channel: channel_1,
+        use_service: true,
        message: "<mark>not marked</mark>",
      )
    end
--- a/plugins/chat/spec/system/chat_composer_draft_spec.rb
+++ b/plugins/chat/spec/system/chat_composer_draft_spec.rb
@ -6,6 +6,7 @@ RSpec.describe "Chat composer draft", type: :system do
  fab!(:message_1) do
    Fabricate(
      :chat_message,
+      use_service: true,
      chat_channel: channel_1,
      message: "This is a message for draft and replies",
    )