Merge branch 'reset-relative-position-on-import' into 'master'

Re-compute issue relative position on project import

See merge request gitlab-org/gitlab!59175

changelogs/unreleased/reset-relative-position-on-import.yml

+---
+title: Export issues sorted by relative position and recompute issue relative position
+  on project import
+merge_request: 59175
+author:
+type: changed

db/post_migrate/20210429143649_add_new_issues_index_for_relative_position.rb

+# frozen_string_literal: true
+
+class AddNewIssuesIndexForRelativePosition < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  disable_ddl_transaction!
+
+  INDEX_NAME = 'idx_issues_on_project_id_and_rel_asc_and_id'
+
+  def up
+    add_concurrent_index :issues, [:project_id, :relative_position, :id], name: INDEX_NAME
+  end
+
+  def down
+    remove_concurrent_index_by_name(:issues, INDEX_NAME)
+  end
+end

db/schema_migrations/20210429143649

+949e1323d1fadd8db3b8b337f3071ab7b7a7c45b11dc40924fb64c074596a327
\ No newline at end of file

db/structure.sql

@@ -21970,6 +21970,8 @@ CREATE INDEX idx_issues_on_project_id_and_created_at_and_id_and_state_id ON issu
 
 CREATE INDEX idx_issues_on_project_id_and_due_date_and_id_and_state_id ON issues USING btree (project_id, due_date, id, state_id) WHERE (due_date IS NOT NULL);
 
+CREATE INDEX idx_issues_on_project_id_and_rel_asc_and_id ON issues USING btree (project_id, relative_position, id);
+
 CREATE INDEX idx_issues_on_project_id_and_rel_position_and_state_id_and_id ON issues USING btree (project_id, relative_position, state_id, id DESC);
 
 CREATE INDEX idx_issues_on_project_id_and_updated_at_and_id_and_state_id ON issues USING btree (project_id, updated_at, id, state_id);

doc/development/import_export.md

