From 062e4fb4f32a3de4987223532ceb7bdbe95bbe96 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?R=C3=A9gis=20Hanol?= <regis@hanol.fr>
Date: Mon, 16 Dec 2024 18:36:29 +0100
Subject: [PATCH] FIX: add server-side limit for DM channels (#30300)

When fetching DM channels, we would fetch all the user's DM channels,
and limit the number of DM channels we display on the client-side using
the `DIRECT_MESSAGE_CHANNELS_LIMIT`.

This is obviously not scalable as users are having more and more
discussions with other users.

This commit ensures we always limit the number of DM channels we send to the
client.

It adds the `MAX_DM_CHANNEL_RESULTS` in `ChannelFetcher` and use it to
limit the records returned by the server. This value should always be
lower than `DIRECT_MESSAGE_CHANNELS_LIMIT` on the client-side, so that
when the user has more "unread DMs" they still show up on the client as
they read them.

While adding a spec, I also updated the spec that ensures we have a
limit on public channels to use `stub_const` to temporarily lower the number
of fixtures we create during the test.
---
 plugins/chat/lib/chat/channel_fetcher.rb      | 16 +++++++---
 .../spec/lib/chat/channel_fetcher_spec.rb     | 32 ++++++++++++++++---
 2 files changed, 38 insertions(+), 10 deletions(-)

diff --git a/plugins/chat/lib/chat/channel_fetcher.rb b/plugins/chat/lib/chat/channel_fetcher.rb
index 3cf836e028b..5bab77b5d07 100644
--- a/plugins/chat/lib/chat/channel_fetcher.rb
+++ b/plugins/chat/lib/chat/channel_fetcher.rb
@@ -4,6 +4,9 @@ module Chat
   class ChannelFetcher
     MAX_PUBLIC_CHANNEL_RESULTS = 50
 
+    # NOTE: this should always be > than `DIRECT_MESSAGE_CHANNELS_LIMIT` in `chat-channel-manager.js`
+    MAX_DM_CHANNEL_RESULTS = 75
+
     def self.structured(guardian, include_threads: false)
       memberships = Chat::ChannelMembershipManager.all_for_user(guardian.user)
       public_channels = secured_public_channels(guardian, status: :open, following: true)
@@ -232,16 +235,19 @@ module Chat
       end
 
       query =
-        query.order("last_message.created_at DESC NULLS LAST").group(
-          "chat_channels.id",
-          "last_message.id",
-        )
+        query
+          .limit(MAX_DM_CHANNEL_RESULTS)
+          .order("last_message.created_at DESC NULLS LAST")
+          .group("chat_channels.id", "last_message.id")
 
       channels = query.to_a
+
       preload_fields =
         User.allowed_user_custom_fields(guardian) +
           UserField.all.pluck(:id).map { |fid| "#{User::USER_FIELD_PREFIX}#{fid}" }
-      User.preload_custom_fields(channels.map { |c| c.chatable.users }.flatten, preload_fields)
+
+      User.preload_custom_fields(channels.flat_map { _1.chatable.users }, preload_fields)
+
       channels
     end
 
diff --git a/plugins/chat/spec/lib/chat/channel_fetcher_spec.rb b/plugins/chat/spec/lib/chat/channel_fetcher_spec.rb
index c509305a88f..f6805d11ee0 100644
--- a/plugins/chat/spec/lib/chat/channel_fetcher_spec.rb
+++ b/plugins/chat/spec/lib/chat/channel_fetcher_spec.rb
@@ -279,12 +279,14 @@ describe Chat::ChannelFetcher do
     end
 
     it "ensures limit has a max value" do
-      over_limit = Chat::ChannelFetcher::MAX_PUBLIC_CHANNEL_RESULTS + 1
-      over_limit.times { Fabricate(:category_channel) }
+      stub_const(Chat::ChannelFetcher, "MAX_PUBLIC_CHANNEL_RESULTS", 5) do
+        limit = Chat::ChannelFetcher::MAX_PUBLIC_CHANNEL_RESULTS + 1
+        limit.times { Fabricate(:category_channel) }
 
-      expect(described_class.secured_public_channels(guardian, limit: over_limit).length).to eq(
-        Chat::ChannelFetcher::MAX_PUBLIC_CHANNEL_RESULTS,
-      )
+        expect(described_class.secured_public_channels(guardian, limit:).size).to eq(
+          Chat::ChannelFetcher::MAX_PUBLIC_CHANNEL_RESULTS,
+        )
+      end
     end
 
     it "does not show the user category channels they cannot access" do
@@ -412,6 +414,26 @@ describe Chat::ChannelFetcher do
       result = described_class.tracking_state([direct_message_channel1.id], guardian)
       expect(result.channel_tracking[target_membership.chat_channel_id][:unread_count]).to eq(0)
     end
+
+    it "limits the number of results returned" do
+      stub_const(Chat::ChannelFetcher, "MAX_DM_CHANNEL_RESULTS", 5) do
+        (Chat::ChannelFetcher::MAX_DM_CHANNEL_RESULTS + 1).times do
+          chat_channel = Fabricate(:direct_message_channel)
+          Fabricate(
+            :user_chat_channel_membership_for_dm,
+            chat_channel:,
+            user: user1,
+            following: true,
+          )
+          Chat::DirectMessageUser.create!(direct_message: chat_channel.chatable, user: user1)
+          Fabricate(:chat_message, chat_channel:, user: user2)
+        end
+
+        expect(described_class.secured_direct_message_channels(user1.id, guardian).size).to eq(
+          Chat::ChannelFetcher::MAX_DM_CHANNEL_RESULTS,
+        )
+      end
+    end
   end
 
   describe ".unreads_total" do