diff --git a/app/models/project.rb b/app/models/project.rb index 8dfe22122820c556c156d0e319d8f06639448af2..482215e841fa6622fce53e44a7a69df9d44f552e 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -1869,16 +1869,24 @@ class Project < ApplicationRecord end 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? - update_attribute(name, old_values + value) + # succeeded + true else + # if this is another relation or attribute, update just object update_attribute(name, value) end - - rescue ActiveRecord::RecordNotSaved => e - handle_update_attribute_error(e, value) + rescue ActiveRecord::RecordInvalid => e + raise e, "Failed to set #{name}: #{e.message}" end # Tries to set repository as read_only, checking for existing Git transfers in progress beforehand @@ -2267,18 +2275,6 @@ class Project < ApplicationRecord ContainerRepository.build_root_repository(self).has_tags? 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) return false unless user diff --git a/changelogs/unreleased/optimise-import-performance.yml b/changelogs/unreleased/optimise-import-performance.yml new file mode 100644 index 0000000000000000000000000000000000000000..c63f44d51093bcd4ee2e4ff38e5b2e87755bea56 --- /dev/null +++ b/changelogs/unreleased/optimise-import-performance.yml @@ -0,0 +1,5 @@ +--- +title: Optimise import performance +merge_request: 31045 +author: +type: performance diff --git a/lib/gitlab/import_export/attributes_finder.rb b/lib/gitlab/import_export/attributes_finder.rb index 409243e68a53864746b45d7f95ac1c7c3411b8de..42cd94add794686a862dc2b7c4351644293e7fc0 100644 --- a/lib/gitlab/import_export/attributes_finder.rb +++ b/lib/gitlab/import_export/attributes_finder.rb @@ -45,7 +45,7 @@ module Gitlab end def key_from_hash(value) - value.is_a?(Hash) ? value.keys.first : value + value.is_a?(Hash) ? value.first.first : value end end end diff --git a/lib/gitlab/import_export/json_hash_builder.rb b/lib/gitlab/import_export/json_hash_builder.rb index b145f37c052e9159f2b2c4e977a27c556ae0d73b..a92e38623618de54778daab5f7c7a9e1d39f614a 100644 --- a/lib/gitlab/import_export/json_hash_builder.rb +++ b/lib/gitlab/import_export/json_hash_builder.rb @@ -27,7 +27,7 @@ module Gitlab # {:merge_requests=>[:merge_request_diff, :notes]} def process_model_objects(model_object_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| @attributes_finder.parse(current_key) { |hash| json_config_hash[current_key] ||= hash } diff --git a/lib/gitlab/import_export/members_mapper.rb b/lib/gitlab/import_export/members_mapper.rb index a154de5419e4f1bed8ffe00ad0f4c72f4e4c3f8a..71e9064c5fd31f8c926f6a6419368d7253c6effe 100644 --- a/lib/gitlab/import_export/members_mapper.rb +++ b/lib/gitlab/import_export/members_mapper.rb @@ -35,7 +35,7 @@ module Gitlab end 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 private diff --git a/lib/gitlab/import_export/relation_factory.rb b/lib/gitlab/import_export/relation_factory.rb index 1b545b1d04939389824ebb84539e46a2a7b980b9..0be49e27acb0a843ab1a5576c79000aa0cca19f1 100644 --- a/lib/gitlab/import_export/relation_factory.rb +++ b/lib/gitlab/import_export/relation_factory.rb @@ -185,7 +185,7 @@ module Gitlab return unless EXISTING_OBJECT_CHECK.include?(@relation_name) return unless @relation_hash['group_id'] - @relation_hash['group_id'] = @project.group&.id + @relation_hash['group_id'] = @project.namespace_id end def reset_tokens! diff --git a/spec/controllers/user_callouts_controller_spec.rb b/spec/controllers/user_callouts_controller_spec.rb index babc93a83e5317f6aa7052964e72654be0ab3757..07eaff2da09b35afb9ded69ee5839e701c048a18 100644 --- a/spec/controllers/user_callouts_controller_spec.rb +++ b/spec/controllers/user_callouts_controller_spec.rb @@ -13,7 +13,7 @@ describe UserCalloutsController do subject { post :create, params: { feature_name: feature_name }, format: :json } 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 it 'creates a callout entry with dismissed state' do @@ -28,7 +28,7 @@ describe UserCalloutsController do end 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 subject diff --git a/spec/lib/gitlab/import_export/project.group.json b/spec/lib/gitlab/import_export/project.group.json index 1a561e81e4aeadff6c6e2ae16d5bc19e00d403be..66f5bb4c87b9ce82134c929cfcb40179199b3b36 100644 --- a/spec/lib/gitlab/import_export/project.group.json +++ b/spec/lib/gitlab/import_export/project.group.json @@ -19,7 +19,7 @@ "labels": [ { "id": 2, - "title": "project label", + "title": "A project label", "color": "#428bca", "project_id": 8, "created_at": "2016-07-22T08:55:44.161Z", @@ -105,7 +105,7 @@ "updated_at": "2017-08-15T18:37:40.795Z", "label": { "id": 6, - "title": "project label", + "title": "A project label", "color": "#A8D695", "project_id": null, "created_at": "2017-08-15T18:37:19.698Z", @@ -162,7 +162,7 @@ "updated_at": "2017-08-15T18:37:40.795Z", "label": { "id": 2, - "title": "project label", + "title": "A project label", "color": "#A8D695", "project_id": null, "created_at": "2017-08-15T18:37:19.698Z", diff --git a/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb b/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb index e6ce3f1bcea92cb1eaabb46e042aacfed2a639fd..2f98d6614f9dd71864e48b74ee3d96e270094c8b 100644 --- a/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb +++ b/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb @@ -272,7 +272,7 @@ describe Gitlab::ImportExport::ProjectTreeRestorer do end 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 it 'has milestones' do @@ -325,7 +325,7 @@ describe Gitlab::ImportExport::ProjectTreeRestorer do it_behaves_like 'restores project correctly', issues: 1, - labels: 1, + labels: 2, milestones: 1, first_issue_labels: 1, services: 1 @@ -402,7 +402,7 @@ describe Gitlab::ImportExport::ProjectTreeRestorer do it_behaves_like 'restores project successfully' it_behaves_like 'restores project correctly', issues: 2, - labels: 1, + labels: 2, milestones: 2, first_issue_labels: 1 diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 927c072be10cc07b244159df7e9bf1994cb0bc5c..e5d08cfb1bb893e228d522af401c8577109bc2c1 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -3117,11 +3117,8 @@ describe Project do let(:project) { create(:project) } 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)]) } - .to raise_error(ActiveRecord::RecordNotSaved, error_message) + .to raise_error(ActiveRecord::RecordInvalid, /Failed to set merge_requests:/) end it 'updates the project successfully' do