Use RequestStoreWrap for Commit#author

We also try to use instance variable to cache the result if
RequestStore is not available, so we could keep the same logic,
using the same cache key. Also introduce a way to specify method
specific cache key

app/models/commit.rb

 class Commit
   extend ActiveModel::Naming
+  extend Gitlab::Cache::RequestStoreWrap
 
   include ActiveModel::Conversion
   include Noteable
@@ -169,19 +170,9 @@ class Commit
   end
 
   def author
-    if RequestStore.active?
-      key = "commit_author:#{author_email.downcase}"
-      # nil is a valid value since no author may exist in the system
-      if RequestStore.store.key?(key)
-        @author = RequestStore.store[key]
-      else
-        @author = find_author_by_any_email
-        RequestStore.store[key] = @author
-      end
-    else
-      @author ||= find_author_by_any_email
-    end
+    User.find_by_any_email(author_email.downcase)
   end
+  request_store_wrap(:author) { author_email.downcase }
 
   def committer
     @committer ||= User.find_by_any_email(committer_email.downcase)
@@ -368,10 +359,6 @@ class Commit
     end
   end
 
-  def find_author_by_any_email
-    User.find_by_any_email(author_email.downcase)
-  end
-
   def repo_changes
     changes = { added: [], modified: [], removed: [] }
 

lib/gitlab/cache/request_store_wrap.rb

@@ -37,23 +37,45 @@ module Gitlab
         end
       end
 
-      def request_store_wrap(method_name)
-        const_get(:RequestStoreWrapExtension)
-          .send(:define_method, method_name) do |*args|
-            return super(*args) unless RequestStore.active?
+      def request_store_wrap(method_name, &method_key_block)
+        const_get(:RequestStoreWrapExtension).module_eval do
+          define_method(method_name) do |*args|
+            store =
+              if RequestStore.active?
+                RequestStore.store
+              else
+                ivar_name = # ! and ? cannot be used as ivar name
+                  "@#{method_name.to_s.tr('!', "\u2605").tr('?', "\u2606")}"
 
-            klass = self.class
-            key = [klass.name,
-                   method_name,
-                   *instance_exec(&klass.request_store_wrap_key),
-                   *args].join(':')
+                instance_variable_get(ivar_name) ||
+                  instance_variable_set(ivar_name, {})
+              end
+
+            key = send("#{method_name}_cache_key", args)
 
-            if RequestStore.store.key?(key)
-              RequestStore.store[key]
+            if store.key?(key)
+              store[key]
             else
-              RequestStore.store[key] = super(*args)
+              store[key] = super(*args)
             end
           end
+
+          cache_key_method_name = "#{method_name}_cache_key"
+
+          define_method(cache_key_method_name) do |args|
+            klass = self.class
+
+            instance_key = instance_exec(&klass.request_store_wrap_key) if
+              klass.request_store_wrap_key
+
+            method_key = instance_exec(&method_key_block) if method_key_block
+
+            [klass.name, method_name, *instance_key, *method_key, *args]
+              .join(':')
+          end
+
+          private cache_key_method_name
+        end
       end
     end
   end

spec/lib/gitlab/cache/request_store_wrap_spec.rb

@@ -5,20 +5,17 @@ describe Gitlab::Cache::RequestStoreWrap, :request_store do
     Class.new do
       extend Gitlab::Cache::RequestStoreWrap
 
-      attr_accessor :id, :name, :result
+      attr_accessor :id, :name, :result, :extra
 
       def self.name
         'ExpensiveAlgorithm'
       end
 
-      def initialize(id, name, result)
+      def initialize(id, name, result, extra = nil)
         self.id = id
         self.name = name
         self.result = result
-      end
-
-      request_store_wrap_key do
-        [id, name]
+        self.extra = nil
       end
 
       request_store_wrap def compute(arg)
@@ -28,6 +25,11 @@ describe Gitlab::Cache::RequestStoreWrap, :request_store do
       request_store_wrap def repute(arg)
         result << arg
       end
+
+      def dispute(arg)
+        result << arg
+      end
+      request_store_wrap(:dispute) { extra }
     end
   end
 
