Commit 0e58741c authored by Kassio Borges's avatar Kassio Borges

BulkImports: avoid infinity recursion on group migration

The current subgroup migration strategy works iteratively, which means
that each level of a group tree is imported before moving to the next
level.

This becomes a problem when the target group is within the source group,
which creates a _infinity_ loop of import.

>> Example:

Given a group tree like:

- Group A
  - Group B
  - Group C
    - Group X
    - Group Y

If one decide to import the Group A into Group B, a recursion will
happen, making the import stop only when the depth limit is achieved or
some other database constraint, like the size of a group path.

- Group A
  - Group B
    - Group A
      - Group B
        - Group A
          - Group B
            - Group A
              - Group B
                - ...
              - Group C
                - Group X
                - Group Y
          - Group C
            - Group X
            - Group Y
      - Group C
        - Group X
        - Group Y
  - Group C
    - Group X
    - Group Y

To fix that, destination namespaces cannot be a descendant of the source
group.

Related to: https://gitlab.com/gitlab-org/gitlab/-/issues/300445
parent 5a9393eb
...@@ -43,6 +43,8 @@ class BulkImports::Entity < ApplicationRecord ...@@ -43,6 +43,8 @@ class BulkImports::Entity < ApplicationRecord
validate :validate_parent_is_a_group, if: :parent validate :validate_parent_is_a_group, if: :parent
validate :validate_imported_entity_type validate :validate_imported_entity_type
validate :validate_destination_namespace_ascendency, if: :group_entity?
enum source_type: { group_entity: 0, project_entity: 1 } enum source_type: { group_entity: 0, project_entity: 1 }
state_machine :status, initial: :created do state_machine :status, initial: :created do
...@@ -107,4 +109,17 @@ class BulkImports::Entity < ApplicationRecord ...@@ -107,4 +109,17 @@ class BulkImports::Entity < ApplicationRecord
) )
end end
end end
def validate_destination_namespace_ascendency
source = Group.find_by_full_path(source_full_path)
return unless source
if source.self_and_descendants.any? { |namespace| namespace.full_path == destination_namespace }
errors.add(
:destination_namespace,
s_('BulkImport|destination group cannot be part of the source group tree')
)
end
end
end end
---
title: 'BulkImports: avoid infinity recursion on group migration'
merge_request: 52931
author:
type: fixed
...@@ -4926,6 +4926,9 @@ msgstr "" ...@@ -4926,6 +4926,9 @@ msgstr ""
msgid "BulkImport|Update of import statuses with realtime changes failed" msgid "BulkImport|Update of import statuses with realtime changes failed"
msgstr "" msgstr ""
msgid "BulkImport|destination group cannot be part of the source group tree"
msgstr ""
msgid "BulkImport|expected an associated Group but has an associated Project" msgid "BulkImport|expected an associated Group but has an associated Project"
msgstr "" msgstr ""
......
...@@ -81,6 +81,37 @@ RSpec.describe BulkImports::Entity, type: :model do ...@@ -81,6 +81,37 @@ RSpec.describe BulkImports::Entity, type: :model do
expect(entity.errors).to include(:parent) expect(entity.errors).to include(:parent)
end end
end end
context 'validate destination namespace of a group_entity' do
it 'is invalid if destination namespace is the source namespace' do
group_a = create(:group, path: 'group_a')
entity = build(
:bulk_import_entity,
:group_entity,
source_full_path: group_a.full_path,
destination_namespace: group_a.full_path
)
expect(entity).not_to be_valid
expect(entity.errors).to include(:destination_namespace)
end
it 'is invalid if destination namespace is a descendant of the source' do
group_a = create(:group, path: 'group_a')
group_b = create(:group, parent: group_a, path: 'group_b')
entity = build(
:bulk_import_entity,
:group_entity,
source_full_path: group_a.full_path,
destination_namespace: group_b.full_path
)
expect(entity).not_to be_valid
expect(entity.errors).to include(:destination_namespace)
end
end
end end
describe "#update_tracker_for" do describe "#update_tracker_for" do
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment