From 0d5d89d467792518a5ac5290d41f0d359885c95c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kamil=20Trzci=C5=84ski?= Date: Fri, 20 Mar 2020 14:41:25 +0100 Subject: [PATCH 1/2] Refactor JSON reader/writer for import/export - adds `allowed_path` to strong validate `LegacyReader` and `LegacyWriter` - does not change any of todays behaviour for data serialization - makes the interface for reader and writer to accept `importable/exportable_path` that will be used by `ndjson` to store data in separate files - moves consuming of attributes outside of the relation tree restorer to ease the way how we process them - splits specs for `LegacyReader::User/File` and introduces shared example for that: this is also the majority of changes introduced by this MR in order to move them - slightly code-formats and re-organises group and project import/export --- .../import_export/group/tree_restorer.rb | 58 ++++--- .../import_export/json/legacy_reader.rb | 56 ++++--- .../import_export/json/legacy_writer.rb | 63 +++++--- .../json/streaming_serializer.rb | 21 +-- .../import_export/project/tree_restorer.rb | 29 +++- .../import_export/project/tree_saver.rb | 15 +- .../import_export/relation_tree_restorer.rb | 16 +- .../json/legacy_reader/file_spec.rb | 32 ++++ .../json/legacy_reader/hash_spec.rb | 35 ++++ .../json/legacy_reader/shared_example.rb | 110 +++++++++++++ .../import_export/json/legacy_reader_spec.rb | 149 ------------------ .../import_export/json/legacy_writer_spec.rb | 92 +++++++---- .../relation_tree_restorer_spec.rb | 35 ++-- 13 files changed, 428 insertions(+), 283 deletions(-) create mode 100644 spec/lib/gitlab/import_export/json/legacy_reader/file_spec.rb create mode 100644 spec/lib/gitlab/import_export/json/legacy_reader/hash_spec.rb create mode 100644 spec/lib/gitlab/import_export/json/legacy_reader/shared_example.rb delete mode 100644 spec/lib/gitlab/import_export/json/legacy_reader_spec.rb diff --git a/lib/gitlab/import_export/group/tree_restorer.rb b/lib/gitlab/import_export/group/tree_restorer.rb index 247e39a68b994c..f6ebd83bfaa7a8 100644 --- a/lib/gitlab/import_export/group/tree_restorer.rb +++ b/lib/gitlab/import_export/group/tree_restorer.rb @@ -4,12 +4,13 @@ module Gitlab module ImportExport module Group class TreeRestorer + include Gitlab::Utils::StrongMemoize + attr_reader :user attr_reader :shared attr_reader :group def initialize(user:, shared:, group:, group_hash:) - @path = File.join(shared.export_path, 'group.json') @user = user @shared = shared @group = group @@ -17,17 +18,14 @@ def initialize(user:, shared:, group:, group_hash:) end def restore - @relation_reader ||= - if @group_hash.present? - ImportExport::JSON::LegacyReader::User.new(@group_hash, reader.group_relation_names) - else - ImportExport::JSON::LegacyReader::File.new(@path, reader.group_relation_names) - end + @group_attributes = relation_reader.consume_attributes(nil) + @group_members = relation_reader.consume_relation(nil, 'members') + + # We need to remove `name` and `path` as we did consume it in previous pass + @group_attributes.delete('name') + @group_attributes.delete('path') - @group_members = @relation_reader.consume_relation('members') - @children = @relation_reader.consume_attribute('children') - @relation_reader.consume_attribute('name') - @relation_reader.consume_attribute('path') + @children = @group_attributes.delete('children') if members_mapper.map && restorer.restore @children&.each do |group_hash| @@ -53,16 +51,32 @@ def restore private + def relation_reader + strong_memoize(:relation_reader) do + if @group_hash.present? + ImportExport::JSON::LegacyReader::Hash.new( + @group_hash, + relation_names: reader.group_relation_names) + else + ImportExport::JSON::LegacyReader::File.new( + File.join(shared.export_path, 'group.json'), + relation_names: reader.group_relation_names) + end + end + end + def restorer @relation_tree_restorer ||= RelationTreeRestorer.new( - user: @user, - shared: @shared, - importable: @group, - relation_reader: @relation_reader, - members_mapper: members_mapper, - object_builder: object_builder, - relation_factory: relation_factory, - reader: reader + user: @user, + shared: @shared, + relation_reader: relation_reader, + members_mapper: members_mapper, + object_builder: object_builder, + relation_factory: relation_factory, + reader: reader, + importable: @group, + importable_attributes: @group_attributes, + importable_path: nil ) end @@ -88,7 +102,11 @@ def sub_group_visibility_level(group_hash, parent_group) end def members_mapper - @members_mapper ||= Gitlab::ImportExport::MembersMapper.new(exported_members: @group_members, user: @user, importable: @group) + @members_mapper ||= Gitlab::ImportExport::MembersMapper.new( + exported_members: @group_members, + user: @user, + importable: @group + ) end def relation_factory diff --git a/lib/gitlab/import_export/json/legacy_reader.rb b/lib/gitlab/import_export/json/legacy_reader.rb index 477e41ae3eb67a..57579fe9deff7b 100644 --- a/lib/gitlab/import_export/json/legacy_reader.rb +++ b/lib/gitlab/import_export/json/legacy_reader.rb @@ -5,19 +5,25 @@ module ImportExport module JSON class LegacyReader class File < LegacyReader - def initialize(path, relation_names) + include Gitlab::Utils::StrongMemoize + + def initialize(path, relation_names:, allowed_path: nil) @path = path - super(relation_names) + super( + relation_names: relation_names, + allowed_path: allowed_path) end - def valid? + def exist? ::File.exist?(@path) end - private + protected def tree_hash - @tree_hash ||= read_hash + strong_memoize(:tree_hash) do + read_hash + end end def read_hash @@ -28,13 +34,15 @@ def read_hash end end - class User < LegacyReader - def initialize(tree_hash, relation_names) + class Hash < LegacyReader + def initialize(tree_hash, relation_names:, allowed_path: nil) @tree_hash = tree_hash - super(relation_names) + super( + relation_names: relation_names, + allowed_path: allowed_path) end - def valid? + def exist? @tree_hash.present? end @@ -43,11 +51,16 @@ def valid? attr_reader :tree_hash end - def initialize(relation_names) + def initialize(relation_names:, allowed_path:) @relation_names = relation_names.map(&:to_s) + + # This is legacy reader, to be used in transition + # period before `.ndjson`, + # we strong validate what is being readed + @allowed_path = allowed_path end - def valid? + def exist? raise NotImplementedError end @@ -55,15 +68,22 @@ def legacy? true end - def root_attributes(excluded_attributes = []) - attributes.except(*excluded_attributes.map(&:to_s)) + def consume_attributes(importable_path) + unless importable_path == @allowed_path + raise ArgumentError, "Invalid #{importable_path} passed to `consume_attributes`. Use #{@allowed_path} instead." + end + + attributes end - def consume_relation(key) + def consume_relation(importable_path, key) + unless importable_path == @allowed_path + raise ArgumentError, "Invalid #{importable_name} passed to `consume_relation`. Use #{@allowed_path} instead." + end + value = relations.delete(key) return value unless block_given? - return if value.nil? if value.is_a?(Array) @@ -75,17 +95,13 @@ def consume_relation(key) end end - def consume_attribute(key) - attributes.delete(key) - end - def sort_ci_pipelines_by_id relations['ci_pipelines']&.sort_by! { |hash| hash['id'] } end private - attr_reader :relation_names + attr_reader :relation_names, :allowed_path def tree_hash raise NotImplementedError diff --git a/lib/gitlab/import_export/json/legacy_writer.rb b/lib/gitlab/import_export/json/legacy_writer.rb index c935e360a653b1..7be21410d2611b 100644 --- a/lib/gitlab/import_export/json/legacy_writer.rb +++ b/lib/gitlab/import_export/json/legacy_writer.rb @@ -8,11 +8,15 @@ class LegacyWriter attr_reader :path - def initialize(path) + def initialize(path, allowed_path:) @path = path - @last_array = nil @keys = Set.new + # This is legacy writer, to be used in transition + # period before `.ndjson`, + # we strong validate what is being written + @allowed_path = allowed_path + mkdir_p(File.dirname(@path)) file.write('{}') end @@ -22,12 +26,44 @@ def close @file = nil end - def set(hash) + def write_attributes(exportable_path, hash) + unless exportable_path == @allowed_path + raise ArgumentError, "Invalid #{exportable_path}" + end + hash.each do |key, value| write(key, value) end end + def write_relation(exportable_path, key, value) + unless exportable_path == @allowed_path + raise ArgumentError, "Invalid #{exportable_path}" + end + + write(key, value) + end + + def write_relation_array(exportable_path, key, items) + unless exportable_path == @allowed_path + raise ArgumentError, "Invalid #{exportable_path}" + end + + write(key, []) + + # rewind by two bytes, to overwrite ']}' + file.pos = file.size - 2 + + items.each_with_index do |item, idx| + file.write(',') if idx > 0 + file.write(item.to_json) + end + + file.write(']}') + end + + private + def write(key, value) raise ArgumentError, "key '#{key}' already written" if @keys.include?(key) @@ -41,29 +77,8 @@ def write(key, value) file.write('}') @keys.add(key) - @last_array = nil - @last_array_count = nil - end - - def append(key, value) - unless @last_array == key - write(key, []) - - @last_array = key - @last_array_count = 0 - end - - # rewind by two bytes, to overwrite ']}' - file.pos = file.size - 2 - - file.write(',') if @last_array_count > 0 - file.write(value.to_json) - file.write(']}') - @last_array_count += 1 end - private - def file @file ||= File.open(@path, "wb") end diff --git a/lib/gitlab/import_export/json/streaming_serializer.rb b/lib/gitlab/import_export/json/streaming_serializer.rb index d053bf1616608a..348c58df6a23e9 100644 --- a/lib/gitlab/import_export/json/streaming_serializer.rb +++ b/lib/gitlab/import_export/json/streaming_serializer.rb @@ -14,8 +14,9 @@ def to_json(*_args) end end - def initialize(exportable, relations_schema, json_writer) + def initialize(exportable, relations_schema, json_writer, exportable_path:) @exportable = exportable + @exportable_path = exportable_path @relations_schema = relations_schema @json_writer = json_writer end @@ -35,7 +36,7 @@ def execute def serialize_root attributes = exportable.as_json( relations_schema.merge(include: nil, preloads: nil)) - json_writer.set(attributes) + json_writer.write_attributes(@exportable_path, attributes) end def serialize_relation(definition) @@ -53,20 +54,22 @@ def serialize_relation(definition) end def serialize_many_relations(key, records, options) - key_preloads = preloads&.dig(key) - records = records.preload(key_preloads) if key_preloads + enumerator = Enumerator.new do |items| + key_preloads = preloads&.dig(key) + records = records.preload(key_preloads) if key_preloads - records.find_each(batch_size: BATCH_SIZE) do |record| - json = Raw.new(record.to_json(options)) - - json_writer.append(key, json) + records.find_each(batch_size: BATCH_SIZE) do |record| + items << Raw.new(record.to_json(options)) + end end + + json_writer.write_relation_array(@exportable_path, key, enumerator) end def serialize_single_relation(key, record, options) json = Raw.new(record.to_json(options)) - json_writer.write(key, json) + json_writer.write_relation(@exportable_path, key, json) end def includes diff --git a/lib/gitlab/import_export/project/tree_restorer.rb b/lib/gitlab/import_export/project/tree_restorer.rb index f8d25e14c02652..99e57d9decd242 100644 --- a/lib/gitlab/import_export/project/tree_restorer.rb +++ b/lib/gitlab/import_export/project/tree_restorer.rb @@ -4,6 +4,8 @@ module Gitlab module ImportExport module Project class TreeRestorer + include Gitlab::Utils::StrongMemoize + attr_reader :user attr_reader :shared attr_reader :project @@ -15,9 +17,8 @@ def initialize(user:, shared:, project:) end def restore - @relation_reader = ImportExport::JSON::LegacyReader::File.new(File.join(shared.export_path, 'project.json'), reader.project_relation_names) - - @project_members = @relation_reader.consume_relation('project_members') + @project_attributes = relation_reader.consume_attributes(importable_path) + @project_members = relation_reader.consume_relation(importable_path, 'project_members') if relation_tree_restorer.restore import_failure_service.with_retry(action: 'set_latest_merge_request_diff_ids!') do @@ -35,16 +36,28 @@ def restore private + def relation_reader + strong_memoize(:relation_reader) do + ImportExport::JSON::LegacyReader::File.new( + File.join(shared.export_path, 'project.json'), + relation_names: reader.project_relation_names, + allowed_path: importable_path + ) + end + end + def relation_tree_restorer @relation_tree_restorer ||= RelationTreeRestorer.new( user: @user, shared: @shared, - importable: @project, - relation_reader: @relation_reader, + relation_reader: relation_reader, object_builder: object_builder, members_mapper: members_mapper, relation_factory: relation_factory, - reader: reader + reader: reader, + importable: @project, + importable_attributes: @project_attributes, + importable_path: importable_path ) end @@ -69,6 +82,10 @@ def reader def import_failure_service @import_failure_service ||= ImportFailureService.new(@project) end + + def importable_path + "project" + end end end end diff --git a/lib/gitlab/import_export/project/tree_saver.rb b/lib/gitlab/import_export/project/tree_saver.rb index 01000c9d6d92ec..988776fe600b32 100644 --- a/lib/gitlab/import_export/project/tree_saver.rb +++ b/lib/gitlab/import_export/project/tree_saver.rb @@ -15,10 +15,17 @@ def initialize(project:, current_user:, shared:, params: {}) end def save - json_writer = ImportExport::JSON::LegacyWriter.new(@full_path) - - serializer = ImportExport::JSON::StreamingSerializer.new(exportable, reader.project_tree, json_writer) - serializer.execute + json_writer = ImportExport::JSON::LegacyWriter.new( + @full_path, + allowed_path: "project" + ) + + ImportExport::JSON::StreamingSerializer.new( + exportable, + reader.project_tree, + json_writer, + exportable_path: "project" + ).execute true rescue => e diff --git a/lib/gitlab/import_export/relation_tree_restorer.rb b/lib/gitlab/import_export/relation_tree_restorer.rb index 1157e18c7f9002..7f25222ba41b5a 100644 --- a/lib/gitlab/import_export/relation_tree_restorer.rb +++ b/lib/gitlab/import_export/relation_tree_restorer.rb @@ -11,7 +11,15 @@ class RelationTreeRestorer attr_reader :importable attr_reader :relation_reader - def initialize(user:, shared:, importable:, relation_reader:, members_mapper:, object_builder:, relation_factory:, reader:) + def initialize( # rubocop:disable Metrics/ParameterLists + user:, shared:, relation_reader:, + members_mapper:, object_builder:, + relation_factory:, + reader:, + importable:, + importable_attributes:, + importable_path: + ) @user = user @shared = shared @importable = importable @@ -20,6 +28,8 @@ def initialize(user:, shared:, importable:, relation_reader:, members_mapper:, o @object_builder = object_builder @relation_factory = relation_factory @reader = reader + @importable_attributes = importable_attributes + @importable_path = importable_path end def restore @@ -57,7 +67,7 @@ def create_relations! end def process_relation!(relation_key, relation_definition) - @relation_reader.consume_relation(relation_key) do |data_hash, relation_index| + @relation_reader.consume_relation(@importable_path, relation_key) do |data_hash, relation_index| process_relation_item!(relation_key, relation_definition, relation_index, data_hash) end end @@ -94,7 +104,7 @@ def relations end def update_params! - params = @relation_reader.root_attributes(relations.keys) + params = @importable_attributes.except(*relations.keys.map(&:to_s)) params = params.merge(present_override_params) # Cleaning all imported and overridden params diff --git a/spec/lib/gitlab/import_export/json/legacy_reader/file_spec.rb b/spec/lib/gitlab/import_export/json/legacy_reader/file_spec.rb new file mode 100644 index 00000000000000..1021ce3cd50c64 --- /dev/null +++ b/spec/lib/gitlab/import_export/json/legacy_reader/file_spec.rb @@ -0,0 +1,32 @@ +# frozen_string_literal: true + +require 'spec_helper' +require_relative 'shared_example.rb' + +describe Gitlab::ImportExport::JSON::LegacyReader::File do + it_behaves_like 'import/export json legacy reader' do + let(:valid_path) { 'spec/fixtures/lib/gitlab/import_export/light/project.json' } + let(:data) { valid_path } + let(:json_data) { JSON.parse(File.read(valid_path)) } + end + + describe '#exist?' do + let(:legacy_reader) do + described_class.new(path, relation_names: []) + end + + subject { legacy_reader.exist? } + + context 'given valid path' do + let(:path) { 'spec/fixtures/lib/gitlab/import_export/light/project.json' } + + it { is_expected.to be true } + end + + context 'given invalid path' do + let(:path) { 'spec/non-existing-folder/do-not-create-this-file.json' } + + it { is_expected.to be false } + end + end +end diff --git a/spec/lib/gitlab/import_export/json/legacy_reader/hash_spec.rb b/spec/lib/gitlab/import_export/json/legacy_reader/hash_spec.rb new file mode 100644 index 00000000000000..8c4dfd2f356be4 --- /dev/null +++ b/spec/lib/gitlab/import_export/json/legacy_reader/hash_spec.rb @@ -0,0 +1,35 @@ +# frozen_string_literal: true + +require 'spec_helper' +require_relative 'shared_example.rb' + +describe Gitlab::ImportExport::JSON::LegacyReader::Hash do + it_behaves_like 'import/export json legacy reader' do + let(:path) { 'spec/fixtures/lib/gitlab/import_export/light/project.json' } + + # the hash is modified by the `LegacyReader` + # we need to deep-dup it + let(:json_data) { JSON.parse(File.read(path)) } + let(:data) { JSON.parse(File.read(path)) } + end + + describe '#exist?' do + let(:legacy_reader) do + described_class.new(tree_hash, relation_names: []) + end + + subject { legacy_reader.exist? } + + context 'tree_hash is nil' do + let(:tree_hash) { nil } + + it { is_expected.to be_falsey } + end + + context 'tree_hash presents' do + let(:tree_hash) { { "issues": [] } } + + it { is_expected.to be_truthy } + end + end +end diff --git a/spec/lib/gitlab/import_export/json/legacy_reader/shared_example.rb b/spec/lib/gitlab/import_export/json/legacy_reader/shared_example.rb new file mode 100644 index 00000000000000..297a59467035da --- /dev/null +++ b/spec/lib/gitlab/import_export/json/legacy_reader/shared_example.rb @@ -0,0 +1,110 @@ +# frozen_string_literal: true + +RSpec.shared_examples 'import/export json legacy reader' do + let(:relation_names) { [] } + + let(:legacy_reader) do + described_class.new( + data, + relation_names: relation_names, + allowed_path: "project") + end + + describe '#consume_attributes' do + context 'when valid path is passed' do + subject { legacy_reader.consume_attributes("project") } + + context 'no excluded attributes' do + let(:excluded_attributes) { [] } + let(:relation_names) { [] } + + it 'returns the whole tree from parsed JSON' do + expect(subject).to eq(json_data) + end + end + + context 'some attributes are excluded' do + let(:relation_names) { %w[milestones labels] } + + it 'returns hash without excluded attributes and relations' do + expect(subject).not_to include('milestones', 'labels') + end + end + end + + context 'when invalid path is passed' do + it 'raises an exception' do + expect { legacy_reader.consume_attributes("invalid-path") } + .to raise_error(ArgumentError) + end + end + end + + describe '#consume_relation' do + context 'when valid path is passed' do + let(:key) { 'description' } + + context 'block not given' do + it 'returns value of the key' do + expect(legacy_reader).to receive(:relations).and_return({ key => 'test value' }) + expect(legacy_reader.consume_relation("project", key)).to eq('test value') + end + end + + context 'key has been consumed' do + before do + legacy_reader.consume_relation("project", key) + end + + it 'does not yield' do + expect do |blk| + legacy_reader.consume_relation("project", key, &blk) + end.not_to yield_control + end + end + + context 'value is nil' do + before do + expect(legacy_reader).to receive(:relations).and_return({ key => nil }) + end + + it 'does not yield' do + expect do |blk| + legacy_reader.consume_relation("project", key, &blk) + end.not_to yield_control + end + end + + context 'value is not array' do + before do + expect(legacy_reader).to receive(:relations).and_return({ key => 'value' }) + end + + it 'yield the value with index 0' do + expect do |blk| + legacy_reader.consume_relation("project", key, &blk) + end.to yield_with_args('value', 0) + end + end + + context 'value is an array' do + before do + expect(legacy_reader).to receive(:relations).and_return({ key => %w[item1 item2 item3] }) + end + + it 'yield each array element with index' do + expect do |blk| + legacy_reader.consume_relation("project", key, &blk) + end.to yield_successive_args(['item1', 0], ['item2', 1], ['item3', 2]) + end + end + end + + context 'when invalid path is passed' do + it 'raises an exception' do + expect { legacy_reader.consume_relation("invalid") } + .to raise_error(ArgumentError) + end + end + end +end diff --git a/spec/lib/gitlab/import_export/json/legacy_reader_spec.rb b/spec/lib/gitlab/import_export/json/legacy_reader_spec.rb deleted file mode 100644 index 0009a5f81de4f4..00000000000000 --- a/spec/lib/gitlab/import_export/json/legacy_reader_spec.rb +++ /dev/null @@ -1,149 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -describe Gitlab::ImportExport::JSON::LegacyReader::User do - let(:relation_names) { [] } - let(:legacy_reader) { described_class.new(tree_hash, relation_names) } - - describe '#valid?' do - subject { legacy_reader.valid? } - - context 'tree_hash not present' do - let(:tree_hash) { nil } - - it { is_expected.to be false } - end - - context 'tree_hash presents' do - let(:tree_hash) { { "issues": [] } } - - it { is_expected.to be true } - end - end -end - -describe Gitlab::ImportExport::JSON::LegacyReader::File do - let(:fixture) { 'spec/fixtures/lib/gitlab/import_export/light/project.json' } - let(:project_tree) { JSON.parse(File.read(fixture)) } - let(:relation_names) { [] } - let(:legacy_reader) { described_class.new(path, relation_names) } - - describe '#valid?' do - subject { legacy_reader.valid? } - - context 'given valid path' do - let(:path) { fixture } - - it { is_expected.to be true } - end - - context 'given invalid path' do - let(:path) { 'spec/non-existing-folder/do-not-create-this-file.json' } - - it { is_expected.to be false } - end - end - - describe '#root_attributes' do - let(:path) { fixture } - - subject { legacy_reader.root_attributes(excluded_attributes) } - - context 'No excluded attributes' do - let(:excluded_attributes) { [] } - let(:relation_names) { [] } - - it 'returns the whole tree from parsed JSON' do - expect(subject).to eq(project_tree) - end - end - - context 'Some attributes are excluded' do - let(:excluded_attributes) { %w[milestones labels issues services snippets] } - let(:relation_names) { %w[import_type archived] } - - it 'returns hash without excluded attributes and relations' do - expect(subject).not_to include('milestones', 'labels', 'issues', 'services', 'snippets', 'import_type', 'archived') - end - end - end - - describe '#consume_relation' do - let(:path) { fixture } - let(:key) { 'description' } - - context 'block not given' do - it 'returns value of the key' do - expect(legacy_reader).to receive(:relations).and_return({ key => 'test value' }) - expect(legacy_reader.consume_relation(key)).to eq('test value') - end - end - - context 'key has been consumed' do - before do - legacy_reader.consume_relation(key) - end - - it 'does not yield' do - expect do |blk| - legacy_reader.consume_relation(key, &blk) - end.not_to yield_control - end - end - - context 'value is nil' do - before do - expect(legacy_reader).to receive(:relations).and_return({ key => nil }) - end - - it 'does not yield' do - expect do |blk| - legacy_reader.consume_relation(key, &blk) - end.not_to yield_control - end - end - - context 'value is not array' do - before do - expect(legacy_reader).to receive(:relations).and_return({ key => 'value' }) - end - - it 'yield the value with index 0' do - expect do |blk| - legacy_reader.consume_relation(key, &blk) - end.to yield_with_args('value', 0) - end - end - - context 'value is an array' do - before do - expect(legacy_reader).to receive(:relations).and_return({ key => %w[item1 item2 item3] }) - end - - it 'yield each array element with index' do - expect do |blk| - legacy_reader.consume_relation(key, &blk) - end.to yield_successive_args(['item1', 0], ['item2', 1], ['item3', 2]) - end - end - end - - describe '#tree_hash' do - let(:path) { fixture } - - subject { legacy_reader.send(:tree_hash) } - - it 'parses the JSON into the expected tree' do - expect(subject).to eq(project_tree) - end - - context 'invalid JSON' do - let(:path) { 'spec/fixtures/lib/gitlab/import_export/invalid_json/project.json' } - - it 'raise Exception' do - expect { subject }.to raise_exception(Gitlab::ImportExport::Error, 'Incorrect JSON format') - end - end - end -end diff --git a/spec/lib/gitlab/import_export/json/legacy_writer_spec.rb b/spec/lib/gitlab/import_export/json/legacy_writer_spec.rb index b4cdfee3b221bb..1f39b26e46a0e1 100644 --- a/spec/lib/gitlab/import_export/json/legacy_writer_spec.rb +++ b/spec/lib/gitlab/import_export/json/legacy_writer_spec.rb @@ -5,20 +5,37 @@ describe Gitlab::ImportExport::JSON::LegacyWriter do let(:path) { "#{Dir.tmpdir}/legacy_writer_spec/test.json" } - subject { described_class.new(path) } + subject do + described_class.new(path, allowed_path: "project") + end after do FileUtils.rm_rf(path) end - describe "#write" do + describe "#write_attributes" do + it "writes correct json" do + expected_hash = { "key" => "value_1", "key_1" => "value_2" } + subject.write_attributes("project", expected_hash) + + expect(subject_json).to eq(expected_hash) + end + + context 'when invalid path is used' do + it 'raises an exception' do + expect { subject.write_attributes("invalid", { "key" => "value" }) } + .to raise_error(ArgumentError) + end + end + end + + describe "#write_relation" do context "when key is already written" do it "raises exception" do - key = "key" - value = "value" - subject.write(key, value) + subject.write_relation("project", "key", "old value") - expect { subject.write(key, "new value") }.to raise_exception("key '#{key}' already written") + expect { subject.write_relation("project", "key", "new value") } + .to raise_exception("key 'key' already written") end end @@ -27,53 +44,58 @@ it "writes correct json" do expected_hash = { "key" => "value_1", "key_1" => "value_2" } expected_hash.each do |key, value| - subject.write(key, value) + subject.write_relation("project", key, value) end - subject.close - expect(saved_json(path)).to eq(expected_hash) + expect(subject_json).to eq(expected_hash) end end end + + context 'when invalid path is used' do + it 'raises an exception' do + expect { subject.write_relation("invalid", "key", "value") } + .to raise_error(ArgumentError) + 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 + describe "#write_relation_array" do + context 'when array is used' do + it 'writes correct json' do + subject.write_relation_array("project", "key", ["value"]) - expect(saved_json(path)).to eq(expected_hash) + expect(subject_json).to eq({ "key" => ["value"] }) 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 + context 'when enumerable is used' do + it 'writes correct json' do + values = %w(value1 value2) + + enumerator = Enumerator.new do |items| + values.each { |value| items << value } + end + + subject.write_relation_array("project", "key", enumerator) - expect(saved_json(path)).to eq(expected_hash) + expect(subject_json).to eq({ "key" => values }) 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 + context "when key is already written" do + it "raises an exception" do + subject.write_relation_array("project", "key", %w(old_value)) - expect(saved_json(path)).to eq(expected_hash) + expect { subject.write_relation_array("project", "key", %w(new_value)) } + .to raise_error(ArgumentError) + end end end - def saved_json(filename) - ::JSON.parse(IO.read(filename)) + def subject_json + subject.close + + ::JSON.parse(IO.read(subject.path)) end end diff --git a/spec/lib/gitlab/import_export/relation_tree_restorer_spec.rb b/spec/lib/gitlab/import_export/relation_tree_restorer_spec.rb index e36144b1a30909..1f9e2a4babba4d 100644 --- a/spec/lib/gitlab/import_export/relation_tree_restorer_spec.rb +++ b/spec/lib/gitlab/import_export/relation_tree_restorer_spec.rb @@ -14,23 +14,24 @@ let(:user) { create(:user) } let(:shared) { Gitlab::ImportExport::Shared.new(importable) } - let(:members_mapper) { Gitlab::ImportExport::MembersMapper.new(exported_members: {}, user: user, importable: importable) } + let(:attributes) { {} } - let(:importable_hash) do - json = IO.read(path) - ActiveSupport::JSON.decode(json) + let(:members_mapper) do + Gitlab::ImportExport::MembersMapper.new(exported_members: {}, user: user, importable: importable) end let(:relation_tree_restorer) do described_class.new( - user: user, - shared: shared, - relation_reader: relation_reader, - importable: importable, - object_builder: object_builder, - members_mapper: members_mapper, - relation_factory: relation_factory, - reader: reader + user: user, + shared: shared, + relation_reader: relation_reader, + object_builder: object_builder, + members_mapper: members_mapper, + relation_factory: relation_factory, + reader: reader, + importable: importable, + importable_path: 'projects/10', + importable_attributes: attributes ) end @@ -100,7 +101,15 @@ let(:reader) { Gitlab::ImportExport::Reader.new(shared: shared) } context 'using legacy reader' do - let(:relation_reader) { Gitlab::ImportExport::JSON::LegacyReader::File.new(path, reader.project_relation_names) } + let(:relation_reader) do + Gitlab::ImportExport::JSON::LegacyReader::File.new( + path, + relation_names: reader.project_relation_names, + allowed_path: "project" + ) + end + + let(:attributes) { relation_reader.consume_attributes("project") } it_behaves_like 'import project successfully' -- GitLab From 9957080267ddae077db3d65d59b8514e1d0c07a8 Mon Sep 17 00:00:00 2001 From: Qingyu Zhao Date: Wed, 25 Mar 2020 17:41:37 +1100 Subject: [PATCH 2/2] Fix spec failure --- .../gitlab/import_export/relation_tree_restorer_spec.rb | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/spec/lib/gitlab/import_export/relation_tree_restorer_spec.rb b/spec/lib/gitlab/import_export/relation_tree_restorer_spec.rb index 1f9e2a4babba4d..52e1efa70e0949 100644 --- a/spec/lib/gitlab/import_export/relation_tree_restorer_spec.rb +++ b/spec/lib/gitlab/import_export/relation_tree_restorer_spec.rb @@ -30,7 +30,7 @@ relation_factory: relation_factory, reader: reader, importable: importable, - importable_path: 'projects/10', + importable_path: nil, importable_attributes: attributes ) end @@ -104,12 +104,11 @@ let(:relation_reader) do Gitlab::ImportExport::JSON::LegacyReader::File.new( path, - relation_names: reader.project_relation_names, - allowed_path: "project" + relation_names: reader.project_relation_names ) end - let(:attributes) { relation_reader.consume_attributes("project") } + let(:attributes) { relation_reader.consume_attributes(nil) } it_behaves_like 'import project successfully' @@ -128,7 +127,7 @@ let(:importable) { create(:group, parent: group) } let(:object_builder) { Gitlab::ImportExport::Group::ObjectBuilder } let(:relation_factory) { Gitlab::ImportExport::Group::RelationFactory } - let(:relation_reader) { Gitlab::ImportExport::JSON::LegacyReader::File.new(path, reader.group_relation_names) } + let(:relation_reader) { Gitlab::ImportExport::JSON::LegacyReader::File.new(path, relation_names: reader.group_relation_names) } let(:reader) do Gitlab::ImportExport::Reader.new( shared: shared, -- GitLab