From c62d3610c6915a58a1dc683605d5819cb5577baf Mon Sep 17 00:00:00 2001 From: David Battersby Date: Thu, 25 Apr 2024 20:29:00 +0800 Subject: [PATCH] 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. --- plugins/chat/app/models/chat/message.rb | 9 +++---- plugins/chat/app/models/chat/thread.rb | 3 +-- .../chat/in_reply_to_serializer.rb | 4 --- .../serializers/chat/message_serializer.rb | 2 +- .../thread_original_message_serializer.rb | 4 --- .../chat/thread_preview_serializer.rb | 2 +- .../chat/app/services/chat/create_message.rb | 5 ++++ .../chat/app/services/chat/update_message.rb | 5 ++++ ...0422042830_add_excerpt_to_chat_messages.rb | 7 +++++ .../integration/outgoing_web_hooks_spec.rb | 23 +++++++--------- plugins/chat/spec/models/chat/message_spec.rb | 10 ++++--- plugins/chat/spec/plugin_helper.rb | 4 +-- .../chat/chat_message_serializer_spec.rb | 2 +- .../chat/in_reply_to_serializer_spec.rb | 4 ++- .../spec/services/chat/create_message_spec.rb | 4 +++ .../spec/services/chat/update_message_spec.rb | 26 +++++++++++++++++++ .../spec/system/chat/composer/channel_spec.rb | 7 ++++- plugins/chat/spec/system/chat_channel_spec.rb | 3 ++- .../spec/system/chat_composer_draft_spec.rb | 1 + 19 files changed, 84 insertions(+), 41 deletions(-) create mode 100644 plugins/chat/db/migrate/20240422042830_add_excerpt_to_chat_messages.rb diff --git a/plugins/chat/app/models/chat/message.rb b/plugins/chat/app/models/chat/message.rb index a236ffa2f7d..2d0c49e688f 100644 --- 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 diff --git a/plugins/chat/app/models/chat/thread.rb b/plugins/chat/app/models/chat/thread.rb index 8de438c01e4..e9482796a28 100644 --- 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! diff --git a/plugins/chat/app/serializers/chat/in_reply_to_serializer.rb b/plugins/chat/app/serializers/chat/in_reply_to_serializer.rb index 5d69815628f..06a85d65aa6 100644 --- 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 diff --git a/plugins/chat/app/serializers/chat/message_serializer.rb b/plugins/chat/app/serializers/chat/message_serializer.rb index 5c43a6062f9..11894de7ca5 100644 --- 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 diff --git a/plugins/chat/app/serializers/chat/thread_original_message_serializer.rb b/plugins/chat/app/serializers/chat/thread_original_message_serializer.rb index e686f1059b1..9fda1a0946f 100644 --- 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 diff --git a/plugins/chat/app/serializers/chat/thread_preview_serializer.rb b/plugins/chat/app/serializers/chat/thread_preview_serializer.rb index e7fb373d11c..65ba953c4e3 100644 --- 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 diff --git a/plugins/chat/app/services/chat/create_message.rb b/plugins/chat/app/services/chat/create_message.rb index fa06798c2a9..a943ed6a43c 100644 --- 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 = diff --git a/plugins/chat/app/services/chat/update_message.rb b/plugins/chat/app/services/chat/update_message.rb index dae7d27227b..2ea3c6057ea 100644 --- 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 diff --git a/plugins/chat/db/migrate/20240422042830_add_excerpt_to_chat_messages.rb b/plugins/chat/db/migrate/20240422042830_add_excerpt_to_chat_messages.rb new file mode 100644 index 00000000000..e5210d4c27b --- /dev/null +++ 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 diff --git a/plugins/chat/spec/integration/outgoing_web_hooks_spec.rb b/plugins/chat/spec/integration/outgoing_web_hooks_spec.rb index 8426b7f5bb9..67c4ed0ca49 100644 --- 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) } diff --git a/plugins/chat/spec/models/chat/message_spec.rb b/plugins/chat/spec/models/chat/message_spec.rb index 9cf725caab9..cb294315a36 100644 --- 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 \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 diff --git a/plugins/chat/spec/plugin_helper.rb b/plugins/chat/spec/plugin_helper.rb index 8b6451f04b6..2b4fec15ba2 100644 --- 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("…", "…"), - ) + message.excerpt end end diff --git a/plugins/chat/spec/serializer/chat/chat_message_serializer_spec.rb b/plugins/chat/spec/serializer/chat/chat_message_serializer_spec.rb index 74e1c84634f..d8e516f7478 100644 --- 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 ■■■■■") diff --git a/plugins/chat/spec/serializer/chat/in_reply_to_serializer_spec.rb b/plugins/chat/spec/serializer/chat/in_reply_to_serializer_spec.rb index a2bb283d7cb..6c2e8c0409a 100644 --- 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 ■■■■■") diff --git a/plugins/chat/spec/services/chat/create_message_spec.rb b/plugins/chat/spec/services/chat/create_message_spec.rb index 54e9bd7f47e..79b23b52214 100644 --- 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) diff --git a/plugins/chat/spec/services/chat/update_message_spec.rb b/plugins/chat/spec/services/chat/update_message_spec.rb index d46bc20aa9c..646f7716e84 100644 --- 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("

Change to this!

") 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 diff --git a/plugins/chat/spec/system/chat/composer/channel_spec.rb b/plugins/chat/spec/system/chat/composer/channel_spec.rb index 2c18551ec79..fb30596c980 100644 --- 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: "not marked") + Fabricate( + :chat_message, + use_service: true, + chat_channel: channel_1, + message: "not marked", + ) end it "renders text in the details" do diff --git a/plugins/chat/spec/system/chat_channel_spec.rb b/plugins/chat/spec/system/chat_channel_spec.rb index 48960115335..1c97ad6b4ef 100644 --- 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: "not marked", ) end diff --git a/plugins/chat/spec/system/chat_composer_draft_spec.rb b/plugins/chat/spec/system/chat_composer_draft_spec.rb index c64aa3dcce0..bd06e069bd5 100644 --- 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", )