From 7feabd9e49fbfb2ba4d04a463c962602e720f1bc Mon Sep 17 00:00:00 2001 From: David Taylor Date: Thu, 20 Dec 2018 17:13:05 +0000 Subject: [PATCH] PERF: Eradicate N+1 queries from the theme admin page --- app/controllers/admin/themes_controller.rb | 11 +++++++++-- app/models/theme.rb | 6 ++++-- app/models/theme_setting.rb | 1 + app/serializers/theme_serializer.rb | 2 +- lib/theme_settings_manager.rb | 4 +++- spec/components/theme_settings_manager_spec.rb | 17 ++++++++++++++++- 6 files changed, 34 insertions(+), 7 deletions(-) diff --git a/app/controllers/admin/themes_controller.rb b/app/controllers/admin/themes_controller.rb index 3e39de47a88..ab0347e9280 100644 --- a/app/controllers/admin/themes_controller.rb +++ b/app/controllers/admin/themes_controller.rb @@ -96,8 +96,15 @@ class Admin::ThemesController < Admin::AdminController end def index - @themes = Theme.order(:name).includes(:theme_fields, :remote_theme) - @color_schemes = ColorScheme.all.to_a + @themes = Theme.order(:name).includes(:child_themes, + :remote_theme, + :theme_settings, + :settings_field, + :user, + :color_scheme, + theme_fields: :upload + ) + @color_schemes = ColorScheme.all.includes(:theme, color_scheme_colors: :color_scheme).to_a light = ColorScheme.new(name: I18n.t("color_schemes.light")) @color_schemes.unshift(light) diff --git a/app/models/theme.rb b/app/models/theme.rb index f3b90ff6f0b..29073467bfe 100644 --- a/app/models/theme.rb +++ b/app/models/theme.rb @@ -16,10 +16,12 @@ class Theme < ActiveRecord::Base has_many :theme_fields, dependent: :destroy has_many :theme_settings, dependent: :destroy has_many :child_theme_relation, class_name: 'ChildTheme', foreign_key: 'parent_theme_id', dependent: :destroy - has_many :child_themes, through: :child_theme_relation, source: :child_theme + has_many :child_themes, -> { order(:name) }, through: :child_theme_relation, source: :child_theme has_many :color_schemes belongs_to :remote_theme + has_one :settings_field, -> { where(target_id: Theme.targets[:settings], name: "yaml") }, class_name: 'ThemeField' + validate :component_validations scope :user_selectable, ->() { @@ -346,7 +348,7 @@ class Theme < ActiveRecord::Base end def settings - field = theme_fields.where(target_id: Theme.targets[:settings], name: "yaml").first + field = settings_field return [] unless field && field.error.nil? settings = [] diff --git a/app/models/theme_setting.rb b/app/models/theme_setting.rb index 3616cb8a716..bb58c0e167c 100644 --- a/app/models/theme_setting.rb +++ b/app/models/theme_setting.rb @@ -9,6 +9,7 @@ class ThemeSetting < ActiveRecord::Base theme.clear_cached_settings! theme.remove_from_cache! theme.theme_fields.update_all(value_baked: nil) + theme.theme_settings.reload SvgSprite.expire_cache if self.name.to_s.include?("_icon") CSP::Extension.clear_theme_extensions_cache! if name.to_s == CSP::Extension::THEME_SETTING end diff --git a/app/serializers/theme_serializer.rb b/app/serializers/theme_serializer.rb index 41dfe0120d6..eb744802dd2 100644 --- a/app/serializers/theme_serializer.rb +++ b/app/serializers/theme_serializer.rb @@ -75,7 +75,7 @@ class ThemeSerializer < ChildThemeSerializer end def child_themes - object.child_themes.order(:name) + object.child_themes end def settings diff --git a/lib/theme_settings_manager.rb b/lib/theme_settings_manager.rb index 80802719440..81c2a7ef0c9 100644 --- a/lib/theme_settings_manager.rb +++ b/lib/theme_settings_manager.rb @@ -45,7 +45,9 @@ class ThemeSettingsManager end def db_record - ThemeSetting.where(name: @name, data_type: type, theme: @theme).first + # theme.theme_settings will already be preloaded, so it is better to use + # `find` on an array, rather than make a round trip to the database + theme.theme_settings.to_a.find { |i| i.name.to_s == @name.to_s && i.data_type.to_s == type.to_s } end def has_record? diff --git a/spec/components/theme_settings_manager_spec.rb b/spec/components/theme_settings_manager_spec.rb index 1dd8a93d065..feb0382298e 100644 --- a/spec/components/theme_settings_manager_spec.rb +++ b/spec/components/theme_settings_manager_spec.rb @@ -3,8 +3,8 @@ require 'theme_settings_manager' describe ThemeSettingsManager do + let(:theme) { Fabricate(:theme) } let(:theme_settings) do - theme = Fabricate(:theme) yaml = File.read("#{Rails.root}/spec/fixtures/theme_settings/valid_settings.yaml") theme.set_field(target: :settings, name: "yaml", value: yaml) theme.save! @@ -37,12 +37,15 @@ describe ThemeSettingsManager do expect(bool_setting.value).to eq(true) # default bool_setting.value = "true" + theme.reload expect(bool_setting.value).to eq(true) bool_setting.value = "falsse" # intentionally misspelled + theme.reload expect(bool_setting.value).to eq(false) bool_setting.value = true + theme.reload expect(bool_setting.value).to eq(true) end end @@ -51,15 +54,19 @@ describe ThemeSettingsManager do it "is always an integer" do int_setting = find_by_name(:integer_setting) int_setting.value = 1.6 + theme.reload expect(int_setting.value).to eq(1) int_setting.value = "4.3" + theme.reload expect(int_setting.value).to eq(4) int_setting.value = "10" + theme.reload expect(int_setting.value).to eq(10) int_setting.value = "text" + theme.reload expect(int_setting.value).to eq(0) end @@ -69,9 +76,11 @@ describe ThemeSettingsManager do expect { int_setting.value = 61 }.to raise_error(Discourse::InvalidParameters) int_setting.value = 60 + theme.reload expect(int_setting.value).to eq(60) int_setting.value = 1 + theme.reload expect(int_setting.value).to eq(1) end end @@ -80,12 +89,15 @@ describe ThemeSettingsManager do it "is always a float" do float_setting = find_by_name(:float_setting) float_setting.value = 1.615 + theme.reload expect(float_setting.value).to eq(1.615) float_setting.value = "3.1415" + theme.reload expect(float_setting.value).to eq(3.1415) float_setting.value = 10 + theme.reload expect(float_setting.value).to eq(10) end @@ -96,6 +108,7 @@ describe ThemeSettingsManager do expect { float_setting.value = "text" }.to raise_error(Discourse::InvalidParameters) float_setting.value = 9.521 + theme.reload expect(float_setting.value).to eq(9.521) end end @@ -106,9 +119,11 @@ describe ThemeSettingsManager do expect { string_setting.value = "a" }.to raise_error(Discourse::InvalidParameters) string_setting.value = "ab" + theme.reload expect(string_setting.value).to eq("ab") string_setting.value = "ab" * 10 + theme.reload expect(string_setting.value).to eq("ab" * 10) expect { string_setting.value = ("a" * 21) }.to raise_error(Discourse::InvalidParameters)