Add latest changes from gitlab-org/gitlab@13-7-stable-eev13.7.0-rc42

9 files changed, 266 insertions, 154 deletions

diff --git a/spec/lib/bulk_imports/common/extractors/graphql_extractor_spec.rb b/spec/lib/bulk_imports/common/extractors/graphql_extractor_spec.rb
index cde8e2d5c18..a7a19fb73fc 100644
--- a/spec/lib/bulk_imports/common/extractors/graphql_extractor_spec.rb
+++ b/spec/lib/bulk_imports/common/extractors/graphql_extractor_spec.rb
@@ -41,12 +41,11 @@ RSpec.describe BulkImports::Common::Extractors::GraphqlExtractor do
     end
 
     context 'when variables are present' do
-      let(:query) { { query: double(to_s: 'test', variables: { full_path: :source_full_path }) } }
+      let(:variables) { { foo: :bar } }
+      let(:query) { { query: double(to_s: 'test', variables: variables) } }
 
       it 'builds graphql query variables for import entity' do
-        expected_variables = { full_path: import_entity.source_full_path }
-
-        expect(graphql_client).to receive(:execute).with(anything, expected_variables)
+        expect(graphql_client).to receive(:execute).with(anything, variables)
 
         subject.extract(context).first
       end
diff --git a/spec/lib/bulk_imports/common/transformers/graphql_cleaner_transformer_spec.rb b/spec/lib/bulk_imports/common/transformers/graphql_cleaner_transformer_spec.rb
deleted file mode 100644
index 8f39b6e7c93..00000000000
--- a/spec/lib/bulk_imports/common/transformers/graphql_cleaner_transformer_spec.rb
+++ /dev/null
@@ -1,88 +0,0 @@
-# frozen_string_literal: true
-
-require 'spec_helper'
-
-RSpec.describe BulkImports::Common::Transformers::GraphqlCleanerTransformer do
-  describe '#transform' do
-    let_it_be(:expected_output) do
-      {
-        'name' => 'test',
-        'fullName' => 'test',
-        'description' => 'test',
-        'labels' => [
-          { 'title' => 'label1' },
-          { 'title' => 'label2' },
-          { 'title' => 'label3' }
-        ]
-      }
-    end
-
-    it 'deep cleans hash from GraphQL keys' do
-      data = {
-        'data' => {
-          'group' => {
-            'name' => 'test',
-            'fullName' => 'test',
-            'description' => 'test',
-            'labels' => {
-              'edges' => [
-                { 'node' => { 'title' => 'label1' } },
-                { 'node' => { 'title' => 'label2' } },
-                { 'node' => { 'title' => 'label3' } }
-              ]
-            }
-          }
-        }
-      }
-
-      transformed_data = described_class.new.transform(nil, data)
-
-      expect(transformed_data).to eq(expected_output)
-    end
-
-    context 'when data does not have data/group nesting' do
-      it 'deep cleans hash from GraphQL keys' do
-        data = {
-          'name' => 'test',
-          'fullName' => 'test',
-          'description' => 'test',
-          'labels' => {
-            'edges' => [
-              { 'node' => { 'title' => 'label1' } },
-              { 'node' => { 'title' => 'label2' } },
-              { 'node' => { 'title' => 'label3' } }
-            ]
-          }
-        }
-
-        transformed_data = described_class.new.transform(nil, data)
-
-        expect(transformed_data).to eq(expected_output)
-      end
-    end
-
-    context 'when data is not a hash' do
-      it 'does not perform transformation' do
-        data = 'test'
-
-        transformed_data = described_class.new.transform(nil, data)
-
-        expect(transformed_data).to eq(data)
-      end
-    end
-
-    context 'when nested data is not an array or hash' do
-      it 'only removes top level data/group keys' do
-        data = {
-          'data' => {
-            'group' => 'test'
-          }
-        }
-
-        transformed_data = described_class.new.transform(nil, data)
-
-        expect(transformed_data).to eq('test')
-      end
-    end
-  end
-end
diff --git a/spec/lib/bulk_imports/common/transformers/hash_key_digger_spec.rb b/spec/lib/bulk_imports/common/transformers/hash_key_digger_spec.rb
new file mode 100644
index 00000000000..2b33701653e
--- /dev/null
+++ b/spec/lib/bulk_imports/common/transformers/hash_key_digger_spec.rb
@@ -0,0 +1,28 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe BulkImports::Common::Transformers::HashKeyDigger do
+  describe '#transform' do
+    it 'when the key_path is an array' do
+      data = { foo: { bar: :value } }
+      key_path = %i[foo bar]
+      transformed = described_class.new(key_path: key_path).transform(nil, data)
+
+      expect(transformed).to eq(:value)
+    end
+
+    it 'when the key_path is not an array' do
+      data = { foo: { bar: :value } }
+      key_path = :foo
+      transformed = described_class.new(key_path: key_path).transform(nil, data)
+
+      expect(transformed).to eq({ bar: :value })
+    end
+
+    it "when the data is not a hash" do
+      expect { described_class.new(key_path: nil).transform(nil, nil) }
+        .to raise_error(ArgumentError, "Given data must be a Hash")
+    end
+  end
+end
diff --git a/spec/lib/bulk_imports/common/transformers/prohibited_attributes_transformer_spec.rb b/spec/lib/bulk_imports/common/transformers/prohibited_attributes_transformer_spec.rb
new file mode 100644
index 00000000000..03d138b227c
--- /dev/null
+++ b/spec/lib/bulk_imports/common/transformers/prohibited_attributes_transformer_spec.rb
@@ -0,0 +1,72 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe BulkImports::Common::Transformers::ProhibitedAttributesTransformer do
+  describe '#transform' do
+    let_it_be(:hash) do
+      {
+        'id' => 101,
+        'service_id' => 99,
+        'moved_to_id' => 99,
+        'namespace_id' => 99,
+        'ci_id' => 99,
+        'random_project_id' => 99,
+        'random_id' => 99,
+        'milestone_id' => 99,
+        'project_id' => 99,
+        'user_id' => 99,
+        'random_id_in_the_middle' => 99,
+        'notid' => 99,
+        'import_source' => 'test',
+        'import_type' => 'test',
+        'non_existent_attr' => 'test',
+        'some_html' => '<p>dodgy html</p>',
+        'legit_html' => '<p>legit html</p>',
+        '_html' => '<p>perfectly ordinary html</p>',
+        'cached_markdown_version' => 12345,
+        'custom_attributes' => 'test',
+        'some_attributes_metadata' => 'test',
+        'group_id' => 99,
+        'commit_id' => 99,
+        'issue_ids' => [1, 2, 3],
+        'merge_request_ids' => [1, 2, 3],
+        'note_ids' => [1, 2, 3],
+        'remote_attachment_url' => 'http://something.dodgy',
+        'remote_attachment_request_header' => 'bad value',
+        'remote_attachment_urls' => %w(http://something.dodgy http://something.okay),
+        'attributes' => {
+          'issue_ids' => [1, 2, 3],
+          'merge_request_ids' => [1, 2, 3],
+          'note_ids' => [1, 2, 3]
+        },
+        'variables_attributes' => {
+          'id' => 1
+        },
+        'attr_with_nested_attrs' => {
+          'nested_id' => 1,
+          'nested_attr' => 2
+        }
+      }
+    end
+
+    let(:expected_hash) do
+      {
+        'random_id_in_the_middle' => 99,
+        'notid' => 99,
+        'import_source' => 'test',
+        'import_type' => 'test',
+        'non_existent_attr' => 'test',
+        'attr_with_nested_attrs' => {
+          'nested_attr' => 2
+        }
+      }
+    end
+
+    it 'removes prohibited attributes' do
+      transformed_hash = subject.transform(nil, hash)
+
+      expect(transformed_hash).to eq(expected_hash)
+    end
+  end
+end
diff --git a/spec/lib/bulk_imports/groups/pipelines/group_pipeline_spec.rb b/spec/lib/bulk_imports/groups/pipelines/group_pipeline_spec.rb
index 3949dd23b49..c9b481388c3 100644
--- a/spec/lib/bulk_imports/groups/pipelines/group_pipeline_spec.rb
+++ b/spec/lib/bulk_imports/groups/pipelines/group_pipeline_spec.rb
@@ -72,7 +72,6 @@ RSpec.describe BulkImports::Groups::Pipelines::GroupPipeline do
 
   describe 'pipeline parts' do
     it { expect(described_class).to include_module(BulkImports::Pipeline) }
-    it { expect(described_class).to include_module(BulkImports::Pipeline::Attributes) }
     it { expect(described_class).to include_module(BulkImports::Pipeline::Runner) }
 
     it 'has extractors' do
@@ -90,13 +89,17 @@ RSpec.describe BulkImports::Groups::Pipelines::GroupPipeline do
     it 'has transformers' do
       expect(described_class.transformers)
         .to contain_exactly(
-          { klass: BulkImports::Common::Transformers::GraphqlCleanerTransformer, options: nil },
+          { klass: BulkImports::Common::Transformers::HashKeyDigger, options: { key_path: %w[data group] } },
           { klass: BulkImports::Common::Transformers::UnderscorifyKeysTransformer, options: nil },
-          { klass: BulkImports::Groups::Transformers::GroupAttributesTransformer, options: nil })
+          { klass: BulkImports::Common::Transformers::ProhibitedAttributesTransformer, options: nil },
+          { klass: BulkImports::Groups::Transformers::GroupAttributesTransformer, options: nil }
+        )
     end
 
     it 'has loaders' do
