Merge branch 'jhyson/export_failures' into 'master'

Ensure all errors are logged in Group Import

See merge request gitlab-org/gitlab!25619

app/services/groups/import_export/export_service.rb

@@ -11,11 +11,7 @@ module Groups
       end
 
       def execute
-        unless @current_user.can?(:admin_group, @group)
-          raise ::Gitlab::ImportExport::Error.new(
-            "User with ID: %s does not have permission to Group %s with ID: %s." %
-              [@current_user.id, @group.name, @group.id])
-        end
+        validate_user_permissions
 
         save!
       ensure
@@ -26,6 +22,14 @@ module Groups
 
       attr_accessor :shared
 
+      def validate_user_permissions
+        unless @current_user.can?(:admin_group, @group)
+          @shared.error(::Gitlab::ImportExport::Error.permission_error(@current_user, @group))
+
+          notify_error!
+        end
+      end
+
       def save!
         if savers.all?(&:save)
           notify_success

app/services/groups/import_export/import_service.rb

@@ -12,15 +12,14 @@ module Groups
       end
 
       def execute
-        validate_user_permissions
+        if valid_user_permissions? && import_file && restorer.restore
+          notify_success
 
-        if import_file && restorer.restore
           @group
         else
-          raise StandardError.new(@shared.errors.to_sentence)
+          notify_error!
         end
-      rescue => e
-        raise StandardError.new(e.message)
+
       ensure
         remove_import_file
       end
@@ -49,13 +48,37 @@ module Groups
         upload.save!
       end
 
-      def validate_user_permissions
-        unless current_user.can?(:admin_group, group)
-          raise ::Gitlab::ImportExport::Error.new(
-            "User with ID: %s does not have permission to Group %s with ID: %s." %
-              [current_user.id, group.name, group.id])
+      def valid_user_permissions?
+        if current_user.can?(:admin_group, group)
+          true
+        else
+          @shared.error(::Gitlab::ImportExport::Error.permission_error(current_user, group))
+
+          false
         end
       end
+
+      def notify_success
+        @shared.logger.info(
+          group_id:   @group.id,
+          group_name: @group.name,
+          message:    'Group Import/Export: Import succeeded'
+        )
+      end
+
+      def notify_error
+        @shared.logger.error(
+          group_id:   @group.id,
+          group_name: @group.name,
+          message:    "Group Import/Export: Errors occurred, see '#{Gitlab::ErrorTracking::Logger.file_name}' for details"
+        )
+      end
+
+      def notify_error!
+        notify_error
+
+        raise Gitlab::ImportExport::Error.new(@shared.errors.to_sentence)
+      end
     end
   end
 end

app/services/projects/import_export/export_service.rb

@@ -5,9 +5,7 @@ module Projects
     class ExportService < BaseService
       def execute(after_export_strategy = nil, options = {})
         unless project.template_source? || can?(current_user, :admin_project, project)
-          raise ::Gitlab::ImportExport::Error.new(
-            "User with ID: %s does not have permission to Project %s with ID: %s." %
-              [current_user.id, project.name, project.id])
+          raise ::Gitlab::ImportExport::Error.permission_error(current_user, project)
         end
 
         @shared = project.import_export_shared

changelogs/unreleased/jhyson-export_failures.yml

+---
+title: Ensure all errors are logged in Group Import
+merge_request: 25619
+author:
+type: changed

lib/gitlab/import_export/error.rb

@@ -2,6 +2,13 @@
 
 module Gitlab
   module ImportExport
-    Error = Class.new(StandardError)
+    class Error < StandardError
+      def self.permission_error(user, importable)
+        self.new(
+          "User with ID: %s does not have required permissions for %s: %s with ID: %s" %
+          [user.id, importable.class.name, importable.name, importable.id]
+        )
+      end
+    end
   end
 end

lib/gitlab/import_export/group/tree_restorer.rb

@@ -49,11 +49,7 @@ module Gitlab
           json = IO.read(@path)
           ActiveSupport::JSON.decode(json)
         rescue => e
-          @shared.logger.error(
-            group_id:   @group.id,
-            group_name: @group.name,
-            message:    "Import/Export error: #{e.message}"
-          )
+          @shared.error(e)
 
           raise Gitlab::ImportExport::Error.new('Incorrect JSON format')
         end

lib/gitlab/import_export/shared.rb

@@ -94,14 +94,6 @@ module Gitlab
         end
       end
 
