Merge branch 'fj-refactor-group-importer-exporter' into 'master'

Refactor code in groups' importer and exporter

See merge request gitlab-org/gitlab!52767

app/services/groups/import_export/export_service.rb

@@ -12,13 +12,13 @@ module Groups
       end
 
       def async_execute
-        GroupExportWorker.perform_async(@current_user.id, @group.id, @params)
+        GroupExportWorker.perform_async(current_user.id, group.id, params)
       end
 
       def execute
         validate_user_permissions
 
-        remove_existing_export! if @group.export_file_exists?
+        remove_existing_export! if group.export_file_exists?
 
         save!
       ensure
@@ -27,25 +27,29 @@ module Groups
 
       private
 
+      attr_reader :group, :current_user, :params
       attr_accessor :shared
 
       def validate_user_permissions
-        unless @current_user.can?(:admin_group, @group)
-          @shared.error(::Gitlab::ImportExport::Error.permission_error(@current_user, @group))
+        unless current_user.can?(:admin_group, group)
+          shared.error(::Gitlab::ImportExport::Error.permission_error(current_user, group))
 
           notify_error!
         end
       end
 
       def remove_existing_export!
-        import_export_upload = @group.import_export_upload
+        import_export_upload = group.import_export_upload
 
         import_export_upload.remove_export_file!
         import_export_upload.save
       end
 
       def save!
-        if savers.all?(&:save)
+        # We cannot include the file_saver with the other savers because
+        # it removes the tmp dir. This means that if we want to add new savers
+        # in EE the data won't be available.
+        if savers.all?(&:save) && file_saver.save
           notify_success
         else
           notify_error!
@@ -53,32 +57,36 @@ module Groups
       end
 
       def savers
-        [version_saver, tree_exporter, file_saver]
+        [version_saver, tree_exporter]
       end
 
       def tree_exporter
         tree_exporter_class.new(
-          group: @group,
-          current_user: @current_user,
-          shared: @shared,
-          params: @params
+          group: group,
+          current_user: current_user,
+          shared: shared,
+          params: params
         )
       end
 
       def tree_exporter_class
-        if ::Feature.enabled?(:group_export_ndjson, @group&.parent, default_enabled: true)
+        if ndjson?
           Gitlab::ImportExport::Group::TreeSaver
         else
           Gitlab::ImportExport::Group::LegacyTreeSaver
         end
       end
 
+      def ndjson?
+        ::Feature.enabled?(:group_export_ndjson, group&.parent, default_enabled: :yaml)
+      end
+
       def version_saver
         Gitlab::ImportExport::VersionSaver.new(shared: shared)
       end
 
       def file_saver
-        Gitlab::ImportExport::Saver.new(exportable: @group, shared: @shared)
+        Gitlab::ImportExport::Saver.new(exportable: group, shared: shared)
       end
 
       def remove_archive_tmp_dir
@@ -94,22 +102,22 @@ module Groups
       def notify_success
         @logger.info(
           message: 'Group Export succeeded',
-          group_id: @group.id,
-          group_name: @group.name
+          group_id: group.id,
+          group_name: group.name
         )
 
-        notification_service.group_was_exported(@group, @current_user)
+        notification_service.group_was_exported(group, current_user)
       end
 
       def notify_error
         @logger.error(
           message: 'Group Export failed',
-          group_id: @group.id,
-          group_name: @group.name,
-          errors: @shared.errors.join(', ')
+          group_id: group.id,
+          group_name: group.name,
+          errors: shared.errors.join(', ')
         )
 
-        notification_service.group_was_not_exported(@group, @current_user, @shared.errors)
+        notification_service.group_was_not_exported(group, current_user, shared.errors)
       end
 
       def notification_service

app/services/groups/import_export/import_service.rb

@@ -3,7 +3,7 @@
 module Groups
   module ImportExport
     class ImportService
-      attr_reader :current_user, :group, :params
+      attr_reader :current_user, :group, :shared
 
       def initialize(group:, user:)
         @group = group
@@ -26,10 +26,10 @@ module Groups
       end
 
       def execute
-        if valid_user_permissions? && import_file && restorer.restore
+        if valid_user_permissions? && import_file && restorers.all?(&:restore)
           notify_success
 
-          @group
+          group
         else
           notify_error!
         end
@@ -43,37 +43,41 @@ module Groups
 
       def import_file
         @import_file ||= Gitlab::ImportExport::FileImporter.import(
-          importable: @group,
+          importable: group,
           archive_file: nil,
-          shared: @shared
+          shared: shared
         )
       end
 
