From e34fb7e0b28a99efd8578af53f2accfce12e6e5c Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Tue, 11 Apr 2023 15:40:25 +1000 Subject: [PATCH] DEV: Chat thread reply counter cache (#21050) Similar to 22a55ef0ce42823b860f2d3432e4d7f14a00752f, this commit adds a replies_count to the Chat::Thread table, which is updated every 15 minutes via PeriodicalUpdates. This is done so the new thread indicator for the UI can show the count without intense serializer queries, but in future we likely want this to update more frequently. --- .../jobs/scheduled/chat/periodical_updates.rb | 1 + plugins/chat/app/models/chat/channel.rb | 43 +++++++++--------- plugins/chat/app/models/chat/message.rb | 1 + plugins/chat/app/models/chat/thread.rb | 32 +++++++++++++ ..._thread_not_deleted_index_chat_messages.rb | 24 ++++++++++ ...t_message_replies_count_to_chat_threads.rb | 8 ++++ ...230411023340_update_thread_reply_counts.rb | 22 +++++++++ .../chat/spec/fabricators/chat_fabricator.rb | 2 + plugins/chat/spec/models/chat/thread_spec.rb | 45 +++++++++++++++++++ 9 files changed, 157 insertions(+), 21 deletions(-) create mode 100644 plugins/chat/db/migrate/20230411012630_add_thread_not_deleted_index_chat_messages.rb create mode 100644 plugins/chat/db/migrate/20230411023246_add_chat_message_replies_count_to_chat_threads.rb create mode 100644 plugins/chat/db/post_migrate/20230411023340_update_thread_reply_counts.rb create mode 100644 plugins/chat/spec/models/chat/thread_spec.rb diff --git a/plugins/chat/app/jobs/scheduled/chat/periodical_updates.rb b/plugins/chat/app/jobs/scheduled/chat/periodical_updates.rb index 7b6c1f3318e..e2faabe637c 100644 --- a/plugins/chat/app/jobs/scheduled/chat/periodical_updates.rb +++ b/plugins/chat/app/jobs/scheduled/chat/periodical_updates.rb @@ -9,6 +9,7 @@ module Jobs # TODO: Add rebaking of old messages (baked_version < # Chat::Message::BAKED_VERSION or baked_version IS NULL) ::Chat::Channel.ensure_consistency! + ::Chat::Thread.ensure_consistency! nil end end diff --git a/plugins/chat/app/models/chat/channel.rb b/plugins/chat/app/models/chat/channel.rb index c5c67d23d95..6d9067af40a 100644 --- a/plugins/chat/app/models/chat/channel.rb +++ b/plugins/chat/app/models/chat/channel.rb @@ -165,32 +165,33 @@ end # # Table name: chat_channels # -# id :bigint not null, primary key -# chatable_id :integer not null -# deleted_at :datetime -# deleted_by_id :integer -# featured_in_category_id :integer -# delete_after_seconds :integer -# chatable_type :string not null -# created_at :datetime not null -# updated_at :datetime not null -# name :string -# description :text -# status :integer default("open"), not null -# user_count :integer default(0), not null -# last_message_sent_at :datetime not null -# auto_join_users :boolean default(FALSE), not null -# allow_channel_wide_mentions :boolean default(TRUE), not null -# user_count_stale :boolean default(FALSE), not null -# slug :string -# type :string -# threading_enabled :boolean default(FALSE), not null +# id :bigint not null, primary key +# chatable_id :integer not null +# deleted_at :datetime +# deleted_by_id :integer +# featured_in_category_id :integer +# delete_after_seconds :integer +# chatable_type :string not null +# created_at :datetime not null +# updated_at :datetime not null +# name :string +# description :text +# status :integer default("open"), not null +# user_count :integer default(0), not null +# last_message_sent_at :datetime not null +# auto_join_users :boolean default(FALSE), not null +# user_count_stale :boolean default(FALSE), not null +# slug :string +# type :string +# allow_channel_wide_mentions :boolean default(TRUE), not null +# messages_count :integer default(0), not null +# threading_enabled :boolean default(FALSE), not null # # Indexes # -# index_chat_channels_on_messages_count (messages_count) # index_chat_channels_on_chatable_id (chatable_id) # index_chat_channels_on_chatable_id_and_chatable_type (chatable_id,chatable_type) +# index_chat_channels_on_messages_count (messages_count) # index_chat_channels_on_slug (slug) UNIQUE # index_chat_channels_on_status (status) # diff --git a/plugins/chat/app/models/chat/message.rb b/plugins/chat/app/models/chat/message.rb index 18339a65be7..2a1640570d5 100644 --- a/plugins/chat/app/models/chat/message.rb +++ b/plugins/chat/app/models/chat/message.rb @@ -348,6 +348,7 @@ end # Indexes # # idx_chat_messages_by_created_at_not_deleted (created_at) WHERE (deleted_at IS NULL) +# idx_chat_messages_by_thread_id_not_deleted (thread_id) WHERE (deleted_at IS NULL) # index_chat_messages_on_chat_channel_id_and_created_at (chat_channel_id,created_at) # index_chat_messages_on_chat_channel_id_and_id (chat_channel_id,id) WHERE (deleted_at IS NULL) # index_chat_messages_on_last_editor_id (last_editor_id) diff --git a/plugins/chat/app/models/chat/thread.rb b/plugins/chat/app/models/chat/thread.rb index 25fb68f45b4..f78fc05f02e 100644 --- a/plugins/chat/app/models/chat/thread.rb +++ b/plugins/chat/app/models/chat/thread.rb @@ -18,6 +18,10 @@ module Chat enum :status, { open: 0, read_only: 1, closed: 2, archived: 3 }, scopes: false + def replies + self.chat_messages.where.not(id: self.original_message_id) + end + def url "#{channel.url}/t/#{self.id}" end @@ -29,6 +33,32 @@ module Chat def excerpt original_message.excerpt(max_length: EXCERPT_LENGTH) end + + def self.ensure_consistency! + update_counts + end + + def self.update_counts + # NOTE: Chat::Thread#replies_count is not updated every time + # a message is created or deleted in a channel, the UI will lag + # behind unless it is kept in sync with MessageBus. The count + # has 1 subtracted from it to account for the original message. + # + # It is updated eventually via Jobs::Chat::PeriodicalUpdates. In + # future we may want to update this more frequently. + DB.exec <<~SQL + UPDATE chat_threads threads + SET replies_count = subquery.replies_count + FROM ( + SELECT COUNT(*) - 1 AS replies_count, thread_id + FROM chat_messages + WHERE chat_messages.deleted_at IS NULL AND thread_id IS NOT NULL + GROUP BY thread_id + ) subquery + WHERE threads.id = subquery.thread_id + AND subquery.replies_count != threads.replies_count + SQL + end end end @@ -44,6 +74,7 @@ end # title :string # created_at :datetime not null # updated_at :datetime not null +# replies_count :integer default(0), not null # # Indexes # @@ -51,5 +82,6 @@ end # index_chat_threads_on_channel_id_and_status (channel_id,status) # index_chat_threads_on_original_message_id (original_message_id) # index_chat_threads_on_original_message_user_id (original_message_user_id) +# index_chat_threads_on_replies_count (replies_count) # index_chat_threads_on_status (status) # diff --git a/plugins/chat/db/migrate/20230411012630_add_thread_not_deleted_index_chat_messages.rb b/plugins/chat/db/migrate/20230411012630_add_thread_not_deleted_index_chat_messages.rb new file mode 100644 index 00000000000..3596ea46a09 --- /dev/null +++ b/plugins/chat/db/migrate/20230411012630_add_thread_not_deleted_index_chat_messages.rb @@ -0,0 +1,24 @@ +# frozen_string_literal: true + +class AddThreadNotDeletedIndexChatMessages < ActiveRecord::Migration[7.0] + disable_ddl_transaction! + + def change + execute <<~SQL + DROP INDEX IF EXISTS idx_chat_messages_by_thread_id_not_deleted + SQL + + execute <<~SQL + CREATE INDEX CONCURRENTLY IF NOT EXISTS + idx_chat_messages_by_thread_id_not_deleted + ON chat_messages (thread_id) + WHERE deleted_at IS NULL + SQL + end + + def down + execute <<~SQL + DROP INDEX IF EXISTS idx_chat_messages_by_thread_id_not_deleted + SQL + end +end diff --git a/plugins/chat/db/migrate/20230411023246_add_chat_message_replies_count_to_chat_threads.rb b/plugins/chat/db/migrate/20230411023246_add_chat_message_replies_count_to_chat_threads.rb new file mode 100644 index 00000000000..6c2702d2876 --- /dev/null +++ b/plugins/chat/db/migrate/20230411023246_add_chat_message_replies_count_to_chat_threads.rb @@ -0,0 +1,8 @@ +# frozen_string_literal: true + +class AddChatMessageRepliesCountToChatThreads < ActiveRecord::Migration[7.0] + def change + add_column :chat_threads, :replies_count, :integer, null: false, default: 0 + add_index :chat_threads, :replies_count + end +end diff --git a/plugins/chat/db/post_migrate/20230411023340_update_thread_reply_counts.rb b/plugins/chat/db/post_migrate/20230411023340_update_thread_reply_counts.rb new file mode 100644 index 00000000000..b3dc2998e8c --- /dev/null +++ b/plugins/chat/db/post_migrate/20230411023340_update_thread_reply_counts.rb @@ -0,0 +1,22 @@ +# frozen_string_literal: true + +class UpdateThreadReplyCounts < ActiveRecord::Migration[7.0] + def up + DB.exec <<~SQL + UPDATE chat_threads threads + SET replies_count = subquery.replies_count + FROM ( + SELECT COUNT(*) - 1 AS replies_count, thread_id + FROM chat_messages + WHERE chat_messages.deleted_at IS NULL AND thread_id IS NOT NULL + GROUP BY thread_id + ) subquery + WHERE threads.id = subquery.thread_id + AND subquery.replies_count != threads.replies_count + SQL + end + + def down + raise ActiveRecord::IrreversibleMigration + end +end diff --git a/plugins/chat/spec/fabricators/chat_fabricator.rb b/plugins/chat/spec/fabricators/chat_fabricator.rb index e66fba77824..e2d89aaf0a2 100644 --- a/plugins/chat/spec/fabricators/chat_fabricator.rb +++ b/plugins/chat/spec/fabricators/chat_fabricator.rb @@ -147,4 +147,6 @@ Fabricator(:chat_thread, class_name: "Chat::Thread") do original_message do |attrs| Fabricate(:chat_message, chat_channel: attrs[:channel] || Fabricate(:chat_channel)) end + + after_create { |thread| thread.original_message.update!(thread_id: thread.id) } end diff --git a/plugins/chat/spec/models/chat/thread_spec.rb b/plugins/chat/spec/models/chat/thread_spec.rb new file mode 100644 index 00000000000..dbed433a1c3 --- /dev/null +++ b/plugins/chat/spec/models/chat/thread_spec.rb @@ -0,0 +1,45 @@ +# frozen_string_literal: true + +RSpec.describe Chat::Thread do + describe ".ensure_consistency!" do + fab!(:channel) { Fabricate(:category_channel) } + fab!(:thread_1) { Fabricate(:chat_thread, channel: channel) } + fab!(:thread_2) { Fabricate(:chat_thread, channel: channel) } + fab!(:thread_3) { Fabricate(:chat_thread, channel: channel) } + + before do + Fabricate(:chat_message, chat_channel: channel, thread: thread_1) + Fabricate(:chat_message, chat_channel: channel, thread: thread_1) + Fabricate(:chat_message, chat_channel: channel, thread: thread_1) + + Fabricate(:chat_message, chat_channel: channel, thread: thread_2) + Fabricate(:chat_message, chat_channel: channel, thread: thread_2) + Fabricate(:chat_message, chat_channel: channel, thread: thread_2) + Fabricate(:chat_message, chat_channel: channel, thread: thread_2) + + Fabricate(:chat_message, chat_channel: channel, thread: thread_3) + end + + describe "updating replies_count for all threads" do + it "counts correctly and does not include the original message" do + described_class.ensure_consistency! + expect(thread_1.reload.replies_count).to eq(3) + expect(thread_2.reload.replies_count).to eq(4) + expect(thread_3.reload.replies_count).to eq(1) + end + + it "does not count deleted messages" do + thread_1.chat_messages.last.trash! + described_class.ensure_consistency! + expect(thread_1.reload.replies_count).to eq(2) + end + + it "sets the replies count to 0 if all the messages but the original message are deleted" do + thread_1.replies.delete_all + + described_class.ensure_consistency! + expect(thread_1.reload.replies_count).to eq(0) + end + end + end +end