-      def log_error(details)
-        @logger.error(log_base_data.merge(details))
-      end
-
-      def log_debug(details)
-        @logger.debug(log_base_data.merge(details))
-      end
-
       def log_base_data
         log = {
           importer:        'Import/Export',

spec/lib/gitlab/import_export/error_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::ImportExport::Error do
+  describe '.permission_error' do
+    subject(:error) do
+      described_class.permission_error(user, importable)
+    end
+
+    let(:user) { build(:user, id: 1) }
+
+    context 'when supplied a project' do
+      let(:importable) { build(:project, id: 1, name: 'project1') }
+
+      it 'returns an error with the correct message' do
+        expect(error.message)
+          .to eq 'User with ID: 1 does not have required permissions for Project: project1 with ID: 1'
+      end
+    end
+
+    context 'when supplied a group' do
+      let(:importable) { build(:group, id: 1, name: 'group1') }
+
+      it 'returns an error with the correct message' do
+        expect(error.message)
+          .to eq 'User with ID: 1 does not have required permissions for Group: group1 with ID: 1'
+      end
+    end
+  end
+end

spec/services/groups/import_export/export_service_spec.rb

@@ -38,12 +38,31 @@ describe Groups::ImportExport::ExportService do
       let!(:another_user) { create(:user) }
       let(:service) { described_class.new(group: group, user: another_user, params: { shared: shared }) }
 
+      let(:expected_message) do
+        "User with ID: %s does not have required permissions for Group: %s with ID: %s" %
+          [another_user.id, group.name, group.id]
+      end
+
       it 'fails' do
-        expected_message =
-          "User with ID: %s does not have permission to Group %s with ID: %s." %
-            [another_user.id, group.name, group.id]
         expect { service.execute }.to raise_error(Gitlab::ImportExport::Error).with_message(expected_message)
       end
+
+      it 'logs the error' do
+        expect(shared.logger).to receive(:error).with(
+          group_id:   group.id,
+          group_name: group.name,
+          error:      expected_message,
+          message:    'Group Import/Export: Export failed'
+        )
+
+        expect { service.execute }.to raise_error(Gitlab::ImportExport::Error)
+      end
+
+      it 'tracks the error' do
+        expect(shared).to receive(:error) { |param| expect(param.message).to eq expected_message }
+
+        expect { service.execute }.to raise_error(Gitlab::ImportExport::Error)
+      end
     end
 
     context 'when export fails' do

spec/services/groups/import_export/import_service_spec.rb

@@ -9,6 +9,8 @@ describe Groups::ImportExport::ImportService do
     let(:service) { described_class.new(group: group, user: user) }
     let(:import_file) { fixture_file_upload('spec/fixtures/group_export.tar.gz') }
 
+    let(:import_logger) { instance_double(Gitlab::Import::Logger) }
+
     subject { service.execute }
 
     before do
@@ -25,13 +27,82 @@ describe Groups::ImportExport::ImportService do
 
         expect(group.import_export_upload.import_file.file).to be_nil
       end
+
+      it 'logs the import success' do
+        allow(Gitlab::Import::Logger).to receive(:build).and_return(import_logger)
+
+        expect(import_logger).to receive(:info).with(
+          group_id:   group.id,
+          group_name: group.name,
+          message:    'Group Import/Export: Import succeeded'
+        )
+
+        subject
+      end
     end
 
     context 'when user does not have correct permissions' do
       let(:user) { create(:user) }
 
-      it 'raises exception' do
-        expect { subject }.to raise_error(StandardError)
+      it 'logs the error and raises an exception' do
+        allow(Gitlab::Import::Logger).to receive(:build).and_return(import_logger)
+
+        expect(import_logger).to receive(:error).with(
+          group_id:   group.id,
+          group_name: group.name,
+          message:    a_string_including('Errors occurred')
+        )
+
+        expect { subject }.to raise_error(Gitlab::ImportExport::Error)
+      end
+
+      it 'tracks the error' do
+        shared = Gitlab::ImportExport::Shared.new(group)
+        allow(Gitlab::ImportExport::Shared).to receive(:new).and_return(shared)
+
+        expect(shared).to receive(:error) do |param|
+          expect(param.message).to include 'does not have required permissions for'
+        end
+
+        expect { subject }.to raise_error(Gitlab::ImportExport::Error)
+      end
+    end
+
+    context 'when there are errors with the import file' do
+      let(:import_file) { fixture_file_upload('spec/fixtures/symlink_export.tar.gz') }
+
+      before do
+        allow(Gitlab::Import::Logger).to receive(:build).and_return(import_logger)
+      end
+
+      it 'logs the error and raises an exception' do
+        expect(import_logger).to receive(:error).with(
+          group_id:   group.id,
+          group_name: group.name,
+          message:    a_string_including('Errors occurred')
+        )
+
+        expect { subject }.to raise_error(Gitlab::ImportExport::Error)
+      end
+    end
+
+    context 'when there are errors with the sub-relations' do
+      let(:import_file) { fixture_file_upload('spec/fixtures/group_export_invalid_subrelations.tar.gz') }
+
+      it 'successfully imports the group' do
+        expect(subject).to be_truthy
+      end
+
+      it 'logs the import success' do
+        allow(Gitlab::Import::Logger).to receive(:build).and_return(import_logger)
+
+        expect(import_logger).to receive(:info).with(
+          group_id:   group.id,
+          group_name: group.name,
+          message:    'Group Import/Export: Import succeeded'
+        )
+
+        subject
       end
     end
   end

spec/services/projects/import_export/export_service_spec.rb

@@ -164,7 +164,7 @@ describe Projects::ImportExport::ExportService do
 
       it 'fails' do
         expected_message =
-          "User with ID: %s does not have permission to Project %s with ID: %s." %
+          "User with ID: %s does not have required permissions for Project: %s with ID: %s" %
             [another_user.id, project.name, project.id]
         expect { service.execute }.to raise_error(Gitlab::ImportExport::Error).with_message(expected_message)
       end