Import group with ndjson only if needed

app/services/groups/import_export/import_service.rb

@@ -36,7 +36,7 @@ module Groups
 
       def restorer
         @restorer ||=
-          if ::Feature.enabled?(:group_import_export_ndjson, @group&.parent)
+          if ndjson?
             Gitlab::ImportExport::Group::TreeRestorer.new(
               user: @current_user,
               shared: @shared,
@@ -52,6 +52,11 @@ module Groups
           end
       end
 
+      def ndjson?
+        ::Feature.enabled?(:group_import_export_ndjson, @group&.parent) &&
+          File.exist?(File.join(@shared.export_path, 'tree/groups/_all.ndjson'))
+      end
+
       def remove_import_file
         upload = @group.import_export_upload
 

spec/services/groups/import_export/import_service_spec.rb

@@ -3,18 +3,15 @@
 require 'spec_helper'
 
 describe Groups::ImportExport::ImportService do
-  describe '#execute with TreeRestorer' do
+  context 'with group_import_export_ndjson feature flag disabled' do
     let(:user) { create(:admin) }
     let(:group) { create(:group) }
-    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 }
+    subject(:service) { described_class.new(group: group, user: user) }
 
     before do
-      stub_feature_flags(group_import_export_ndjson: true)
+      stub_feature_flags(group_import_export_ndjson: false)
 
       ImportExportUpload.create(group: group, import_file: import_file)
 
@@ -23,186 +20,227 @@ describe Groups::ImportExport::ImportService do
       allow(import_logger).to receive(:info)
     end
 
-    context 'when user has correct permissions' do
-      it 'imports group structure successfully' do
-        expect(subject).to be_truthy
-      end
+    context 'with a json file' do
+      let(:import_file) { fixture_file_upload('spec/fixtures/legacy_group_export.tar.gz') }
 
-      it 'removes import file' do
-        subject
+      it 'uses LegacyTreeRestorer to import the file' do
+        expect(Gitlab::ImportExport::Group::LegacyTreeRestorer).to receive(:new).and_call_original
 
-        expect(group.import_export_upload.import_file.file).to be_nil
+        service.execute
       end
+    end
 
-      it 'logs the import success' do
-        expect(import_logger).to receive(:info).with(
-          group_id:   group.id,
-          group_name: group.name,
-          message:    'Group Import/Export: Import succeeded'
-        ).once
+    context 'with a ndjson file' do
+      let(:import_file) { fixture_file_upload('spec/fixtures/group_export.tar.gz') }
 
-        subject
+      it 'fails to import' do
+        expect { service.execute }.to raise_error(Gitlab::ImportExport::Error, 'Incorrect JSON format')
       end
     end
+  end
 
-    context 'when user does not have correct permissions' do
-      let(:user) { create(:user) }
+  context 'with group_import_export_ndjson feature flag enabled' do
+    before do
+      stub_feature_flags(group_import_export_ndjson: true)
+    end
+
+    context 'when importing a ndjson export' do
+      let(:user) { create(:admin) }
+      let(:group) { create(:group) }
+      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 }
 
-      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')
-        )
+      before do
+        ImportExportUpload.create(group: group, import_file: import_file)
 
-        expect { subject }.to raise_error(Gitlab::ImportExport::Error)
+        allow(Gitlab::Import::Logger).to receive(:build).and_return(import_logger)
+        allow(import_logger).to receive(:error)
+        allow(import_logger).to receive(:info)
       end
 
-      it 'tracks the error' do
-        shared = Gitlab::ImportExport::Shared.new(group)
-        allow(Gitlab::ImportExport::Shared).to receive(:new).and_return(shared)
+      context 'when user has correct permissions' do
+        it 'imports group structure successfully' do
+          expect(subject).to be_truthy
+        end
+
+        it 'removes import file' do
+          subject
 
-        expect(shared).to receive(:error) do |param|
-          expect(param.message).to include 'does not have required permissions for'
+          expect(group.import_export_upload.import_file.file).to be_nil
         end
 
-        expect { subject }.to raise_error(Gitlab::ImportExport::Error)
+        it 'logs the import success' do
+          expect(import_logger).to receive(:info).with(
+            group_id:   group.id,
+            group_name: group.name,
+            message:    'Group Import/Export: Import succeeded'
+          ).once
+
+          subject
+        end
       end
-    end
 
-    context 'when there are errors with the import file' do
-      let(:import_file) { fixture_file_upload('spec/fixtures/symlink_export.tar.gz') }
+      context 'when user does not have correct permissions' do
+        let(:user) { create(:user) }
 
-      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')
-        ).once
+        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
+          expect { subject }.to raise_error(Gitlab::ImportExport::Error)
+        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 'tracks the error' do
+          shared = Gitlab::ImportExport::Shared.new(group)
+          allow(Gitlab::ImportExport::Shared).to receive(:new).and_return(shared)
 
-      it 'successfully imports the group' do
-        expect(subject).to be_truthy
+          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
 
-      it 'logs the import success' do
-        allow(Gitlab::Import::Logger).to receive(:build).and_return(import_logger)
+      context 'when there are errors with the import file' do
+        let(:import_file) { fixture_file_upload('spec/fixtures/symlink_export.tar.gz') }
 