-      expect(described_class.loaders).to contain_exactly({ klass: BulkImports::Groups::Loaders::GroupLoader, options: nil })
+      expect(described_class.loaders).to contain_exactly({
+        klass: BulkImports::Groups::Loaders::GroupLoader, options: nil
+      })
     end
   end
 end
diff --git a/spec/lib/bulk_imports/groups/pipelines/subgroup_entities_pipeline_spec.rb b/spec/lib/bulk_imports/groups/pipelines/subgroup_entities_pipeline_spec.rb
index 60a4a796682..788a6e98c45 100644
--- a/spec/lib/bulk_imports/groups/pipelines/subgroup_entities_pipeline_spec.rb
+++ b/spec/lib/bulk_imports/groups/pipelines/subgroup_entities_pipeline_spec.rb
@@ -55,7 +55,6 @@ RSpec.describe BulkImports::Groups::Pipelines::SubgroupEntitiesPipeline do
 
   describe 'pipeline parts' do
     it { expect(described_class).to include_module(BulkImports::Pipeline) }
-    it { expect(described_class).to include_module(BulkImports::Pipeline::Attributes) }
     it { expect(described_class).to include_module(BulkImports::Pipeline::Runner) }
 
     it 'has extractors' do
@@ -67,8 +66,8 @@ RSpec.describe BulkImports::Groups::Pipelines::SubgroupEntitiesPipeline do
 
     it 'has transformers' do
       expect(described_class.transformers).to contain_exactly(
-        klass: BulkImports::Groups::Transformers::SubgroupToEntityTransformer,
-        options: nil
+        { klass: BulkImports::Common::Transformers::ProhibitedAttributesTransformer, options: nil },
+        { klass: BulkImports::Groups::Transformers::SubgroupToEntityTransformer, options: nil }
       )
     end
 
