Merge branch 'request-store-wrap' into 'master'

Add RequestCache to cache via RequestStore

See merge request !12920

10 files changed, 258 insertions, 30 deletions

diff --git a/app/models/commit.rb b/app/models/commit.rb
index 21b906e1110..1e19f00106a 100644
--- a/app/models/commit.rb
+++ b/app/models/commit.rb
@@ -1,5 +1,6 @@
 class Commit
   extend ActiveModel::Naming
+  extend Gitlab::Cache::RequestCache
 
   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_cache(: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: [] }
 
diff --git a/app/models/note.rb b/app/models/note.rb
index 3d39047d32f..d0e3bc0bfed 100644
--- a/app/models/note.rb
+++ b/app/models/note.rb
@@ -190,7 +190,7 @@ class Note < ActiveRecord::Base
   # override to return commits, which are not active record
   def noteable
     if for_commit?
-      project.commit(commit_id)
+      @commit ||= project.commit(commit_id)
     else
       super
     end
diff --git a/changelogs/unreleased/request-store-wrap.yml b/changelogs/unreleased/request-store-wrap.yml
new file mode 100644
index 00000000000..8017054b77b
--- /dev/null
+++ b/changelogs/unreleased/request-store-wrap.yml
@@ -0,0 +1,4 @@
+---
+title: Add RequestCache which makes caching with RequestStore easier
+merge_request: 12920
+author:
diff --git a/lib/gitlab/cache/request_cache.rb b/lib/gitlab/cache/request_cache.rb
new file mode 100644
index 00000000000..f1a04affd38
--- /dev/null
+++ b/lib/gitlab/cache/request_cache.rb
@@ -0,0 +1,94 @@
+module Gitlab
+  module Cache
+    # This module provides a simple way to cache values in RequestStore,
+    # and the cache key would be based on the class name, method name,
+    # optionally customized instance level values, optionally customized
+    # method level values, and optional method arguments.
+    #
+    # A simple example:
+    #
+    # class UserAccess
+    #   extend Gitlab::Cache::RequestCache
+    #
+    #   request_cache_key do
+    #     [user&.id, project&.id]
+    #   end
+    #
+    #   request_cache def can_push_to_branch?(ref)
+    #     # ...
+    #   end
+    # end
+    #
+    # This way, the result of `can_push_to_branch?` would be cached in
+    # `RequestStore.store` based on the cache key. If RequestStore is not
+    # currently active, then it would be stored in a hash saved in an
+    # instance variable, so the cache logic would be the same.
+    # Here's another example using customized method level values:
+    #
+    # class Commit
+    #   extend Gitlab::Cache::RequestCache
+    #
+    #   def author
+    #     User.find_by_any_email(author_email.downcase)
+    #   end
+    #   request_cache(:author) { author_email.downcase }
+    # end
+    #
+    # So that we could have different strategies for different methods
+    #
+    module RequestCache
+      def self.extended(klass)
+        return if klass < self
+
+        extension = Module.new
+        klass.const_set(:RequestCacheExtension, extension)
+        klass.prepend(extension)
+      end
+
+      def request_cache_key(&block)
+        if block_given?
+          @request_cache_key = block
+        else
+          @request_cache_key
+        end
+      end
+
+      def request_cache(method_name, &method_key_block)
+        const_get(:RequestCacheExtension).module_eval do
+          cache_key_method_name = "#{method_name}_cache_key"
+
+          define_method(method_name) do |*args|
+            store =
+              if RequestStore.active?
+                RequestStore.store
+              else
+                ivar_name = # ! and ? cannot be used as ivar name
+                  "@cache_#{method_name.to_s.tr('!?', "\u2605\u2606")}"
+
+                instance_variable_get(ivar_name) ||
+                  instance_variable_set(ivar_name, {})
+              end
+
+            key = __send__(cache_key_method_name, args)
+
+            store.fetch(key) { store[key] = super(*args) }
+          end
+
+          define_method(cache_key_method_name) do |args|
+            klass = self.class
+
+            instance_key = instance_exec(&klass.request_cache_key) if
+              klass.request_cache_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
+end
diff --git a/lib/gitlab/user_access.rb b/lib/gitlab/user_access.rb
index 3b922da7ced..8e91ee7287c 100644
--- a/lib/gitlab/user_access.rb
+++ b/lib/gitlab/user_access.rb
@@ -1,5 +1,11 @@
 module Gitlab
   class UserAccess
+    extend Gitlab::Cache::RequestCache
+
+    request_cache_key do
+      [user&.id, project&.id]
+    end
+
     attr_reader :user, :project
 
     def initialize(user, project: nil)
@@ -28,7 +34,7 @@ module Gitlab
       true
     end
 
-    def can_create_tag?(ref)
+    request_cache def can_create_tag?(ref)
       return false unless can_access_git?
 
       if ProtectedTag.protected?(project, ref)
@@ -38,7 +44,7 @@ module Gitlab
       end
     end
 
-    def can_delete_branch?(ref)
+    request_cache def can_delete_branch?(ref)
       return false unless can_access_git?
 
       if ProtectedBranch.protected?(project, ref)
@@ -48,7 +54,7 @@ module Gitlab
       end
     end
 
-    def can_push_to_branch?(ref)
+    request_cache def can_push_to_branch?(ref)
       return false unless can_access_git?
 
       if ProtectedBranch.protected?(project, ref)
@@ -60,7 +66,7 @@ module Gitlab
       end
     end
 
-    def can_merge_to_branch?(ref)
+    request_cache def can_merge_to_branch?(ref)
       return false unless can_access_git?
 
       if ProtectedBranch.protected?(project, ref)
