From eb6ab7b0b3d852fe07fb14d97c238cd0382ce29a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Francisco=20Javier=20L=C3=B3pez?= Date: Wed, 28 Nov 2018 16:47:50 +0100 Subject: Code review comments applied --- lib/gitlab/import_export/project_tree_restorer.rb | 2 +- lib/gitlab/import_export/project_tree_saver.rb | 2 +- .../import_export/relation_rename_factory.rb | 29 ------ .../import_export/relation_rename_service.rb | 48 ++++++++++ .../import_export/relation_rename_factory_spec.rb | 99 ------------------- .../import_export/relation_rename_service_spec.rb | 105 +++++++++++++++++++++ 6 files changed, 155 insertions(+), 130 deletions(-) delete mode 100644 lib/gitlab/import_export/relation_rename_factory.rb create mode 100644 lib/gitlab/import_export/relation_rename_service.rb delete mode 100644 spec/lib/gitlab/import_export/relation_rename_factory_spec.rb create mode 100644 spec/lib/gitlab/import_export/relation_rename_service_spec.rb diff --git a/lib/gitlab/import_export/project_tree_restorer.rb b/lib/gitlab/import_export/project_tree_restorer.rb index 56ab474f729..a56ec65b9f1 100644 --- a/lib/gitlab/import_export/project_tree_restorer.rb +++ b/lib/gitlab/import_export/project_tree_restorer.rb @@ -26,7 +26,7 @@ module Gitlab @project_members = @tree_hash.delete('project_members') - RelationRenameFactory.rename(@tree_hash) + RelationRenameService.rename(@tree_hash) ActiveRecord::Base.uncached do ActiveRecord::Base.no_touching do diff --git a/lib/gitlab/import_export/project_tree_saver.rb b/lib/gitlab/import_export/project_tree_saver.rb index d951590a2e2..a7a8bb1197e 100644 --- a/lib/gitlab/import_export/project_tree_saver.rb +++ b/lib/gitlab/import_export/project_tree_saver.rb @@ -34,7 +34,7 @@ module Gitlab project_json['project_members'] += group_members_json - RelationRenameFactory.add_renames(project_json) + RelationRenameService.add_renames(project_json) project_json.to_json end diff --git a/lib/gitlab/import_export/relation_rename_factory.rb b/lib/gitlab/import_export/relation_rename_factory.rb deleted file mode 100644 index e9bb9cac729..00000000000 --- a/lib/gitlab/import_export/relation_rename_factory.rb +++ /dev/null @@ -1,29 +0,0 @@ -# frozen_string_literal: true - -module Gitlab - module ImportExport - class RelationRenameFactory - RENAMES = { 'pipelines' => 'ci_pipelines' }.freeze - - def self.rename(tree_hash) - return if tree_hash&.empty? - - RENAMES.each do |old_name, new_name| - old_entry = tree_hash.delete(old_name) - - next if tree_hash[new_name] || !old_entry - - tree_hash[new_name] = old_entry - end - end - - def self.add_renames(tree_hash) - RENAMES.each do |old_name, new_name| - next if tree_hash.key?(old_name) - - tree_hash[old_name] = tree_hash[new_name] - end - end - end - end -end diff --git a/lib/gitlab/import_export/relation_rename_service.rb b/lib/gitlab/import_export/relation_rename_service.rb new file mode 100644 index 00000000000..bdd0afc773d --- /dev/null +++ b/lib/gitlab/import_export/relation_rename_service.rb @@ -0,0 +1,48 @@ +# frozen_string_literal: true + +# This class is intended to help with relation renames within Gitlab versions +# and allow compatibility between versions. +# If you have to change one relationship name that is imported/exported, +# you should add it to the RENAMES constant indicating the old name and the +# new one. +# The behavior of these renamed relationships should be transient and it should +# only last one release until you completely remove the renaming from the list. +# +# When importing, this class will check the project hash and: +# - if only the old relationship name is found, it will rename it with the new one +# - if only the new relationship name is found, it will do nothing +# - if it finds both, it will use the new relationship data +# +# When exporting, this class will duplicate the keys in the resulting file. +# This way, if we open the file in an old version of the exporter it will work +# and also it will with the newer versions. +module Gitlab + module ImportExport + class RelationRenameService + RENAMES = { + 'pipelines' => 'ci_pipelines' # Added in 11.6, remove in 11.7 + }.freeze + + def self.rename(tree_hash) + return if tree_hash&.empty? + + RENAMES.each do |old_name, new_name| + old_entry = tree_hash.delete(old_name) + + next if tree_hash[new_name] + next unless old_entry + + tree_hash[new_name] = old_entry + end + end + + def self.add_renames(tree_hash) + RENAMES.each do |old_name, new_name| + next if tree_hash.key?(old_name) + + tree_hash[old_name] = tree_hash[new_name] + end + end + end + end +end diff --git a/spec/lib/gitlab/import_export/relation_rename_factory_spec.rb b/spec/lib/gitlab/import_export/relation_rename_factory_spec.rb deleted file mode 100644 index 3e7159042c8..00000000000 --- a/spec/lib/gitlab/import_export/relation_rename_factory_spec.rb +++ /dev/null @@ -1,99 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -describe Gitlab::ImportExport::RelationRenameFactory do - let(:renames) do - { - 'example_relation1' => 'new_example_relation1', - 'example_relation2' => 'new_example_relation2' - } - end - - let(:user) { create(:admin) } - let(:group) { create(:group, :nested) } - let!(:project) { create(:project, :builds_disabled, :issues_disabled, name: 'project', path: 'project') } - let(:shared) { project.import_export_shared } - - before do - stub_const("#{described_class}::RENAMES", renames) - end - - context 'when importing' do - let(:project_tree_restorer) { Gitlab::ImportExport::ProjectTreeRestorer.new(user: user, shared: shared, project: project) } - let(:import_path) { 'spec/lib/gitlab/import_export' } - let(:file_content) { IO.read("#{import_path}/project.json") } - let!(:json_file) { ActiveSupport::JSON.decode(file_content) } - let(:tree_hash) { project_tree_restorer.instance_variable_get(:@tree_hash) } - - before do - allow(shared).to receive(:export_path).and_return(import_path) - allow(ActiveSupport::JSON).to receive(:decode).with(file_content).and_return(json_file) - end - - context 'when the file has only old relationship names' do - before do - renames.each do |k, v| - json_file[k.to_s] = [] - end - end - - it 'renames old relationships to the new name' do - expect(json_file.keys).to include(*renames.keys) - - project_tree_restorer.restore - - expect(json_file.keys).to include(*renames.values) - expect(json_file.keys).not_to include(*renames.keys) - end - end - - context 'when the file has both the old and new relationships' do - before do - renames.each do |k, v| - json_file[k.to_s] = [1] - json_file[v.to_s] = [2] - end - end - - it 'uses the new relationships and removes the old ones from the hash' do - expect(json_file.keys).to include(*renames.keys) - - project_tree_restorer.restore - - expect(json_file.keys).to include(*renames.values) - expect(json_file.values_at(*renames.values).flatten.uniq.first).to eq 2 - expect(json_file.keys).not_to include(*renames.keys) - end - end - - context 'when the file has only new relationship names' do - before do - renames.each do |k, v| - json_file[v.to_s] = [] - end - end - - it 'uses the new relationships' do - expect(json_file.keys).not_to include(*renames.keys) - - project_tree_restorer.restore - - expect(json_file.keys).to include(*renames.values) - end - end - end - - context 'when exporting' do - let(:project_tree_saver) { Gitlab::ImportExport::ProjectTreeSaver.new(project: project, current_user: user, shared: shared) } - let(:project_tree) { project_tree_saver.send(:project_json) } - - it 'adds old relationships to the exported file' do - project_tree.merge!(renames.values.map { |v| [v, []] }.to_h) - - saved_data = ActiveSupport::JSON.decode(project_tree_saver.send(:project_json_tree)) - - expect(saved_data.keys).to include(*(renames.keys + renames.values)) - end - end -end diff --git a/spec/lib/gitlab/import_export/relation_rename_service_spec.rb b/spec/lib/gitlab/import_export/relation_rename_service_spec.rb new file mode 100644 index 00000000000..4606c824799 --- /dev/null +++ b/spec/lib/gitlab/import_export/relation_rename_service_spec.rb @@ -0,0 +1,105 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Gitlab::ImportExport::RelationRenameService do + let(:renames) do + { + 'example_relation1' => 'new_example_relation1', + 'example_relation2' => 'new_example_relation2' + } + end + + let(:user) { create(:admin) } + let(:group) { create(:group, :nested) } + let!(:project) { create(:project, :builds_disabled, :issues_disabled, name: 'project', path: 'project') } + let(:shared) { project.import_export_shared } + + before do + stub_const("#{described_class}::RENAMES", renames) + end + + context 'when importing' do + let(:project_tree_restorer) { Gitlab::ImportExport::ProjectTreeRestorer.new(user: user, shared: shared, project: project) } + let(:import_path) { 'spec/lib/gitlab/import_export' } + let(:file_content) { IO.read("#{import_path}/project.json") } + let!(:json_file) { ActiveSupport::JSON.decode(file_content) } + let(:tree_hash) { project_tree_restorer.instance_variable_get(:@tree_hash) } + + before do + allow(shared).to receive(:export_path).and_return(import_path) + allow(ActiveSupport::JSON).to receive(:decode).with(file_content).and_return(json_file) + end + + context 'when the file has only old relationship names' do + before do + renames.each do |old_name, _| + json_file[old_name.to_s] = [] + end + end + + it 'renames old relationships to the new name' do + expect(json_file.keys).to include(*renames.keys) + + project_tree_restorer.restore + + expect(json_file.keys).to include(*renames.values) + expect(json_file.keys).not_to include(*renames.keys) + end + end + + context 'when the file has both the old and new relationships' do + before do + renames.each do |old_name, new_name| + json_file[old_name.to_s] = [1] + json_file[new_name.to_s] = [2] + end + end + + it 'uses the new relationships and removes the old ones from the hash' do + expect(json_file.keys).to include(*renames.keys) + + project_tree_restorer.restore + + expect(json_file.keys).to include(*renames.values) + expect(json_file.values_at(*renames.values).flatten.uniq.first).to eq 2 + expect(json_file.keys).not_to include(*renames.keys) + end + end + + context 'when the file has only new relationship names' do + before do + renames.each do |_, new_name| + json_file[new_name.to_s] = [] + end + end + + it 'uses the new relationships' do + expect(json_file.keys).not_to include(*renames.keys) + + project_tree_restorer.restore + + expect(json_file.keys).to include(*renames.values) + end + end + end + + context 'when exporting' do + let(:project_tree_saver) { Gitlab::ImportExport::ProjectTreeSaver.new(project: project, current_user: user, shared: shared) } + let(:project_tree) { project_tree_saver.send(:project_json) } + + it 'adds old relationships to the exported file' do + project_tree.merge!(renames.values.map { |new_name| [new_name, []] }.to_h) + + allow(project_tree_saver).to receive(:save) do |arg| + project_tree_saver.send(:project_json_tree) + end + + result = project_tree_saver.save + + saved_data = ActiveSupport::JSON.decode(result) + + expect(saved_data.keys).to include(*(renames.keys + renames.values)) + end + end +end -- cgit v1.2.1