Merge branch '210522-api-expose-import-failures' into 'master'

Expose unrecovered import failures in /import REST endpoint

See merge request gitlab-org/gitlab!28915

app/models/import_failure.rb

@@ -6,4 +6,11 @@ class ImportFailure < ApplicationRecord
 
   validates :project, presence: true, unless: :group
   validates :group, presence: true, unless: :project
+
+  # Returns any `import_failures` for relations that were unrecoverable errors or failed after
+  # several retries. An import can be successful even if some relations failed to import correctly.
+  # A retry_count of 0 indicates that either no retries were attempted, or they were exceeded.
+  scope :hard_failures_by_correlation_id, ->(correlation_id) {
+    where(correlation_id_value: correlation_id, retry_count: 0).order(created_at: :desc)
+  }
 end

app/models/project_import_state.rb

@@ -72,6 +72,10 @@ class ProjectImportState < ApplicationRecord
     end
   end
 
+  def relation_hard_failures(limit:)
+    project.import_failures.hard_failures_by_correlation_id(correlation_id).limit(limit)
+  end
+
   def mark_as_failed(error_message)
     original_errors = errors.dup
     sanitized_message = Gitlab::UrlSanitizer.sanitize(error_message)

changelogs/unreleased/210522-api-expose-import-failures.yml

+---
+title: Expose relations that failed to import in /import endpoints
+merge_request: 28915
+author:
+type: changed

db/migrate/20200409085956_add_partial_index_on_import_failures_retry_count.rb

+# frozen_string_literal: true
+
+class AddPartialIndexOnImportFailuresRetryCount < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    add_concurrent_index :import_failures, [:project_id, :correlation_id_value], where: 'retry_count = 0'
+  end
+
+  def down
+    remove_concurrent_index :import_failures, [:project_id, :correlation_id_value]
+  end
+end

db/structure.sql

@@ -9373,6 +9373,8 @@ CREATE INDEX index_import_failures_on_correlation_id_value ON public.import_fail
 
 CREATE INDEX index_import_failures_on_group_id_not_null ON public.import_failures USING btree (group_id) WHERE (group_id IS NOT NULL);
 
+CREATE INDEX index_import_failures_on_project_id_and_correlation_id_value ON public.import_failures USING btree (project_id, correlation_id_value) WHERE (retry_count = 0);
+
 CREATE INDEX index_import_failures_on_project_id_not_null ON public.import_failures USING btree (project_id) WHERE (project_id IS NOT NULL);
 
 CREATE UNIQUE INDEX index_index_statuses_on_project_id ON public.index_statuses USING btree (project_id);
@@ -13196,6 +13198,7 @@ COPY "schema_migrations" (version) FROM STDIN;
 20200408154604
 20200408154624
 20200408175424
+20200409085956
 20200409211607
 20200410232012
 20200414144547

doc/api/project_import_export.md

@@ -173,7 +173,8 @@ requests.post(url, headers=headers, data=data, files=files)
   "path_with_namespace": "root/api-project",
   "created_at": "2018-02-13T09:05:58.023Z",
   "import_status": "scheduled",