@@ -284,6 +284,27 @@ methods:
     - :type
 ```
 
+Customize the export order of the model relationships:
+
+```yaml
+# Specify a custom export reordering for a given relationship
+# For example for issues we use a custom export reordering by relative_position, so that on import, we can reset the
+# relative position value, but still keep the issues order to the order in which issues were in the exported project.
+# By default the ordering of relations is done by PK.
+# column - specify the column by which to reorder, by default it is relation's PK
+# direction - specify the ordering direction :asc or :desc, default :asc
+# nulls_position - specify where would null values be positioned. Because custom ordering column can contain nulls we
+#                  need to also specify where would the nulls be placed. It can be :nulls_last or :nulls_first, defaults
+#                  to :nulls_last
+
+export_reorders:
+  project:
+    issues:
+      column: :relative_position
+      direction: :asc
+      nulls_position: :nulls_last
+```
+
 ### Import
 
 The import job status moves from `none` to `finished` or `failed` into different states:

ee/spec/lib/gitlab/import_export/group/relation_factory_spec.rb

@@ -12,6 +12,7 @@ RSpec.describe Gitlab::ImportExport::Group::RelationFactory do
     described_class.create(
       relation_sym: relation_sym,
       relation_hash: relation_hash,
+      relation_index: 1,
       members_mapper: members_mapper,
       object_builder: Gitlab::ImportExport::Group::ObjectBuilder,
       user: importer_user,

lib/gitlab/import_export/attributes_finder.rb

@@ -3,7 +3,7 @@
 module Gitlab
   module ImportExport
     class AttributesFinder
-      attr_reader :tree, :included_attributes, :excluded_attributes, :methods, :preloads
+      attr_reader :tree, :included_attributes, :excluded_attributes, :methods, :preloads, :export_reorders
 
       def initialize(config:)
         @tree = config[:tree] || {}
@@ -11,6 +11,7 @@ module Gitlab
         @excluded_attributes = config[:excluded_attributes] || {}
         @methods = config[:methods] || {}
         @preloads = config[:preloads] || {}
+        @export_reorders = config[:export_reorders] || {}
       end
 
       def find_root(model_key)
@@ -33,7 +34,8 @@ module Gitlab
           except: @excluded_attributes[model_key],
           methods: @methods[model_key],
           include: resolve_model_tree(model_tree),
-          preload: resolve_preloads(model_key, model_tree)
+          preload: resolve_preloads(model_key, model_tree),
+          export_reorder: @export_reorders[model_key]
         }.compact
       end
 

lib/gitlab/import_export/base/relation_factory.rb

@@ -44,8 +44,9 @@ module Gitlab
           relation_name.to_s.constantize
         end
 
-        def initialize(relation_sym:, relation_hash:, members_mapper:, object_builder:, user:, importable:, excluded_keys: [])
+        def initialize(relation_sym:, relation_index:, relation_hash:, members_mapper:, object_builder:, user:, importable:, excluded_keys: [])
           @relation_name = self.class.overrides[relation_sym]&.to_sym || relation_sym
+          @relation_index = relation_index
           @relation_hash = relation_hash.except('noteable_id')
           @members_mapper = members_mapper
           @object_builder = object_builder

lib/gitlab/import_export/group/import_export.yml

@@ -58,6 +58,8 @@ methods:
 
 preloads:
 
+export_reorders:
+
 # EE specific relationships and settings to include. All of this will be merged
 # into the previous structures if EE is used.
 ee:

lib/gitlab/import_export/group/legacy_import_export.yml

@@ -60,6 +60,8 @@ methods:
 
 preloads:
 
+export_reorders:
+
 # EE specific relationships and settings to include. All of this will be merged
 # into the previous structures if EE is used.
 ee:

lib/gitlab/import_export/json/streaming_serializer.rb

@@ -67,14 +67,9 @@ module Gitlab
         def serialize_many_relations(key, records, options)
           enumerator = Enumerator.new do |items|
             key_preloads = preloads&.dig(key)
-            records = records.preload(key_preloads) if key_preloads
 
-            records.in_batches(of: batch_size) do |batch| # rubocop:disable Cop/InBatches
-              # order each batch by its primary key to ensure
-              # consistent and predictable ordering of each exported relation
-              # as additional `WHERE` clauses can impact the order in which data is being
-              # returned by database when no `ORDER` is specified
-              batch = batch.reorder(batch.klass.primary_key)
+            batch(records, key) do |batch|
+              batch = batch.preload(key_preloads) if key_preloads
 
               batch.each do |record|
                 items << Raw.new(record.to_json(options))
@@ -85,6 +80,29 @@ module Gitlab
           json_writer.write_relation_array(@exportable_path, key, enumerator)
         end
 
+        def batch(relation, key)
+          opts = { of: batch_size }
+          order_by = reorders(relation, key)
+
+          # we need to sort issues by non primary key column(relative_position)
+          # and `in_batches` does not support that
+          if order_by
+            scope = relation.reorder(order_by)
+
+            Gitlab::Pagination::Keyset::Iterator.new(scope: scope, use_union_optimization: true).each_batch(**opts) do |batch|
+              yield batch
+            end
+          else
+            relation.in_batches(**opts) do |batch| # rubocop:disable Cop/InBatches
+              # order each batch by its primary key to ensure
+              # consistent and predictable ordering of each exported relation
+              # as additional `WHERE` clauses can impact the order in which data is being
+              # returned by database when no `ORDER` is specified
+              yield batch.reorder(batch.klass.primary_key)
+            end
+          end
+        end
+
         def serialize_many_each(key, records, options)
           enumerator = Enumerator.new do |items|
             records.each do |record|
@@ -112,6 +130,42 @@ module Gitlab
         def batch_size
           @batch_size ||= self.class.batch_size(@exportable)
         end
+
+        def reorders(relation, key)
+          export_reorder = relations_schema[:export_reorder]&.dig(key)
+          return unless export_reorder
+
+          custom_reorder(relation.klass, export_reorder)
+        end
+
+        def custom_reorder(klass, order_by)
+          arel_table = klass.arel_table
+          column = order_by[:column] || klass.primary_key
+          direction = order_by[:direction] || :asc
+          nulls_position = order_by[:nulls_position] || :nulls_last
+
+          arel_order_classes = ::Gitlab::Pagination::Keyset::ColumnOrderDefinition::AREL_ORDER_CLASSES.invert
+          reverse_direction = ::Gitlab::Pagination::Keyset::ColumnOrderDefinition::REVERSED_ORDER_DIRECTIONS[direction]
+          reverse_nulls_position = ::Gitlab::Pagination::Keyset::ColumnOrderDefinition::REVERSED_NULL_POSITIONS[nulls_position]
+          order_expression = ::Gitlab::Database.nulls_order(column, direction, nulls_position)
+          reverse_order_expression = ::Gitlab::Database.nulls_order(column, reverse_direction, reverse_nulls_position)
+
+          ::Gitlab::Pagination::Keyset::Order.build([
+            ::Gitlab::Pagination::Keyset::ColumnOrderDefinition.new(
+              attribute_name: column,
+              column_expression: arel_table[column],
+              order_expression: order_expression,
+              reversed_order_expression: reverse_order_expression,
+              order_direction: direction,
+              nullable: nulls_position,
+              distinct: false
+            ),
+            ::Gitlab::Pagination::Keyset::ColumnOrderDefinition.new(
+              attribute_name: klass.primary_key,
+              order_expression: arel_order_classes[direction].new(arel_table[klass.primary_key.to_sym])
+            )
+          ])
+        end
       end
     end
   end

lib/gitlab/import_export/project/import_export.yml

@@ -395,6 +395,8 @@ methods:
     - :state
 
 preloads:
+  issues:
+    project: :route
   statuses:
     # TODO: We cannot preload tags, as they are not part of `GenericCommitStatus`
     # tags: # needed by tag_list
@@ -404,6 +406,29 @@ preloads:
     target_project: # needed by target_branch_sha
     assignees: # needed by assigne_id that is implemented by DeprecatedAssignee
 
+# Specify a custom export reordering for a given relationship
+# For example for issues we use a custom export reordering by relative_position, so that on import, we can reset the
+# relative position value, but still keep the issues order to the order in which issues were in the exported project.
+# By default the ordering of relations is done by PK.
+# column - specify the column by which to reorder, by default it is relation's PK
+# direction - specify the ordering direction :asc or :desc, default :asc
+# nulls_position - specify where would null values be positioned. Because custom ordering column can contain nulls we
+#                  need to also specify where would the nulls be placed. It can be :nulls_last or :nulls_first, defaults
+#                  to :nulls_last
+# Example:
+# export_reorders:
+#   project:
+#     issues:
+#       column: :relative_position
+#       direction: :asc
+#       nulls_position: :nulls_last
+export_reorders:
+  project:
+    issues:
+      column: :relative_position
+      direction: :asc
+      nulls_position: :nulls_last
+
 # EE specific relationships and settings to include. All of this will be merged
 # into the previous structures if EE is used.
 ee:

lib/gitlab/import_export/project/relation_factory.rb

@@ -80,6 +80,7 @@ module Gitlab
           when :notes then setup_note
           when :'Ci::Pipeline' then setup_pipeline
           when *BUILD_MODELS then setup_build
+          when :issues then setup_issue
           end
 
           update_project_references
@@ -135,6 +136,22 @@ module Gitlab
           end
         end
 
+        def setup_issue
+          @relation_hash['relative_position'] = compute_relative_position
+        end
+
+        def compute_relative_position
+          return unless max_relative_position
+
+          max_relative_position + (@relation_index + 1) * Gitlab::RelativePositioning::IDEAL_DISTANCE
+        end
+
+        def max_relative_position
+          Rails.cache.fetch("import:#{@importable.model_name.plural}:#{@importable.id}:hierarchy_max_issues_relative_position", expires_in: 24.hours) do
+            ::RelativePositioning.mover.context(Issue.in_projects(@importable.root_ancestor.all_projects).first)&.max_relative_position || ::Gitlab::RelativePositioning::START_POSITION
+          end
+        end
+
         def legacy_trigger?
           @relation_name == :'Ci::Trigger' && @relation_hash['owner_id'].nil?
         end

lib/gitlab/import_export/relation_tree_restorer.rb

@@ -155,7 +155,7 @@ module Gitlab
           transform_sub_relations!(data_hash, sub_relation_key, sub_relation_definition, relation_index)
         end
 
-        relation = @relation_factory.create(**relation_factory_params(relation_key, data_hash))
+        relation = @relation_factory.create(**relation_factory_params(relation_key, relation_index, data_hash))
 
         if relation && !relation.valid?
           @shared.logger.warn(
@@ -221,8 +221,9 @@ module Gitlab
         importable_class.to_s.downcase.to_sym
       end
 
-      def relation_factory_params(relation_key, data_hash)
+      def relation_factory_params(relation_key, relation_index, data_hash)
         {
+          relation_index: relation_index,
           relation_sym: relation_key.to_sym,
           relation_hash: data_hash,
           importable: @importable,

spec/lib/gitlab/import_export/base/relation_factory_spec.rb

@@ -13,6 +13,7 @@ RSpec.describe Gitlab::ImportExport::Base::RelationFactory do
   subject do
     described_class.create(relation_sym: relation_sym,
                            relation_hash: relation_hash,
+                           relation_index: 1,
                            object_builder: Gitlab::ImportExport::Project::ObjectBuilder,
                            members_mapper: members_mapper,
                            user: user,

spec/lib/gitlab/import_export/config_spec.rb

@@ -25,7 +25,7 @@ RSpec.describe Gitlab::ImportExport::Config do
           expect { subject }.not_to raise_error
           expect(subject).to be_a(Hash)
           expect(subject.keys).to contain_exactly(
-            :tree, :excluded_attributes, :included_attributes, :methods, :preloads)
+            :tree, :excluded_attributes, :included_attributes, :methods, :preloads, :export_reorders)
         end
       end
     end

spec/lib/gitlab/import_export/group/relation_factory_spec.rb

@@ -12,6 +12,7 @@ RSpec.describe Gitlab::ImportExport::Group::RelationFactory do
     described_class.create(
       relation_sym: relation_sym,
       relation_hash: relation_hash,
+      relation_index: 1,
       members_mapper: members_mapper,
       object_builder: Gitlab::ImportExport::Group::ObjectBuilder,
       user: importer_user,

spec/lib/gitlab/import_export/json/streaming_serializer_spec.rb

@@ -30,12 +30,14 @@ RSpec.describe Gitlab::ImportExport::JSON::StreamingSerializer do
   let(:json_writer) { instance_double('Gitlab::ImportExport::JSON::LegacyWriter') }
   let(:hash) { { name: exportable.name, description: exportable.description }.stringify_keys }
   let(:include) { [] }
+  let(:custom_orderer) { nil }
 
   let(:relations_schema) do
     {
       only: [:name, :description],
       include: include,
-      preload: { issues: nil }
+      preload: { issues: nil },
+      export_reorder: custom_orderer
     }
   end
 
@@ -57,19 +59,63 @@ RSpec.describe Gitlab::ImportExport::JSON::StreamingSerializer do
         [{ issues: { include: [] } }]
       end
 
+      before do
+        create_list(:issue, 3, project: exportable, relative_position: 10000) # ascending ids, same position positive
+        create_list(:issue, 3, project: exportable, relative_position: -5000) # ascending ids, same position negative
+        create_list(:issue, 3, project: exportable, relative_position: 0) # ascending ids, duplicate positions
+        create_list(:issue, 3, project: exportable, relative_position: nil) # no position
+        create_list(:issue, 3, :with_desc_relative_position, project: exportable ) # ascending ids, descending position
+      end
+
       it 'calls json_writer.write_relation_array with proper params' do
         expect(json_writer).to receive(:write_relation_array).with(exportable_path, :issues, array_including(issue.to_json))
 
         subject.execute
       end
 
-      context 'relation ordering' do
-        before do
-          create_list(:issue, 5, project: exportable)
+      context 'default relation ordering' do
+        it 'orders exported issues by primary key(:id)' do
+          expected_issues = exportable.issues.reorder(:id).map(&:to_json)
+
+          expect(json_writer).to receive(:write_relation_array).with(exportable_path, :issues, expected_issues)
+
+          subject.execute
         end
+      end
 
-        it 'orders exported issues by primary key' do
-          expected_issues = exportable.issues.reorder(:id).map(&:to_json)
+      context 'custom relation ordering ascending' do
+        let(:custom_orderer) do
+          {
+            issues: {
+              column: :relative_position,
+              direction: :asc,
+              nulls_position: :nulls_last
+            }
+          }
+        end
+
+        it 'orders exported issues by custom column(relative_position)' do
+          expected_issues = exportable.issues.reorder(:relative_position, :id).map(&:to_json)
+
+          expect(json_writer).to receive(:write_relation_array).with(exportable_path, :issues, expected_issues)
+
+          subject.execute
+        end
+      end
+
+      context 'custom relation ordering descending' do
+        let(:custom_orderer) do
+          {
+            issues: {
+              column: :relative_position,
+              direction: :desc,
+              nulls_position: :nulls_first
+            }
+          }
+        end
+
+        it 'orders exported issues by custom column(relative_position)' do
+          expected_issues = exportable.issues.order_relative_position_desc.order(id: :desc).map(&:to_json)
 
           expect(json_writer).to receive(:write_relation_array).with(exportable_path, :issues, expected_issues)
 

spec/lib/gitlab/import_export/project/relation_factory_spec.rb

@@ -2,7 +2,7 @@
 
 require 'spec_helper'
 
-RSpec.describe Gitlab::ImportExport::Project::RelationFactory do
+RSpec.describe Gitlab::ImportExport::Project::RelationFactory, :use_clean_rails_memory_store_caching do
   let(:group) { create(:group) }
   let(:project) { create(:project, :repository, group: group) }
   let(:members_mapper) { double('members_mapper').as_null_object }
@@ -13,6 +13,7 @@ RSpec.describe Gitlab::ImportExport::Project::RelationFactory do
     described_class.create(
       relation_sym: relation_sym,
       relation_hash: relation_hash,
+      relation_index: 1,
       object_builder: Gitlab::ImportExport::Project::ObjectBuilder,
       members_mapper: members_mapper,
       user: importer_user,
@@ -171,6 +172,75 @@ RSpec.describe Gitlab::ImportExport::Project::RelationFactory do
     end
   end
 
+  context 'issue object' do
+    let(:relation_sym) { :issues }
+
+    let(:exported_member) do
+      {
+        "id" => 111,
+        "access_level" => 30,
+        "source_id" => 1,
+        "source_type" => "Project",
+        "user_id" => 3,
+        "notification_level" => 3,
+        "created_at" => "2016-11-18T09:29:42.634Z",
+        "updated_at" => "2016-11-18T09:29:42.634Z",
+        "user" => {
+          "id" => admin.id,
+          "email" => admin.email,
+          "username" => admin.username
+        }
+      }
+    end
+
+    let(:members_mapper) do
+      Gitlab::ImportExport::MembersMapper.new(
+        exported_members: [exported_member],
+        user: importer_user,
+        importable: project)
+    end
+
+    let(:relation_hash) do
+      {
+        'id' => 20,
+        'target_branch' => "feature",
+        'source_branch' => "feature_conflict",
+        'project_id' => project.id,
+        'author_id' => admin.id,
+        'assignee_id' => admin.id,
+        'updated_by_id' => admin.id,
+        'title' => "Issue 1",
+        'created_at' => "2016-06-14T15:02:36.568Z",
+        'updated_at' => "2016-06-14T15:02:56.815Z",
+        'state' => "opened",
+        'description' => "Description",
+        "relative_position" => 25111 # just a random position
+      }
+    end
+
+    it 'has preloaded project' do
+      expect(created_object.project).to equal(project)
+    end
+
+    context 'computing relative position' do
+      context 'when max relative position in the hierarchy is not cached' do
+        it 'has computed new relative_position' do
+          expect(created_object.relative_position).to equal(1026) # 513*2 - ideal distance
+        end
+      end
+
+      context 'when max relative position in the hierarchy is cached' do
+        before do
+          Rails.cache.write("import:#{project.model_name.plural}:#{project.id}:hierarchy_max_issues_relative_position", 10000)
+        end
+
+        it 'has computed new relative_position' do
+          expect(created_object.relative_position).to equal(10000 + 1026) # 513*2 - ideal distance
+        end
+      end
+    end
+  end
+
   context 'label object' do
     let(:relation_sym) { :labels }
     let(:relation_hash) do

spec/lib/gitlab/import_export/project/sample/relation_factory_spec.rb

@@ -17,6 +17,7 @@ RSpec.describe Gitlab::ImportExport::Project::Sample::RelationFactory do
     described_class.create( # rubocop:disable Rails/SaveBang
       relation_sym: relation_sym,
       relation_hash: relation_hash,
+      relation_index: 1,
       object_builder: Gitlab::ImportExport::Project::ObjectBuilder,
       members_mapper: members_mapper,
       user: importer_user,