Merge branch 'feature/fetch-commit-signs-from-gitaly-in-batch' into 'master'

Fetch commit signatures from Gitaly in batches

Closes gitaly#1046

See merge request gitlab-org/gitlab-ce!17456

4 files changed, 95 insertions, 65 deletions

diff --git a/spec/lib/gitlab/git/commit_spec.rb b/spec/lib/gitlab/git/commit_spec.rb
index 0b20a6349a2..a05feaac1ca 100644
--- a/spec/lib/gitlab/git/commit_spec.rb
+++ b/spec/lib/gitlab/git/commit_spec.rb
@@ -393,81 +393,111 @@ describe Gitlab::Git::Commit, seed_helper: true do
       end
     end
 
-    describe '.extract_signature' do
-      subject { described_class.extract_signature(repository, commit_id) }
-
-      shared_examples '.extract_signature' do
-        context 'when the commit is signed' do
-          let(:commit_id) { '0b4bc9a49b562e85de7cc9e834518ea6828729b9' }
-
-          it 'returns signature and signed text' do
-            signature, signed_text = subject
-
-            expected_signature = <<~SIGNATURE
-              -----BEGIN PGP SIGNATURE-----
-              Version: GnuPG/MacGPG2 v2.0.22 (Darwin)
-              Comment: GPGTools - https://gpgtools.org
+    shared_examples 'extracting commit signature' do
+      context 'when the commit is signed' do
+        let(:commit_id) { '0b4bc9a49b562e85de7cc9e834518ea6828729b9' }
+
+        it 'returns signature and signed text' do
+          signature, signed_text = subject
+
+          expected_signature = <<~SIGNATURE
+            -----BEGIN PGP SIGNATURE-----
+            Version: GnuPG/MacGPG2 v2.0.22 (Darwin)
+            Comment: GPGTools - https://gpgtools.org
+
+            iQEcBAABCgAGBQJTDvaZAAoJEGJ8X1ifRn8XfvYIAMuB0yrbTGo1BnOSoDfyrjb0
+            Kw2EyUzvXYL72B63HMdJ+/0tlSDC6zONF3fc+bBD8z+WjQMTbwFNMRbSSy2rKEh+
+            mdRybOP3xBIMGgEph0/kmWln39nmFQBsPRbZBWoU10VfI/ieJdEOgOphszgryRar
+            TyS73dLBGE9y9NIININVaNISet9D9QeXFqc761CGjh4YIghvPpi+YihMWapGka6v
+            hgKhX+hc5rj+7IEE0CXmlbYR8OYvAbAArc5vJD7UTxAY4Z7/l9d6Ydt9GQ25khfy
+            ANFgltYzlR6evLFmDjssiP/mx/ZMN91AL0ueJ9nNGv411Mu2CUW+tDCaQf35mdc=
+            =j51i
+            -----END PGP SIGNATURE-----
+          SIGNATURE
+
+          expect(signature).to eq(expected_signature.chomp)
+          expect(signature).to be_a_binary_string
+
+          expected_signed_text = <<~SIGNED_TEXT
+            tree 22bfa2fbd217df24731f43ff43a4a0f8db759dae
+            parent ae73cb07c9eeaf35924a10f713b364d32b2dd34f
+            author Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com> 1393489561 +0200
+            committer Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com> 1393489561 +0200
+
+            Feature added
+
+            Signed-off-by: Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>
+          SIGNED_TEXT
+
+          expect(signed_text).to eq(expected_signed_text)
+          expect(signed_text).to be_a_binary_string
+        end
+      end
 
-              iQEcBAABCgAGBQJTDvaZAAoJEGJ8X1ifRn8XfvYIAMuB0yrbTGo1BnOSoDfyrjb0
-              Kw2EyUzvXYL72B63HMdJ+/0tlSDC6zONF3fc+bBD8z+WjQMTbwFNMRbSSy2rKEh+
-              mdRybOP3xBIMGgEph0/kmWln39nmFQBsPRbZBWoU10VfI/ieJdEOgOphszgryRar
-              TyS73dLBGE9y9NIININVaNISet9D9QeXFqc761CGjh4YIghvPpi+YihMWapGka6v
-              hgKhX+hc5rj+7IEE0CXmlbYR8OYvAbAArc5vJD7UTxAY4Z7/l9d6Ydt9GQ25khfy
-              ANFgltYzlR6evLFmDjssiP/mx/ZMN91AL0ueJ9nNGv411Mu2CUW+tDCaQf35mdc=
-              =j51i
-              -----END PGP SIGNATURE-----
-            SIGNATURE
+      context 'when the commit has no signature' do
+        let(:commit_id) { '4b4918a572fa86f9771e5ba40fbd48e1eb03e2c6' }
 
