Extract Gitlab::Geo::Cache from Gitlab::Geo

We should avoid putting binary ActiveRecord objects in the cache.
This changes introduces a new cache Gitlab::Geo::Cache that writes
a string containing a JSON representation of the objects to the
cache, and parses the cached value back when requested.

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, klass: GeoNode) { GeoNode.current_node }
     end
 
     def self.primary_node
-      self.cache_value(:primary_node) { GeoNode.primary_node }
+      self.cache_value(:primary_node, klass: GeoNode) { GeoNode.primary_node }
     end
 
     def self.secondary_nodes
-      self.cache_value(:secondary_nodes) { GeoNode.secondary_nodes }
+      self.cache_value(:secondary_nodes, klass: 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::Geo::Cache.new
     end
 
-    def self.cache_value(raw_key, &block)
-      return yield unless Gitlab::SafeRequestStore.active?
+    def self.request_store_cache
+      @request_store_cache ||= Gitlab::Geo::Cache.new(backend: Gitlab::SafeRequestStore)
+    end
 
-      key = cache_key_for(raw_key)
+    def self.cache_value(key, klass: nil, &block)
+      return yield unless request_store_cache.active?
 
-      Gitlab::SafeRequestStore.fetch(key) do
+      request_store_cache.fetch(key, klass: klass) 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, klass: klass, 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/geo/cache.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Geo
+    class Cache
+      attr_reader :namespace, :backend
+
+      def initialize(backend: Rails.cache)
+        @backend = backend
+        @namespace = :geo
+      end
+
+      def active?
+        if backend.respond_to?(:active?)
+          backend.active?
+        else
+          true
+        end
+      end
+
+      def cache_key(key)
+        "#{namespace}:#{key}:#{Rails.version}"
+      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 = {})
+        backend.write(cache_key(key), *[value.to_json, options].reject(&:blank?))
+      end
+
+      def fetch(key, options = {}, &block)
+        klass = options.delete(:klass)
+        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
+end

