PERF: Only load the current user's topic_user for bookmarks list (#17873)

Previously, for every bookmarked topic, all topic_user records were being preloaded. Only the current user's record is actually required.

This commit introduces a new `perform_custom_preload!` API which bookmarkables can use to add custom preloading logic. We use this in topic_bookmarkable to load just the topic_user data we need (in the same way as `topic_list.rb`).

Co-authored-by: Blake Erickson <o.blakeerickson@gmail.com>

app/serializers/user_post_topic_bookmark_base_serializer.rb

@@ -15,7 +15,6 @@ class UserPostTopicBookmarkBaseSerializer < UserBookmarkBaseSerializer
              :archived,
              :archetype,
              :highest_post_number,
-             :last_read_post_number,
              :bumped_at,
              :slug
 
@@ -51,10 +50,6 @@ class UserPostTopicBookmarkBaseSerializer < UserBookmarkBaseSerializer
     scope.is_whisperer? ? topic.highest_staff_post_number : topic.highest_post_number
   end
 
-  def last_read_post_number
-    topic_user&.last_read_post_number
-  end
-
   def bumped_at
     topic.bumped_at
   end
@@ -62,10 +57,4 @@ class UserPostTopicBookmarkBaseSerializer < UserBookmarkBaseSerializer
   def slug
     topic.slug
   end
-
-  private
-
-  def topic_user
-    @topic_user ||= topic.topic_users.find { |tu| tu.user_id == scope.user.id }
-  end
 end

app/serializers/user_topic_bookmark_serializer.rb

@@ -4,6 +4,9 @@ class UserTopicBookmarkSerializer < UserPostTopicBookmarkBaseSerializer
   # it does not matter what the linked post number is for topic bookmarks,
   # on the client we always take the user to the last unread post in the
   # topic when the bookmark URL is clicked
+
+  attributes :last_read_post_number
+
   def linked_post_number
     1
   end
@@ -63,9 +66,17 @@ class UserTopicBookmarkSerializer < UserPostTopicBookmarkBaseSerializer
     end
   end
 
+  def last_read_post_number
+    topic_user&.last_read_post_number
+  end
+
   private
 
   def topic
     object.bookmarkable
   end
+
+  def topic_user
+    topic.user_data
+  end
 end

app/services/base_bookmarkable.rb

@@ -39,6 +39,15 @@ class BaseBookmarkable
     preload_associations.present?
   end
 
+  ##
+  #
+  # Implementations can define their own preloading logic here
+  # @param [Array] bookmarks_of_type The list of bookmarks to preload data for. Already filtered to be of the correct class.
+  # @param [Guardian] guardian An instance of Guardian for the current_user
+  def self.perform_custom_preload!(bookmarks_of_type, guardian)
+    nil
+  end
+
   ##
   # This is where the main query to filter the bookmarks by the provided bookmarkable
   # type should occur. This should join on additional tables that are required later

app/services/post_bookmarkable.rb

@@ -12,7 +12,7 @@ class PostBookmarkable < BaseBookmarkable
   end
 
   def self.preload_associations
-    [{ topic: [:topic_users, :tags] }, :user]
+    [{ topic: [:tags] }, :user]
   end
 
   def self.list_query(user, guardian)

app/services/registered_bookmarkable.rb

@@ -55,18 +55,25 @@ class RegisteredBookmarkable
   #
   # [{ topic: [:topic_users, :tags] }, :user]
   #
+  # For more advanced preloading, bookmarkable classes can implement `perform_custom_preload!`
+  #
   # @param [Array] bookmarks The array of bookmarks after initial listing and filtering, note this is
   #                          array _not_ an ActiveRecord::Relation.
   # @return [void]
-  def perform_preload(bookmarks)
-    return if !bookmarkable_klass.has_preloads?
+  def perform_preload(bookmarks, guardian)
+    bookmarks_of_type = Bookmark.select_type(bookmarks, bookmarkable_klass.model.to_s)
+    return if bookmarks_of_type.empty?
 
-    ActiveRecord::Associations::Preloader
-      .new(
-        records: Bookmark.select_type(bookmarks, bookmarkable_klass.model.to_s),
-        associations: [bookmarkable: bookmarkable_klass.preload_associations]
-      )
-      .call
+    if bookmarkable_klass.has_preloads?
+      ActiveRecord::Associations::Preloader
+        .new(
+          records: bookmarks_of_type,
+          associations: [bookmarkable: bookmarkable_klass.preload_associations]
+        )
+        .call
+    end
+
+    bookmarkable_klass.perform_custom_preload!(bookmarks_of_type, guardian)
   end
 
   def can_send_reminder?(bookmark)

app/services/topic_bookmarkable.rb

@@ -12,7 +12,16 @@ class TopicBookmarkable < BaseBookmarkable
   end
 
   def self.preload_associations
