Merge branch '43270-import-with-milestones-failing' into 'master'

Resolve "Import with Milestones Failing"

Closes #43270 and #47626

See merge request gitlab-org/gitlab-ce!19961

changelogs/unreleased/43270-import-with-milestones-failing.yml

+---
+title: Fix label and milestone duplicated records and IID errors
+merge_request: 19961
+author:
+type: fixed

lib/gitlab/import_export/group_project_object_builder.rb

+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

lib/gitlab/import_export/project_tree_restorer.rb

 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

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

spec/lib/gitlab/import_export/group_project_object_builder_spec.rb

+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

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",

spec/lib/gitlab/import_export/project.milestone-iid.json

+{
+  "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": []
+}

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