ee/spec/lib/gitlab/geo/cache_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::Geo::Cache do
+  let(:backend) { double('backend').as_null_object }
+  let(:key) { 'foo' }
+  let(:expanded_key) { "geo:#{key}:#{Rails.version}" }
+  let(:node) { create(:geo_node) }
+
+  subject(:cache) { described_class.new(backend: backend) }
+
+  describe '#active?' do
+    context 'when backend respond to active? method' do
+      it 'delegates to the underlying cache implementation' do
+        backend = double('backend', active?: false)
+
+        cache = described_class.new(backend: backend)
+
+        expect(cache.active?).to eq(false)
+      end
+    end
+
+    context 'when backend does not respond to active? method' do
+      it 'returns true' do
+        backend = double('backend')
+
+        cache = described_class.new(backend: backend)
+
+        expect(cache.active?).to eq(true)
+      end
+    end
+  end
+
+  describe '#cache_key' do
+    it 'expands out the key with namespace and Rails version' do
+      cache_key = cache.cache_key(key)
+
+      expect(cache_key).to eq(expanded_key)
+    end
+  end
+
+  describe '#expire' do
+    it 'expires the given key from the cache' do
+      cache.expire(key)
+
+      expect(backend).to have_received(:delete).with(expanded_key)
+    end
+  end
+
+  describe '#read' do
+    it 'reads the given key from the cache' do
+      cache.read(key)
+
+      expect(backend).to have_received(:read).with(expanded_key)
+    end
+
+    it 'returns the cached value when there is data in the cache with the given key' do
+      allow(backend).to receive(:read)
+        .with(expanded_key)
+        .and_return("true")
+
+      expect(cache.read(key)).to eq(true)
+    end
+
+    it 'returns nil when there is no data in the cache with the given key' do
+      allow(backend).to receive(:read)
+        .with(expanded_key)
+        .and_return(nil)
+
+      expect(cache.read(key)).to be_nil
+    end
+
+    context 'when the cached value is a hash' do
+      it 'parses the cached value' do
+        allow(backend).to receive(:read)
+          .with(expanded_key)
+          .and_return(node.to_json)
+
+        expect(cache.read(key, GeoNode)).to eq(node)
+      end
+
+      it 'returns nil when klass is nil' do
+        allow(backend).to receive(:read)
+          .with(expanded_key)
+          .and_return(node.to_json)
+
+        expect(cache.read(key)).to be_nil
+      end
+
+      it 'gracefully handles bad cached entry' do
+        allow(backend).to receive(:read)
+          .with(expanded_key)
+          .and_return('{')
+
+        expect(cache.read(key, GeoNode)).to be_nil
+      end
+
+      it 'gracefully handles an empty hash' do
+        allow(backend).to receive(:read)
+          .with(expanded_key)
+          .and_return('{}')
+
+        expect(cache.read(key, GeoNode)).to be_a(GeoNode)
+      end
+
+      it 'gracefully handles unknown attributes' do
+        allow(backend).to receive(:read)
+          .with(expanded_key)
+          .and_return(node.attributes.merge(unknown_attribute: 1).to_json)
+
+        expect(cache.read(key, GeoNode)).to be_nil
+      end
+    end
+
+    context 'when the cached value is an array' do
+      it 'parses the cached value' do
+        allow(backend).to receive(:read)
+          .with(expanded_key)
+          .and_return([node].to_json)
+
+        expect(cache.read(key, GeoNode)).to eq([node])
+      end
+
+      it 'returns an empty array when klass is nil' do
+        allow(backend).to receive(:read)
+          .with(expanded_key)
+          .and_return([node].to_json)
+
+        expect(cache.read(key)).to eq([])
+      end
+
+      it 'gracefully handles bad cached entry' do
+        allow(backend).to receive(:read)
+          .with(expanded_key)
+          .and_return('[')
+
+        expect(cache.read(key, GeoNode)).to be_nil
+      end
+
+      it 'gracefully handles an empty array' do
+        allow(backend).to receive(:read)
+          .with(expanded_key)
+          .and_return('[]')
+
+        expect(cache.read(key, GeoNode)).to eq([])
+      end
+
+      it 'gracefully handles unknown attributes' do
+        allow(backend).to receive(:read)
+          .with(expanded_key)
+          .and_return([{ unknown_attribute: 1 }, node.attributes].to_json)
+
+        expect(cache.read(key, GeoNode)).to eq([node])
+      end
+    end
+  end
+
+  describe '#write' do
+    it 'writes value to the cache with the given key' do
+      cache.write(key, true)
+
+      expect(backend).to have_received(:write).with(expanded_key, "true")
+    end
+
+    it 'writes a string containing a JSON representation of the value to the cache' do
+      cache.write(key, node)
+
+      expect(backend).to have_received(:write)
+        .with(expanded_key, node.to_json)
+    end
+
+    it 'passes options the underlying cache implementation' do
+      cache.write(key, true, expires_in: 15.seconds)
+
+      expect(backend).to have_received(:write)
+        .with(expanded_key, "true", expires_in: 15.seconds)
+    end
+
+    it 'does not pass options to the underlying cache implementation when options is empty' do
+      cache.write(key, true, {})
+
+      expect(backend).to have_received(:write)
+        .with(expanded_key, "true")
+    end
+
+    it 'does not pass options to the underlying cache implementation when options is nil' do
+      cache.write(key, true, nil)
+
+      expect(backend).to have_received(:write)
+        .with(expanded_key, "true")
+    end
+  end
+
+  describe '#fetch', :use_clean_rails_memory_store_caching do
+    let(:backend) { Rails.cache }
+
+    it 'requires a block' do
+      expect { cache.fetch(key) }.to raise_error(LocalJumpError)
+    end
+
+    context 'when the given key does not exist in the cache' do
+      context 'when the result of the block is truthy' do
+        it 'returns the result of the block' do
+          result = cache.fetch(key) { true }
+
+          expect(result).to eq(true)
+        end
+
+        it 'caches the value' do
+          expect(backend).to receive(:write).with(expanded_key, "true")
+
+          cache.fetch(key) { true }
+        end
+      end
+
+      context 'when the result of the block is false' do
+        it 'returns the result of the block' do
+          result = cache.fetch(key) { false }
+
+          expect(result).to eq(false)
+        end
+
+        it 'caches the value' do
+          expect(backend).to receive(:write).with(expanded_key, "false")
+
+          cache.fetch(key) { false }
+        end
+      end
+
+      context 'when the result of the block is nil' do
+        it 'returns the result of the block' do
+          result = cache.fetch(key) { nil }
+
+          expect(result).to eq(nil)
+        end
+
+        it 'caches the value' do
+          expect(backend).to receive(:write).with(expanded_key, "null")
+
+          cache.fetch(key) { nil }
+        end
+      end
+    end
+
+    context 'whenn the given key exists in the cache' do
+      context 'when the cached value is a hash' do
+        before do
+          backend.write(expanded_key, node.to_json)
+        end
+
+        it 'parses the cached value' do
+          result = cache.fetch(key, klass: GeoNode) { 'block result' }
+
+          expect(result).to eq(node)
+        end
+
+        it 'returns the result of the block when klass is nil' do
+          result = cache.fetch(key, klass: nil) { 'block result' }
+
+          expect(result).to eq('block result')
+        end
+
+        it 'returns the result of the block when klass is not informed' do
+          result = cache.fetch(key) { 'block result' }
+
+          expect(result).to eq('block result')
+        end
+      end
+
+      context 'when the cached value is a array' do
+        before do
+          backend.write(expanded_key, [node].to_json)
+        end
+
+        it 'parses the cached value' do
+          result = cache.fetch(key, klass: GeoNode) { 'block result' }
+
+          expect(result).to eq([node])
+        end
+
+        it 'returns an empty array when klass is nil' do
+          result = cache.fetch(key, klass: nil) { 'block result' }
+
+          expect(result).to eq([])
+        end
+
+        it 'returns an empty array when klass is not informed' do
+          result = cache.fetch(key) { 'block result' }
+
+          expect(result).to eq([])
+        end
+      end
+
+      context 'when the cached value is true' do
+        before do
+          backend.write(expanded_key, "true")
+        end
+
+        it 'returns the cached value' do
+          result = cache.fetch(key) { 'block result' }
+
+          expect(result).to eq(true)
+        end
+
+        it 'does not execute the block' do
+          expect { |block| cache.fetch(key, &block) }.not_to yield_control
+        end
+
+        it 'does not write to the cache' do
+          expect(backend).not_to receive(:write)
+
+          cache.fetch(key) { 'block result' }
+        end
+      end
+
+      context 'when the cached value is false' do
+        before do
+          backend.write(expanded_key, "false")
+        end
+
+        it 'returns the cached value' do
+          result = cache.fetch(key) { 'block result' }
+
+          expect(result).to eq(false)
+        end
+
+        it 'does not execute the block' do
+          expect { |block| cache.fetch(key, &block) }.not_to yield_control
+        end
+
+        it 'does not write to the cache' do
+          expect(backend).not_to receive(:write)
+
+          cache.fetch(key) { 'block result' }
+        end
+      end
+
+      context 'when the cached value is nil' do
+        before do
+          backend.write(expanded_key, "null")
+        end
+
+        it 'returns the result of the block' do
+          result = cache.fetch(key) { 'block result' }
+
+          expect(result).to eq('block result')
+        end
+
+        it 'writes the result of the block to the cache' do
+          expect(backend).to receive(:write)
+            .with(expanded_key, 'block result'.to_json)
+
+          cache.fetch(key) { 'block result' }
+        end
+      end
+    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