Address MR comments

- Remove Sidekiq Metrics Exporter from tasks
- Add comment for feature-flag removal
- Replace in_batches with find_each
- Fix identation
- Fix write file in initializer
- Fix typo in streaming serializer
- Fix group relation tree saver
- Add missing specs
- Add spec for tree_saver spec
- Fix Tree Saver to use full_path
- Add spec for legacy_writer
- Fix JSON namespace
- Rename old Project::TreeSaver to LegacyTreeSaver
- Add feature_flag in export_service
- Fix specs

app/presenters/projects/import_export/project_export_presenter.rb

@@ -6,9 +6,7 @@ module Projects
       presents :project
 
       def project_members
-        group_members.as_json(group_members_tree).each do |group_member|
-          group_member['source_type'] = 'Project' # Make group members project members of the future import
-        end
+        super + converted_group_members
       end
 
       def as_json(*_args)
@@ -17,9 +15,16 @@ module Projects
 
       private
 
+      def converted_group_members
+        group_members.each do |group_member|
+          group_member.source_type = 'Project' # Make group members project members of the future import
+        end
+      end
+
       # rubocop: disable CodeReuse/ActiveRecord
       def group_members
         return [] unless current_user.can?(:admin_group, project.group)
+
         # We need `.where.not(user_id: nil)` here otherwise when a group has an
         # invitee, it would make the following query return 0 rows since a NULL
         # user_id would be present in the subquery

app/services/projects/import_export/export_service.rb

@@ -61,6 +61,7 @@ module Projects
         if ::Feature.enabled?(:streaming_serializer, project)
           Gitlab::ImportExport::Project::TreeSaver
         else
+          # Once we remove :streaming_serializer feature flag, Project::LegacyTreeSaver should be removed as well
           Gitlab::ImportExport::Project::LegacyTreeSaver
         end
       end

ee/spec/lib/ee/gitlab/import_export/group/tree_saver_spec.rb

@@ -185,6 +185,6 @@ describe Gitlab::ImportExport::Group::TreeSaver do
   end
 
   def group_json(filename)
-    JSON.parse(IO.read(filename))
+    ::JSON.parse(IO.read(filename))
   end
 end

ee/spec/lib/ee/gitlab/import_export/project/tree_saver_spec.rb

@@ -80,6 +80,6 @@ describe Gitlab::ImportExport::Project::TreeSaver do
   end
 
   def project_json(filename)
-    JSON.parse(IO.read(filename))
+    ::JSON.parse(IO.read(filename))
   end
 end

lib/gitlab/import_export/group/tree_saver.rb

@@ -49,7 +49,7 @@ module Gitlab
         end
 
         def tree_saver
-          @tree_saver ||= RelationTreeSaver.new
+          @tree_saver ||= LegacyRelationTreeSaver.new
         end
       end
     end

lib/gitlab/import_export/json/legacy_writer.rb

+# frozen_string_literal: true
+
 module Gitlab
   module ImportExport
     module JSON
@@ -12,7 +14,6 @@ module Gitlab
           @keys = Set.new
 
           mkdir_p(File.dirname(@path))
-          file.write('{}')
         end
 
         def close
@@ -27,10 +28,10 @@ module Gitlab
         end
 
         def write(key, value)
-          raise RuntimeError, "key '#{key}' already written" if @keys.include?(key)
+          raise ArgumentError, "key '#{key}' already written" if @keys.include?(key)
 
           # rewind by one byte, to overwrite '}'
-          file.pos = file.size-1
+          file.pos = file.size - 1
 
           file.write(',') if @keys.any?
           file.write(key.to_json)
@@ -52,7 +53,7 @@ module Gitlab
           end
 
           # rewind by two bytes, to overwrite ']}'
-          file.pos = file.size-2
+          file.pos = file.size - 2
 
           file.write(',') if @last_array_count > 0
           file.write(value.to_json)
@@ -63,7 +64,11 @@ module Gitlab
         private
 
         def file