-    [:topic_users, :tags, { posts: :user }]
+    [:tags, { posts: :user }]
+  end
+
+  def self.perform_custom_preload!(topic_bookmarks, guardian)
+    topics = topic_bookmarks.map(&:bookmarkable)
+    topic_user_lookup = TopicUser.lookup_for(guardian.user, topics)
+
+    topics.each do |topic|
+      topic.user_data = topic_user_lookup[topic.id]
+    end
   end
 
   def self.list_query(user, guardian)

lib/bookmark_query.rb

@@ -10,7 +10,7 @@ class BookmarkQuery
   end
 
   def self.preload(bookmarks, object)
-    preload_polymorphic_associations(bookmarks)
+    preload_polymorphic_associations(bookmarks, object.guardian)
     if @preload
       @preload.each { |preload| preload.call(bookmarks, object) }
     end
@@ -20,12 +20,14 @@ class BookmarkQuery
   # life easier, which conditionally chooses the bookmark serializer to use based
   # on the type, and we want the associations all loaded ahead of time to make
   # sure we are not doing N+1s.
-  def self.preload_polymorphic_associations(bookmarks)
+  def self.preload_polymorphic_associations(bookmarks, guardian)
     Bookmark.registered_bookmarkables.each do |registered_bookmarkable|
-      registered_bookmarkable.perform_preload(bookmarks)
+      registered_bookmarkable.perform_preload(bookmarks, guardian)
     end
   end
 
+  attr_reader :guardian
+
   def initialize(user:, guardian: nil, params: {})
     @user = user
     @params = params

spec/requests/users_controller_spec.rb

@@ -5476,6 +5476,90 @@ RSpec.describe UsersController do
       expect(response.status).to eq(200)
       expect(response.parsed_body['bookmarks']).to eq([])
     end
+
+  end
+
+  describe "#bookmarks excerpts" do
+    fab!(:user) { Fabricate(:user) }
+    let!(:topic) { Fabricate(:topic, user: user) }
+    let!(:post) { Fabricate(:post, topic: topic) }
+    let!(:bookmark) { Fabricate(:bookmark, name: 'Test', user: user, bookmarkable: topic) }
+
+    it "uses the last_read_post_number + 1 for the bookmarks excerpt" do
+      next_unread_post = Fabricate(:post_with_long_raw_content, topic: bookmark.bookmarkable)
+      Fabricate(:post_with_external_links, topic: bookmark.bookmarkable)
+      bookmark.reload
+      TopicUser.change(user.id, bookmark.bookmarkable.id, { last_read_post_number: post.post_number })
+
+      sign_in(user)
+
+      get "/u/#{user.username}/bookmarks.json"
+      expect(response.status).to eq(200)
+      bookmark_list = response.parsed_body["user_bookmark_list"]["bookmarks"]
+      expected_excerpt = PrettyText.excerpt(next_unread_post.cooked, 300, keep_emoji_images: true)
+      expect(bookmark_list.first["excerpt"]).to eq(expected_excerpt)
+    end
+
+    it "does not use a small post for the last unread cooked post" do
+      small_action_post = Fabricate(:small_action, topic: bookmark.bookmarkable)
+      next_unread_post = Fabricate(:post_with_long_raw_content, topic: bookmark.bookmarkable)
+      Fabricate(:post_with_external_links, topic: bookmark.bookmarkable)
+      bookmark.reload
+      TopicUser.change(user.id, bookmark.bookmarkable.id, { last_read_post_number: post.post_number })
+
+      sign_in(user)
+
+      get "/u/#{user.username}/bookmarks.json"
+      expect(response.status).to eq(200)
+      bookmark_list = response.parsed_body["user_bookmark_list"]["bookmarks"]
+
+      expect(bookmark_list.first["excerpt"]).to eq(PrettyText.excerpt(next_unread_post.cooked, 300, keep_emoji_images: true))
+    end
+
+    it "handles the last read post in the topic being a small post by getting the last read regular post" do
+      last_regular_post = Fabricate(:post_with_long_raw_content, topic: bookmark.bookmarkable)
+      small_action_post = Fabricate(:small_action, topic: bookmark.bookmarkable)
+      bookmark.reload
+      topic.reload
+      TopicUser.change(user.id, bookmark.bookmarkable.id, { last_read_post_number: small_action_post.post_number })
+
+      sign_in(user)
+
+      get "/u/#{user.username}/bookmarks.json"
+      expect(response.status).to eq(200)
+      bookmark_list = response.parsed_body["user_bookmark_list"]["bookmarks"]
+
+      expect(bookmark_list.first["excerpt"]).to eq(PrettyText.excerpt(last_regular_post.cooked, 300, keep_emoji_images: true))
+    end
+
+    describe "bookmarkable_url" do
+      context "with the link_to_first_unread_post option" do
+        it "is a full topic URL to the first unread post in the topic when the option is set" do
+          TopicUser.change(user.id, bookmark.bookmarkable.id, { last_read_post_number: post.post_number })
+
+          sign_in(user)
+
+          get "/u/#{user.username}/user-menu-bookmarks.json"
+          expect(response.status).to eq(200)
+          bookmark_list = response.parsed_body["bookmarks"]
+
+          expect(bookmark_list.first["bookmarkable_url"]).to end_with("/t/#{topic.slug}/#{topic.id}/#{post.post_number + 1}")
+        end
+
+        it "is a full topic URL to the first post in the topic when the option isn't set" do
+          TopicUser.change(user.id, bookmark.bookmarkable.id, { last_read_post_number: post.post_number })
+
+          sign_in(user)
+
+          get "/u/#{user.username}/bookmarks.json"
+          expect(response.status).to eq(200)
+          bookmark_list = response.parsed_body["user_bookmark_list"]["bookmarks"]
+
+          expect(bookmark_list.first["bookmarkable_url"]).to end_with("/t/#{topic.slug}/#{topic.id}")
+        end
+      end
+    end
+
   end
 
   describe "#private_message_topic_tracking_state" do

