Fetch commit signatures from Gitaly in batches

Closes gitaly#1046

GITALY_SERVER_VERSION

-0.85.0
+0.87.0

Gemfile

@@ -411,7 +411,7 @@ group :ed25519 do
 end
 # Gitaly GRPC client
-gem 'gitaly-proto', '~> 0.85.0', require: 'gitaly'
+gem 'gitaly-proto', '~> 0.87.0', require: 'gitaly'
 # Locked until https://github.com/google/protobuf/issues/4210 is closed
 gem 'google-protobuf', '= 3.5.1'

Gemfile.lock

@@ -285,7 +285,7 @@ GEM
       po_to_json (>= 1.0.0)
       rails (>= 3.2.0)
     gherkin-ruby (0.3.2)
-    gitaly-proto (0.85.0)
+    gitaly-proto (0.87.0)
       google-protobuf (~> 3.1)
       grpc (~> 1.0)
     github-linguist (5.3.3)
@@ -1057,7 +1057,7 @@ DEPENDENCIES
   gettext (~> 3.2.2)
   gettext_i18n_rails (~> 1.8.0)
   gettext_i18n_rails_js (~> 1.2.0)
-  gitaly-proto (~> 0.85.0)
+  gitaly-proto (~> 0.87.0)
   github-linguist (~> 5.3.3)
   gitlab-flowdock-git-hook (~> 1.0.1)
   gitlab-markup (~> 1.6.2)

app/controllers/projects/commits_controller.rb

@@ -14,37 +14,31 @@ class Projects::CommitsController < Projects::ApplicationController
     @merge_request = MergeRequestsFinder.new(current_user, project_id: @project.id).execute.opened
       .find_by(source_project: @project, source_branch: @ref, target_branch: @repository.root_ref)
-    # https://gitlab.com/gitlab-org/gitaly/issues/931
-    Gitlab::GitalyClient.allow_n_plus_1_calls do
-      respond_to do |format|
-        format.html
-        format.atom { render layout: 'xml.atom' }
-        format.json do
-          pager_json(
-            'projects/commits/_commits',
-            @commits.size,
-            project: @project,
-            ref: @ref)
-        end
+    respond_to do |format|
+      format.html
+      format.atom { render layout: 'xml.atom' }
+      format.json do
+        pager_json(
+          'projects/commits/_commits',
+          @commits.size,
+          project: @project,
+          ref: @ref)
       end
     end
   end
   def signatures
-    # https://gitlab.com/gitlab-org/gitaly/issues/931
-    Gitlab::GitalyClient.allow_n_plus_1_calls do
-      respond_to do |format|
-        format.json do
-          render json: {
-            signatures: @commits.select(&:has_signature?).map do |commit|
-              {
-                commit_sha: commit.sha,
-                html: view_to_html_string('projects/commit/_signature', signature: commit.signature)
-              }
-            end
-          }
-        end
+    respond_to do |format|
+      format.json do
+        render json: {
+          signatures: @commits.select(&:has_signature?).map do |commit|
+            {
+              commit_sha: commit.sha,
+              html: view_to_html_string('projects/commit/_signature', signature: commit.signature)
+            }
+          end
+        }
       end
     end
   end

app/models/commit.rb

@@ -19,6 +19,7 @@ class Commit
   attr_accessor :project, :author
   attr_accessor :redacted_description_html
   attr_accessor :redacted_title_html
+  attr_reader :gpg_commit
   DIFF_SAFE_LINES = Gitlab::Git::DiffCollection::DEFAULT_LIMITS[:max_lines]
@@ -110,6 +111,7 @@ class Commit
     @raw = raw_commit
     @project = project
     @statuses = {}
+    @gpg_commit = Gitlab::Gpg::Commit.new(self) if project
   end
   def id
@@ -452,8 +454,4 @@ class Commit
   def merged_merge_request_no_cache(user)
     MergeRequestsFinder.new(user, project_id: project.id).find_by(merge_commit_sha: id) if merge_commit?
   end
-
-  def gpg_commit
-    @gpg_commit ||= Gitlab::Gpg::Commit.new(self)
-  end
 end

lib/gitlab/git/commit.rb

@@ -250,6 +250,45 @@ module Gitlab
           end
         end