-          @file ||= File.open(@path, "wb")
+          @file ||= begin
+            file = File.open(@path, "wb")
+            file.write('{}')
+            file
+          end
         end
       end
     end

lib/gitlab/import_export/json/streaming_serializer.rb

@@ -42,8 +42,7 @@ module Gitlab
           raise ArgumentError, 'definition needs to be Hash' unless definition.is_a?(Hash)
           raise ArgumentError, 'definition needs to have exactly one Hash element' unless definition.one?
 
-          key = definition.first.first
-          options = definition.first.second
+          key, options = definition.first
 
           record = exportable.public_send(key) # rubocop: disable GitlabSecurity/PublicSend
           if record.is_a?(ActiveRecord::Relation)
@@ -53,17 +52,14 @@ module Gitlab
           end
         end
 
-        def serialize_many_relations(key, record, options)
+        def serialize_many_relations(key, records, options)
           key_preloads = preloads&.dig(key)
+          records = records.preload(key_preloads) if key_preloads
 
-          record.in_batches(of: BATCH_SIZE) do |batch| # rubocop:disable Cop/InBatches
-            batch = batch.preload(key_preloads) if key_preloads
+          records.find_each(batch_size: BATCH_SIZE) do |record|
+            json = Raw.new(record.to_json(options))
 
-            batch.each do |item|
-              item = Raw.new(item.to_json(options))
-
-              json_writer.append(key, item)
-            end
+            json_writer.append(key, json)
           end
         end
 

lib/gitlab/import_export/relation_tree_saver.rb → lib/gitlab/import_export/legacy_relation_tree_saver.rb

@@ -2,7 +2,7 @@
 
 module Gitlab
   module ImportExport
-    class RelationTreeSaver
+    class LegacyRelationTreeSaver
       include Gitlab::ImportExport::CommandLineUtil
 
       def serialize(exportable, relations_tree)
@@ -18,7 +18,7 @@ module Gitlab
       def save(tree, dir_path, filename)
         mkdir_p(dir_path)
 
-        tree_json = JSON.generate(tree)
+        tree_json = ::JSON.generate(tree)
 
         File.write(File.join(dir_path, filename), tree_json)
       end

lib/gitlab/import_export/project/legacy_tree_saver.rb

@@ -60,7 +60,7 @@ module Gitlab
         end
 
         def tree_saver
-          @tree_saver ||= RelationTreeSaver.new
+          @tree_saver ||= Gitlab::ImportExport::LegacyRelationTreeSaver.new
         end
       end
     end

lib/gitlab/import_export/project/tree_saver.rb

@@ -15,7 +15,7 @@ module Gitlab
         end
 
         def save
-          json_writer = ImportExport::JSON::LegacyWriter.new(File.join(@shared.export_path, "project.json"))
+          json_writer = ImportExport::JSON::LegacyWriter.new(@full_path)
 
           serializer = ImportExport::JSON::StreamingSerializer.new(exportable, reader.project_tree, json_writer)
           serializer.execute
@@ -40,11 +40,10 @@ module Gitlab
 
         def exportable_params
           params = {
-          presenter_class: presenter_class,
-            current_user: @current_user,
-            group_members_tree: reader.group_members_tree
+            presenter_class: presenter_class,
+            current_user: @current_user
           }
-          params[:override_description] = @params[:description] if @params[:description]
+          params[:override_description] = @params[:description] if @params[:description].present?
           params
         end
 

lib/tasks/gitlab/import_export/export.rake

@@ -19,7 +19,6 @@ namespace :gitlab do
 
         if ENV['EXPORT_DEBUG'].present?
           ActiveRecord::Base.logger = logger
-          Gitlab::Metrics::Exporter::SidekiqExporter.instance.start
           logger.level = Logger::DEBUG
         else
           logger.level = Logger::INFO

lib/tasks/gitlab/import_export/import.rake

@@ -23,7 +23,6 @@ namespace :gitlab do
 
         if ENV['IMPORT_DEBUG'].present?
           ActiveRecord::Base.logger = logger
