DEV: Store theme sprites in the DB (#20501)

Let's avoid fetching sprites from the CDN during page rendering.

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") },

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
 

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
+#

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

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

lib/svg_sprite.rb

@@ -243,53 +243,71 @@ module SvgSprite
     badge_icons
   end
 
-  def self.custom_svg_sprites(theme_id)
+  def self.core_svg_sprites
-    get_set_cache("custom_svg_sprites_#{Theme.transform_ids(theme_id).join(",")}") do
+    @core_svg_sprites ||=
-      plugin_paths = []
+      begin
-      Discourse
+        CORE_SVG_SPRITES.map do |path|
-        .plugins
+          { filename: File.basename(path, ".svg"), sprite: File.read(path) }
-        .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
         end
+      end
+  end
 
-      if theme_id.present?
+  # Just used in tests
-        ThemeField
+  def self.clear_plugin_svg_sprite_cache!
-          .where(
+    @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)
+        end
-          .each do |upload_id, child_theme_id|
+
-            begin
+        theme_sprites.map do |upload_id, sprite|
-              upload = Upload.find(upload_id)
+          { filename: "theme_#{theme_id}_#{upload_id}.svg", sprite: sprite }
-              custom_sprites << {
+        end
-                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
-      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)
+  def self.sprites_for(theme_id)
-    sprites = []
+    sprites = core_svg_sprites
-
+    sprites += custom_svg_sprites(theme_id) if theme_id.present?
-    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?
-
     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

lib/tasks/svg_sprites.rake

@@ -0,0 +1,5 @@
+# frozen_string_literal: true
+
+task "svg_sprites:refetch" => [:environment] do |_, args|
+  ThemeSvgSprite.refetch!
+end

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
 

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
+      "<svg></svg>"
+    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"
+      theme_field
-      sprite = UploadCreator.new(file_from_fixtures(fname), fname, for_theme: true).create_for(-1)
-
-      theme_field.update(upload: sprite)
       expect(SvgSprite.custom_svg_sprites(theme.id).size).to eq(1)
 
       theme_field.destroy!

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