@@ -50,51 +52,69 @@ describe Gitlab::Cache::RequestStoreWrap, :request_store do
       expect(algorithm.result).to eq(result)
     end
 
-    it 'computes twice for the different keys, id' do
+    it 'computes twice for the different class name' do
       algorithm.compute(true)
-      algorithm.id = 'ad'
+      allow(klass).to receive(:name).and_return('CheapAlgo')
       result = algorithm.compute(true)
 
       expect(result).to eq([true, true])
       expect(algorithm.result).to eq(result)
     end
 
-    it 'computes twice for the different keys, name' do
+    it 'computes twice for the different method' do
       algorithm.compute(true)
-      algorithm.name = 'same'
-      result = algorithm.compute(true)
+      result = algorithm.repute(true)
 
       expect(result).to eq([true, true])
       expect(algorithm.result).to eq(result)
     end
 
-    it 'computes twice for the different class name' do
+    it 'computes twice if RequestStore starts over' do
       algorithm.compute(true)
-      allow(klass).to receive(:name).and_return('CheapAlgo')
+      RequestStore.end!
+      RequestStore.clear!
+      RequestStore.begin!
       result = algorithm.compute(true)
 
       expect(result).to eq([true, true])
       expect(algorithm.result).to eq(result)
     end
 
-    it 'computes twice for the different method' do
+    context 'when request_store_wrap_key is provided' do
+      before do
+        klass.request_store_wrap_key do
+          [id, name]
+        end
+      end
+
+      it 'computes twice for the different keys, id' do
         algorithm.compute(true)
-      result = algorithm.repute(true)
+        algorithm.id = 'ad'
+        result = algorithm.compute(true)
 
         expect(result).to eq([true, true])
         expect(algorithm.result).to eq(result)
       end
 
-    it 'computes twice if RequestStore starts over' do
+      it 'computes twice for the different keys, name' do
         algorithm.compute(true)
-      RequestStore.end!
-      RequestStore.clear!
-      RequestStore.begin!
+        algorithm.name = 'same'
         result = algorithm.compute(true)
 
         expect(result).to eq([true, true])
         expect(algorithm.result).to eq(result)
       end
+
+      it 'uses extra method cache key if provided' do
+        algorithm.dispute(true) # miss
+        algorithm.extra = true
+        algorithm.dispute(true) # miss
+        result = algorithm.dispute(true) # hit
+
+        expect(result).to eq([true, true])
+        expect(algorithm.result).to eq(result)
+      end
+    end
   end
 
   context 'when RequestStore is inactive' do
@@ -102,10 +122,18 @@ describe Gitlab::Cache::RequestStoreWrap, :request_store do
       RequestStore.end!
     end
 
-    it 'computes twice even if everything is the same' do
+    it 'computes only once if it is the same instance for the same key' do
       algorithm.compute(true)
       result = algorithm.compute(true)
 
+      expect(result).to eq([true])
+      expect(algorithm.result).to eq(result)
+    end
+
+    it 'computes twice for different instances even if keys are the same' do
+      algorithm.compute(true)
+      result = klass.new('id', 'name', algorithm.result).compute(true)
+
       expect(result).to eq([true, true])
       expect(algorithm.result).to eq(result)
     end

spec/models/commit_spec.rb

@@ -19,17 +19,15 @@ describe Commit, models: true do
       expect(commit.author).to eq(user)
     end
 
-    it 'caches the author' do
-      allow(RequestStore).to receive(:active?).and_return(true)
+    it 'caches the author', :request_store do
       user = create(:user, email: commit.author_email)
-      expect_any_instance_of(Commit).to receive(:find_author_by_any_email).and_call_original
+      expect(User).to receive(:find_by_any_email).and_call_original
 
       expect(commit.author).to eq(user)
-      key = "commit_author:#{commit.author_email}"
+      key = "Commit:author:#{commit.author_email.downcase}"
       expect(RequestStore.store[key]).to eq(user)
 
       expect(commit.author).to eq(user)
-      RequestStore.store.clear
     end
   end