Merge branch 'dm-gpg-signature-performance' into 'master'

Only create commit GPG signature when necessary

See merge request !13561

app/models/commit.rb

@@ -392,6 +392,6 @@ class Commit
   end
 
   def gpg_commit
-    @gpg_commit ||= Gitlab::Gpg::Commit.new(self)
+    @gpg_commit ||= Gitlab::Gpg::Commit.for_commit(self)
   end
 end

app/models/gpg_signature.rb

@@ -18,4 +18,8 @@ class GpgSignature < ActiveRecord::Base
   def commit
     project.commit(commit_sha)
   end
+
+  def gpg_commit
+    Gitlab::Gpg::Commit.new(project, commit_sha)
+  end
 end

app/services/git_push_service.rb

@@ -90,8 +90,19 @@ class GitPushService < BaseService
   end
 
   def update_signatures
-    @push_commits.each do |commit|
-      CreateGpgSignatureWorker.perform_async(commit.sha, @project.id)
+    commit_shas = @push_commits.last(PROCESS_COMMIT_LIMIT).map(&:sha)
+
+    return if commit_shas.empty?
+
+    shas_with_cached_signatures = GpgSignature.where(commit_sha: commit_shas).pluck(:commit_sha)
+    commit_shas -= shas_with_cached_signatures
+
+    return if commit_shas.empty?
+
+    commit_shas = Gitlab::Git::Commit.shas_with_signatures(project.repository, commit_shas)
+
+    commit_shas.each do |sha|
+      CreateGpgSignatureWorker.perform_async(sha, project.id)
     end
   end
 

app/workers/create_gpg_signature_worker.rb

@@ -4,13 +4,9 @@ class CreateGpgSignatureWorker
 
   def perform(commit_sha, project_id)
     project = Project.find_by(id: project_id)
-
     return unless project
 
-    commit = project.commit(commit_sha)
-
-    return unless commit
-
-    commit.signature
+    # This calculates and caches the signature in the database
+    Gitlab::Gpg::Commit.new(project, commit_sha).signature
   end
 end

config/initializers/active_record_array_type_casting.rb

+module ActiveRecord
+  class PredicateBuilder
+    class ArrayHandler
+      module TypeCasting
+        def call(attribute, value)
+          # This is necessary because by default ActiveRecord does not respect
+          # custom type definitions (like our `ShaAttribute`) when providing an
+          # array in `where`, like in `where(commit_sha: [sha1, sha2, sha3])`.
+          model = attribute.relation&.engine
+          type = model.user_provided_columns[attribute.name] if model
+          value = value.map { |value| type.type_cast_for_database(value) } if type
+
+          super(attribute, value)
+        end
+      end
+
+      prepend TypeCasting
+    end
+  end
+end

lib/gitlab/git/commit.rb

@@ -219,6 +219,16 @@ module Gitlab
 
           @rugged_sort_types.fetch(sort_type, Rugged::SORT_NONE)
         end
+
+        def shas_with_signatures(repository, shas)
+          shas.select do |sha|
+            begin
+              Rugged::Commit.extract_signature(repository.rugged, sha)
+            rescue Rugged::OdbError
+              false
+            end
+          end
+        end
       end
 
       def initialize(raw_commit, head = nil)
@@ -319,15 +329,6 @@ module Gitlab
         end
       end
 
-      # Get the gpg signature of this commit.
-      #
-      # Ex.
-      #   commit.signature(repo)
-      #
-      def signature(repo)
-        Rugged::Commit.extract_signature(repo.rugged, sha)
-      end
-
       def stats
         Gitlab::Git::CommitStats.new(self)
       end

lib/gitlab/gpg/commit.rb

 module Gitlab
   module Gpg
     class Commit
-      attr_reader :commit
+      def self.for_commit(commit)
+        new(commit.project, commit.sha)
+      end
 
-      def initialize(commit)
-        @commit = commit
+      def initialize(project, sha)
+        @project = project
+        @sha = sha
 
