From 21bb28df91ecc1950dda271e8f3808e5614912ba Mon Sep 17 00:00:00 2001 From: Alan Guo Xiang Tan Date: Fri, 23 Aug 2024 16:10:50 +0800 Subject: [PATCH] PERF: Ensure suggested topics is only loaded on last page of topic view (#28507) This commit improves `TopicsController#show` to not load suggested and related topics unless it is the last page of the topic's view. Previously, we avoided loading suggested and related topics by the use of conditionals in the `TopicViewSerializer` to avoid calling `TopicView#suggested_topics` and `TopicView#related_topics`. However, this pattern is not reliable as the methods can still be called from other spots in the code base. Instead, we ensure that `TopicView#include_suggested` and `TopicView#include_related` is set correctly on the instance of `TopicView` which ensures that for the given instance, `TopicView#suggested_topics` and `TopicView#related_topics` will be a noop. --- app/controllers/topics_controller.rb | 9 ++-- app/serializers/suggested_topics_mixin.rb | 6 +-- lib/topic_view.rb | 7 ++- spec/requests/topics_controller_spec.rb | 51 +++++++++++++++---- .../serializers/topic_view_serializer_spec.rb | 30 +++++------ 5 files changed, 66 insertions(+), 37 deletions(-) diff --git a/app/controllers/topics_controller.rb b/app/controllers/topics_controller.rb index a439676f041..b5d910a8259 100644 --- a/app/controllers/topics_controller.rb +++ b/app/controllers/topics_controller.rb @@ -1336,15 +1336,18 @@ class TopicsController < ApplicationController return end + if params[:replies_to_post_number] || params[:filter_upwards_post_id] || + params[:filter_top_level_replies] || @topic_view.next_page.present? + @topic_view.include_suggested = false + @topic_view.include_related = false + end + topic_view_serializer = TopicViewSerializer.new( @topic_view, scope: guardian, root: false, include_raw: !!params[:include_raw], - exclude_suggested_and_related: - !!params[:replies_to_post_number] || !!params[:filter_upwards_post_id] || - !!params[:filter_top_level_replies], ) respond_to do |format| diff --git a/app/serializers/suggested_topics_mixin.rb b/app/serializers/suggested_topics_mixin.rb index e257a9436d6..835b9f7eb19 100644 --- a/app/serializers/suggested_topics_mixin.rb +++ b/app/serializers/suggested_topics_mixin.rb @@ -8,13 +8,11 @@ module SuggestedTopicsMixin end def include_related_messages? - return false if @options[:exclude_suggested_and_related] - object.next_page.nil? && object.related_messages&.topics + object.related_messages&.topics end def include_suggested_topics? - return false if @options[:exclude_suggested_and_related] - object.next_page.nil? && object.suggested_topics&.topics + object.suggested_topics&.topics end def include_suggested_group_name? diff --git a/lib/topic_view.rb b/lib/topic_view.rb index 9dbf50264c4..000464d0f62 100644 --- a/lib/topic_view.rb +++ b/lib/topic_view.rb @@ -41,6 +41,8 @@ class TopicView :user_custom_fields, :post_custom_fields, :post_number, + :include_suggested, + :include_related, ) delegate :category, to: :topic, allow_nil: true, private: true @@ -50,8 +52,10 @@ class TopicView 1000 end + CHUNK_SIZE = 20 + def self.chunk_size - 20 + CHUNK_SIZE end def self.default_post_custom_fields @@ -644,6 +648,7 @@ class TopicView { include_random: true, pm_params: pm_params }, self, ) + TopicQuery.new(@user).list_suggested_for(topic, **kwargs) end else diff --git a/spec/requests/topics_controller_spec.rb b/spec/requests/topics_controller_spec.rb index 6a8fef42358..66af5a01982 100644 --- a/spec/requests/topics_controller_spec.rb +++ b/spec/requests/topics_controller_spec.rb @@ -2236,9 +2236,6 @@ RSpec.describe TopicsController do fab!(:private_topic) { pm } fab!(:topic) { Fabricate(:post, user: post_author1).topic } - fab!(:p1) { Fabricate(:post, user: topic.user) } - fab!(:p2) { Fabricate(:post, user: topic.user) } - describe "when topic is not allowed" do it "should return the right response" do SiteSetting.detailed_404 = true @@ -3277,18 +3274,50 @@ RSpec.describe TopicsController do end end - it "returns a list of categories" do + it "returns suggested topics only when loading the last chunk of posts in a topic" do + topic_post_2 = Fabricate(:post, topic: topic) + topic_post_3 = Fabricate(:post, topic: topic) + topic_post_4 = Fabricate(:post, topic: topic) + + stub_const(TopicView, "CHUNK_SIZE", 2) do + get "/t/#{topic.slug}/#{topic.id}.json" + + expect(response.status).to eq(200) + expect(response.parsed_body.has_key?("suggested_topics")).to eq(false) + + get "/t/#{topic.slug}/#{topic.id}/4.json" + + expect(response.status).to eq(200) + expect(response.parsed_body.has_key?("suggested_topics")).to eq(true) + end + end + + it "returns a list of categories when `lazy_load_categories_group` site setting is enabled for the current user" do SiteSetting.lazy_load_categories_groups = "#{Group::AUTO_GROUPS[:everyone]}" - topic.update!(category: Fabricate(:category)) + + topic_post_2 = Fabricate(:post, topic: topic) + topic_post_3 = Fabricate(:post, topic: topic) + topic_post_4 = Fabricate(:post, topic: topic) dest_topic.update!(category: Fabricate(:category)) - get "/t/#{topic.slug}/#{topic.id}.json" + stub_const(TopicView, "CHUNK_SIZE", 2) do + get "/t/#{topic.slug}/#{topic.id}.json" - expect(response.parsed_body["categories"].map { |c| c["id"] }).to contain_exactly( - SiteSetting.uncategorized_category_id, - topic.category_id, - dest_topic.category_id, - ) + expect(response.status).to eq(200) + expect(response.parsed_body.has_key?("suggested_topics")).to eq(false) + expect(response.parsed_body["categories"].map { _1["id"] }).to contain_exactly( + topic.category_id, + ) + + get "/t/#{topic.slug}/#{topic.id}/4.json" + + expect(response.status).to eq(200) + expect(response.parsed_body.has_key?("suggested_topics")).to eq(true) + expect(response.parsed_body["categories"].map { _1["id"] }).to contain_exactly( + topic.category_id, + dest_topic.category_id, + ) + end end end diff --git a/spec/serializers/topic_view_serializer_spec.rb b/spec/serializers/topic_view_serializer_spec.rb index a54fcf6b81c..0b2bb245214 100644 --- a/spec/serializers/topic_view_serializer_spec.rb +++ b/spec/serializers/topic_view_serializer_spec.rb @@ -1,10 +1,12 @@ # frozen_string_literal: true RSpec.describe TopicViewSerializer do - def serialize_topic(topic, user_arg) - topic_view = TopicView.new(topic.id, user_arg) + def serialize_topic(topic, user_arg, opts = {}) + topic_view = TopicView.new(topic.id, user_arg, opts) + serializer = TopicViewSerializer.new(topic_view, scope: Guardian.new(user_arg), root: false).as_json + JSON.parse(MultiJson.dump(serializer)).deep_symbolize_keys! end @@ -99,27 +101,19 @@ RSpec.describe TopicViewSerializer do before { TopicUser.update_last_read(user, topic2.id, 0, 0, 0) } - describe "when loading last chunk" do - it "should include suggested topics" do - json = serialize_topic(topic, user) + it "should include suggested topics if `TopicView#include_suggested` is set to `true`" do + json = serialize_topic(topic, user, include_suggested: true) - expect(json[:suggested_topics].first[:id]).to eq(topic2.id) - end + expect(json[:suggested_topics].first[:id]).to eq(topic2.id) end - describe "when not loading last chunk" do - fab!(:post) { Fabricate(:post, topic: topic) } - fab!(:post2) { Fabricate(:post, topic: topic) } + it "should not include suggested topics if `TopicView#include_suggested` is set to `false`" do + post = Fabricate(:post, topic:) + post2 = Fabricate(:post, topic:) - it "should not include suggested topics" do - post - post2 - topic_view = TopicView.new(topic.id, user, post_ids: [post.id]) - topic_view.next_page - json = described_class.new(topic_view, scope: Guardian.new(user), root: false).as_json + json = serialize_topic(topic, user, include_suggested: false) - expect(json[:suggested_topics]).to eq(nil) - end + expect(json[:suggested_topics]).to eq(nil) end describe "with private messages" do