diff --git a/spec/lib/bulk_imports/importers/group_importer_spec.rb b/spec/lib/bulk_imports/importers/group_importer_spec.rb
index 95ac5925c97..95dca7fc486 100644
--- a/spec/lib/bulk_imports/importers/group_importer_spec.rb
+++ b/spec/lib/bulk_imports/importers/group_importer_spec.rb
@@ -18,12 +18,12 @@ RSpec.describe BulkImports::Importers::GroupImporter do
   subject { described_class.new(bulk_import_entity) }
 
   before do
+    allow(Gitlab).to receive(:ee?).and_return(false)
     allow(BulkImports::Pipeline::Context).to receive(:new).and_return(context)
-    stub_http_requests
   end
 
   describe '#execute' do
-    it "starts the entity and run its pipelines" do
+    it 'starts the entity and run its pipelines' do
       expect(bulk_import_entity).to receive(:start).and_call_original
       expect_to_run_pipeline BulkImports::Groups::Pipelines::GroupPipeline, context: context
       expect_to_run_pipeline BulkImports::Groups::Pipelines::SubgroupEntitiesPipeline, context: context
@@ -32,6 +32,18 @@ RSpec.describe BulkImports::Importers::GroupImporter do
 
       expect(bulk_import_entity.reload).to be_finished
     end
