Improve performance of PR import
This removes unneeded `.reload` call which makes AR to load ALL objects, and create its in-memory representation.
This commit is contained in:
parent
eb377b85de
commit
f3ad51f8a5
3 changed files with 79 additions and 1 deletions
5
changelogs/unreleased/fix-pull-request-importer.yml
Normal file
5
changelogs/unreleased/fix-pull-request-importer.yml
Normal file
|
@ -0,0 +1,5 @@
|
|||
---
|
||||
title: Improve performance of PR import
|
||||
merge_request: 27121
|
||||
author:
|
||||
type: performance
|
|
@ -22,7 +22,7 @@ module Gitlab
|
|||
# additional work that is strictly necessary.
|
||||
merge_request_id = insert_and_return_id(attributes, project.merge_requests)
|
||||
|
||||
merge_request = project.merge_requests.reload.find(merge_request_id)
|
||||
merge_request = project.merge_requests.reset.find(merge_request_id)
|
||||
|
||||
[merge_request, false]
|
||||
end
|
||||
|
|
73
spec/lib/gitlab/import/merge_request_helpers_spec.rb
Normal file
73
spec/lib/gitlab/import/merge_request_helpers_spec.rb
Normal file
|
@ -0,0 +1,73 @@
|
|||
# frozen_string_literal: true
|
||||
|
||||
require 'spec_helper'
|
||||
|
||||
describe Gitlab::Import::MergeRequestHelpers, type: :helper do
|
||||
set(:project) { create(:project, :repository) }
|
||||
set(:user) { create(:user) }
|
||||
|
||||
describe '.create_merge_request_without_hooks' do
|
||||
let(:iid) { 42 }
|
||||
|
||||
let(:attributes) do
|
||||
{
|
||||
iid: iid,
|
||||
title: 'My Pull Request',
|
||||
description: 'This is my pull request',
|
||||
source_project_id: project.id,
|
||||
target_project_id: project.id,
|
||||
source_branch: 'master-42',
|
||||
target_branch: 'master',
|
||||
state: :merged,
|
||||
author_id: user.id,
|
||||
assignee_id: user.id
|
||||
}
|
||||
end
|
||||
|
||||
subject { helper.create_merge_request_without_hooks(project, attributes, iid) }
|
||||
|
||||
context 'when merge request does not exist' do
|
||||
it 'returns a new object' do
|
||||
expect(subject.first).not_to be_nil
|
||||
expect(subject.second).to eq(false)
|
||||
end
|
||||
|
||||
it 'does load all existing objects' do
|
||||
5.times do |iid|
|
||||
MergeRequest.create!(
|
||||
attributes.merge(iid: iid, source_branch: iid.to_s))
|
||||
end
|
||||
|
||||
# does ensure that we only load object twice
|
||||
# 1. by #insert_and_return_id
|
||||
# 2. by project.merge_requests.find
|
||||
expect_any_instance_of(MergeRequest).to receive(:attributes)
|
||||
.twice.times.and_call_original
|
||||
|
||||
expect(subject.first).not_to be_nil
|
||||
expect(subject.second).to eq(false)
|
||||
end
|
||||
end
|
||||
|
||||
context 'when merge request does exist' do
|
||||
before do
|
||||
MergeRequest.create!(attributes)
|
||||
end
|
||||
|
||||
it 'returns an existing object' do
|
||||
expect(subject.first).not_to be_nil
|
||||
expect(subject.second).to eq(true)
|
||||
end
|
||||
end
|
||||
|
||||
context 'when project is deleted' do
|
||||
before do
|
||||
project.delete
|
||||
end
|
||||
|
||||
it 'returns an existing object' do
|
||||
expect(subject.first).to be_nil
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
Loading…
Reference in a new issue