From da025818024f800a00b3e8b1db3d54aec57a21e1 Mon Sep 17 00:00:00 2001 From: Krzysztof Kotlarek Date: Fri, 16 May 2025 12:05:09 +0800 Subject: [PATCH 1/2] FIX: N+1 in admin themes page `strict_loading` was added to prevent it happening in the future. Few adjustments had to be made: - include color_scheme and color_scheme_colors, also for parent and child themes; - internal translations were using preload_fields, but it was too deep to correctly use preloaded tables. I had to pass `preloaded_locale_fields` manually; - pass theme object to colour scheme serializer. It is because theme load color scheme and then color scheme serializer search again for theme name. --- app/controllers/admin/themes_controller.rb | 6 ++- app/models/theme.rb | 16 ++++---- app/serializers/basic_theme_serializer.rb | 5 ++- app/serializers/color_scheme_serializer.rb | 6 +-- app/serializers/theme_serializer.rb | 12 +++++- spec/requests/admin/themes_controller_spec.rb | 38 +++++++++++++++++++ 6 files changed, 66 insertions(+), 17 deletions(-) diff --git a/app/controllers/admin/themes_controller.rb b/app/controllers/admin/themes_controller.rb index e262230848c02..68ea28610a51d 100644 --- a/app/controllers/admin/themes_controller.rb +++ b/app/controllers/admin/themes_controller.rb @@ -169,8 +169,10 @@ def import end def index - @themes = Theme.include_relations.order(:name) - @color_schemes = ColorScheme.all.includes(:theme, color_scheme_colors: :color_scheme).to_a + @themes = Theme.strict_loading.include_relations.order(:name) + + @color_schemes = + ColorScheme.strict_loading.all.includes(:theme, color_scheme_colors: :color_scheme).to_a payload = { themes: serialize_data(@themes, ThemeSerializer), diff --git a/app/models/theme.rb b/app/models/theme.rb index dde8675fe64a4..66729164c5ffb 100644 --- a/app/models/theme.rb +++ b/app/models/theme.rb @@ -84,11 +84,11 @@ def self.cache scope :include_relations, -> do include_basic_relations.includes( - :child_themes, :theme_settings, :settings_field, - :color_scheme, + color_scheme: %i[color_scheme_colors], theme_fields: %i[upload theme_settings_migration], + child_themes: %i[color_scheme locale_fields theme_translation_overrides], ) end @@ -96,10 +96,11 @@ def self.cache -> do includes( :remote_theme, + :color_scheme, :user, :locale_fields, :theme_translation_overrides, - parent_themes: %i[locale_fields theme_translation_overrides], + parent_themes: %i[color_scheme locale_fields theme_translation_overrides], ) end @@ -658,15 +659,16 @@ def add_relative_theme!(kind, theme) end end - def internal_translations - @internal_translations ||= translations(internal: true) + def internal_translations(preloaded_locale_fields: nil) + @internal_translations ||= + translations(internal: true, preloaded_locale_fields: preloaded_locale_fields) end - def translations(internal: false) + def translations(internal: false, preloaded_locale_fields: nil) fallbacks = I18n.fallbacks[I18n.locale] begin data = - locale_fields.first&.translation_data( + (preloaded_locale_fields&.first || locale_fields.first)&.translation_data( with_overrides: false, internal: internal, fallback_fields: locale_fields, diff --git a/app/serializers/basic_theme_serializer.rb b/app/serializers/basic_theme_serializer.rb index 3339209f4a8d1..7c909f05e7b1f 100644 --- a/app/serializers/basic_theme_serializer.rb +++ b/app/serializers/basic_theme_serializer.rb @@ -12,6 +12,9 @@ def default end def description - object.internal_translations.find { |t| t.key == "theme_metadata.description" }&.value + object + .internal_translations(preloaded_locale_fields: object.locale_fields) + .find { |t| t.key == "theme_metadata.description" } + &.value end end diff --git a/app/serializers/color_scheme_serializer.rb b/app/serializers/color_scheme_serializer.rb index cbc492db8c95b..beb2871e30cb8 100644 --- a/app/serializers/color_scheme_serializer.rb +++ b/app/serializers/color_scheme_serializer.rb @@ -5,11 +5,7 @@ class ColorSchemeSerializer < ApplicationSerializer has_many :colors, serializer: ColorSchemeColorSerializer, embed: :objects def theme_name - object.theme&.name - end - - def theme_id - object.theme&.id + (options[:theme] || object.theme)&.name end def colors diff --git a/app/serializers/theme_serializer.rb b/app/serializers/theme_serializer.rb index 45580feb75132..3abd653573581 100644 --- a/app/serializers/theme_serializer.rb +++ b/app/serializers/theme_serializer.rb @@ -3,7 +3,8 @@ require "base64" class ThemeSerializer < BasicThemeSerializer - attributes :color_scheme_id, + attributes :color_scheme, + :color_scheme_id, :user_selectable, :auto_update, :remote_theme_id, @@ -15,7 +16,6 @@ class ThemeSerializer < BasicThemeSerializer :theme_fields, :screenshot_url - has_one :color_scheme, serializer: ColorSchemeSerializer, embed: :object has_one :user, serializer: UserNameSerializer, embed: :object has_one :disabled_by, serializer: UserNameSerializer, embed: :object @@ -32,6 +32,14 @@ def initialize(theme, options = {}) object.theme_fields.each { |o| @errors << o.error if o.error } end + def color_scheme + ColorSchemeSerializer.new(object.color_scheme, root: false, theme: object) + end + + def include_color_scheme? + object.color_scheme_id + end + def theme_fields ActiveModel::ArraySerializer.new( object.theme_fields, diff --git a/spec/requests/admin/themes_controller_spec.rb b/spec/requests/admin/themes_controller_spec.rb index cd9c3833447a3..261e6ba825841 100644 --- a/spec/requests/admin/themes_controller_spec.rb +++ b/spec/requests/admin/themes_controller_spec.rb @@ -476,6 +476,44 @@ expect(theme_json["remote_theme"]["remote_version"]).to eq("7") end + + it "does not result in N+1 queries" do + # warmup + get "/admin/themes.json" + expect(response.status).to eq(200) + + theme = Fabricate(:theme, color_scheme: Fabricate(:color_scheme)) + Fabricate( + :theme_field, + target_id: Theme.targets[:translations], + theme: theme, + name: "en", + value: + "en:\n theme_metadata:\n description: \"A simple, beautiful theme that improves the out of the box experience for Discourse sites.\"\n topic_pinned: \"Pinned\"\n topic_hot: \"Hot\"\n user_replied: \"replied\"\n user_posted: \"posted\"\n user_updated: \"updated\"\n", + ) + first_request_queries = + track_sql_queries do + get "/admin/themes.json" + expect(response.status).to eq(200) + end + + theme_2 = Fabricate(:theme, color_scheme: Fabricate(:color_scheme)) + Fabricate( + :theme_field, + target_id: Theme.targets[:translations], + theme: theme_2, + name: "en", + value: + "en:\n theme_metadata:\n description: \"A simple, beautiful theme that improves the out of the box experience for Discourse sites.\"\n topic_pinned: \"Pinned\"\n topic_hot: \"Hot\"\n user_replied: \"replied\"\n user_posted: \"posted\"\n user_updated: \"updated\"\n", + ) + second_request_queries = + track_sql_queries do + get "/admin/themes.json" + expect(response.status).to eq(200) + end + + expect(first_request_queries.count).to eq(second_request_queries.count) + end end it "allows themes and components to be edited" do From f2628d65afbc04a41ee25cae4f4759e69fe4e313 Mon Sep 17 00:00:00 2001 From: Krzysztof Kotlarek Date: Fri, 16 May 2025 15:38:25 +0800 Subject: [PATCH 2/2] FIX: include theme --- app/models/theme.rb | 2 +- app/serializers/color_scheme_serializer.rb | 2 +- app/serializers/theme_serializer.rb | 12 ++---------- 3 files changed, 4 insertions(+), 12 deletions(-) diff --git a/app/models/theme.rb b/app/models/theme.rb index 66729164c5ffb..319ffe2346ec6 100644 --- a/app/models/theme.rb +++ b/app/models/theme.rb @@ -96,10 +96,10 @@ def self.cache -> do includes( :remote_theme, - :color_scheme, :user, :locale_fields, :theme_translation_overrides, + color_scheme: %i[theme], parent_themes: %i[color_scheme locale_fields theme_translation_overrides], ) end diff --git a/app/serializers/color_scheme_serializer.rb b/app/serializers/color_scheme_serializer.rb index beb2871e30cb8..e277d4cf7e4fb 100644 --- a/app/serializers/color_scheme_serializer.rb +++ b/app/serializers/color_scheme_serializer.rb @@ -5,7 +5,7 @@ class ColorSchemeSerializer < ApplicationSerializer has_many :colors, serializer: ColorSchemeColorSerializer, embed: :objects def theme_name - (options[:theme] || object.theme)&.name + object.theme&.name end def colors diff --git a/app/serializers/theme_serializer.rb b/app/serializers/theme_serializer.rb index 3abd653573581..45580feb75132 100644 --- a/app/serializers/theme_serializer.rb +++ b/app/serializers/theme_serializer.rb @@ -3,8 +3,7 @@ require "base64" class ThemeSerializer < BasicThemeSerializer - attributes :color_scheme, - :color_scheme_id, + attributes :color_scheme_id, :user_selectable, :auto_update, :remote_theme_id, @@ -16,6 +15,7 @@ class ThemeSerializer < BasicThemeSerializer :theme_fields, :screenshot_url + has_one :color_scheme, serializer: ColorSchemeSerializer, embed: :object has_one :user, serializer: UserNameSerializer, embed: :object has_one :disabled_by, serializer: UserNameSerializer, embed: :object @@ -32,14 +32,6 @@ def initialize(theme, options = {}) object.theme_fields.each { |o| @errors << o.error if o.error } end - def color_scheme - ColorSchemeSerializer.new(object.color_scheme, root: false, theme: object) - end - - def include_color_scheme? - object.color_scheme_id - end - def theme_fields ActiveModel::ArraySerializer.new( object.theme_fields,