PERF: Strict loading for SidebarSection queries (#21717)

What is this change required?

I noticed that actions in `SidebarSectionsController` resulted in
lots of N+1 queries problem and I wanted a solution to
prevent such problems without having to write N+1 queries tests. I have
also used strict loading for `SidebarSection` queries in performance
sensitive spots.

Note that in this commit, I have also set `config.active_record.action_on_strict_loading_violation = :log`
for the production environment so that we have more visibility of
potential N+1 queries problem in the logs. In development and test
environment, we're sticking with the default of raising an error.

app/assets/javascripts/discourse/app/controllers/sidebar-section-form.js

@@ -359,11 +359,12 @@ export default Controller.extend(ModalFunctionality, {
           return ajax(`/sidebar_sections/${this.model.id}`, {
             type: "DELETE",
           })
-            .then((data) => {
+            .then(() => {
               const newSidebarSections =
                 this.currentUser.sidebar_sections.filter((section) => {
-                  return section.id !== data["sidebar_section"].id;
+                  return section.id !== this.model.id;
                 });
+
               this.currentUser.set("sidebar_sections", newSidebarSections);
               this.send("closeModal");
             })

app/controllers/sidebar_sections_controller.rb

@@ -7,9 +7,12 @@ class SidebarSectionsController < ApplicationController
   def index
     sections =
       SidebarSection
+        .strict_loading
+        .includes(:sidebar_urls)
         .where("public OR user_id = ?", current_user.id)
         .order("(public IS TRUE) DESC")
         .map { |section| SidebarSectionSerializer.new(section, root: false) }
+
     render json: sections
   end
 
@@ -23,7 +26,7 @@ class SidebarSectionsController < ApplicationController
       Site.clear_anon_cache!
     end
 
-    render json: SidebarSectionSerializer.new(sidebar_section)
+    render_serialized(sidebar_section, SidebarSectionSerializer)
   rescue ActiveRecord::RecordInvalid => e
     render_json_error(e.record.errors.full_messages.first)
   end
@@ -35,6 +38,7 @@ class SidebarSectionsController < ApplicationController
     ActiveRecord::Base.transaction do
       sidebar_section.update!(section_params.merge(sidebar_urls_attributes: links_params))
       sidebar_section.sidebar_section_links.update_all(user_id: sidebar_section.user_id)
+
       order =
         sidebar_section
           .sidebar_urls
@@ -55,7 +59,7 @@ class SidebarSectionsController < ApplicationController
       Site.clear_anon_cache!
     end
 
-    render_serialized(sidebar_section.reload, SidebarSectionSerializer)
+    render_serialized(sidebar_section, SidebarSectionSerializer)
   rescue ActiveRecord::RecordInvalid => e
     render_json_error(e.record.errors.full_messages.first)
   rescue Discourse::InvalidAccess
@@ -71,17 +75,17 @@ class SidebarSectionsController < ApplicationController
     when "community"
       sidebar_section.reset_community!
     end
-    render_serialized(sidebar_section.reload, SidebarSectionSerializer)
+
+    render_serialized(sidebar_section, SidebarSectionSerializer)
   end
 
   def reorder
     sidebar_section = SidebarSection.find_by(id: reorder_params["sidebar_section_id"])
     @guardian.ensure_can_edit!(sidebar_section)
-
     order = reorder_params["links_order"].map(&:to_i).each_with_index.to_h
-
     set_order(sidebar_section, order)
-    render json: sidebar_section
+
+    render_serialized_sidebar_section(sidebar_section)
   rescue Discourse::InvalidAccess
     render json: failed_json, status: 403
   end
@@ -95,7 +99,8 @@ class SidebarSectionsController < ApplicationController
       StaffActionLogger.new(current_user).log_destroy_public_sidebar_section(sidebar_section)
       MessageBus.publish("/refresh-sidebar-sections", nil)
     end
-    render json: SidebarSectionSerializer.new(sidebar_section)
+
+    render json: success_json
   rescue Discourse::InvalidAccess
     render json: failed_json, status: 403
   end
@@ -127,6 +132,7 @@ class SidebarSectionsController < ApplicationController
         .sidebar_section_links
         .sort_by { |link| order[link.linkable_id] }
         .map { |link| link.attributes.merge(position: position_generator.next) }
+
     sidebar_section.sidebar_section_links.upsert_all(links, update_only: [:position])
   end
 

app/models/sidebar_section.rb

@@ -5,6 +5,7 @@ class SidebarSection < ActiveRecord::Base
 
   belongs_to :user
   has_many :sidebar_section_links, -> { order("position") }, dependent: :destroy
+
   has_many :sidebar_urls,
            through: :sidebar_section_links,
            source: :linkable,

app/serializers/current_user_serializer.rb

@@ -79,7 +79,7 @@ class CurrentUserSerializer < BasicUserSerializer
     SidebarSection
       .public_sections
       .or(SidebarSection.where(user_id: object.id))
-      .includes(sidebar_section_links: :linkable)
+      .includes(:sidebar_urls)
       .order("(section_type IS NOT NULL) DESC, (public IS TRUE) DESC")
       .map { |section| SidebarSectionSerializer.new(section, root: false) }
   end

app/serializers/sidebar_section_serializer.rb

@@ -4,7 +4,7 @@ class SidebarSectionSerializer < ApplicationSerializer
   attributes :id, :title, :links, :slug, :public, :section_type
 
   def links
-    object.sidebar_section_links.map { |link| SidebarUrlSerializer.new(link.linkable, root: false) }
+    object.sidebar_urls.map { |sidebar_url| SidebarUrlSerializer.new(sidebar_url, root: false) }
   end
 
   def slug

app/serializers/site_serializer.rb

@@ -260,7 +260,7 @@ class SiteSerializer < ApplicationSerializer
   def anonymous_sidebar_sections
     SidebarSection
       .public_sections
-      .includes(sidebar_section_links: :linkable)
+      .includes(:sidebar_urls)
       .order("(section_type IS NOT NULL) DESC, (public IS TRUE) DESC")
       .map { |section| SidebarSectionSerializer.new(section, root: false) }
   end

config/environments/production.rb

@@ -71,4 +71,6 @@ Discourse::Application.configure do
   if ENV["RAILS_LOG_TO_STDOUT"].present?
     config.logger = ActiveSupport::TaggedLogging.new(Logger.new(STDOUT))
   end
+
+  config.active_record.action_on_strict_loading_violation = :log
 end

spec/models/sidebar_section_spec.rb

@@ -17,9 +17,10 @@ RSpec.describe SidebarSection do
     community_section.update!(title: "test")
     community_section.sidebar_section_links.first.linkable.update!(name: "everything edited")
     community_section.sidebar_section_links.last.destroy!
-
     community_section.reset_community!
+
     expect(community_section.reload.title).to eq("Community")
+
     expect(community_section.sidebar_section_links.all.map { |link| link.linkable.name }).to eq(
       ["Everything", "My Posts", "Review", "Admin", "Users", "About", "FAQ", "Groups", "Badges"],
     )