From 8e08484fdb6a44658da93c08615b64f34fac2fc8 Mon Sep 17 00:00:00 2001 From: Rodrigo Tomonari Date: Thu, 30 Jun 2022 17:50:13 +0100 Subject: [PATCH 1/3] Fix group name conflict when migrating groups via BulkImport Modify BulkImport to make group names unique as a namespace can not have subgroups with the same name. Changelog: fixed --- .../group_attributes_transformer.rb | 18 +++++ .../group_attributes_transformer_spec.rb | 65 ++++++++++++++++--- 2 files changed, 75 insertions(+), 8 deletions(-) diff --git a/lib/bulk_imports/groups/transformers/group_attributes_transformer.rb b/lib/bulk_imports/groups/transformers/group_attributes_transformer.rb index df27275b664b11..20a5b12b99c1f6 100644 --- a/lib/bulk_imports/groups/transformers/group_attributes_transformer.rb +++ b/lib/bulk_imports/groups/transformers/group_attributes_transformer.rb @@ -8,6 +8,7 @@ def transform(context, data) import_entity = context.entity data + .then { |data| transform_name(import_entity, data) } .then { |data| transform_path(import_entity, data) } .then { |data| transform_full_path(data) } .then { |data| transform_parent(context, import_entity, data) } @@ -18,6 +19,23 @@ def transform(context, data) private + def transform_name(import_entity, data) + if import_entity.destination_namespace.present? + namespace = Namespace.find_by_full_path(import_entity.destination_namespace) + namespace_children_names = namespace.children.pluck(:name) # rubocop: disable CodeReuse/ActiveRecord + new_name = source_name = data['name'] + counter = 1 + while namespace_children_names.include?(new_name) + new_name = "#{source_name}(#{counter})" + counter += 1 + end + + data['name'] = new_name + end + + data + end + def transform_path(import_entity, data) data['path'] = import_entity.destination_name.parameterize data diff --git a/spec/lib/bulk_imports/groups/transformers/group_attributes_transformer_spec.rb b/spec/lib/bulk_imports/groups/transformers/group_attributes_transformer_spec.rb index c42ca9bef3be6d..d775cf6b026790 100644 --- a/spec/lib/bulk_imports/groups/transformers/group_attributes_transformer_spec.rb +++ b/spec/lib/bulk_imports/groups/transformers/group_attributes_transformer_spec.rb @@ -4,12 +4,12 @@ RSpec.describe BulkImports::Groups::Transformers::GroupAttributesTransformer do describe '#transform' do - let_it_be(:user) { create(:user) } let_it_be(:parent) { create(:group) } - let_it_be(:bulk_import) { create(:bulk_import, user: user) } - let_it_be(:entity) do - create( + let(:bulk_import) { build_stubbed(:bulk_import) } + + let(:entity) do + build_stubbed( :bulk_import_entity, bulk_import: bulk_import, source_full_path: 'source/full/path', @@ -18,8 +18,8 @@ ) end - let_it_be(:tracker) { create(:bulk_import_tracker, entity: entity) } - let_it_be(:context) { BulkImports::Pipeline::Context.new(tracker) } + let(:tracker) { build_stubbed(:bulk_import_tracker, entity: entity) } + let(:context) { BulkImports::Pipeline::Context.new(tracker) } let(:data) do { @@ -87,14 +87,63 @@ end context 'when destination namespace is empty' do - it 'does not set parent id' do - entity.update!(destination_namespace: '') + before do + entity.destination_namespace = '' + end + it 'does not set parent id' do transformed_data = subject.transform(context, data) expect(transformed_data).not_to have_key('parent_id') end end end + + describe 'group name transformation' do + context 'when destination namespace is empty' do + before do + entity.destination_namespace = '' + end + + it 'does not transform name' do + transformed_data = subject.transform(context, data) + + expect(transformed_data['name']).to eq('Source Group Name') + end + end + + context 'when destination namespace is present' do + context 'when destination namespace does not have a group with same name' do + it 'does not transform name' do + transformed_data = subject.transform(context, data) + + expect(transformed_data['name']).to eq('Source Group Name') + end + end + + context 'when destination namespace already have a group with the same name' do + before do + create(:group, parent: parent, name: 'Source Group Name', path: 'group_1') + create(:group, parent: parent, name: 'Source Group Name(1)', path: 'group_2') + create(:group, parent: parent, name: 'Source Group Name(2)', path: 'group_3') + create(:group, parent: parent, name: 'Source Group Name(1)(1)', path: 'group_4') + end + + it 'makes the name unique by appeding a counter', :aggregate_failures do + transformed_data = subject.transform(context, data.merge('name' => 'Source Group Name')) + expect(transformed_data['name']).to eq('Source Group Name(3)') + + transformed_data = subject.transform(context, data.merge('name' => 'Source Group Name(2)')) + expect(transformed_data['name']).to eq('Source Group Name(2)(1)') + + transformed_data = subject.transform(context, data.merge('name' => 'Source Group Name(1)')) + expect(transformed_data['name']).to eq('Source Group Name(1)(2)') + + transformed_data = subject.transform(context, data.merge('name' => 'Source Group Name(1)(1)')) + expect(transformed_data['name']).to eq('Source Group Name(1)(1)(1)') + end + end + end + end end end -- GitLab From 1fc4a7612adf6e1279949c586d2b59019d2640da Mon Sep 17 00:00:00 2001 From: Rodrigo Tomonari Date: Mon, 4 Jul 2022 12:43:41 +0100 Subject: [PATCH 2/3] Fetch namespace only once --- .../group_attributes_transformer.rb | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/lib/bulk_imports/groups/transformers/group_attributes_transformer.rb b/lib/bulk_imports/groups/transformers/group_attributes_transformer.rb index 20a5b12b99c1f6..6bcd02effa65fd 100644 --- a/lib/bulk_imports/groups/transformers/group_attributes_transformer.rb +++ b/lib/bulk_imports/groups/transformers/group_attributes_transformer.rb @@ -7,11 +7,15 @@ class GroupAttributesTransformer def transform(context, data) import_entity = context.entity + if import_entity.destination_namespace.present? + namespace = Namespace.find_by_full_path(import_entity.destination_namespace) + end + data - .then { |data| transform_name(import_entity, data) } + .then { |data| transform_name(import_entity, namespace, data) } .then { |data| transform_path(import_entity, data) } .then { |data| transform_full_path(data) } - .then { |data| transform_parent(context, import_entity, data) } + .then { |data| transform_parent(context, import_entity, namespace, data) } .then { |data| transform_visibility_level(data) } .then { |data| transform_project_creation_level(data) } .then { |data| transform_subgroup_creation_level(data) } @@ -19,9 +23,8 @@ def transform(context, data) private - def transform_name(import_entity, data) - if import_entity.destination_namespace.present? - namespace = Namespace.find_by_full_path(import_entity.destination_namespace) + def transform_name(import_entity, namespace, data) + if namespace.present? namespace_children_names = namespace.children.pluck(:name) # rubocop: disable CodeReuse/ActiveRecord new_name = source_name = data['name'] counter = 1 @@ -46,11 +49,8 @@ def transform_full_path(data) data end - def transform_parent(context, import_entity, data) - unless import_entity.destination_namespace.blank? - namespace = Namespace.find_by_full_path(import_entity.destination_namespace) - data['parent_id'] = namespace.id - end + def transform_parent(context, import_entity, namespace, data) + data['parent_id'] = namespace.id if namespace.present? data end -- GitLab From a97719d05b900bb9a31217f1867ced3c4e644fdb Mon Sep 17 00:00:00 2001 From: Rodrigo Tomonari Date: Mon, 4 Jul 2022 13:24:34 +0100 Subject: [PATCH 3/3] Use Uniquify class --- .../transformers/group_attributes_transformer.rb | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/lib/bulk_imports/groups/transformers/group_attributes_transformer.rb b/lib/bulk_imports/groups/transformers/group_attributes_transformer.rb index 6bcd02effa65fd..3067e0997c243a 100644 --- a/lib/bulk_imports/groups/transformers/group_attributes_transformer.rb +++ b/lib/bulk_imports/groups/transformers/group_attributes_transformer.rb @@ -26,14 +26,12 @@ def transform(context, data) def transform_name(import_entity, namespace, data) if namespace.present? namespace_children_names = namespace.children.pluck(:name) # rubocop: disable CodeReuse/ActiveRecord - new_name = source_name = data['name'] - counter = 1 - while namespace_children_names.include?(new_name) - new_name = "#{source_name}(#{counter})" - counter += 1 - end - data['name'] = new_name + if namespace_children_names.include?(data['name']) + data['name'] = Uniquify.new(1).string(-> (counter) { "#{data['name']}(#{counter})" }) do |base| + namespace_children_names.include?(base) + end + end end data -- GitLab