Optimise import performanceoptimise-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`

10 files changed, 32 insertions, 34 deletions

diff --git a/app/models/project.rb b/app/models/project.rb
index ece7507e55c..08221642295 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -1854,16 +1854,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
@@ -2252,18 +2260,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 00000000000..c63f44d5109
--- /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 409243e68a5..42cd94add79 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 b145f37c052..a92e3862361 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 a154de5419e..71e9064c5fd 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 1b545b1d049..0be49e27acb 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 babc93a83e5..07eaff2da09 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 1a561e81e4a..66f5bb4c87b 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 3b7de185cf1..b9f6595762b 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 bcb2da7eed2..da9e204d4ca 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -3097,11 +3097,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