Merge remote-tracking branch 'upstream/master' into 30634-protected-pipeline

* upstream/master: (25 commits)
  Remove unneeded asserts and add tests for inactive RequestStore
  Rename the methods to make it fit with current name
  Follow feedback on the merge request
  Make sure it checks against the tag only when it's a tag
  Renamed Gitaly services
  fix transient rspec failure due to Poll.js race condition
  Refactor variables initialization in dropzone_input.js
  cache the cache key...
  avoid #respond_to? in Cache.id_for
  cache DeclarativePolicy.class_for at the class level
  Update 9.3-to-9.4.md
  fix padding on filtered search dropdown. Styles should only apply to li in list
  Cache Note#notable for commits and fix tests
  Add changelog entry
  Update the comments for the new functionality
  Use RequestStoreWrap for Commit#author
  Skip dead jobs queue for web hooks and project services
  Add RequestStoreWrap to cache via RequestStore
  Don't track cached queries in Gitlab::PerformanceBar::PeekQueryTracker
  Add changelog entry
  ...

6 files changed, 161 insertions, 28 deletions

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/lib/gitlab/git/repository_spec.rb b/spec/lib/gitlab/git/repository_spec.rb
index 3eeed6126a0..83d067b2c31 100644
--- a/spec/lib/gitlab/git/repository_spec.rb
+++ b/spec/lib/gitlab/git/repository_spec.rb
@@ -45,11 +45,11 @@ describe Gitlab::Git::Repository, seed_helper: true do
     end
 
     it 'gets the branch name from GitalyClient' do
-      expect_any_instance_of(Gitlab::GitalyClient::Ref).to receive(:default_branch_name)
+      expect_any_instance_of(Gitlab::GitalyClient::RefService).to receive(:default_branch_name)
       repository.root_ref
     end
 
-    it_behaves_like 'wrapping gRPC errors', Gitlab::GitalyClient::Ref, :default_branch_name do
+    it_behaves_like 'wrapping gRPC errors', Gitlab::GitalyClient::RefService, :default_branch_name do
       subject { repository.root_ref }
     end
   end
@@ -132,11 +132,11 @@ describe Gitlab::Git::Repository, seed_helper: true do
     it { is_expected.not_to include("branch-from-space") }
 
     it 'gets the branch names from GitalyClient' do
-      expect_any_instance_of(Gitlab::GitalyClient::Ref).to receive(:branch_names)
+      expect_any_instance_of(Gitlab::GitalyClient::RefService).to receive(:branch_names)
       subject
     end
 
-    it_behaves_like 'wrapping gRPC errors', Gitlab::GitalyClient::Ref, :branch_names
+    it_behaves_like 'wrapping gRPC errors', Gitlab::GitalyClient::RefService, :branch_names
   end
 
   describe '#tag_names' do
@@ -160,11 +160,11 @@ describe Gitlab::Git::Repository, seed_helper: true do
     it { is_expected.not_to include("v5.0.0") }
 
     it 'gets the tag names from GitalyClient' do
-      expect_any_instance_of(Gitlab::GitalyClient::Ref).to receive(:tag_names)
+      expect_any_instance_of(Gitlab::GitalyClient::RefService).to receive(:tag_names)
       subject
     end
 
-    it_behaves_like 'wrapping gRPC errors', Gitlab::GitalyClient::Ref, :tag_names
+    it_behaves_like 'wrapping gRPC errors', Gitlab::GitalyClient::RefService, :tag_names
   end
 
   shared_examples 'archive check' do |extenstion|
@@ -368,7 +368,7 @@ describe Gitlab::Git::Repository, seed_helper: true do
 
     context 'when Gitaly commit_count feature is enabled' do
       it_behaves_like 'counting commits'
-      it_behaves_like 'wrapping gRPC errors', Gitlab::GitalyClient::Commit, :commit_count do
+      it_behaves_like 'wrapping gRPC errors', Gitlab::GitalyClient::CommitService, :commit_count do
         subject { repository.commit_count('master') }
       end
     end
@@ -1225,12 +1225,12 @@ describe Gitlab::Git::Repository, seed_helper: true do
     end
 
     it 'gets the branches from GitalyClient' do
-      expect_any_instance_of(Gitlab::GitalyClient::Ref).to receive(:local_branches)
+      expect_any_instance_of(Gitlab::GitalyClient::RefService).to receive(:local_branches)
         .and_return([])
       @repo.local_branches
     end
 