+
+    context 'when failed' do
+      let(:bulk_import_entity) { create(:bulk_import_entity, :failed, bulk_import: bulk_import) }
+
+      it 'does not transition entity to finished state' do
+        allow(bulk_import_entity).to receive(:start!)
+
+        subject.execute
+
+        expect(bulk_import_entity.reload).to be_failed
+      end
+    end
   end
 
   def expect_to_run_pipeline(klass, context:)
@@ -39,18 +51,4 @@ RSpec.describe BulkImports::Importers::GroupImporter do
       expect(pipeline).to receive(:run).with(context)
     end
   end
-
-  def stub_http_requests
-    double_response = double(
-      code: 200,
-      success?: true,
-      parsed_response: {},
-      headers: {}
-    )
-
-    allow_next_instance_of(BulkImports::Clients::Http) do |client|
-      allow(client).to receive(:get).and_return(double_response)
-      allow(client).to receive(:post).and_return(double_response)
-    end
-  end
 end
diff --git a/spec/lib/bulk_imports/pipeline/runner_spec.rb b/spec/lib/bulk_imports/pipeline/runner_spec.rb
index 8c882c799ec..60833e83dcc 100644
--- a/spec/lib/bulk_imports/pipeline/runner_spec.rb
+++ b/spec/lib/bulk_imports/pipeline/runner_spec.rb
@@ -3,26 +3,32 @@
 require 'spec_helper'
 
 RSpec.describe BulkImports::Pipeline::Runner do
-  describe 'pipeline runner' do
-    before do
-      extractor = Class.new do
-        def initialize(options = {}); end
+  let(:extractor) do
+    Class.new do
+      def initialize(options = {}); end
 
-        def extract(context); end
-      end
+      def extract(context); end
+    end
+  end
 
-      transformer = Class.new do
-        def initialize(options = {}); end
+  let(:transformer) do
+    Class.new do
+      def initialize(options = {}); end
 
-        def transform(context, entry); end
-      end
+      def transform(context); end
+    end
+  end
 
-      loader = Class.new do
-        def initialize(options = {}); end
+  let(:loader) do
+    Class.new do
+      def initialize(options = {}); end
 
-        def load(context, entry); end
-      end
+      def load(context); end
+    end
+  end
 
+  describe 'pipeline runner' do
+    before do
       stub_const('BulkImports::Extractor', extractor)
       stub_const('BulkImports::Transformer', transformer)
       stub_const('BulkImports::Loader', loader)
@@ -38,37 +44,126 @@ RSpec.describe BulkImports::Pipeline::Runner do
       stub_const('BulkImports::MyPipeline', pipeline)
     end
 
-    it 'runs pipeline extractor, transformer, loader' do
-      context = instance_double(
-        BulkImports::Pipeline::Context,
-        entity: instance_double(BulkImports::Entity, id: 1, source_type: 'group')
-      )
-      entries = [{ foo: :bar }]
-
-      expect_next_instance_of(BulkImports::Extractor) do |extractor|
-        expect(extractor).to receive(:extract).with(context).and_return(entries)
+    context 'when entity is not marked as failed' do
+      let(:context) do
+        instance_double(
+          BulkImports::Pipeline::Context,
+          entity: instance_double(BulkImports::Entity, id: 1, source_type: 'group', failed?: false)
+        )
       end
 