-        @signature_text, @signed_text = commit.raw.signature(commit.project.repository)
+        @signature_text, @signed_text =
+          begin
+            Rugged::Commit.extract_signature(project.repository.rugged, sha)
+          rescue Rugged::OdbError
+            nil
+          end
       end
 
       def has_signature?
@@ -16,18 +24,20 @@ module Gitlab
       def signature
         return unless has_signature?
 
-        cached_signature = GpgSignature.find_by(commit_sha: commit.sha)
-        return cached_signature if cached_signature.present?
+        return @signature if @signature
 
-        using_keychain do |gpg_key|
-          create_cached_signature!(gpg_key)
-        end
+        cached_signature = GpgSignature.find_by(commit_sha: @sha)
+        return @signature = cached_signature if cached_signature.present?
+
+        @signature = create_cached_signature!
       end
 
       def update_signature!(cached_signature)
         using_keychain do |gpg_key|
           cached_signature.update_attributes!(attributes(gpg_key))
         end
+
+        @signature = cached_signature
       end
 
       private
@@ -55,16 +65,18 @@ module Gitlab
         end
       end
 
-      def create_cached_signature!(gpg_key)
+      def create_cached_signature!
+        using_keychain do |gpg_key|
           GpgSignature.create!(attributes(gpg_key))
         end