-            expect(signature).to eq(expected_signature.chomp)
-            expect(signature).to be_a_binary_string
+        it 'returns nil' do
+          expect(subject).to be_nil
+        end
+      end
 
-            expected_signed_text = <<~SIGNED_TEXT
-              tree 22bfa2fbd217df24731f43ff43a4a0f8db759dae
-              parent ae73cb07c9eeaf35924a10f713b364d32b2dd34f
-              author Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com> 1393489561 +0200
-              committer Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com> 1393489561 +0200
+      context 'when the commit cannot be found' do
+        let(:commit_id) { Gitlab::Git::BLANK_SHA }
 
-              Feature added
+        it 'returns nil' do
+          expect(subject).to be_nil
+        end
+      end
 
-              Signed-off-by: Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>
-            SIGNED_TEXT
+      context 'when the commit ID is invalid' do
+        let(:commit_id) { '4b4918a572fa86f9771e5ba40fbd48e' }
 
-            expect(signed_text).to eq(expected_signed_text)
-            expect(signed_text).to be_a_binary_string
-          end
+        it 'raises ArgumentError' do
+          expect { subject }.to raise_error(ArgumentError)
         end
+      end
+    end
 
-        context 'when the commit has no signature' do
-          let(:commit_id) { '4b4918a572fa86f9771e5ba40fbd48e1eb03e2c6' }
-
-          it 'returns nil' do
-            expect(subject).to be_nil
+    describe '.extract_signature_lazily' do
+      shared_examples 'loading signatures in batch once' do
+        it 'fetches signatures in batch once' do
+          commit_ids = %w[0b4bc9a49b562e85de7cc9e834518ea6828729b9 4b4918a572fa86f9771e5ba40fbd48e1eb03e2c6]
+          signatures = commit_ids.map do |commit_id|
+            described_class.extract_signature_lazily(repository, commit_id)
           end
-        end
 
-        context 'when the commit cannot be found' do
-          let(:commit_id) { Gitlab::Git::BLANK_SHA }
+          expect(described_class).to receive(:batch_signature_extraction)
+            .with(repository, commit_ids)
+            .once
+            .and_return({})
 
-          it 'returns nil' do
-            expect(subject).to be_nil
-          end
+          2.times { signatures.each(&:itself) }
         end
+      end
 
-        context 'when the commit ID is invalid' do
-          let(:commit_id) { '4b4918a572fa86f9771e5ba40fbd48e' }
+      subject { described_class.extract_signature_lazily(repository, commit_id).itself }
 
-          it 'raises ArgumentError' do
-            expect { subject }.to raise_error(ArgumentError)
-          end
-        end
+      context 'with Gitaly extract_commit_signature_in_batch feature enabled' do
+        it_behaves_like 'extracting commit signature'
+        it_behaves_like 'loading signatures in batch once'
+      end
+
+      context 'with Gitaly extract_commit_signature_in_batch feature disabled', :disable_gitaly do
+        it_behaves_like 'extracting commit signature'
+        it_behaves_like 'loading signatures in batch once'
       end
+    end
+
+    describe '.extract_signature' do
+      subject { described_class.extract_signature(repository, commit_id) }
 
       context 'with gitaly' do
-        it_behaves_like '.extract_signature'
+        it_behaves_like 'extracting commit signature'
       end
 
-      context 'without gitaly', :skip_gitaly_mock do
-        it_behaves_like '.extract_signature'
+      context 'without gitaly', :disable_gitaly do
+        it_behaves_like 'extracting commit signature'
       end
     end
   end
diff --git a/spec/lib/gitlab/gpg/commit_spec.rb b/spec/lib/gitlab/gpg/commit_spec.rb
index 67c62458f0f..8c6d673391b 100644
--- a/spec/lib/gitlab/gpg/commit_spec.rb
+++ b/spec/lib/gitlab/gpg/commit_spec.rb
@@ -38,7 +38,7 @@ describe Gitlab::Gpg::Commit do
           end
 
           before do
