From 84f590ab83b90d4c6754e247744567f75274fb69 Mon Sep 17 00:00:00 2001 From: Daniel Waterworth Date: Tue, 14 Mar 2023 13:11:45 -0500 Subject: [PATCH] DEV: Store theme sprites in the DB (#20501) Let's avoid fetching sprites from the CDN during page rendering. --- app/models/theme.rb | 1 + app/models/theme_field.rb | 41 ++++++- app/models/theme_svg_sprite.rb | 26 ++++ .../20230224193734_create_theme_svg_sprite.rb | 15 +++ .../20230224225129_backfill_svg_sprites.rb | 9 ++ lib/svg_sprite.rb | 114 ++++++++++-------- lib/tasks/svg_sprites.rake | 5 + spec/lib/svg_sprite/svg_sprite_spec.rb | 9 +- spec/models/theme_field_spec.rb | 28 ++++- spec/models/theme_svg_sprite_spec.rb | 47 ++++++++ 10 files changed, 232 insertions(+), 63 deletions(-) create mode 100644 app/models/theme_svg_sprite.rb create mode 100644 db/migrate/20230224193734_create_theme_svg_sprite.rb create mode 100644 db/post_migrate/20230224225129_backfill_svg_sprites.rb create mode 100644 lib/tasks/svg_sprites.rake create mode 100644 spec/models/theme_svg_sprite_spec.rb diff --git a/app/models/theme.rb b/app/models/theme.rb index 808a01a1aa4..902219e03b5 100644 --- a/app/models/theme.rb +++ b/app/models/theme.rb @@ -33,6 +33,7 @@ class Theme < ActiveRecord::Base has_many :color_schemes belongs_to :remote_theme, dependent: :destroy has_one :theme_modifier_set, dependent: :destroy + has_one :theme_svg_sprite, dependent: :destroy has_one :settings_field, -> { where(target_id: Theme.targets[:settings], name: "yaml") }, diff --git a/app/models/theme_field.rb b/app/models/theme_field.rb index e7f51ce76f7..415f1d5c97c 100644 --- a/app/models/theme_field.rb +++ b/app/models/theme_field.rb @@ -44,6 +44,11 @@ class ThemeField < ActiveRecord::Base .select("DISTINCT ON (X.theme_sort_column) *") } + scope :svg_sprite_fields, + -> { + where(type_id: ThemeField.theme_var_type_ids, name: SvgSprite.theme_sprite_variable_name) + } + def self.types @types ||= Enum.new( @@ -656,9 +661,41 @@ class ThemeField < ActiveRecord::Base end end - after_save { dependent_fields.each(&:invalidate_baked!) } + def upsert_svg_sprite! + begin + content = upload.content + rescue => e + Discourse.warn_exception(e, message: "Failed to fetch svg sprite for theme field #{id}") + else + if content.length > 4 * 1024**2 + Rails.logger.warn( + "can't store theme svg sprite for theme #{theme_id} and upload #{upload_id}, sprite too big", + ) + else + ThemeSvgSprite.upsert( + { theme_id: theme_id, upload_id: upload_id, sprite: content }, + unique_by: :theme_id, + ) + end + end + end - after_destroy { DB.after_commit { SvgSprite.expire_cache } if svg_sprite_field? } + after_save do + dependent_fields.each(&:invalidate_baked!) + + if upload && svg_sprite_field? + upsert_svg_sprite! + DB.after_commit { SvgSprite.expire_cache } + end + end + + after_destroy do + if svg_sprite_field? + ThemeSvgSprite.where(theme_id: theme_id).delete_all + + DB.after_commit { SvgSprite.expire_cache } + end + end private diff --git a/app/models/theme_svg_sprite.rb b/app/models/theme_svg_sprite.rb new file mode 100644 index 00000000000..be8e087aeaa --- /dev/null +++ b/app/models/theme_svg_sprite.rb @@ -0,0 +1,26 @@ +# frozen_string_literal: true + +class ThemeSvgSprite < ActiveRecord::Base + belongs_to :theme + + def self.refetch! + ThemeField.svg_sprite_fields.find_each(&:upsert_svg_sprite!) + DB.after_commit { SvgSprite.expire_cache } + end +end + +# == Schema Information +# +# Table name: theme_svg_sprites +# +# id :bigint not null, primary key +# theme_id :integer not null +# upload_id :integer not null +# sprite :binary not null +# created_at :datetime not null +# updated_at :datetime not null +# +# Indexes +# +# index_theme_svg_sprites_on_theme_id (theme_id) UNIQUE +# diff --git a/db/migrate/20230224193734_create_theme_svg_sprite.rb b/db/migrate/20230224193734_create_theme_svg_sprite.rb new file mode 100644 index 00000000000..12439e44fa5 --- /dev/null +++ b/db/migrate/20230224193734_create_theme_svg_sprite.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +class CreateThemeSvgSprite < ActiveRecord::Migration[7.0] + def change + create_table :theme_svg_sprites do |t| + t.integer :theme_id, null: false + t.integer :upload_id, null: false + t.binary :sprite, null: false + + t.timestamps + end + + add_index :theme_svg_sprites, :theme_id, unique: true + end +end diff --git a/db/post_migrate/20230224225129_backfill_svg_sprites.rb b/db/post_migrate/20230224225129_backfill_svg_sprites.rb new file mode 100644 index 00000000000..b7b8b04d69e --- /dev/null +++ b/db/post_migrate/20230224225129_backfill_svg_sprites.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +class BackfillSvgSprites < ActiveRecord::Migration[7.0] + disable_ddl_transaction! + + def up + ThemeSvgSprite.refetch! + end +end diff --git a/lib/svg_sprite.rb b/lib/svg_sprite.rb index f044723ccbf..dac5925cba3 100644 --- a/lib/svg_sprite.rb +++ b/lib/svg_sprite.rb @@ -243,53 +243,71 @@ module SvgSprite badge_icons end - def self.custom_svg_sprites(theme_id) - get_set_cache("custom_svg_sprites_#{Theme.transform_ids(theme_id).join(",")}") do - plugin_paths = [] - Discourse - .plugins - .map { |plugin| File.dirname(plugin.path) } - .each { |path| plugin_paths << "#{path}/svg-icons/*.svg" } - - custom_sprite_paths = Dir.glob(plugin_paths) - - custom_sprites = - custom_sprite_paths.map do |path| - if File.exist?(path) - { filename: "#{File.basename(path, ".svg")}", sprite: File.read(path) } - end + def self.core_svg_sprites + @core_svg_sprites ||= + begin + CORE_SVG_SPRITES.map do |path| + { filename: File.basename(path, ".svg"), sprite: File.read(path) } end + end + end - if theme_id.present? - ThemeField - .where( + # Just used in tests + def self.clear_plugin_svg_sprite_cache! + @plugin_svg_sprites = nil + end + + def self.plugin_svg_sprites + @plugin_svg_sprites ||= + begin + plugin_paths = [] + Discourse + .plugins + .map { |plugin| File.dirname(plugin.path) } + .each { |path| plugin_paths << "#{path}/svg-icons/*.svg" } + + custom_sprite_paths = Dir.glob(plugin_paths) + + custom_sprite_paths.map do |path| + { filename: File.basename(path, ".svg"), sprite: File.read(path) } + end + end + end + + def self.theme_svg_sprites(theme_id) + if theme_id.present? + theme_ids = Theme.transform_ids(theme_id) + + get_set_cache("theme_svg_sprites_#{theme_ids.join(",")}") do + theme_field_uploads = + ThemeField.where( type_id: ThemeField.types[:theme_upload_var], name: THEME_SPRITE_VAR_NAME, - theme_id: Theme.transform_ids(theme_id), + theme_id: theme_ids, + ).pluck(:upload_id) + + theme_sprites = ThemeSvgSprite.where(theme_id: theme_ids).pluck(:upload_id, :sprite) + missing_sprites = (theme_field_uploads - theme_sprites.map(&:first)) + + if missing_sprites.present? + Rails.logger.warn( + "Missing ThemeSvgSprites for theme #{theme_id}, uploads #{missing_sprites.join(", ")}", ) - .pluck(:upload_id, :theme_id) - .each do |upload_id, child_theme_id| - begin - upload = Upload.find(upload_id) - custom_sprites << { - filename: "theme_#{theme_id}_#{upload_id}.svg", - sprite: upload.content, - } - rescue => e - name = - begin - Theme.find(child_theme_id).name - rescue StandardError - nil - end - Discourse.warn_exception(e, message: "#{name} theme contains a corrupt svg upload") - end - end + end + + theme_sprites.map do |upload_id, sprite| + { filename: "theme_#{theme_id}_#{upload_id}.svg", sprite: sprite } + end end - custom_sprites + else + [] end end + def self.custom_svg_sprites(theme_id) + plugin_svg_sprites + theme_svg_sprites(theme_id) + end + def self.all_icons(theme_id = nil) get_set_cache("icons_#{Theme.transform_ids(theme_id).join(",")}") do Set @@ -321,17 +339,9 @@ module SvgSprite cache&.clear end - def self.sprite_sources(theme_id) - sprites = [] - - CORE_SVG_SPRITES.each do |path| - if File.exist?(path) - sprites << { filename: "#{File.basename(path, ".svg")}", sprite: File.read(path) } - end - end - - sprites = sprites + custom_svg_sprites(theme_id) if theme_id.present? - + def self.sprites_for(theme_id) + sprites = core_svg_sprites + sprites += custom_svg_sprites(theme_id) if theme_id.present? sprites end @@ -393,7 +403,7 @@ License - https://fontawesome.com/license/free (Icons: CC BY 4.0, Fonts: SIL OFL .each do |sym| icon_id = prepare_symbol(sym, item[:filename]) - if icons.include? icon_id + if icon_id.present? sym.attributes["id"].value = icon_id sym.css("title").each(&:remove) svg_subset << sym.to_xml @@ -407,7 +417,7 @@ License - https://fontawesome.com/license/free (Icons: CC BY 4.0, Fonts: SIL OFL def self.search(searched_icon) searched_icon = process(searched_icon.dup) - sprite_sources(SiteSetting.default_theme_id).each do |item| + sprites_for(SiteSetting.default_theme_id).each do |item| svg_file = Nokogiri.XML(item[:sprite]) svg_file @@ -430,7 +440,7 @@ License - https://fontawesome.com/license/free (Icons: CC BY 4.0, Fonts: SIL OFL icons = all_icons(SiteSetting.default_theme_id) if only_available results = Set.new - sprite_sources(SiteSetting.default_theme_id).each do |item| + sprites_for(SiteSetting.default_theme_id).each do |item| svg_file = Nokogiri.XML(item[:sprite]) svg_file diff --git a/lib/tasks/svg_sprites.rake b/lib/tasks/svg_sprites.rake new file mode 100644 index 00000000000..09f3d7364ed --- /dev/null +++ b/lib/tasks/svg_sprites.rake @@ -0,0 +1,5 @@ +# frozen_string_literal: true + +task "svg_sprites:refetch" => [:environment] do |_, args| + ThemeSvgSprite.refetch! +end diff --git a/spec/lib/svg_sprite/svg_sprite_spec.rb b/spec/lib/svg_sprite/svg_sprite_spec.rb index 26dbf35a670..bdd7a91b75f 100644 --- a/spec/lib/svg_sprite/svg_sprite_spec.rb +++ b/spec/lib/svg_sprite/svg_sprite_spec.rb @@ -3,7 +3,10 @@ RSpec.describe SvgSprite do fab!(:theme) { Fabricate(:theme) } - before { SvgSprite.expire_cache } + before do + SvgSprite.clear_plugin_svg_sprite_cache! + SvgSprite.expire_cache + end it "can generate a bundle" do bundle = SvgSprite.bundle @@ -185,14 +188,14 @@ RSpec.describe SvgSprite do expect(sprite_files).to match(/my-custom-theme-icon/) SvgSprite.bundle(theme.id) - expect(SvgSprite.cache.hash.keys).to include("custom_svg_sprites_#{theme.id}") + expect(SvgSprite.cache.hash.keys).to include("theme_svg_sprites_#{theme.id}") external_copy = Discourse.store.download(upload_s3) File.delete external_copy.try(:path) SvgSprite.bundle(theme.id) # after a temp file is missing, bundling still works - expect(SvgSprite.cache.hash.keys).to include("custom_svg_sprites_#{theme.id}") + expect(SvgSprite.cache.hash.keys).to include("theme_svg_sprites_#{theme.id}") end end diff --git a/spec/models/theme_field_spec.rb b/spec/models/theme_field_spec.rb index 7098184d57a..cde2bcea1a9 100644 --- a/spec/models/theme_field_spec.rb +++ b/spec/models/theme_field_spec.rb @@ -611,7 +611,24 @@ HTML end describe "SVG sprite theme fields" do - let(:upload) { Fabricate(:upload) } + let :svg_content do + "" + end + + let :upload_file do + tmp = Tempfile.new("test.svg") + File.write(tmp.path, svg_content) + tmp + end + + after { upload_file.unlink } + + let(:upload) do + UploadCreator.new(upload_file, "test.svg", for_theme: true).create_for( + Discourse::SYSTEM_USER_ID, + ) + end + let(:theme) { Fabricate(:theme) } let(:theme_field) do ThemeField.create!( @@ -626,15 +643,14 @@ HTML end it "is rebaked when upload changes" do - theme_field.update(upload: Fabricate(:upload)) + fname = "custom-theme-icon-sprite.svg" + sprite = UploadCreator.new(file_from_fixtures(fname), fname, for_theme: true).create_for(-1) + theme_field.update(upload: sprite) expect(theme_field.value_baked).to eq(nil) end it "clears SVG sprite cache when upload is deleted" do - fname = "custom-theme-icon-sprite.svg" - sprite = UploadCreator.new(file_from_fixtures(fname), fname, for_theme: true).create_for(-1) - - theme_field.update(upload: sprite) + theme_field expect(SvgSprite.custom_svg_sprites(theme.id).size).to eq(1) theme_field.destroy! diff --git a/spec/models/theme_svg_sprite_spec.rb b/spec/models/theme_svg_sprite_spec.rb new file mode 100644 index 00000000000..635cc1fb619 --- /dev/null +++ b/spec/models/theme_svg_sprite_spec.rb @@ -0,0 +1,47 @@ +# frozen_string_literal: true + +RSpec.describe ThemeSvgSprite do + fab!(:theme) { Fabricate(:theme) } + + describe "#refetch!" do + context "when an upload exists" do + before do + fname = "custom-theme-icon-sprite.svg" + upload = UploadCreator.new(file_from_fixtures(fname), fname, for_theme: true).create_for(-1) + + theme.set_field( + target: :common, + name: SvgSprite.theme_sprite_variable_name, + upload_id: upload.id, + type: :theme_upload_var, + ) + + theme.save! + end + + it "fetches values from the store and puts them in the table" do + expect(ThemeSvgSprite.count).to eq(1) + + sprite = ThemeSvgSprite.first + original_content = sprite.sprite + + expect(original_content).not_to be_empty + + sprite.update!(sprite: "INVALID") + + ThemeSvgSprite.refetch! + + sprite.reload + expect(sprite.sprite).to eq(original_content) + expect(ThemeSvgSprite.count).to eq(1) + end + end + + # It needs to do this since the cache is based on values in this table + it "expires the svg sprite cache" do + SvgSprite.expects(:expire_cache) + + ThemeSvgSprite.refetch! + end + end +end