diff --git a/.rubocop_todo/layout/space_in_lambda_literal.yml b/.rubocop_todo/layout/space_in_lambda_literal.yml index 7e3af689b5f0199f876f229b9cc9d82fa78e63a2..e0113d2628d937c48da3b3d4a05127517fded500 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 db1c8073df8c8e1573c9c7aad8d23e12b6f3fbad..1e5725a2ef0dbb2c6ea86d5abb5cd141d934d7f4 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 e4b751a92da36f08532199fa14d49461f46e40f6..8f27a2c9f4584a7b4ad27468dfff884379afb526 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 5453792e9043d91c9cd3a7ec227426adb595cd89..ae47c95036ea5003df00d9ea01eb72661af05074 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 bf7200726a1faf04a1b2bd14c8fc1d7cc201562b..f93822a9d955e52cfe105ce6ff6028724f538b9d 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 0000000000000000000000000000000000000000..a522590d037fa34e0991e95b07e59b64d2f68d4f --- /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 0000000000000000000000000000000000000000..cf7d9211460c752aa67a5c98c4fff27c751762c7 --- /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 0000000000000000000000000000000000000000..28e737fa43c3a8ececb2bfd785e29d28fe23a3ed --- /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 9852f6c9652b2869ccf9b24081bf628c5ff84a54..8ab99875a0a8b7bd8d1ad1dd1ff049b693124651 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 30d16347828965327d23d45d38dc376d30c8ade5..9628e9fbf4d3c9e9019fdfd26fef4ea991182358 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 14af3028a6efe666795dd15ec248b8994ceb3387..d565f3f3150f2f56ad5a4bba4a3d2fe09ed5682a 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 67afd2035c4fb108becec43a4010a51e9df48e07..afb1bfb6dc928c2b95cd5b247bae7e6cd8279462 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