Merge branch '8765-geo-gitlab-geo-should-not-serialize-activerecord-objects' into 'master'

Geo - Gitlab::Geo should not serialize ActiveRecord objects

Closes #8827

See merge request gitlab-org/gitlab-ee!8802

ee/lib/gitlab/geo.rb

@@ -15,15 +15,15 @@ module Gitlab
     ).freeze
 
     def self.current_node
-      self.cache_value(:current_node) { GeoNode.current_node }
+      self.cache_value(:current_node, as: GeoNode) { GeoNode.current_node }
     end
 
     def self.primary_node
-      self.cache_value(:primary_node) { GeoNode.primary_node }
+      self.cache_value(:primary_node, as: GeoNode) { GeoNode.primary_node }
     end
 
     def self.secondary_nodes
-      self.cache_value(:secondary_nodes) { GeoNode.secondary_nodes }
+      self.cache_value(:secondary_nodes, as: GeoNode) { GeoNode.secondary_nodes }
     end
 
     def self.connected?
@@ -31,7 +31,7 @@ module Gitlab
     end
 
     def self.enabled?
-      cache_value(:node_enabled) { GeoNode.exists? }
+      self.cache_value(:node_enabled) { GeoNode.exists? }
     end
 
     def self.primary?
@@ -79,29 +79,29 @@ module Gitlab
       end
     end
 
-    def self.cache_key_for(key)
-      "geo:#{key}:#{Rails.version}"
+    def self.cache
+      @cache ||= Gitlab::JsonCache.new(namespace: :geo)
     end
 
-    def self.cache_value(raw_key, &block)
-      return yield unless Gitlab::SafeRequestStore.active?
+    def self.request_store_cache
+      @request_store_cache ||= Gitlab::JsonCache.new(namespace: :geo, backend: Gitlab::SafeRequestStore)
+    end
 
-      key = cache_key_for(raw_key)
+    def self.cache_value(key, as: nil, &block)
+      return yield unless request_store_cache.active?
 
-      Gitlab::SafeRequestStore.fetch(key) do
+      request_store_cache.fetch(key, as: as) do
         # We need a short expire time as we can't manually expire on a secondary node
-        Rails.cache.fetch(key, expires_in: 15.seconds) { yield }
+        cache.fetch(key, as: as, expires_in: 15.seconds) { yield }
       end
     end
 
     def self.expire_cache!
-      return true unless Gitlab::SafeRequestStore.active?
-
-      CACHE_KEYS.each do |raw_key|
-        key = cache_key_for(raw_key)
+      return true unless request_store_cache.active?
 
-        Rails.cache.delete(key)
-        Gitlab::SafeRequestStore.delete(key)
+      CACHE_KEYS.each do |key|
+        cache.expire(key)
+        request_store_cache.expire(key)
       end
 
       true

ee/lib/gitlab/json_cache.rb

+# frozen_string_literal: true
+
+module Gitlab
+  class JsonCache
+    attr_reader :backend, :cache_key_with_version, :namespace
+
+    def initialize(options = {})
+      @backend = options.fetch(:backend, Rails.cache)
+      @namespace = options.fetch(:namespace, nil)
+      @cache_key_with_version = options.fetch(:cache_key_with_version, true)
+    end
+
+    def active?
+      if backend.respond_to?(:active?)
+        backend.active?
+      else
+        true
+      end
+    end
+
+    def cache_key(key)
+      expanded_cache_key = [namespace, key].compact
+
+      if cache_key_with_version
+        expanded_cache_key << Rails.version
+      end
+
+      expanded_cache_key.join(':')
+    end
+
+    def expire(key)
+      backend.delete(cache_key(key))
+    end
+
+    def read(key, klass = nil)
+      value = backend.read(cache_key(key))
+      value = parse_value(value, klass) if value
+      value
+    end
+
+    def write(key, value, options = nil)
+      backend.write(cache_key(key), value.to_json, options)
+    end
+
+    def fetch(key, options = {}, &block)
+      klass = options.delete(:as)
+      value = read(key, klass)
+
+      return value unless value.nil?
+
+      value = yield
+
+      write(key, value, options)
+
+      value
+    end
+
+    private
+
+    def parse_value(raw, klass)
+      value = ActiveSupport::JSON.decode(raw)
+
+      case value
+      when Hash then parse_entry(value, klass)
+      when Array then parse_entries(value, klass)
+      else
+        value
+      end
+    rescue ActiveSupport::JSON.parse_error
+      nil
+    end
+
+    def parse_entry(raw, klass)
+      klass.new(raw) if valid_entry?(raw, klass)
+    end
+
+    def valid_entry?(raw, klass)
+      return false unless klass && raw.is_a?(Hash)
+
+      (raw.keys - klass.attribute_names).empty?
+    end
+
+    def parse_entries(values, klass)
+      values.map { |value| parse_entry(value, klass) }.compact
+    end
+  end
+end

ee/spec/lib/gitlab/geo_spec.rb

 require 'spec_helper'
 
-describe Gitlab::Geo, :geo do
+describe Gitlab::Geo, :geo, :request_store do
   include ::EE::GeoHelpers
 
   set(:primary_node)   { create(:geo_node, :primary) }
   set(:secondary_node) { create(:geo_node) }
 
   shared_examples 'a Geo cached value' do |method, key|
-    it 'includes Rails.version in the cache key', :request_store do
-      expect(Rails.cache).to receive(:fetch)
-        .with("geo:#{key}:#{Rails.version}", expires_in: 15.seconds)
+    it 'includes Rails.version in the cache key' do
+      expect(Rails.cache).to receive(:write)
+        .with("geo:#{key}:#{Rails.version}", an_instance_of(String), expires_in: 15.seconds)
 
       described_class.public_send(method)
     end
@@ -32,6 +32,10 @@ describe Gitlab::Geo, :geo do
   end
 
   describe '.secondary_nodes' do
+    it 'returns a list of Geo secondary nodes' do
+      expect(described_class.secondary_nodes).to match_array(secondary_node)
+    end
+
     it_behaves_like 'a Geo cached value', :secondary_nodes, :secondary_nodes
   end
 
@@ -114,6 +118,7 @@ describe Gitlab::Geo, :geo do
   describe '.oauth_authentication' do
     before do
       stub_secondary_node
+      stub_current_geo_node(secondary_node)
     end
 
     it_behaves_like 'a Geo cached value', :oauth_authentication, :oauth_application