From 913db5d546c8c52121cf2175d8ee9ca981f43ceb Mon Sep 17 00:00:00 2001 From: David Taylor Date: Wed, 17 Aug 2022 02:40:24 +0100 Subject: [PATCH] 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 --- ...ser_post_topic_bookmark_base_serializer.rb | 11 --- .../user_topic_bookmark_serializer.rb | 11 +++ app/services/base_bookmarkable.rb | 9 ++ app/services/post_bookmarkable.rb | 2 +- app/services/registered_bookmarkable.rb | 23 +++-- app/services/topic_bookmarkable.rb | 11 ++- lib/bookmark_query.rb | 8 +- spec/requests/users_controller_spec.rb | 84 +++++++++++++++++++ .../user_topic_bookmark_serializer_spec.rb | 60 ------------- 9 files changed, 135 insertions(+), 84 deletions(-) delete mode 100644 spec/serializers/user_topic_bookmark_serializer_spec.rb diff --git a/app/serializers/user_post_topic_bookmark_base_serializer.rb b/app/serializers/user_post_topic_bookmark_base_serializer.rb index b91002a4b1e..ca5d50e684d 100644 --- a/app/serializers/user_post_topic_bookmark_base_serializer.rb +++ b/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 diff --git a/app/serializers/user_topic_bookmark_serializer.rb b/app/serializers/user_topic_bookmark_serializer.rb index b4ef6762d2c..8aa78bc88b3 100644 --- a/app/serializers/user_topic_bookmark_serializer.rb +++ b/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 diff --git a/app/services/base_bookmarkable.rb b/app/services/base_bookmarkable.rb index dacc02c07e4..6976eece625 100644 --- a/app/services/base_bookmarkable.rb +++ b/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 diff --git a/app/services/post_bookmarkable.rb b/app/services/post_bookmarkable.rb index 5690d30db67..09373f6e99a 100644 --- a/app/services/post_bookmarkable.rb +++ b/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) diff --git a/app/services/registered_bookmarkable.rb b/app/services/registered_bookmarkable.rb index ce3cb1a4901..fc40dee521e 100644 --- a/app/services/registered_bookmarkable.rb +++ b/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) diff --git a/app/services/topic_bookmarkable.rb b/app/services/topic_bookmarkable.rb index 45ce54adfd0..67c474664c1 100644 --- a/app/services/topic_bookmarkable.rb +++ b/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) diff --git a/lib/bookmark_query.rb b/lib/bookmark_query.rb index 0d238e39abb..50c8b25bcbf 100644 --- a/lib/bookmark_query.rb +++ b/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 diff --git a/spec/requests/users_controller_spec.rb b/spec/requests/users_controller_spec.rb index b37bb8e447a..466273e16a5 100644 --- a/spec/requests/users_controller_spec.rb +++ b/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 diff --git a/spec/serializers/user_topic_bookmark_serializer_spec.rb b/spec/serializers/user_topic_bookmark_serializer_spec.rb deleted file mode 100644 index 38201107567..00000000000 --- a/spec/serializers/user_topic_bookmark_serializer_spec.rb +++ /dev/null @@ -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