From 82b0d0b5dc2fcf2585b05bf58a072455eb6e103e Mon Sep 17 00:00:00 2001
From: James Nutt <jnutt@gitlab.com>
Date: Fri, 22 Dec 2023 00:14:48 +0000
Subject: [PATCH] Save external_identifiers on ImportFailure for file import
relations
The `FileImport` model has a field for tracking the external identifier
associated with a failed record import. This field is used for other
importers such as GitHub and BitBucket, but not file-based import.
This MR tracks the IID associated with issues, merge requests,
milestones and CI pipelines that fail to import. It also lays the
groundwork for tracking identifiers for other relations, should the
need arise and we can identify a unique key.
Changelog: changed
---
.../layout/space_in_lambda_literal.yml | 1 -
.../style/explicit_block_argument.yml | 1 -
.../style/keyword_parameters_order.yml | 1 -
.../group/relation_tree_restorer.rb | 7 +++-
.../import_export/import_failure_service.rb | 8 ++--
.../tree/project.json | 11 ++++++
.../tree/project/issues.ndjson | 2 +
.../tree/project/milestones.ndjson | 2 +
.../group/relation_tree_restorer_spec.rb | 37 ++++++++++++++++---
.../import_failure_service_spec.rb | 3 +-
.../project/tree_restorer_spec.rb | 18 ++++++++-
.../import_failure_service_shared_examples.rb | 1 +
12 files changed, 78 insertions(+), 14 deletions(-)
create mode 100644 spec/fixtures/lib/gitlab/import_export/with_invalid_issues_and_milestones/tree/project.json
create mode 100644 spec/fixtures/lib/gitlab/import_export/with_invalid_issues_and_milestones/tree/project/issues.ndjson
create mode 100644 spec/fixtures/lib/gitlab/import_export/with_invalid_issues_and_milestones/tree/project/milestones.ndjson
diff --git a/.rubocop_todo/layout/space_in_lambda_literal.yml b/.rubocop_todo/layout/space_in_lambda_literal.yml
index 7e3af689b5f01..e0113d2628d93 100644
--- a/.rubocop_todo/layout/space_in_lambda_literal.yml
+++ b/.rubocop_todo/layout/space_in_lambda_literal.yml
@@ -354,7 +354,6 @@ Layout/SpaceInLambdaLiteral:
- 'lib/gitlab/event_store.rb'
- 'lib/gitlab/gl_repository.rb'
- 'lib/gitlab/health_checks/server.rb'
- - 'lib/gitlab/import_export/import_failure_service.rb'
- 'lib/gitlab/merge_requests/message_generator.rb'
- 'lib/gitlab/metrics/exporter/base_exporter.rb'
- 'lib/gitlab/visibility_level.rb'
diff --git a/.rubocop_todo/style/explicit_block_argument.yml b/.rubocop_todo/style/explicit_block_argument.yml
index db1c8073df8c8..1e5725a2ef0db 100644
--- a/.rubocop_todo/style/explicit_block_argument.yml
+++ b/.rubocop_todo/style/explicit_block_argument.yml
@@ -49,7 +49,6 @@ Style/ExplicitBlockArgument:
- 'lib/gitlab/gitaly_client/storage_settings.rb'
- 'lib/gitlab/github_import/client.rb'
- 'lib/gitlab/graphql/tracers/application_context_tracer.rb'
- - 'lib/gitlab/import_export/import_failure_service.rb'
- 'lib/gitlab/import_export/json/ndjson_writer.rb'
- 'lib/gitlab/import_export/json/streaming_serializer.rb'
- 'lib/gitlab/import_export/project/base_task.rb'
diff --git a/.rubocop_todo/style/keyword_parameters_order.yml b/.rubocop_todo/style/keyword_parameters_order.yml
index e4b751a92da36..8f27a2c9f4584 100644
--- a/.rubocop_todo/style/keyword_parameters_order.yml
+++ b/.rubocop_todo/style/keyword_parameters_order.yml
@@ -18,7 +18,6 @@ Style/KeywordParametersOrder:
- 'lib/gitlab/diff/diff_refs.rb'
- 'lib/gitlab/email/smime/signer.rb'
- 'lib/gitlab/exclusive_lease.rb'
- - 'lib/gitlab/import_export/import_failure_service.rb'
- 'lib/gitlab/merge_requests/mergeability/results_store.rb'
- 'lib/gitlab/usage_data_counters/editor_unique_counter.rb'
- 'lib/microsoft_teams/notifier.rb'
diff --git a/lib/gitlab/import_export/group/relation_tree_restorer.rb b/lib/gitlab/import_export/group/relation_tree_restorer.rb
index 5453792e9043d..ae47c95036ea5 100644
--- a/lib/gitlab/import_export/group/relation_tree_restorer.rb
+++ b/lib/gitlab/import_export/group/relation_tree_restorer.rb
@@ -84,7 +84,8 @@ def process_relation_item!(relation_key, relation_definition, relation_index, da
source: 'process_relation_item!',
relation_key: relation_key,
relation_index: relation_index,
- exception: e)
+ exception: e,
+ external_identifiers: external_identifiers(data_hash))
end
def save_relation_object(relation_object, relation_key, relation_definition, relation_index)
@@ -314,6 +315,10 @@ def log_invalid_subrelations(invalid_subrelations, relation_key)
def importable_column_name
@column_name ||= @importable.class.reflect_on_association(:import_failures).foreign_key.to_sym
end
+
+ def external_identifiers(data_hash)
+ { iid: data_hash['iid'] }.compact
+ end
end
end
end
diff --git a/lib/gitlab/import_export/import_failure_service.rb b/lib/gitlab/import_export/import_failure_service.rb
index bf7200726a1fa..f93822a9d955e 100644
--- a/lib/gitlab/import_export/import_failure_service.rb
+++ b/lib/gitlab/import_export/import_failure_service.rb
@@ -13,7 +13,7 @@ def initialize(importable)
end
def with_retry(action:, relation_key: nil, relation_index: nil)
- on_retry = -> (exception, retry_count, *_args) do
+ on_retry = ->(exception, retry_count, *_args) do
log_import_failure(
source: action,
relation_key: relation_key,
@@ -27,7 +27,8 @@ def with_retry(action:, relation_key: nil, relation_index: nil)
end
end
- def log_import_failure(source:, relation_key: nil, relation_index: nil, exception:, retry_count: 0)
+ def log_import_failure(
+ source:, exception:, relation_key: nil, relation_index: nil, retry_count: 0, external_identifiers: {})
attributes = {
relation_index: relation_index,
source: source,
@@ -45,7 +46,8 @@ def log_import_failure(source:, relation_key: nil, relation_index: nil, exceptio
exception_class: exception.class.to_s,
exception_message: exception.message.truncate(255),
correlation_id_value: Labkit::Correlation::CorrelationId.current_or_new_id,
- relation_key: relation_key
+ relation_key: relation_key,
+ external_identifiers: external_identifiers
)
)
end
diff --git a/spec/fixtures/lib/gitlab/import_export/with_invalid_issues_and_milestones/tree/project.json b/spec/fixtures/lib/gitlab/import_export/with_invalid_issues_and_milestones/tree/project.json
new file mode 100644
index 0000000000000..a522590d037fa
--- /dev/null
+++ b/spec/fixtures/lib/gitlab/import_export/with_invalid_issues_and_milestones/tree/project.json
@@ -0,0 +1,11 @@
+{
+ "id": 5,
+ "description": "Nisi et repellendus ut enim quo accusamus vel magnam.",
+ "import_type": "gitlab_project",
+ "creator_id": 999,
+ "visibility_level": 10,
+ "archived": false,
+ "hooks": [
+
+ ]
+}
diff --git a/spec/fixtures/lib/gitlab/import_export/with_invalid_issues_and_milestones/tree/project/issues.ndjson b/spec/fixtures/lib/gitlab/import_export/with_invalid_issues_and_milestones/tree/project/issues.ndjson
new file mode 100644
index 0000000000000..cf7d9211460c7
--- /dev/null
+++ b/spec/fixtures/lib/gitlab/import_export/with_invalid_issues_and_milestones/tree/project/issues.ndjson
@@ -0,0 +1,2 @@
+{"id":39,"author_id":22,"project_id":null,"created_at":"2016-06-14T15:02:08.233Z","updated_at":"2016-06-14T15:02:48.194Z","position":0,"branch_name":null,"description":"Voluptate vel reprehenderit facilis omnis voluptas magnam tenetur.","state":"opened","iid":9,"updated_by_id":null,"confidential":false,"due_date":"2020-08-14","moved_to_id":null,"issue_assignees":[],"milestone":{"id":1,"title":"test milestone","project_id":8,"description":"test milestone","due_date":null,"created_at":"2016-06-14T15:02:04.415Z","updated_at":"2016-06-14T15:02:04.415Z","state":"active","iid":1,"events":[{"id":487,"target_type":"Milestone","target_id":1,"project_id":46,"created_at":"2016-06-14T15:02:04.418Z","updated_at":"2016-06-14T15:02:04.418Z","action":1,"author_id":18}]},"notes":[{"id":359,"note":"Quo eius velit quia et id quam.","noteable_type":"Issue","author_id":26,"created_at":"2016-06-14T15:02:48.009Z","updated_at":"2016-06-14T15:02:48.009Z","project_id":5,"attachment":{"url":null},"line_code":null,"commit_id":null,"noteable_id":39,"system":false,"st_diff":null,"updated_by_id":null,"author":{"name":"User 4"},"events":[]},{"id":360,"note":"Nulla commodi ratione cumque id autem.","noteable_type":"Issue","author_id":25,"created_at":"2016-06-14T15:02:48.032Z","updated_at":"2016-06-14T15:02:48.032Z","project_id":5,"attachment":{"url":null},"line_code":null,"commit_id":null,"noteable_id":39,"system":false,"st_diff":null,"updated_by_id":null,"author":{"name":"User 3"},"events":[]},{"id":361,"note":"Illum non ea sed dolores corrupti.","noteable_type":"Issue","author_id":22,"created_at":"2016-06-14T15:02:48.056Z","updated_at":"2016-06-14T15:02:48.056Z","project_id":5,"attachment":{"url":null},"line_code":null,"commit_id":null,"noteable_id":39,"system":false,"st_diff":null,"updated_by_id":null,"author":{"name":"User 0"},"events":[]},{"id":362,"note":"Facere dolores ipsum dolorum maiores omnis occaecati ab.","noteable_type":"Issue","author_id":20,"created_at":"2016-06-14T15:02:48.082Z","updated_at":"2016-06-14T15:02:48.082Z","project_id":5,"attachment":{"url":null},"line_code":null,"commit_id":null,"noteable_id":39,"system":false,"st_diff":null,"updated_by_id":null,"author":{"name":"Ottis Schuster II"},"events":[]},{"id":363,"note":"Quod laudantium similique sint aut est ducimus.","noteable_type":"Issue","author_id":16,"created_at":"2016-06-14T15:02:48.113Z","updated_at":"2016-06-14T15:02:48.113Z","project_id":5,"attachment":{"url":null},"line_code":null,"commit_id":null,"noteable_id":39,"system":false,"st_diff":null,"updated_by_id":null,"author":{"name":"Rhett Emmerich IV"},"events":[]},{"id":364,"note":"Aut omnis eos esse incidunt vero reiciendis.","noteable_type":"Issue","author_id":15,"created_at":"2016-06-14T15:02:48.139Z","updated_at":"2016-06-14T15:02:48.139Z","project_id":5,"attachment":{"url":null},"line_code":null,"commit_id":null,"noteable_id":39,"system":false,"st_diff":null,"updated_by_id":null,"author":{"name":"Burdette Bernier"},"events":[]},{"id":365,"note":"Beatae dolore et doloremque asperiores sunt.","noteable_type":"Issue","author_id":6,"created_at":"2016-06-14T15:02:48.162Z","updated_at":"2016-06-14T15:02:48.162Z","project_id":5,"attachment":{"url":null},"line_code":null,"commit_id":null,"noteable_id":39,"system":false,"st_diff":null,"updated_by_id":null,"author":{"name":"Ari Wintheiser"},"events":[]},{"id":366,"note":"Doloribus ipsam ex delectus rerum libero recusandae modi repellendus.","noteable_type":"Issue","author_id":1,"created_at":"2016-06-14T15:02:48.192Z","updated_at":"2016-06-14T15:02:48.192Z","project_id":5,"attachment":{"url":null},"line_code":null,"commit_id":null,"noteable_id":39,"system":false,"st_diff":null,"updated_by_id":null,"author":{"name":"Administrator"},"events":[]}]}
+{"id":39,"author_id":22,"project_id":null,"created_at":"2016-06-14T15:02:08.233Z","updated_at":"2016-06-14T15:02:48.194Z","position":0,"branch_name":null,"description":"Voluptate vel reprehenderit facilis omnis voluptas magnam tenetur.","state":"opened","updated_by_id":null,"confidential":false,"due_date":"2020-08-14","moved_to_id":null,"issue_assignees":[],"milestone":{"id":1,"title":"test milestone","project_id":8,"description":"test milestone","due_date":null,"created_at":"2016-06-14T15:02:04.415Z","updated_at":"2016-06-14T15:02:04.415Z","state":"active","iid":1,"events":[{"id":487,"target_type":"Milestone","target_id":1,"project_id":46,"created_at":"2016-06-14T15:02:04.418Z","updated_at":"2016-06-14T15:02:04.418Z","action":1,"author_id":18}]},"notes":[{"id":359,"note":"Quo eius velit quia et id quam.","noteable_type":"Issue","author_id":26,"created_at":"2016-06-14T15:02:48.009Z","updated_at":"2016-06-14T15:02:48.009Z","project_id":5,"attachment":{"url":null},"line_code":null,"commit_id":null,"noteable_id":39,"system":false,"st_diff":null,"updated_by_id":null,"author":{"name":"User 4"},"events":[]},{"id":360,"note":"Nulla commodi ratione cumque id autem.","noteable_type":"Issue","author_id":25,"created_at":"2016-06-14T15:02:48.032Z","updated_at":"2016-06-14T15:02:48.032Z","project_id":5,"attachment":{"url":null},"line_code":null,"commit_id":null,"noteable_id":39,"system":false,"st_diff":null,"updated_by_id":null,"author":{"name":"User 3"},"events":[]},{"id":361,"note":"Illum non ea sed dolores corrupti.","noteable_type":"Issue","author_id":22,"created_at":"2016-06-14T15:02:48.056Z","updated_at":"2016-06-14T15:02:48.056Z","project_id":5,"attachment":{"url":null},"line_code":null,"commit_id":null,"noteable_id":39,"system":false,"st_diff":null,"updated_by_id":null,"author":{"name":"User 0"},"events":[]},{"id":362,"note":"Facere dolores ipsum dolorum maiores omnis occaecati ab.","noteable_type":"Issue","author_id":20,"created_at":"2016-06-14T15:02:48.082Z","updated_at":"2016-06-14T15:02:48.082Z","project_id":5,"attachment":{"url":null},"line_code":null,"commit_id":null,"noteable_id":39,"system":false,"st_diff":null,"updated_by_id":null,"author":{"name":"Ottis Schuster II"},"events":[]},{"id":363,"note":"Quod laudantium similique sint aut est ducimus.","noteable_type":"Issue","author_id":16,"created_at":"2016-06-14T15:02:48.113Z","updated_at":"2016-06-14T15:02:48.113Z","project_id":5,"attachment":{"url":null},"line_code":null,"commit_id":null,"noteable_id":39,"system":false,"st_diff":null,"updated_by_id":null,"author":{"name":"Rhett Emmerich IV"},"events":[]},{"id":364,"note":"Aut omnis eos esse incidunt vero reiciendis.","noteable_type":"Issue","author_id":15,"created_at":"2016-06-14T15:02:48.139Z","updated_at":"2016-06-14T15:02:48.139Z","project_id":5,"attachment":{"url":null},"line_code":null,"commit_id":null,"noteable_id":39,"system":false,"st_diff":null,"updated_by_id":null,"author":{"name":"Burdette Bernier"},"events":[]},{"id":365,"note":"Beatae dolore et doloremque asperiores sunt.","noteable_type":"Issue","author_id":6,"created_at":"2016-06-14T15:02:48.162Z","updated_at":"2016-06-14T15:02:48.162Z","project_id":5,"attachment":{"url":null},"line_code":null,"commit_id":null,"noteable_id":39,"system":false,"st_diff":null,"updated_by_id":null,"author":{"name":"Ari Wintheiser"},"events":[]},{"id":366,"note":"Doloribus ipsam ex delectus rerum libero recusandae modi repellendus.","noteable_type":"Issue","author_id":1,"created_at":"2016-06-14T15:02:48.192Z","updated_at":"2016-06-14T15:02:48.192Z","project_id":5,"attachment":{"url":null},"line_code":null,"commit_id":null,"noteable_id":39,"system":false,"st_diff":null,"updated_by_id":null,"author":{"name":"Administrator"},"events":[]}]}
diff --git a/spec/fixtures/lib/gitlab/import_export/with_invalid_issues_and_milestones/tree/project/milestones.ndjson b/spec/fixtures/lib/gitlab/import_export/with_invalid_issues_and_milestones/tree/project/milestones.ndjson
new file mode 100644
index 0000000000000..28e737fa43c3a
--- /dev/null
+++ b/spec/fixtures/lib/gitlab/import_export/with_invalid_issues_and_milestones/tree/project/milestones.ndjson
@@ -0,0 +1,2 @@
+{"id":1,"title":null,"project_id":8,"description":123,"due_date":null,"created_at":"NOT A DATE","updated_at":"NOT A DATE","state":"active","iid":1,"group_id":null}
+{"id":42,"title":"A valid milestone","project_id":8,"description":"Project-level milestone","due_date":null,"created_at":"2016-06-14T15:02:04.415Z","updated_at":"2016-06-14T15:02:04.415Z","state":"active","iid":1,"group_id":null}
diff --git a/spec/lib/gitlab/import_export/group/relation_tree_restorer_spec.rb b/spec/lib/gitlab/import_export/group/relation_tree_restorer_spec.rb
index 9852f6c9652b2..8ab99875a0a8b 100644
--- a/spec/lib/gitlab/import_export/group/relation_tree_restorer_spec.rb
+++ b/spec/lib/gitlab/import_export/group/relation_tree_restorer_spec.rb
@@ -52,10 +52,10 @@
)
end
- subject { relation_tree_restorer.restore }
+ subject(:restore_relations) { relation_tree_restorer.restore }
it 'restores group tree' do
- expect(subject).to eq(true)
+ expect(restore_relations).to eq(true)
end
it 'logs top-level relation creation' do
@@ -64,7 +64,7 @@
.with(hash_including(message: '[Project/Group Import] Created new object relation'))
.at_least(:once)
- subject
+ restore_relations
end
describe 'relation object saving' do
@@ -100,7 +100,7 @@
error_messages: "Label can't be blank, Position can't be blank, and Position is not a number"
)
- subject
+ restore_relations
board = importable.boards.last
failure = importable.import_failures.first
@@ -115,6 +115,33 @@
end
end
+ context 'when invalid relation object has a loggable external identifier' do
+ before do
+ allow(relation_reader)
+ .to receive(:consume_relation)
+ .with(importable_name, 'milestones')
+ .and_return([
+ [invalid_milestone, 0],
+ [invalid_milestone_with_no_iid, 1]
+ ])
+ end
+
+ let(:invalid_milestone) { build(:milestone, iid: 123, name: nil) }
+ let(:invalid_milestone_with_no_iid) { build(:milestone, iid: nil, name: nil) }
+
+ it 'logs invalid record with external identifier' do
+ restore_relations
+
+ iids_for_failures = importable.import_failures.collect { |f| [f.relation_key, f.external_identifiers] }
+ expected_iids = [
+ ["milestones", { "iid" => invalid_milestone.iid }],
+ ["milestones", {}]
+ ]
+
+ expect(iids_for_failures).to match_array(expected_iids)
+ end
+ end
+
context 'when relation object is persisted' do
before do
allow(relation_reader)
@@ -129,7 +156,7 @@
it 'saves import failure with nested errors' do
label.priorities << [LabelPriority.new, LabelPriority.new]
- subject
+ restore_relations
failure = importable.import_failures.first
diff --git a/spec/lib/gitlab/import_export/import_failure_service_spec.rb b/spec/lib/gitlab/import_export/import_failure_service_spec.rb
index 30d1634782896..9628e9fbf4d3c 100644
--- a/spec/lib/gitlab/import_export/import_failure_service_spec.rb
+++ b/spec/lib/gitlab/import_export/import_failure_service_spec.rb
@@ -21,7 +21,8 @@
relation_key: relation_key,
relation_index: relation_index,
exception: exception,
- retry_count: retry_count)
+ retry_count: retry_count,
+ external_identifiers: { iid: 1234 })
end
before do
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 14af3028a6efe..d565f3f3150f2 100644
--- a/spec/lib/gitlab/import_export/project/tree_restorer_spec.rb
+++ b/spec/lib/gitlab/import_export/project/tree_restorer_spec.rb
@@ -1114,9 +1114,10 @@ def match_mr1_note(content_regex)
let(:user) { create(:user) }
let!(:project) { create(:project, :builds_disabled, :issues_disabled, name: 'project', path: 'project') }
let(:project_tree_restorer) { described_class.new(user: user, shared: shared, project: project) }
+ let(:project_fixture) { 'with_invalid_records' }
before do
- setup_import_export_config('with_invalid_records')
+ setup_import_export_config(project_fixture)
setup_reader
subject
@@ -1142,6 +1143,21 @@ def match_mr1_note(content_regex)
expect(import_failure.correlation_id_value).not_to be_empty
expect(import_failure.created_at).to be_present
end
+
+ context 'when there are a mix of invalid milestones and issues with IIDs' do
+ let(:project_fixture) { 'with_invalid_issues_and_milestones' }
+
+ it 'tracks the relation IID if present' do
+ iids_for_failures = project.import_failures.collect { |f| [f.relation_key, f.external_identifiers] }
+ expected_iids = [
+ ["milestones", { "iid" => 1 }],
+ ["issues", { "iid" => 9 }],
+ ["issues", {}]
+ ]
+
+ expect(iids_for_failures).to match_array(expected_iids)
+ end
+ end
end
end
diff --git a/spec/support/shared_examples/lib/gitlab/import_export/import_failure_service_shared_examples.rb b/spec/support/shared_examples/lib/gitlab/import_export/import_failure_service_shared_examples.rb
index 67afd2035c4fb..afb1bfb6dc928 100644
--- a/spec/support/shared_examples/lib/gitlab/import_export/import_failure_service_shared_examples.rb
+++ b/spec/support/shared_examples/lib/gitlab/import_export/import_failure_service_shared_examples.rb
@@ -34,6 +34,7 @@
expect(import_failure.exception_message).to eq(standard_error_message)
expect(import_failure.correlation_id_value).to eq(correlation_id)
expect(import_failure.retry_count).to eq(retry_count)
+ expect(import_failure.external_identifiers).to eq("iid" => 1234)
end
end
end
--
GitLab