Merge branch '45175-cache-json-instead-of-activerecord-objects-in-appearance-and-applicationsetting' into 'master'

Resolve "Cache JSON instead of ActiveRecord objects in `Appearance` and `ApplicationSetting`"

Closes #45175

See merge request gitlab-org/gitlab-ce!18754

5 files changed, 61 insertions, 47 deletions

diff --git a/app/controllers/admin/application_settings_controller.rb b/app/controllers/admin/application_settings_controller.rb
index 8958eab0423..cdfe3d6ab1e 100644
--- a/app/controllers/admin/application_settings_controller.rb
+++ b/app/controllers/admin/application_settings_controller.rb
@@ -52,7 +52,7 @@ class Admin::ApplicationSettingsController < Admin::ApplicationController
   private
 
   def set_application_setting
-    @application_setting = ApplicationSetting.current
+    @application_setting = ApplicationSetting.current_without_cache
   end
 
   def application_setting_params
diff --git a/app/models/appearance.rb b/app/models/appearance.rb
index f8713138a93..67cc84a9140 100644
--- a/app/models/appearance.rb
+++ b/app/models/appearance.rb
@@ -1,6 +1,6 @@
 class Appearance < ActiveRecord::Base
+  include CacheableAttributes
   include CacheMarkdownField
-  include AfterCommitQueue
   include ObjectStorage::BackgroundMove
   include WithUploads
 
@@ -15,16 +15,9 @@ class Appearance < ActiveRecord::Base
   mount_uploader :logo,         AttachmentUploader
   mount_uploader :header_logo,  AttachmentUploader
 
-  CACHE_KEY = "current_appearance:#{Gitlab::VERSION}".freeze
-
-  after_commit :flush_redis_cache
-
-  def self.current
-    Rails.cache.fetch(CACHE_KEY) { first }
-  end
-
-  def flush_redis_cache
-    Rails.cache.delete(CACHE_KEY)
+  # Overrides CacheableAttributes.current_without_cache
+  def self.current_without_cache
+    first
   end
 
   def single_appearance_row
diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb
index 451e512aef7..e8ccb320fae 100644
--- a/app/models/application_setting.rb
+++ b/app/models/application_setting.rb
@@ -1,11 +1,11 @@
 class ApplicationSetting < ActiveRecord::Base
+  include CacheableAttributes
   include CacheMarkdownField
   include TokenAuthenticatable
 
   add_authentication_token_field :runners_registration_token
   add_authentication_token_field :health_check_access_token
 
-  CACHE_KEY = 'application_setting.last'.freeze
   DOMAIN_LIST_SEPARATOR = %r{\s*[,;]\s*     # comma or semicolon, optionally surrounded by whitespace
                             |               # or
                             \s              # any whitespace character
@@ -229,40 +229,6 @@ class ApplicationSetting < ActiveRecord::Base
 
   after_commit do
     reset_memoized_terms
-    Rails.cache.write(CACHE_KEY, self)
-  end
-
-  def self.current
-    ensure_cache_setup
-
-    Rails.cache.fetch(CACHE_KEY) do
-      ApplicationSetting.last.tap do |settings|
-        # do not cache nils
-        raise 'missing settings' unless settings
-      end
-    end
-  rescue
-    # Fall back to an uncached value if there are any problems (e.g. redis down)
-    ApplicationSetting.last
-  end
-
-  def self.expire
-    Rails.cache.delete(CACHE_KEY)
-  rescue
-    # Gracefully handle when Redis is not available. For example,
-    # omnibus may fail here during gitlab:assets:compile.
-  end
-
-  def self.cached
-    value = Rails.cache.read(CACHE_KEY)
-    ensure_cache_setup if value.present?
-    value
-  end
-
-  def self.ensure_cache_setup
-    # This is a workaround for a Rails bug that causes attribute methods not
-    # to be loaded when read from cache: https://github.com/rails/rails/issues/27348
-    ApplicationSetting.define_attribute_methods
   end
 
   def self.defaults
diff --git a/app/models/concerns/cacheable_attributes.rb b/app/models/concerns/cacheable_attributes.rb
new file mode 100644
index 00000000000..b32459fdabf
--- /dev/null
+++ b/app/models/concerns/cacheable_attributes.rb
@@ -0,0 +1,54 @@
+module CacheableAttributes
+  extend ActiveSupport::Concern
+
+  included do
+    after_commit { self.class.expire }
+  end
+
+  class_methods do
+    # Can be overriden
+    def current_without_cache
+      last
+    end
+
+    def cache_key
+      "#{name}:#{Gitlab::VERSION}:#{Gitlab.migrations_hash}:json".freeze
+    end
+
+    def defaults
+      {}
+    end
+
+    def build_from_defaults(attributes = {})
+      new(defaults.merge(attributes))
+    end
+
+    def cached
+      json_attributes = Rails.cache.read(cache_key)
+      return nil unless json_attributes.present?
+
+      build_from_defaults(JSON.parse(json_attributes))
+    end
+
+    def current
+      cached_record = cached
+      return cached_record if cached_record.present?
+
+      current_without_cache.tap { |current_record| current_record&.cache! }
+    rescue
+      # Fall back to an uncached value if there are any problems (e.g. Redis down)
+      current_without_cache
+    end
+
+    def expire
+      Rails.cache.delete(cache_key)
+    rescue
+      # Gracefully handle when Redis is not available. For example,
+      # omnibus may fail here during gitlab:assets:compile.
+    end
+  end
+
+  def cache!
+    Rails.cache.write(self.class.cache_key, attributes.to_json)
+  end
+end
diff --git a/app/uploaders/object_storage.rb b/app/uploaders/object_storage.rb
index a3549cada95..f2a8afccdeb 100644
--- a/app/uploaders/object_storage.rb
+++ b/app/uploaders/object_storage.rb
@@ -103,6 +103,7 @@ module ObjectStorage
     end
 
     included do
+      include AfterCommitQueue
       after_save on: [:create, :update] do
         background_upload(changed_mounts)
       end