-      def restorer
-        @restorer ||=
+      def restorers
+        [tree_restorer]
+      end
+
+      def tree_restorer
+        @tree_restorer ||=
           if ndjson?
             Gitlab::ImportExport::Group::TreeRestorer.new(
-              user: @current_user,
-              shared: @shared,
-              group: @group
+              user: current_user,
+              shared: shared,
+              group: group
             )
           else
             Gitlab::ImportExport::Group::LegacyTreeRestorer.new(
-              user: @current_user,
-              shared: @shared,
-              group: @group,
+              user: current_user,
+              shared: shared,
+              group: group,
               group_hash: nil
             )
           end
       end
 
       def ndjson?
-        ::Feature.enabled?(:group_import_ndjson, @group&.parent, default_enabled: true) &&
-          File.exist?(File.join(@shared.export_path, 'tree/groups/_all.ndjson'))
+        ::Feature.enabled?(:group_import_ndjson, group&.parent, default_enabled: true) &&
+          File.exist?(File.join(shared.export_path, 'tree/groups/_all.ndjson'))
       end
 
       def remove_import_file
-        upload = @group.import_export_upload
+        upload = group.import_export_upload
 
         return unless upload&.import_file&.file
 
@@ -85,7 +89,7 @@ module Groups
         if current_user.can?(:admin_group, group)
           true
         else
-          @shared.error(::Gitlab::ImportExport::Error.permission_error(current_user, group))
+          shared.error(::Gitlab::ImportExport::Error.permission_error(current_user, group))
 
           false
         end
@@ -93,16 +97,16 @@ module Groups
 
       def notify_success
         @logger.info(
-          group_id:   @group.id,
-          group_name: @group.name,
+          group_id:   group.id,
+          group_name: group.name,
           message:    'Group Import/Export: Import succeeded'
         )
       end
 
       def notify_error
         @logger.error(
-          group_id:   @group.id,
-          group_name: @group.name,
+          group_id:   group.id,
+          group_name: group.name,
           message:    "Group Import/Export: Errors occurred, see '#{Gitlab::ErrorTracking::Logger.file_name}' for details"
         )
       end
@@ -110,11 +114,11 @@ module Groups
       def notify_error!
         notify_error
 
-        raise Gitlab::ImportExport::Error.new(@shared.errors.to_sentence)
+        raise Gitlab::ImportExport::Error.new(shared.errors.to_sentence)
       end
 
       def remove_base_tmp_dir
-        FileUtils.rm_rf(@shared.base_path)
+        FileUtils.rm_rf(shared.base_path)
       end
     end
   end

lib/gitlab/import_export/group/tree_restorer.rb

@@ -6,7 +6,7 @@ module Gitlab
       class TreeRestorer
         include Gitlab::Utils::StrongMemoize
 
-        attr_reader :user, :shared
+        attr_reader :user, :shared, :groups_mapping
 
         def initialize(user:, shared:, group:)
           @user = user

lib/gitlab/import_export/repo_saver.rb

@@ -37,7 +37,8 @@ module Gitlab
       end
 
       def bundle_to_disk
-        mkdir_p(shared.export_path)
+        mkdir_p(File.dirname(bundle_full_path))
+
         repository.bundle_to_disk(bundle_full_path)
       rescue => e
         shared.error(e)

spec/lib/gitlab/import_export/group/tree_restorer_spec.rb

@@ -21,6 +21,7 @@ RSpec.describe Gitlab::ImportExport::Group::TreeRestorer do
         group_tree_restorer = described_class.new(user: user, shared: @shared, group: @group)
 
         expect(group_tree_restorer.restore).to be_truthy
+        expect(group_tree_restorer.groups_mapping).not_to be_empty
       end
     end
 

spec/lib/gitlab/import_export/repo_saver_spec.rb

@@ -25,6 +25,14 @@ RSpec.describe Gitlab::ImportExport::RepoSaver do
       expect(bundler.save).to be true
     end
 
+    it 'creates the directory for the repository' do
+      allow(bundler).to receive(:bundle_full_path).and_return('/foo/bar/file.tar.gz')
+
+      expect(FileUtils).to receive(:mkdir_p).with('/foo/bar', anything)
+
+      bundler.save # rubocop:disable Rails/SaveBang
+    end
+
     context 'when the repo is empty' do
       let!(:project) { create(:project) }
 

spec/services/groups/import_export/export_service_spec.rb

@@ -71,6 +71,16 @@ RSpec.describe Groups::ImportExport::ExportService do
       service.execute
     end
 
+    it 'compresses and removes tmp files' do
+      expect(group.import_export_upload).to be_nil
+      expect(Gitlab::ImportExport::Saver).to receive(:new).and_call_original
+
+      service.execute
+
+      expect(Dir.exist?(shared.archive_path)).to eq false
+      expect(File.exist?(group.import_export_upload.export_file.path)).to eq true
+    end
+
     it 'notifies the user' do
       expect_next_instance_of(NotificationService) do |instance|
         expect(instance).to receive(:group_was_exported)