DEV: Handle null theme id in theme site setting cache (#33745)

There are a few legit scenarios where the current
theme ID may be blank, such as safe mode. In this
case it will be better to return default site setting
values rather than to cause random/undefined behaviour
in the UI.

Also clears the theme site setting cache when the
theme ID is null, along with the other fix here this
was probably causing issues in system spec runs

app/models/theme.rb

@@ -327,6 +327,8 @@ class Theme < ActiveRecord::Base
       .each do |theme_id|
         Discourse.cache.delete(SiteSettingExtension.theme_site_settings_cache_key(theme_id))
       end
+
+    Discourse.cache.delete(SiteSettingExtension.theme_site_settings_cache_key(nil))
   end
 
   def self.clear_default!

app/models/theme_site_setting.rb

@@ -99,6 +99,12 @@ class ThemeSiteSetting < ActiveRecord::Base
     theme_site_setting_values_map
   end
 
+  def self.generate_defaults_map
+    SiteSetting.themeable_site_settings.to_h do |setting_name|
+      [setting_name, SiteSetting.defaults[setting_name]]
+    end
+  end
+
   def setting_rb_value
     SiteSetting.type_supervisor.to_rb_value(self.name, self.value, self.data_type)
   end

lib/site_setting_extension.rb

@@ -186,7 +186,7 @@ module SiteSettingExtension
   end
 
   def themeable_site_settings
-    themeable.select { |_, value| value }.keys
+    themeable.select { |_, value| value }.keys.sort
   end
 
   def client_settings_json
@@ -234,7 +234,16 @@ module SiteSettingExtension
 
   def theme_site_settings_json_uncached(theme_id)
     begin
-      MultiJson.dump(theme_site_settings[theme_id])
+      # There are a few legit scenarios where the current
+      # theme ID may be blank, such as safe mode. In this
+      # case it will be better to return default site setting
+      # values rather than to cause random/undefined behaviour
+      # in the UI.
+      if theme_id.blank?
+        MultiJson.dump(ThemeSiteSetting.generate_defaults_map)
+      else
+        MultiJson.dump(theme_site_settings[theme_id])
+      end
     rescue => err
       # If something goes wrong here we really need to be aware of it in tests.
       raise err if Rails.env.test?
@@ -377,6 +386,8 @@ module SiteSettingExtension
   end
 
   def self.theme_site_settings_cache_key(theme_id)
+    theme_id = "notheme" if theme_id.blank?
+
     # NOTE: we use the git version in the key to ensure
     # that we don't end up caching the incorrect version
     # in cases where we are cycling unicorns

spec/lib/site_setting_extension_spec.rb

@@ -1055,6 +1055,13 @@ RSpec.describe SiteSettingExtension do
           %Q|{"enable_welcome_banner":false,"search_experience":"search_icon"}|,
         )
       end
+
+      it "returns default JSON when the theme_id is null" do
+        SiteSetting.refresh!
+        expect(SiteSetting.theme_site_settings_json_uncached(nil)).to eq(
+          %Q|{"enable_welcome_banner":true,"search_experience":"search_icon"}|,
+        )
+      end
     end
   end
 

spec/rails_helper.rb

@@ -154,6 +154,7 @@ module TestSetup
     WordWatcher.disable_cache
 
     SiteSetting.provider.all.each { |setting| SiteSetting.remove_override!(setting.name) }
+    SiteSetting.refresh!(refresh_site_settings: false, refresh_theme_site_settings: true)
 
     # very expensive IO operations
     SiteSetting.automatically_download_gravatars = false
@@ -332,8 +333,8 @@ RSpec.configure do |config|
     Discourse::Application.load_tasks
 
     ThemeField.delete_all
+    JavascriptCache.delete_all
     ThemeSiteSetting.delete_all
-    Theme.expire_site_setting_cache!
     SiteSetting.refresh!
 
     # Rebase defaults