diff --git a/spec/factories/commits.rb b/spec/factories/commits.rb
index 36b9645438a..89e260cf65b 100644
--- a/spec/factories/commits.rb
+++ b/spec/factories/commits.rb
@@ -4,14 +4,19 @@ FactoryGirl.define do
   factory :commit do
     git_commit RepoHelpers.sample_commit
     project factory: :empty_project
-    author { build(:author) }
 
     initialize_with do
       new(git_commit, project)
     end
 
+    after(:build) do |commit|
+      allow(commit).to receive(:author).and_return build(:author)
+    end
+
     trait :without_author do
-      author nil
+      after(:build) do |commit|
+        allow(commit).to receive(:author).and_return nil
+      end
     end
   end
 end
diff --git a/spec/features/participants_autocomplete_spec.rb b/spec/features/participants_autocomplete_spec.rb
index 382d83ca051..81b0a2f541b 100644
--- a/spec/features/participants_autocomplete_spec.rb
+++ b/spec/features/participants_autocomplete_spec.rb
@@ -54,7 +54,8 @@ feature 'Member autocomplete', :js do
     let(:note) { create(:note_on_commit, project: project, commit_id: project.commit.id) }
 
     before do
-      allow_any_instance_of(Commit).to receive(:author).and_return(author)
+      allow(User).to receive(:find_by_any_email)
+        .with(noteable.author_email.downcase).and_return(author)
 
       visit project_commit_path(project, noteable)
     end
diff --git a/spec/lib/gitlab/cache/request_cache_spec.rb b/spec/lib/gitlab/cache/request_cache_spec.rb
new file mode 100644
index 00000000000..5b82c216a13
--- /dev/null
+++ b/spec/lib/gitlab/cache/request_cache_spec.rb
@@ -0,0 +1,133 @@
+require 'spec_helper'
+
+describe Gitlab::Cache::RequestCache do
+  let(:klass) do
+    Class.new do
+      extend Gitlab::Cache::RequestCache
+
+      attr_accessor :id, :name, :result, :extra
+
+      def self.name
+        'ExpensiveAlgorithm'
+      end
+
+      def initialize(id, name, result, extra = nil)
+        self.id = id
+        self.name = name
+        self.result = result
+        self.extra = nil
+      end
+
+      request_cache def compute(arg)
+        result << arg
+      end
+
+      request_cache def repute(arg)
+        result << arg
+      end
+
+      def dispute(arg)
+        result << arg
+      end
+      request_cache(:dispute) { extra }
+    end
+  end
+
+  let(:algorithm) { klass.new('id', 'name', []) }
+
+  shared_examples 'cache for the same instance' do
+    it 'does not compute twice for the same argument' do
+      algorithm.compute(true)
+      result = algorithm.compute(true)
+
+      expect(result).to eq([true])
+    end
+
+    it 'computes twice for the different argument' do
+      algorithm.compute(true)
+      result = algorithm.compute(false)
+
+      expect(result).to eq([true, false])
+    end
+
+    it 'computes twice for the different class name' do
+      algorithm.compute(true)
+      allow(klass).to receive(:name).and_return('CheapAlgo')
+      result = algorithm.compute(true)
+
+      expect(result).to eq([true, true])
+    end
+
+    it 'computes twice for the different method' do
+      algorithm.compute(true)
+      result = algorithm.repute(true)
+
+      expect(result).to eq([true, true])
+    end
+
+    context 'when request_cache_key is provided' do
+      before do
+        klass.request_cache_key do
+          [id, name]
+        end
+      end
+
+      it 'computes twice for the different keys, id' do
+        algorithm.compute(true)
+        algorithm.id = 'ad'
+        result = algorithm.compute(true)
+
+        expect(result).to eq([true, true])
+      end
+
+      it 'computes twice for the different keys, name' do
+        algorithm.compute(true)
+        algorithm.name = 'same'
+        result = algorithm.compute(true)
+
+        expect(result).to eq([true, true])
+      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])
+      end
+    end
+  end
+
+  context 'when RequestStore is active', :request_store do
+    it_behaves_like 'cache for the same instance'
+
+    it 'computes once for different instances when keys are the same' do
+      algorithm.compute(true)
+      result = klass.new('id', 'name', algorithm.result).compute(true)
+
+      expect(result).to eq([true])
+    end
+
+    it 'computes twice if RequestStore starts over' do
+      algorithm.compute(true)
+      RequestStore.end!
+      RequestStore.clear!
+      RequestStore.begin!
+      result = algorithm.compute(true)
+
+      expect(result).to eq([true, true])
+    end
+  end
+
+  context 'when RequestStore is inactive' do
+    it_behaves_like 'cache for the same instance'
+
+    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])
+    end
+  end
+end
diff --git a/spec/models/commit_spec.rb b/spec/models/commit_spec.rb
index 6056d78da4e..528b211c9d6 100644
--- a/spec/models/commit_spec.rb
+++ b/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
 
diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb
index f1e00c1163b..4fc5eb0a527 100644
--- a/spec/services/notification_service_spec.rb
+++ b/spec/services/notification_service_spec.rb
@@ -383,7 +383,7 @@ describe NotificationService, services: true do
       before do
         build_team(note.project)
         reset_delivered_emails!
-        allow_any_instance_of(Commit).to receive(:author).and_return(@u_committer)
+        allow(note.noteable).to receive(:author).and_return(@u_committer)
         update_custom_notification(:new_note, @u_guest_custom, resource: project)
         update_custom_notification(:new_note, @u_custom_global)
       end