-          Gitlab::Metrics::Exporter::SidekiqExporter.instance.start
           logger.level = Logger::DEBUG
         else
           logger.level = Logger::INFO

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

@@ -197,6 +197,6 @@ describe Gitlab::ImportExport::Group::TreeSaver do
   end
 
   def group_json(filename)
-    JSON.parse(IO.read(filename))
+    ::JSON.parse(IO.read(filename))
   end
 end

spec/lib/gitlab/import_export/json/legacy_writer_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::ImportExport::JSON::LegacyWriter do
+  let(:path) { "#{Dir.tmpdir}/legacy_writer_spec/test.json" }
+
+  subject { described_class.new(path) }
+
+  after do
+    FileUtils.rm_rf(path)
+  end
+
+  describe "#write" do
+    context "when key is already written" do
+      it "raises exception" do
+        key = "key"
+        value = "value"
+        subject.write(key, value)
+
+        expect { subject.write(key, "new value") }.to raise_exception("key '#{key}' already written")
+      end
+    end
+
+    context "when key is not already written" do
+      context "when multiple key value pairs are stored" do
+        it "writes correct json" do
+          expected_hash = { "key" => "value_1", "key_1" => "value_2" }
+          expected_hash.each do |key, value|
+            subject.write(key, value)
+          end
+          subject.close
+
+          expect(saved_json(path)).to eq(expected_hash)
+        end
+      end
+    end
+  end
+
+  describe "#append" do
+    context "when key is already written" do
+      it "appends values under a given key" do
+        key = "key"
+        values = %w(value_1 value_2)
+        expected_hash = { key => values }
+        values.each do |value|
+          subject.append(key, value)
+        end
+        subject.close
+
+        expect(saved_json(path)).to eq(expected_hash)
+      end
+    end
+
+    context "when key is not already written" do
+      it "writes correct json" do
+        expected_hash = { "key" => ["value"] }
+        subject.append("key", "value")
+        subject.close
+
+        expect(saved_json(path)).to eq(expected_hash)
+      end
+    end
+  end
+
+  describe "#set" do
+    it "writes correct json" do
+      expected_hash = { "key" => "value_1", "key_1" => "value_2" }
+      subject.set(expected_hash)
+      subject.close
+
+      expect(saved_json(path)).to eq(expected_hash)
+    end
+  end
+
+  def saved_json(filename)
+    ::JSON.parse(IO.read(filename))
+  end
+end

spec/lib/gitlab/import_export/relation_tree_saver_spec.rb → spec/lib/gitlab/import_export/legacy_relation_tree_saver_spec.rb

@@ -2,7 +2,7 @@
 
 require 'spec_helper'
 
-describe Gitlab::ImportExport::RelationTreeSaver do
+describe Gitlab::ImportExport::LegacyRelationTreeSaver do
   let(:exportable) { create(:group) }
   let(:relation_tree_saver) { described_class.new }
   let(:tree) { {} }

spec/lib/gitlab/import_export/project/tree_saver_spec.rb

@@ -25,57 +25,6 @@ describe Gitlab::ImportExport::Project::TreeSaver do
       expect(project_tree_saver.save).to be true
     end
 
-    context ':export_fast_serialize feature flag checks' do
-      before do
-        expect(Gitlab::ImportExport::Reader).to receive(:new).with(shared: shared).and_return(reader)
-        expect(reader).to receive(:project_tree).and_return(project_tree)
-      end
-
-      let(:serializer) { instance_double('Gitlab::ImportExport::FastHashSerializer') }
-      let(:reader) { instance_double('Gitlab::ImportExport::Reader') }
-      let(:project_tree) do
-        {
-          include: [{ issues: { include: [] } }],
-          preload: { issues: nil }
-        }
-      end
-
-      context 'when :export_fast_serialize feature is enabled' do
-        before do
-          stub_feature_flags(export_fast_serialize: true)
-        end
-
-        it 'uses FastHashSerializer' do
-          expect(Gitlab::ImportExport::FastHashSerializer)
-            .to receive(:new)
-            .with(project, project_tree)
-            .and_return(serializer)
-
-          expect(serializer).to receive(:execute)
-
-          project_tree_saver.save
-        end
-      end
-
-      context 'when :export_fast_serialize feature is disabled' do
-        before do
-          stub_feature_flags(export_fast_serialize: false)
-        end
-
-        it 'is serialized via built-in `as_json`' do
-          expect(project).to receive(:as_json).with(project_tree)
-
-          project_tree_saver.save
-        end
-      end
-    end
-
-    # It is mostly duplicated in
-    # `spec/lib/gitlab/import_export/fast_hash_serializer_spec.rb`
-    # except:
-    # context 'with description override' do
-    # context 'group members' do
-    # ^ These are specific for the Project::TreeSaver
     context 'JSON' do
       let(:saved_project_json) do
         project_tree_saver.save
