From 4335eba3eed5345185bcb415a0f100a062365ce4 Mon Sep 17 00:00:00 2001 From: Rodrigo Tomonari Date: Wed, 24 Jul 2024 17:59:39 -0300 Subject: [PATCH 01/14] Integrate improved user mapping with Direct Transfer Update Direct Transfer to assign contributions to placeholder users for every existing member in the source instance. Contributions created by non-members are assigned to the Import User. --- app/models/approval.rb | 1 + app/models/award_emoji.rb | 1 + app/models/bulk_imports/configuration.rb | 4 + app/models/ci/pipeline.rb | 1 + app/models/ci/pipeline_schedule.rb | 1 + app/models/commit_status.rb | 1 + .../import/has_placeholder_user_references.rb | 9 ++ app/models/concerns/issuable.rb | 1 + app/models/deployment.rb | 1 + app/models/event.rb | 1 + app/models/import/source_user.rb | 4 + app/models/list.rb | 1 + app/models/merge_request.rb | 1 + app/models/merge_request/metrics.rb | 1 + app/models/merge_request_assignee.rb | 1 + app/models/merge_request_reviewer.rb | 1 + app/models/note.rb | 1 + app/models/release.rb | 1 + app/models/resource_event.rb | 1 + app/models/resource_label_event.rb | 1 + app/models/snippet.rb | 1 + app/models/timelog.rb | 1 + app/services/bulk_imports/create_service.rb | 4 + app/workers/bulk_imports/entity_worker.rb | 8 ++ .../approval_group_rules_user.rb | 1 + ee/app/models/board_assignee.rb | 2 + .../groups/pipelines/epics_pipeline_spec.rb | 2 +- .../common/graphql/get_members_query.rb | 1 + lib/bulk_imports/groups/stage.rb | 17 +++- lib/bulk_imports/ndjson_pipeline.rb | 59 +++++++++++- lib/bulk_imports/projects/stage.rb | 15 +++- lib/gitlab/import/source_user_mapper.rb | 54 +++++------ .../import_export/base/relation_factory.rb | 4 + .../base/relation_object_saver.rb | 20 +++-- .../pipelines/member_source_users_pipeline.rb | 89 +++++++++++++++++++ ...mber_source_user_attributes_transformer.rb | 60 +++++++++++++ lib/import/bulk_imports/ephemeral_data.rb | 37 ++++++++ .../bulk_imports/source_users_mapper.rb | 65 ++++++++++++++ 38 files changed, 436 insertions(+), 38 deletions(-) create mode 100644 app/models/concerns/import/has_placeholder_user_references.rb create mode 100644 lib/import/bulk_imports/common/pipelines/member_source_users_pipeline.rb create mode 100644 lib/import/bulk_imports/common/transformers/member_source_user_attributes_transformer.rb create mode 100644 lib/import/bulk_imports/ephemeral_data.rb create mode 100644 lib/import/bulk_imports/source_users_mapper.rb diff --git a/app/models/approval.rb b/app/models/approval.rb index c3992994dd3a00..e771e3a23b5c33 100644 --- a/app/models/approval.rb +++ b/app/models/approval.rb @@ -3,6 +3,7 @@ class Approval < ApplicationRecord include CreatedAtFilterable include Importable + include Import::HasPlaceholderUserReferences include ShaAttribute belongs_to :user diff --git a/app/models/award_emoji.rb b/app/models/award_emoji.rb index 6db92efa8a0bee..d9b4513685dba8 100644 --- a/app/models/award_emoji.rb +++ b/app/models/award_emoji.rb @@ -7,6 +7,7 @@ class AwardEmoji < ApplicationRecord include Participable include GhostUser include Importable + include Import::HasPlaceholderUserReferences belongs_to :awardable, polymorphic: true # rubocop:disable Cop/PolymorphicAssociations belongs_to :user diff --git a/app/models/bulk_imports/configuration.rb b/app/models/bulk_imports/configuration.rb index 6d9f598583ea05..2c4aa6c14b0e3f 100644 --- a/app/models/bulk_imports/configuration.rb +++ b/app/models/bulk_imports/configuration.rb @@ -19,4 +19,8 @@ class BulkImports::Configuration < ApplicationRecord key: Settings.attr_encrypted_db_key_base_32, mode: :per_attribute_iv, algorithm: 'aes-256-gcm' + + def source_hostname + URI.parse(url).host + end end diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb index f37fa2edcf81c3..82c6906c75504d 100644 --- a/app/models/ci/pipeline.rb +++ b/app/models/ci/pipeline.rb @@ -6,6 +6,7 @@ class Pipeline < Ci::ApplicationRecord include Ci::HasStatus include Ci::HasCompletionReason include Importable + include Import::HasPlaceholderUserReferences include AfterCommitQueue include Presentable include Gitlab::Allowable diff --git a/app/models/ci/pipeline_schedule.rb b/app/models/ci/pipeline_schedule.rb index a7b4a5b84c4fac..4cc0e77c2a3ad9 100644 --- a/app/models/ci/pipeline_schedule.rb +++ b/app/models/ci/pipeline_schedule.rb @@ -4,6 +4,7 @@ module Ci class PipelineSchedule < Ci::ApplicationRecord extend ::Gitlab::Utils::Override include Importable + include Import::HasPlaceholderUserReferences include StripAttribute include CronSchedulable include Limitable diff --git a/app/models/commit_status.rb b/app/models/commit_status.rb index 4edb403514fc41..de7eb587387223 100644 --- a/app/models/commit_status.rb +++ b/app/models/commit_status.rb @@ -4,6 +4,7 @@ class CommitStatus < Ci::ApplicationRecord include Ci::Partitionable include Ci::HasStatus include Importable + include Import::HasPlaceholderUserReferences include AfterCommitQueue include Presentable include BulkInsertableAssociations diff --git a/app/models/concerns/import/has_placeholder_user_references.rb b/app/models/concerns/import/has_placeholder_user_references.rb new file mode 100644 index 00000000000000..416c795be7ab5b --- /dev/null +++ b/app/models/concerns/import/has_placeholder_user_references.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +module Import + module HasPlaceholderUserReferences + extend ActiveSupport::Concern + + attr_accessor :placeholder_user_references + end +end diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb index e8d89173585a6f..7dc0ee50e9c7d1 100644 --- a/app/models/concerns/issuable.rb +++ b/app/models/concerns/issuable.rb @@ -19,6 +19,7 @@ module Issuable include Awardable include Taskable include Importable + include Import::HasPlaceholderUserReferences include Transitionable include Editable include AfterCommitQueue diff --git a/app/models/deployment.rb b/app/models/deployment.rb index ae5f72529f01ef..eb5558c5adee3c 100644 --- a/app/models/deployment.rb +++ b/app/models/deployment.rb @@ -6,6 +6,7 @@ class Deployment < ApplicationRecord include AfterCommitQueue include UpdatedAtFilterable include Importable + include Import::HasPlaceholderUserReferences include Gitlab::Utils::StrongMemoize include FastDestroyAll include EachBatch diff --git a/app/models/event.rb b/app/models/event.rb index 0ed7bbe8a03ea8..b9b3d17dbc6510 100644 --- a/app/models/event.rb +++ b/app/models/event.rb @@ -11,6 +11,7 @@ class Event < ApplicationRecord include ShaAttribute include EachBatch include Import::HasImportSource + include Import::HasPlaceholderUserReferences ACTIONS = HashWithIndifferentAccess.new( created: 1, diff --git a/app/models/import/source_user.rb b/app/models/import/source_user.rb index 29141ad4d91ee6..598739ad6cbdd3 100644 --- a/app/models/import/source_user.rb +++ b/app/models/import/source_user.rb @@ -108,5 +108,9 @@ def reassignable_status? def cancelable_status? awaiting_approval? || rejected? end + + def assignee + accepted_status? ? reassign_to_user : placeholder_user + end end end diff --git a/app/models/list.rb b/app/models/list.rb index fba0e51bdf8da5..185cb4dd8881cf 100644 --- a/app/models/list.rb +++ b/app/models/list.rb @@ -3,6 +3,7 @@ class List < ApplicationRecord include Boards::Listable include Importable + include Import::HasPlaceholderUserReferences belongs_to :board belongs_to :label diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index dd68cb0d95fe59..f5c2991542006e 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -26,6 +26,7 @@ class MergeRequest < ApplicationRecord include IdInOrdered include Todoable include Spammable + include Import::HasPlaceholderUserReferences ignore_columns :head_pipeline_id_convert_to_bigint, remove_with: '17.4', remove_after: '2024-08-14' diff --git a/app/models/merge_request/metrics.rb b/app/models/merge_request/metrics.rb index 05ce207c9fbe3a..96d168582f8a7f 100644 --- a/app/models/merge_request/metrics.rb +++ b/app/models/merge_request/metrics.rb @@ -1,6 +1,7 @@ # frozen_string_literal: true class MergeRequest::Metrics < ApplicationRecord + include Import::HasPlaceholderUserReferences include IgnorableColumns ignore_columns :pipeline_id_convert_to_bigint, remove_with: '17.4', remove_after: '2024-08-14' diff --git a/app/models/merge_request_assignee.rb b/app/models/merge_request_assignee.rb index 3e4a35805c9e74..231e5160fe2ec6 100644 --- a/app/models/merge_request_assignee.rb +++ b/app/models/merge_request_assignee.rb @@ -1,6 +1,7 @@ # frozen_string_literal: true class MergeRequestAssignee < ApplicationRecord + include Import::HasPlaceholderUserReferences include EachBatch belongs_to :merge_request, touch: true diff --git a/app/models/merge_request_reviewer.rb b/app/models/merge_request_reviewer.rb index e1e2805cd8f349..362d3a2710d28a 100644 --- a/app/models/merge_request_reviewer.rb +++ b/app/models/merge_request_reviewer.rb @@ -2,6 +2,7 @@ class MergeRequestReviewer < ApplicationRecord include MergeRequestReviewerState + include Import::HasPlaceholderUserReferences include BulkInsertSafe # must be included _last_ i.e. after any other concerns belongs_to :merge_request diff --git a/app/models/note.rb b/app/models/note.rb index d0cb30529a10d2..aa8d9f08a777ed 100644 --- a/app/models/note.rb +++ b/app/models/note.rb @@ -16,6 +16,7 @@ class Note < ApplicationRecord include Awardable include Importable include Import::HasImportSource + include Import::HasPlaceholderUserReferences include FasterCacheKeys include Redactable include CacheMarkdownField diff --git a/app/models/release.rb b/app/models/release.rb index 322c10328c665f..5854bc71f12ee2 100644 --- a/app/models/release.rb +++ b/app/models/release.rb @@ -4,6 +4,7 @@ class Release < ApplicationRecord include Presentable include CacheMarkdownField include Importable + include Import::HasPlaceholderUserReferences include Gitlab::Utils::StrongMemoize include EachBatch include FromUnion diff --git a/app/models/resource_event.rb b/app/models/resource_event.rb index 88c1576a41bf42..4b947a9cbd986e 100644 --- a/app/models/resource_event.rb +++ b/app/models/resource_event.rb @@ -3,6 +3,7 @@ class ResourceEvent < ApplicationRecord include Gitlab::Utils::StrongMemoize include Importable + include Import::HasPlaceholderUserReferences include IssueResourceEvent include WorkItemResourceEvent diff --git a/app/models/resource_label_event.rb b/app/models/resource_label_event.rb index 5ee6f03047e440..d1b28e5c0c65b0 100644 --- a/app/models/resource_label_event.rb +++ b/app/models/resource_label_event.rb @@ -4,6 +4,7 @@ class ResourceLabelEvent < ResourceEvent include CacheMarkdownField include MergeRequestResourceEvent include Import::HasImportSource + include Import::HasPlaceholderUserReferences include FromUnion cache_markdown_field :reference diff --git a/app/models/snippet.rb b/app/models/snippet.rb index ba30cbc71dace7..831ea8a9801b6c 100644 --- a/app/models/snippet.rb +++ b/app/models/snippet.rb @@ -20,6 +20,7 @@ class Snippet < ApplicationRecord include CreatedAtFilterable include EachBatch include Import::HasImportSource + include Import::HasPlaceholderUserReferences include SafelyChangeColumnDefault columns_changing_default :organization_id diff --git a/app/models/timelog.rb b/app/models/timelog.rb index 8f6c025a6f1c7c..249bd7bcacf626 100644 --- a/app/models/timelog.rb +++ b/app/models/timelog.rb @@ -6,6 +6,7 @@ class Timelog < ApplicationRecord MAX_TOTAL_TIME_SPENT = 31557600.seconds.to_i # a year include Importable + include Import::HasPlaceholderUserReferences include Sortable before_save :set_project diff --git a/app/services/bulk_imports/create_service.rb b/app/services/bulk_imports/create_service.rb index b7b851eb70567b..88aa764a643495 100644 --- a/app/services/bulk_imports/create_service.rb +++ b/app/services/bulk_imports/create_service.rb @@ -85,6 +85,10 @@ def create_bulk_import ) bulk_import.create_configuration!(credentials.slice(:url, :access_token)) + if Feature.enabled?(:importer_user_mapping, current_user) + ::Import::BulkImports::EphemeralData.new(bulk_import.id).enable_importer_user_mapping + end + Array.wrap(params).each do |entity_params| track_access_level(entity_params) diff --git a/app/workers/bulk_imports/entity_worker.rb b/app/workers/bulk_imports/entity_worker.rb index 81edafbc696bad..a1f846ca4c296c 100644 --- a/app/workers/bulk_imports/entity_worker.rb +++ b/app/workers/bulk_imports/entity_worker.rb @@ -78,6 +78,14 @@ def start_next_stage pipeline_tracker.stage, entity.id ) + + if Import::BulkImports::EphemeralData.new(entity.bulk_import.id).importer_user_mapping_enabled? + Import::LoadPlaceholderReferencesWorker.perform_async( + Import::SOURCE_DIRECT_TRANSFER, + entity.bulk_import.id, + 'current_user_id' => entity.bulk_import.user_id + ) + end end end end diff --git a/ee/app/models/approval_rules/approval_group_rules_user.rb b/ee/app/models/approval_rules/approval_group_rules_user.rb index 5b5da9fa6df3f6..a0e02dae25bc6a 100644 --- a/ee/app/models/approval_rules/approval_group_rules_user.rb +++ b/ee/app/models/approval_rules/approval_group_rules_user.rb @@ -5,6 +5,7 @@ module ApprovalRules class ApprovalGroupRulesUser < ApplicationRecord include ApprovalRuleUserLike + include Import::HasPlaceholderUserReferences belongs_to :user belongs_to :approval_group_rule, class_name: 'ApprovalRules::ApprovalGroupRule' diff --git a/ee/app/models/board_assignee.rb b/ee/app/models/board_assignee.rb index d18d0a4678298c..73f27713a1294b 100644 --- a/ee/app/models/board_assignee.rb +++ b/ee/app/models/board_assignee.rb @@ -1,6 +1,8 @@ # frozen_string_literal: true class BoardAssignee < ApplicationRecord + include Import::HasPlaceholderUserReferences + belongs_to :board belongs_to :assignee, class_name: 'User' diff --git a/ee/spec/lib/bulk_imports/groups/pipelines/epics_pipeline_spec.rb b/ee/spec/lib/bulk_imports/groups/pipelines/epics_pipeline_spec.rb index 2e5ae57b67a971..421d09a5c495b9 100644 --- a/ee/spec/lib/bulk_imports/groups/pipelines/epics_pipeline_spec.rb +++ b/ee/spec/lib/bulk_imports/groups/pipelines/epics_pipeline_spec.rb @@ -5,7 +5,7 @@ RSpec.describe BulkImports::Groups::Pipelines::EpicsPipeline, feature_category: :importers do let_it_be(:user) { create(:user) } let_it_be(:group) { create(:group) } - let_it_be(:bulk_import) { create(:bulk_import, user: user) } + let_it_be(:bulk_import) { create(:bulk_import, :with_configuration, user: user) } let_it_be(:filepath) { 'ee/spec/fixtures/bulk_imports/gz/epics.ndjson.gz' } let_it_be(:entity) do create( diff --git a/lib/bulk_imports/common/graphql/get_members_query.rb b/lib/bulk_imports/common/graphql/get_members_query.rb index 4f7533ee25f1ef..9fcd1da7013dab 100644 --- a/lib/bulk_imports/common/graphql/get_members_query.rb +++ b/lib/bulk_imports/common/graphql/get_members_query.rb @@ -30,6 +30,7 @@ def to_s user_gid: id public_email: publicEmail username: username + name: name } } } diff --git a/lib/bulk_imports/groups/stage.rb b/lib/bulk_imports/groups/stage.rb index 0abb355d796d08..eb0fb0ecec7c7d 100644 --- a/lib/bulk_imports/groups/stage.rb +++ b/lib/bulk_imports/groups/stage.rb @@ -70,7 +70,9 @@ def config pipeline: BulkImports::Common::Pipelines::EntityFinisher, stage: 4 } - }.merge(project_entities_pipeline) + } + .merge(project_entities_pipeline) + .merge(importer_user_mapping) end def project_entities_pipeline @@ -86,6 +88,19 @@ def project_entities_pipeline end end + def importer_user_mapping + if Import::BulkImports::EphemeralData.new(bulk_import.id).importer_user_mapping_enabled? + { + members: { + pipeline: Import::BulkImports::Common::Pipelines::MemberSourceUsersPipeline, + stage: 1 + } + } + else + {} + end + end + def migrate_projects? bulk_import_entity.migrate_projects end diff --git a/lib/bulk_imports/ndjson_pipeline.rb b/lib/bulk_imports/ndjson_pipeline.rb index a038d445f0ba91..37d1b1856e96ac 100644 --- a/lib/bulk_imports/ndjson_pipeline.rb +++ b/lib/bulk_imports/ndjson_pipeline.rb @@ -46,7 +46,9 @@ def load(_context, object) importable: portable ) - saver.execute + saver.execute do |persisted_objects| + push_placeholder_references(persisted_objects) + end capture_invalid_subrelations(saver.invalid_subrelations) else @@ -85,6 +87,8 @@ def deep_transform_relation!(relation_hash, relation_key, relation_definition, & end end + create_import_source_users(relation_key, relation_hash) + yield(relation_key, relation_hash) end @@ -119,7 +123,11 @@ def relation end def members_mapper - @members_mapper ||= BulkImports::UsersMapper.new(context: context) + @members_mapper ||= if EphemeralData.new(context.bulk_import_id).importer_user_mapping? + SourceUsersMapper.new(context: context) + else + UsersMapper.new(context: context) + end end def portable_class_sym @@ -142,6 +150,53 @@ def capture_invalid_subrelations(invalid_subrelations) ) end end + + def source_user_mapper + @source_user_mapper ||= Gitlab::Import::SourceUserMapper.new( + namespace: context.portable.root_ancestor, + import_type: Import::SOURCE_DIRECT_TRANSFER, + source_hostname: context.configuration.source_hostname + ) + end + + # Creates an Import::SourceUser for each source_user_identifier found in the relation_hash + # and associate it with the ImportUser + def create_import_source_users(_relation_key, relation_hash) + relation_factory::USER_REFERENCES.each do |reference| + next unless relation_hash[reference] + + # TODO: Add cache + source_user_mapper.find_or_create_source_user( + source_name: nil, + source_username: nil, + source_user_identifier: relation_hash[reference], + use_import_user: true + ) + end + end + + def push_placeholder_references(persisted_objects) + persisted_objects.each do |persisted_object| + next unless persisted_object.respond_to?(:placeholder_user_references) + + persisted_object.placeholder_user_references.each do |attribute, source_user_identifier| + # TODO: Add cache + source_user = source_user_mapper.find_source_user(source_user_identifier) + + ::Import::PlaceholderReferences::PushService.from_record( + import_source: ::Import::SOURCE_DIRECT_TRANSFER, + import_uid: context.bulk_import_id, + record: persisted_object, + user_reference_column: attribute.to_sym, + source_user: source_user + ).execute + end + end + end + + def importer_user_mapping_enabled? + Import::BulkImports::EphemeralData.new(context.bulk_import_id).importer_user_mapping_enabled? + end end def persist_relation(attributes) diff --git a/lib/bulk_imports/projects/stage.rb b/lib/bulk_imports/projects/stage.rb index eecd567f54f2ff..a65406942b3f77 100644 --- a/lib/bulk_imports/projects/stage.rb +++ b/lib/bulk_imports/projects/stage.rb @@ -141,9 +141,22 @@ def config finisher: { pipeline: BulkImports::Common::Pipelines::EntityFinisher, stage: 6 - } + }.merge(importer_user_mapping) } end + + def importer_user_mapping + if Import::BulkImports::EphemeralData.new(bulk_import.id).importer_user_mapping_enabled? + { + members: { + pipeline: Import::BulkImports::Common::Pipelines::MemberSourceUsersPipeline, + stage: 1 + } + } + else + {} + end + end end end end diff --git a/lib/gitlab/import/source_user_mapper.rb b/lib/gitlab/import/source_user_mapper.rb index dc23f890ce809a..9f76b95e73787b 100644 --- a/lib/gitlab/import/source_user_mapper.rb +++ b/lib/gitlab/import/source_user_mapper.rb @@ -11,42 +11,35 @@ def initialize(namespace:, import_type:, source_hostname:) @source_hostname = source_hostname end - def find_or_create_internal_user(source_name:, source_username:, source_user_identifier:) - @source_name = source_name - @source_username = source_username - @source_user_identifier = source_user_identifier + def find_source_user(source_user_identifier) + ::Import::SourceUser.find_source_user( + source_user_identifier: source_user_identifier, + namespace: namespace, + source_hostname: source_hostname, + import_type: import_type + ) + end + + def find_or_create_source_user(source_name:, source_username:, source_user_identifier:, use_import_user: false) + source_user = find_source_user(source_user_identifier) - internal_user = find_internal_user - return internal_user if internal_user + return source_user if source_user in_lock(lock_key(source_user_identifier), sleep_sec: 2.seconds) do |retried| if retried - internal_user = find_internal_user - next internal_user if internal_user + source_user = find_source_user(source_user_identifier) + next source_user if source_user end - create_source_user_mapping + create_source_user_mapping(source_name, source_username, source_user_identifier, use_import_user) end end private - attr_reader :namespace, :import_type, :source_hostname, :source_name, :source_username, :source_user_identifier - - def find_internal_user - source_user = ::Import::SourceUser.find_source_user( - source_user_identifier: source_user_identifier, - namespace: namespace, - source_hostname: source_hostname, - import_type: import_type - ) - - return unless source_user + attr_reader :namespace, :import_type, :source_hostname - source_user.accepted_reassign_to_user || source_user.placeholder_user - end - - def create_source_user_mapping + def create_source_user_mapping(source_name, source_username, source_user_identifier, use_import_user) ::Import::SourceUser.transaction do import_source_user = ::Import::SourceUser.new( namespace: namespace, @@ -57,15 +50,18 @@ def create_source_user_mapping source_hostname: source_hostname ) - internal_user = create_placeholder_user - import_source_user.placeholder_user = internal_user + import_source_user.placeholder_user = if use_import_user + import_user + else + create_placeholder_user(source_name, source_username) + end import_source_user.save! import_source_user end end - def create_placeholder_user + def create_placeholder_user(source_name, source_username) # If limit is reached, get import user instead, but that's not implemented yet Gitlab::Import::PlaceholderUserCreator.new( import_type: import_type, @@ -75,6 +71,10 @@ def create_placeholder_user ).execute end + def import_user + @import_user ||= Gitlab::Import::ImportUserCreator.new(portable: namespace).execute + end + def lock_key(source_user_identifier) "import:source_user_mapper:#{namespace.id}:#{import_type}:#{source_hostname}:#{source_user_identifier}" end diff --git a/lib/gitlab/import_export/base/relation_factory.rb b/lib/gitlab/import_export/base/relation_factory.rb index ab3c7e4d6ea0e0..c41f664b723590 100644 --- a/lib/gitlab/import_export/base/relation_factory.rb +++ b/lib/gitlab/import_export/base/relation_factory.rb @@ -192,6 +192,10 @@ def imported_object existing_or_new_object.imported_from = @import_source end + if existing_or_new_object.respond_to?(:placeholder_user_references) + existing_or_new_object.placeholder_user_references = @original_user + end + existing_or_new_object rescue ActiveRecord::RecordNotUnique # as the operation is not atomic, retry in the unlikely scenario an INSERT is diff --git a/lib/gitlab/import_export/base/relation_object_saver.rb b/lib/gitlab/import_export/base/relation_object_saver.rb index 62bd650c1d5c8a..d539f4cbabed06 100644 --- a/lib/gitlab/import_export/base/relation_object_saver.rb +++ b/lib/gitlab/import_export/base/relation_object_saver.rb @@ -16,7 +16,7 @@ class RelationObjectSaver BATCH_SIZE = 100 - attr_reader :invalid_subrelations + attr_reader :invalid_subrelations, :valid_records # @param relation_object [Object] Object of a project/group, e.g. an issue # @param relation_key [String] Name of the object association to group/project, e.g. :issues @@ -38,12 +38,14 @@ def initialize(relation_object:, relation_key:, relation_definition:, importable @invalid_subrelations = [] end - def execute + def execute(&block) move_subrelations relation_object.save! - save_subrelations + yield [relation_object] if block + + save_subrelations(&block) end private @@ -51,7 +53,7 @@ def execute attr_reader :relation_object, :relation_key, :relation_definition, :importable, :collection_subrelations # rubocop:disable GitlabSecurity/PublicSend - def save_subrelations + def save_subrelations(&block) collection_subrelations.each_pair do |relation_name, records| records.each_slice(BATCH_SIZE) do |batch| valid_records, invalid_records = batch.partition { |record| record.valid? } @@ -67,10 +69,16 @@ def save_subrelations invalid_records.each do |invalid_record| relation_object.public_send(relation_name) << invalid_record - invalid_subrelations << invalid_record unless invalid_record.persisted? + if invalid_record.persisted? + valid_records << invalid_record + else + invalid_subrelations << invalid_record + end end - relation_object.save + relation_object.save.tap do + yield valid_records if block + end end end end diff --git a/lib/import/bulk_imports/common/pipelines/member_source_users_pipeline.rb b/lib/import/bulk_imports/common/pipelines/member_source_users_pipeline.rb new file mode 100644 index 00000000000000..266edc2357f825 --- /dev/null +++ b/lib/import/bulk_imports/common/pipelines/member_source_users_pipeline.rb @@ -0,0 +1,89 @@ +# frozen_string_literal: true + +module Import + module BulkImports + module Common + module Pipelines + class MemberSourceUsersPipeline + include ::BulkImports::Pipeline + + GROUP_MEMBER_RELATIONS = %i[direct inherited shared_from_groups].freeze + PROJECT_MEMBER_RELATIONS = %i[direct inherited invited_groups shared_into_ancestors].freeze + + transformer ::BulkImports::Common::Transformers::ProhibitedAttributesTransformer + transformer Transformers::MemberSourceUserAttributesTransformer + + def extract(context) + graphql_extractor.extract(context) + end + + def load(_context, data) + return unless data + + user_id = data[:user_id] + + # Current user is already a member + return if user_id == current_user.id + + user_membership = existing_user_membership(user_id) + + # User is already a member with higher existing (inherited) membership + return if user_membership && user_membership[:access_level] >= data[:access_level] + + source_source = data.delete(:source_user) + + # Create new membership for any other access level + member = portable.members.new(data) + member.importing = true # avoid sending new member notification to the invited user + member.save! + + push_placeholder_reference(source_source, member) + + member + end + + private + + def graphql_extractor + @graphql_extractor ||= ::BulkImports::Common::Extractors::GraphqlExtractor + .new(query: ::BulkImports::Common::Graphql::GetMembersQuery) + end + + def existing_user_membership(user_id) + execute_finder.find_by_user_id(user_id) + end + + def finder + @finder ||= if context.entity.group? + ::GroupMembersFinder.new(portable, current_user) + else + ::MembersFinder.new(portable, current_user) + end + end + + def execute_finder + finder.execute(include_relations: finder_relations) + end + + def finder_relations + if context.entity.group? + GROUP_MEMBER_RELATIONS + else + PROJECT_MEMBER_RELATIONS + end + end + + def push_placeholder_reference(source_source, member) + PlaceholderReferences::PushService.from_record( + import_source: SOURCE_DIRECT_TRANSFER, + import_uid: context.bulk_import_id, + record: member, + user_reference_column: :user_id, + source_user: source_source + ).execute + end + end + end + end + end +end diff --git a/lib/import/bulk_imports/common/transformers/member_source_user_attributes_transformer.rb b/lib/import/bulk_imports/common/transformers/member_source_user_attributes_transformer.rb new file mode 100644 index 00000000000000..45a009272214ad --- /dev/null +++ b/lib/import/bulk_imports/common/transformers/member_source_user_attributes_transformer.rb @@ -0,0 +1,60 @@ +# frozen_string_literal: true + +module Import + module BulkImports + module Common + module Transformers + class MemberSourceUserAttributesTransformer + def transform(context, data) + access_level = data&.dig('access_level', 'integer_value') + + return unless data + return unless valid_access_level?(access_level) + + source_user = find_or_create_source_user(context, data) + + return if source_user.assignee.import_user? + + { + user_id: source_user.assignee.id, + source_user: source_user, + access_level: access_level, + created_at: data['created_at'], + updated_at: data['updated_at'], + expires_at: data['expires_at'], + created_by_id: context.current_user.id + } + end + + private + + def valid_access_level?(access_level) + Gitlab::Access.options_with_owner.value?(access_level) + end + + def find_or_create_source_user(context, data) + gid = data&.dig('user', 'user_gid') + + source_user_id = GlobalID.parse(gid).model_id + source_name = data.dig('user', 'name') + source_username = data.dig('user', 'username') + + source_user_mapper(context).find_or_create_source_user( + source_user_identifier: source_user_id, + source_name: source_name, + source_username: source_username + ) + end + + def source_user_mapper(context) + Gitlab::Import::SourceUserMapper.new( + import_type: SOURCE_DIRECT_TRANSFER, + namespace: context.portable.root_ancestor, + source_hostname: context.configuration.source_hostname + ) + end + end + end + end + end +end diff --git a/lib/import/bulk_imports/ephemeral_data.rb b/lib/import/bulk_imports/ephemeral_data.rb new file mode 100644 index 00000000000000..cfb76f761ba4c6 --- /dev/null +++ b/lib/import/bulk_imports/ephemeral_data.rb @@ -0,0 +1,37 @@ +# frozen_string_literal: true + +# This class is used to store ephemeral data during a BulkImport. +module Import + module BulkImports + class EphemeralData + def initialize(bulk_import_id) + @bulk_import_id = bulk_import_id + end + + def add(field, value) + Gitlab::Cache::Import::Caching.hash_add(cache_key, field, value) + end + + def read(field) + hash = Gitlab::Cache::Import::Caching.values_from_hash(cache_key) + hash[field] + end + + def enable_importer_user_mapping + add('importer_user_mapping', 'enabled') + end + + def importer_user_mapping_enabled? + read('importer_user_mapping') == 'enabled' + end + + private + + attr_reader :bulk_import_id + + def cache_key + "bulk_import_ephemeral_data_#{bulk_import_id}" + end + end + end +end diff --git a/lib/import/bulk_imports/source_users_mapper.rb b/lib/import/bulk_imports/source_users_mapper.rb new file mode 100644 index 00000000000000..06d797c91c7ae8 --- /dev/null +++ b/lib/import/bulk_imports/source_users_mapper.rb @@ -0,0 +1,65 @@ +# frozen_string_literal: true + +module Import + module BulkImports + class SourceUsersMapper + include Gitlab::Utils::StrongMemoize + + class InnerCache + def initialize(source_user_mapper, import_user) + @source_user_mapper = source_user_mapper + @import_user = import_user + end + + def [](user_identifier) + find_source_user(user_identifier) || @import_user.id + end + + # TODO: Add cache for this method + def find_source_user(user_identifier) + @source_user_mapper.find_source_user(user_identifier)&.assignee&.id + end + end + + def initialize(context:) + @context = context + end + + def map + inner_cache + end + + def include?(user_identifier) + inner_cache.find_source_user(user_identifier) + end + + def default_user_id + import_user.id + end + + private + + attr_reader :context + + def root_ancestor + @root_ancestor ||= context.portable.root_ancestor + end + + def import_user + @import_user ||= Gitlab::Import::ImportUserCreator.new(portable: root_ancestor).execute + end + + def source_user_mapper + @source_user_mapper ||= Gitlab::Import::SourceUserMapper.new( + namespace: root_ancestor, + import_type: SOURCE_DIRECT_TRANSFER, + source_hostname: context.configuration.source_hostname + ) + end + + def inner_cache + @inner_cache ||= InnerCache.new(source_user_mapper, import_user) + end + end + end +end -- GitLab From 282aa0f648b06a8a1dcb3c6de6345920f97dbf8f Mon Sep 17 00:00:00 2001 From: Rodrigo Tomonari Date: Thu, 25 Jul 2024 14:22:39 -0300 Subject: [PATCH 02/14] Refactor code and add some specs --- app/models/import/source_user.rb | 8 +- app/services/bulk_imports/create_service.rb | 9 +- .../wip/bulk_import_importer_user_mapping.yml | 9 ++ .../common/pipelines/members_pipeline.rb | 16 ++ .../member_attributes_transformer.rb | 51 +++++- lib/bulk_imports/groups/stage.rb | 17 +- lib/bulk_imports/ndjson_pipeline.rb | 18 +-- lib/bulk_imports/pipeline/context.rb | 4 + lib/bulk_imports/projects/stage.rb | 15 +- lib/gitlab/cache/import/caching.rb | 16 +- lib/gitlab/import/source_user_mapper.rb | 13 ++ lib/gitlab/import_export/members_mapper.rb | 8 +- .../pipelines/member_source_users_pipeline.rb | 89 ---------- ...mber_source_user_attributes_transformer.rb | 60 ------- lib/import/bulk_imports/ephemeral_data.rb | 17 +- .../bulk_imports/source_users_mapper.rb | 25 +-- spec/factories/import_source_users.rb | 1 + .../common/pipelines/members_pipeline_spec.rb | 68 +++++++- .../member_attributes_transformer_spec.rb | 153 +++++++++++++++++- .../lib/bulk_imports/pipeline/context_spec.rb | 24 ++- spec/lib/bulk_imports/users_mapper_spec.rb | 6 - .../base/relation_object_saver_spec.rb | 12 ++ .../bulk_imports/ephemeral_data_spec.rb | 43 +++++ .../models/bulk_imports/configuration_spec.rb | 10 +- spec/models/import/source_user_spec.rb | 6 +- .../bulk_imports/create_service_spec.rb | 32 +++- 26 files changed, 482 insertions(+), 248 deletions(-) create mode 100644 config/feature_flags/wip/bulk_import_importer_user_mapping.yml delete mode 100644 lib/import/bulk_imports/common/pipelines/member_source_users_pipeline.rb delete mode 100644 lib/import/bulk_imports/common/transformers/member_source_user_attributes_transformer.rb create mode 100644 spec/lib/import/bulk_imports/ephemeral_data_spec.rb diff --git a/app/models/import/source_user.rb b/app/models/import/source_user.rb index 598739ad6cbdd3..119d3aec15f47e 100644 --- a/app/models/import/source_user.rb +++ b/app/models/import/source_user.rb @@ -93,8 +93,8 @@ def sort_by_attribute(method) end end - def accepted_reassign_to_user - reassign_to_user if accepted_status? + def assignee + accepted_status? ? reassign_to_user : placeholder_user end def accepted_status? @@ -108,9 +108,5 @@ def reassignable_status? def cancelable_status? awaiting_approval? || rejected? end - - def assignee - accepted_status? ? reassign_to_user : placeholder_user - end end end diff --git a/app/services/bulk_imports/create_service.rb b/app/services/bulk_imports/create_service.rb index 88aa764a643495..c5da7840c710e9 100644 --- a/app/services/bulk_imports/create_service.rb +++ b/app/services/bulk_imports/create_service.rb @@ -53,6 +53,11 @@ def execute extra: { source_equals_destination: source_equals_destination? } ) + if Feature.enabled?(:importer_user_mapping, current_user) && + Feature.enabled?(:bulk_import_importer_user_mapping, current_user) + ::Import::BulkImports::EphemeralData.new(bulk_import.id).enable_importer_user_mapping + end + BulkImportWorker.perform_async(bulk_import.id) ServiceResponse.success(payload: bulk_import) @@ -85,10 +90,6 @@ def create_bulk_import ) bulk_import.create_configuration!(credentials.slice(:url, :access_token)) - if Feature.enabled?(:importer_user_mapping, current_user) - ::Import::BulkImports::EphemeralData.new(bulk_import.id).enable_importer_user_mapping - end - Array.wrap(params).each do |entity_params| track_access_level(entity_params) diff --git a/config/feature_flags/wip/bulk_import_importer_user_mapping.yml b/config/feature_flags/wip/bulk_import_importer_user_mapping.yml new file mode 100644 index 00000000000000..1876f3ec6f16ab --- /dev/null +++ b/config/feature_flags/wip/bulk_import_importer_user_mapping.yml @@ -0,0 +1,9 @@ +--- +name: bulk_import_importer_user_mapping +feature_issue_url: https://gitlab.com/groups/gitlab-org/-/epics/12378 +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/160650 +rollout_issue_url: +milestone: '17.3' +group: group::import and integrate +type: wip +default_enabled: false diff --git a/lib/bulk_imports/common/pipelines/members_pipeline.rb b/lib/bulk_imports/common/pipelines/members_pipeline.rb index dbaae8f6ef23af..49cdcf432a9986 100644 --- a/lib/bulk_imports/common/pipelines/members_pipeline.rb +++ b/lib/bulk_imports/common/pipelines/members_pipeline.rb @@ -29,10 +29,16 @@ def load(_context, data) # User is already a member with higher existing (inherited) membership return if user_membership && user_membership[:access_level] >= data[:access_level] + source_source = data.delete(:source_user) + # Create new membership for any other access level member = portable.members.new(data) member.importing = true # avoid sending new member notification to the invited user member.save! + + push_placeholder_reference(source_source, member) if context.importer_user_mapping_enabled? + + member end private @@ -65,6 +71,16 @@ def finder_relations PROJECT_MEMBER_RELATIONS end end + + def push_placeholder_reference(source_source, member) + Import::PlaceholderReferences::PushService.from_record( + import_source: Import::SOURCE_DIRECT_TRANSFER, + import_uid: context.bulk_import_id, + record: member, + user_reference_column: :user_id, + source_user: source_source + ).execute + end end end end diff --git a/lib/bulk_imports/common/transformers/member_attributes_transformer.rb b/lib/bulk_imports/common/transformers/member_attributes_transformer.rb index 4cd87ec2b59a4d..aa12cbfbffb678 100644 --- a/lib/bulk_imports/common/transformers/member_attributes_transformer.rb +++ b/lib/bulk_imports/common/transformers/member_attributes_transformer.rb @@ -5,6 +5,8 @@ module Common module Transformers class MemberAttributesTransformer def transform(context, data) + return source_user_transformer(context, data) if context.importer_user_mapping_enabled? + user = find_user(data&.dig('user', 'public_email')) access_level = data&.dig('access_level', 'integer_value') @@ -12,7 +14,7 @@ def transform(context, data) return unless user return unless valid_access_level?(access_level) - cache_source_user_data(data, user, context) + cache_source_username_data(data, user, context) { user_id: user.id, @@ -26,6 +28,29 @@ def transform(context, data) private + def source_user_transformer(context, data) + access_level = data&.dig('access_level', 'integer_value') + + return unless data + return unless valid_access_level?(access_level) + + source_user = find_or_create_source_user(context, data) + + # To not add user as a member is the source user is assigned to the ImportUser because we cannot + # create multiple members for the same user. + return if source_user.assignee.import_user? + + { + user_id: source_user.assignee.id, + source_user: source_user, + access_level: access_level, + created_at: data['created_at'], + updated_at: data['updated_at'], + expires_at: data['expires_at'], + created_by_id: context.current_user.id + } + end + def find_user(email) return unless email @@ -36,7 +61,7 @@ def valid_access_level?(access_level) Gitlab::Access.options_with_owner.value?(access_level) end - def cache_source_user_data(data, user, context) + def cache_source_username_data(data, user, context) gid = data&.dig('user', 'user_gid') return unless gid @@ -52,6 +77,28 @@ def cache_source_user_data(data, user, context) mapper.cache_source_username(source_username, user.username) end + + def find_or_create_source_user(context, data) + gid = data&.dig('user', 'user_gid') + + source_user_id = GlobalID.parse(gid).model_id + source_name = data.dig('user', 'name') + source_username = data.dig('user', 'username') + + source_user_mapper(context).find_or_create_source_user( + source_user_identifier: source_user_id, + source_name: source_name, + source_username: source_username + ) + end + + def source_user_mapper(context) + Gitlab::Import::SourceUserMapper.new( + import_type: Import::SOURCE_DIRECT_TRANSFER, + namespace: context.portable.root_ancestor, + source_hostname: context.configuration.source_hostname + ) + end end end end diff --git a/lib/bulk_imports/groups/stage.rb b/lib/bulk_imports/groups/stage.rb index eb0fb0ecec7c7d..0abb355d796d08 100644 --- a/lib/bulk_imports/groups/stage.rb +++ b/lib/bulk_imports/groups/stage.rb @@ -70,9 +70,7 @@ def config pipeline: BulkImports::Common::Pipelines::EntityFinisher, stage: 4 } - } - .merge(project_entities_pipeline) - .merge(importer_user_mapping) + }.merge(project_entities_pipeline) end def project_entities_pipeline @@ -88,19 +86,6 @@ def project_entities_pipeline end end - def importer_user_mapping - if Import::BulkImports::EphemeralData.new(bulk_import.id).importer_user_mapping_enabled? - { - members: { - pipeline: Import::BulkImports::Common::Pipelines::MemberSourceUsersPipeline, - stage: 1 - } - } - else - {} - end - end - def migrate_projects? bulk_import_entity.migrate_projects end diff --git a/lib/bulk_imports/ndjson_pipeline.rb b/lib/bulk_imports/ndjson_pipeline.rb index 37d1b1856e96ac..f2ad0ccc44b62f 100644 --- a/lib/bulk_imports/ndjson_pipeline.rb +++ b/lib/bulk_imports/ndjson_pipeline.rb @@ -87,7 +87,7 @@ def deep_transform_relation!(relation_hash, relation_key, relation_definition, & end end - create_import_source_users(relation_key, relation_hash) + create_import_source_users(relation_key, relation_hash) if context.importer_user_mapping_enabled? yield(relation_key, relation_hash) end @@ -123,8 +123,8 @@ def relation end def members_mapper - @members_mapper ||= if EphemeralData.new(context.bulk_import_id).importer_user_mapping? - SourceUsersMapper.new(context: context) + @members_mapper ||= if context.importer_user_mapping_enabled? + Import::BulkImports::SourceUsersMapper.new(context: context) else UsersMapper.new(context: context) end @@ -159,13 +159,14 @@ def source_user_mapper ) end - # Creates an Import::SourceUser for each source_user_identifier found in the relation_hash - # and associate it with the ImportUser + # Creates an Import::SourceUser for each source_user_identifier found in the relation_hash and associate + # it with the ImportUser. def create_import_source_users(_relation_key, relation_hash) relation_factory::USER_REFERENCES.each do |reference| next unless relation_hash[reference] - # TODO: Add cache + next if source_user_mapper.source_user_exists?(relation_hash[reference]) + source_user_mapper.find_or_create_source_user( source_name: nil, source_username: nil, @@ -180,7 +181,6 @@ def push_placeholder_references(persisted_objects) next unless persisted_object.respond_to?(:placeholder_user_references) persisted_object.placeholder_user_references.each do |attribute, source_user_identifier| - # TODO: Add cache source_user = source_user_mapper.find_source_user(source_user_identifier) ::Import::PlaceholderReferences::PushService.from_record( @@ -193,10 +193,6 @@ def push_placeholder_references(persisted_objects) end end end - - def importer_user_mapping_enabled? - Import::BulkImports::EphemeralData.new(context.bulk_import_id).importer_user_mapping_enabled? - end end def persist_relation(attributes) diff --git a/lib/bulk_imports/pipeline/context.rb b/lib/bulk_imports/pipeline/context.rb index fde24cf3646f6f..c5d473852d4471 100644 --- a/lib/bulk_imports/pipeline/context.rb +++ b/lib/bulk_imports/pipeline/context.rb @@ -43,6 +43,10 @@ def current_user def configuration @configuration ||= bulk_import.configuration end + + def importer_user_mapping_enabled? + Import::BulkImports::EphemeralData.new(bulk_import_id).importer_user_mapping_enabled? + end end end end diff --git a/lib/bulk_imports/projects/stage.rb b/lib/bulk_imports/projects/stage.rb index a65406942b3f77..eecd567f54f2ff 100644 --- a/lib/bulk_imports/projects/stage.rb +++ b/lib/bulk_imports/projects/stage.rb @@ -141,21 +141,8 @@ def config finisher: { pipeline: BulkImports::Common::Pipelines::EntityFinisher, stage: 6 - }.merge(importer_user_mapping) - } - end - - def importer_user_mapping - if Import::BulkImports::EphemeralData.new(bulk_import.id).importer_user_mapping_enabled? - { - members: { - pipeline: Import::BulkImports::Common::Pipelines::MemberSourceUsersPipeline, - stage: 1 - } } - else - {} - end + } end end end diff --git a/lib/gitlab/cache/import/caching.rb b/lib/gitlab/cache/import/caching.rb index beaf9eb666fd95..7692af6875f19c 100644 --- a/lib/gitlab/cache/import/caching.rb +++ b/lib/gitlab/cache/import/caching.rb @@ -254,7 +254,7 @@ def self.hash_add(raw_key, field, value, timeout: TIMEOUT) # Returns the values of the given hash. # - # raw_key - The key of the set to check. + # raw_key - The key of the hash to check. def self.values_from_hash(raw_key) key = cache_key_for(raw_key) @@ -263,6 +263,20 @@ def self.values_from_hash(raw_key) end end + # Returns a single value of the given hash. + # + # raw_key - The key of the hash to check. + # field - The field to get from the hash. + def self.value_from_hash(raw_key, field, timeout: TIMEOUT) + key = cache_key_for(raw_key) + + value = with_redis { |redis| redis.hget(key, field) } + + with_redis { |redis| redis.expire(key, timeout) } if value.present? + + value + end + # Increments value of a field in a hash # # raw_key - The key of the hash to add to. diff --git a/lib/gitlab/import/source_user_mapper.rb b/lib/gitlab/import/source_user_mapper.rb index 9f76b95e73787b..ebe01b36cc74f8 100644 --- a/lib/gitlab/import/source_user_mapper.rb +++ b/lib/gitlab/import/source_user_mapper.rb @@ -25,6 +25,19 @@ def find_or_create_source_user(source_name:, source_username:, source_user_ident return source_user if source_user + create_source_user( + source_name: source_name, + source_username: source_username, + source_user_identifier: source_user_identifier, + use_import_user: use_import_user + ) + end + + def source_user_exists?(source_user_identifier) + find_source_user(source_user_identifier).present? + end + + def create_source_user(source_name:, source_username:, source_user_identifier:, use_import_user: false) in_lock(lock_key(source_user_identifier), sleep_sec: 2.seconds) do |retried| if retried source_user = find_source_user(source_user_identifier) diff --git a/lib/gitlab/import_export/members_mapper.rb b/lib/gitlab/import_export/members_mapper.rb index 635710f94ab6a3..9c3f5e3cf67223 100644 --- a/lib/gitlab/import_export/members_mapper.rb +++ b/lib/gitlab/import_export/members_mapper.rb @@ -26,16 +26,16 @@ def map end end - def default_user_id - @user.id - end - def include?(old_user_id) map.has_key?(old_user_id) end private + def default_user_id + @user.id + end + def missing_keys_tracking_hash Hash.new do |_, key| default_user_id diff --git a/lib/import/bulk_imports/common/pipelines/member_source_users_pipeline.rb b/lib/import/bulk_imports/common/pipelines/member_source_users_pipeline.rb deleted file mode 100644 index 266edc2357f825..00000000000000 --- a/lib/import/bulk_imports/common/pipelines/member_source_users_pipeline.rb +++ /dev/null @@ -1,89 +0,0 @@ -# frozen_string_literal: true - -module Import - module BulkImports - module Common - module Pipelines - class MemberSourceUsersPipeline - include ::BulkImports::Pipeline - - GROUP_MEMBER_RELATIONS = %i[direct inherited shared_from_groups].freeze - PROJECT_MEMBER_RELATIONS = %i[direct inherited invited_groups shared_into_ancestors].freeze - - transformer ::BulkImports::Common::Transformers::ProhibitedAttributesTransformer - transformer Transformers::MemberSourceUserAttributesTransformer - - def extract(context) - graphql_extractor.extract(context) - end - - def load(_context, data) - return unless data - - user_id = data[:user_id] - - # Current user is already a member - return if user_id == current_user.id - - user_membership = existing_user_membership(user_id) - - # User is already a member with higher existing (inherited) membership - return if user_membership && user_membership[:access_level] >= data[:access_level] - - source_source = data.delete(:source_user) - - # Create new membership for any other access level - member = portable.members.new(data) - member.importing = true # avoid sending new member notification to the invited user - member.save! - - push_placeholder_reference(source_source, member) - - member - end - - private - - def graphql_extractor - @graphql_extractor ||= ::BulkImports::Common::Extractors::GraphqlExtractor - .new(query: ::BulkImports::Common::Graphql::GetMembersQuery) - end - - def existing_user_membership(user_id) - execute_finder.find_by_user_id(user_id) - end - - def finder - @finder ||= if context.entity.group? - ::GroupMembersFinder.new(portable, current_user) - else - ::MembersFinder.new(portable, current_user) - end - end - - def execute_finder - finder.execute(include_relations: finder_relations) - end - - def finder_relations - if context.entity.group? - GROUP_MEMBER_RELATIONS - else - PROJECT_MEMBER_RELATIONS - end - end - - def push_placeholder_reference(source_source, member) - PlaceholderReferences::PushService.from_record( - import_source: SOURCE_DIRECT_TRANSFER, - import_uid: context.bulk_import_id, - record: member, - user_reference_column: :user_id, - source_user: source_source - ).execute - end - end - end - end - end -end diff --git a/lib/import/bulk_imports/common/transformers/member_source_user_attributes_transformer.rb b/lib/import/bulk_imports/common/transformers/member_source_user_attributes_transformer.rb deleted file mode 100644 index 45a009272214ad..00000000000000 --- a/lib/import/bulk_imports/common/transformers/member_source_user_attributes_transformer.rb +++ /dev/null @@ -1,60 +0,0 @@ -# frozen_string_literal: true - -module Import - module BulkImports - module Common - module Transformers - class MemberSourceUserAttributesTransformer - def transform(context, data) - access_level = data&.dig('access_level', 'integer_value') - - return unless data - return unless valid_access_level?(access_level) - - source_user = find_or_create_source_user(context, data) - - return if source_user.assignee.import_user? - - { - user_id: source_user.assignee.id, - source_user: source_user, - access_level: access_level, - created_at: data['created_at'], - updated_at: data['updated_at'], - expires_at: data['expires_at'], - created_by_id: context.current_user.id - } - end - - private - - def valid_access_level?(access_level) - Gitlab::Access.options_with_owner.value?(access_level) - end - - def find_or_create_source_user(context, data) - gid = data&.dig('user', 'user_gid') - - source_user_id = GlobalID.parse(gid).model_id - source_name = data.dig('user', 'name') - source_username = data.dig('user', 'username') - - source_user_mapper(context).find_or_create_source_user( - source_user_identifier: source_user_id, - source_name: source_name, - source_username: source_username - ) - end - - def source_user_mapper(context) - Gitlab::Import::SourceUserMapper.new( - import_type: SOURCE_DIRECT_TRANSFER, - namespace: context.portable.root_ancestor, - source_hostname: context.configuration.source_hostname - ) - end - end - end - end - end -end diff --git a/lib/import/bulk_imports/ephemeral_data.rb b/lib/import/bulk_imports/ephemeral_data.rb index cfb76f761ba4c6..68876f298c4627 100644 --- a/lib/import/bulk_imports/ephemeral_data.rb +++ b/lib/import/bulk_imports/ephemeral_data.rb @@ -8,15 +8,6 @@ def initialize(bulk_import_id) @bulk_import_id = bulk_import_id end - def add(field, value) - Gitlab::Cache::Import::Caching.hash_add(cache_key, field, value) - end - - def read(field) - hash = Gitlab::Cache::Import::Caching.values_from_hash(cache_key) - hash[field] - end - def enable_importer_user_mapping add('importer_user_mapping', 'enabled') end @@ -29,6 +20,14 @@ def importer_user_mapping_enabled? attr_reader :bulk_import_id + def add(field, value) + Gitlab::Cache::Import::Caching.hash_add(cache_key, field, value) + end + + def read(field) + Gitlab::Cache::Import::Caching.value_from_hash(cache_key, field) + end + def cache_key "bulk_import_ephemeral_data_#{bulk_import_id}" end diff --git a/lib/import/bulk_imports/source_users_mapper.rb b/lib/import/bulk_imports/source_users_mapper.rb index 06d797c91c7ae8..7598c66f1899cf 100644 --- a/lib/import/bulk_imports/source_users_mapper.rb +++ b/lib/import/bulk_imports/source_users_mapper.rb @@ -1,23 +1,28 @@ # frozen_string_literal: true +# This class is meant to be used by Gitlab::ImportExport::Base::RelationFactory when mapping users +# +# It's assumed that source users for each source_user_identifier have been created before executing the RelationFactory. module Import module BulkImports class SourceUsersMapper include Gitlab::Utils::StrongMemoize - class InnerCache + # Gitlab::ImportExport::Base::RelationFactory expects SourceUsersMapper#map to return a hash that is why + # a mocked hash is used here. + class MockedHash def initialize(source_user_mapper, import_user) @source_user_mapper = source_user_mapper @import_user = import_user + @memory_cache = {} end def [](user_identifier) - find_source_user(user_identifier) || @import_user.id + find_source_user(user_identifier) end - # TODO: Add cache for this method def find_source_user(user_identifier) - @source_user_mapper.find_source_user(user_identifier)&.assignee&.id + @memory_cache[user_identifier] ||= @source_user_mapper.find_source_user(user_identifier).assignee.id end end @@ -26,15 +31,11 @@ def initialize(context:) end def map - inner_cache + mocked_hash end def include?(user_identifier) - inner_cache.find_source_user(user_identifier) - end - - def default_user_id - import_user.id + mocked_hash.find_source_user(user_identifier) end private @@ -57,8 +58,8 @@ def source_user_mapper ) end - def inner_cache - @inner_cache ||= InnerCache.new(source_user_mapper, import_user) + def mocked_hash + @mocked_hash ||= MockedHash.new(source_user_mapper, import_user) end end end diff --git a/spec/factories/import_source_users.rb b/spec/factories/import_source_users.rb index 9bcb9c356ac40f..614dfcfb89bf21 100644 --- a/spec/factories/import_source_users.rb +++ b/spec/factories/import_source_users.rb @@ -27,6 +27,7 @@ end trait :completed do + reassign_to_user factory: :user status { 5 } end diff --git a/spec/lib/bulk_imports/common/pipelines/members_pipeline_spec.rb b/spec/lib/bulk_imports/common/pipelines/members_pipeline_spec.rb index 39ccdeb344d064..40d9418929c983 100644 --- a/spec/lib/bulk_imports/common/pipelines/members_pipeline_spec.rb +++ b/spec/lib/bulk_imports/common/pipelines/members_pipeline_spec.rb @@ -3,8 +3,9 @@ require 'spec_helper' RSpec.describe BulkImports::Common::Pipelines::MembersPipeline, feature_category: :importers do + let_it_be(:default_organization) { create(:organization, :default) } let_it_be(:user) { create(:user) } - let_it_be(:bulk_import) { create(:bulk_import, user: user) } + let_it_be(:bulk_import) { create(:bulk_import, :with_configuration, user: user) } let_it_be(:member_user1) { create(:user, email: 'email1@email.com') } let_it_be(:member_user2) { create(:user, email: 'email2@email.com') } let_it_be(:member_data) do @@ -29,7 +30,7 @@ allow(pipeline).to receive(:set_source_objects_counter) end - def extracted_data(email:, has_next_page: false) + def extracted_data(email: '', id: 1, has_next_page: false) data = { 'created_at' => '2020-01-01T00:00:00Z', 'updated_at' => '2020-01-02T00:00:00Z', @@ -38,7 +39,10 @@ def extracted_data(email:, has_next_page: false) 'integer_value' => 30 }, 'user' => { - 'public_email' => email + 'user_gid' => "gid://gitlab/User/#{id}", + 'public_email' => email, + 'name' => 'source_name', + 'username' => 'source_username' } } @@ -71,6 +75,64 @@ def extracted_data(email:, has_next_page: false) { user_id: member_user2.id, access_level: 30 } ) end + + context 'when importer_user_mapping is enabled' do + before do + allow_next_instance_of(Import::BulkImports::EphemeralData, bulk_import.id) do |ephemeral_data| + allow(ephemeral_data).to receive(:importer_user_mapping_enabled?).and_return(true) + end + + first_page = extracted_data(id: 101, has_next_page: true) + last_page = extracted_data(id: 102) + + allow_next_instance_of(BulkImports::Common::Extractors::GraphqlExtractor) do |extractor| + allow(extractor).to receive(:extract).and_return(first_page, last_page) + end + end + + let!(:import_source_user) do + create(:import_source_user, + namespace: context.portable.root_ancestor, + source_hostname: bulk_import.configuration.source_hostname, + import_type: Import::SOURCE_DIRECT_TRANSFER, + source_user_identifier: '101' + ) + end + + it 'find and creates source users and create membership for the assignee users' do + expect { pipeline.run }.to change { portable.members.count }.by(2).and( + change { Import::SourceUser.count }.by(1) + ) + + created_import_source_user = Import::SourceUser.find_source_user( + source_user_identifier: '102', + namespace: context.portable.root_ancestor, + source_hostname: bulk_import.configuration.source_hostname, + import_type: Import::SOURCE_DIRECT_TRANSFER + ) + + expect(members).to contain_exactly( + { user_id: import_source_user.placeholder_user.id, access_level: 30 }, + { user_id: created_import_source_user.placeholder_user.id, access_level: 30 } + ) + end + + it 'pushes placeholder references for each members' do + allow(Import::PlaceholderReferences::PushService).to receive(:from_record).twice + + pipeline.run + + portable.members.each do |member| + expect(Import::PlaceholderReferences::PushService).to have_received(:from_record).with( + import_source: Import::SOURCE_DIRECT_TRANSFER, + import_uid: bulk_import.id, + record: member, + user_reference_column: :user_id, + source_user: Import::SourceUser.find_by(placeholder_user: member.user) + ) + end + end + end end describe '#load' do diff --git a/spec/lib/bulk_imports/common/transformers/member_attributes_transformer_spec.rb b/spec/lib/bulk_imports/common/transformers/member_attributes_transformer_spec.rb index 4565de32c7010d..a176fb3cb74c78 100644 --- a/spec/lib/bulk_imports/common/transformers/member_attributes_transformer_spec.rb +++ b/spec/lib/bulk_imports/common/transformers/member_attributes_transformer_spec.rb @@ -2,10 +2,11 @@ require 'spec_helper' -RSpec.describe BulkImports::Common::Transformers::MemberAttributesTransformer, feature_category: :importers do +RSpec.describe BulkImports::Common::Transformers::MemberAttributesTransformer, :with_current_organization, feature_category: :importers do + let_it_be(:default_organization) { create(:organization, :default) } let_it_be(:user) { create(:user) } let_it_be(:secondary_email) { 'secondary@email.com' } - let_it_be(:bulk_import) { create(:bulk_import, user: user) } + let_it_be(:bulk_import) { create(:bulk_import, :with_configuration, user: user) } shared_examples 'members attribute transformer' do let_it_be(:tracker) { create(:bulk_import_tracker, entity: entity) } @@ -142,18 +143,159 @@ end end + shared_examples 'import source user members attribute transformer' do + let_it_be(:tracker) { create(:bulk_import_tracker, entity: entity) } + let_it_be(:context) { BulkImports::Pipeline::Context.new(tracker) } + + let_it_be(:import_source_user) do + create(:import_source_user, + namespace: context.portable.root_ancestor, + source_hostname: bulk_import.configuration.source_hostname, + import_type: Import::SOURCE_DIRECT_TRANSFER, + source_user_identifier: '101' + ) + end + + let_it_be(:reassigned_import_source_user) do + create(:import_source_user, :completed, + namespace: context.portable.root_ancestor, + source_hostname: bulk_import.configuration.source_hostname, + import_type: Import::SOURCE_DIRECT_TRANSFER, + source_user_identifier: '102' + ) + end + + let_it_be(:import_user_import_source_user) do + create(:import_source_user, + placeholder_user: create(:user, :import_user), + namespace: context.portable.root_ancestor, + source_hostname: bulk_import.configuration.source_hostname, + import_type: Import::SOURCE_DIRECT_TRANSFER, + source_user_identifier: '103' + ) + end + + it 'returns nil when receives no data' do + expect(subject.transform(context, nil)).to eq(nil) + end + + describe 'format access level' do + it 'ignores record if no access level is given' do + data = member_data(access_level: nil) + + expect(subject.transform(context, data)).to be_nil + end + + it 'ignores record if is not a valid access level' do + data = member_data(access_level: 999) + + expect(subject.transform(context, data)).to be_nil + end + end + + context 'when source user does not exist' do + let(:data) { member_data(gid: 'gid://gitlab/User/999') } + + it 'creates a import source user' do + expect { subject.transform(context, data) }.to change { Import::SourceUser.count }.by(1) + + expect(Import::SourceUser.last).to have_attributes( + source_user_identifier: '999', + source_username: 'source_username', + source_name: 'source_name', + import_type: Import::SOURCE_DIRECT_TRANSFER.to_s + ) + end + + it 'uses import source user placeholder_user as user' do + expect(subject.transform(context, data)).to match( + a_hash_including({ + user_id: Import::SourceUser.last.placeholder_user.id, + source_user: Import::SourceUser.last + }) + ) + end + end + + context 'when a source user exists and is using a placeholder user' do + let(:data) { member_data(gid: 'gid://gitlab/User/101') } + + it 'does not create a import source user' do + expect { subject.transform(context, data) }.not_to change { Import::SourceUser.count } + end + + it 'uses import source user placeholder_user as user' do + expect(subject.transform(context, data)).to match( + a_hash_including({ + user_id: import_source_user.placeholder_user.id, + source_user: import_source_user + }) + ) + end + end + + context 'when a source user exists and was reassigned to a real user' do + let(:data) { member_data(gid: 'gid://gitlab/User/102') } + + it 'does not create a import source user' do + expect { subject.transform(context, data) }.not_to change { Import::SourceUser.count } + end + + it 'uses import source user reassign_to_user as user' do + expect(subject.transform(context, data)).to match( + hash_including({ + user_id: reassigned_import_source_user.reassign_to_user.id, + source_user: reassigned_import_source_user + }) + ) + end + end + + context 'when a source user exists and is using the ImportUser' do + let(:data) { member_data(gid: 'gid://gitlab/User/103') } + + it 'does not create a import source user' do + expect { subject.transform(context, data) }.not_to change { Import::SourceUser.count } + end + + it 'returns nil' do + expect(subject.transform(context, data)).to eq(nil) + end + end + end + context 'with a project' do - let_it_be(:entity) { create(:bulk_import_entity, bulk_import: bulk_import, project: project) } let_it_be(:project) { create(:project) } + let_it_be(:entity) { create(:bulk_import_entity, :project_entity, bulk_import: bulk_import, project: project) } include_examples 'members attribute transformer' + + context 'when importer_user_mapping is enabled' do + before do + allow_next_instance_of(Import::BulkImports::EphemeralData, bulk_import.id) do |ephemeral_data| + allow(ephemeral_data).to receive(:importer_user_mapping_enabled?).and_return(true) + end + end + + include_examples 'import source user members attribute transformer' + end end context 'with a group' do - let_it_be(:entity) { create(:bulk_import_entity, bulk_import: bulk_import, group: group) } let_it_be(:group) { create(:group) } + let_it_be(:entity) { create(:bulk_import_entity, bulk_import: bulk_import, group: group) } include_examples 'members attribute transformer' + + context 'when importer_user_mapping is enabled' do + before do + allow_next_instance_of(Import::BulkImports::EphemeralData, bulk_import.id) do |ephemeral_data| + allow(ephemeral_data).to receive(:importer_user_mapping_enabled?).and_return(true) + end + end + + include_examples 'import source user members attribute transformer' + end end def member_data(email: '', gid: nil, access_level: 30) @@ -167,7 +309,8 @@ def member_data(email: '', gid: nil, access_level: 30) 'user' => { 'user_gid' => gid, 'public_email' => email, - 'username' => 'source_username' + 'username' => 'source_username', + 'name' => 'source_name' } } end diff --git a/spec/lib/bulk_imports/pipeline/context_spec.rb b/spec/lib/bulk_imports/pipeline/context_spec.rb index 0f1d00172cdce0..b9fb498e291ca3 100644 --- a/spec/lib/bulk_imports/pipeline/context_spec.rb +++ b/spec/lib/bulk_imports/pipeline/context_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe BulkImports::Pipeline::Context do +RSpec.describe BulkImports::Pipeline::Context, feature_category: :importers do let_it_be(:user) { create(:user) } let_it_be(:group) { create(:group) } let_it_be(:bulk_import) { create(:bulk_import, user: user) } @@ -74,4 +74,26 @@ it { expect(subject.import_export_config).to be_instance_of(BulkImports::FileTransfer::ProjectConfig) } end end + + describe '#importer_user_mapping_enabled?' do + subject { described_class.new(tracker, extra: :data).importer_user_mapping_enabled? } + + before do + allow_next_instance_of(Import::BulkImports::EphemeralData, bulk_import.id) do |ephemeral_data| + allow(ephemeral_data).to receive(:importer_user_mapping_enabled?).and_return(status) + end + end + + context 'when importer user mapping is disabled' do + let(:status) { false } + + it { is_expected.to eq(false) } + end + + context 'when importer user mapping is enabled' do + let(:status) { true } + + it { is_expected.to eq(true) } + end + end end diff --git a/spec/lib/bulk_imports/users_mapper_spec.rb b/spec/lib/bulk_imports/users_mapper_spec.rb index dc2beb42080457..3ddb165edf23cf 100644 --- a/spec/lib/bulk_imports/users_mapper_spec.rb +++ b/spec/lib/bulk_imports/users_mapper_spec.rb @@ -50,12 +50,6 @@ end end - describe '#default_user_id' do - it 'returns current user id' do - expect(subject.default_user_id).to eq(user.id) - end - end - describe '#include?' do context 'when source user id is present in the map' do it 'returns true' do diff --git a/spec/lib/gitlab/import_export/base/relation_object_saver_spec.rb b/spec/lib/gitlab/import_export/base/relation_object_saver_spec.rb index 13d94fdb6feaca..2233db0a47f789 100644 --- a/spec/lib/gitlab/import_export/base/relation_object_saver_spec.rb +++ b/spec/lib/gitlab/import_export/base/relation_object_saver_spec.rb @@ -41,6 +41,10 @@ issue = project.issues.last expect(issue.notes.count).to eq(2) end + + it 'yields the saved records' do + expect { |b| saver.execute(&b) }.to yield_successive_args([relation_object], notes) + end end context 'when subrelation is not a collection' do @@ -76,6 +80,10 @@ expect(issue.notes.count).to eq(1) end + it 'yields the saved records' do + expect { |b| saver.execute(&b) }.to yield_successive_args([relation_object], [note]) + end + context 'when invalid subrelation can still be persisted' do let(:relation_key) { 'merge_requests' } let(:relation_definition) { { 'approvals' => {} } } @@ -91,6 +99,10 @@ expect(project.merge_requests.first.approvals.count).to eq(2) expect(project.merge_requests.first.approvals.first.persisted?).to eq(true) end + + it 'yields the saved records' do + expect { |b| saver.execute(&b) }.to yield_successive_args([relation_object], [approval_1, approval_2]) + end end context 'when importable is group' do diff --git a/spec/lib/import/bulk_imports/ephemeral_data_spec.rb b/spec/lib/import/bulk_imports/ephemeral_data_spec.rb new file mode 100644 index 00000000000000..9cccd1df1054ae --- /dev/null +++ b/spec/lib/import/bulk_imports/ephemeral_data_spec.rb @@ -0,0 +1,43 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Import::BulkImports::EphemeralData, :clean_gitlab_redis_shared_state, feature_category: :importers do + let(:ephemeral_data) { described_class.new(123) } + + describe '#enable_importer_user_mapping' do + it 'enables importer_user_mapping' do + ephemeral_data.enable_importer_user_mapping + + expect(ephemeral_data.importer_user_mapping_enabled?).to eq(true) + end + end + + describe '#importer_user_mapping_enabled?' do + context 'when importer_user_mapping is enabled' do + before do + ephemeral_data.enable_importer_user_mapping + end + + it 'returns true' do + expect(ephemeral_data.importer_user_mapping_enabled?).to eq(true) + end + end + + context 'when importer_user_mapping is not enabled' do + it 'returns false' do + expect(ephemeral_data.importer_user_mapping_enabled?).to eq(false) + end + end + + context 'when importer_user_mapping is enabled for a different bulk_import_id' do + before do + ephemeral_data.enable_importer_user_mapping + end + + it 'returns false' do + expect(described_class.new(456).importer_user_mapping_enabled?).to eq(false) + end + end + end +end diff --git a/spec/models/bulk_imports/configuration_spec.rb b/spec/models/bulk_imports/configuration_spec.rb index 1cbfef631ac3c7..54309bc2faa39d 100644 --- a/spec/models/bulk_imports/configuration_spec.rb +++ b/spec/models/bulk_imports/configuration_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe BulkImports::Configuration, type: :model do +RSpec.describe BulkImports::Configuration, type: :model, feature_category: :importers do describe 'associations' do it { is_expected.to belong_to(:bulk_import).required } end @@ -14,4 +14,12 @@ it { is_expected.to validate_presence_of(:url) } it { is_expected.to validate_presence_of(:access_token) } end + + describe '#source_hostname' do + let(:configuration) { described_class.new(url: 'http://example.com') } + + it 'returns the hostname of the URL' do + expect(configuration.source_hostname).to eq('example.com') + end + end end diff --git a/spec/models/import/source_user_spec.rb b/spec/models/import/source_user_spec.rb index 8fd359d714c307..1c8e09b423bfd3 100644 --- a/spec/models/import/source_user_spec.rb +++ b/spec/models/import/source_user_spec.rb @@ -215,10 +215,10 @@ end end - describe '#accepted_reassign_to_user' do + describe '#assignee' do let_it_be(:source_user) { build(:import_source_user, :with_reassign_to_user) } - subject(:accepted_reassign_to_user) { source_user.accepted_reassign_to_user } + subject(:assignee) { source_user.assignee } before do allow(source_user).to receive(:accepted_status?).and_return(accepted) @@ -233,7 +233,7 @@ context 'when not accepted' do let(:accepted) { false } - it { is_expected.to be_nil } + it { is_expected.to eq(source_user.placeholder_user) } end end diff --git a/spec/services/bulk_imports/create_service_spec.rb b/spec/services/bulk_imports/create_service_spec.rb index 141f20eb03d80a..a8fb35243a17bf 100644 --- a/spec/services/bulk_imports/create_service_spec.rb +++ b/spec/services/bulk_imports/create_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe BulkImports::CreateService, feature_category: :importers do +RSpec.describe BulkImports::CreateService, :clean_gitlab_redis_shared_state, feature_category: :importers do include GraphqlHelpers let(:user) { create(:user) } @@ -369,6 +369,36 @@ ) end + it 'enables importer_user_mapping' do + subject.execute + + expect(Import::BulkImports::EphemeralData.new(BulkImport.last.id).importer_user_mapping_enabled?).to eq(true) + end + + context 'when importer_user_mapping feature flag is disable' do + before do + stub_feature_flags(importer_user_mapping: false) + end + + it 'does not enable importer_user_mapping' do + subject.execute + + expect(Import::BulkImports::EphemeralData.new(BulkImport.last.id).importer_user_mapping_enabled?).to eq(false) + end + end + + context 'when bulk_import_importer_user_mapping feature flag is disable' do + before do + stub_feature_flags(bulk_import_importer_user_mapping: false) + end + + it 'does not enable importer_user_mapping' do + subject.execute + + expect(Import::BulkImports::EphemeralData.new(BulkImport.last.id).importer_user_mapping_enabled?).to eq(false) + end + end + context 'on the same instance' do before do allow(Settings.gitlab).to receive(:base_url).and_return('http://gitlab.example') -- GitLab From 0a74cbf1f7fe20229309d21a3612ce7d3de657bd Mon Sep 17 00:00:00 2001 From: Rodrigo Tomonari Date: Fri, 26 Jul 2024 18:23:27 -0300 Subject: [PATCH 03/14] More refactoring and specs --- app/models/approval.rb | 2 +- app/models/award_emoji.rb | 2 +- app/models/ci/pipeline.rb | 2 +- app/models/ci/pipeline_schedule.rb | 2 +- app/models/commit_status.rb | 2 +- ...ences.rb => has_source_user_references.rb} | 4 +- app/models/concerns/issuable.rb | 2 +- app/models/deployment.rb | 2 +- app/models/event.rb | 2 +- app/models/issue_assignee.rb | 1 + app/models/list.rb | 2 +- app/models/merge_request.rb | 2 +- app/models/merge_request/metrics.rb | 2 +- app/models/merge_request_assignee.rb | 2 +- app/models/merge_request_reviewer.rb | 2 +- app/models/note.rb | 2 +- app/models/release.rb | 2 +- app/models/resource_event.rb | 2 +- app/models/resource_label_event.rb | 2 +- app/models/snippet.rb | 2 +- app/models/timelog.rb | 2 +- .../placeholder_references/push_service.rb | 11 +- .../approval_group_rules_user.rb | 2 +- ee/app/models/board_assignee.rb | 2 +- lib/bulk_imports/ndjson_pipeline.rb | 22 +-- lib/gitlab/import/source_user_mapper.rb | 10 +- .../import_export/base/relation_factory.rb | 4 +- .../bulk_imports/source_users_mapper.rb | 46 ++--- spec/lib/bulk_imports/ndjson_pipeline_spec.rb | 176 ++++++++++++++++-- spec/lib/gitlab/cache/import/caching_spec.rb | 34 +++- .../gitlab/import/source_user_mapper_spec.rb | 147 +++++---------- .../bulk_imports/source_users_mapper_spec.rb | 50 +++++ .../push_service_spec.rb | 14 ++ 33 files changed, 367 insertions(+), 194 deletions(-) rename app/models/concerns/import/{has_placeholder_user_references.rb => has_source_user_references.rb} (51%) create mode 100644 spec/lib/import/bulk_imports/source_users_mapper_spec.rb diff --git a/app/models/approval.rb b/app/models/approval.rb index e771e3a23b5c33..87accfd660679c 100644 --- a/app/models/approval.rb +++ b/app/models/approval.rb @@ -3,7 +3,7 @@ class Approval < ApplicationRecord include CreatedAtFilterable include Importable - include Import::HasPlaceholderUserReferences + include Import::HasSourceUserReferences include ShaAttribute belongs_to :user diff --git a/app/models/award_emoji.rb b/app/models/award_emoji.rb index d9b4513685dba8..c5d1901a3a2a53 100644 --- a/app/models/award_emoji.rb +++ b/app/models/award_emoji.rb @@ -7,7 +7,7 @@ class AwardEmoji < ApplicationRecord include Participable include GhostUser include Importable - include Import::HasPlaceholderUserReferences + include Import::HasSourceUserReferences belongs_to :awardable, polymorphic: true # rubocop:disable Cop/PolymorphicAssociations belongs_to :user diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb index 82c6906c75504d..11b3a180882e2c 100644 --- a/app/models/ci/pipeline.rb +++ b/app/models/ci/pipeline.rb @@ -6,7 +6,7 @@ class Pipeline < Ci::ApplicationRecord include Ci::HasStatus include Ci::HasCompletionReason include Importable - include Import::HasPlaceholderUserReferences + include Import::HasSourceUserReferences include AfterCommitQueue include Presentable include Gitlab::Allowable diff --git a/app/models/ci/pipeline_schedule.rb b/app/models/ci/pipeline_schedule.rb index 4cc0e77c2a3ad9..3771ed0282edaf 100644 --- a/app/models/ci/pipeline_schedule.rb +++ b/app/models/ci/pipeline_schedule.rb @@ -4,7 +4,7 @@ module Ci class PipelineSchedule < Ci::ApplicationRecord extend ::Gitlab::Utils::Override include Importable - include Import::HasPlaceholderUserReferences + include Import::HasSourceUserReferences include StripAttribute include CronSchedulable include Limitable diff --git a/app/models/commit_status.rb b/app/models/commit_status.rb index de7eb587387223..699f8c9c60bf09 100644 --- a/app/models/commit_status.rb +++ b/app/models/commit_status.rb @@ -4,7 +4,7 @@ class CommitStatus < Ci::ApplicationRecord include Ci::Partitionable include Ci::HasStatus include Importable - include Import::HasPlaceholderUserReferences + include Import::HasSourceUserReferences include AfterCommitQueue include Presentable include BulkInsertableAssociations diff --git a/app/models/concerns/import/has_placeholder_user_references.rb b/app/models/concerns/import/has_source_user_references.rb similarity index 51% rename from app/models/concerns/import/has_placeholder_user_references.rb rename to app/models/concerns/import/has_source_user_references.rb index 416c795be7ab5b..667e89f09b8b7a 100644 --- a/app/models/concerns/import/has_placeholder_user_references.rb +++ b/app/models/concerns/import/has_source_user_references.rb @@ -1,9 +1,9 @@ # frozen_string_literal: true module Import - module HasPlaceholderUserReferences + module HasSourceUserReferences extend ActiveSupport::Concern - attr_accessor :placeholder_user_references + attr_accessor :source_user_references end end diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb index 7dc0ee50e9c7d1..d8e4d32b912049 100644 --- a/app/models/concerns/issuable.rb +++ b/app/models/concerns/issuable.rb @@ -19,7 +19,7 @@ module Issuable include Awardable include Taskable include Importable - include Import::HasPlaceholderUserReferences + include Import::HasSourceUserReferences include Transitionable include Editable include AfterCommitQueue diff --git a/app/models/deployment.rb b/app/models/deployment.rb index eb5558c5adee3c..0c5ff78ac7c3e0 100644 --- a/app/models/deployment.rb +++ b/app/models/deployment.rb @@ -6,7 +6,7 @@ class Deployment < ApplicationRecord include AfterCommitQueue include UpdatedAtFilterable include Importable - include Import::HasPlaceholderUserReferences + include Import::HasSourceUserReferences include Gitlab::Utils::StrongMemoize include FastDestroyAll include EachBatch diff --git a/app/models/event.rb b/app/models/event.rb index b9b3d17dbc6510..7aeef9baf1718d 100644 --- a/app/models/event.rb +++ b/app/models/event.rb @@ -11,7 +11,7 @@ class Event < ApplicationRecord include ShaAttribute include EachBatch include Import::HasImportSource - include Import::HasPlaceholderUserReferences + include Import::HasSourceUserReferences ACTIONS = HashWithIndifferentAccess.new( created: 1, diff --git a/app/models/issue_assignee.rb b/app/models/issue_assignee.rb index 088ccb6a61dc80..b6a7c5ba5d535c 100644 --- a/app/models/issue_assignee.rb +++ b/app/models/issue_assignee.rb @@ -1,6 +1,7 @@ # frozen_string_literal: true class IssueAssignee < ApplicationRecord + include Import::HasSourceUserReferences include EachBatch extend SuppressCompositePrimaryKeyWarning diff --git a/app/models/list.rb b/app/models/list.rb index 185cb4dd8881cf..fc42897e0ab5ca 100644 --- a/app/models/list.rb +++ b/app/models/list.rb @@ -3,7 +3,7 @@ class List < ApplicationRecord include Boards::Listable include Importable - include Import::HasPlaceholderUserReferences + include Import::HasSourceUserReferences belongs_to :board belongs_to :label diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index f5c2991542006e..8df5bac5875768 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -26,7 +26,7 @@ class MergeRequest < ApplicationRecord include IdInOrdered include Todoable include Spammable - include Import::HasPlaceholderUserReferences + include Import::HasSourceUserReferences ignore_columns :head_pipeline_id_convert_to_bigint, remove_with: '17.4', remove_after: '2024-08-14' diff --git a/app/models/merge_request/metrics.rb b/app/models/merge_request/metrics.rb index 96d168582f8a7f..673dd216c0bc7d 100644 --- a/app/models/merge_request/metrics.rb +++ b/app/models/merge_request/metrics.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true class MergeRequest::Metrics < ApplicationRecord - include Import::HasPlaceholderUserReferences + include Import::HasSourceUserReferences include IgnorableColumns ignore_columns :pipeline_id_convert_to_bigint, remove_with: '17.4', remove_after: '2024-08-14' diff --git a/app/models/merge_request_assignee.rb b/app/models/merge_request_assignee.rb index 231e5160fe2ec6..fc9495b7440db6 100644 --- a/app/models/merge_request_assignee.rb +++ b/app/models/merge_request_assignee.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true class MergeRequestAssignee < ApplicationRecord - include Import::HasPlaceholderUserReferences + include Import::HasSourceUserReferences include EachBatch belongs_to :merge_request, touch: true diff --git a/app/models/merge_request_reviewer.rb b/app/models/merge_request_reviewer.rb index 362d3a2710d28a..a75763ac07ce82 100644 --- a/app/models/merge_request_reviewer.rb +++ b/app/models/merge_request_reviewer.rb @@ -2,7 +2,7 @@ class MergeRequestReviewer < ApplicationRecord include MergeRequestReviewerState - include Import::HasPlaceholderUserReferences + include Import::HasSourceUserReferences include BulkInsertSafe # must be included _last_ i.e. after any other concerns belongs_to :merge_request diff --git a/app/models/note.rb b/app/models/note.rb index aa8d9f08a777ed..976218987f0db3 100644 --- a/app/models/note.rb +++ b/app/models/note.rb @@ -16,7 +16,7 @@ class Note < ApplicationRecord include Awardable include Importable include Import::HasImportSource - include Import::HasPlaceholderUserReferences + include Import::HasSourceUserReferences include FasterCacheKeys include Redactable include CacheMarkdownField diff --git a/app/models/release.rb b/app/models/release.rb index 5854bc71f12ee2..6a32817ad5f5a2 100644 --- a/app/models/release.rb +++ b/app/models/release.rb @@ -4,7 +4,7 @@ class Release < ApplicationRecord include Presentable include CacheMarkdownField include Importable - include Import::HasPlaceholderUserReferences + include Import::HasSourceUserReferences include Gitlab::Utils::StrongMemoize include EachBatch include FromUnion diff --git a/app/models/resource_event.rb b/app/models/resource_event.rb index 4b947a9cbd986e..f3c04a708d4547 100644 --- a/app/models/resource_event.rb +++ b/app/models/resource_event.rb @@ -3,7 +3,7 @@ class ResourceEvent < ApplicationRecord include Gitlab::Utils::StrongMemoize include Importable - include Import::HasPlaceholderUserReferences + include Import::HasSourceUserReferences include IssueResourceEvent include WorkItemResourceEvent diff --git a/app/models/resource_label_event.rb b/app/models/resource_label_event.rb index d1b28e5c0c65b0..6d54a01ab3ca9a 100644 --- a/app/models/resource_label_event.rb +++ b/app/models/resource_label_event.rb @@ -4,7 +4,7 @@ class ResourceLabelEvent < ResourceEvent include CacheMarkdownField include MergeRequestResourceEvent include Import::HasImportSource - include Import::HasPlaceholderUserReferences + include Import::HasSourceUserReferences include FromUnion cache_markdown_field :reference diff --git a/app/models/snippet.rb b/app/models/snippet.rb index 831ea8a9801b6c..d00504f8e468c1 100644 --- a/app/models/snippet.rb +++ b/app/models/snippet.rb @@ -20,7 +20,7 @@ class Snippet < ApplicationRecord include CreatedAtFilterable include EachBatch include Import::HasImportSource - include Import::HasPlaceholderUserReferences + include Import::HasSourceUserReferences include SafelyChangeColumnDefault columns_changing_default :organization_id diff --git a/app/models/timelog.rb b/app/models/timelog.rb index 249bd7bcacf626..5b8818afec07e4 100644 --- a/app/models/timelog.rb +++ b/app/models/timelog.rb @@ -6,7 +6,7 @@ class Timelog < ApplicationRecord MAX_TOTAL_TIME_SPENT = 31557600.seconds.to_i # a year include Importable - include Import::HasPlaceholderUserReferences + include Import::HasSourceUserReferences include Sortable before_save :set_project diff --git a/app/services/import/placeholder_references/push_service.rb b/app/services/import/placeholder_references/push_service.rb index 5f1c119c9a373a..77acf8bccc513c 100644 --- a/app/services/import/placeholder_references/push_service.rb +++ b/app/services/import/placeholder_references/push_service.rb @@ -4,8 +4,15 @@ module Import module PlaceholderReferences class PushService < BaseService class << self - def from_record(import_source:, import_uid:, source_user:, record:, user_reference_column:, composite_key: nil) - numeric_key = record.id if composite_key.nil? && record.id.is_a?(Integer) + def from_record(import_source:, import_uid:, source_user:, record:, user_reference_column:) + if record.is_a?(IssueAssignee) + composite_key = { + 'issue_id' => record.issue_id, + 'user_id' => record.user_id + } + else + numeric_key = record.id + end new( import_source: import_source, diff --git a/ee/app/models/approval_rules/approval_group_rules_user.rb b/ee/app/models/approval_rules/approval_group_rules_user.rb index a0e02dae25bc6a..3e83b53415534a 100644 --- a/ee/app/models/approval_rules/approval_group_rules_user.rb +++ b/ee/app/models/approval_rules/approval_group_rules_user.rb @@ -5,7 +5,7 @@ module ApprovalRules class ApprovalGroupRulesUser < ApplicationRecord include ApprovalRuleUserLike - include Import::HasPlaceholderUserReferences + include Import::HasSourceUserReferences belongs_to :user belongs_to :approval_group_rule, class_name: 'ApprovalRules::ApprovalGroupRule' diff --git a/ee/app/models/board_assignee.rb b/ee/app/models/board_assignee.rb index 73f27713a1294b..ab7c14527fa047 100644 --- a/ee/app/models/board_assignee.rb +++ b/ee/app/models/board_assignee.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true class BoardAssignee < ApplicationRecord - include Import::HasPlaceholderUserReferences + include Import::HasSourceUserReferences belongs_to :board belongs_to :assignee, class_name: 'User' diff --git a/lib/bulk_imports/ndjson_pipeline.rb b/lib/bulk_imports/ndjson_pipeline.rb index f2ad0ccc44b62f..add220ad0d37b5 100644 --- a/lib/bulk_imports/ndjson_pipeline.rb +++ b/lib/bulk_imports/ndjson_pipeline.rb @@ -47,7 +47,7 @@ def load(_context, object) ) saver.execute do |persisted_objects| - push_placeholder_references(persisted_objects) + push_placeholder_references(persisted_objects) if context.importer_user_mapping_enabled? end capture_invalid_subrelations(saver.invalid_subrelations) @@ -87,6 +87,8 @@ def deep_transform_relation!(relation_hash, relation_key, relation_definition, & end end + # Create missing Import::SourceUser objects during the transformation step, as no data + # related to the relation has been persisted yet. create_import_source_users(relation_key, relation_hash) if context.importer_user_mapping_enabled? yield(relation_key, relation_hash) @@ -159,14 +161,12 @@ def source_user_mapper ) end - # Creates an Import::SourceUser for each source_user_identifier found in the relation_hash and associate - # it with the ImportUser. + # Creates an Import::SourceUser objects for each source_user_identifier + # found in the relation_hash and associate it with the ImportUser. def create_import_source_users(_relation_key, relation_hash) relation_factory::USER_REFERENCES.each do |reference| next unless relation_hash[reference] - next if source_user_mapper.source_user_exists?(relation_hash[reference]) - source_user_mapper.find_or_create_source_user( source_name: nil, source_username: nil, @@ -177,18 +177,18 @@ def create_import_source_users(_relation_key, relation_hash) end def push_placeholder_references(persisted_objects) - persisted_objects.each do |persisted_object| - next unless persisted_object.respond_to?(:placeholder_user_references) + persisted_objects.each do |object| + next unless object.respond_to?(:source_user_references) - persisted_object.placeholder_user_references.each do |attribute, source_user_identifier| + object.source_user_references.each do |attribute, source_user_identifier| source_user = source_user_mapper.find_source_user(source_user_identifier) ::Import::PlaceholderReferences::PushService.from_record( import_source: ::Import::SOURCE_DIRECT_TRANSFER, import_uid: context.bulk_import_id, - record: persisted_object, - user_reference_column: attribute.to_sym, - source_user: source_user + record: object, + source_user: source_user, + user_reference_column: attribute.to_sym ).execute end end diff --git a/lib/gitlab/import/source_user_mapper.rb b/lib/gitlab/import/source_user_mapper.rb index ebe01b36cc74f8..6c39bb7254648b 100644 --- a/lib/gitlab/import/source_user_mapper.rb +++ b/lib/gitlab/import/source_user_mapper.rb @@ -33,9 +33,9 @@ def find_or_create_source_user(source_name:, source_username:, source_user_ident ) end - def source_user_exists?(source_user_identifier) - find_source_user(source_user_identifier).present? - end + private + + attr_reader :namespace, :import_type, :source_hostname def create_source_user(source_name:, source_username:, source_user_identifier:, use_import_user: false) in_lock(lock_key(source_user_identifier), sleep_sec: 2.seconds) do |retried| @@ -48,10 +48,6 @@ def create_source_user(source_name:, source_username:, source_user_identifier:, end end - private - - attr_reader :namespace, :import_type, :source_hostname - def create_source_user_mapping(source_name, source_username, source_user_identifier, use_import_user) ::Import::SourceUser.transaction do import_source_user = ::Import::SourceUser.new( diff --git a/lib/gitlab/import_export/base/relation_factory.rb b/lib/gitlab/import_export/base/relation_factory.rb index c41f664b723590..fbf5a5f6c53904 100644 --- a/lib/gitlab/import_export/base/relation_factory.rb +++ b/lib/gitlab/import_export/base/relation_factory.rb @@ -192,8 +192,8 @@ def imported_object existing_or_new_object.imported_from = @import_source end - if existing_or_new_object.respond_to?(:placeholder_user_references) - existing_or_new_object.placeholder_user_references = @original_user + if existing_or_new_object.respond_to?(:source_user_references) + existing_or_new_object.source_user_references = @original_user end existing_or_new_object diff --git a/lib/import/bulk_imports/source_users_mapper.rb b/lib/import/bulk_imports/source_users_mapper.rb index 7598c66f1899cf..2ce91b99acb59f 100644 --- a/lib/import/bulk_imports/source_users_mapper.rb +++ b/lib/import/bulk_imports/source_users_mapper.rb @@ -1,28 +1,28 @@ # frozen_string_literal: true -# This class is meant to be used by Gitlab::ImportExport::Base::RelationFactory when mapping users +# This class is used by Gitlab::ImportExport::Base::RelationFactory when mapping +# users from the source to the destination. # -# It's assumed that source users for each source_user_identifier have been created before executing the RelationFactory. +# It is required that Import::SourceUser objects with source_user_identifier exists +# before executing Gitlab::ImportExport::Base::RelationFactory. The latter class was +# not modified to create Import::SourceUser if it does not exist since the class is +# also used by file based imports. module Import module BulkImports class SourceUsersMapper include Gitlab::Utils::StrongMemoize - # Gitlab::ImportExport::Base::RelationFactory expects SourceUsersMapper#map to return a hash that is why - # a mocked hash is used here. + # Gitlab::ImportExport::Base::RelationFactory expects member_mapper#map to + # return an object that responds to []. For the others mappers a hash is + # returned. In this case SourceUsersMapper#map returns a class that responds + # to []. class MockedHash - def initialize(source_user_mapper, import_user) + def initialize(source_user_mapper) @source_user_mapper = source_user_mapper - @import_user = import_user - @memory_cache = {} end def [](user_identifier) - find_source_user(user_identifier) - end - - def find_source_user(user_identifier) - @memory_cache[user_identifier] ||= @source_user_mapper.find_source_user(user_identifier).assignee.id + @source_user_mapper.find_source_user(user_identifier).assignee.id end end @@ -31,36 +31,26 @@ def initialize(context:) end def map - mocked_hash + @map ||= MockedHash.new(source_user_mapper) end - def include?(user_identifier) - mocked_hash.find_source_user(user_identifier) + # Import::SourceUser with the respective user_identifier will always since + # they are created beforehand. + def include?(_user_identifier) + true end private attr_reader :context - def root_ancestor - @root_ancestor ||= context.portable.root_ancestor - end - - def import_user - @import_user ||= Gitlab::Import::ImportUserCreator.new(portable: root_ancestor).execute - end - def source_user_mapper @source_user_mapper ||= Gitlab::Import::SourceUserMapper.new( - namespace: root_ancestor, + namespace: context.portable.root_ancestor, import_type: SOURCE_DIRECT_TRANSFER, source_hostname: context.configuration.source_hostname ) end - - def mocked_hash - @mocked_hash ||= MockedHash.new(source_user_mapper, import_user) - end end end end diff --git a/spec/lib/bulk_imports/ndjson_pipeline_spec.rb b/spec/lib/bulk_imports/ndjson_pipeline_spec.rb index 56ebb885f5e021..15ec03b4a5e441 100644 --- a/spec/lib/bulk_imports/ndjson_pipeline_spec.rb +++ b/spec/lib/bulk_imports/ndjson_pipeline_spec.rb @@ -6,8 +6,28 @@ let_it_be(:group) { create(:group) } let_it_be(:project) { create(:project) } let_it_be(:user) { create(:user) } - let(:tracker) { instance_double(BulkImports::Tracker, bulk_import_entity_id: 1) } - let(:context) { instance_double(BulkImports::Pipeline::Context, tracker: tracker, extra: { batch_number: 1 }) } + let_it_be(:bulk_import) { create(:bulk_import, :with_configuration, user: user) } + let_it_be(:entity) { create(:bulk_import_entity, bulk_import: bulk_import, group: group) } + let_it_be(:tracker) { create(:bulk_import_tracker, entity: entity) } + let_it_be(:context) { BulkImports::Pipeline::Context.new(tracker, batch_number: 1) } + + let_it_be(:source_user_1) do + create(:import_source_user, + import_type: ::Import::SOURCE_DIRECT_TRANSFER, + namespace: group, + source_user_identifier: 101, + source_hostname: bulk_import.configuration.source_hostname + ) + end + + let_it_be(:source_user_2) do + create(:import_source_user, + import_type: ::Import::SOURCE_DIRECT_TRANSFER, + namespace: group, + source_user_identifier: 102, + source_hostname: bulk_import.configuration.source_hostname + ) + end let(:klass) do Class.new do @@ -39,10 +59,21 @@ def initialize(portable, user, context) it 'saves completed entry in cache' do subject.save_processed_entry("entry", 10) - expected_cache_key = "ndjson_pipeline_class/1/1" + expected_cache_key = "ndjson_pipeline_class/#{entity.id}/1" expect(Gitlab::Cache::Import::Caching.read(expected_cache_key)).to eq("10") end + context "when is not a batched pipeline" do + let(:context) { BulkImports::Pipeline::Context.new(tracker) } + + it 'saves completed entry using batch number 0' do + subject.save_processed_entry("entry", 10) + + expected_cache_key = "ndjson_pipeline_class/#{entity.id}/0" + expect(Gitlab::Cache::Import::Caching.read(expected_cache_key)).to eq("10") + end + end + it 'identifies completed entries' do subject.save_processed_entry("entry", 10) @@ -61,6 +92,60 @@ def initialize(portable, user, context) expect(transformed[:relation_key]).to eq('test') end + context 'when importer_user_mapping is enabled' do + before do + allow(context).to receive(:importer_user_mapping_enabled?).and_return(true) + end + + it 'creates Import::SourceUser objects for each user reference in relation hash' do + relation_hash = { + "author_id" => 101, + "updated_by_id" => nil, + "project_id" => 1, + "title" => "Imported MR", + "notes" => + [ + { + "note" => "Issue note", + "author_id" => 102, + "award_emoji" => [ + { + "name" => "clapper", "user_id" => 105 + }, + { + "name" => "clapper", "user_id" => 103 + } + ] + } + ], + "merge_request_assignees" => [ + { + "user_id" => 103 + }, + { + "user_id" => 104 + } + ], + "metrics" => { + "merged_by_id" => 105 + } + } + + relation_definition = { + "approvals" => {}, + "metrics" => {}, + "award_emoji" => {}, + "merge_request_assignees" => {}, + "notes" => { "author" => {}, "award_emoji" => {} } + } + + expect { subject.deep_transform_relation!(relation_hash, 'test', relation_definition) { |a, _b| a } } + .to change { Import::SourceUser.count }.by(5) + + expect(Import::SourceUser.pluck(:source_user_identifier)).to match_array(%w[101 102 103 104 105]) + end + end + context 'when subrelations is an array' do it 'transforms each element of the array' do relation_hash = { @@ -121,16 +206,16 @@ def initialize(portable, user, context) end describe '#transform' do - it 'calls relation factory' do - hash = { key: :value } - data = [hash, 1] - user = double - config = double(relation_excluded_keys: nil, top_relation_tree: []) - import_double = instance_double(BulkImport, id: 1) - entity_double = instance_double(BulkImports::Entity, id: 2) - context = double(portable: group, current_user: user, import_export_config: config, bulk_import: import_double, entity: entity_double) + let(:hash) { { key: :value } } + let(:data) { [hash, 1] } + let(:config) { double(relation_excluded_keys: nil, top_relation_tree: []) } + + before do allow(subject).to receive(:import_export_config).and_return(config) allow(subject).to receive(:context).and_return(context) + end + + it 'calls relation factory' do relation_object = double expect(Gitlab::ImportExport::Group::RelationFactory) @@ -152,6 +237,30 @@ def initialize(portable, user, context) subject.transform(context, data) end + context 'when importer_user_mapping is enabled' do + before do + allow(context).to receive(:importer_user_mapping_enabled?).and_return(true) + end + + it 'calls relation factory with SourceUsersMapper' do + expect(Gitlab::ImportExport::Group::RelationFactory) + .to receive(:create) + .with( + relation_index: 1, + relation_sym: :test, + relation_hash: hash, + importable: group, + members_mapper: instance_of(Import::BulkImports::SourceUsersMapper), + object_builder: Gitlab::ImportExport::Group::ObjectBuilder, + user: user, + excluded_keys: nil, + import_source: Import::SOURCE_DIRECT_TRANSFER + ).and_return(double(assign_attributes: nil)) + + subject.transform(context, data) + end + end + context 'when data is nil' do before do expect(Gitlab::ImportExport::Group::RelationFactory).not_to receive(:create) @@ -185,12 +294,6 @@ def initialize(portable, user, context) context 'when object is invalid' do it 'captures invalid subrelations' do - entity = create(:bulk_import_entity, group: group) - tracker = create(:bulk_import_tracker, entity: entity) - context = BulkImports::Pipeline::Context.new(tracker) - - allow(subject).to receive(:context).and_return(context) - object = group.labels.new(priorities: [LabelPriority.new]) object.validate @@ -236,6 +339,45 @@ def initialize(portable, user, context) expect(subject.load(nil, nil)).to be_nil end end + + context 'when importer_user_mapping is enabled' do + before do + allow(context).to receive(:importer_user_mapping_enabled?).and_return(true) + end + + it 'pushes a placeholder reference for every user referenced the persisted object has' do + merge_request = MergeRequest.new( + author: source_user_1.assignee, + updated_by: source_user_2.assignee, + source_user_references: { 'author_id' => '101', 'updated_by_id' => '102' } + ) + + allow_next_instance_of(Gitlab::ImportExport::Base::RelationObjectSaver) do |saver| + allow(saver).to receive(:execute) + .and_yield([merge_request]) + end + + expect(Import::PlaceholderReferences::PushService).to receive(:from_record).with( + import_source: ::Import::SOURCE_DIRECT_TRANSFER, + import_uid: context.bulk_import_id, + record: merge_request, + user_reference_column: :author_id, + source_user: source_user_1, + composite_key: nil + ).and_call_original + + expect(::Import::PlaceholderReferences::PushService).to receive(:from_record).with( + import_source: ::Import::SOURCE_DIRECT_TRANSFER, + import_uid: context.bulk_import_id, + record: merge_request, + user_reference_column: :updated_by_id, + source_user: source_user_2, + composite_key: nil + ).and_call_original + + subject.load(nil, merge_request) + end + end end describe '#relation_class' do diff --git a/spec/lib/gitlab/cache/import/caching_spec.rb b/spec/lib/gitlab/cache/import/caching_spec.rb index 948961805838f0..c84721eb7cb70a 100644 --- a/spec/lib/gitlab/cache/import/caching_spec.rb +++ b/spec/lib/gitlab/cache/import/caching_spec.rb @@ -221,13 +221,37 @@ end end + describe '.value_from_hash' do + it 'returns nil when field was not set' do + expect(described_class.value_from_hash('foo', 'bar')).to eq(nil) + end + + it 'returns the value of the field' do + described_class.hash_add('foo', 'bar', 1) + + expect(described_class.value_from_hash('foo', 'bar')).to eq('1') + end + + it 'refreshes the cache key if a value is present' do + described_class.hash_add('foo', 'bar', 1) + + redis = double(:redis) + + expect(redis).to receive(:hget).with(/foo/, 'bar').and_return('1') + expect(redis).to receive(:expire).with(/foo/, described_class::TIMEOUT) + expect(Gitlab::Redis::SharedState).to receive(:with).twice.and_yield(redis) + + described_class.value_from_hash('foo', 'bar') + end + end + describe '.hash_increment' do it 'increments a value in a hash' do described_class.hash_increment('foo', 'field', 1) described_class.hash_increment('foo', 'field', 5) key = described_class.cache_key_for('foo') - values = Gitlab::Redis::Cache.with { |r| r.hgetall(key) } + values = Gitlab::Redis::SharedState.with { |r| r.hgetall(key) } expect(values).to eq({ 'field' => '6' }) end @@ -237,7 +261,7 @@ described_class.hash_increment('another-foo', 'another-field', 'not-an-integer') key = described_class.cache_key_for('foo') - values = Gitlab::Redis::Cache.with { |r| r.hgetall(key) } + values = Gitlab::Redis::SharedState.with { |r| r.hgetall(key) } expect(values).to eq({}) end @@ -248,7 +272,7 @@ described_class.hash_increment('another-foo', 'another-field', -5) key = described_class.cache_key_for('foo') - values = Gitlab::Redis::Cache.with { |r| r.hgetall(key) } + values = Gitlab::Redis::SharedState.with { |r| r.hgetall(key) } expect(values).to eq({}) end @@ -315,7 +339,7 @@ described_class.list_add('foo', 20) key = described_class.cache_key_for('foo') - values = Gitlab::Redis::Cache.with { |r| r.lrange(key, 0, -1) } + values = Gitlab::Redis::SharedState.with { |r| r.lrange(key, 0, -1) } expect(values).to eq(%w[10 20]) end @@ -328,7 +352,7 @@ described_class.list_add('foo', 40, limit: 3) key = described_class.cache_key_for('foo') - values = Gitlab::Redis::Cache.with { |r| r.lrange(key, 0, -1) } + values = Gitlab::Redis::SharedState.with { |r| r.lrange(key, 0, -1) } expect(values).to eq(%w[20 30 40]) end diff --git a/spec/lib/gitlab/import/source_user_mapper_spec.rb b/spec/lib/gitlab/import/source_user_mapper_spec.rb index 001e3a47e0f3ed..53fdac301b757b 100644 --- a/spec/lib/gitlab/import/source_user_mapper_spec.rb +++ b/spec/lib/gitlab/import/source_user_mapper_spec.rb @@ -3,21 +3,32 @@ require 'spec_helper' RSpec.describe Gitlab::Import::SourceUserMapper, feature_category: :importers do - describe '#find_or_create_internal_user' do - let_it_be(:namespace) { create(:namespace) } + let_it_be(:namespace) { create(:namespace) } + let_it_be(:import_type) { 'github' } + let_it_be(:source_hostname) { 'github.com' } + + let_it_be(:existing_import_source_user) do + create( + :import_source_user, + namespace: namespace, + import_type: import_type, + source_hostname: source_hostname, + source_user_identifier: '101') + end + + let_it_be(:import_source_user_from_another_import) { create(:import_source_user) } - let_it_be(:import_type) { 'github' } - let_it_be(:source_hostname) { 'github.com' } - let_it_be(:source_name) { 'Pry Contributor' } - let_it_be(:source_username) { 'a_pry_contributor' } - let_it_be(:source_user_identifier) { '123456' } + describe '#find_or_create_source_user' do + let(:source_name) { 'Pry Contributor' } + let(:source_username) { 'a_pry_contributor' } + let(:source_user_identifier) { '123456' } - subject(:find_or_create_internal_user) do + subject(:find_or_create_source_user) do described_class.new( namespace: namespace, import_type: import_type, source_hostname: source_hostname - ).find_or_create_internal_user( + ).find_or_create_source_user( source_name: source_name, source_username: source_username, source_user_identifier: source_user_identifier @@ -26,7 +37,7 @@ shared_examples 'creates an import_source_user and a unique placeholder user' do it 'creates a import_source_user with an internal placeholder user' do - expect { find_or_create_internal_user }.to change { Import::SourceUser.count }.from(2).to(3) + expect { find_or_create_source_user }.to change { Import::SourceUser.count }.by(1) new_import_source_user = Import::SourceUser.last @@ -42,7 +53,7 @@ end it 'creates a new placeholder user with a unique email and username' do - expect { find_or_create_internal_user }.to change { User.where(user_type: :placeholder).count }.by(1) + expect { find_or_create_source_user }.to change { User.where(user_type: :placeholder).count }.by(1) new_placeholder_user = User.where(user_type: :placeholder).last @@ -54,28 +65,18 @@ shared_examples 'it does not create an import_source_user or placeholder user' do it 'does not create a import_source_user' do - expect { find_or_create_internal_user }.not_to change { Import::SourceUser.count } + expect { find_or_create_source_user }.not_to change { Import::SourceUser.count } end it 'does not create any internal users' do - expect { find_or_create_internal_user }.not_to change { User.count } + expect { find_or_create_source_user }.not_to change { User.count } end end context 'when the placeholder user limit has not been reached' do - let_it_be(:import_source_user_from_another_import) { create(:import_source_user) } - let_it_be(:different_source_user_from_same_import) do - create(:import_source_user, - namespace_id: namespace.id, - import_type: import_type, - source_hostname: source_hostname, - source_user_identifier: '999999' - ) - end - it_behaves_like 'creates an import_source_user and a unique placeholder user' - context 'when retried and another placeholder user is not created while waiting' do + context 'when retried and another source user is not created while waiting' do before do allow_next_instance_of(described_class) do |source_user_mapper| allow(source_user_mapper).to receive(:in_lock).and_yield(true) @@ -85,16 +86,7 @@ it_behaves_like 'creates an import_source_user and a unique placeholder user' end - context 'when retried and another placeholder user was made while waiting' do - let_it_be(:existing_import_source_user) do - create( - :import_source_user, - namespace: namespace, - import_type: import_type, - source_hostname: source_hostname, - source_user_identifier: '123456') - end - + context 'when retried and another source user was made while waiting' do before do allow_next_instance_of(described_class) do |source_user_mapper| allow(source_user_mapper).to receive(:in_lock).and_yield(true) @@ -103,87 +95,44 @@ allow(Import::SourceUser).to receive(:find_source_user).and_return(nil, existing_import_source_user) end - it 'returns the existing placeholder user' do - expect(find_or_create_internal_user).to eq(existing_import_source_user.placeholder_user) + it 'returns the existing source user' do + expect(find_or_create_source_user).to eq(existing_import_source_user) end it_behaves_like 'it does not create an import_source_user or placeholder user' end context 'and an import source user exists for current import source' do - let_it_be(:existing_import_source_user) do - create( - :import_source_user, - namespace: namespace, - import_type: import_type, - source_hostname: source_hostname, - source_user_identifier: '123456') - end + let(:source_user_identifier) { existing_import_source_user.source_user_identifier } - it 'returns the existing placeholder user' do - expect(find_or_create_internal_user).to eq(existing_import_source_user.placeholder_user) + it 'returns the existing source user' do + expect(find_or_create_source_user).to eq(existing_import_source_user) end it_behaves_like 'it does not create an import_source_user or placeholder user' end + end + end - context 'and the source user does not map to a placeholder user' do - let_it_be(:existing_import_source_user) do - create( - :import_source_user, - :completed, - :with_reassign_to_user, - placeholder_user: nil, - namespace: namespace, - import_type: import_type, - source_hostname: source_hostname, - source_user_identifier: '123456') - end - - it 'returns the existing reassigned user' do - expect(find_or_create_internal_user).to eq(existing_import_source_user.reassign_to_user) - end - - it_behaves_like 'it does not create an import_source_user or placeholder user' - end - - context 'and the source_user maps to a reassigned user' do - let_it_be(:existing_import_source_user) do - create( - :import_source_user, - :with_reassign_to_user, - namespace: namespace, - import_type: import_type, - source_hostname: source_hostname, - source_user_identifier: '123456') - end - - before do - allow_next_found_instance_of(Import::SourceUser) do |source_user| - allow(source_user).to receive(:accepted_status?).and_return(accepted) - end - end - - context 'when reassigned user has accepted the mapping' do - let(:accepted) { true } - - it_behaves_like 'it does not create an import_source_user or placeholder user' + describe '#find_source_user' do + let(:source_user_identifier) { existing_import_source_user.source_user_identifier } - it 'returns the existing reassign to user' do - expect(find_or_create_internal_user).to eq(existing_import_source_user.reassign_to_user) - end - end + subject(:find_source_user) do + described_class.new( + namespace: namespace, + import_type: import_type, + source_hostname: source_hostname + ).find_source_user(source_user_identifier) + end - context 'when reassigned user has not accepted the mapping' do - let(:accepted) { false } + it 'returns the existing source user' do + expect(find_source_user).to eq(existing_import_source_user) + end - it_behaves_like 'it does not create an import_source_user or placeholder user' + context 'when source user does not exist' do + let(:source_user_identifier) { '999999' } - it 'returns the existing placeholder user' do - expect(find_or_create_internal_user).to eq(existing_import_source_user.placeholder_user) - end - end - end + it { is_expected.to be_nil } end end end diff --git a/spec/lib/import/bulk_imports/source_users_mapper_spec.rb b/spec/lib/import/bulk_imports/source_users_mapper_spec.rb new file mode 100644 index 00000000000000..4c67b4374ee2a0 --- /dev/null +++ b/spec/lib/import/bulk_imports/source_users_mapper_spec.rb @@ -0,0 +1,50 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Import::BulkImports::SourceUsersMapper, feature_category: :importers do + let_it_be(:portable) { create(:group) } + let(:configuration) { instance_double(BulkImports::Configuration, source_hostname: 'example.com') } + let(:context) do + instance_double(BulkImports::Pipeline::Context, portable: portable, configuration: configuration) + end + + let_it_be(:import_source_user_1) do + create(:import_source_user, + namespace: portable, + import_type: Import::SOURCE_DIRECT_TRANSFER, + source_hostname: 'example.com', + source_user_identifier: 101 + ) + end + + let_it_be(:import_source_user_2) do + create(:import_source_user, + :completed, + namespace: portable, + import_type: Import::SOURCE_DIRECT_TRANSFER, + source_hostname: 'example.com', + source_user_identifier: 102 + ) + end + + subject(:mapper) { described_class.new(context: context) } + + describe '#map' do + it 'returns placeholder user id' do + expect(mapper.map['101']).to eq(import_source_user_1.placeholder_user.id) + end + + context 'when import source user have been reassigned to a real user' do + it 'returns real user user id' do + expect(mapper.map['102']).to eq(import_source_user_2.reassign_to_user.id) + end + end + end + + describe '#include?' do + it 'always returns true' do + expect(mapper.include?('any_identifier')).to eq(true) + end + end +end diff --git a/spec/services/import/placeholder_references/push_service_spec.rb b/spec/services/import/placeholder_references/push_service_spec.rb index b6de3af4337793..32821bf16b2664 100644 --- a/spec/services/import/placeholder_references/push_service_spec.rb +++ b/spec/services/import/placeholder_references/push_service_spec.rb @@ -78,6 +78,20 @@ expect(result.payload).to eq(serialized_reference: expected_result) expect(set).to contain_exactly(expected_result) end + + context 'when record is an IssueAssignee' do + let(:record) { IssueAssignee.new(issue_id: 1, user_id: 2) } + + it 'pushes a composition key' do + expected_result = [ + { issue_id: 1, user_id: 2 }, 'IssueAssignee', source_user.namespace_id, nil, source_user.id, 'author_id' + ].to_json + + expect(result).to be_success + expect(result.payload).to eq(serialized_reference: expected_result) + expect(set).to contain_exactly(expected_result) + end + end end def set -- GitLab From 642910f44c73aadb6d5a7d15739c1a777a6a88b7 Mon Sep 17 00:00:00 2001 From: Rodrigo Tomonari Date: Fri, 26 Jul 2024 19:30:35 -0300 Subject: [PATCH 04/14] Fix failed specs --- spec/lib/bulk_imports/ndjson_pipeline_spec.rb | 16 +++++++--------- .../lib/gitlab/import/source_user_mapper_spec.rb | 15 ++++++++++++++- 2 files changed, 21 insertions(+), 10 deletions(-) diff --git a/spec/lib/bulk_imports/ndjson_pipeline_spec.rb b/spec/lib/bulk_imports/ndjson_pipeline_spec.rb index 15ec03b4a5e441..1ca8b15ec34848 100644 --- a/spec/lib/bulk_imports/ndjson_pipeline_spec.rb +++ b/spec/lib/bulk_imports/ndjson_pipeline_spec.rb @@ -99,7 +99,7 @@ def initialize(portable, user, context) it 'creates Import::SourceUser objects for each user reference in relation hash' do relation_hash = { - "author_id" => 101, + "author_id" => source_user_1.source_user_identifier, "updated_by_id" => nil, "project_id" => 1, "title" => "Imported MR", @@ -107,13 +107,13 @@ def initialize(portable, user, context) [ { "note" => "Issue note", - "author_id" => 102, + "author_id" => 103, "award_emoji" => [ { "name" => "clapper", "user_id" => 105 }, { - "name" => "clapper", "user_id" => 103 + "name" => "clapper", "user_id" => source_user_2.source_user_identifier } ] } @@ -140,9 +140,9 @@ def initialize(portable, user, context) } expect { subject.deep_transform_relation!(relation_hash, 'test', relation_definition) { |a, _b| a } } - .to change { Import::SourceUser.count }.by(5) - + .to change { Import::SourceUser.count }.by(3).and change { User.count }.by(1) expect(Import::SourceUser.pluck(:source_user_identifier)).to match_array(%w[101 102 103 104 105]) + expect(User.last.import_user?).to eq(true) end end @@ -362,8 +362,7 @@ def initialize(portable, user, context) import_uid: context.bulk_import_id, record: merge_request, user_reference_column: :author_id, - source_user: source_user_1, - composite_key: nil + source_user: source_user_1 ).and_call_original expect(::Import::PlaceholderReferences::PushService).to receive(:from_record).with( @@ -371,8 +370,7 @@ def initialize(portable, user, context) import_uid: context.bulk_import_id, record: merge_request, user_reference_column: :updated_by_id, - source_user: source_user_2, - composite_key: nil + source_user: source_user_2 ).and_call_original subject.load(nil, merge_request) diff --git a/spec/lib/gitlab/import/source_user_mapper_spec.rb b/spec/lib/gitlab/import/source_user_mapper_spec.rb index 53fdac301b757b..a0c480327561a6 100644 --- a/spec/lib/gitlab/import/source_user_mapper_spec.rb +++ b/spec/lib/gitlab/import/source_user_mapper_spec.rb @@ -22,6 +22,7 @@ let(:source_name) { 'Pry Contributor' } let(:source_username) { 'a_pry_contributor' } let(:source_user_identifier) { '123456' } + let(:use_import_user) { false } subject(:find_or_create_source_user) do described_class.new( @@ -41,7 +42,7 @@ new_import_source_user = Import::SourceUser.last - expect(new_import_source_user.placeholder_user.user_type).to eq('placeholder') + expect(new_import_source_user.placeholder_user.placeholder?).to eq(true) expect(new_import_source_user.attributes).to include({ 'namespace_id' => namespace.id, 'import_type' => import_type, @@ -111,6 +112,18 @@ it_behaves_like 'it does not create an import_source_user or placeholder user' end + + context 'and use_import_user is true' do + let(:use_import_user) { true } + + it 'creates source user mapped to the ImportUser' do + expect { find_or_create_source_user }.to change { Import::SourceUser.count }.by(1) + + new_import_source_user = Import::SourceUser.last + + expect(new_import_source_user.placeholder_user.import_user?).to eq(true) + end + end end end -- GitLab From f5b12c7ce5eb57839972487c3552a2d7bc51afea Mon Sep 17 00:00:00 2001 From: Rodrigo Tomonari Date: Fri, 26 Jul 2024 21:16:37 -0300 Subject: [PATCH 05/14] Fix use_import_user spec --- spec/lib/gitlab/import/source_user_mapper_spec.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/spec/lib/gitlab/import/source_user_mapper_spec.rb b/spec/lib/gitlab/import/source_user_mapper_spec.rb index a0c480327561a6..6aa2ae69612211 100644 --- a/spec/lib/gitlab/import/source_user_mapper_spec.rb +++ b/spec/lib/gitlab/import/source_user_mapper_spec.rb @@ -32,7 +32,8 @@ ).find_or_create_source_user( source_name: source_name, source_username: source_username, - source_user_identifier: source_user_identifier + source_user_identifier: source_user_identifier, + use_import_user: use_import_user ) end -- GitLab From f06a85e87011dbeee06d82a668216320a86b7100 Mon Sep 17 00:00:00 2001 From: Rodrigo Tomonari Date: Mon, 29 Jul 2024 18:50:26 -0300 Subject: [PATCH 06/14] Refactor relation factory and add specs --- app/models/design_management/version.rb | 1 + app/models/import/source_user.rb | 2 +- .../common/pipelines/boards_pipeline_spec.rb | 10 +- .../member_attributes_transformer.rb | 6 +- lib/bulk_imports/ndjson_pipeline.rb | 38 +++++- .../base/relation_object_saver.rb | 20 +-- .../import_export/project/relation_factory.rb | 1 + .../bulk_imports/source_users_mapper.rb | 2 +- spec/lib/bulk_imports/ndjson_pipeline_spec.rb | 98 ++++++++++++-- .../pipelines/ci_pipelines_pipeline_spec.rb | 128 +++++++++++++++++- .../pipelines/issues_pipeline_spec.rb | 89 +++++++++++- .../pipelines/merge_requests_pipeline_spec.rb | 81 ++++++++++- .../pipeline_schedules_pipeline_spec.rb | 18 ++- .../protected_branches_pipeline_spec.rb | 11 +- .../snippets_repository_pipeline_spec.rb | 3 + .../base/relation_object_saver_spec.rb | 12 -- spec/models/import/source_user_spec.rb | 4 +- 17 files changed, 460 insertions(+), 64 deletions(-) diff --git a/app/models/design_management/version.rb b/app/models/design_management/version.rb index a0e830b94b0273..48f76eb1280977 100644 --- a/app/models/design_management/version.rb +++ b/app/models/design_management/version.rb @@ -3,6 +3,7 @@ module DesignManagement class Version < ApplicationRecord include Importable + include Import::HasSourceUserReferences include ShaAttribute include AfterCommitQueue include Gitlab::Utils::StrongMemoize diff --git a/app/models/import/source_user.rb b/app/models/import/source_user.rb index 119d3aec15f47e..570e881a21ab48 100644 --- a/app/models/import/source_user.rb +++ b/app/models/import/source_user.rb @@ -93,7 +93,7 @@ def sort_by_attribute(method) end end - def assignee + def mapped_user accepted_status? ? reassign_to_user : placeholder_user end diff --git a/ee/spec/lib/bulk_imports/common/pipelines/boards_pipeline_spec.rb b/ee/spec/lib/bulk_imports/common/pipelines/boards_pipeline_spec.rb index eed43adf7cd8d5..4a04807a0e350b 100644 --- a/ee/spec/lib/bulk_imports/common/pipelines/boards_pipeline_spec.rb +++ b/ee/spec/lib/bulk_imports/common/pipelines/boards_pipeline_spec.rb @@ -5,7 +5,7 @@ RSpec.describe BulkImports::Common::Pipelines::BoardsPipeline, feature_category: :importers do let_it_be(:user) { create(:user) } let_it_be(:group) { create(:group) } - let_it_be(:bulk_import) { create(:bulk_import, user: user) } + let_it_be(:bulk_import) { create(:bulk_import, :with_configuration, user: user) } let_it_be(:filepath) { 'ee/spec/fixtures/bulk_imports/gz/boards.ndjson.gz' } let_it_be(:entity) do @@ -22,25 +22,31 @@ let_it_be(:tracker) { create(:bulk_import_tracker, entity: entity) } let_it_be(:context) { BulkImports::Pipeline::Context.new(tracker) } + let(:importer_user_mapping_enabled) { false } + let(:tmpdir) { Dir.mktmpdir } before do stub_licensed_features(board_assignee_lists: true, board_milestone_lists: true) FileUtils.copy_file(filepath, File.join(tmpdir, 'boards.ndjson.gz')) group.add_owner(user) + + allow(context).to receive(:importer_user_mapping_enabled?).and_return(importer_user_mapping_enabled) end subject { described_class.new(context) } describe '#run' do - it 'imports group boards into destination group and removes tmpdir' do + before do allow(Dir).to receive(:mktmpdir).and_return(tmpdir) allow_next_instance_of(BulkImports::FileDownloadService) do |service| allow(service).to receive(:execute) end allow(subject).to receive(:set_source_objects_counter) + end + it 'imports group boards into destination group and removes tmpdir' do expect { subject.run }.to change(Board, :count).by(2) lists = group.boards.find_by(name: 'first board').lists diff --git a/lib/bulk_imports/common/transformers/member_attributes_transformer.rb b/lib/bulk_imports/common/transformers/member_attributes_transformer.rb index aa12cbfbffb678..f4848b95f415cc 100644 --- a/lib/bulk_imports/common/transformers/member_attributes_transformer.rb +++ b/lib/bulk_imports/common/transformers/member_attributes_transformer.rb @@ -36,12 +36,12 @@ def source_user_transformer(context, data) source_user = find_or_create_source_user(context, data) - # To not add user as a member is the source user is assigned to the ImportUser because we cannot + # To not add user as a member is the source user is mapped to the ImportUser because we cannot # create multiple members for the same user. - return if source_user.assignee.import_user? + return if source_user.mapped_user.import_user? { - user_id: source_user.assignee.id, + user_id: source_user.mapped_user.id, source_user: source_user, access_level: access_level, created_at: data['created_at'], diff --git a/lib/bulk_imports/ndjson_pipeline.rb b/lib/bulk_imports/ndjson_pipeline.rb index add220ad0d37b5..fe846771519f42 100644 --- a/lib/bulk_imports/ndjson_pipeline.rb +++ b/lib/bulk_imports/ndjson_pipeline.rb @@ -46,9 +46,9 @@ def load(_context, object) importable: portable ) - saver.execute do |persisted_objects| - push_placeholder_references(persisted_objects) if context.importer_user_mapping_enabled? - end + saver.execute + + push_placeholder_references(object) if context.importer_user_mapping_enabled? capture_invalid_subrelations(saver.invalid_subrelations) else @@ -176,13 +176,41 @@ def create_import_source_users(_relation_key, relation_hash) end end - def push_placeholder_references(persisted_objects) - persisted_objects.each do |object| + # rubocop:disable GitlabSecurity/PublicSend -- only methods in the relation_definition are called + def scan_objects(relation_definition, relation_object, flatten_objects) + relation_definition.each_key do |definition| + subrelation = relation_object.public_send(definition) + association = relation_object.class.reflect_on_association(definition) + + next if subrelation.nil? || association.nil? + + if association.collection? + subrelation.records.each do |record| + flatten_objects << record + + scan_objects(relation_definition[definition], record, flatten_objects) + end + else + flatten_objects << subrelation + end + end + end + # rubocop:enable GitlabSecurity/PublicSend + + def push_placeholder_references(object) + flatten_objects = [object] + + scan_objects(relation_definition, object, flatten_objects) + + flatten_objects.each do |object| + next unless object.persisted? next unless object.respond_to?(:source_user_references) object.source_user_references.each do |attribute, source_user_identifier| source_user = source_user_mapper.find_source_user(source_user_identifier) + next if source_user.accepted_status? && object[attribute] == source_user.reassign_to_user_id + ::Import::PlaceholderReferences::PushService.from_record( import_source: ::Import::SOURCE_DIRECT_TRANSFER, import_uid: context.bulk_import_id, diff --git a/lib/gitlab/import_export/base/relation_object_saver.rb b/lib/gitlab/import_export/base/relation_object_saver.rb index d539f4cbabed06..62bd650c1d5c8a 100644 --- a/lib/gitlab/import_export/base/relation_object_saver.rb +++ b/lib/gitlab/import_export/base/relation_object_saver.rb @@ -16,7 +16,7 @@ class RelationObjectSaver BATCH_SIZE = 100 - attr_reader :invalid_subrelations, :valid_records + attr_reader :invalid_subrelations # @param relation_object [Object] Object of a project/group, e.g. an issue # @param relation_key [String] Name of the object association to group/project, e.g. :issues @@ -38,14 +38,12 @@ def initialize(relation_object:, relation_key:, relation_definition:, importable @invalid_subrelations = [] end - def execute(&block) + def execute move_subrelations relation_object.save! - yield [relation_object] if block - - save_subrelations(&block) + save_subrelations end private @@ -53,7 +51,7 @@ def execute(&block) attr_reader :relation_object, :relation_key, :relation_definition, :importable, :collection_subrelations # rubocop:disable GitlabSecurity/PublicSend - def save_subrelations(&block) + def save_subrelations collection_subrelations.each_pair do |relation_name, records| records.each_slice(BATCH_SIZE) do |batch| valid_records, invalid_records = batch.partition { |record| record.valid? } @@ -69,16 +67,10 @@ def save_subrelations(&block) invalid_records.each do |invalid_record| relation_object.public_send(relation_name) << invalid_record - if invalid_record.persisted? - valid_records << invalid_record - else - invalid_subrelations << invalid_record - end + invalid_subrelations << invalid_record unless invalid_record.persisted? end - relation_object.save.tap do - yield valid_records if block - end + relation_object.save end end end diff --git a/lib/gitlab/import_export/project/relation_factory.rb b/lib/gitlab/import_export/project/relation_factory.rb index ce45b280de301b..15ac3d1358b13d 100644 --- a/lib/gitlab/import_export/project/relation_factory.rb +++ b/lib/gitlab/import_export/project/relation_factory.rb @@ -199,6 +199,7 @@ def setup_release def setup_pipeline_schedule @relation_hash['active'] = false @relation_hash['owner_id'] = @user.id + @original_user.delete('owner_id') # unset original user to not push placeholder references end def setup_merge_request diff --git a/lib/import/bulk_imports/source_users_mapper.rb b/lib/import/bulk_imports/source_users_mapper.rb index 2ce91b99acb59f..90a0ab19c31efd 100644 --- a/lib/import/bulk_imports/source_users_mapper.rb +++ b/lib/import/bulk_imports/source_users_mapper.rb @@ -22,7 +22,7 @@ def initialize(source_user_mapper) end def [](user_identifier) - @source_user_mapper.find_source_user(user_identifier).assignee.id + @source_user_mapper.find_source_user(user_identifier).mapped_user.id end end diff --git a/spec/lib/bulk_imports/ndjson_pipeline_spec.rb b/spec/lib/bulk_imports/ndjson_pipeline_spec.rb index 1ca8b15ec34848..e8707e38fb912a 100644 --- a/spec/lib/bulk_imports/ndjson_pipeline_spec.rb +++ b/spec/lib/bulk_imports/ndjson_pipeline_spec.rb @@ -29,6 +29,15 @@ ) end + let_it_be(:source_user_reassigned) do + create(:import_source_user, :completed, + import_type: ::Import::SOURCE_DIRECT_TRANSFER, + namespace: group, + source_user_identifier: 103, + source_hostname: bulk_import.configuration.source_hostname + ) + end + let(:klass) do Class.new do include BulkImports::NdjsonPipeline @@ -107,7 +116,7 @@ def initialize(portable, user, context) [ { "note" => "Issue note", - "author_id" => 103, + "author_id" => source_user_reassigned.source_user_identifier, "award_emoji" => [ { "name" => "clapper", "user_id" => 105 @@ -116,11 +125,15 @@ def initialize(portable, user, context) "name" => "clapper", "user_id" => source_user_2.source_user_identifier } ] + }, + { + "note" => "Issue note 2", + "author_id" => 106 } ], "merge_request_assignees" => [ { - "user_id" => 103 + "user_id" => source_user_reassigned.source_user_identifier }, { "user_id" => 104 @@ -141,7 +154,7 @@ def initialize(portable, user, context) expect { subject.deep_transform_relation!(relation_hash, 'test', relation_definition) { |a, _b| a } } .to change { Import::SourceUser.count }.by(3).and change { User.count }.by(1) - expect(Import::SourceUser.pluck(:source_user_identifier)).to match_array(%w[101 102 103 104 105]) + expect(Import::SourceUser.pluck(:source_user_identifier)).to match_array(%w[101 102 103 104 105 106]) expect(User.last.import_user?).to eq(true) end end @@ -343,19 +356,35 @@ def initialize(portable, user, context) context 'when importer_user_mapping is enabled' do before do allow(context).to receive(:importer_user_mapping_enabled?).and_return(true) + allow(subject).to receive(:relation_definition).and_return({ "notes" => { "events" => {} } }) end - it 'pushes a placeholder reference for every user referenced the persisted object has' do - merge_request = MergeRequest.new( - author: source_user_1.assignee, - updated_by: source_user_2.assignee, - source_user_references: { 'author_id' => '101', 'updated_by_id' => '102' } + it 'pushes a placeholder reference for every source user reference the persisted object has' do + merge_request = build(:merge_request, + source_project: project, + target_project: project, + author: source_user_1.mapped_user, + updated_by: source_user_1.mapped_user, + source_user_references: { + 'author_id' => source_user_1.source_user_identifier, + 'updated_by_id' => source_user_1.source_user_identifier + } ) - - allow_next_instance_of(Gitlab::ImportExport::Base::RelationObjectSaver) do |saver| - allow(saver).to receive(:execute) - .and_yield([merge_request]) - end + note = build(:note, + project: project, + author: source_user_1.mapped_user, + updated_by: source_user_2.mapped_user, + source_user_references: { + 'author_id' => source_user_1.source_user_identifier, + 'updated_by_id' => source_user_2.source_user_identifier + } + ) + merge_request.notes << note + event = build(:event, + author: source_user_1.mapped_user, + source_user_references: { 'author_id' => source_user_1.source_user_identifier } + ) + note.events << event expect(Import::PlaceholderReferences::PushService).to receive(:from_record).with( import_source: ::Import::SOURCE_DIRECT_TRANSFER, @@ -370,11 +399,54 @@ def initialize(portable, user, context) import_uid: context.bulk_import_id, record: merge_request, user_reference_column: :updated_by_id, + source_user: source_user_1 + ).and_call_original + + expect(::Import::PlaceholderReferences::PushService).to receive(:from_record).with( + import_source: ::Import::SOURCE_DIRECT_TRANSFER, + import_uid: context.bulk_import_id, + record: note, + user_reference_column: :author_id, + source_user: source_user_1 + ).and_call_original + + expect(::Import::PlaceholderReferences::PushService).to receive(:from_record).with( + import_source: ::Import::SOURCE_DIRECT_TRANSFER, + import_uid: context.bulk_import_id, + record: note, + user_reference_column: :updated_by_id, source_user: source_user_2 ).and_call_original + expect(::Import::PlaceholderReferences::PushService).to receive(:from_record).with( + import_source: ::Import::SOURCE_DIRECT_TRANSFER, + import_uid: context.bulk_import_id, + record: event, + user_reference_column: :author_id, + source_user: source_user_1 + ).and_call_original + subject.load(nil, merge_request) end + + context 'when source user is mapped to a real user' do + it 'does not push a placeholder reference' do + merge_request = build(:merge_request, + source_project: project, + target_project: project, + author: source_user_reassigned.mapped_user, + updated_by: source_user_reassigned.mapped_user, + source_user_references: { + 'author_id' => source_user_reassigned.source_user_identifier, + 'updated_by_id' => source_user_reassigned.source_user_identifier + } + ) + + expect(::Import::PlaceholderReferences::PushService).not_to receive(:from_record) + + subject.load(nil, merge_request) + end + end end end diff --git a/spec/lib/bulk_imports/projects/pipelines/ci_pipelines_pipeline_spec.rb b/spec/lib/bulk_imports/projects/pipelines/ci_pipelines_pipeline_spec.rb index cd59aa599f2733..6facbe5f9ccf76 100644 --- a/spec/lib/bulk_imports/projects/pipelines/ci_pipelines_pipeline_spec.rb +++ b/spec/lib/bulk_imports/projects/pipelines/ci_pipelines_pipeline_spec.rb @@ -6,7 +6,7 @@ let_it_be(:user) { create(:user) } let_it_be(:group) { create(:group) } let_it_be(:project) { create(:project, group: group) } - let_it_be(:bulk_import) { create(:bulk_import, user: user) } + let_it_be(:bulk_import) { create(:bulk_import, :with_configuration, user: user) } let_it_be(:entity) do create( :bulk_import_entity, @@ -19,6 +19,9 @@ ) end + let_it_be(:tracker) { create(:bulk_import_tracker, entity: entity) } + let_it_be(:context) { BulkImports::Pipeline::Context.new(tracker) } + let(:ci_pipeline_attributes) { {} } let(:ci_pipeline) do { @@ -38,8 +41,8 @@ }.merge(ci_pipeline_attributes) end - let_it_be(:tracker) { create(:bulk_import_tracker, entity: entity) } - let_it_be(:context) { BulkImports::Pipeline::Context.new(tracker) } + let(:importer_user_mapping_enabled) { false } + let(:extract_data) { [ci_pipeline, ci_pipeline2] } subject(:pipeline) { described_class.new(context) } @@ -53,7 +56,7 @@ allow_next_instance_of(BulkImports::Common::Extractors::NdjsonExtractor) do |extractor| allow(extractor).to receive(:extract).and_return( - BulkImports::Pipeline::ExtractedData.new(data: [ci_pipeline, ci_pipeline2]) + BulkImports::Pipeline::ExtractedData.new(data: extract_data) ) end @@ -61,6 +64,9 @@ allow(repository).to receive(:fetch_source_branch!) end + allow(context).to receive(:importer_user_mapping_enabled?).and_return(importer_user_mapping_enabled) + allow(Import::PlaceholderReferences::PushService).to receive(:from_record).and_call_original + pipeline.run end @@ -184,5 +190,119 @@ expect(merge_request.description).to eq('test merge request') end end + + context 'when importer_user_mapping is enabled' do + let(:importer_user_mapping_enabled) { true } + let(:extract_data) { [ci_pipeline] } + let(:ci_pipeline) do + { + sha: "fakesha", + ref: "fakeref", + project_id: 7, + source: "web", + user_id: 101, + stages: [ + { + name: 'Stage 1', + builds: [ + { + status: "success", + name: "build", + stage_idx: 1, + ref: "master", + type: "Ci::Build", + scheduling_type: "stage", + commit_id: 2, + project_id: 7, + user_id: 101 + } + ], + generic_commit_statuses: [ + { + status: "success", + name: "generic", + stage_idx: 1, + ref: "master", + type: "GenericCommitStatus", + scheduling_type: "stage", + commit_id: 1, + project_id: 7, + user_id: 101 + } + ], + bridges: [ + { + status: "success", + name: "bridge", + stage_idx: 1, + ref: "master", + type: "Ci::Bridge", + scheduling_type: "stage", + commit_id: 1, + project_id: 7, + user_id: 101 + } + ] + } + ] + }.merge(ci_pipeline_attributes) + .deep_stringify_keys + end + + it 'imports ci pipelines and map user references to placeholder users', :aggregate_failures do + ci_pipeline = project.all_pipelines.first + stage = project.all_pipelines.first.stages.first + build = stage.builds.first + generic_commit_status = stage.generic_commit_statuses.first + bridge = stage.bridges.first + + expect(ci_pipeline.user.import_user?).to eq(true) + expect(build.user.import_user?).to eq(true) + expect(generic_commit_status.user.import_user?).to eq(true) + expect(bridge.user.import_user?).to eq(true) + + source_user = Import::SourceUser.find_by(source_user_identifier: 101) + expect(source_user.placeholder_user.import_user?).to eq(true) + + expect(Import::PlaceholderReferences::PushService).to have_received(:from_record).exactly(4).times + end + + context 'when merge request is present in the extract data' do + let(:ci_pipeline_attributes) do + { + source: 'merge_request_event', + merge_request: { + iid: 1, + title: 'MR', + source_branch: 'source_branch', + target_branch: 'master', + source_sha: 'testsha', + target_sha: 'targetsha', + source_repository: 'test repository', + target_repository: 'test repository', + target_project_id: 7, + source_project_id: 7, + author_id: 101 + } + } + end + + it 'pushes placeholder references for the merge request' do + expect(Import::PlaceholderReferences::PushService).to have_received(:from_record).with(a_hash_including( + record: an_instance_of(MergeRequest) + )) + end + + context 'when merge request already exists in the database' do + let_it_be(:merge_request) { create(:merge_request, source_project: project, iid: 1) } + + it 'does not push placeholder references for the merge request' do + expect(Import::PlaceholderReferences::PushService).not_to have_received(:from_record).with(a_hash_including( + record: an_instance_of(MergeRequest) + )) + end + end + end + end end end diff --git a/spec/lib/bulk_imports/projects/pipelines/issues_pipeline_spec.rb b/spec/lib/bulk_imports/projects/pipelines/issues_pipeline_spec.rb index 87a92d7f23cfbb..3461094198e6b8 100644 --- a/spec/lib/bulk_imports/projects/pipelines/issues_pipeline_spec.rb +++ b/spec/lib/bulk_imports/projects/pipelines/issues_pipeline_spec.rb @@ -6,7 +6,7 @@ let_it_be(:user) { create(:user) } let_it_be(:group) { create(:group) } let_it_be(:project) { create(:project, group: group) } - let_it_be(:bulk_import) { create(:bulk_import, user: user) } + let_it_be(:bulk_import) { create(:bulk_import, :with_configuration, user: user) } let_it_be(:entity) do create( :bulk_import_entity, @@ -34,6 +34,8 @@ }.merge(issue_attributes) end + let(:importer_user_mapping_enabled) { false } + subject(:pipeline) { described_class.new(context) } describe '#run', :clean_gitlab_redis_shared_state do @@ -46,6 +48,8 @@ end allow(pipeline).to receive(:set_source_objects_counter) + allow(context).to receive(:importer_user_mapping_enabled?).and_return(importer_user_mapping_enabled) + allow(Import::PlaceholderReferences::PushService).to receive(:from_record).and_call_original pipeline.run end @@ -201,5 +205,88 @@ end end end + + context 'assignees' do + let(:issue_attributes) { { 'issue_assignees' => [{ 'user_id' => user.id }] } } + + it 'restores issue assignees' do + expect(project.issues.last.assignees).to contain_exactly(user) + end + end + + context 'when importer_user_mapping is enabled' do + let(:importer_user_mapping_enabled) { true } + let(:issue) do + { + title: 'Imported issue', + author_id: 101, + iid: 38, + updated_by_id: 101, + last_edited_at: '2019-12-27T00:00:00.000Z', + last_edited_by_id: 101, + closed_by_id: 101, + state: 'opened', + events: [{ author_id: 101, action: 'closed', target_type: 'Issue' }], + timelogs: [{ time_spent: 72000, spent_at: '2019-12-27T00:00:00.000Z', user_id: 101 }], + notes: [ + { + note: 'Note', + noteable_type: 'Issue', + author_id: 101, + updated_by_id: 101, + resolved_by_id: 101, + events: [{ action: 'created', author_id: 101 }] + } + ], + resource_label_events: [{ action: 'add', user_id: 101, label: { title: 'Ambalt', color: '#33594f' } }], + resource_milestone_events: [{ user_id: 101, action: 'add', state: 'opened', milestone: { title: 'Sprint 1' } }], + resource_state_events: [{ user_id: 101, state: 'closed' }], + designs: [{ filename: 'design.png', iid: 101 }], + design_versions: [{ + sha: '0ec80e1499f275d0553a2831608dd6938672eb44', + author_id: 101, + actions: [{ event: 'creation', design: { filename: 'design.png', iid: 1 } }] + }], + issue_assignees: [{ user_id: 101 }], + award_emoji: [{ name: 'clapper', user_id: 101 }] + }.deep_stringify_keys + end + + it 'imports issues and map user references to placeholder users', :aggregate_failures do + issue = project.issues.last + event = issue.events.first + note = issue.notes.first + note_event = note.events.first + timelog = issue.timelogs.first + resource_label_event = issue.resource_label_events.first + resource_milestone_event = issue.resource_milestone_events.first + resource_state_events = issue.resource_state_events.first + design_version = issue.design_versions.first + issue_assignee = issue.issue_assignees.first + award_emoji = issue.award_emoji.first + + expect(issue.author.import_user?).to eq(true) + expect(issue.updated_by.import_user?).to eq(true) + expect(issue.last_edited_by.import_user?).to eq(true) + expect(issue.closed_by.import_user?).to eq(true) + expect(event.author.import_user?).to eq(true) + expect(timelog.user.import_user?).to eq(true) + expect(note.author.import_user?).to eq(true) + expect(note.updated_by.import_user?).to eq(true) + expect(note.resolved_by.import_user?).to eq(true) + expect(note_event.author.import_user?).to eq(true) + expect(resource_label_event.user.import_user?).to eq(true) + expect(resource_milestone_event.user.import_user?).to eq(true) + expect(resource_state_events.user.import_user?).to eq(true) + expect(design_version.author.import_user?).to eq(true) + expect(issue_assignee.assignee.import_user?).to eq(true) + expect(award_emoji.user.import_user?).to eq(true) + + source_user = Import::SourceUser.find_by(source_user_identifier: 101) + expect(source_user.placeholder_user.import_user?).to eq(true) + + expect(Import::PlaceholderReferences::PushService).to have_received(:from_record).exactly(16).times + end + end end end diff --git a/spec/lib/bulk_imports/projects/pipelines/merge_requests_pipeline_spec.rb b/spec/lib/bulk_imports/projects/pipelines/merge_requests_pipeline_spec.rb index b843d8b4a4257f..d3f0fa81fa1ea8 100644 --- a/spec/lib/bulk_imports/projects/pipelines/merge_requests_pipeline_spec.rb +++ b/spec/lib/bulk_imports/projects/pipelines/merge_requests_pipeline_spec.rb @@ -7,7 +7,7 @@ let_it_be(:another_user) { create(:user) } let_it_be(:group) { create(:group) } let_it_be(:project) { create(:project, :repository, group: group) } - let_it_be(:bulk_import) { create(:bulk_import, user: user) } + let_it_be(:bulk_import) { create(:bulk_import, :with_configuration, user: user) } let_it_be(:entity) do create( :bulk_import_entity, @@ -23,6 +23,8 @@ let_it_be(:tracker) { create(:bulk_import_tracker, entity: entity) } let_it_be(:context) { BulkImports::Pipeline::Context.new(tracker) } + let(:importer_user_mapping_enabled) { false } + let(:mr) do { 'iid' => 7, @@ -103,6 +105,9 @@ allow(::Projects::ImportExport::AfterImportMergeRequestsWorker).to receive(:perform_async) allow(pipeline).to receive(:set_source_objects_counter) + allow(context).to receive(:importer_user_mapping_enabled?).and_return(importer_user_mapping_enabled) + allow(Import::PlaceholderReferences::PushService).to receive(:from_record).and_call_original + pipeline.run end @@ -350,5 +355,79 @@ end end end + + context 'when importer_user_mapping is enabled' do + let(:importer_user_mapping_enabled) { true } + + let(:mr) do + { + title: 'Imported MR', + author_id: 101, + iid: 38, + source_project_id: 1234, + target_project_id: 1234, + description: 'Description', + source_branch: 'feature', + target_branch: 'main', + source_branch_sha: 'ABCD', + target_branch_sha: 'DCBA', + updated_by_id: 101, + merge_user_id: 101, + last_edited_at: '2019-12-27T00:00:00.000Z', + last_edited_by_id: 101, + state: 'opened', + approvals: [{ user_id: 101 }], + metrics: { merged_by_id: 101, latest_closed_by_id: 101 }, + merge_request_assignees: [{ user_id: 101 }], + merge_request_reviewers: [{ user_id: 101, state: 'unreviewed' }], + events: [{ author_id: 101, action: 'created', target_type: 'MergeRequest' }], + timelogs: [{ time_spent: 72000, spent_at: '2019-12-27T00:00:00.000Z', user_id: 101 }], + notes: [{ note: 'Note', noteable_type: 'Issue', author_id: 101, updated_by_id: 101, resolved_by_id: 101 }], + resource_label_events: [{ action: 'add', user_id: 101, label: { title: 'Ambalt', color: '#33594f' } }], + resource_milestone_events: [{ user_id: 101, action: 'add', state: 'opened', milestone: { title: 'Sprint' } }], + resource_state_events: [{ user_id: 101, state: 'closed' }], + award_emoji: [{ name: 'clapper', user_id: 101 }] + }.deep_stringify_keys + end + + it 'imports merge_requests and map user references to placeholder users', :aggregate_failures do + merge_request = project.merge_requests.last + approval = merge_request.approvals.first + metrics = merge_request.metrics + merge_request_assignee = merge_request.merge_request_assignees.first + merge_request_reviewer = merge_request.merge_request_reviewers.first + event = merge_request.events.first + note = merge_request.notes.first + timelog = merge_request.timelogs.first + resource_label_event = merge_request.resource_label_events.first + resource_milestone_event = merge_request.resource_milestone_events.first + resource_state_events = merge_request.resource_state_events.first + award_emoji = merge_request.award_emoji.first + + expect(merge_request.author.import_user?).to eq(true) + expect(merge_request.merge_user.import_user?).to eq(true) + expect(merge_request.last_edited_by.import_user?).to eq(true) + expect(merge_request.updated_by.import_user?).to eq(true) + expect(approval.user.import_user?).to eq(true) + expect(metrics.merged_by.import_user?).to eq(true) + expect(metrics.latest_closed_by.import_user?).to eq(true) + expect(merge_request_assignee.assignee.import_user?).to eq(true) + expect(merge_request_reviewer.reviewer.import_user?).to eq(true) + expect(event.author.import_user?).to eq(true) + expect(timelog.user.import_user?).to eq(true) + expect(note.author.import_user?).to eq(true) + expect(note.updated_by.import_user?).to eq(true) + expect(note.resolved_by.import_user?).to eq(true) + expect(resource_label_event.user.import_user?).to eq(true) + expect(resource_milestone_event.user.import_user?).to eq(true) + expect(resource_state_events.user.import_user?).to eq(true) + expect(award_emoji.user.import_user?).to eq(true) + + source_user = Import::SourceUser.find_by(source_user_identifier: 101) + expect(source_user.placeholder_user.import_user?).to eq(true) + + expect(Import::PlaceholderReferences::PushService).to have_received(:from_record).exactly(18).times + end + end end end diff --git a/spec/lib/bulk_imports/projects/pipelines/pipeline_schedules_pipeline_spec.rb b/spec/lib/bulk_imports/projects/pipelines/pipeline_schedules_pipeline_spec.rb index c3fcd356cf1226..5ceee7a315a08c 100644 --- a/spec/lib/bulk_imports/projects/pipelines/pipeline_schedules_pipeline_spec.rb +++ b/spec/lib/bulk_imports/projects/pipelines/pipeline_schedules_pipeline_spec.rb @@ -6,7 +6,7 @@ let_it_be(:user) { create(:user) } let_it_be(:group) { create(:group) } let_it_be(:project) { create(:project, group: group) } - let_it_be(:bulk_import) { create(:bulk_import, user: user) } + let_it_be(:bulk_import) { create(:bulk_import, :with_configuration, user: user) } let_it_be(:entity) do create( :bulk_import_entity, @@ -22,6 +22,7 @@ let_it_be(:tracker) { create(:bulk_import_tracker, entity: entity) } let_it_be(:context) { BulkImports::Pipeline::Context.new(tracker) } + let(:importer_user_mapping_enabled) { false } let(:schedule_attributes) { {} } let(:schedule) do { @@ -43,6 +44,9 @@ allow(extractor).to receive(:extract).and_return(BulkImports::Pipeline::ExtractedData.new(data: [schedule])) end + allow(context).to receive(:importer_user_mapping_enabled?).and_return(importer_user_mapping_enabled) + allow(Import::PlaceholderReferences::PushService).to receive(:from_record).and_call_original + allow(pipeline).to receive(:set_source_objects_counter) pipeline.run @@ -63,4 +67,16 @@ expect(project.pipeline_schedules.first.active).to be_falsey end end + + context 'when importer_user_mapping is enabled' do + let(:importer_user_mapping_enabled) { true } + + let(:schedule_attributes) { { 'owner_id' => 101 } } + + it 'imports schedules and does not placeholder references', :aggregate_failures do + expect(project.pipeline_schedules.first.owner).to eq(user) + + expect(Import::PlaceholderReferences::PushService).not_to have_received(:from_record) + end + end end diff --git a/spec/lib/bulk_imports/projects/pipelines/protected_branches_pipeline_spec.rb b/spec/lib/bulk_imports/projects/pipelines/protected_branches_pipeline_spec.rb index 3e182c57d8765b..ae8c5eb8cd1f85 100644 --- a/spec/lib/bulk_imports/projects/pipelines/protected_branches_pipeline_spec.rb +++ b/spec/lib/bulk_imports/projects/pipelines/protected_branches_pipeline_spec.rb @@ -5,11 +5,12 @@ RSpec.describe BulkImports::Projects::Pipelines::ProtectedBranchesPipeline, feature_category: :importers do let_it_be(:user) { create(:user) } let_it_be(:project) { create(:project) } - let_it_be(:bulk_import) { create(:bulk_import, user: user) } + let_it_be(:bulk_import) { create(:bulk_import, :with_configuration, user: user) } let_it_be(:entity) { create(:bulk_import_entity, :project_entity, project: project, bulk_import: bulk_import) } let_it_be(:tracker) { create(:bulk_import_tracker, entity: entity) } let_it_be(:context) { BulkImports::Pipeline::Context.new(tracker) } - let_it_be(:protected_branch) do + + let(:protected_branch) do { 'name' => 'main', 'created_at' => '2016-06-14T15:02:47.967Z', @@ -34,13 +35,15 @@ subject(:pipeline) { described_class.new(context) } describe '#run' do - it 'imports protected branch information' do + before do allow_next_instance_of(BulkImports::Common::Extractors::NdjsonExtractor) do |extractor| - allow(extractor).to receive(:extract).and_return(BulkImports::Pipeline::ExtractedData.new(data: [protected_branch, 0])) + allow(extractor).to receive(:extract).and_return(BulkImports::Pipeline::ExtractedData.new(data: [protected_branch])) end allow(pipeline).to receive(:set_source_objects_counter) + end + it 'imports protected branch information' do pipeline.run imported_protected_branch = project.protected_branches.last diff --git a/spec/lib/bulk_imports/projects/pipelines/snippets_repository_pipeline_spec.rb b/spec/lib/bulk_imports/projects/pipelines/snippets_repository_pipeline_spec.rb index 8ecea8a10dc5c2..e3a132a508e755 100644 --- a/spec/lib/bulk_imports/projects/pipelines/snippets_repository_pipeline_spec.rb +++ b/spec/lib/bulk_imports/projects/pipelines/snippets_repository_pipeline_spec.rb @@ -22,6 +22,7 @@ let(:tracker) { create(:bulk_import_tracker, entity: entity) } let(:context) { BulkImports::Pipeline::Context.new(tracker) } + let(:importer_user_mapping_enabled) { false } subject(:pipeline) { described_class.new(context) } @@ -47,6 +48,8 @@ before do allow(pipeline).to receive(:set_source_objects_counter) + allow(context).to receive(:importer_user_mapping_enabled?).and_return(importer_user_mapping_enabled) + allow(Import::PlaceholderReferences::PushService).to receive(:from_record).and_call_original end describe 'extractor' do diff --git a/spec/lib/gitlab/import_export/base/relation_object_saver_spec.rb b/spec/lib/gitlab/import_export/base/relation_object_saver_spec.rb index 2233db0a47f789..13d94fdb6feaca 100644 --- a/spec/lib/gitlab/import_export/base/relation_object_saver_spec.rb +++ b/spec/lib/gitlab/import_export/base/relation_object_saver_spec.rb @@ -41,10 +41,6 @@ issue = project.issues.last expect(issue.notes.count).to eq(2) end - - it 'yields the saved records' do - expect { |b| saver.execute(&b) }.to yield_successive_args([relation_object], notes) - end end context 'when subrelation is not a collection' do @@ -80,10 +76,6 @@ expect(issue.notes.count).to eq(1) end - it 'yields the saved records' do - expect { |b| saver.execute(&b) }.to yield_successive_args([relation_object], [note]) - end - context 'when invalid subrelation can still be persisted' do let(:relation_key) { 'merge_requests' } let(:relation_definition) { { 'approvals' => {} } } @@ -99,10 +91,6 @@ expect(project.merge_requests.first.approvals.count).to eq(2) expect(project.merge_requests.first.approvals.first.persisted?).to eq(true) end - - it 'yields the saved records' do - expect { |b| saver.execute(&b) }.to yield_successive_args([relation_object], [approval_1, approval_2]) - end end context 'when importable is group' do diff --git a/spec/models/import/source_user_spec.rb b/spec/models/import/source_user_spec.rb index 1c8e09b423bfd3..7f98365f08abf7 100644 --- a/spec/models/import/source_user_spec.rb +++ b/spec/models/import/source_user_spec.rb @@ -215,10 +215,10 @@ end end - describe '#assignee' do + describe '#mapped_user' do let_it_be(:source_user) { build(:import_source_user, :with_reassign_to_user) } - subject(:assignee) { source_user.assignee } + subject(:mapped_user) { source_user.mapped_user } before do allow(source_user).to receive(:accepted_status?).and_return(accepted) -- GitLab From 8fcf0fc61296a63bea3f18466ba8929360f3d794 Mon Sep 17 00:00:00 2001 From: Rodrigo Tomonari Date: Tue, 30 Jul 2024 15:47:36 -0300 Subject: [PATCH 07/14] Do not push references for real users --- .../common/pipelines/members_pipeline.rb | 2 + .../common/pipelines/members_pipeline_spec.rb | 57 +++++++++++++------ 2 files changed, 42 insertions(+), 17 deletions(-) diff --git a/lib/bulk_imports/common/pipelines/members_pipeline.rb b/lib/bulk_imports/common/pipelines/members_pipeline.rb index 49cdcf432a9986..74615a452f1689 100644 --- a/lib/bulk_imports/common/pipelines/members_pipeline.rb +++ b/lib/bulk_imports/common/pipelines/members_pipeline.rb @@ -73,6 +73,8 @@ def finder_relations end def push_placeholder_reference(source_source, member) + return unless member.user.placeholder? + Import::PlaceholderReferences::PushService.from_record( import_source: Import::SOURCE_DIRECT_TRANSFER, import_uid: context.bulk_import_id, diff --git a/spec/lib/bulk_imports/common/pipelines/members_pipeline_spec.rb b/spec/lib/bulk_imports/common/pipelines/members_pipeline_spec.rb index 40d9418929c983..1a35f9aa13bde0 100644 --- a/spec/lib/bulk_imports/common/pipelines/members_pipeline_spec.rb +++ b/spec/lib/bulk_imports/common/pipelines/members_pipeline_spec.rb @@ -77,35 +77,55 @@ def extracted_data(email: '', id: 1, has_next_page: false) end context 'when importer_user_mapping is enabled' do - before do - allow_next_instance_of(Import::BulkImports::EphemeralData, bulk_import.id) do |ephemeral_data| - allow(ephemeral_data).to receive(:importer_user_mapping_enabled?).and_return(true) - end - - first_page = extracted_data(id: 101, has_next_page: true) - last_page = extracted_data(id: 102) + let!(:import_source_user) do + create(:import_source_user, + namespace: context.portable.root_ancestor, + source_hostname: bulk_import.configuration.source_hostname, + import_type: Import::SOURCE_DIRECT_TRANSFER, + source_user_identifier: '101' + ) + end - allow_next_instance_of(BulkImports::Common::Extractors::GraphqlExtractor) do |extractor| - allow(extractor).to receive(:extract).and_return(first_page, last_page) - end + let!(:reassigned_import_source_user) do + create(:import_source_user, + :completed, + namespace: context.portable.root_ancestor, + source_hostname: bulk_import.configuration.source_hostname, + import_type: Import::SOURCE_DIRECT_TRANSFER, + source_user_identifier: '102' + ) end - let!(:import_source_user) do + let!(:import_user_import_source_user) do create(:import_source_user, + placeholder_user: create(:user, :import_user), namespace: context.portable.root_ancestor, source_hostname: bulk_import.configuration.source_hostname, import_type: Import::SOURCE_DIRECT_TRANSFER, - source_user_identifier: '101' + source_user_identifier: '103' ) end - it 'find and creates source users and create membership for the assignee users' do - expect { pipeline.run }.to change { portable.members.count }.by(2).and( + before do + allow(context).to receive(:importer_user_mapping_enabled?).and_return(true) + + first_page = extracted_data(id: import_source_user.source_user_identifier, has_next_page: true) + second_page = extracted_data(id: reassigned_import_source_user.source_user_identifier, has_next_page: true) + third_page = extracted_data(id: import_user_import_source_user.source_user_identifier, has_next_page: true) + last_page = extracted_data(id: 104) + + allow_next_instance_of(BulkImports::Common::Extractors::GraphqlExtractor) do |extractor| + allow(extractor).to receive(:extract).and_return(first_page, second_page, third_page, last_page) + end + end + + it 'find and creates source users and create membership for the mapped users' do + expect { pipeline.run }.to change { portable.members.count }.by(3).and( change { Import::SourceUser.count }.by(1) ) created_import_source_user = Import::SourceUser.find_source_user( - source_user_identifier: '102', + source_user_identifier: '104', namespace: context.portable.root_ancestor, source_hostname: bulk_import.configuration.source_hostname, import_type: Import::SOURCE_DIRECT_TRANSFER @@ -113,16 +133,19 @@ def extracted_data(email: '', id: 1, has_next_page: false) expect(members).to contain_exactly( { user_id: import_source_user.placeholder_user.id, access_level: 30 }, + { user_id: reassigned_import_source_user.reassign_to_user.id, access_level: 30 }, { user_id: created_import_source_user.placeholder_user.id, access_level: 30 } ) end - it 'pushes placeholder references for each members' do + it 'pushes placeholder references for each members that is a placeholder user' do allow(Import::PlaceholderReferences::PushService).to receive(:from_record).twice pipeline.run - portable.members.each do |member| + placeholder_user_members = portable.members.select { |member| member.user.placeholder? } + + placeholder_user_members.each do |member| expect(Import::PlaceholderReferences::PushService).to have_received(:from_record).with( import_source: Import::SOURCE_DIRECT_TRANSFER, import_uid: bulk_import.id, -- GitLab From a90c5e73e903c123af7ab40ed013057e6f9f1fbb Mon Sep 17 00:00:00 2001 From: Rodrigo Tomonari Date: Tue, 30 Jul 2024 15:56:08 -0300 Subject: [PATCH 08/14] Undo file changes --- .../common/pipelines/boards_pipeline_spec.rb | 10 ++-------- .../groups/pipelines/epics_pipeline_spec.rb | 2 +- lib/gitlab/import_export/members_mapper.rb | 8 ++++---- .../pipelines/protected_branches_pipeline_spec.rb | 11 ++++------- .../pipelines/snippets_repository_pipeline_spec.rb | 3 --- spec/lib/bulk_imports/users_mapper_spec.rb | 6 ++++++ 6 files changed, 17 insertions(+), 23 deletions(-) diff --git a/ee/spec/lib/bulk_imports/common/pipelines/boards_pipeline_spec.rb b/ee/spec/lib/bulk_imports/common/pipelines/boards_pipeline_spec.rb index 4a04807a0e350b..eed43adf7cd8d5 100644 --- a/ee/spec/lib/bulk_imports/common/pipelines/boards_pipeline_spec.rb +++ b/ee/spec/lib/bulk_imports/common/pipelines/boards_pipeline_spec.rb @@ -5,7 +5,7 @@ RSpec.describe BulkImports::Common::Pipelines::BoardsPipeline, feature_category: :importers do let_it_be(:user) { create(:user) } let_it_be(:group) { create(:group) } - let_it_be(:bulk_import) { create(:bulk_import, :with_configuration, user: user) } + let_it_be(:bulk_import) { create(:bulk_import, user: user) } let_it_be(:filepath) { 'ee/spec/fixtures/bulk_imports/gz/boards.ndjson.gz' } let_it_be(:entity) do @@ -22,31 +22,25 @@ let_it_be(:tracker) { create(:bulk_import_tracker, entity: entity) } let_it_be(:context) { BulkImports::Pipeline::Context.new(tracker) } - let(:importer_user_mapping_enabled) { false } - let(:tmpdir) { Dir.mktmpdir } before do stub_licensed_features(board_assignee_lists: true, board_milestone_lists: true) FileUtils.copy_file(filepath, File.join(tmpdir, 'boards.ndjson.gz')) group.add_owner(user) - - allow(context).to receive(:importer_user_mapping_enabled?).and_return(importer_user_mapping_enabled) end subject { described_class.new(context) } describe '#run' do - before do + it 'imports group boards into destination group and removes tmpdir' do allow(Dir).to receive(:mktmpdir).and_return(tmpdir) allow_next_instance_of(BulkImports::FileDownloadService) do |service| allow(service).to receive(:execute) end allow(subject).to receive(:set_source_objects_counter) - end - it 'imports group boards into destination group and removes tmpdir' do expect { subject.run }.to change(Board, :count).by(2) lists = group.boards.find_by(name: 'first board').lists diff --git a/ee/spec/lib/bulk_imports/groups/pipelines/epics_pipeline_spec.rb b/ee/spec/lib/bulk_imports/groups/pipelines/epics_pipeline_spec.rb index 421d09a5c495b9..2e5ae57b67a971 100644 --- a/ee/spec/lib/bulk_imports/groups/pipelines/epics_pipeline_spec.rb +++ b/ee/spec/lib/bulk_imports/groups/pipelines/epics_pipeline_spec.rb @@ -5,7 +5,7 @@ RSpec.describe BulkImports::Groups::Pipelines::EpicsPipeline, feature_category: :importers do let_it_be(:user) { create(:user) } let_it_be(:group) { create(:group) } - let_it_be(:bulk_import) { create(:bulk_import, :with_configuration, user: user) } + let_it_be(:bulk_import) { create(:bulk_import, user: user) } let_it_be(:filepath) { 'ee/spec/fixtures/bulk_imports/gz/epics.ndjson.gz' } let_it_be(:entity) do create( diff --git a/lib/gitlab/import_export/members_mapper.rb b/lib/gitlab/import_export/members_mapper.rb index 9c3f5e3cf67223..635710f94ab6a3 100644 --- a/lib/gitlab/import_export/members_mapper.rb +++ b/lib/gitlab/import_export/members_mapper.rb @@ -26,16 +26,16 @@ def map end end + def default_user_id + @user.id + end + def include?(old_user_id) map.has_key?(old_user_id) end private - def default_user_id - @user.id - end - def missing_keys_tracking_hash Hash.new do |_, key| default_user_id diff --git a/spec/lib/bulk_imports/projects/pipelines/protected_branches_pipeline_spec.rb b/spec/lib/bulk_imports/projects/pipelines/protected_branches_pipeline_spec.rb index ae8c5eb8cd1f85..3e182c57d8765b 100644 --- a/spec/lib/bulk_imports/projects/pipelines/protected_branches_pipeline_spec.rb +++ b/spec/lib/bulk_imports/projects/pipelines/protected_branches_pipeline_spec.rb @@ -5,12 +5,11 @@ RSpec.describe BulkImports::Projects::Pipelines::ProtectedBranchesPipeline, feature_category: :importers do let_it_be(:user) { create(:user) } let_it_be(:project) { create(:project) } - let_it_be(:bulk_import) { create(:bulk_import, :with_configuration, user: user) } + let_it_be(:bulk_import) { create(:bulk_import, user: user) } let_it_be(:entity) { create(:bulk_import_entity, :project_entity, project: project, bulk_import: bulk_import) } let_it_be(:tracker) { create(:bulk_import_tracker, entity: entity) } let_it_be(:context) { BulkImports::Pipeline::Context.new(tracker) } - - let(:protected_branch) do + let_it_be(:protected_branch) do { 'name' => 'main', 'created_at' => '2016-06-14T15:02:47.967Z', @@ -35,15 +34,13 @@ subject(:pipeline) { described_class.new(context) } describe '#run' do - before do + it 'imports protected branch information' do allow_next_instance_of(BulkImports::Common::Extractors::NdjsonExtractor) do |extractor| - allow(extractor).to receive(:extract).and_return(BulkImports::Pipeline::ExtractedData.new(data: [protected_branch])) + allow(extractor).to receive(:extract).and_return(BulkImports::Pipeline::ExtractedData.new(data: [protected_branch, 0])) end allow(pipeline).to receive(:set_source_objects_counter) - end - it 'imports protected branch information' do pipeline.run imported_protected_branch = project.protected_branches.last diff --git a/spec/lib/bulk_imports/projects/pipelines/snippets_repository_pipeline_spec.rb b/spec/lib/bulk_imports/projects/pipelines/snippets_repository_pipeline_spec.rb index e3a132a508e755..8ecea8a10dc5c2 100644 --- a/spec/lib/bulk_imports/projects/pipelines/snippets_repository_pipeline_spec.rb +++ b/spec/lib/bulk_imports/projects/pipelines/snippets_repository_pipeline_spec.rb @@ -22,7 +22,6 @@ let(:tracker) { create(:bulk_import_tracker, entity: entity) } let(:context) { BulkImports::Pipeline::Context.new(tracker) } - let(:importer_user_mapping_enabled) { false } subject(:pipeline) { described_class.new(context) } @@ -48,8 +47,6 @@ before do allow(pipeline).to receive(:set_source_objects_counter) - allow(context).to receive(:importer_user_mapping_enabled?).and_return(importer_user_mapping_enabled) - allow(Import::PlaceholderReferences::PushService).to receive(:from_record).and_call_original end describe 'extractor' do diff --git a/spec/lib/bulk_imports/users_mapper_spec.rb b/spec/lib/bulk_imports/users_mapper_spec.rb index 3ddb165edf23cf..dc2beb42080457 100644 --- a/spec/lib/bulk_imports/users_mapper_spec.rb +++ b/spec/lib/bulk_imports/users_mapper_spec.rb @@ -50,6 +50,12 @@ end end + describe '#default_user_id' do + it 'returns current user id' do + expect(subject.default_user_id).to eq(user.id) + end + end + describe '#include?' do context 'when source user id is present in the map' do it 'returns true' do -- GitLab From 6b2a7a2ff3ca460a09f8ba2b3d4f729383a68316 Mon Sep 17 00:00:00 2001 From: Rodrigo Tomonari Date: Wed, 31 Jul 2024 20:03:51 -0300 Subject: [PATCH 09/14] Do not create references for system_note_meta --- lib/bulk_imports/ndjson_pipeline.rb | 7 ++++ spec/lib/bulk_imports/ndjson_pipeline_spec.rb | 41 ++++++++++++++++++- .../pipelines/issues_pipeline_spec.rb | 3 +- 3 files changed, 49 insertions(+), 2 deletions(-) diff --git a/lib/bulk_imports/ndjson_pipeline.rb b/lib/bulk_imports/ndjson_pipeline.rb index fe846771519f42..6cdfaf6fad8787 100644 --- a/lib/bulk_imports/ndjson_pipeline.rb +++ b/lib/bulk_imports/ndjson_pipeline.rb @@ -209,6 +209,13 @@ def push_placeholder_references(object) object.source_user_references.each do |attribute, source_user_identifier| source_user = source_user_mapper.find_source_user(source_user_identifier) + # Verify if the attribute is an ActiveRecord attribute. This check + # prevents a reference from being created for attributes that do + # not exist on the database table. For example, SystemNoteMetadata + # responds to `source_user_references` as the method is delegated to + # notes causing a reference to be created for all note's + # source_user_references + next unless object.has_attribute?(attribute) next if source_user.accepted_status? && object[attribute] == source_user.reassign_to_user_id ::Import::PlaceholderReferences::PushService.from_record( diff --git a/spec/lib/bulk_imports/ndjson_pipeline_spec.rb b/spec/lib/bulk_imports/ndjson_pipeline_spec.rb index e8707e38fb912a..574f05df0e2c9c 100644 --- a/spec/lib/bulk_imports/ndjson_pipeline_spec.rb +++ b/spec/lib/bulk_imports/ndjson_pipeline_spec.rb @@ -356,7 +356,9 @@ def initialize(portable, user, context) context 'when importer_user_mapping is enabled' do before do allow(context).to receive(:importer_user_mapping_enabled?).and_return(true) - allow(subject).to receive(:relation_definition).and_return({ "notes" => { "events" => {} } }) + allow(subject).to receive(:relation_definition).and_return( + { "notes" => { "events" => {}, "system_note_metadata" => {} } } + ) end it 'pushes a placeholder reference for every source user reference the persisted object has' do @@ -447,6 +449,43 @@ def initialize(portable, user, context) subject.load(nil, merge_request) end end + + context 'when object delegates `source_user_references` to a different object' do + it 'does not push a placeholder reference' do + merge_request = build(:merge_request, + source_project: project, + target_project: project, + author: source_user_1.mapped_user, + source_user_references: { + 'author_id' => source_user_1.source_user_identifier + } + ) + + system_note_metadata = build(:system_note_metadata) + + note = build(:note, + project: project, + author: source_user_1.mapped_user, + source_user_references: { + 'author_id' => source_user_1.source_user_identifier + }, + system_note_metadata: system_note_metadata + ) + + merge_request.notes << note + + expect(Import::PlaceholderReferences::PushService).to receive(:from_record) + .with(a_hash_including(record: merge_request)).and_call_original + + expect(Import::PlaceholderReferences::PushService).to receive(:from_record) + .with(a_hash_including(record: note)).and_call_original + + expect(Import::PlaceholderReferences::PushService).not_to receive(:from_record) + .with(a_hash_including(record: system_note_metadata)).and_call_original + + subject.load(nil, merge_request) + end + end end end diff --git a/spec/lib/bulk_imports/projects/pipelines/issues_pipeline_spec.rb b/spec/lib/bulk_imports/projects/pipelines/issues_pipeline_spec.rb index 3461094198e6b8..881f8e8307a8d3 100644 --- a/spec/lib/bulk_imports/projects/pipelines/issues_pipeline_spec.rb +++ b/spec/lib/bulk_imports/projects/pipelines/issues_pipeline_spec.rb @@ -235,7 +235,8 @@ author_id: 101, updated_by_id: 101, resolved_by_id: 101, - events: [{ action: 'created', author_id: 101 }] + events: [{ action: 'created', author_id: 101 }], + system_note_metadata: { commit_count: nil, action: "cross_reference" } } ], resource_label_events: [{ action: 'add', user_id: 101, label: { title: 'Ambalt', color: '#33594f' } }], -- GitLab From 013f222bbfa0a5ad774b454e8fc12b0847241dd2 Mon Sep 17 00:00:00 2001 From: Rodrigo Tomonari Date: Wed, 31 Jul 2024 21:18:01 -0300 Subject: [PATCH 10/14] Code review feedback --- .../import/placeholder_references/push_service.rb | 8 ++++---- lib/bulk_imports/common/pipelines/members_pipeline.rb | 4 ++-- .../common/transformers/member_attributes_transformer.rb | 4 ++-- lib/bulk_imports/ndjson_pipeline.rb | 4 ++-- 4 files changed, 10 insertions(+), 10 deletions(-) diff --git a/app/services/import/placeholder_references/push_service.rb b/app/services/import/placeholder_references/push_service.rb index 77acf8bccc513c..be98939fd67d1a 100644 --- a/app/services/import/placeholder_references/push_service.rb +++ b/app/services/import/placeholder_references/push_service.rb @@ -5,11 +5,11 @@ module PlaceholderReferences class PushService < BaseService class << self def from_record(import_source:, import_uid:, source_user:, record:, user_reference_column:) + composite_key = nil + numeric_key = nil + if record.is_a?(IssueAssignee) - composite_key = { - 'issue_id' => record.issue_id, - 'user_id' => record.user_id - } + composite_key = { 'issue_id' => record.issue_id, 'user_id' => record.user_id } else numeric_key = record.id end diff --git a/lib/bulk_imports/common/pipelines/members_pipeline.rb b/lib/bulk_imports/common/pipelines/members_pipeline.rb index 74615a452f1689..67618b8bd938ab 100644 --- a/lib/bulk_imports/common/pipelines/members_pipeline.rb +++ b/lib/bulk_imports/common/pipelines/members_pipeline.rb @@ -29,14 +29,14 @@ def load(_context, data) # User is already a member with higher existing (inherited) membership return if user_membership && user_membership[:access_level] >= data[:access_level] - source_source = data.delete(:source_user) + source_user = data.delete(:source_user) # Create new membership for any other access level member = portable.members.new(data) member.importing = true # avoid sending new member notification to the invited user member.save! - push_placeholder_reference(source_source, member) if context.importer_user_mapping_enabled? + push_placeholder_reference(source_user, member) if context.importer_user_mapping_enabled? member end diff --git a/lib/bulk_imports/common/transformers/member_attributes_transformer.rb b/lib/bulk_imports/common/transformers/member_attributes_transformer.rb index f4848b95f415cc..1415138bc945f7 100644 --- a/lib/bulk_imports/common/transformers/member_attributes_transformer.rb +++ b/lib/bulk_imports/common/transformers/member_attributes_transformer.rb @@ -5,7 +5,7 @@ module Common module Transformers class MemberAttributesTransformer def transform(context, data) - return source_user_transformer(context, data) if context.importer_user_mapping_enabled? + return source_user_member(context, data) if context.importer_user_mapping_enabled? user = find_user(data&.dig('user', 'public_email')) access_level = data&.dig('access_level', 'integer_value') @@ -28,7 +28,7 @@ def transform(context, data) private - def source_user_transformer(context, data) + def source_user_member(context, data) access_level = data&.dig('access_level', 'integer_value') return unless data diff --git a/lib/bulk_imports/ndjson_pipeline.rb b/lib/bulk_imports/ndjson_pipeline.rb index 6cdfaf6fad8787..3a0969361e6cf3 100644 --- a/lib/bulk_imports/ndjson_pipeline.rb +++ b/lib/bulk_imports/ndjson_pipeline.rb @@ -87,8 +87,8 @@ def deep_transform_relation!(relation_hash, relation_key, relation_definition, & end end - # Create missing Import::SourceUser objects during the transformation step, as no data - # related to the relation has been persisted yet. + # Create missing Import::SourceUser objects during the transformation step, as data + # related to the relation has not persisted yet. create_import_source_users(relation_key, relation_hash) if context.importer_user_mapping_enabled? yield(relation_key, relation_hash) -- GitLab From bb8e7ab20f009ad64407a07d207dd5695dae0d09 Mon Sep 17 00:00:00 2001 From: Rodrigo Tomonari Date: Fri, 2 Aug 2024 14:40:02 -0300 Subject: [PATCH 11/14] Add documentation to ndjson pipeline methods --- lib/bulk_imports/ndjson_pipeline.rb | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/lib/bulk_imports/ndjson_pipeline.rb b/lib/bulk_imports/ndjson_pipeline.rb index 3a0969361e6cf3..42bd1e74eebefe 100644 --- a/lib/bulk_imports/ndjson_pipeline.rb +++ b/lib/bulk_imports/ndjson_pipeline.rb @@ -176,6 +176,23 @@ def create_import_source_users(_relation_key, relation_hash) end end + # Method recursively scans through the relationships of an object based + # on the relation_definition and places all objects into a flattened list + # of objects. + # + # For example, if the relation_definition is: { "notes" => { "events" => {} } } + # + # and the relation_object a merge_request with the following notes: + # + # event1 = Event.new + # note1 = Note.new(events: [event1]) + # event2 = Event.new + # note2 = Note.new(events: [event2]) + # merge_request = MergeRequest.new(notes:[note1, note2]) + # + # the flatten_objects list will contain: + # [note1, event1, note2, event2] + # # rubocop:disable GitlabSecurity/PublicSend -- only methods in the relation_definition are called def scan_objects(relation_definition, relation_object, flatten_objects) relation_definition.each_key do |definition| @@ -216,6 +233,9 @@ def push_placeholder_references(object) # notes causing a reference to be created for all note's # source_user_references next unless object.has_attribute?(attribute) + + # Do not create a reference if the object is already associated + # with a real user. next if source_user.accepted_status? && object[attribute] == source_user.reassign_to_user_id ::Import::PlaceholderReferences::PushService.from_record( -- GitLab From e34b5146d1e401a802ac5b428746813691b686f5 Mon Sep 17 00:00:00 2001 From: Rodrigo Tomonari Date: Tue, 6 Aug 2024 21:51:37 +0000 Subject: [PATCH 12/14] Apply 3 suggestion(s) to 2 file(s) Co-authored-by: Luke Duncalfe --- .../common/transformers/member_attributes_transformer.rb | 4 ++-- spec/models/bulk_imports/configuration_spec.rb | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/bulk_imports/common/transformers/member_attributes_transformer.rb b/lib/bulk_imports/common/transformers/member_attributes_transformer.rb index 1415138bc945f7..1749d54e9af027 100644 --- a/lib/bulk_imports/common/transformers/member_attributes_transformer.rb +++ b/lib/bulk_imports/common/transformers/member_attributes_transformer.rb @@ -36,7 +36,7 @@ def source_user_member(context, data) source_user = find_or_create_source_user(context, data) - # To not add user as a member is the source user is mapped to the ImportUser because we cannot + # Do not add user as a member if the source user is mapped to the ImportUser because we cannot # create multiple members for the same user. return if source_user.mapped_user.import_user? @@ -79,7 +79,7 @@ def cache_source_username_data(data, user, context) end def find_or_create_source_user(context, data) - gid = data&.dig('user', 'user_gid') + gid = data.dig('user', 'user_gid') source_user_id = GlobalID.parse(gid).model_id source_name = data.dig('user', 'name') diff --git a/spec/models/bulk_imports/configuration_spec.rb b/spec/models/bulk_imports/configuration_spec.rb index 54309bc2faa39d..4518a93fb5c4de 100644 --- a/spec/models/bulk_imports/configuration_spec.rb +++ b/spec/models/bulk_imports/configuration_spec.rb @@ -16,7 +16,7 @@ end describe '#source_hostname' do - let(:configuration) { described_class.new(url: 'http://example.com') } + let(:configuration) { described_class.new(url: 'http://example.com/subdir') } it 'returns the hostname of the URL' do expect(configuration.source_hostname).to eq('example.com') -- GitLab From 565d3f6ec2cdf38374ed67f178907138872d1dbf Mon Sep 17 00:00:00 2001 From: Rodrigo Tomonari Date: Tue, 6 Aug 2024 19:32:17 -0300 Subject: [PATCH 13/14] Remove use_import_user options --- lib/bulk_imports/ndjson_pipeline.rb | 3 +- lib/gitlab/import/source_user_mapper.rb | 18 +++------ spec/lib/bulk_imports/ndjson_pipeline_spec.rb | 2 +- .../pipelines/ci_pipelines_pipeline_spec.rb | 11 +++--- .../pipelines/issues_pipeline_spec.rb | 35 +++++++++--------- .../pipelines/merge_requests_pipeline_spec.rb | 37 ++++++++++--------- .../gitlab/import/source_user_mapper_spec.rb | 16 +------- 7 files changed, 52 insertions(+), 70 deletions(-) diff --git a/lib/bulk_imports/ndjson_pipeline.rb b/lib/bulk_imports/ndjson_pipeline.rb index 42bd1e74eebefe..5d322280a0a31e 100644 --- a/lib/bulk_imports/ndjson_pipeline.rb +++ b/lib/bulk_imports/ndjson_pipeline.rb @@ -170,8 +170,7 @@ def create_import_source_users(_relation_key, relation_hash) source_user_mapper.find_or_create_source_user( source_name: nil, source_username: nil, - source_user_identifier: relation_hash[reference], - use_import_user: true + source_user_identifier: relation_hash[reference] ) end end diff --git a/lib/gitlab/import/source_user_mapper.rb b/lib/gitlab/import/source_user_mapper.rb index 6c39bb7254648b..3ad2b3fc70205c 100644 --- a/lib/gitlab/import/source_user_mapper.rb +++ b/lib/gitlab/import/source_user_mapper.rb @@ -20,7 +20,7 @@ def find_source_user(source_user_identifier) ) end - def find_or_create_source_user(source_name:, source_username:, source_user_identifier:, use_import_user: false) + def find_or_create_source_user(source_name:, source_username:, source_user_identifier:) source_user = find_source_user(source_user_identifier) return source_user if source_user @@ -28,8 +28,7 @@ def find_or_create_source_user(source_name:, source_username:, source_user_ident create_source_user( source_name: source_name, source_username: source_username, - source_user_identifier: source_user_identifier, - use_import_user: use_import_user + source_user_identifier: source_user_identifier ) end @@ -37,18 +36,18 @@ def find_or_create_source_user(source_name:, source_username:, source_user_ident attr_reader :namespace, :import_type, :source_hostname - def create_source_user(source_name:, source_username:, source_user_identifier:, use_import_user: false) + def create_source_user(source_name:, source_username:, source_user_identifier:) in_lock(lock_key(source_user_identifier), sleep_sec: 2.seconds) do |retried| if retried source_user = find_source_user(source_user_identifier) next source_user if source_user end - create_source_user_mapping(source_name, source_username, source_user_identifier, use_import_user) + create_source_user_mapping(source_name, source_username, source_user_identifier) end end - def create_source_user_mapping(source_name, source_username, source_user_identifier, use_import_user) + def create_source_user_mapping(source_name, source_username, source_user_identifier) ::Import::SourceUser.transaction do import_source_user = ::Import::SourceUser.new( namespace: namespace, @@ -59,12 +58,7 @@ def create_source_user_mapping(source_name, source_username, source_user_identif source_hostname: source_hostname ) - import_source_user.placeholder_user = if use_import_user - import_user - else - create_placeholder_user(source_name, source_username) - end - + import_source_user.placeholder_user = create_placeholder_user(source_name, source_username) import_source_user.save! import_source_user end diff --git a/spec/lib/bulk_imports/ndjson_pipeline_spec.rb b/spec/lib/bulk_imports/ndjson_pipeline_spec.rb index 574f05df0e2c9c..be63a301705b0d 100644 --- a/spec/lib/bulk_imports/ndjson_pipeline_spec.rb +++ b/spec/lib/bulk_imports/ndjson_pipeline_spec.rb @@ -155,7 +155,7 @@ def initialize(portable, user, context) expect { subject.deep_transform_relation!(relation_hash, 'test', relation_definition) { |a, _b| a } } .to change { Import::SourceUser.count }.by(3).and change { User.count }.by(1) expect(Import::SourceUser.pluck(:source_user_identifier)).to match_array(%w[101 102 103 104 105 106]) - expect(User.last.import_user?).to eq(true) + expect(User.last.placeholder?).to eq(true) end end diff --git a/spec/lib/bulk_imports/projects/pipelines/ci_pipelines_pipeline_spec.rb b/spec/lib/bulk_imports/projects/pipelines/ci_pipelines_pipeline_spec.rb index 6facbe5f9ccf76..92dd9b52fc4a9d 100644 --- a/spec/lib/bulk_imports/projects/pipelines/ci_pipelines_pipeline_spec.rb +++ b/spec/lib/bulk_imports/projects/pipelines/ci_pipelines_pipeline_spec.rb @@ -3,6 +3,7 @@ require 'spec_helper' RSpec.describe BulkImports::Projects::Pipelines::CiPipelinesPipeline, feature_category: :importers do + let_it_be(:default_organization) { create(:organization, :default) } let_it_be(:user) { create(:user) } let_it_be(:group) { create(:group) } let_it_be(:project) { create(:project, group: group) } @@ -256,13 +257,13 @@ generic_commit_status = stage.generic_commit_statuses.first bridge = stage.bridges.first - expect(ci_pipeline.user.import_user?).to eq(true) - expect(build.user.import_user?).to eq(true) - expect(generic_commit_status.user.import_user?).to eq(true) - expect(bridge.user.import_user?).to eq(true) + expect(ci_pipeline.user.placeholder?).to eq(true) + expect(build.user.placeholder?).to eq(true) + expect(generic_commit_status.user.placeholder?).to eq(true) + expect(bridge.user.placeholder?).to eq(true) source_user = Import::SourceUser.find_by(source_user_identifier: 101) - expect(source_user.placeholder_user.import_user?).to eq(true) + expect(source_user.placeholder_user.placeholder?).to eq(true) expect(Import::PlaceholderReferences::PushService).to have_received(:from_record).exactly(4).times end diff --git a/spec/lib/bulk_imports/projects/pipelines/issues_pipeline_spec.rb b/spec/lib/bulk_imports/projects/pipelines/issues_pipeline_spec.rb index 881f8e8307a8d3..9298702cc42aed 100644 --- a/spec/lib/bulk_imports/projects/pipelines/issues_pipeline_spec.rb +++ b/spec/lib/bulk_imports/projects/pipelines/issues_pipeline_spec.rb @@ -3,6 +3,7 @@ require 'spec_helper' RSpec.describe BulkImports::Projects::Pipelines::IssuesPipeline, feature_category: :importers do + let_it_be(:default_organization) { create(:organization, :default) } let_it_be(:user) { create(:user) } let_it_be(:group) { create(:group) } let_it_be(:project) { create(:project, group: group) } @@ -266,25 +267,25 @@ issue_assignee = issue.issue_assignees.first award_emoji = issue.award_emoji.first - expect(issue.author.import_user?).to eq(true) - expect(issue.updated_by.import_user?).to eq(true) - expect(issue.last_edited_by.import_user?).to eq(true) - expect(issue.closed_by.import_user?).to eq(true) - expect(event.author.import_user?).to eq(true) - expect(timelog.user.import_user?).to eq(true) - expect(note.author.import_user?).to eq(true) - expect(note.updated_by.import_user?).to eq(true) - expect(note.resolved_by.import_user?).to eq(true) - expect(note_event.author.import_user?).to eq(true) - expect(resource_label_event.user.import_user?).to eq(true) - expect(resource_milestone_event.user.import_user?).to eq(true) - expect(resource_state_events.user.import_user?).to eq(true) - expect(design_version.author.import_user?).to eq(true) - expect(issue_assignee.assignee.import_user?).to eq(true) - expect(award_emoji.user.import_user?).to eq(true) + expect(issue.author.placeholder?).to eq(true) + expect(issue.updated_by.placeholder?).to eq(true) + expect(issue.last_edited_by.placeholder?).to eq(true) + expect(issue.closed_by.placeholder?).to eq(true) + expect(event.author.placeholder?).to eq(true) + expect(timelog.user.placeholder?).to eq(true) + expect(note.author.placeholder?).to eq(true) + expect(note.updated_by.placeholder?).to eq(true) + expect(note.resolved_by.placeholder?).to eq(true) + expect(note_event.author.placeholder?).to eq(true) + expect(resource_label_event.user.placeholder?).to eq(true) + expect(resource_milestone_event.user.placeholder?).to eq(true) + expect(resource_state_events.user.placeholder?).to eq(true) + expect(design_version.author.placeholder?).to eq(true) + expect(issue_assignee.assignee.placeholder?).to eq(true) + expect(award_emoji.user.placeholder?).to eq(true) source_user = Import::SourceUser.find_by(source_user_identifier: 101) - expect(source_user.placeholder_user.import_user?).to eq(true) + expect(source_user.placeholder_user.placeholder?).to eq(true) expect(Import::PlaceholderReferences::PushService).to have_received(:from_record).exactly(16).times end diff --git a/spec/lib/bulk_imports/projects/pipelines/merge_requests_pipeline_spec.rb b/spec/lib/bulk_imports/projects/pipelines/merge_requests_pipeline_spec.rb index d3f0fa81fa1ea8..65fafdfc6430cc 100644 --- a/spec/lib/bulk_imports/projects/pipelines/merge_requests_pipeline_spec.rb +++ b/spec/lib/bulk_imports/projects/pipelines/merge_requests_pipeline_spec.rb @@ -3,6 +3,7 @@ require 'spec_helper' RSpec.describe BulkImports::Projects::Pipelines::MergeRequestsPipeline, feature_category: :importers do + let_it_be(:default_organization) { create(:organization, :default) } let_it_be(:user) { create(:user) } let_it_be(:another_user) { create(:user) } let_it_be(:group) { create(:group) } @@ -404,24 +405,24 @@ resource_state_events = merge_request.resource_state_events.first award_emoji = merge_request.award_emoji.first - expect(merge_request.author.import_user?).to eq(true) - expect(merge_request.merge_user.import_user?).to eq(true) - expect(merge_request.last_edited_by.import_user?).to eq(true) - expect(merge_request.updated_by.import_user?).to eq(true) - expect(approval.user.import_user?).to eq(true) - expect(metrics.merged_by.import_user?).to eq(true) - expect(metrics.latest_closed_by.import_user?).to eq(true) - expect(merge_request_assignee.assignee.import_user?).to eq(true) - expect(merge_request_reviewer.reviewer.import_user?).to eq(true) - expect(event.author.import_user?).to eq(true) - expect(timelog.user.import_user?).to eq(true) - expect(note.author.import_user?).to eq(true) - expect(note.updated_by.import_user?).to eq(true) - expect(note.resolved_by.import_user?).to eq(true) - expect(resource_label_event.user.import_user?).to eq(true) - expect(resource_milestone_event.user.import_user?).to eq(true) - expect(resource_state_events.user.import_user?).to eq(true) - expect(award_emoji.user.import_user?).to eq(true) + expect(merge_request.author.placeholder?).to eq(true) + expect(merge_request.merge_user.placeholder?).to eq(true) + expect(merge_request.last_edited_by.placeholder?).to eq(true) + expect(merge_request.updated_by.placeholder?).to eq(true) + expect(approval.user.placeholder?).to eq(true) + expect(metrics.merged_by.placeholder?).to eq(true) + expect(metrics.latest_closed_by.placeholder?).to eq(true) + expect(merge_request_assignee.assignee.placeholder?).to eq(true) + expect(merge_request_reviewer.reviewer.placeholder?).to eq(true) + expect(event.author.placeholder?).to eq(true) + expect(timelog.user.placeholder?).to eq(true) + expect(note.author.placeholder?).to eq(true) + expect(note.updated_by.placeholder?).to eq(true) + expect(note.resolved_by.placeholder?).to eq(true) + expect(resource_label_event.user.placeholder?).to eq(true) + expect(resource_milestone_event.user.placeholder?).to eq(true) + expect(resource_state_events.user.placeholder?).to eq(true) + expect(award_emoji.user.placeholder?).to eq(true) source_user = Import::SourceUser.find_by(source_user_identifier: 101) expect(source_user.placeholder_user.import_user?).to eq(true) diff --git a/spec/lib/gitlab/import/source_user_mapper_spec.rb b/spec/lib/gitlab/import/source_user_mapper_spec.rb index 6aa2ae69612211..e18785f145dbb5 100644 --- a/spec/lib/gitlab/import/source_user_mapper_spec.rb +++ b/spec/lib/gitlab/import/source_user_mapper_spec.rb @@ -22,7 +22,6 @@ let(:source_name) { 'Pry Contributor' } let(:source_username) { 'a_pry_contributor' } let(:source_user_identifier) { '123456' } - let(:use_import_user) { false } subject(:find_or_create_source_user) do described_class.new( @@ -32,8 +31,7 @@ ).find_or_create_source_user( source_name: source_name, source_username: source_username, - source_user_identifier: source_user_identifier, - use_import_user: use_import_user + source_user_identifier: source_user_identifier ) end @@ -113,18 +111,6 @@ it_behaves_like 'it does not create an import_source_user or placeholder user' end - - context 'and use_import_user is true' do - let(:use_import_user) { true } - - it 'creates source user mapped to the ImportUser' do - expect { find_or_create_source_user }.to change { Import::SourceUser.count }.by(1) - - new_import_source_user = Import::SourceUser.last - - expect(new_import_source_user.placeholder_user.import_user?).to eq(true) - end - end end end -- GitLab From 00c5465a2d4efcb431ed9c58a57200cfdc17ba46 Mon Sep 17 00:00:00 2001 From: Rodrigo Tomonari Date: Tue, 6 Aug 2024 19:55:04 -0300 Subject: [PATCH 14/14] Move source_user_mapper to context --- .../common/pipelines/members_pipeline.rb | 4 ++-- .../member_attributes_transformer.rb | 10 +--------- lib/bulk_imports/ndjson_pipeline.rb | 12 ++++------- lib/bulk_imports/pipeline/context.rb | 8 ++++++++ .../bulk_imports/source_users_mapper.rb | 8 +------- spec/lib/bulk_imports/ndjson_pipeline_spec.rb | 3 ++- .../lib/bulk_imports/pipeline/context_spec.rb | 16 ++++++++++++++- .../bulk_imports/source_users_mapper_spec.rb | 20 ++++++++++++++----- 8 files changed, 48 insertions(+), 33 deletions(-) diff --git a/lib/bulk_imports/common/pipelines/members_pipeline.rb b/lib/bulk_imports/common/pipelines/members_pipeline.rb index 67618b8bd938ab..c3b06f8a77707a 100644 --- a/lib/bulk_imports/common/pipelines/members_pipeline.rb +++ b/lib/bulk_imports/common/pipelines/members_pipeline.rb @@ -72,7 +72,7 @@ def finder_relations end end - def push_placeholder_reference(source_source, member) + def push_placeholder_reference(source_user, member) return unless member.user.placeholder? Import::PlaceholderReferences::PushService.from_record( @@ -80,7 +80,7 @@ def push_placeholder_reference(source_source, member) import_uid: context.bulk_import_id, record: member, user_reference_column: :user_id, - source_user: source_source + source_user: source_user ).execute end end diff --git a/lib/bulk_imports/common/transformers/member_attributes_transformer.rb b/lib/bulk_imports/common/transformers/member_attributes_transformer.rb index 1749d54e9af027..5b6f7bb802df72 100644 --- a/lib/bulk_imports/common/transformers/member_attributes_transformer.rb +++ b/lib/bulk_imports/common/transformers/member_attributes_transformer.rb @@ -85,20 +85,12 @@ def find_or_create_source_user(context, data) source_name = data.dig('user', 'name') source_username = data.dig('user', 'username') - source_user_mapper(context).find_or_create_source_user( + context.source_user_mapper.find_or_create_source_user( source_user_identifier: source_user_id, source_name: source_name, source_username: source_username ) end - - def source_user_mapper(context) - Gitlab::Import::SourceUserMapper.new( - import_type: Import::SOURCE_DIRECT_TRANSFER, - namespace: context.portable.root_ancestor, - source_hostname: context.configuration.source_hostname - ) - end end end end diff --git a/lib/bulk_imports/ndjson_pipeline.rb b/lib/bulk_imports/ndjson_pipeline.rb index 5d322280a0a31e..fb8e9105b82f24 100644 --- a/lib/bulk_imports/ndjson_pipeline.rb +++ b/lib/bulk_imports/ndjson_pipeline.rb @@ -132,6 +132,10 @@ def members_mapper end end + def source_user_mapper + context.source_user_mapper + end + def portable_class_sym portable.class.to_s.downcase.to_sym end @@ -153,14 +157,6 @@ def capture_invalid_subrelations(invalid_subrelations) end end - def source_user_mapper - @source_user_mapper ||= Gitlab::Import::SourceUserMapper.new( - namespace: context.portable.root_ancestor, - import_type: Import::SOURCE_DIRECT_TRANSFER, - source_hostname: context.configuration.source_hostname - ) - end - # Creates an Import::SourceUser objects for each source_user_identifier # found in the relation_hash and associate it with the ImportUser. def create_import_source_users(_relation_key, relation_hash) diff --git a/lib/bulk_imports/pipeline/context.rb b/lib/bulk_imports/pipeline/context.rb index c5d473852d4471..737f4112002a62 100644 --- a/lib/bulk_imports/pipeline/context.rb +++ b/lib/bulk_imports/pipeline/context.rb @@ -44,6 +44,14 @@ def configuration @configuration ||= bulk_import.configuration end + def source_user_mapper + @source_user_mapper ||= Gitlab::Import::SourceUserMapper.new( + namespace: portable.root_ancestor, + import_type: Import::SOURCE_DIRECT_TRANSFER, + source_hostname: configuration.source_hostname + ) + end + def importer_user_mapping_enabled? Import::BulkImports::EphemeralData.new(bulk_import_id).importer_user_mapping_enabled? end diff --git a/lib/import/bulk_imports/source_users_mapper.rb b/lib/import/bulk_imports/source_users_mapper.rb index 90a0ab19c31efd..801f796f6e02f0 100644 --- a/lib/import/bulk_imports/source_users_mapper.rb +++ b/lib/import/bulk_imports/source_users_mapper.rb @@ -44,13 +44,7 @@ def include?(_user_identifier) attr_reader :context - def source_user_mapper - @source_user_mapper ||= Gitlab::Import::SourceUserMapper.new( - namespace: context.portable.root_ancestor, - import_type: SOURCE_DIRECT_TRANSFER, - source_hostname: context.configuration.source_hostname - ) - end + delegate :source_user_mapper, to: :context end end end diff --git a/spec/lib/bulk_imports/ndjson_pipeline_spec.rb b/spec/lib/bulk_imports/ndjson_pipeline_spec.rb index be63a301705b0d..29f2a208d8238c 100644 --- a/spec/lib/bulk_imports/ndjson_pipeline_spec.rb +++ b/spec/lib/bulk_imports/ndjson_pipeline_spec.rb @@ -3,6 +3,7 @@ require 'spec_helper' RSpec.describe BulkImports::NdjsonPipeline, feature_category: :importers do + let_it_be(:default_organization) { create(:organization, :default) } let_it_be(:group) { create(:group) } let_it_be(:project) { create(:project) } let_it_be(:user) { create(:user) } @@ -153,7 +154,7 @@ def initialize(portable, user, context) } expect { subject.deep_transform_relation!(relation_hash, 'test', relation_definition) { |a, _b| a } } - .to change { Import::SourceUser.count }.by(3).and change { User.count }.by(1) + .to change { Import::SourceUser.count }.by(3).and change { User.count }.by(3) expect(Import::SourceUser.pluck(:source_user_identifier)).to match_array(%w[101 102 103 104 105 106]) expect(User.last.placeholder?).to eq(true) end diff --git a/spec/lib/bulk_imports/pipeline/context_spec.rb b/spec/lib/bulk_imports/pipeline/context_spec.rb index b9fb498e291ca3..d73a3d4db005e9 100644 --- a/spec/lib/bulk_imports/pipeline/context_spec.rb +++ b/spec/lib/bulk_imports/pipeline/context_spec.rb @@ -5,7 +5,7 @@ RSpec.describe BulkImports::Pipeline::Context, feature_category: :importers do let_it_be(:user) { create(:user) } let_it_be(:group) { create(:group) } - let_it_be(:bulk_import) { create(:bulk_import, user: user) } + let_it_be(:bulk_import) { create(:bulk_import, :with_configuration, user: user) } let_it_be(:project) { create(:project) } let_it_be(:project_entity) { create(:bulk_import_entity, :project_entity, project: project) } let_it_be(:project_tracker) { create(:bulk_import_tracker, entity: project_entity) } @@ -75,6 +75,20 @@ end end + describe '#source_user_mapper' do + it { expect(subject.source_user_mapper).to be_instance_of(Gitlab::Import::SourceUserMapper) } + + it 'builds with the correct arguments' do + expect(Gitlab::Import::SourceUserMapper).to receive(:new).with( + namespace: group.root_ancestor, + import_type: Import::SOURCE_DIRECT_TRANSFER, + source_hostname: bulk_import.configuration.source_hostname + ) + + subject.source_user_mapper + end + end + describe '#importer_user_mapping_enabled?' do subject { described_class.new(tracker, extra: :data).importer_user_mapping_enabled? } diff --git a/spec/lib/import/bulk_imports/source_users_mapper_spec.rb b/spec/lib/import/bulk_imports/source_users_mapper_spec.rb index 4c67b4374ee2a0..7d8d9a0931e253 100644 --- a/spec/lib/import/bulk_imports/source_users_mapper_spec.rb +++ b/spec/lib/import/bulk_imports/source_users_mapper_spec.rb @@ -4,16 +4,26 @@ RSpec.describe Import::BulkImports::SourceUsersMapper, feature_category: :importers do let_it_be(:portable) { create(:group) } - let(:configuration) { instance_double(BulkImports::Configuration, source_hostname: 'example.com') } - let(:context) do - instance_double(BulkImports::Pipeline::Context, portable: portable, configuration: configuration) + let_it_be(:bulk_import) { create(:bulk_import, :with_configuration) } + let_it_be(:entity) do + create( + :bulk_import_entity, + group: portable, + bulk_import: bulk_import, + source_full_path: 'source/full/path', + destination_slug: 'My-Destination-Group', + destination_namespace: portable.full_path + ) end + let_it_be(:tracker) { create(:bulk_import_tracker, entity: entity) } + let_it_be(:context) { BulkImports::Pipeline::Context.new(tracker) } + let_it_be(:import_source_user_1) do create(:import_source_user, namespace: portable, import_type: Import::SOURCE_DIRECT_TRANSFER, - source_hostname: 'example.com', + source_hostname: bulk_import.configuration.source_hostname, source_user_identifier: 101 ) end @@ -23,7 +33,7 @@ :completed, namespace: portable, import_type: Import::SOURCE_DIRECT_TRANSFER, - source_hostname: 'example.com', + source_hostname: bulk_import.configuration.source_hostname, source_user_identifier: 102 ) end -- GitLab