-            allow(Gitlab::Git::Commit).to receive(:extract_signature)
+            allow(Gitlab::Git::Commit).to receive(:extract_signature_lazily)
             .with(Gitlab::Git::Repository, commit_sha)
             .and_return(
               [
@@ -101,7 +101,7 @@ describe Gitlab::Gpg::Commit do
           end
 
           before do
-            allow(Gitlab::Git::Commit).to receive(:extract_signature)
+            allow(Gitlab::Git::Commit).to receive(:extract_signature_lazily)
             .with(Gitlab::Git::Repository, commit_sha)
             .and_return(
               [
@@ -140,7 +140,7 @@ describe Gitlab::Gpg::Commit do
           end
 
           before do
-            allow(Gitlab::Git::Commit).to receive(:extract_signature)
+            allow(Gitlab::Git::Commit).to receive(:extract_signature_lazily)
             .with(Gitlab::Git::Repository, commit_sha)
             .and_return(
               [
@@ -175,7 +175,7 @@ describe Gitlab::Gpg::Commit do
           end
 
           before do
-            allow(Gitlab::Git::Commit).to receive(:extract_signature)
+            allow(Gitlab::Git::Commit).to receive(:extract_signature_lazily)
             .with(Gitlab::Git::Repository, commit_sha)
             .and_return(
               [
@@ -211,7 +211,7 @@ describe Gitlab::Gpg::Commit do
         end
 
         before do
-          allow(Gitlab::Git::Commit).to receive(:extract_signature)
+          allow(Gitlab::Git::Commit).to receive(:extract_signature_lazily)
           .with(Gitlab::Git::Repository, commit_sha)
           .and_return(
             [
@@ -241,7 +241,7 @@ describe Gitlab::Gpg::Commit do
       let!(:commit) { create :commit, project: project, sha: commit_sha }
 
       before do
-        allow(Gitlab::Git::Commit).to receive(:extract_signature)
+        allow(Gitlab::Git::Commit).to receive(:extract_signature_lazily)
           .with(Gitlab::Git::Repository, commit_sha)
           .and_return(
             [
diff --git a/spec/lib/gitlab/gpg/invalid_gpg_signature_updater_spec.rb b/spec/lib/gitlab/gpg/invalid_gpg_signature_updater_spec.rb
index c034eccf2a6..6fbffc38444 100644
--- a/spec/lib/gitlab/gpg/invalid_gpg_signature_updater_spec.rb
+++ b/spec/lib/gitlab/gpg/invalid_gpg_signature_updater_spec.rb
@@ -26,7 +26,7 @@ RSpec.describe Gitlab::Gpg::InvalidGpgSignatureUpdater do
     before do
       allow_any_instance_of(Project).to receive(:commit).and_return(commit)
 
-      allow(Gitlab::Git::Commit).to receive(:extract_signature)
+      allow(Gitlab::Git::Commit).to receive(:extract_signature_lazily)
         .with(Gitlab::Git::Repository, commit_sha)
         .and_return(signature)
     end
diff --git a/spec/services/merge_requests/build_service_spec.rb b/spec/services/merge_requests/build_service_spec.rb
index 3a935d98540..6aed481939e 100644
--- a/spec/services/merge_requests/build_service_spec.rb
+++ b/spec/services/merge_requests/build_service_spec.rb
@@ -15,8 +15,8 @@ describe MergeRequests::BuildService do
   let(:target_branch) { 'master' }
   let(:merge_request) { service.execute }
   let(:compare) { double(:compare, commits: commits) }
-  let(:commit_1) { double(:commit_1, safe_message: "Initial commit\n\nCreate the app") }
-  let(:commit_2) { double(:commit_2, safe_message: 'This is a bad commit message!') }
+  let(:commit_1) { double(:commit_1, sha: 'f00ba7', safe_message: "Initial commit\n\nCreate the app") }
+  let(:commit_2) { double(:commit_2, sha: 'f00ba7', safe_message: 'This is a bad commit message!') }
   let(:commits) { nil }
 
   let(:service) do