Update GitLab Import to log to all exceptions

We want to ensure that all exceptions go through the same exception
handler so that we have consistent error handling.

Update manual logging instances to use the shared handler so that we can
take advantage of the exception handling introduced in
https://gitlab.com/gitlab-org/gitlab/-/merge_requests/17819

Ensure that all raised errors are logged to this shared handler before
continuing to raise.

Addresses https://gitlab.com/gitlab-org/gitlab/issues/38101

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,19 @@ module Groups
 
       attr_accessor :shared
 
+      def validate_user_permissions
+        unless @current_user.can?(:admin_group, @group)
+          @shared.error(
+            ::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]
+            )
+          )
+
+          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,42 @@ 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.new(
+              "User with ID: %s does not have permission to Group %s with ID: %s." %
+              [current_user.id, group.name, group.id]
+            )
+          )
+
+          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

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/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/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 permission to 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 permission to'
+        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