diff --git a/app/models/commit_signatures/ssh_signature.rb b/app/models/commit_signatures/ssh_signature.rb index e9e16651d1c95479627bb3ff2160a8697e71dda5..176c8f02a3024f01f8d092f3822b41bc83456264 100644 --- a/app/models/commit_signatures/ssh_signature.rb +++ b/app/models/commit_signatures/ssh_signature.rb @@ -2,12 +2,22 @@ module CommitSignatures class SshSignature < ApplicationRecord + extend ::Gitlab::Utils::Override include CommitSignature include SignatureType belongs_to :key, optional: true belongs_to :user, optional: true + override :safe_create! + def self.safe_create!(attributes) + create_with(attributes) + .safe_find_or_create_by!( # rubocop:disable Performance/ActiveRecordSubtransactionMethods -- This overrides an existent class method defined in CommitSignature concern + project_id: attributes[:project].id, + commit_sha: attributes[:commit_sha] + ) + end + def type :ssh end diff --git a/app/models/concerns/commit_signature.rb b/app/models/concerns/commit_signature.rb index 3a11c72fe5408a20f7b496d1d03a45f1122ab73f..69b2c2c6291421303fa1278bdcfc9760fb311677 100644 --- a/app/models/concerns/commit_signature.rb +++ b/app/models/concerns/commit_signature.rb @@ -17,6 +17,7 @@ module CommitSignature validates :project_id, presence: true scope :by_commit_sha, ->(shas) { where(commit_sha: shas) } + scope :by_commit_shas_and_project_ids, ->(shas, project_ids) { where(commit_sha: shas, project_id: project_ids) } end class_methods do diff --git a/db/post_migrate/20251202160003_replace_index_ssh_signatures_on_commit_sha.rb b/db/post_migrate/20251202160003_replace_index_ssh_signatures_on_commit_sha.rb new file mode 100644 index 0000000000000000000000000000000000000000..263d09c0d0483a9d11b09712ccdf2152d15cb7c5 --- /dev/null +++ b/db/post_migrate/20251202160003_replace_index_ssh_signatures_on_commit_sha.rb @@ -0,0 +1,22 @@ +# frozen_string_literal: true + +class ReplaceIndexSshSignaturesOnCommitSha < Gitlab::Database::Migration[2.3] + milestone '18.7' + disable_ddl_transaction! + + OLD_UNIQUE_INDEX = 'index_ssh_signatures_on_commit_sha' + NEW_COMPOUND_INDEX = 'index_ssh_signatures_on_project_id_and_commit_sha' + REDUNDANT_INDEX = 'index_ssh_signatures_on_project_id' + + def up + add_concurrent_index :ssh_signatures, [:project_id, :commit_sha], unique: true, name: NEW_COMPOUND_INDEX + remove_concurrent_index_by_name :ssh_signatures, OLD_UNIQUE_INDEX + remove_concurrent_index_by_name :ssh_signatures, REDUNDANT_INDEX + end + + def down + add_concurrent_index :ssh_signatures, :commit_sha, unique: true, name: OLD_UNIQUE_INDEX + add_concurrent_index :ssh_signatures, :project_id, name: REDUNDANT_INDEX + remove_concurrent_index_by_name :ssh_signatures, NEW_COMPOUND_INDEX + end +end diff --git a/db/schema_migrations/20251202160003 b/db/schema_migrations/20251202160003 new file mode 100644 index 0000000000000000000000000000000000000000..e6bd584b7bc2457ec283d5ee220e68cbadf7125d --- /dev/null +++ b/db/schema_migrations/20251202160003 @@ -0,0 +1 @@ +5a34f0c657661d64e9c04d7a980e4661742a754c3bdd2dc0f508eb07ab273821 \ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index 1f138366289db45c5d5614bdf4574ae4c16099dc..c3c6c184e925d2969892d5e6c6f49b3a9a4ca513 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -44153,11 +44153,9 @@ CREATE INDEX index_sprints_on_title ON sprints USING btree (title); CREATE INDEX index_sprints_on_title_trigram ON sprints USING gin (title gin_trgm_ops); -CREATE UNIQUE INDEX index_ssh_signatures_on_commit_sha ON ssh_signatures USING btree (commit_sha); - CREATE INDEX index_ssh_signatures_on_key_id ON ssh_signatures USING btree (key_id); -CREATE INDEX index_ssh_signatures_on_project_id ON ssh_signatures USING btree (project_id); +CREATE UNIQUE INDEX index_ssh_signatures_on_project_id_and_commit_sha ON ssh_signatures USING btree (project_id, commit_sha); CREATE INDEX index_ssh_signatures_on_user_id ON ssh_signatures USING btree (user_id); diff --git a/lib/gitlab/ssh/commit.rb b/lib/gitlab/ssh/commit.rb index ceeb606c8236b11af254f1574e8914d92769feea..c89c1576338b9399f9f31ec4cd0d085128263127 100644 --- a/lib/gitlab/ssh/commit.rb +++ b/lib/gitlab/ssh/commit.rb @@ -3,8 +3,22 @@ module Gitlab module Ssh class Commit < Gitlab::Repositories::BaseSignedCommit + extend ::Gitlab::Utils::Override + private + override :lazy_signature + def lazy_signature + BatchLoader.for([@commit.project.id, @commit.sha]).batch do |project_sha_pairs, loader| + project_ids = project_sha_pairs.map(&:first).uniq + shas = project_sha_pairs.map(&:last).uniq + + signature_class.by_commit_shas_and_project_ids(shas, project_ids).each do |signature| + loader.call([signature.project_id, signature.commit_sha], signature) + end + end + end + def signature_class CommitSignatures::SshSignature end diff --git a/spec/lib/gitlab/ssh/commit_spec.rb b/spec/lib/gitlab/ssh/commit_spec.rb index 2df300cd034b7bffda0f4316330975796cebaee0..904de337d2aa27de20284d2b2c6f496c5a153cda 100644 --- a/spec/lib/gitlab/ssh/commit_spec.rb +++ b/spec/lib/gitlab/ssh/commit_spec.rb @@ -167,4 +167,102 @@ ) end end + + describe '#lazy_signature' do + let_it_be(:project1) { create(:project, :repository) } + let_it_be(:project2) { create(:project, :repository) } + let_it_be(:commit1) { create(:commit, project: project1, sha: '1234567890abcdef1234567890abcdef12345678') } + let_it_be(:commit2) { create(:commit, project: project1, sha: 'abcdef1234567890abcdef1234567890abcdef12') } + let_it_be(:commit3) { create(:commit, project: project2, sha: 'fedcba0987654321fedcba0987654321fedcba09') } + let_it_be(:commit4) { create(:commit, project: project2, sha: '1234567890abcdef1234567890abcdef12345678') } + + let_it_be(:signature1) do + create( + :ssh_signature, + project: project1, + commit_sha: commit1.sha, + verification_status: :verified + ) + end + + let_it_be(:signature2) do + create( + :ssh_signature, + project: project1, + commit_sha: commit2.sha, + verification_status: :verified + ) + end + + let_it_be(:signature3) do + create( + :ssh_signature, + project: project2, + commit_sha: commit3.sha, + verification_status: :verified + ) + end + + before do + allow(Gitlab::Git::Commit).to receive(:extract_signature_lazily).and_return(nil) + end + + it 'batches signature loading by project_id and commit_sha pairs' do + ssh_commit1 = described_class.new(commit1) + ssh_commit2 = described_class.new(commit2) + ssh_commit3 = described_class.new(commit3) + + # Expect a single batched query for all signatures + expect(CommitSignatures::SshSignature).to receive(:by_commit_shas_and_project_ids).once.and_call_original + + sig1 = ssh_commit1.send(:lazy_signature).itself + sig2 = ssh_commit2.send(:lazy_signature).itself + sig3 = ssh_commit3.send(:lazy_signature).itself + + # Verify each commit gets its correct signature + expect(sig1).to eq(signature1) + expect(sig1.project_id).to eq(project1.id) + expect(sig1.commit_sha).to eq(commit1.sha) + + expect(sig2).to eq(signature2) + expect(sig2.project_id).to eq(project1.id) + expect(sig2.commit_sha).to eq(commit2.sha) + + expect(sig3).to eq(signature3) + expect(sig3.project_id).to eq(project2.id) + expect(sig3.commit_sha).to eq(commit3.sha) + end + + it 'correctly maps signatures to commits with same commit_sha in different projects' do + same_sha = '1111111111111111111111111111111111111111' + commit_proj1 = create(:commit, project: project1, sha: same_sha) + commit_proj2 = create(:commit, project: project2, sha: same_sha) + sig_proj1 = create(:ssh_signature, project: project1, commit_sha: same_sha) + sig_proj2 = create(:ssh_signature, project: project2, commit_sha: same_sha) + + ssh_commit1 = described_class.new(commit_proj1) + ssh_commit2 = described_class.new(commit_proj2) + + loaded_sig1 = ssh_commit1.send(:lazy_signature).itself + loaded_sig2 = ssh_commit2.send(:lazy_signature).itself + + # Each commit should get its own project's signature, not the other's + expect(loaded_sig1).to eq(sig_proj1) + expect(loaded_sig1.project_id).to eq(project1.id) + + expect(loaded_sig2).to eq(sig_proj2) + expect(loaded_sig2.project_id).to eq(project2.id) + + # Verify they're different signatures + expect(loaded_sig1).not_to eq(loaded_sig2) + expect(loaded_sig1.commit_sha).to eq(loaded_sig2.commit_sha) + end + + it 'returns nil when no signature exists for a commit' do + commit_without_sig = create(:commit, project: project1, sha: 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa') + ssh_commit = described_class.new(commit_without_sig) + + expect(ssh_commit.send(:lazy_signature).itself).to be_nil + end + end end diff --git a/spec/models/commit_signatures/gpg_signature_spec.rb b/spec/models/commit_signatures/gpg_signature_spec.rb index 225191359e97f8277dc6822905d79b0d2b33b597..d82b5ecb655cab622b666d43ab10d51b09f6a994 100644 --- a/spec/models/commit_signatures/gpg_signature_spec.rb +++ b/spec/models/commit_signatures/gpg_signature_spec.rb @@ -22,7 +22,11 @@ end it_behaves_like 'having unique enum values' - it_behaves_like 'commit signature' + + it_behaves_like 'commit signature' do + let(:signature_attributes) { { commit_sha: commit_sha } } + end + it_behaves_like 'signature with type checking', :gpg describe 'associations' do diff --git a/spec/models/commit_signatures/ssh_signature_spec.rb b/spec/models/commit_signatures/ssh_signature_spec.rb index 6054767ea4484a244c6d095742684a63033a2d30..25d1881cb46028ee10eb7baa03517513d4a9fb6b 100644 --- a/spec/models/commit_signatures/ssh_signature_spec.rb +++ b/spec/models/commit_signatures/ssh_signature_spec.rb @@ -30,7 +30,23 @@ end it_behaves_like 'having unique enum values' - it_behaves_like 'commit signature' + + it_behaves_like 'commit signature' do + let(:signature_attributes) { { commit_sha: commit_sha, project: signature.project } } + + it 'creates separate signatures for the same commit_sha in different projects' do + project2 = create(:project, :repository) + + signature1 = described_class.safe_create!(attributes.merge(project: project)) + signature2 = described_class.safe_create!(attributes.merge(project: project2)) + + expect(signature1).not_to eq(signature2) + expect(signature1.project_id).to eq(project.id) + expect(signature2.project_id).to eq(project2.id) + expect(signature1.commit_sha).to eq(signature2.commit_sha) + end + end + it_behaves_like 'signature with type checking', :ssh describe 'associations' do diff --git a/spec/models/commit_signatures/x509_commit_signature_spec.rb b/spec/models/commit_signatures/x509_commit_signature_spec.rb index 6b55d57870aee47806af4f06a0aa80b104ae28a6..47b0e31077f95afe9f3c5282fe28ea8457dfaf87 100644 --- a/spec/models/commit_signatures/x509_commit_signature_spec.rb +++ b/spec/models/commit_signatures/x509_commit_signature_spec.rb @@ -26,7 +26,11 @@ let(:signature) { create(:x509_commit_signature, commit_sha: commit_sha, x509_certificate: x509_certificate) } it_behaves_like 'having unique enum values' - it_behaves_like 'commit signature' + + it_behaves_like 'commit signature' do + let(:signature_attributes) { { commit_sha: commit_sha } } + end + it_behaves_like 'signature with type checking', :x509 describe 'validation' do diff --git a/spec/support/shared_examples/models/commit_signature_shared_examples.rb b/spec/support/shared_examples/models/commit_signature_shared_examples.rb index 56d5c1da3afa5972caec6a715da71936d3affbb9..654905f0dd37f6346fd17956da71415b718c79f0 100644 --- a/spec/support/shared_examples/models/commit_signature_shared_examples.rb +++ b/spec/support/shared_examples/models/commit_signature_shared_examples.rb @@ -13,10 +13,10 @@ end describe '.safe_create!' do - it 'finds a signature by commit sha if it existed' do + it 'finds a signature by existent unique attributes' do signature - expect(described_class.safe_create!(commit_sha: commit_sha)).to eq(signature) + expect(described_class.safe_create!(signature_attributes)).to eq(signature) end it 'creates a new signature if it was not found' do