Flush InternalId records after import

After the import has finished, we flush (delete) the InternalId records
related to the project at hand. This means we're starting over with
tracking correct internal id values, a new record will be created
automatically when the next internal id is generated.

The GitHub importer assigns iid values by using supplied values from
GitHub. We skip tracking internal id values during the import in favor
of higher concurrency. Deleting any InternalId records after the import
has finished is an extra measure to guarantee consistency.

Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/54270.

4 files changed, 42 insertions, 0 deletions

diff --git a/app/models/internal_id.rb b/app/models/internal_id.rb
index 4eb211eff61..e75c6eb2331 100644
--- a/app/models/internal_id.rb
+++ b/app/models/internal_id.rb
@@ -66,6 +66,17 @@ class InternalId < ActiveRecord::Base
       InternalIdGenerator.new(subject, scope, usage, init).generate
     end
 
+    # Flushing records is generally safe in a sense that those
+    # records are going to be re-created when needed.
+    #
+    # A filter condition has to be provided to not accidentally flush
+    # records for all projects.
+    def flush_records!(filter)
+      raise ArgumentError, "filter cannot be empty" if filter.blank?
+
+      where(filter).delete_all
+    end
+
     def available?
       @available_flag ||= ActiveRecord::Migrator.current_version >= REQUIRED_SCHEMA_VERSION # rubocop:disable Gitlab/PredicateMemoization
     end
diff --git a/app/models/project.rb b/app/models/project.rb
index 15465d9b356..7f6da130658 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -1585,6 +1585,13 @@ class Project < ActiveRecord::Base
   def after_import
     repository.after_import
     wiki.repository.after_import
+
+    # The import assigns iid values on its own, e.g. by re-using GitHub ids.
+    # Flush existing InternalId records for this project for consistency reasons.
+    # Those records are going to be recreated with the next normal creation
+    # of a model instance (e.g. an Issue).
+    InternalId.flush_records!(project: self)
+
     import_state.finish
     import_state.remove_jid
     update_project_counter_caches
diff --git a/spec/models/internal_id_spec.rb b/spec/models/internal_id_spec.rb
index 4696341c05f..d32f163f05b 100644
--- a/spec/models/internal_id_spec.rb
+++ b/spec/models/internal_id_spec.rb
@@ -13,6 +13,29 @@ describe InternalId do
     it { is_expected.to validate_presence_of(:usage) }
   end
 
+  describe '.flush_records!' do
+    subject { described_class.flush_records!(project: project) }
+
+    let(:another_project) { create(:project) }
+
+    before do
+      create_list(:issue, 2, project: project)
+      create_list(:issue, 2, project: another_project)
+    end
+
+    it 'deletes all records for the given project' do
+      expect { subject }.to change { described_class.where(project: project).count }.from(1).to(0)
+    end
+
+    it 'retains records for other projects' do
+      expect { subject }.not_to change { described_class.where(project: another_project).count }
+    end
+
+    it 'does not allow an empty filter' do
+      expect { described_class.flush_records!({}) }.to raise_error(/filter cannot be empty/)
+    end
+  end
+
   describe '.generate_next' do
     subject { described_class.generate_next(issue, scope, usage, init) }
 
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index 4b061b5e24f..7d3f2dfe374 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -3767,6 +3767,7 @@ describe Project do
       expect(import_state).to receive(:remove_jid)
       expect(project).to receive(:after_create_default_branch)
       expect(project).to receive(:refresh_markdown_cache!)
+      expect(InternalId).to receive(:flush_records!).with(project: project)
 
       project.after_import
     end