From a886532cc04bfa7ec6885ea883889f6d138961bd Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?R=C3=A9my=20Coutable?= <remy@rymai.me>
Date: Tue, 29 May 2018 17:49:52 +0200
Subject: Revert to caching the AR object in CacheableAttributes
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Caching the attributes as JSON and manually instantiating the record
ended-up very complex since the edge-cases such as upload fields,
serialized fields, and fields with custom accessors had to be handled.

To ensure 3 points out of 4 are checked from https://gitlab.com/gitlab-org/gitlab-ce/issues/45175
we now include the Rails version in the cache key.

Signed-off-by: Rémy Coutable <remy@rymai.me>
---
 app/models/concerns/cacheable_attributes.rb | 34 +++++++++++++++++++++--------
 1 file changed, 25 insertions(+), 9 deletions(-)

(limited to 'app/models')

diff --git a/app/models/concerns/cacheable_attributes.rb b/app/models/concerns/cacheable_attributes.rb
index b32459fdabf..dd4fccc811d 100644
--- a/app/models/concerns/cacheable_attributes.rb
+++ b/app/models/concerns/cacheable_attributes.rb
@@ -6,15 +6,16 @@ module CacheableAttributes
   end
 
   class_methods do
+    def cache_key
+      "#{name}:#{Gitlab::VERSION}:#{Gitlab.migrations_hash}:#{Rails.version}".freeze
+    end
+
     # Can be overriden
     def current_without_cache
       last
     end
 
-    def cache_key
-      "#{name}:#{Gitlab::VERSION}:#{Gitlab.migrations_hash}:json".freeze
-    end
-
+    # Can be overriden
     def defaults
       {}
     end
@@ -24,10 +25,14 @@ module CacheableAttributes
     end
 
     def cached
-      json_attributes = Rails.cache.read(cache_key)
-      return nil unless json_attributes.present?
+      retrieve_from_cache
+    end
+
+    def retrieve_from_cache
+      record = Rails.cache.read(cache_key)
+      ensure_cache_setup if record.present?
 
-      build_from_defaults(JSON.parse(json_attributes))
+      record
     end
 
     def current
@@ -35,7 +40,12 @@ module CacheableAttributes
       return cached_record if cached_record.present?
 
       current_without_cache.tap { |current_record| current_record&.cache! }
-    rescue
+    rescue => e
+      if Rails.env.production?
+        Rails.logger.warn("Cached record for #{name} couldn't be loaded, falling back to uncached record: #{e}")
+      else
+        raise e
+      end
       # Fall back to an uncached value if there are any problems (e.g. Redis down)
       current_without_cache
     end
@@ -46,9 +56,15 @@ module CacheableAttributes
       # Gracefully handle when Redis is not available. For example,
       # omnibus may fail here during gitlab:assets:compile.
     end
+
+    def 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
+      define_attribute_methods
+    end
   end
 
   def cache!
-    Rails.cache.write(self.class.cache_key, attributes.to_json)
+    Rails.cache.write(self.class.cache_key, self)
   end
 end
-- 
cgit v1.2.3


From 4eda09e3fbfe82fd1467e97cbc5bd085b91f257d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?R=C3=A9my=20Coutable?= <remy@rymai.me>
Date: Tue, 29 May 2018 18:39:03 +0200
Subject: Use RequestStore in CacheableAttributes.cached for greater
 performance
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Signed-off-by: Rémy Coutable <remy@rymai.me>
---
 app/models/concerns/cacheable_attributes.rb | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

(limited to 'app/models')

diff --git a/app/models/concerns/cacheable_attributes.rb b/app/models/concerns/cacheable_attributes.rb
index dd4fccc811d..d58d7165969 100644
--- a/app/models/concerns/cacheable_attributes.rb
+++ b/app/models/concerns/cacheable_attributes.rb
@@ -25,7 +25,11 @@ module CacheableAttributes
     end
 
     def cached
-      retrieve_from_cache
+      if RequestStore.active?
+        RequestStore[:"#{name}_cached_attributes"] ||= retrieve_from_cache
+      else
+        retrieve_from_cache
+      end
     end
 
     def retrieve_from_cache
-- 
cgit v1.2.3