Merge branch '37057-record-import-failures' into 'master'

Project Import: Fail gracefully & record failures

See merge request gitlab-org/gitlab!20727

app/models/import_failure.rb

+# frozen_string_literal: true
+
+class ImportFailure < ApplicationRecord
+  belongs_to :project
+
+  validates :project, presence: true
+end

app/models/project.rb

@@ -295,6 +295,8 @@ class Project < ApplicationRecord
 
   has_one :pages_metadatum, class_name: 'ProjectPagesMetadatum', inverse_of: :project
 
+  has_many :import_failures, inverse_of: :project
+
   accepts_nested_attributes_for :variables, allow_destroy: true
   accepts_nested_attributes_for :project_feature, update_only: true
   accepts_nested_attributes_for :import_data

changelogs/unreleased/37057-record-import-failures.yml

+---
+title: Collect project import failures instead of failing fast
+merge_request: 20727
+author:
+type: other

db/migrate/20191125140458_create_import_failures.rb

+# frozen_string_literal: true
+
+class CreateImportFailures < ActiveRecord::Migration[5.2]
+  DOWNTIME = false
+
+  def change
+    create_table :import_failures do |t|
+      t.integer :relation_index
+      t.references :project, null: false, index: true
+      t.datetime_with_timezone :created_at, null: false
+      t.string :relation_key, limit: 64
+      t.string :exception_class, limit: 128
+      t.string :correlation_id_value, limit: 128, index: true
+      t.string :exception_message, limit: 255
+    end
+  end
+end

db/schema.rb

@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 2019_11_24_150431) do
+ActiveRecord::Schema.define(version: 2019_11_25_140458) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "pg_trgm"
@@ -1948,6 +1948,18 @@ ActiveRecord::Schema.define(version: 2019_11_24_150431) do
     t.index ["updated_at"], name: "index_import_export_uploads_on_updated_at"
   end
 
+  create_table "import_failures", force: :cascade do |t|
+    t.integer "relation_index"
+    t.bigint "project_id", null: false
+    t.datetime_with_timezone "created_at", null: false
+    t.string "relation_key", limit: 64
+    t.string "exception_class", limit: 128
+    t.string "correlation_id_value", limit: 128
+    t.string "exception_message", limit: 255
+    t.index ["correlation_id_value"], name: "index_import_failures_on_correlation_id_value"
+    t.index ["project_id"], name: "index_import_failures_on_project_id"
+  end
+
   create_table "index_statuses", id: :serial, force: :cascade do |t|
     t.integer "project_id", null: false
     t.datetime "indexed_at"

ee/spec/lib/ee/gitlab/import_export/project_tree_restorer_spec.rb

@@ -20,7 +20,7 @@ describe Gitlab::ImportExport::ProjectTreeRestorer do
       restored_project_json
     end
 
-    it_behaves_like 'restores project correctly', issues: 2
+    it_behaves_like 'restores project successfully', issues: 2
 
     it 'restores project associations correctly' do
       expect(project.designs.size).to eq(7)

lib/gitlab/import_export/project_tree_restorer.rb

@@ -85,13 +85,16 @@ module Gitlab
         # we do not care if we process array or hash
         data_hashes = [data_hashes] unless data_hashes.is_a?(Array)
 
+        relation_index = 0
+
         # consume and remove objects from memory
         while data_hash = data_hashes.shift
-          process_project_relation_item!(relation_key, relation_definition, data_hash)
+          process_project_relation_item!(relation_key, relation_definition, relation_index, data_hash)
+          relation_index += 1
         end
       end
 
-      def process_project_relation_item!(relation_key, relation_definition, data_hash)
+      def process_project_relation_item!(relation_key, relation_definition, relation_index, data_hash)
         relation_object = build_relation(relation_key, relation_definition, data_hash)
         return unless relation_object
         return if group_model?(relation_object)
@@ -100,6 +103,25 @@ module Gitlab
         relation_object.save!
 
         save_id_mapping(relation_key, data_hash, relation_object)
+      rescue => e
+        # re-raise if not enabled
+        raise e unless Feature.enabled?(:import_graceful_failures, @project.group)
+
+        log_import_failure(relation_key, relation_index, e)
+      end
+
+      def log_import_failure(relation_key, relation_index, exception)
+        Gitlab::Sentry.track_acceptable_exception(exception,
+          extra: { project_id: @project.id, relation_key: relation_key, relation_index: relation_index })
+
+        ImportFailure.create(
+          project: @project,
+          relation_key: relation_key,
+          relation_index: relation_index,
+          exception_class: exception.class.to_s,
+          exception_message: exception.message.truncate(255),
+          correlation_id_value: Labkit::Correlation::CorrelationId.current_id
+        )
       end
 
       # Older, serialized CI pipeline exports may only have a

