Commit 8d1e97fc authored by Kamil Trzciński's avatar Kamil Trzciński

Optimise import performance

- Fix `O(n)` complexity of `append_or_update_attribute`,
  we append objects to an array and re-save project
- Remove the usage of `keys.include?` as it performs `O(n)`
  search, instead use `.has_key?`
- Remove the usage of `.keys.first` as it performs a copy
  of all keys, instead use `.first.first`
parent 5e102f17
...@@ -1854,16 +1854,24 @@ class Project < ApplicationRecord ...@@ -1854,16 +1854,24 @@ class Project < ApplicationRecord
end end
def append_or_update_attribute(name, value) def append_or_update_attribute(name, value)
old_values = public_send(name.to_s) # rubocop:disable GitlabSecurity/PublicSend if Project.reflect_on_association(name).try(:macro) == :has_many
# if this is 1-to-N relation, update the parent object
value.each do |item|
item.update!(
Project.reflect_on_association(name).foreign_key => id)
end
# force to drop relation cache
public_send(name).reset # rubocop:disable GitlabSecurity/PublicSend
if Project.reflect_on_association(name).try(:macro) == :has_many && old_values.any? # succeeded
update_attribute(name, old_values + value) true
else else
# if this is another relation or attribute, update just object
update_attribute(name, value) update_attribute(name, value)
end end
rescue ActiveRecord::RecordInvalid => e
rescue ActiveRecord::RecordNotSaved => e raise e, "Failed to set #{name}: #{e.message}"
handle_update_attribute_error(e, value)
end end
# Tries to set repository as read_only, checking for existing Git transfers in progress beforehand # Tries to set repository as read_only, checking for existing Git transfers in progress beforehand
...@@ -2252,18 +2260,6 @@ class Project < ApplicationRecord ...@@ -2252,18 +2260,6 @@ class Project < ApplicationRecord
ContainerRepository.build_root_repository(self).has_tags? ContainerRepository.build_root_repository(self).has_tags?
end end
def handle_update_attribute_error(ex, value)
if ex.message.start_with?('Failed to replace')
if value.respond_to?(:each)
invalid = value.detect(&:invalid?)
raise ex, ([ex.message] + invalid.errors.full_messages).join(' ') if invalid
end
end
raise ex
end
def fetch_branch_allows_collaboration(user, branch_name = nil) def fetch_branch_allows_collaboration(user, branch_name = nil)
return false unless user return false unless user
......
---
title: Optimise import performance
merge_request: 31045
author:
type: performance
...@@ -45,7 +45,7 @@ module Gitlab ...@@ -45,7 +45,7 @@ module Gitlab
end end
def key_from_hash(value) def key_from_hash(value)
value.is_a?(Hash) ? value.keys.first : value value.is_a?(Hash) ? value.first.first : value
end end
end end
end end
......
...@@ -27,7 +27,7 @@ module Gitlab ...@@ -27,7 +27,7 @@ module Gitlab
# {:merge_requests=>[:merge_request_diff, :notes]} # {:merge_requests=>[:merge_request_diff, :notes]}
def process_model_objects(model_object_hash) def process_model_objects(model_object_hash)
json_config_hash = {} json_config_hash = {}
current_key = model_object_hash.keys.first current_key = model_object_hash.first.first
model_object_hash.values.flatten.each do |model_object| model_object_hash.values.flatten.each do |model_object|
@attributes_finder.parse(current_key) { |hash| json_config_hash[current_key] ||= hash } @attributes_finder.parse(current_key) { |hash| json_config_hash[current_key] ||= hash }
......
...@@ -35,7 +35,7 @@ module Gitlab ...@@ -35,7 +35,7 @@ module Gitlab
end end
def include?(old_author_id) def include?(old_author_id)
map.keys.include?(old_author_id) && map[old_author_id] != default_user_id map.has_key?(old_author_id) && map[old_author_id] != default_user_id
end end
private private
......
...@@ -185,7 +185,7 @@ module Gitlab ...@@ -185,7 +185,7 @@ module Gitlab
return unless EXISTING_OBJECT_CHECK.include?(@relation_name) return unless EXISTING_OBJECT_CHECK.include?(@relation_name)
return unless @relation_hash['group_id'] return unless @relation_hash['group_id']
@relation_hash['group_id'] = @project.group&.id @relation_hash['group_id'] = @project.namespace_id
end end
def reset_tokens! def reset_tokens!
......
...@@ -13,7 +13,7 @@ describe UserCalloutsController do ...@@ -13,7 +13,7 @@ describe UserCalloutsController do
subject { post :create, params: { feature_name: feature_name }, format: :json } subject { post :create, params: { feature_name: feature_name }, format: :json }
context 'with valid feature name' do context 'with valid feature name' do
let(:feature_name) { UserCallout.feature_names.keys.first } let(:feature_name) { UserCallout.feature_names.first.first }
context 'when callout entry does not exist' do context 'when callout entry does not exist' do
it 'creates a callout entry with dismissed state' do it 'creates a callout entry with dismissed state' do
...@@ -28,7 +28,7 @@ describe UserCalloutsController do ...@@ -28,7 +28,7 @@ describe UserCalloutsController do
end end
context 'when callout entry already exists' do context 'when callout entry already exists' do
let!(:callout) { create(:user_callout, feature_name: UserCallout.feature_names.keys.first, user: user) } let!(:callout) { create(:user_callout, feature_name: UserCallout.feature_names.first.first, user: user) }
it 'returns success' do it 'returns success' do
subject subject
......
...@@ -19,7 +19,7 @@ ...@@ -19,7 +19,7 @@
"labels": [ "labels": [
{ {
"id": 2, "id": 2,
"title": "project label", "title": "A project label",
"color": "#428bca", "color": "#428bca",
"project_id": 8, "project_id": 8,
"created_at": "2016-07-22T08:55:44.161Z", "created_at": "2016-07-22T08:55:44.161Z",
...@@ -105,7 +105,7 @@ ...@@ -105,7 +105,7 @@
"updated_at": "2017-08-15T18:37:40.795Z", "updated_at": "2017-08-15T18:37:40.795Z",
"label": { "label": {
"id": 6, "id": 6,
"title": "project label", "title": "A project label",
"color": "#A8D695", "color": "#A8D695",
"project_id": null, "project_id": null,
"created_at": "2017-08-15T18:37:19.698Z", "created_at": "2017-08-15T18:37:19.698Z",
...@@ -162,7 +162,7 @@ ...@@ -162,7 +162,7 @@
"updated_at": "2017-08-15T18:37:40.795Z", "updated_at": "2017-08-15T18:37:40.795Z",
"label": { "label": {
"id": 2, "id": 2,
"title": "project label", "title": "A project label",
"color": "#A8D695", "color": "#A8D695",
"project_id": null, "project_id": null,
"created_at": "2017-08-15T18:37:19.698Z", "created_at": "2017-08-15T18:37:19.698Z",
......
...@@ -272,7 +272,7 @@ describe Gitlab::ImportExport::ProjectTreeRestorer do ...@@ -272,7 +272,7 @@ describe Gitlab::ImportExport::ProjectTreeRestorer do
end end
it 'has label priorities' do it 'has label priorities' do
expect(project.labels.first.priorities).not_to be_empty expect(project.labels.find_by(title: 'A project label').priorities).not_to be_empty
end end
it 'has milestones' do it 'has milestones' do
...@@ -325,7 +325,7 @@ describe Gitlab::ImportExport::ProjectTreeRestorer do ...@@ -325,7 +325,7 @@ describe Gitlab::ImportExport::ProjectTreeRestorer do
it_behaves_like 'restores project correctly', it_behaves_like 'restores project correctly',
issues: 1, issues: 1,
labels: 1, labels: 2,
milestones: 1, milestones: 1,
first_issue_labels: 1, first_issue_labels: 1,
services: 1 services: 1
...@@ -402,7 +402,7 @@ describe Gitlab::ImportExport::ProjectTreeRestorer do ...@@ -402,7 +402,7 @@ describe Gitlab::ImportExport::ProjectTreeRestorer do
it_behaves_like 'restores project successfully' it_behaves_like 'restores project successfully'
it_behaves_like 'restores project correctly', it_behaves_like 'restores project correctly',
issues: 2, issues: 2,
labels: 1, labels: 2,
milestones: 2, milestones: 2,
first_issue_labels: 1 first_issue_labels: 1
......
...@@ -3097,11 +3097,8 @@ describe Project do ...@@ -3097,11 +3097,8 @@ describe Project do
let(:project) { create(:project) } let(:project) { create(:project) }
it 'shows full error updating an invalid MR' do it 'shows full error updating an invalid MR' do
error_message = 'Failed to replace merge_requests because one or more of the new records could not be saved.'\
' Validate fork Source project is not a fork of the target project'
expect { project.append_or_update_attribute(:merge_requests, [create(:merge_request)]) } expect { project.append_or_update_attribute(:merge_requests, [create(:merge_request)]) }
.to raise_error(ActiveRecord::RecordNotSaved, error_message) .to raise_error(ActiveRecord::RecordInvalid, /Failed to set merge_requests:/)
end end
it 'updates the project successfully' do it 'updates the project successfully' do
......
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