-    it_behaves_like 'wrapping gRPC errors', Gitlab::GitalyClient::Ref, :local_branches do
+    it_behaves_like 'wrapping gRPC errors', Gitlab::GitalyClient::RefService, :local_branches do
       subject { @repo.local_branches }
     end
   end
diff --git a/spec/lib/gitlab/gitaly_client/commit_spec.rb b/spec/lib/gitlab/gitaly_client/commit_service_spec.rb
index dff5b25c712..fee5bb45fe5 100644
--- a/spec/lib/gitlab/gitaly_client/commit_spec.rb
+++ b/spec/lib/gitlab/gitaly_client/commit_service_spec.rb
@@ -1,7 +1,7 @@
 require 'spec_helper'
 
-describe Gitlab::GitalyClient::Commit do
-  let(:diff_stub) { double('Gitaly::Diff::Stub') }
+describe Gitlab::GitalyClient::CommitService do
+  let(:diff_stub) { double('Gitaly::DiffService::Stub') }
   let(:project) { create(:project, :repository) }
   let(:repository) { project.repository }
   let(:repository_message) { repository.gitaly_repository }
@@ -16,7 +16,7 @@ describe Gitlab::GitalyClient::Commit do
           right_commit_id: commit.id
         )
 
-        expect_any_instance_of(Gitaly::Diff::Stub).to receive(:commit_diff).with(request, kind_of(Hash))
+        expect_any_instance_of(Gitaly::DiffService::Stub).to receive(:commit_diff).with(request, kind_of(Hash))
 
         described_class.new(repository).diff_from_parent(commit)
       end
@@ -31,7 +31,7 @@ describe Gitlab::GitalyClient::Commit do
           right_commit_id: initial_commit.id
         )
 
-        expect_any_instance_of(Gitaly::Diff::Stub).to receive(:commit_diff).with(request, kind_of(Hash))
+        expect_any_instance_of(Gitaly::DiffService::Stub).to receive(:commit_diff).with(request, kind_of(Hash))
 
         described_class.new(repository).diff_from_parent(initial_commit)
       end
@@ -61,7 +61,7 @@ describe Gitlab::GitalyClient::Commit do
           right_commit_id: commit.id
         )
 
-        expect_any_instance_of(Gitaly::Diff::Stub).to receive(:commit_delta).with(request, kind_of(Hash)).and_return([])
+        expect_any_instance_of(Gitaly::DiffService::Stub).to receive(:commit_delta).with(request, kind_of(Hash)).and_return([])
 
         described_class.new(repository).commit_deltas(commit)
       end
@@ -76,7 +76,7 @@ describe Gitlab::GitalyClient::Commit do
           right_commit_id: initial_commit.id
         )
 
-        expect_any_instance_of(Gitaly::Diff::Stub).to receive(:commit_delta).with(request, kind_of(Hash)).and_return([])
+        expect_any_instance_of(Gitaly::DiffService::Stub).to receive(:commit_delta).with(request, kind_of(Hash)).and_return([])
 
         described_class.new(repository).commit_deltas(initial_commit)
       end
diff --git a/spec/lib/gitlab/gitaly_client/notifications_spec.rb b/spec/lib/gitlab/gitaly_client/notification_service_spec.rb
index 7404ffe0f06..d9597c4aa78 100644
--- a/spec/lib/gitlab/gitaly_client/notifications_spec.rb
+++ b/spec/lib/gitlab/gitaly_client/notification_service_spec.rb
@@ -1,6 +1,6 @@
 require 'spec_helper'
 
-describe Gitlab::GitalyClient::Notifications do
+describe Gitlab::GitalyClient::NotificationService do
   describe '#post_receive' do
     let(:project) { create(:empty_project) }
     let(:storage_name) { project.repository_storage }
@@ -8,7 +8,7 @@ describe Gitlab::GitalyClient::Notifications do
     subject { described_class.new(project.repository) }
 
     it 'sends a post_receive message' do
-      expect_any_instance_of(Gitaly::Notifications::Stub)
+      expect_any_instance_of(Gitaly::NotificationService::Stub)
         .to receive(:post_receive).with(gitaly_request_with_path(storage_name, relative_path), kind_of(Hash))
 
       subject.post_receive
diff --git a/spec/lib/gitlab/gitaly_client/ref_spec.rb b/spec/lib/gitlab/gitaly_client/ref_service_spec.rb
index 7c090460764..1e8ed9d645b 100644
--- a/spec/lib/gitlab/gitaly_client/ref_spec.rb
+++ b/spec/lib/gitlab/gitaly_client/ref_service_spec.rb
@@ -1,6 +1,6 @@
 require 'spec_helper'
 
