diff --git a/changelogs/unreleased/43270-import-with-milestones-failing.yml b/changelogs/unreleased/43270-import-with-milestones-failing.yml new file mode 100644 index 0000000000000000000000000000000000000000..13bf8072376e0d008aefaae3458043d407b78d80 --- /dev/null +++ b/changelogs/unreleased/43270-import-with-milestones-failing.yml @@ -0,0 +1,5 @@ +--- +title: Fix label and milestone duplicated records and IID errors +merge_request: 19961 +author: +type: fixed diff --git a/lib/gitlab/import_export/group_project_object_builder.rb b/lib/gitlab/import_export/group_project_object_builder.rb new file mode 100644 index 0000000000000000000000000000000000000000..6c2af77011984042ac63c2b52387f78eb9f26513 --- /dev/null +++ b/lib/gitlab/import_export/group_project_object_builder.rb @@ -0,0 +1,90 @@ +module Gitlab + module ImportExport + # Given a class, it finds or creates a new object + # (initializes in the case of Label) at group or project level. + # If it does not exist in the group, it creates it at project level. + # + # Example: + # `GroupProjectObjectBuilder.build(Label, label_attributes)` + # finds or initializes a label with the given attributes. + # + # It also adds some logic around Group Labels/Milestones for edge cases. + class GroupProjectObjectBuilder + def self.build(*args) + Project.transaction do + new(*args).find + end + end + + def initialize(klass, attributes) + @klass = klass < Label ? Label : klass + @attributes = attributes + @group = @attributes['group'] + @project = @attributes['project'] + end + + def find + find_object || @klass.create(project_attributes) + end + + private + + def find_object + @klass.where(where_clause).first + end + + def where_clause + @attributes.slice('title').map do |key, value| + scope_clause = table[:project_id].eq(@project.id) + scope_clause = scope_clause.or(table[:group_id].eq(@group.id)) if @group + + table[key].eq(value).and(scope_clause) + end.reduce(:or) + end + + def table + @table ||= @klass.arel_table + end + + def project_attributes + @attributes.except('group').tap do |atts| + if label? + atts['type'] = 'ProjectLabel' # Always create project labels + elsif milestone? + if atts['group_id'] # Transform new group milestones into project ones + atts['iid'] = nil + atts.delete('group_id') + else + claim_iid + end + end + end + end + + def label? + @klass == Label + end + + def milestone? + @klass == Milestone + end + + # If an existing group milestone used the IID + # claim the IID back and set the group milestone to use one available + # This is necessary to fix situations like the following: + # - Importing into a user namespace project with exported group milestones + # where the IID of the Group milestone could conflict with a project one. + def claim_iid + # The milestone has to be a group milestone, as it's the only case where + # we set the IID as the maximum. The rest of them are fixed. + milestone = @project.milestones.find_by(iid: @attributes['iid']) + + return unless milestone + + milestone.iid = nil + milestone.ensure_project_iid! + milestone.save! + end + end + end +end diff --git a/lib/gitlab/import_export/project_tree_restorer.rb b/lib/gitlab/import_export/project_tree_restorer.rb index 4eb67fbe11e629f6d2341c766c9648c8df96c0ea..76b99b1de162ed41886a90720b520f77f269a50d 100644 --- a/lib/gitlab/import_export/project_tree_restorer.rb +++ b/lib/gitlab/import_export/project_tree_restorer.rb @@ -1,8 +1,8 @@ module Gitlab module ImportExport class ProjectTreeRestorer - # Relations which cannot have both group_id and project_id at the same time - RESTRICT_PROJECT_AND_GROUP = %i(milestone milestones).freeze + # Relations which cannot be saved at project level (and have a group assigned) + GROUP_MODELS = [GroupLabel, Milestone].freeze def initialize(user:, shared:, project:) @path = File.join(shared.export_path, 'project.json') @@ -70,12 +70,23 @@ module Gitlab def save_relation_hash(relation_hash_batch, relation_key) relation_hash = create_relation(relation_key, relation_hash_batch) + remove_group_models(relation_hash) if relation_hash.is_a?(Array) + @saved = false unless restored_project.append_or_update_attribute(relation_key, relation_hash) # Restore the project again, extra query that skips holding the AR objects in memory @restored_project = Project.find(@project_id) end + # Remove project models that became group models as we found them at group level. + # This no longer required saving them at the root project level. + # For example, in the case of an existing group label that matched the title. + def remove_group_models(relation_hash) + relation_hash.reject! do |value| + GROUP_MODELS.include?(value.class) && value.group_id + end + end + def default_relation_list reader.tree.reject do |model| model.is_a?(Hash) && model[:project_members] @@ -170,7 +181,7 @@ module Gitlab def create_relation(relation, relation_hash_list) relation_array = [relation_hash_list].flatten.map do |relation_hash| Gitlab::ImportExport::RelationFactory.create(relation_sym: relation.to_sym, - relation_hash: parsed_relation_hash(relation_hash, relation.to_sym), + relation_hash: relation_hash, members_mapper: members_mapper, user: @user, project: @restored_project, @@ -180,18 +191,6 @@ module Gitlab relation_hash_list.is_a?(Array) ? relation_array : relation_array.first end - def parsed_relation_hash(relation_hash, relation_type) - if RESTRICT_PROJECT_AND_GROUP.include?(relation_type) - params = {} - params['group_id'] = restored_project.group.try(:id) if relation_hash['group_id'] - params['project_id'] = restored_project.id if relation_hash['project_id'] - else - params = { 'group_id' => restored_project.group.try(:id), 'project_id' => restored_project.id } - end - - relation_hash.merge(params) - end - def reader @reader ||= Gitlab::ImportExport::Reader.new(shared: @shared) end diff --git a/lib/gitlab/import_export/relation_factory.rb b/lib/gitlab/import_export/relation_factory.rb index c5cf290f1919e3853fa737c7d4addd3b6e5920f6..091e81028bb66a4209e3ab6a006903d8f992d991 100644 --- a/lib/gitlab/import_export/relation_factory.rb +++ b/lib/gitlab/import_export/relation_factory.rb @@ -54,6 +54,8 @@ module Gitlab @project = project @imported_object_retries = 0 + @relation_hash['project_id'] = @project.id + # Remove excluded keys from relation_hash # We don't do this in the parsed_relation_hash because of the 'transformed attributes' # For example, MergeRequestDiffFiles exports its diff attribute as utf8_diff. Then, @@ -80,15 +82,12 @@ module Gitlab case @relation_name when :merge_request_diff_files then setup_diff when :notes then setup_note - when :project_label, :project_labels then setup_label - when :milestone, :milestones then setup_milestone when 'Ci::Pipeline' then setup_pipeline - else - @relation_hash['project_id'] = @project.id end update_user_references update_project_references + update_group_references remove_duplicate_assignees reset_tokens! @@ -151,39 +150,23 @@ module Gitlab end def update_project_references - project_id = @relation_hash.delete('project_id') - # If source and target are the same, populate them with the new project ID. if @relation_hash['source_project_id'] - @relation_hash['source_project_id'] = same_source_and_target? ? project_id : MergeRequestParser::FORKED_PROJECT_ID + @relation_hash['source_project_id'] = same_source_and_target? ? @relation_hash['project_id'] : MergeRequestParser::FORKED_PROJECT_ID end - # project_id may not be part of the export, but we always need to populate it if required. - @relation_hash['project_id'] = project_id - @relation_hash['target_project_id'] = project_id if @relation_hash['target_project_id'] + @relation_hash['target_project_id'] = @relation_hash['project_id'] if @relation_hash['target_project_id'] end def same_source_and_target? @relation_hash['target_project_id'] && @relation_hash['target_project_id'] == @relation_hash['source_project_id'] end - def setup_label - # If there's no group, move the label to a project label - if @relation_hash['type'] == 'GroupLabel' && @relation_hash['group_id'] - @relation_hash['project_id'] = nil - @relation_name = :group_label - else - @relation_hash['group_id'] = nil - @relation_hash['type'] = 'ProjectLabel' - end - end + def update_group_references + return unless EXISTING_OBJECT_CHECK.include?(@relation_name) + return unless @relation_hash['group_id'] - def setup_milestone - if @relation_hash['group_id'] - @relation_hash['group_id'] = @project.group.id - else - @relation_hash['project_id'] = @project.id - end + @relation_hash['group_id'] = @project.group&.id end def reset_tokens! @@ -271,15 +254,7 @@ module Gitlab end def existing_object - @existing_object ||= - begin - existing_object = find_or_create_object! - - # Done in two steps, as MySQL behaves differently than PostgreSQL using - # the +find_or_create_by+ method and does not return the ID the second time. - existing_object.update!(parsed_relation_hash) - existing_object - end + @existing_object ||= find_or_create_object! end def unknown_service? @@ -288,29 +263,16 @@ module Gitlab end def find_or_create_object! - finder_attributes = if @relation_name == :group_label - %w[title group_id] - elsif parsed_relation_hash['project_id'] - %w[title project_id] - else - %w[title group_id] - end - - finder_hash = parsed_relation_hash.slice(*finder_attributes) - - if label? - label = relation_class.find_or_initialize_by(finder_hash) - parsed_relation_hash.delete('priorities') if label.persisted? - - label.save! - label - else - relation_class.find_or_create_by(finder_hash) + return relation_class.find_or_create_by(project_id: @project.id) if @relation_name == :project_feature + + # Can't use IDs as validation exists calling `group` or `project` attributes + finder_hash = parsed_relation_hash.tap do |hash| + hash['group'] = @project.group if relation_class.attribute_method?('group_id') + hash['project'] = @project + hash.delete('project_id') end - end - def label? - @relation_name.to_s.include?('label') + GroupProjectObjectBuilder.build(relation_class, finder_hash) end end end diff --git a/spec/lib/gitlab/import_export/group_project_object_builder_spec.rb b/spec/lib/gitlab/import_export/group_project_object_builder_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..6a803c48b341de0dcf74632eef3274289bf2ce66 --- /dev/null +++ b/spec/lib/gitlab/import_export/group_project_object_builder_spec.rb @@ -0,0 +1,52 @@ +require 'spec_helper' + +describe Gitlab::ImportExport::GroupProjectObjectBuilder do + let(:project) do + create(:project, + :builds_disabled, + :issues_disabled, + name: 'project', + path: 'project', + group: create(:group)) + end + + context 'labels' do + it 'finds the right group label' do + group_label = create(:group_label, 'name': 'group label', 'group': project.group) + + expect(described_class.build(Label, + 'title' => 'group label', + 'project' => project, + 'group' => project.group)).to eq(group_label) + end + + it 'creates a new label' do + label = described_class.build(Label, + 'title' => 'group label', + 'project' => project, + 'group' => project.group) + + expect(label.persisted?).to be true + end + end + + context 'milestones' do + it 'finds the right group milestone' do + milestone = create(:milestone, 'name' => 'group milestone', 'group' => project.group) + + expect(described_class.build(Milestone, + 'title' => 'group milestone', + 'project' => project, + 'group' => project.group)).to eq(milestone) + end + + it 'creates a new milestone' do + milestone = described_class.build(Milestone, + 'title' => 'group milestone', + 'project' => project, + 'group' => project.group) + + expect(milestone.persisted?).to be true + end + end +end diff --git a/spec/lib/gitlab/import_export/project.light.json b/spec/lib/gitlab/import_export/project.light.json index c13cf4a050752dcf6906be90dc45ee92a6e80ce4..ba2248073f5877ef0db10e2ef06b99214639572e 100644 --- a/spec/lib/gitlab/import_export/project.light.json +++ b/spec/lib/gitlab/import_export/project.light.json @@ -7,7 +7,7 @@ "milestones": [ { "id": 1, - "title": "Project milestone", + "title": "A milestone", "project_id": 8, "description": "Project-level milestone", "due_date": null, @@ -66,8 +66,8 @@ "group_milestone_id": null, "milestone": { "id": 1, - "title": "Project milestone", - "project_id": 8, + "title": "A milestone", + "group_id": 8, "description": "Project-level milestone", "due_date": null, "created_at": "2016-06-14T15:02:04.415Z", @@ -86,7 +86,7 @@ "updated_at": "2017-08-15T18:37:40.795Z", "label": { "id": 6, - "title": "Another project label", + "title": "Another label", "color": "#A8D695", "project_id": null, "created_at": "2017-08-15T18:37:19.698Z", diff --git a/spec/lib/gitlab/import_export/project.milestone-iid.json b/spec/lib/gitlab/import_export/project.milestone-iid.json new file mode 100644 index 0000000000000000000000000000000000000000..b028147b5eb73033218e87d2637843c9190f2510 --- /dev/null +++ b/spec/lib/gitlab/import_export/project.milestone-iid.json @@ -0,0 +1,80 @@ +{ + "description": "Nisi et repellendus ut enim quo accusamus vel magnam.", + "import_type": "gitlab_project", + "creator_id": 123, + "visibility_level": 10, + "archived": false, + "issues": [ + { + "id": 1, + "title": "Fugiat est minima quae maxime non similique.", + "assignee_id": null, + "project_id": 8, + "author_id": 1, + "created_at": "2017-07-07T18:13:01.138Z", + "updated_at": "2017-08-15T18:37:40.807Z", + "branch_name": null, + "description": "Quam totam fuga numquam in eveniet.", + "state": "opened", + "iid": 20, + "updated_by_id": 1, + "confidential": false, + "due_date": null, + "moved_to_id": null, + "lock_version": null, + "time_estimate": 0, + "closed_at": null, + "last_edited_at": null, + "last_edited_by_id": null, + "group_milestone_id": null, + "milestone": { + "id": 1, + "title": "Group-level milestone", + "description": "Group-level milestone", + "due_date": null, + "created_at": "2016-06-14T15:02:04.415Z", + "updated_at": "2016-06-14T15:02:04.415Z", + "state": "active", + "iid": 1, + "group_id": 8 + } + }, + { + "id": 2, + "title": "est minima quae maxime non similique.", + "assignee_id": null, + "project_id": 8, + "author_id": 1, + "created_at": "2017-07-07T18:13:01.138Z", + "updated_at": "2017-08-15T18:37:40.807Z", + "branch_name": null, + "description": "Quam totam fuga numquam in eveniet.", + "state": "opened", + "iid": 21, + "updated_by_id": 1, + "confidential": false, + "due_date": null, + "moved_to_id": null, + "lock_version": null, + "time_estimate": 0, + "closed_at": null, + "last_edited_at": null, + "last_edited_by_id": null, + "group_milestone_id": null, + "milestone": { + "id": 2, + "title": "Another milestone", + "project_id": 8, + "description": "milestone", + "due_date": null, + "created_at": "2016-06-14T15:02:04.415Z", + "updated_at": "2016-06-14T15:02:04.415Z", + "state": "active", + "iid": 1, + "group_id": null + } + } + ], + "snippets": [], + "hooks": [] +} diff --git a/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb b/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb index 68ddc947e0209a76c4bde63510be150b3d2d5d70..bac5693c8301225d2aaedc8c8324a1bb8e9b6e48 100644 --- a/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb +++ b/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb @@ -189,8 +189,8 @@ describe Gitlab::ImportExport::ProjectTreeRestorer do @project.pipelines.zip([2, 2, 2, 2, 2]) .each do |(pipeline, expected_status_size)| - expect(pipeline.statuses.size).to eq(expected_status_size) - end + expect(pipeline.statuses.size).to eq(expected_status_size) + end end end @@ -246,13 +246,6 @@ describe Gitlab::ImportExport::ProjectTreeRestorer do expect(project.issues.size).to eq(results.fetch(:issues, 0)) end - it 'has issue with group label and project label' do - labels = project.issues.first.labels - - expect(labels.where(type: "ProjectLabel").count).to eq(results.fetch(:first_issue_labels, 0)) - expect(labels.where(type: "ProjectLabel").where.not(group_id: nil).count).to eq(0) - end - it 'does not set params that are excluded from import_export settings' do expect(project.import_type).to be_nil expect(project.creator_id).not_to eq 123 @@ -268,12 +261,6 @@ describe Gitlab::ImportExport::ProjectTreeRestorer do it 'has group milestone' do expect(project.group.milestones.size).to eq(results.fetch(:milestones, 0)) end - - it 'has issue with group label' do - labels = project.issues.first.labels - - expect(labels.where(type: "GroupLabel").count).to eq(results.fetch(:first_issue_labels, 0)) - end end context 'Light JSON' do @@ -360,13 +347,72 @@ describe Gitlab::ImportExport::ProjectTreeRestorer do it_behaves_like 'restores project correctly', issues: 2, labels: 1, - milestones: 1, + milestones: 2, first_issue_labels: 1 it_behaves_like 'restores group correctly', - labels: 1, - milestones: 1, + labels: 0, + milestones: 0, first_issue_labels: 1 end + + context 'with existing group models' do + let!(:project) do + create(:project, + :builds_disabled, + :issues_disabled, + name: 'project', + path: 'project', + group: create(:group)) + end + + before do + project_tree_restorer.instance_variable_set(:@path, "spec/lib/gitlab/import_export/project.light.json") + end + + it 'imports labels' do + create(:group_label, name: 'Another label', group: project.group) + + expect_any_instance_of(Gitlab::ImportExport::Shared).not_to receive(:error) + + restored_project_json + + expect(project.labels.count).to eq(1) + end + + it 'imports milestones' do + create(:milestone, name: 'A milestone', group: project.group) + + expect_any_instance_of(Gitlab::ImportExport::Shared).not_to receive(:error) + + restored_project_json + + expect(project.group.milestones.count).to eq(1) + expect(project.milestones.count).to eq(0) + end + end + + context 'with clashing milestones on IID' do + let!(:project) do + create(:project, + :builds_disabled, + :issues_disabled, + name: 'project', + path: 'project', + group: create(:group)) + end + + it 'preserves the project milestone IID' do + project_tree_restorer.instance_variable_set(:@path, "spec/lib/gitlab/import_export/project.milestone-iid.json") + + expect_any_instance_of(Gitlab::ImportExport::Shared).not_to receive(:error) + + restored_project_json + + expect(project.milestones.count).to eq(2) + expect(Milestone.find_by_title('Another milestone').iid).to eq(1) + expect(Milestone.find_by_title('Group-level milestone').iid).to eq(2) + end + end end end