@@ -392,6 +341,6 @@ describe Gitlab::ImportExport::Project::TreeSaver do
   end
 
   def project_json(filename)
-    JSON.parse(IO.read(filename))
+    ::JSON.parse(IO.read(filename))
   end
 end

spec/presenters/projects/import_export/project_export_presenter_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Projects::ImportExport::ProjectExportPresenter do
+  let_it_be(:group) { create(:group) }
+  let_it_be(:project) { create(:project, group: group) }
+  let_it_be(:project) { create(:project, group: group) }
+  let_it_be(:user) { create(:user) }
+  let(:description) { "overridden description" }
+
+  subject { described_class.new(project, current_user: user, override_description: description) }
+
+  describe '#as_json' do
+    context "override_description provided" do
+      it "overrides description" do
+        expect(subject.as_json["description"]).to eq(description)
+      end
+    end
+
+    context "override_description not provided" do
+      subject { described_class.new(project, current_user: user) }
+
+      it "keeps original description" do
+        expect(subject.as_json["description"]).to eq(project.description)
+      end
+    end
+  end
+
+  describe '#project_members' do
+    let(:user2) { create(:user, email: 'group@member.com') }
+    let(:member_emails) do
+      subject.project_members.map do |pm|
+        pm.user.email
+      end
+    end
+
+    before do
+      group.add_developer(user2)
+    end
+
+    it 'does not export group members if it has no permission' do
+      group.add_developer(user)
+
+      expect(member_emails).not_to include('group@member.com')
+    end
+
+    it 'does not export group members as maintainer' do
+      group.add_maintainer(user)
+
+      expect(member_emails).not_to include('group@member.com')
+    end
+
+    it 'exports group members as group owner' do
+      group.add_owner(user)
+
+      expect(member_emails).to include('group@member.com')
+    end
+
+    context 'as admin' do
+      let(:user) { create(:admin) }
+
+      it 'exports group members as admin' do
+        expect(member_emails).to include('group@member.com')
+      end
+
+      it 'exports group members as project members' do
+        member_types = subject.project_members.map { |pm| pm.source_type }
+
+        expect(member_types).to all(eq('Project'))
+      end
+    end
+  end
+end

spec/services/projects/import_export/export_service_spec.rb

@@ -26,10 +26,28 @@ describe Projects::ImportExport::ExportService do
       service.execute
     end
 
-    it 'saves the models' do
-      expect(Gitlab::ImportExport::Project::TreeSaver).to receive(:new).and_call_original
+    context 'when :streaming_serializer feature is enabled' do
+      before do
+        stub_feature_flags(streaming_serializer: true)
+      end
 
-      service.execute
+      it 'saves the models' do
+        expect(Gitlab::ImportExport::Project::TreeSaver).to receive(:new).and_call_original
+
+        service.execute
+      end
+    end
+
+    context 'when :streaming_serializer feature is disabled' do
+      before do
+        stub_feature_flags(streaming_serializer: false)
+      end
+
+      it 'saves the models' do
+        expect(Gitlab::ImportExport::Project::LegacyTreeSaver).to receive(:new).and_call_original
+
+        service.execute
+      end
     end
 
     it 'saves the uploads' do