-describe Gitlab::GitalyClient::Ref do
+describe Gitlab::GitalyClient::RefService do
   let(:project) { create(:empty_project) }
   let(:storage_name) { project.repository_storage }
   let(:relative_path) { project.path_with_namespace + '.git' }
@@ -8,7 +8,7 @@ describe Gitlab::GitalyClient::Ref do
 
   describe '#branch_names' do
     it 'sends a find_all_branch_names message' do
-      expect_any_instance_of(Gitaly::Ref::Stub)
+      expect_any_instance_of(Gitaly::RefService::Stub)
         .to receive(:find_all_branch_names)
         .with(gitaly_request_with_path(storage_name, relative_path), kind_of(Hash))
         .and_return([])
@@ -19,7 +19,7 @@ describe Gitlab::GitalyClient::Ref do
 
   describe '#tag_names' do
     it 'sends a find_all_tag_names message' do
-      expect_any_instance_of(Gitaly::Ref::Stub)
+      expect_any_instance_of(Gitaly::RefService::Stub)
         .to receive(:find_all_tag_names)
         .with(gitaly_request_with_path(storage_name, relative_path), kind_of(Hash))
         .and_return([])
@@ -30,7 +30,7 @@ describe Gitlab::GitalyClient::Ref do
 
   describe '#default_branch_name' do
     it 'sends a find_default_branch_name message' do
-      expect_any_instance_of(Gitaly::Ref::Stub)
+      expect_any_instance_of(Gitaly::RefService::Stub)
         .to receive(:find_default_branch_name)
         .with(gitaly_request_with_path(storage_name, relative_path), kind_of(Hash))
         .and_return(double(name: 'foo'))
@@ -41,7 +41,7 @@ describe Gitlab::GitalyClient::Ref do
 
   describe '#local_branches' do
     it 'sends a find_local_branches message' do
-      expect_any_instance_of(Gitaly::Ref::Stub)
+      expect_any_instance_of(Gitaly::RefService::Stub)
         .to receive(:find_local_branches)
         .with(gitaly_request_with_path(storage_name, relative_path), kind_of(Hash))
         .and_return([])
@@ -50,7 +50,7 @@ describe Gitlab::GitalyClient::Ref do
     end
 
     it 'parses and sends the sort parameter' do
-      expect_any_instance_of(Gitaly::Ref::Stub)
+      expect_any_instance_of(Gitaly::RefService::Stub)
         .to receive(:find_local_branches)
         .with(gitaly_request_with_params(sort_by: :UPDATED_DESC), kind_of(Hash))
         .and_return([])
@@ -59,7 +59,7 @@ describe Gitlab::GitalyClient::Ref do
     end
 
     it 'translates known mismatches on sort param values' do
-      expect_any_instance_of(Gitaly::Ref::Stub)
+      expect_any_instance_of(Gitaly::RefService::Stub)
         .to receive(:find_local_branches)
         .with(gitaly_request_with_params(sort_by: :NAME), kind_of(Hash))
         .and_return([])
diff --git a/spec/lib/gitlab/gitaly_client_spec.rb b/spec/lib/gitlab/gitaly_client_spec.rb
index ce7b18b784a..558ddb3fbd6 100644
--- a/spec/lib/gitlab/gitaly_client_spec.rb
+++ b/spec/lib/gitlab/gitaly_client_spec.rb
@@ -16,9 +16,9 @@ describe Gitlab::GitalyClient, lib: true, skip_gitaly_mock: true do
           'default' => { 'gitaly_address' => address }
         })
 
-        expect(Gitaly::Commit::Stub).to receive(:new).with(address, any_args)
+        expect(Gitaly::CommitService::Stub).to receive(:new).with(address, any_args)
 
-        described_class.stub(:commit, 'default')
+        described_class.stub(:commit_service, 'default')
       end
     end
 
@@ -31,9 +31,9 @@ describe Gitlab::GitalyClient, lib: true, skip_gitaly_mock: true do
           'default' => { 'gitaly_address' => prefixed_address }
         })
 
-        expect(Gitaly::Commit::Stub).to receive(:new).with(address, any_args)
+        expect(Gitaly::CommitService::Stub).to receive(:new).with(address, any_args)
 
-        described_class.stub(:commit, 'default')
+        described_class.stub(:commit_service, 'default')
       end
     end
   end