Merge branch 'id-fix-nplus1-for-signatures' into 'master'

Remove N+1 for fetching commit signature

See merge request gitlab-org/gitlab!18389

app/models/gpg_signature.rb

@@ -23,6 +23,8 @@ class GpgSignature < ApplicationRecord
   validates :project_id, presence: true
   validates :gpg_key_primary_keyid, presence: true
 
+  scope :by_commit_sha, ->(shas) { where(commit_sha: shas) }
+
   def self.with_key_and_subkeys(gpg_key)
     subkey_ids = gpg_key.subkeys.pluck(:id)
 

changelogs/unreleased/id-fix-nplus1-for-signatures.yml

+---
+title: Remove N+1 for fetching commits signatures
+merge_request: 18389
+author:
+type: performance

lib/gitlab/gpg/commit.rb

@@ -10,6 +10,8 @@ module Gitlab
 
         repo = commit.project.repository.raw_repository
         @signature_data = Gitlab::Git::Commit.extract_signature_lazily(repo, commit.sha || commit.id)
+
+        lazy_signature
       end
 
       def signature_text
@@ -28,18 +30,16 @@ module Gitlab
         !!(signature_text && signed_text)
       end
 
-      # rubocop: disable CodeReuse/ActiveRecord
       def signature
         return unless has_signature?
 
         return @signature if @signature
 
-        cached_signature = GpgSignature.find_by(commit_sha: @commit.sha)
+        cached_signature = lazy_signature&.itself
         return @signature = cached_signature if cached_signature.present?
 
         @signature = create_cached_signature!
       end
-      # rubocop: enable CodeReuse/ActiveRecord
 
       def update_signature!(cached_signature)
         using_keychain do |gpg_key|
@@ -50,6 +50,14 @@ module Gitlab
 
       private
 
+      def lazy_signature
+        BatchLoader.for(@commit.sha).batch do |shas, loader|
+          GpgSignature.by_commit_sha(shas).each do |signature|
+            loader.call(signature.commit_sha, signature)
+          end
+        end
+      end
+
       def using_keychain
         Gitlab::Gpg.using_tmp_keychain do
           # first we need to get the fingerprint from the signature to query the gpg

spec/lib/gitlab/gpg/commit_spec.rb

@@ -370,5 +370,33 @@ describe Gitlab::Gpg::Commit do
 
       it_behaves_like 'returns the cached signature on second call'
     end
+
+    context 'multiple commits with signatures' do
+      let(:first_signature) { create(:gpg_signature) }
+
+      let(:gpg_key) { create(:gpg_key, key: GpgHelpers::User2.public_key) }
+      let(:second_signature) { create(:gpg_signature, gpg_key: gpg_key) }
+
+      let!(:first_commit) { create(:commit, project: project, sha: first_signature.commit_sha) }
+      let!(:second_commit) { create(:commit, project: project, sha: second_signature.commit_sha) }
+
+      let(:commits) do
+        [first_commit, second_commit].map do |commit|
+          gpg_commit = described_class.new(commit)
+
+          allow(gpg_commit).to receive(:has_signature?).and_return(true)
+
+          gpg_commit
+        end
+      end
+
+      it 'does an aggregated sql request instead of 2 separate ones' do
+        recorder = ActiveRecord::QueryRecorder.new do
+          commits.each(&:signature)
+        end
+
+        expect(recorder.count).to eq(1)
+      end
+    end
   end
 end

spec/models/gpg_signature_spec.rb

@@ -60,6 +60,18 @@ RSpec.describe GpgSignature do
     end
   end
 
+  describe '.by_commit_sha scope' do
+    let(:gpg_key) { create(:gpg_key, key: GpgHelpers::User2.public_key) }
+    let!(:another_gpg_signature) { create(:gpg_signature, gpg_key: gpg_key) }
+
+    it 'returns all gpg signatures by sha' do
+      expect(described_class.by_commit_sha(commit_sha)).to eq([gpg_signature])
+      expect(
+        described_class.by_commit_sha([commit_sha, another_gpg_signature.commit_sha])
+      ).to contain_exactly(gpg_signature, another_gpg_signature)
+    end
+  end
+
   describe '#commit' do
     it 'fetches the commit through the project' do
       expect_any_instance_of(Project).to receive(:commit).with(commit_sha).and_return(commit)