spec/db/schema_spec.rb

@@ -43,6 +43,7 @@ describe 'Database schema' do
     geo_nodes: %w[oauth_application_id],
     geo_repository_deleted_events: %w[project_id],
     geo_upload_deleted_events: %w[upload_id model_id],
+    import_failures: %w[project_id],
     identities: %w[user_id],
     issues: %w[last_edited_by_id state_id],
     jira_tracker_data: %w[jira_issue_transition_id],

spec/fixtures/lib/gitlab/import_export/with_invalid_records/project.json

+{
+  "description": "Nisi et repellendus ut enim quo accusamus vel magnam.",
+  "import_type": "gitlab_project",
+  "creator_id": 123,
+  "visibility_level": 10,
+  "archived": false,
+  "milestones": [
+    {
+      "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
+    }
+  ],
+  "labels": [],
+  "issues": [],
+  "services": [],
+  "snippets": [],
+  "hooks": []
+}

spec/lib/gitlab/import_export/all_models.yml

@@ -434,6 +434,7 @@ project:
 - upstream_project_subscriptions
 - downstream_project_subscriptions
 - service_desk_setting
+- import_failures
 award_emoji:
 - awardable
 - user

spec/lib/gitlab/import_export/project_tree_restorer_spec.rb

@@ -362,7 +362,7 @@ describe Gitlab::ImportExport::ProjectTreeRestorer do
         expect(restored_project_json).to eq(true)
       end
 
-      it_behaves_like 'restores project correctly',
+      it_behaves_like 'restores project successfully',
                       issues: 1,
                       labels: 2,
                       label_with_priorities: 'A project label',
@@ -375,7 +375,7 @@ describe Gitlab::ImportExport::ProjectTreeRestorer do
           create(:ci_build, token: 'abcd')
         end
 
-        it_behaves_like 'restores project correctly',
+        it_behaves_like 'restores project successfully',
                         issues: 1,
                         labels: 2,
                         label_with_priorities: 'A project label',
@@ -452,7 +452,7 @@ describe Gitlab::ImportExport::ProjectTreeRestorer do
         expect(restored_project_json).to eq(true)
       end
 
-      it_behaves_like 'restores project correctly',
+      it_behaves_like 'restores project successfully',
                       issues: 2,
                       labels: 2,
                       label_with_priorities: 'A project label',
@@ -633,4 +633,46 @@ describe Gitlab::ImportExport::ProjectTreeRestorer do
       end
     end
   end
+
+  context 'JSON with invalid records' do
+    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(:restored_project_json) { project_tree_restorer.restore }
+
+    context 'when some failures occur' do
+      context 'because a relation fails to be processed' do
+        let(:correlation_id) { 'my-correlation-id' }
+
+        before do
+          setup_import_export_config('with_invalid_records')
+
+          Labkit::Correlation::CorrelationId.use_id(correlation_id) do
+            expect(restored_project_json).to eq(true)
+          end
+        end
+
+        it_behaves_like 'restores project successfully',
+          issues: 0,
+          labels: 0,
+          label_with_priorities: nil,
+          milestones: 1,
+          first_issue_labels: 0,
+          services: 0,
+          import_failures: 1
+
+        it 'records the failures in the database' do
+          import_failure = ImportFailure.last
+
+          expect(import_failure.project_id).to eq(project.id)
+          expect(import_failure.relation_key).to eq('milestones')
+          expect(import_failure.relation_index).to be_present
+          expect(import_failure.exception_class).to eq('ActiveRecord::RecordInvalid')
+          expect(import_failure.exception_message).to be_present
+          expect(import_failure.correlation_id_value).to eq('my-correlation-id')
+          expect(import_failure.created_at).to be_present
+        end
+      end
+    end
+  end
 end

spec/support/shared_examples/lib/gitlab/import_export/project_tree_restorer_shared_examples.rb

@@ -2,7 +2,7 @@
 
 # Shared examples for ProjectTreeRestorer (shared to allow the testing
 # of EE-specific features)
-RSpec.shared_examples 'restores project correctly' do |**results|
+RSpec.shared_examples 'restores project successfully' do |**results|
   it 'restores the project' do
     expect(shared.errors).to be_empty
     expect(restored_project_json).to be_truthy
@@ -34,4 +34,8 @@ RSpec.shared_examples 'restores project correctly' do |**results|
     expect(project.import_type).to be_nil
     expect(project.creator_id).not_to eq 123
   end
+
+  it 'records exact number of import failures' do
+    expect(project.import_failures.size).to eq(results.fetch(:import_failures, 0))
+  end
 end