Merge branch 'mark-some-as-not-required-during-import' into 'master'

Add `importing?` to disable some callbacks

See merge request gitlab-org/gitlab!22781

app/models/ci/pipeline.rb

@@ -26,7 +26,7 @@ module Ci
     belongs_to :merge_request, class_name: 'MergeRequest'
     belongs_to :external_pull_request
 
-    has_internal_id :iid, scope: :project, presence: false, ensure_if: -> { !importing? }, init: ->(s) do
+    has_internal_id :iid, scope: :project, presence: false, track_if: -> { !importing? }, ensure_if: -> { !importing? }, init: ->(s) do
       s&.project&.all_pipelines&.maximum(:iid) || s&.project&.all_pipelines&.count
     end
 

app/models/concerns/atomic_internal_id.rb

@@ -27,13 +27,13 @@ module AtomicInternalId
   extend ActiveSupport::Concern
 
   class_methods do
-    def has_internal_id(column, scope:, init:, ensure_if: nil, presence: true) # rubocop:disable Naming/PredicateName
+    def has_internal_id(column, scope:, init:, ensure_if: nil, track_if: nil, presence: true) # rubocop:disable Naming/PredicateName
       # We require init here to retain the ability to recalculate in the absence of a
-      # InternaLId record (we may delete records in `internal_ids` for example).
+      # InternalId record (we may delete records in `internal_ids` for example).
       raise "has_internal_id requires a init block, none given." unless init
       raise "has_internal_id needs to be defined on association." unless self.reflect_on_association(scope)
 
-      before_validation :"track_#{scope}_#{column}!", on: :create
+      before_validation :"track_#{scope}_#{column}!", on: :create, if: track_if
       before_validation :"ensure_#{scope}_#{column}!", on: :create, if: ensure_if
       validates column, presence: presence
 

app/models/deployment.rb

@@ -5,6 +5,7 @@ class Deployment < ApplicationRecord
   include IidRoutes
   include AfterCommitQueue
   include UpdatedAtFilterable
+  include Importable
   include Gitlab::Utils::StrongMemoize
 
   belongs_to :project, required: true
@@ -17,7 +18,7 @@ class Deployment < ApplicationRecord
   has_many :merge_requests,
     through: :deployment_merge_requests
 
-  has_internal_id :iid, scope: :project, init: ->(s) do
+  has_internal_id :iid, scope: :project, track_if: -> { !importing? }, init: ->(s) do
     Deployment.where(project: s.project).maximum(:iid) if s&.project
   end
 

app/models/issue.rb

@@ -31,7 +31,7 @@ class Issue < ApplicationRecord
   belongs_to :duplicated_to, class_name: 'Issue'
   belongs_to :closed_by, class_name: 'User'
 
-  has_internal_id :iid, scope: :project, init: ->(s) { s&.project&.issues&.maximum(:iid) }
+  has_internal_id :iid, scope: :project, track_if: -> { !importing? }, init: ->(s) { s&.project&.issues&.maximum(:iid) }
 
   has_many :issue_milestones
   has_many :milestones, through: :issue_milestones
@@ -78,8 +78,8 @@ class Issue < ApplicationRecord
 
   ignore_column :state, remove_with: '12.7', remove_after: '2019-12-22'
 
-  after_commit :expire_etag_cache
-  after_save :ensure_metrics, unless: :imported?
+  after_commit :expire_etag_cache, unless: :importing?
+  after_save :ensure_metrics, unless: :importing?
 
   attr_spammable :title, spam_title: true
   attr_spammable :description, spam_description: true

app/models/merge_request.rb

@@ -31,7 +31,7 @@ class MergeRequest < ApplicationRecord
   belongs_to :source_project, class_name: "Project"
   belongs_to :merge_user, class_name: "User"
 
-  has_internal_id :iid, scope: :target_project, init: ->(s) { s&.target_project&.merge_requests&.maximum(:iid) }
+  has_internal_id :iid, scope: :target_project, track_if: -> { !importing? }, init: ->(s) { s&.target_project&.merge_requests&.maximum(:iid) }
 
   has_many :merge_request_diffs
 
