Merge branch 'json-cache-key-with-gitlab-revision' into 'master'

Update Gitlab::JsonCache cache_key to include Gitlab.revision by default

See merge request gitlab-org/gitlab!81413

app/models/broadcast_message.rb

@@ -53,7 +53,7 @@ class BroadcastMessage < ApplicationRecord
 
     def cache
       ::Gitlab::SafeRequestStore.fetch(:broadcast_message_json_cache) do
-        Gitlab::JsonCache.new(cache_key_with_version: false)
+        Gitlab::JsonCache.new
       end
     end
 

ee/app/models/license.rb

@@ -315,7 +315,7 @@ class License < ApplicationRecord
 
     def cache
       Gitlab::SafeRequestStore[:license_cache] ||=
-        Gitlab::JsonCache.new(namespace: :ee, backend: ::Gitlab::ProcessMemoryCache.cache_backend)
+        Gitlab::JsonCache.new(namespace: :ee, backend: ::Gitlab::ProcessMemoryCache.cache_backend, cache_key_strategy: :version)
     end
 
     def all_plans

ee/lib/gitlab/geo.rb

@@ -115,11 +115,11 @@ module Gitlab
 
     def self.l1_cache
       SafeRequestStore[:geo_l1_cache] ||=
-        Gitlab::JsonCache.new(namespace: :geo, backend: ::Gitlab::ProcessMemoryCache.cache_backend)
+        Gitlab::JsonCache.new(namespace: :geo, backend: ::Gitlab::ProcessMemoryCache.cache_backend, cache_key_strategy: :version)
     end
 
     def self.l2_cache
-      SafeRequestStore[:geo_l2_cache] ||= Gitlab::JsonCache.new(namespace: :geo)
+      SafeRequestStore[:geo_l2_cache] ||= Gitlab::JsonCache.new(namespace: :geo, cache_key_strategy: :version)
     end
 
     def self.cache_value(raw_key, as: nil, &block)

lib/gitlab/json_cache.rb

@@ -2,12 +2,17 @@
 
 module Gitlab
   class JsonCache
-    attr_reader :backend, :cache_key_with_version, :namespace
+    attr_reader :backend, :namespace
+
+    STRATEGY_KEY_COMPONENTS = {
+      revision: Gitlab.revision,
+      version: [Gitlab::VERSION, Rails.version]
+    }.freeze
 
     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)
+      @cache_key_strategy = options.fetch(:cache_key_strategy, :revision)
     end
 
     def active?
@@ -19,13 +24,12 @@ module Gitlab
     end
 
     def cache_key(key)
-      expanded_cache_key = [namespace, key].compact
-
-      if cache_key_with_version
-        expanded_cache_key << [Gitlab::VERSION, Rails.version]
-      end
+      expanded_cache_key = [namespace, key, *strategy_key_component].compact
+      expanded_cache_key.join(':').freeze
+    end
 
-      expanded_cache_key.flatten.join(':').freeze
+    def strategy_key_component
+      STRATEGY_KEY_COMPONENTS.fetch(@cache_key_strategy)
     end
 
     def expire(key)

spec/lib/gitlab/json_cache_spec.rb

@@ -8,7 +8,7 @@ RSpec.describe Gitlab::JsonCache do
   let(:backend) { double('backend').as_null_object }
   let(:namespace) { 'geo' }
   let(:key) { 'foo' }
-  let(:expanded_key) { "#{namespace}:#{key}:#{Gitlab::VERSION}:#{Rails.version}" }
+  let(:expanded_key) { "#{namespace}:#{key}:#{Gitlab.revision}" }
 
   subject(:cache) { described_class.new(namespace: namespace, backend: backend) }
 
@@ -35,69 +35,63 @@ RSpec.describe Gitlab::JsonCache do
   end
 
   describe '#cache_key' do
-    context 'when namespace is not defined' do
-      context 'when cache_key_with_version is true' do
-        it 'expands out the key with GitLab, and Rails versions' do
-          cache = described_class.new(cache_key_with_version: true)
+    using RSpec::Parameterized::TableSyntax
 
-          cache_key = cache.cache_key(key)
-
-          expect(cache_key).to eq("#{key}:#{Gitlab::VERSION}:#{Rails.version}")
-        end
-      end
+    where(:namespace, :cache_key_strategy, :expanded_key) do
+      nil       | :revision | "#{key}:#{Gitlab.revision}"
+      nil       | :version  | "#{key}:#{Gitlab::VERSION}:#{Rails.version}"
+      namespace | :revision | "#{namespace}:#{key}:#{Gitlab.revision}"
+      namespace | :version  | "#{namespace}:#{key}:#{Gitlab::VERSION}:#{Rails.version}"
+    end
 