-        expect(import_logger).to receive(:info).with(
-          group_id: group.id,
-          group_name: group.name,
-          message: 'Group Import/Export: Import succeeded'
-        )
+        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')
+          ).once
 
-        subject
+          expect { subject }.to raise_error(Gitlab::ImportExport::Error)
+        end
       end
-    end
-  end
-
-  describe '#execute with LegacyTreeRestorer' do
-    let(:user) { create(:admin) }
-    let(:group) { create(:group) }
-    let(:service) { described_class.new(group: group, user: user) }
-    let(:import_file) { fixture_file_upload('spec/fixtures/legacy_group_export.tar.gz') }
 
-    let(:import_logger) { instance_double(Gitlab::Import::Logger) }
+      context 'when there are errors with the sub-relations' do
+        let(:import_file) { fixture_file_upload('spec/fixtures/group_export_invalid_subrelations.tar.gz') }
 
-    subject { service.execute }
+        it 'successfully imports the group' do
+          expect(subject).to be_truthy
+        end
 
-    before do
-      stub_feature_flags(group_import_export_ndjson: false)
+        it 'logs the import success' do
+          allow(Gitlab::Import::Logger).to receive(:build).and_return(import_logger)
 
-      ImportExportUpload.create(group: group, import_file: import_file)
+          expect(import_logger).to receive(:info).with(
+            group_id: group.id,
+            group_name: group.name,
+            message: 'Group Import/Export: Import succeeded'
+          )
 
-      allow(Gitlab::Import::Logger).to receive(:build).and_return(import_logger)
-      allow(import_logger).to receive(:error)
-      allow(import_logger).to receive(:info)
+          subject
+        end
+      end
     end
 
-    context 'when user has correct permissions' do
-      it 'imports group structure successfully' do
-        expect(subject).to be_truthy
-      end
+    context 'when importing a json export' do
+      let(:user) { create(:admin) }
+      let(:group) { create(:group) }
+      let(:service) { described_class.new(group: group, user: user) }
+      let(:import_file) { fixture_file_upload('spec/fixtures/legacy_group_export.tar.gz') }
 
-      it 'removes import file' do
-        subject
+      let(:import_logger) { instance_double(Gitlab::Import::Logger) }
 
-        expect(group.import_export_upload.import_file.file).to be_nil
-      end
+      subject { service.execute }
 
-      it 'logs the import success' do
-        expect(import_logger).to receive(:info).with(
-          group_id:   group.id,
-          group_name: group.name,
-          message:    'Group Import/Export: Import succeeded'
-        ).once
+      before do
+        ImportExportUpload.create(group: group, import_file: import_file)
 
-        subject
+        allow(Gitlab::Import::Logger).to receive(:build).and_return(import_logger)
+        allow(import_logger).to receive(:error)
+        allow(import_logger).to receive(:info)
       end
-    end
 
-    context 'when user does not have correct permissions' do
-      let(:user) { create(:user) }
+      context 'when user has correct permissions' do
+        it 'imports group structure successfully' do
+          expect(subject).to be_truthy
+        end
+
+        it 'removes import file' do
+          subject
+
+          expect(group.import_export_upload.import_file.file).to be_nil
+        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')
-        )
+        it 'logs the import success' do
+          expect(import_logger).to receive(:info).with(
+            group_id:   group.id,
+            group_name: group.name,
+            message:    'Group Import/Export: Import succeeded'
+          ).once
 
-        expect { subject }.to raise_error(Gitlab::ImportExport::Error)
+          subject
+        end
       end
 
-      it 'tracks the error' do
-        shared = Gitlab::ImportExport::Shared.new(group)
-        allow(Gitlab::ImportExport::Shared).to receive(:new).and_return(shared)
+      context 'when user does not have correct permissions' do
+        let(:user) { create(:user) }
 
-        expect(shared).to receive(:error) do |param|
-          expect(param.message).to include 'does not have required permissions for'
+        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
 
-        expect { subject }.to raise_error(Gitlab::ImportExport::Error)
+        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
-    end
 
-    context 'when there are errors with the import file' do
-      let(:import_file) { fixture_file_upload('spec/fixtures/legacy_symlink_export.tar.gz') }
+      context 'when there are errors with the import file' do
+        let(:import_file) { fixture_file_upload('spec/fixtures/legacy_symlink_export.tar.gz') }
 
-      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')
-        ).once
+        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')
+          ).once
 
-        expect { subject }.to raise_error(Gitlab::ImportExport::Error)
+          expect { subject }.to raise_error(Gitlab::ImportExport::Error)
+        end
       end
-    end
 
-    context 'when there are errors with the sub-relations' do
-      let(:import_file) { fixture_file_upload('spec/fixtures/legacy_group_export_invalid_subrelations.tar.gz') }
+      context 'when there are errors with the sub-relations' do
+        let(:import_file) { fixture_file_upload('spec/fixtures/legacy_group_export_invalid_subrelations.tar.gz') }
 
-      it 'successfully imports the group' do
-        expect(subject).to be_truthy
-      end
+        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)
+        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'
-        )
+          expect(import_logger).to receive(:info).with(
+            group_id:   group.id,
+            group_name: group.name,
+            message:    'Group Import/Export: Import succeeded'
+          )
 
-        subject
+          subject
+        end
       end
     end
   end