spec/serializers/user_topic_bookmark_serializer_spec.rb

@@ -1,60 +0,0 @@
-# frozen_string_literal: true
-
-RSpec.describe UserTopicBookmarkSerializer do
-  fab!(:user) { Fabricate(:user) }
-  let!(:topic) { Fabricate(:topic, user: user) }
-  let!(:post) { Fabricate(:post, topic: topic) }
-  let!(:bookmark) { Fabricate(:bookmark, name: 'Test', user: user, bookmarkable: topic) }
-
-  describe "#excerpt" do
-    it "uses the last_read_post_number + 1 for the bookmarks excerpt" do
-      next_unread_post = Fabricate(:post_with_long_raw_content, topic: bookmark.bookmarkable)
-      Fabricate(:post_with_external_links, topic: bookmark.bookmarkable)
-      bookmark.reload
-      TopicUser.change(user.id, bookmark.bookmarkable.id, { last_read_post_number: post.post_number })
-      serializer = UserTopicBookmarkSerializer.new(bookmark, scope: Guardian.new(user))
-      expect(serializer.excerpt).to eq(PrettyText.excerpt(next_unread_post.cooked, 300, keep_emoji_images: true))
-    end
-
-    it "does not use a small post for the last unread cooked post" do
-      small_action_post = Fabricate(:small_action, topic: bookmark.bookmarkable)
-      next_unread_post = Fabricate(:post_with_long_raw_content, topic: bookmark.bookmarkable)
-      Fabricate(:post_with_external_links, topic: bookmark.bookmarkable)
-      bookmark.reload
-      TopicUser.change(user.id, bookmark.bookmarkable.id, { last_read_post_number: post.post_number })
-      serializer = UserTopicBookmarkSerializer.new(bookmark, scope: Guardian.new(user))
-      expect(serializer.excerpt).to eq(PrettyText.excerpt(next_unread_post.cooked, 300, keep_emoji_images: true))
-    end
-
-    it "handles the last read post in the topic being a small post by getting the last read regular post" do
-      last_regular_post = Fabricate(:post_with_long_raw_content, topic: bookmark.bookmarkable)
-      small_action_post = Fabricate(:small_action, topic: bookmark.bookmarkable)
-      bookmark.reload
-      topic.reload
-      TopicUser.change(user.id, bookmark.bookmarkable.id, { last_read_post_number: small_action_post.post_number })
-      serializer = UserTopicBookmarkSerializer.new(bookmark, scope: Guardian.new(user))
-      expect(serializer.cooked).to eq(last_regular_post.cooked)
-      expect(serializer.excerpt).to eq(PrettyText.excerpt(last_regular_post.cooked, 300, keep_emoji_images: true))
-    end
-  end
-
-  describe "#bookmarkable_url" do
-    context "with the link_to_first_unread_post option" do
-      it "is a full topic URL to the first unread post in the topic when the option is set" do
-        TopicUser.change(user.id, bookmark.bookmarkable.id, { last_read_post_number: post.post_number })
-        serializer = UserTopicBookmarkSerializer.new(
-          bookmark,
-          scope: Guardian.new(user),
-          link_to_first_unread_post: true
-        )
-        expect(serializer.bookmarkable_url).to end_with("/t/#{topic.slug}/#{topic.id}/#{post.post_number + 1}")
-      end
-
-      it "is a full topic URL to the first post in the topic when the option isn't set" do
-        TopicUser.change(user.id, bookmark.bookmarkable.id, { last_read_post_number: post.post_number })
-        serializer = UserTopicBookmarkSerializer.new(bookmark, scope: Guardian.new(user))
-        expect(serializer.bookmarkable_url).to end_with("/t/#{topic.slug}/#{topic.id}")
-      end
-    end
-  end
-end