+      end
 
       def attributes(gpg_key)
         user_infos = user_infos(gpg_key)
 
         {
-          commit_sha: commit.sha,
-          project: commit.project,
+          commit_sha: @sha,
+          project: @project,
           gpg_key: gpg_key,
           gpg_key_primary_keyid: gpg_key&.primary_keyid || verified_signature.fingerprint,
           gpg_key_user_name: user_infos[:name],

lib/gitlab/gpg/invalid_gpg_signature_updater.rb

@@ -10,9 +10,7 @@ module Gitlab
           .select(:id, :commit_sha, :project_id)
           .where('gpg_key_id IS NULL OR valid_signature = ?', false)
           .where(gpg_key_primary_keyid: @gpg_key.primary_keyid)
-          .find_each do |gpg_signature|
-            Gitlab::Gpg::Commit.new(gpg_signature.commit).update_signature!(gpg_signature)
-          end
+          .find_each { |sig| sig.gpg_commit.update_signature!(sig) }
       end
     end
   end

spec/lib/gitlab/gpg/commit_spec.rb

 require 'rails_helper'
 
-RSpec.describe Gitlab::Gpg::Commit do
+describe Gitlab::Gpg::Commit do
   describe '#signature' do
     let!(:project) { create :project, :repository, path: 'sample-project' }
     let!(:commit_sha) { '0beec7b5ea3f0fdbc95d0dd47f3c5bc275da8a33'  }
 
-    context 'unisgned commit' do
+    context 'unsigned commit' do
       it 'returns nil' do
-        expect(described_class.new(project.commit).signature).to be_nil
+        expect(described_class.new(project, commit_sha).signature).to be_nil
       end
     end
 
@@ -16,18 +16,19 @@ RSpec.describe Gitlab::Gpg::Commit do
         create :gpg_key, key: GpgHelpers::User1.public_key, user: create(:user, email: GpgHelpers::User1.emails.first)
       end
 
-      let!(:commit) do
-        raw_commit = double(:raw_commit, signature: [
+      before do
+        allow(Rugged::Commit).to receive(:extract_signature)
+          .with(Rugged::Repository, commit_sha)
+          .and_return(
+            [
               GpgHelpers::User1.signed_commit_signature,
               GpgHelpers::User1.signed_commit_base_data
-        ], sha: commit_sha)
-        allow(raw_commit).to receive :save!
-
-        create :commit, git_commit: raw_commit, project: project
+            ]
+          )
       end
 
       it 'returns a valid signature' do
-        expect(described_class.new(commit).signature).to have_attributes(
+        expect(described_class.new(project, commit_sha).signature).to have_attributes(
           commit_sha: commit_sha,
           project: project,
           gpg_key: gpg_key,
@@ -39,7 +40,7 @@ RSpec.describe Gitlab::Gpg::Commit do
       end
 
       it 'returns the cached signature on second call' do
-        gpg_commit = described_class.new(commit)
+        gpg_commit = described_class.new(project, commit_sha)
 
         expect(gpg_commit).to receive(:using_keychain).and_call_original
         gpg_commit.signature
@@ -53,18 +54,19 @@ RSpec.describe Gitlab::Gpg::Commit do
     context 'known but unverified public key' do
       let!(:gpg_key) { create :gpg_key, key: GpgHelpers::User1.public_key }
 
-      let!(:commit) do
-        raw_commit = double(:raw_commit, signature: [
+      before do
+        allow(Rugged::Commit).to receive(:extract_signature)
+          .with(Rugged::Repository, commit_sha)
+          .and_return(
+            [
               GpgHelpers::User1.signed_commit_signature,
               GpgHelpers::User1.signed_commit_base_data
-        ], sha: commit_sha)
-        allow(raw_commit).to receive :save!
-
-        create :commit, git_commit: raw_commit, project: project
+            ]
+          )
       end
 
       it 'returns an invalid signature' do
-        expect(described_class.new(commit).signature).to have_attributes(
+        expect(described_class.new(project, commit_sha).signature).to have_attributes(
           commit_sha: commit_sha,
           project: project,
           gpg_key: gpg_key,
@@ -76,7 +78,7 @@ RSpec.describe Gitlab::Gpg::Commit do
       end
 
       it 'returns the cached signature on second call' do
-        gpg_commit = described_class.new(commit)
+        gpg_commit = described_class.new(project, commit_sha)
 
         expect(gpg_commit).to receive(:using_keychain).and_call_original
         gpg_commit.signature
@@ -88,20 +90,19 @@ RSpec.describe Gitlab::Gpg::Commit do
     end
 
     context 'unknown public key' do
-      let!(:commit) do
-        raw_commit = double(:raw_commit, signature: [
+      before do
+        allow(Rugged::Commit).to receive(:extract_signature)
+          .with(Rugged::Repository, commit_sha)
+          .and_return(
+            [
               GpgHelpers::User1.signed_commit_signature,
               GpgHelpers::User1.signed_commit_base_data
-        ], sha: commit_sha)
-        allow(raw_commit).to receive :save!
-
-        create :commit,
-          git_commit: raw_commit,
-          project: project
+            ]
+          )
       end
 
       it 'returns an invalid signature' do
-        expect(described_class.new(commit).signature).to have_attributes(
+        expect(described_class.new(project, commit_sha).signature).to have_attributes(
           commit_sha: commit_sha,
           project: project,
           gpg_key: nil,
@@ -113,7 +114,7 @@ RSpec.describe Gitlab::Gpg::Commit do
       end
 
       it 'returns the cached signature on second call' do
-        gpg_commit = described_class.new(commit)
+        gpg_commit = described_class.new(project, commit_sha)
 
         expect(gpg_commit).to receive(:using_keychain).and_call_original
         gpg_commit.signature

spec/lib/gitlab/gpg/invalid_gpg_signature_updater_spec.rb

@@ -4,23 +4,16 @@ RSpec.describe Gitlab::Gpg::InvalidGpgSignatureUpdater do
   describe '#run' do
     let!(:commit_sha) { '0beec7b5ea3f0fdbc95d0dd47f3c5bc275da8a33' }
     let!(:project) { create :project, :repository, path: 'sample-project' }
-    let!(:raw_commit) do
-      raw_commit = double(:raw_commit, signature: [
-        GpgHelpers::User1.signed_commit_signature,
-        GpgHelpers::User1.signed_commit_base_data
-      ], sha: commit_sha)
-
-      allow(raw_commit).to receive :save!
-
-      raw_commit
-    end
-
-    let!(:commit) do
-      create :commit, git_commit: raw_commit, project: project
-    end
 
     before do
-      allow_any_instance_of(Project).to receive(:commit).and_return(commit)
+      allow(Rugged::Commit).to receive(:extract_signature)
+        .with(Rugged::Repository, commit_sha)
+        .and_return(
+          [
+            GpgHelpers::User1.signed_commit_signature,
+            GpgHelpers::User1.signed_commit_base_data
+          ]
+        )
     end
 
     context 'gpg signature did have an associated gpg key which was removed later' do

spec/services/git_push_service_spec.rb

@@ -688,12 +688,40 @@ describe GitPushService, services: true do
       )
     end
 
-    it 'calls CreateGpgSignatureWorker.perform_async for each commit' do
+    context 'when the commit has a signature' do
+      context 'when the signature is already cached' do
+        before do
+          create(:gpg_signature, commit_sha: sample_commit.id)
+        end
+
+        it 'does not queue a CreateGpgSignatureWorker' do
+          expect(CreateGpgSignatureWorker).not_to receive(:perform_async).with(sample_commit.id, project.id)
+
+          execute_service(project, user, oldrev, newrev, ref)
+        end
+      end
+
+      context 'when the signature is not yet cached' do
+        it 'queues a CreateGpgSignatureWorker' do
           expect(CreateGpgSignatureWorker).to receive(:perform_async).with(sample_commit.id, project.id)
 
           execute_service(project, user, oldrev, newrev, ref)
         end
       end
+    end
+
+    context 'when the commit does not have a signature' do
+      before do
+        allow(Gitlab::Git::Commit).to receive(:shas_with_signatures).with(project.repository, [sample_commit.id]).and_return([])
+      end
+
+      it 'does not queue a CreateGpgSignatureWorker' do
+        expect(CreateGpgSignatureWorker).not_to receive(:perform_async).with(sample_commit.id, project.id)
+
+        execute_service(project, user, oldrev, newrev, ref)
+      end
+    end
+  end
 
   def execute_service(project, user, oldrev, newrev, ref)
     service = described_class.new(project, user, oldrev: oldrev, newrev: newrev, ref: ref)

spec/workers/create_gpg_signature_worker_spec.rb

 require 'spec_helper'
 
 describe CreateGpgSignatureWorker do
+  let(:project) { create(:project, :repository) }
+
   context 'when GpgKey is found' do
-    it 'calls Commit#signature' do
-      commit_sha = '0beec7b5ea3f0fdbc95d0dd47f3c5bc275da8a33'
-      project = create :project
-      commit = instance_double(Commit)
+    let(:commit_sha) { '0beec7b5ea3f0fdbc95d0dd47f3c5bc275da8a33' }
 
-      allow(Project).to receive(:find_by).with(id: project.id).and_return(project)
-      allow(project).to receive(:commit).with(commit_sha).and_return(commit)
+    it 'calls Gitlab::Gpg::Commit#signature' do
+      expect(Gitlab::Gpg::Commit).to receive(:new).with(project, commit_sha).and_call_original
 
-      expect(commit).to receive(:signature)
+      expect_any_instance_of(Gitlab::Gpg::Commit).to receive(:signature)
 
       described_class.new.perform(commit_sha, project.id)
     end
   end
 
   context 'when Commit is not found' do
-    let(:nonexisting_commit_sha) { 'bogus' }
-    let(:project) { create :project }
+    let(:nonexisting_commit_sha) { '0beec7b5ea3f0fdbc95d0dd47f3c5bc275da8a34' }
 
     it 'does not raise errors' do
       expect { described_class.new.perform(nonexisting_commit_sha, project.id) }.not_to raise_error
     end
-
-    it 'does not call Commit#signature' do
-      expect_any_instance_of(Commit).not_to receive(:signature)
-
-      described_class.new.perform(nonexisting_commit_sha, project.id)
-    end
   end
 
   context 'when Project is not found' do
@@ -38,8 +30,8 @@ describe CreateGpgSignatureWorker do
       expect { described_class.new.perform(anything, nonexisting_project_id) }.not_to raise_error
     end
 
-    it 'does not call Commit#signature' do
-      expect_any_instance_of(Commit).not_to receive(:signature)
+    it 'does not call Gitlab::Gpg::Commit#signature' do
+      expect_any_instance_of(Gitlab::Gpg::Commit).not_to receive(:signature)
 
       described_class.new.perform(anything, nonexisting_project_id)
     end