From bf58ac0b9ec3df33d1c38fd60b2421a7ca78f334 Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Wed, 8 Jul 2026 11:12:45 +0000 Subject: [PATCH] :bug: Eliminate N+1 query storm in unread comment threads The dashboard's unread-comment-threads flow fired one get-profiles-for-file-comments RPC call per file-id, producing 20+ concurrent HTTP requests and 20+ SQL queries for users with unread threads across multiple files. Widen the existing get-profiles-for-file-comments RPC to accept a set of file ids (max 100) instead of a single id. The SQL uses ANY(?::uuid[]) so a 1-element set has the same query plan as the previous file_id = ?. Update the frontend unread-threads flow to send a single batch request, and the workspace fetch-profiles event to wrap the current file id in a 1-element set. The 100 cap is enforced in both the frontend (with a log/warn on overflow) and the schema (:max 100). Fixes #10587 AI-assisted-by: opencode-go/minimax-m3 --- backend/src/app/rpc/commands/comments.clj | 53 ++++++++++-------- .../test/backend_tests/rpc_comment_test.clj | 55 ++++++++++++++++++- frontend/src/app/main/data/comments.cljs | 32 ++++++++--- frontend/src/app/main/data/profile.cljs | 14 ----- 4 files changed, 108 insertions(+), 46 deletions(-) diff --git a/backend/src/app/rpc/commands/comments.clj b/backend/src/app/rpc/commands/comments.clj index 3383d3d343..74c914d974 100644 --- a/backend/src/app/rpc/commands/comments.clj +++ b/backend/src/app/rpc/commands/comments.clj @@ -371,44 +371,51 @@ ;; --- COMMAND: Get file comments users -;; All the profiles that had comment the file, plus the current -;; profile. +;; All the profiles that had comment any of the given files, plus the +;; current profile. The :file-id param is a set (max 100) of file ids +;; so the same method serves both single-file and dashboard batch +;; callers. (def ^:private sql:file-comment-users "WITH available_profiles AS ( - SELECT DISTINCT owner_id AS id - FROM comment - WHERE thread_id IN (SELECT id FROM comment_thread WHERE file_id=?) - ) - SELECT p.id, - p.email, - p.fullname AS name, - p.fullname AS fullname, - p.photo_id, - p.is_active - FROM profile AS p - WHERE p.id IN (SELECT id FROM available_profiles) OR p.id=?") + SELECT DISTINCT c.owner_id AS id + FROM comment AS c + INNER JOIN comment_thread AS ct ON (ct.id = c.thread_id) + WHERE ct.file_id = ANY(?::uuid[]) + ) + SELECT p.id, + p.email, + p.fullname AS name, + p.fullname AS fullname, + p.photo_id, + p.is_active + FROM profile AS p + WHERE p.id IN (SELECT id FROM available_profiles) OR p.id=?") -(defn get-file-comments-users - [conn file-id profile-id] - (db/exec! conn [sql:file-comment-users file-id profile-id])) +(defn- get-file-comments-users + [conn file-ids profile-id] + (let [file-ids (db/create-array conn "uuid" file-ids)] + (db/exec! conn [sql:file-comment-users file-ids profile-id]))) (def ^:private schema:get-profiles-for-file-comments [:map {:title "get-profiles-for-file-comments"} - [:file-id ::sm/uuid] + [:file-id [::sm/set {:max 100} ::sm/uuid]] [:share-id {:optional true} [:maybe ::sm/uuid]]]) (sv/defmethod ::get-profiles-for-file-comments "Retrieves a list of profiles with limited set of properties of all - participants on comment threads of the file." + participants on comment threads of the given file(s)." {::doc/added "1.15" - ::doc/changes ["1.15" "Imported from queries and renamed."] + ::doc/changes ["1.15.0" "Imported from queries and renamed." + "2.17.1" "Schema widened: :file-id now accepts a set (max 100) of file ids."] ::sm/params schema:get-profiles-for-file-comments} [cfg {:keys [::rpc/profile-id file-id share-id]}] - (db/run! cfg (fn [{:keys [::db/conn]}] - (files/check-comment-permissions! conn profile-id file-id share-id) - (get-file-comments-users conn file-id profile-id)))) + (db/run! cfg + (fn [{:keys [::db/conn] :as cfg}] + (doseq [fid file-id] + (files/check-comment-permissions! cfg profile-id fid share-id)) + (get-file-comments-users conn file-id profile-id)))) ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; ;; MUTATION COMMANDS diff --git a/backend/test/backend_tests/rpc_comment_test.clj b/backend/test/backend_tests/rpc_comment_test.clj index 8724cbfdfa..7eaa3c4ba5 100644 --- a/backend/test/backend_tests/rpc_comment_test.clj +++ b/backend/test/backend_tests/rpc_comment_test.clj @@ -216,7 +216,7 @@ (t/testing "get profiles" (let [data {::th/type :get-profiles-for-file-comments ::rpc/profile-id (:id profile-1) - :file-id (:id file-1)} + :file-id #{(:id file-1)}} out (th/command! data)] ;; (th/print-result! out) (t/is (th/success? out)) @@ -227,12 +227,63 @@ (t/testing "get profiles 2" (let [data {::th/type :get-profiles-for-file-comments ::rpc/profile-id (:id profile-2) - :file-id (:id file-1)} + :file-id #{(:id file-1)}} out (th/command! data)] ;; (th/print-result! out) (t/is (not (th/success? out))) (t/is (= :not-found (th/ex-type (:error out)))))) + (t/testing "get profiles batch" + (let [data {::th/type :get-profiles-for-file-comments + ::rpc/profile-id (:id profile-1) + :file-id #{(:id file-1) (:id file-2)}} + out (th/command! data)] + ;; (th/print-result! out) + (t/is (th/success? out)) + (let [profiles (:result out)] + (t/is (= 1 (count profiles))) + (t/is (= (:id profile-1) (-> profiles first :id)))))) + + (t/testing "get profiles batch - permission denied" + (let [data {::th/type :get-profiles-for-file-comments + ::rpc/profile-id (:id profile-2) + :file-id #{(:id file-1) (:id file-2)}} + out (th/command! data)] + ;; (th/print-result! out) + (t/is (not (th/success? out))) + (t/is (= :not-found (th/ex-type (:error out)))))) + + (t/testing "get profiles batch - across multiple files" + ;; Create a second comment thread on file-2 as profile-1, then + ;; verify the batch query unions commenters across both files. + (let [page-id-2 (get-in file-2 [:data :pages 0]) + _ (th/command! {::th/type :create-comment-thread + ::rpc/profile-id (:id profile-1) + :file-id (:id file-2) + :page-id page-id-2 + :position (gpt/point 0) + :content "second file" + :frame-id uuid/zero}) + data {::th/type :get-profiles-for-file-comments + ::rpc/profile-id (:id profile-1) + :file-id #{(:id file-1) (:id file-2)}} + out (th/command! data)] + ;; (th/print-result! out) + (t/is (th/success? out)) + (let [profiles (:result out)] + (t/is (= 1 (count profiles))) + (t/is (= (:id profile-1) (-> profiles first :id)))))) + + (t/testing "get profiles batch - rejects more than 100 file-ids" + (let [ids (set (repeatedly 101 #(random-uuid))) + data {::th/type :get-profiles-for-file-comments + ::rpc/profile-id (:id profile-1) + :file-id ids} + out (th/command! data)] + ;; (th/print-result! out) + (t/is (not (th/success? out))) + (t/is (th/ex-of-code? (:error out) :params-validation)))) + (t/testing "delete comment" (let [thread (-> (th/db-query :comment-thread {:file-id (:id file-1)}) first) comment (-> (th/db-query :comment {:thread-id (:id thread) :content "comment 2 mod"}) first) diff --git a/frontend/src/app/main/data/comments.cljs b/frontend/src/app/main/data/comments.cljs index c0c0dc7409..221974f410 100644 --- a/frontend/src/app/main/data/comments.cljs +++ b/frontend/src/app/main/data/comments.cljs @@ -9,6 +9,7 @@ [app.common.data :as d] [app.common.data.macros :as dm] [app.common.geom.point :as gpt] + [app.common.logging :as log] [app.common.schema :as sm] [app.common.time :as ct] [app.common.types.shape-tree :as ctst] @@ -22,6 +23,12 @@ [beicon.v2.core :as rx] [potok.v2.core :as ptk])) +(def ^:private max-batch-file-ids + "Maximum number of file-ids sent per batch request. Must match + the `:max 100` constraint on the backend schema + `get-profiles-for-file-comments`." + 100) + (def ^:private schema:comment-thread [:map {:title "CommentThread"} [:id ::sm/uuid] @@ -454,13 +461,24 @@ (->> (rp/cmd! :get-unread-comment-threads {:team-id team-id}) (rx/merge-map (fn [comments] - (rx/concat - (rx/of (partial fetched-comments comments)) + (let [ids (vec (into #{} (map :file-id) comments)) + n (count ids) + ids (if (> n max-batch-file-ids) + (do + (log/warn :msg "unread-threads: capping file-ids at max" + :max max-batch-file-ids + :count n + :team-id team-id) + (subvec ids 0 max-batch-file-ids)) + ids)] + (rx/concat + (rx/of (partial fetched-comments comments)) - (->> (rx/from (into #{} (map :file-id) comments)) - (rx/merge-map #(rp/cmd! :get-profiles-for-file-comments {:file-id %})) - (rx/reduce #(merge %1 (d/index-by :id %2)) {}) - (rx/map #(partial fetched-users %)))))) + (if (seq ids) + (->> (rp/cmd! :get-profiles-for-file-comments {:file-id ids}) + (rx/map #(d/index-by :id %)) + (rx/map #(partial fetched-users %))) + (rx/of (partial fetched-users {}))))))) (rx/catch #(rx/throw {:type :comment-error}))))))) (defn mark-all-threads-as-read @@ -682,7 +700,7 @@ (let [file-id (:current-file-id state) share-id (or (-> state :viewer-local :share-id) (:current-share-id state))] - (->> (rp/cmd! :get-profiles-for-file-comments {:file-id file-id :share-id share-id}) + (->> (rp/cmd! :get-profiles-for-file-comments {:file-id #{file-id} :share-id share-id}) (rx/map (fn [profiles] #(update % :profiles merge (d/index-by :id profiles))))))))) diff --git a/frontend/src/app/main/data/profile.cljs b/frontend/src/app/main/data/profile.cljs index e37dc08e45..0f41f9217f 100644 --- a/frontend/src/app/main/data/profile.cljs +++ b/frontend/src/app/main/data/profile.cljs @@ -371,20 +371,6 @@ (js/console.error "delete-photo failed" cause) (rx/of (refresh-profile)))))))) -(defn fetch-file-comments-users - [{:keys [team-id]}] - (assert (uuid? team-id) "expected a valid uuid for `team-id`") - (letfn [(fetched [users state] - (->> users - (d/index-by :id) - (assoc state :file-comments-users)))] - (ptk/reify ::fetch-file-comments-users - ptk/WatchEvent - (watch [_ state _] - (let [share-id (-> state :viewer-local :share-id)] - (->> (rp/cmd! :get-profiles-for-file-comments {:team-id team-id :share-id share-id}) - (rx/map #(partial fetched %)))))))) - ;; --- EVENT: request-account-deletion (def profile-deleted-event?