+        def extract_signature_lazily(repository, commit_id)
+          BatchLoader.for({ repository: repository, commit_id: commit_id }).batch do |items, loader|
+            items_by_repo = items.group_by { |i| i[:repository] }
+
+            items_by_repo.each do |repo, items|
+              commit_ids = items.map { |i| i[:commit_id] }
+
+              signatures = batch_signature_extraction(repository, commit_ids)
+
+              signatures.each do |commit_sha, signature_data|
+                loader.call({ repository: repository, commit_id: commit_sha }, signature_data)
+              end
+            end
+          end
+        end
+
+        def batch_signature_extraction(repository, commit_ids)
+          repository.gitaly_migrate(:extract_commit_signature_in_batch) do |is_enabled|
+            if is_enabled
+              gitaly_batch_signature_extraction(repository, commit_ids)
+            else
+              rugged_batch_signature_extraction(repository, commit_ids)
+            end
+          end
+        end
+
+        def gitaly_batch_signature_extraction(repository, commit_ids)
+          repository.gitaly_commit_client.get_commit_signatures(commit_ids)
+        end
+
+        def rugged_batch_signature_extraction(repository, commit_ids)
+          commit_ids.each_with_object({}) do |commit_id, signatures|
+            signature_data = rugged_extract_signature(repository, commit_id)
+            next unless signature_data
+
+            signatures[commit_id] = signature_data
+          end
+        end
+
         def rugged_extract_signature(repository, commit_id)
           begin
             Rugged::Commit.extract_signature(repository.rugged, commit_id)

lib/gitlab/gitaly_client/commit_service.rb

@@ -319,6 +319,23 @@ module Gitlab
         [signature, signed_text]
       end
+      def get_commit_signatures(commit_ids)
+        request = Gitaly::GetCommitSignaturesRequest.new(repository: @gitaly_repo, commit_ids: commit_ids)
+        response = GitalyClient.call(@repository.storage, :commit_service, :get_commit_signatures, request)
+
+        signatures = Hash.new { |h, k| h[k] = [''.b, ''.b] }
+        current_commit_id = nil
+
+        response.each do |message|
+          current_commit_id = message.commit_id if message.commit_id.present?
+
+          signatures[current_commit_id].first << message.signature
+          signatures[current_commit_id].last << message.signed_text
+        end
+
+        signatures
+      end
+
       private
       def call_commit_diff(request_params, options = {})

lib/gitlab/gpg/commit.rb

 module Gitlab
   module Gpg
     class Commit
+      include Gitlab::Utils::StrongMemoize
+
       def initialize(commit)
         @commit = commit
         repo = commit.project.repository.raw_repository
-        @signature_text, @signed_text = Gitlab::Git::Commit.extract_signature(repo, commit.sha)
+        @signature_data = Gitlab::Git::Commit.extract_signature_lazily(repo, commit.sha || commit.id)
+      end
+
+      def signature_text
+        strong_memoize(:signature_text) do
+          @signature_data&.itself && @signature_data[0]
+        end
+      end
+
+      def signed_text
+        strong_memoize(:signed_text) do
+          @signature_data&.itself && @signature_data[1]
+        end
       end
       def has_signature?
-        !!(@signature_text && @signed_text)
+        !!(signature_text && signed_text)
       end
       def signature
@@ -53,7 +67,7 @@ module Gitlab
       end
       def verified_signature
-        @verified_signature ||= GPGME::Crypto.new.verify(@signature_text, signed_text: @signed_text) do |verified_signature|
+        @verified_signature ||= GPGME::Crypto.new.verify(signature_text, signed_text: signed_text) do |verified_signature|
           break verified_signature
         end
       end

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
-              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
+    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
+      context 'when the commit has no signature' do
+        let(:commit_id) { '4b4918a572fa86f9771e5ba40fbd48e1eb03e2c6' }
+        it 'returns nil' do
+          expect(subject).to be_nil
+        end
+      end
+      context 'when the commit cannot be found' do
+        let(:commit_id) { Gitlab::Git::BLANK_SHA }
+        it 'returns nil' do
+          expect(subject).to be_nil
+        end
+      end
+      context 'when the commit ID is invalid' do
+        let(:commit_id) { '4b4918a572fa86f9771e5ba40fbd48e' }
+        it 'raises ArgumentError' do
+          expect { subject }.to raise_error(ArgumentError)
         end
-        context 'when the commit has no signature' do
-          let(:commit_id) { '4b4918a572fa86f9771e5ba40fbd48e1eb03e2c6' }
-
-          it 'returns nil' do
-            expect(subject).to be_nil
+      end
+    end
+    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 }
-          it 'returns nil' do
-            expect(subject).to be_nil
-          end
+          expect(described_class).to receive(:batch_signature_extraction)
+            .with(repository, commit_ids)
+            .once
+            .and_return({})
+          2.times { signatures.each(&:itself) }
         end
-        context 'when the commit ID is invalid' do
-          let(:commit_id) { '4b4918a572fa86f9771e5ba40fbd48e' }
-          it 'raises ArgumentError' do
-            expect { subject }.to raise_error(ArgumentError)
-          end
-        end
+      end
+      subject { described_class.extract_signature_lazily(repository, commit_id).itself }
+      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

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(
             [

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

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