-      context 'when cache_key_with_version is false' do
-        it 'returns the key' do
-          cache = described_class.new(namespace: nil, cache_key_with_version: false)
+    with_them do
+      let(:cache) { described_class.new(namespace: namespace, cache_key_strategy: cache_key_strategy) }
 
-          cache_key = cache.cache_key(key)
+      subject { cache.cache_key(key) }
 
-          expect(cache_key).to eq(key)
-        end
-      end
+      it { is_expected.to eq expanded_key }
     end
 
-    context 'when namespace is nil' do
-      context 'when cache_key_with_version is true' do
-        it 'expands out the key with GitLab, and Rails versions' do
-          cache = described_class.new(cache_key_with_version: true)
-
-          cache_key = cache.cache_key(key)
+    context 'when cache_key_strategy is unknown' do
+      let(:cache) { described_class.new(namespace: namespace, cache_key_strategy: 'unknown') }
 
-          expect(cache_key).to eq("#{key}:#{Gitlab::VERSION}:#{Rails.version}")
-        end
+      it 'raises KeyError' do
+        expect { cache.cache_key('key') }.to raise_error(KeyError)
       end
+    end
+  end
 
-      context 'when cache_key_with_version is false' do
-        it 'returns the key' do
-          cache = described_class.new(namespace: nil, cache_key_with_version: false)
+  describe '#namespace' do
+    it 'defaults to nil' do
+      cache = described_class.new
+      expect(cache.namespace).to be_nil
+    end
+  end
 
-          cache_key = cache.cache_key(key)
+  describe '#strategy_key_component' do
+    subject { cache.strategy_key_component }
 
-          expect(cache_key).to eq(key)
-        end
-      end
+    it 'defaults to Gitlab.revision' do
+      expect(described_class.new.strategy_key_component).to eq Gitlab.revision
     end
 
-    context 'when namespace is set' do
-      context 'when cache_key_with_version is true' do
-        it 'expands out the key with namespace and Rails version' do
-          cache = described_class.new(namespace: namespace, cache_key_with_version: true)
+    context 'when cache_key_strategy is :revision' do
+      let(:cache) { described_class.new(cache_key_strategy: :revision) }
 
-          cache_key = cache.cache_key(key)
+      it { is_expected.to eq Gitlab.revision }
+    end
 
-          expect(cache_key).to eq("#{namespace}:#{key}:#{Gitlab::VERSION}:#{Rails.version}")
-        end
-      end
+    context 'when cache_key_strategy is :version' do
+      let(:cache) { described_class.new(cache_key_strategy: :version) }
 
-      context 'when cache_key_with_version is false' do
-        it 'expands out the key with namespace' do
-          cache = described_class.new(namespace: namespace, cache_key_with_version: false)
+      it { is_expected.to eq [Gitlab::VERSION, Rails.version] }
+    end
 
-          cache_key = cache.cache_key(key)
+    context 'when cache_key_strategy is invalid' do
+      let(:cache) { described_class.new(cache_key_strategy: 'unknown') }
 
-          expect(cache_key).to eq("#{namespace}:#{key}")
-        end
+      it 'raises KeyError' do
+        expect { subject }.to raise_error(KeyError)
       end
     end
   end

spec/models/broadcast_message_spec.rb

@@ -286,9 +286,9 @@ RSpec.describe BroadcastMessage do
     it 'flushes the Redis cache' do
       message = create(:broadcast_message)
 
-      expect(Rails.cache).to receive(:delete).with(described_class::CACHE_KEY)
-      expect(Rails.cache).to receive(:delete).with(described_class::BANNER_CACHE_KEY)
-      expect(Rails.cache).to receive(:delete).with(described_class::NOTIFICATION_CACHE_KEY)
+      expect(Rails.cache).to receive(:delete).with("#{described_class::CACHE_KEY}:#{Gitlab.revision}")
+      expect(Rails.cache).to receive(:delete).with("#{described_class::BANNER_CACHE_KEY}:#{Gitlab.revision}")
+      expect(Rails.cache).to receive(:delete).with("#{described_class::NOTIFICATION_CACHE_KEY}:#{Gitlab.revision}")
 
       message.flush_redis_cache
     end