@@ -97,8 +97,8 @@ class MergeRequest < ApplicationRecord
   after_create :ensure_merge_request_diff
   after_update :clear_memoized_shas
   after_update :reload_diff_if_branch_changed
-  after_save :ensure_metrics
-  after_commit :expire_etag_cache
+  after_save :ensure_metrics, unless: :importing?
+  after_commit :expire_etag_cache, unless: :importing?
 
   # When this attribute is true some MR validation is ignored
   # It allows us to close or modify broken merge requests

app/models/merge_request_diff.rb

@@ -138,7 +138,7 @@ class MergeRequestDiff < ApplicationRecord
   # All diff information is collected from repository after object is created.
   # It allows you to override variables like head_commit_sha before getting diff.
   after_create :save_git_content, unless: :importing?
-  after_create_commit :set_as_latest_diff
+  after_create_commit :set_as_latest_diff, unless: :importing?
 
   after_save :update_external_diff_store, if: -> { !importing? && saved_change_to_external_diff? }
 

app/models/milestone.rb

@@ -17,6 +17,7 @@ class Milestone < ApplicationRecord
   include StripAttribute
   include Milestoneish
   include FromUnion
+  include Importable
   include Gitlab::SQL::Pattern
 
   prepend_if_ee('::EE::Milestone') # rubocop: disable Cop/InjectEnterpriseEditionModule
@@ -30,8 +31,8 @@ class Milestone < ApplicationRecord
   has_many :milestone_releases
   has_many :releases, through: :milestone_releases
 
-  has_internal_id :iid, scope: :project, init: ->(s) { s&.project&.milestones&.maximum(:iid) }
-  has_internal_id :iid, scope: :group, init: ->(s) { s&.group&.milestones&.maximum(:iid) }
+  has_internal_id :iid, scope: :project, track_if: -> { !importing? }, init: ->(s) { s&.project&.milestones&.maximum(:iid) }
+  has_internal_id :iid, scope: :group, track_if: -> { !importing? }, init: ->(s) { s&.group&.milestones&.maximum(:iid) }
 
   has_many :issues
   has_many :labels, -> { distinct.reorder('labels.title') }, through: :issues

app/models/release.rb

@@ -3,6 +3,7 @@
 class Release < ApplicationRecord
   include Presentable
   include CacheMarkdownField
+  include Importable
   include Gitlab::Utils::StrongMemoize
 
   cache_markdown_field :description
@@ -33,8 +34,8 @@ class Release < ApplicationRecord
 
   delegate :repository, to: :project
 
-  after_commit :create_evidence!, on: :create
-  after_commit :notify_new_release, on: :create
+  after_commit :create_evidence!, on: :create, unless: :importing?
+  after_commit :notify_new_release, on: :create, unless: :importing?
 
   MAX_NUMBER_TO_DISPLAY = 3
 

changelogs/unreleased/mark-some-as-not-required-during-import.yml

+---
+title: Add `importing?` to disable some callbacks
+merge_request:
+author:
+type: performance

lib/gitlab/import/merge_request_helpers.rb

@@ -60,6 +60,7 @@ module Gitlab
         diff.importing = true
         diff.save
         diff.save_git_content
+        diff.set_as_latest_diff
       end
     end
   end

spec/models/concerns/atomic_internal_id_spec.rb

@@ -9,6 +9,32 @@ describe AtomicInternalId do
   let(:scope_attrs) { { project: milestone.project } }
   let(:usage) { :milestones }
 
+  describe '#save!' do
+    context 'when IID is provided' do
+      before do
+        milestone.iid = external_iid
+      end
+
+      it 'tracks the value' do
+        expect(milestone).to receive(:track_project_iid!)
+
+        milestone.save!
+      end
+
+      context 'when importing' do
+        before do
+          milestone.importing = true
+        end
+
+        it 'does not track the value' do
+          expect(milestone).not_to receive(:track_project_iid!)
+
+          milestone.save!
+        end
+      end
+    end
+  end
+
   describe '#track_project_iid!' do
     subject { milestone.track_project_iid! }