Commit 08cf5f7f authored by Igor Drozdov's avatar Igor Drozdov

Reduce SQL requests number for CreateCommitSignatureWorker

Currently, the signatures are calculated as

```
commits = commits.map do |commit|
  case commit.signature_type
    when :PGP
      Gitlab::Gpg::Commit.new(commit).signature
    when :X509
      Gitlab::X509::Commit.new(commit).signature
  end
end
```

In this case, the `lazy_signature&.itself` is directly called
for every commit, but at that point, `BatchLoader` doesn't the
information about a single commit and performs the query on every call.

If we split the code into two steps: initialization and invocation:

```
commits = commits.map do |commit|
  case commit.signature_type
    when :PGP
      Gitlab::Gpg::Commit.new(commit)
    when :X509
      Gitlab::X509::Commit.new(commit)
  end
end
```

will call `lazy_signature` for every commit, i.e store the information
for a single one so on the invocation step:

```
commits.each { |commit| commit.signature }
```

`lazy_signature&.itself` will perform a single batch query since
it already has the info about a single commit
parent abfa2349
......@@ -21,14 +21,19 @@ class CreateCommitSignatureWorker # rubocop:disable Scalability/IdempotentWorker
return if commits.empty?
# This calculates and caches the signature in the database
commits.each do |commit|
# Instantiate commits first to lazily load the signatures
commits.map! do |commit|
case commit.signature_type
when :PGP
Gitlab::Gpg::Commit.new(commit).signature
Gitlab::Gpg::Commit.new(commit)
when :X509
Gitlab::X509::Commit.new(commit).signature
Gitlab::X509::Commit.new(commit)
end
end
# This calculates and caches the signature in the database
commits.each do |commit|
commit&.signature
rescue => e
Rails.logger.error("Failed to create signature for commit #{commit.id}. Error: #{e.message}") # rubocop:disable Gitlab/RailsLogger
end
......
---
title: Reduce SQL requests number for CreateCommitSignatureWorker
merge_request: 29479
author:
type: performance
......@@ -4,6 +4,8 @@ module Gitlab
class SignedCommit
include Gitlab::Utils::StrongMemoize
delegate :id, to: :@commit
def initialize(commit)
@commit = commit
......
......@@ -9,14 +9,14 @@ describe CreateCommitSignatureWorker do
let(:gpg_commit) { instance_double(Gitlab::Gpg::Commit) }
let(:x509_commit) { instance_double(Gitlab::X509::Commit) }
context 'when a signature is found' do
before do
allow(Project).to receive(:find_by).with(id: project.id).and_return(project)
allow(project).to receive(:commits_by).with(oids: commit_shas).and_return(commits)
end
before do
allow(Project).to receive(:find_by).with(id: project.id).and_return(project)
allow(project).to receive(:commits_by).with(oids: commit_shas).and_return(commits)
end
subject { described_class.new.perform(commit_shas, project.id) }
subject { described_class.new.perform(commit_shas, project.id) }
context 'when a signature is found' do
it 'calls Gitlab::Gpg::Commit#signature' do
commits.each do |commit|
allow(commit).to receive(:signature_type).and_return(:PGP)
......@@ -67,9 +67,10 @@ describe CreateCommitSignatureWorker do
end
context 'handles when a string is passed in for the commit SHA' do
let(:commit_shas) { super().first }
before do
allow(Project).to receive(:find_by).with(id: project.id).and_return(project)
allow(project).to receive(:commits_by).with(oids: Array(commit_shas.first)).and_return(commits)
allow(project).to receive(:commits_by).with(oids: [commit_shas]).and_return(commits)
allow(commits.first).to receive(:signature_type).and_return(:PGP)
end
......@@ -78,35 +79,65 @@ describe CreateCommitSignatureWorker do
expect(gpg_commit).to receive(:signature).once
described_class.new.perform(commit_shas.first, project.id)
subject
end
end
context 'when Commit is not found' do
let(:nonexisting_commit_sha) { '0beec7b5ea3f0fdbc95d0dd47f3c5bc275da8a34' }
let(:commit_shas) { [nonexisting_commit_sha] }
it 'does not raise errors' do
expect { described_class.new.perform([nonexisting_commit_sha], project.id) }.not_to raise_error
expect { described_class.new.perform(commit_shas, project.id) }.not_to raise_error
end
end
context 'when Project is not found' do
let(:nonexisting_project_id) { -1 }
let(:commits) { [] }
let(:project) { double(id: non_existing_record_id) }
it 'does not raise errors' do
expect { described_class.new.perform(commit_shas, nonexisting_project_id) }.not_to raise_error
expect { subject }.not_to raise_error
end
it 'does not call Gitlab::Gpg::Commit#signature' do
expect_any_instance_of(Gitlab::Gpg::Commit).not_to receive(:signature)
described_class.new.perform(commit_shas, nonexisting_project_id)
subject
end
it 'does not call Gitlab::X509::Commit#signature' do
expect_any_instance_of(Gitlab::X509::Commit).not_to receive(:signature)
described_class.new.perform(commit_shas, nonexisting_project_id)
subject
end
end
context 'fetching signatures' do
before do
commits.each do |commit|
allow(commit).to receive(:signature_type).and_return(type)
end
end
context 'X509' do
let(:type) { :X509 }
it 'performs a single query for commit signatures' do
expect(X509CommitSignature).to receive(:by_commit_sha).with(commit_shas).once.and_return([])
subject
end
end
context 'PGP' do
let(:type) { :PGP }
it 'performs a single query for commit signatures' do
expect(GpgSignature).to receive(:by_commit_sha).with(commit_shas).once.and_return([])
subject
end
end
end
end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment