From 5cfe3234455250c60b05d3d3368540c5c3fac774 Mon Sep 17 00:00:00 2001 From: Alan Guo Xiang Tan Date: Thu, 25 May 2023 10:10:32 +0900 Subject: [PATCH] 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/controllers/sidebar-section-form.js | 5 +++-- .../sidebar_sections_controller.rb | 20 ++++++++++++------- app/models/sidebar_section.rb | 1 + app/serializers/current_user_serializer.rb | 2 +- app/serializers/sidebar_section_serializer.rb | 2 +- app/serializers/site_serializer.rb | 2 +- config/environments/production.rb | 2 ++ spec/models/sidebar_section_spec.rb | 3 ++- 8 files changed, 24 insertions(+), 13 deletions(-) diff --git a/app/assets/javascripts/discourse/app/controllers/sidebar-section-form.js b/app/assets/javascripts/discourse/app/controllers/sidebar-section-form.js index c41d5632b23..49ef501c9f1 100644 --- a/app/assets/javascripts/discourse/app/controllers/sidebar-section-form.js +++ b/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"); }) diff --git a/app/controllers/sidebar_sections_controller.rb b/app/controllers/sidebar_sections_controller.rb index 9f72bf06fd4..019709b6d88 100644 --- a/app/controllers/sidebar_sections_controller.rb +++ b/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 diff --git a/app/models/sidebar_section.rb b/app/models/sidebar_section.rb index dddbbdde720..0162ca5fe4b 100644 --- a/app/models/sidebar_section.rb +++ b/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, diff --git a/app/serializers/current_user_serializer.rb b/app/serializers/current_user_serializer.rb index c7c03f0bf74..c24a1690a43 100644 --- a/app/serializers/current_user_serializer.rb +++ b/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 diff --git a/app/serializers/sidebar_section_serializer.rb b/app/serializers/sidebar_section_serializer.rb index 14e4f600384..a1b184589c1 100644 --- a/app/serializers/sidebar_section_serializer.rb +++ b/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 diff --git a/app/serializers/site_serializer.rb b/app/serializers/site_serializer.rb index 941b7f17690..242b0ac24bb 100644 --- a/app/serializers/site_serializer.rb +++ b/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 diff --git a/config/environments/production.rb b/config/environments/production.rb index 582ae875037..4d57a995857 100644 --- a/config/environments/production.rb +++ b/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 diff --git a/spec/models/sidebar_section_spec.rb b/spec/models/sidebar_section_spec.rb index 7261efb0f48..f30168ffb0a 100644 --- a/spec/models/sidebar_section_spec.rb +++ b/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"], )