-      expect_next_instance_of(BulkImports::Transformer) do |transformer|
-        expect(transformer).to receive(:transform).with(context, entries.first).and_return(entries.first)
+      it 'runs pipeline extractor, transformer, loader' do
+        entries = [{ foo: :bar }]
+
+        expect_next_instance_of(BulkImports::Extractor) do |extractor|
+          expect(extractor).to receive(:extract).with(context).and_return(entries)
+        end
+
+        expect_next_instance_of(BulkImports::Transformer) do |transformer|
+          expect(transformer).to receive(:transform).with(context, entries.first).and_return(entries.first)
+        end
+
+        expect_next_instance_of(BulkImports::Loader) do |loader|
+          expect(loader).to receive(:load).with(context, entries.first)
+        end
+
+        expect_next_instance_of(Gitlab::Import::Logger) do |logger|
+          expect(logger).to receive(:info)
+            .with(
+              message: 'Pipeline started',
+              pipeline_class: 'BulkImports::MyPipeline',
+              bulk_import_entity_id: 1,
+              bulk_import_entity_type: 'group'
+            )
+          expect(logger).to receive(:info)
+            .with(bulk_import_entity_id: 1, bulk_import_entity_type: 'group', extractor: 'BulkImports::Extractor')
+          expect(logger).to receive(:info)
+            .with(bulk_import_entity_id: 1, bulk_import_entity_type: 'group', transformer: 'BulkImports::Transformer')
+          expect(logger).to receive(:info)
+            .with(bulk_import_entity_id: 1, bulk_import_entity_type: 'group', loader: 'BulkImports::Loader')
+        end
+
+        BulkImports::MyPipeline.new.run(context)
       end
 
-      expect_next_instance_of(BulkImports::Loader) do |loader|
-        expect(loader).to receive(:load).with(context, entries.first)
+      context 'when exception is raised' do
+        let(:entity) { create(:bulk_import_entity, :created) }
+        let(:context) { BulkImports::Pipeline::Context.new(entity: entity) }
+
+        before do
+          allow_next_instance_of(BulkImports::Extractor) do |extractor|
+            allow(extractor).to receive(:extract).with(context).and_raise(StandardError, 'Error!')
+          end
+        end
+
+        it 'logs import failure' do
+          BulkImports::MyPipeline.new.run(context)
+
+          failure = entity.failures.first
+
+          expect(failure).to be_present
+          expect(failure.pipeline_class).to eq('BulkImports::MyPipeline')
+          expect(failure.exception_class).to eq('StandardError')
+          expect(failure.exception_message).to eq('Error!')
+        end
+
+        context 'when pipeline is marked to abort on failure' do
+          before do
+            BulkImports::MyPipeline.abort_on_failure!
+          end
+
+          it 'marks entity as failed' do
+            BulkImports::MyPipeline.new.run(context)
+
+            expect(entity.failed?).to eq(true)
+          end
+
+          it 'logs warn message' do
+            expect_next_instance_of(Gitlab::Import::Logger) do |logger|
+              expect(logger).to receive(:warn)
+                .with(
+                  message: 'Pipeline failed',
+                  pipeline_class: 'BulkImports::MyPipeline',
+                  bulk_import_entity_id: entity.id,
+                  bulk_import_entity_type: entity.source_type
+                )
+            end
+
+            BulkImports::MyPipeline.new.run(context)
+          end
+        end
+
+        context 'when pipeline is not marked to abort on failure' do
+          it 'marks entity as failed' do
+            BulkImports::MyPipeline.new.run(context)
+
+            expect(entity.failed?).to eq(false)
+          end
+        end
       end
+    end
 