-  "correlation_id": "mezklWso3Za"
+  "correlation_id": "mezklWso3Za",
+  "failed_relations": []
 }
 ```
 
@@ -202,6 +203,15 @@ Status can be one of:
 - `finished`
 
 If the status is `failed`, it will include the import error message under `import_error`.
+If the status is `failed`, `started` or `finished`, the `failed_relations` array might
+be populated with any occurrences of relations that failed to import either due to
+unrecoverable errors or because retries were exhausted (a typical example are query timeouts.)
+
+NOTE: **Note:**
+An element's `id` field in `failed_relations` references the failure record, not the relation.
+
+NOTE: **Note:**
+The `failed_relations` array is currently capped to 100 items.
 
 ```json
 {
@@ -213,6 +223,16 @@ If the status is `failed`, it will include the import error message under `impor
   "path_with_namespace": "gitlab-org/gitlab-test",
   "created_at": "2017-08-29T04:36:44.383Z",
   "import_status": "started",
-  "correlation_id": "mezklWso3Za"
+  "correlation_id": "mezklWso3Za",
+  "failed_relations": [
+    {
+      "id": 42,
+      "created_at": "2020-04-02T14:48:59.526Z",
+      "exception_class": "RuntimeError",
+      "exception_message": "A failure occurred",
+      "source": "custom error context",
+      "relation_name": "merge_requests"
+    }
+  ]
 }
 ```

lib/api/entities/project_import_failed_relation.rb

+# frozen_string_literal: true
+
+module API
+  module Entities
+    class ProjectImportFailedRelation < Grape::Entity
+      expose :id, :created_at, :exception_class, :exception_message, :source
+
+      expose :relation_key, as: :relation_name
+    end
+  end
+end

lib/api/entities/project_import_status.rb

@@ -8,6 +8,10 @@ module API
         project.import_state&.correlation_id
       end
 
+      expose :failed_relations, using: Entities::ProjectImportFailedRelation do |project, _options|
+        project.import_state.relation_hard_failures(limit: 100)
+      end
+
       # TODO: Use `expose_nil` once we upgrade the grape-entity gem
       expose :import_error, if: lambda { |project, _ops| project.import_state&.last_error } do |project|
         project.import_state.last_error

spec/factories/import_failures.rb

+# frozen_string_literal: true
+
+require 'securerandom'
+
+FactoryBot.define do
+  factory :import_failure do
+    association :project, factory: :project
+
+    created_at { Time.parse('2020-01-01T00:00:00Z') }
+    exception_class { 'RuntimeError' }
+    exception_message { 'Something went wrong' }
+    source { 'method_call' }
+    correlation_id_value { SecureRandom.uuid }
+
+    trait :hard_failure do
+      retry_count { 0 }
+    end
+
+    trait :soft_failure do
+      retry_count { 1 }
+    end
+  end
+end

spec/factories/projects.rb

@@ -37,6 +37,8 @@ FactoryBot.define do
       group_runners_enabled { nil }
       import_status { nil }
       import_jid { nil }
+      import_correlation_id { nil }
+      import_last_error { nil }
       forward_deployment_enabled { nil }
     end
 
@@ -78,6 +80,8 @@ FactoryBot.define do
         import_state = project.import_state || project.build_import_state
         import_state.status = evaluator.import_status
         import_state.jid = evaluator.import_jid
+        import_state.correlation_id_value = evaluator.import_correlation_id
+        import_state.last_error = evaluator.import_last_error
         import_state.save
       end
     end

spec/lib/api/entities/project_import_failed_relation_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe API::Entities::ProjectImportFailedRelation do
+  describe '#as_json' do
+    subject { entity.as_json }
+
+    let(:import_failure) { build(:import_failure) }
+    let(:entity) { described_class.new(import_failure) }
+
+    it 'includes basic fields', :aggregate_failures do
+      expect(subject).to eq(
+        id: import_failure.id,
+        created_at: import_failure.created_at,
+        exception_class: import_failure.exception_class,
+        exception_message: import_failure.exception_message,
+        relation_name: import_failure.relation_key,
+        source: import_failure.source
+      )
+    end
+  end
+end

spec/lib/api/entities/project_import_status_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe API::Entities::ProjectImportStatus do
+  describe '#as_json' do
+    subject { entity.as_json }
+
+    let(:correlation_id) { 'cid' }
+
+    context 'when import has not finished yet' do
+      let(:project) { create(:project, :import_scheduled, import_correlation_id: correlation_id) }
+      let(:entity) { described_class.new(project) }
+
+      it 'includes basic fields and no failures', :aggregate_failures do
+        expect(subject[:import_status]).to eq('scheduled')
+        expect(subject[:correlation_id]).to eq(correlation_id)
+        expect(subject[:import_error]).to be_nil
+        expect(subject[:failed_relations]).to eq([])
+      end
+    end
+
+    context 'when import has finished with failed relations' do
+      let(:project) { create(:project, :import_finished, import_correlation_id: correlation_id) }
+      let(:entity) { described_class.new(project) }
+
+      it 'includes basic fields with failed relations', :aggregate_failures do
+        create(:import_failure, :hard_failure, project: project, correlation_id_value: correlation_id)
+
+        expect(subject[:import_status]).to eq('finished')
+        expect(subject[:correlation_id]).to eq(correlation_id)
+        expect(subject[:import_error]).to be_nil
+        expect(subject[:failed_relations]).not_to be_empty
+      end
+    end
+
+    context 'when import has failed' do
+      let(:project) { create(:project, :import_failed, import_correlation_id: correlation_id, import_last_error: 'error') }
+      let(:entity) { described_class.new(project) }
+
+      it 'includes basic fields with import error', :aggregate_failures do
+        expect(subject[:import_status]).to eq('failed')
+        expect(subject[:correlation_id]).to eq(correlation_id)
+        expect(subject[:import_error]).to eq('error')
+        expect(subject[:failed_relations]).to eq([])
+      end
+    end
+  end
+end

spec/models/import_failure_spec.rb

@@ -3,7 +3,28 @@
 require 'spec_helper'
 
 describe ImportFailure do
-  describe "Associations" do
+  describe 'Scopes' do
+    let_it_be(:project) { create(:project) }
+    let_it_be(:correlation_id) { 'ABC' }
+    let_it_be(:hard_failure) { create(:import_failure, :hard_failure, project: project, correlation_id_value: correlation_id) }
+    let_it_be(:soft_failure) { create(:import_failure, :soft_failure, project: project, correlation_id_value: correlation_id) }
+    let_it_be(:unrelated_failure) { create(:import_failure, project: project) }
+
+    it 'returns hard failures given a correlation ID' do
+      expect(ImportFailure.hard_failures_by_correlation_id(correlation_id)).to eq([hard_failure])
+    end
+
+    it 'orders hard failures by newest first' do
+      older_failure = hard_failure.dup
+      Timecop.freeze(1.day.before(hard_failure.created_at)) do
+        older_failure.save!
+
+        expect(ImportFailure.hard_failures_by_correlation_id(correlation_id)).to eq([hard_failure, older_failure])
+      end
+    end
+  end
+
+  describe 'Associations' do
     it { is_expected.to belong_to(:project) }
     it { is_expected.to belong_to(:group) }
   end

spec/models/project_import_state_spec.rb

@@ -3,7 +3,10 @@
 require 'spec_helper'
 
 describe ProjectImportState, type: :model do
-  subject { create(:import_state) }
+  let_it_be(:correlation_id) { 'cid' }
+  let_it_be(:import_state, refind: true) { create(:import_state, correlation_id_value: correlation_id) }
+
+  subject { import_state }
 
   describe 'associations' do
     it { is_expected.to belong_to(:project) }
@@ -33,12 +36,24 @@ describe ProjectImportState, type: :model do
     end
 
     it 'records job and correlation IDs', :sidekiq_might_not_need_inline do
-      allow(Labkit::Correlation::CorrelationId).to receive(:current_or_new_id).and_return('abc')
+      allow(Labkit::Correlation::CorrelationId).to receive(:current_or_new_id).and_return(correlation_id)
 
       import_state.schedule
 
       expect(import_state.jid).to be_an_instance_of(String)
-      expect(import_state.correlation_id).to eq('abc')
+      expect(import_state.correlation_id).to eq(correlation_id)
+    end
+  end
+
+  describe '#relation_hard_failures' do
+    let_it_be(:failures) { create_list(:import_failure, 2, :hard_failure, project: import_state.project, correlation_id_value: correlation_id) }
+
+    it 'returns hard relation failures related to this import' do
+      expect(subject.relation_hard_failures(limit: 100)).to match_array(failures)
+    end
+
+    it 'limits returned collection to given maximum' do
+      expect(subject.relation_hard_failures(limit: 1).size).to eq(1)
     end
   end