-      expect_next_instance_of(Gitlab::Import::Logger) do |logger|
-        expect(logger).to receive(:info)
-          .with(message: "Pipeline started", pipeline: 'BulkImports::MyPipeline', entity: 1, entity_type: 'group')
-        expect(logger).to receive(:info)
-          .with(entity: 1, entity_type: 'group', extractor: 'BulkImports::Extractor')
-        expect(logger).to receive(:info)
-          .with(entity: 1, entity_type: 'group', transformer: 'BulkImports::Transformer')
-        expect(logger).to receive(:info)
-          .with(entity: 1, entity_type: 'group', loader: 'BulkImports::Loader')
+    context 'when entity is marked as failed' do
+      let(:context) do
+        instance_double(
+          BulkImports::Pipeline::Context,
+          entity: instance_double(BulkImports::Entity, id: 1, source_type: 'group', failed?: true)
+        )
       end
 
-      BulkImports::MyPipeline.new.run(context)
+      it 'logs and returns without execution' do
+        expect_next_instance_of(Gitlab::Import::Logger) do |logger|
+          expect(logger).to receive(:info)
+            .with(
+              message: 'Skipping due to failed pipeline status',
+              pipeline_class: 'BulkImports::MyPipeline',
+              bulk_import_entity_id: 1,
+              bulk_import_entity_type: 'group'
+            )
+        end
+
+        BulkImports::MyPipeline.new.run(context)
+      end
     end
   end
 end
diff --git a/spec/lib/bulk_imports/pipeline/attributes_spec.rb b/spec/lib/bulk_imports/pipeline_spec.rb
index 54c5dbd4cae..94052be7df2 100644
--- a/spec/lib/bulk_imports/pipeline/attributes_spec.rb
+++ b/spec/lib/bulk_imports/pipeline_spec.rb
@@ -2,7 +2,7 @@
 
 require 'spec_helper'
 
-RSpec.describe BulkImports::Pipeline::Attributes do
+RSpec.describe BulkImports::Pipeline do
   describe 'pipeline attributes' do
     before do
       stub_const('BulkImports::Extractor', Class.new)
@@ -10,7 +10,9 @@ RSpec.describe BulkImports::Pipeline::Attributes do
       stub_const('BulkImports::Loader', Class.new)
 
       klass = Class.new do
-        include BulkImports::Pipeline::Attributes
+        include BulkImports::Pipeline
+
+        abort_on_failure!
 
         extractor BulkImports::Extractor, { foo: :bar }
         transformer BulkImports::Transformer, { foo: :bar }
@@ -25,6 +27,7 @@ RSpec.describe BulkImports::Pipeline::Attributes do
         expect(BulkImports::MyPipeline.extractors).to contain_exactly({ klass: BulkImports::Extractor, options: { foo: :bar } })
         expect(BulkImports::MyPipeline.transformers).to contain_exactly({ klass: BulkImports::Transformer, options: { foo: :bar } })
         expect(BulkImports::MyPipeline.loaders).to contain_exactly({ klass: BulkImports::Loader, options: { foo: :bar } })
+        expect(BulkImports::MyPipeline.abort_on_failure?).to eq(true)
       end
     end
 
@@ -36,6 +39,7 @@ RSpec.describe BulkImports::Pipeline::Attributes do
         BulkImports::MyPipeline.extractor(klass, options)
         BulkImports::MyPipeline.transformer(klass, options)
         BulkImports::MyPipeline.loader(klass, options)
+        BulkImports::MyPipeline.abort_on_failure!
 
         expect(BulkImports::MyPipeline.extractors)
           .to contain_exactly(
@@ -51,6 +55,8 @@ RSpec.describe BulkImports::Pipeline::Attributes do
           .to contain_exactly(
             { klass: BulkImports::Loader, options: { foo: :bar } },
             { klass: klass, options: options })
+
+        expect(BulkImports::MyPipeline.abort_on_failure?).to eq(true)
       end
     end
   end