Merge branch 'ajk-relative-positioning-mover' into 'master'

Exhaustively test relative positioning logic

See merge request gitlab-org/gitlab!41967

app/models/issue.rb

@@ -444,20 +444,9 @@ class Issue < ApplicationRecord
     Gitlab::EtagCaching::Store.new.touch(key)
   end
 
-  def find_next_gap_before
-    super
-  rescue ActiveRecord::QueryCanceled => e
-    # Symptom of running out of space - schedule rebalancing
-    IssueRebalancingWorker.perform_async(nil, project_id)
-    raise e
-  end
-
-  def find_next_gap_after
-    super
-  rescue ActiveRecord::QueryCanceled => e
+  def could_not_move(exception)
     # Symptom of running out of space - schedule rebalancing
     IssueRebalancingWorker.perform_async(nil, project_id)
-    raise e
   end
 end
 

changelogs/unreleased/ajk-relative-positioning-mover.yml

+---
+title: Refactor relative positioning to enable better testing
+merge_request: 41967
+author:
+type: other

ee/app/models/concerns/epic_tree_sorting.rb

@@ -21,15 +21,11 @@ module EpicTreeSorting
   included do
     extend ::Gitlab::Utils::Override
 
-    override :move_sequence
-    def move_sequence(start_pos, end_pos, delta, include_self = false)
-      items_to_update = scoped_items
+    override :update_relative_siblings
+    def update_relative_siblings(relation, range, delta)
+      items_to_update = relation
         .select(:id, :object_type)
-        .where('relative_position BETWEEN ? AND ?', start_pos, end_pos)
-
-      unless include_self
-        items_to_update = relative_siblings(items_to_update)
-      end
+        .where(relative_position: range)
 
       items_to_update.group_by { |item| item.object_type }.each do |type, group_items|
         ids = group_items.map(&:id)
@@ -38,11 +34,13 @@ module EpicTreeSorting
       end
     end
 
-    private
+    override :exclude_self
+    def exclude_self(relation, excluded: self)
+      return relation unless excluded&.id.present?
+
+      object_type = excluded.try(:object_type) || excluded.class.table_name.singularize
 
-    override :relative_siblings
-    def relative_siblings(relation = scoped_items)
-      relation.where.not('object_type = ? AND id = ?', self.class.table_name.singularize, self.id)
+      relation.where.not('object_type = ? AND id = ?', object_type, excluded.id)
     end
   end
 end

ee/spec/models/concerns/epic_tree_sorting_spec.rb

@@ -14,7 +14,7 @@ RSpec.describe EpicTreeSorting do
 
   describe '#relative_siblings' do
     def siblings(obj)
-      obj.send(:relative_siblings).pluck(:id, :object_type)
+      RelativePositioning.mover.context(obj).relative_siblings.pluck(:id, :object_type)
     end
 
     def polymorphic_ident(obj)
@@ -106,9 +106,16 @@ RSpec.describe EpicTreeSorting do
     let!(:epic2) { create(:epic, parent: base_epic, group: group, relative_position: 1003) }
     let!(:epic3) { create(:epic, parent: base_epic, group: group, relative_position: 1005) }
 
+    def move_sequence(range)
+      dx = 500
+      RelativePositioning.mover.context(item).send(:move_sequence, range.first, range.last, dx)
+    end
+
     context 'when self is an epic' do
+      let(:item) { epic1 }
+
       it 'moves all objects correctly' do
-        epic1.move_sequence(1003, 1005, 500)
+        move_sequence(1003..1005)
 
         expect(epic_issue1.reload.relative_position).to eq(1000)
         expect(epic_issue2.reload.relative_position).to eq(1001)
@@ -121,8 +128,10 @@ RSpec.describe EpicTreeSorting do
     end
 
     context 'when self is an epic_issue' do
+      let(:item) { epic_issue1 }
+
       it 'moves all objects correctly' do
-        epic_issue1.move_sequence(1001, 1005, 500)
+        move_sequence(1001..1005)
 
         expect(epic_issue1.reload.relative_position).to eq(1000)
         expect(epic_issue2.reload.relative_position).to eq(1501)

ee/spec/models/epic_issue_spec.rb

@@ -39,8 +39,8 @@ RSpec.describe EpicIssue do
       let_it_be_with_reload(:middle) { create(:epic, group: epic.group, parent: epic, relative_position: 101) }
       let_it_be_with_reload(:right) { create(:epic_issue, epic: epic, relative_position: 102) }
 
-      it 'can create space by using move_sequence_after' do
-        left.move_sequence_after
+      it 'can create space to the right' do
+        RelativePositioning.mover.context(left).create_space_right
         [left, middle, right].each(&:reset)
 
         expect(middle.relative_position - left.relative_position).to be > 1
@@ -48,8 +48,8 @@ RSpec.describe EpicIssue do
         expect(middle.relative_position).to be < right.relative_position
       end
 
-      it 'can create space by using move_sequence_before' do
-        right.move_sequence_before
+      it 'can create space to the left' do
+        RelativePositioning.mover.context(right).create_space_left
         [left, middle, right].each(&:reset)
 
         expect(right.relative_position - middle.relative_position).to be > 1

ee/spec/models/issue_spec.rb

@@ -419,29 +419,11 @@ RSpec.describe Issue do
     let_it_be_with_reload(:issue1) { create(:issue, project: project1, relative_position: issue.relative_position + RelativePositioning::IDEAL_DISTANCE) }
     let(:new_issue) { build(:issue, project: project1, relative_position: nil) }
 
-    describe '#max_relative_position' do
-      it 'returns maximum position' do
-        expect(issue.max_relative_position).to eq issue1.relative_position
-      end
-    end
-
-    describe '#prev_relative_position' do
-      it 'returns previous position if there is an issue above' do
-        expect(issue1.prev_relative_position).to eq issue.relative_position
-      end
-
-      it 'returns nil if there is no issue above' do
-        expect(issue.prev_relative_position).to eq nil
-      end
-    end
-
-    describe '#next_relative_position' do
-      it 'returns next position if there is an issue below' do
-        expect(issue.next_relative_position).to eq issue1.relative_position
-      end
+    describe '.relative_positioning_query_base' do
+      it 'includes cross project issues in the same group' do
+        siblings = Issue.relative_positioning_query_base(issue)
 
-      it 'returns nil if there is no issue below' do
-        expect(issue1.next_relative_position).to eq nil
+        expect(siblings).to include(issue1)
       end
     end
 

lib/gitlab/relative_positioning.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module RelativePositioning
+    STEPS = 10
+    IDEAL_DISTANCE = 2**(STEPS - 1) + 1
+
+    MIN_POSITION = Gitlab::Database::MIN_INT_VALUE
+    START_POSITION = 0
+    MAX_POSITION = Gitlab::Database::MAX_INT_VALUE
+
+    MAX_GAP = IDEAL_DISTANCE * 2
+    MIN_GAP = 2
+
+    NoSpaceLeft = Class.new(StandardError)
+  end
+end

lib/gitlab/relative_positioning/gap.rb

+# frozen_string_literal: true
+#
+module Gitlab
+  module RelativePositioning
+    class Gap
+      attr_reader :start_pos, :end_pos
+
+      def initialize(start_pos, end_pos)
+        @start_pos, @end_pos = start_pos, end_pos
+      end
+
+      def ==(other)
+        other.is_a?(self.class) && other.start_pos == start_pos && other.end_pos == end_pos
+      end
+
+      def delta
+        ((start_pos - end_pos) / 2.0).abs.ceil.clamp(0, RelativePositioning::IDEAL_DISTANCE)
+      end
+    end
+  end
+end

lib/gitlab/relative_positioning/item_context.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module RelativePositioning
+    # This class is API private - it should not be explicitly instantiated
+    # outside of tests
+    # rubocop: disable CodeReuse/ActiveRecord
+    class ItemContext
+      include Gitlab::Utils::StrongMemoize
+
+      attr_reader :object, :model_class, :range
+      attr_accessor :ignoring
+
+      def initialize(object, range, ignoring: nil)
+        @object = object
+        @range = range
+        @model_class = object.class
+        @ignoring = ignoring
+      end
+
+      def ==(other)
+        other.is_a?(self.class) && other.object == object && other.range == range && other.ignoring == ignoring
+      end
+
+      def positioned?
+        relative_position.present?
+      end
+
+      def min_relative_position
+        strong_memoize(:min_relative_position) { calculate_relative_position('MIN') }
+      end
+
+      def max_relative_position
+        strong_memoize(:max_relative_position) { calculate_relative_position('MAX') }
+      end
+
+      def prev_relative_position
+        calculate_relative_position('MAX') { |r| nextify(r, false) } if object.relative_position
+      end
+
+      def next_relative_position
+        calculate_relative_position('MIN') { |r| nextify(r) } if object.relative_position
+      end
+
+      def nextify(relation, gt = true)
+        if gt
+          relation.where("relative_position > ?", relative_position)
+        else
+          relation.where("relative_position < ?", relative_position)
+        end
+      end
+
+      def relative_siblings(relation = scoped_items)
+        object.exclude_self(relation)
+      end
+
+      # Handles the possibility that the position is already occupied by a sibling
+      def place_at_position(position, lhs)
+        current_occupant = relative_siblings.find_by(relative_position: position)
+
+        if current_occupant.present?
+          Mover.new(position, range).move(object, lhs.object, current_occupant)
+        else
+          object.relative_position = position
+        end
+      end
+
+      def lhs_neighbour
+        scoped_items
+          .where('relative_position < ?', relative_position)
+          .reorder(relative_position: :desc)
+          .first
+          .then { |x| neighbour(x) }
+      end
+
+      def rhs_neighbour
+        scoped_items
+          .where('relative_position > ?', relative_position)
+          .reorder(relative_position: :asc)
+          .first
+          .then { |x| neighbour(x) }
+      end
+
+      def neighbour(item)
+        return unless item.present?
+
+        self.class.new(item, range, ignoring: ignoring)
+      end
+
+      def scoped_items
+        r = model_class.relative_positioning_query_base(object)
+        r = object.exclude_self(r, excluded: ignoring) if ignoring.present?
+        r
+      end
+
+      def calculate_relative_position(calculation)
+        # When calculating across projects, this is much more efficient than
+        # MAX(relative_position) without the GROUP BY, due to index usage:
+        # https://gitlab.com/gitlab-org/gitlab-foss/issues/54276#note_119340977
+        relation = scoped_items
+                     .order(Gitlab::Database.nulls_last_order('position', 'DESC'))
+                     .group(grouping_column)
+                     .limit(1)
+
+        relation = yield relation if block_given?
+
+        relation
+          .pluck(grouping_column, Arel.sql("#{calculation}(relative_position) AS position"))
+          .first&.last
+      end
+
+      def grouping_column
+        model_class.relative_positioning_parent_column
+      end
+
+      def max_sibling
+        sib = relative_siblings
+          .order(Gitlab::Database.nulls_last_order('relative_position', 'DESC'))
+          .first
+
+        neighbour(sib)
+      end
+
+      def min_sibling
+        sib = relative_siblings
+          .order(Gitlab::Database.nulls_last_order('relative_position', 'ASC'))
+          .first
+
+        neighbour(sib)
+      end
+
+      def shift_left
+        move_sequence_before(true)
+        object.reset
+      end
+
+      def shift_right
+        move_sequence_after(true)
+        object.reset
+      end
+
+      def create_space_left
+        find_next_gap_before.tap { |gap| move_sequence_before(false, next_gap: gap) }
+      end
+
+      def create_space_right
+        find_next_gap_after.tap { |gap| move_sequence_after(false, next_gap: gap) }
+      end
+
+      def find_next_gap_before
+        items_with_next_pos = scoped_items
+                                .select('relative_position AS pos, LEAD(relative_position) OVER (ORDER BY relative_position DESC) AS next_pos')
+                                .where('relative_position <= ?', relative_position)
+                                .order(relative_position: :desc)
+
+        find_next_gap(items_with_next_pos, range.first)
+      end
+
+      def find_next_gap_after
+        items_with_next_pos = scoped_items
+                                .select('relative_position AS pos, LEAD(relative_position) OVER (ORDER BY relative_position ASC) AS next_pos')
+                                .where('relative_position >= ?', relative_position)
+                                .order(:relative_position)
+
+        find_next_gap(items_with_next_pos, range.last)
+      end
+
+      def find_next_gap(items_with_next_pos, default_end)
+        gap = model_class
+          .from(items_with_next_pos, :items)
+          .where('next_pos IS NULL OR ABS(pos::bigint - next_pos::bigint) >= ?', MIN_GAP)
+          .limit(1)
+          .pluck(:pos, :next_pos)
+          .first
+
+        return if gap.nil? || gap.first == default_end
+
+        Gap.new(gap.first, gap.second || default_end)
+      end
+
+      def relative_position
+        object.relative_position
+      end
+
+      private
+
+      # Moves the sequence before the current item to the middle of the next gap
+      # For example, we have
+      #
+      #   5 . . . . . 11 12 13 14 [15] 16 . 17
+      #               -----------
+      #
+      # This moves the sequence [11 12 13 14] to [8 9 10 11], so we have:
+      #
+      #   5 . . 8 9 10 11 . . . [15] 16 . 17
+      #         ---------
+      #
+      # Creating a gap to the left of the current item. We can understand this as
+      # dividing the 5 spaces between 5 and 11 into two smaller gaps of 2 and 3.
+      #
+      # If `include_self` is true, the current item will also be moved, creating a
+      # gap to the right of the current item:
+      #
+      #   5 . . 8 9 10 11 [14] . . . 16 . 17
+      #         --------------
+      #
+      # As an optimization, the gap can be precalculated and passed to this method.
+      #
+      # @api private
+      # @raises NoSpaceLeft if the sequence cannot be moved
+      def move_sequence_before(include_self = false, next_gap: find_next_gap_before)
+        raise NoSpaceLeft unless next_gap.present?
+
+        delta = next_gap.delta
+
+        move_sequence(next_gap.start_pos, relative_position, -delta, include_self)
+      end
+
+      # Moves the sequence after the current item to the middle of the next gap
+      # For example, we have:
+      #
+      #   8 . 10 [11] 12 13 14 15 . . . . . 21
+      #               -----------
+      #
+      # This moves the sequence [12 13 14 15] to [15 16 17 18], so we have:
+      #
+      #   8 . 10 [11] . . . 15 16 17 18 . . 21
+      #                     -----------
+      #
+      # Creating a gap to the right of the current item. We can understand this as
+      # dividing the 5 spaces between 15 and 21 into two smaller gaps of 3 and 2.
+      #
+      # If `include_self` is true, the current item will also be moved, creating a
+      # gap to the left of the current item:
+      #
+      #   8 . 10 . . . [14] 15 16 17 18 . . 21
+      #                ----------------
+      #
+      # As an optimization, the gap can be precalculated and passed to this method.
+      #
+      # @api private
+      # @raises NoSpaceLeft if the sequence cannot be moved
+      def move_sequence_after(include_self = false, next_gap: find_next_gap_after)
+        raise NoSpaceLeft unless next_gap.present?
+
+        delta = next_gap.delta
+
+        move_sequence(relative_position, next_gap.start_pos, delta, include_self)
+      end
+
+      def move_sequence(start_pos, end_pos, delta, include_self = false)
+        relation = include_self ? scoped_items : relative_siblings
+
+        object.update_relative_siblings(relation, (start_pos..end_pos), delta)
+      end
+    end
+    # rubocop: enable CodeReuse/ActiveRecord
+  end
+end

lib/gitlab/relative_positioning/mover.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module RelativePositioning
+    class Mover
+      attr_reader :range, :start_position
+
+      def initialize(start, range)
+        @range = range
+        @start_position = start
+      end
+
+      def move_to_end(object)
+        focus = context(object, ignoring: object)
+        max_pos = focus.max_relative_position
+
+        move_to_range_end(focus, max_pos)
+      end
+
+      def move_to_start(object)
+        focus = context(object, ignoring: object)
+        min_pos = focus.min_relative_position
+
+        move_to_range_start(focus, min_pos)
+      end
+
+      def move(object, first, last)
+        raise ArgumentError, 'object is required' unless object
+
+        lhs = context(first, ignoring: object)
+        rhs = context(last, ignoring: object)
+        focus = context(object)
+        range = RelativePositioning.range(lhs, rhs)
+
+        if range.cover?(focus)
+          # Moving a object already within a range is a no-op
+        elsif range.open_on_left?
+          move_to_range_start(focus, range.rhs.relative_position)
+        elsif range.open_on_right?
+          move_to_range_end(focus, range.lhs.relative_position)
+        else
+          pos_left, pos_right = create_space_between(range)
+          desired_position = position_between(pos_left, pos_right)
+          focus.place_at_position(desired_position, range.lhs)
+        end
+      end
+
+      def context(object, ignoring: nil)
+        return unless object
+
+        ItemContext.new(object, range, ignoring: ignoring)
+      end
+
+      private
+
+      def gap_too_small?(pos_a, pos_b)
+        return false unless pos_a && pos_b
+
+        (pos_a - pos_b).abs < MIN_GAP
+      end
+
+      def move_to_range_end(context, max_pos)
+        range_end = range.last + 1
+
+        new_pos = if max_pos.nil?
+                    start_position
+                  elsif gap_too_small?(max_pos, range_end)
+                    max = context.max_sibling
+                    max.ignoring = context.object
+                    max.shift_left
+                    position_between(max.relative_position, range_end)
+                  else
+                    position_between(max_pos, range_end)
+                  end
+
+        context.object.relative_position = new_pos
+      end
+
+      def move_to_range_start(context, min_pos)
+        range_end = range.first - 1
+
+        new_pos = if min_pos.nil?
+                    start_position
+                  elsif gap_too_small?(min_pos, range_end)
+                    sib = context.min_sibling
+                    sib.ignoring = context.object
+                    sib.shift_right
+                    position_between(sib.relative_position, range_end)
+                  else
+                    position_between(min_pos, range_end)
+                  end
+
+        context.object.relative_position = new_pos
+      end
+
+      def create_space_between(range)
+        pos_left = range.lhs&.relative_position
+        pos_right = range.rhs&.relative_position
+
+        return [pos_left, pos_right] unless gap_too_small?(pos_left, pos_right)
+
+        gap = range.rhs.create_space_left
+        [pos_left - gap.delta, pos_right]
+      rescue NoSpaceLeft
+        gap = range.lhs.create_space_right
+        [pos_left, pos_right + gap.delta]
+      end
+
+      # This method takes two integer values (positions) and
+      # calculates the position between them. The range is huge as
+      # the maximum integer value is 2147483647.
+      #
+      # We avoid open ranges by clamping the range to [MIN_POSITION, MAX_POSITION].
+      #
+      # Then we handle one of three cases:
+      #  - If the gap is too small, we raise NoSpaceLeft
+      #  - If the gap is larger than MAX_GAP, we place the new position at most
+      #    IDEAL_DISTANCE from the edge of the gap.
+      #  - otherwise we place the new position at the midpoint.
+      #
+      # The new position will always satisfy: pos_before <= midpoint <= pos_after
+      #
+      # As a precondition, the gap between pos_before and pos_after MUST be >= 2.
+      # If the gap is too small, NoSpaceLeft is raised.
+      #
+      # @raises NoSpaceLeft
+      def position_between(pos_before, pos_after)
+        pos_before ||= range.first
+        pos_after ||= range.last
+
+        pos_before, pos_after = [pos_before, pos_after].sort
+
+        gap_width = pos_after - pos_before
+
+        if gap_too_small?(pos_before, pos_after)
+          raise NoSpaceLeft
+        elsif gap_width > MAX_GAP
+          if pos_before <= range.first
+            pos_after - IDEAL_DISTANCE
+          elsif pos_after >= range.last
+            pos_before + IDEAL_DISTANCE
+          else
+            midpoint(pos_before, pos_after)
+          end
+        else
+          midpoint(pos_before, pos_after)
+        end
+      end
+
+      def midpoint(lower_bound, upper_bound)
+        ((lower_bound + upper_bound) / 2.0).ceil.clamp(lower_bound, upper_bound - 1)
+      end
+    end
+  end
+end

lib/gitlab/relative_positioning/range.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module RelativePositioning
+    IllegalRange = Class.new(ArgumentError)
+
+    class Range
+      attr_reader :lhs, :rhs
+
+      def open_on_left?
+        lhs.nil?
+      end
+
+      def open_on_right?
+        rhs.nil?
+      end
+
+      def cover?(item_context)
+        return false unless item_context
+        return false unless item_context.positioned?
+        return true if item_context.object == lhs&.object
+        return true if item_context.object == rhs&.object
+
+        pos = item_context.relative_position
+
+        return lhs.relative_position < pos if open_on_right?
+        return pos < rhs.relative_position if open_on_left?
+
+        lhs.relative_position < pos && pos < rhs.relative_position
+      end
+
+      def ==(other)
+        other.is_a?(RelativePositioning::Range) && lhs == other.lhs && rhs == other.rhs
+      end
+    end
+
+    def self.range(lhs, rhs)
+      if lhs && rhs
+        ClosedRange.new(lhs, rhs)
+      elsif lhs
+        StartingFrom.new(lhs)
+      elsif rhs
+        EndingAt.new(rhs)
+      else
+        raise IllegalRange, 'One of rhs or lhs must be provided' unless lhs && rhs
+      end
+    end
+
+    class ClosedRange < RelativePositioning::Range
+      def initialize(lhs, rhs)
+        @lhs, @rhs = lhs, rhs
+        raise IllegalRange, 'Either lhs or rhs is missing' unless lhs && rhs
+        raise IllegalRange, 'lhs and rhs cannot be the same object' if lhs == rhs
+      end
+    end
+
+    class StartingFrom < RelativePositioning::Range
+      include Gitlab::Utils::StrongMemoize
+
+      def initialize(lhs)
+        @lhs = lhs
+        raise IllegalRange, 'lhs is required' unless lhs
+      end
+
+      def rhs
+        strong_memoize(:rhs) { lhs.rhs_neighbour }
+      end
+    end
+
+    class EndingAt < RelativePositioning::Range
+      include Gitlab::Utils::StrongMemoize
+
+      def initialize(rhs)
+        @rhs = rhs
+        raise IllegalRange, 'rhs is required' unless rhs
+      end
+
+      def lhs
+        strong_memoize(:lhs) { rhs.lhs_neighbour }
+      end
+    end
+  end
+end

spec/lib/gitlab/relative_positioning/item_context_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::RelativePositioning::ItemContext do
+  let_it_be(:default_user) { create_default(:user) }
+  let_it_be(:project, reload: true) { create(:project) }
+
+  def create_issue(pos)
+    create(:issue, project: project, relative_position: pos)
+  end
+
+  range = (101..107) # A deliberately small range, so we can test everything
+  indices = (0..).take(range.size)
+
+  let(:start) { ((range.first + range.last) / 2.0).floor }
+  let(:subjects) { issues.map { |i| described_class.new(i.reset, range) } }
+
+  # This allows us to refer to range in methods and examples
+  let_it_be(:full_range) { range }
+
+  context 'there are gaps at the start and end' do
+    let_it_be(:issues) { (range.first.succ..range.last.pred).map { |pos| create_issue(pos) } }
+
+    it 'is always possible to find a gap' do
+      expect(subjects)
+        .to all(have_attributes(find_next_gap_before: be_present, find_next_gap_after: be_present))
+    end
+
+    where(:index) { indices.reverse.drop(2) }
+
+    with_them do
+      subject { subjects[index] }
+
+      let(:positions) { subject.scoped_items.map(&:relative_position) }
+
+      it 'is possible to shift_right, which will consume the gap at the end' do
+        subject.shift_right
+
+        expect(subject.find_next_gap_after).not_to be_present
+
+        expect(positions).to all(be_between(range.first, range.last))
+        expect(positions).to eq(positions.uniq)
+      end
+
+      it 'is possible to create_space_right, which will move the gap to immediately after' do
+        subject.create_space_right
+
+        expect(subject.find_next_gap_after).to have_attributes(start_pos: subject.relative_position)
+        expect(positions).to all(be_between(range.first, range.last))
+        expect(positions).to eq(positions.uniq)
+      end
+
+      it 'is possible to shift_left, which will consume the gap at the start' do
+        subject.shift_left
+
+        expect(subject.find_next_gap_before).not_to be_present
+        expect(positions).to all(be_between(range.first, range.last))
+        expect(positions).to eq(positions.uniq)
+      end
+
+      it 'is possible to create_space_left, which will move the gap to immediately before' do
+        subject.create_space_left
+
+        expect(subject.find_next_gap_before).to have_attributes(start_pos: subject.relative_position)
+        expect(positions).to all(be_between(range.first, range.last))
+        expect(positions).to eq(positions.uniq)
+      end
+    end
+  end
+
+  context 'there is a gap of multiple spaces' do
+    let_it_be(:issues) { [range.first, range.last].map { |pos| create_issue(pos) } }
+
+    it 'is impossible to move the last element to the right' do
+      expect { subjects.last.shift_right }.to raise_error(Gitlab::RelativePositioning::NoSpaceLeft)
+    end
+
+    it 'is impossible to move the first element to the left' do
+      expect { subjects.first.shift_left }.to raise_error(Gitlab::RelativePositioning::NoSpaceLeft)
+    end
+
+    it 'is possible to move the last element to the left' do
+      subject = subjects.last
+
+      expect { subject.shift_left }.to change { subject.relative_position }.by(be < 0)
+    end
+
+    it 'is possible to move the first element to the right' do
+      subject = subjects.first
+
+      expect { subject.shift_right }.to change { subject.relative_position }.by(be > 0)
+    end
+
+    it 'is possible to find the gap from the right' do
+      gap = Gitlab::RelativePositioning::Gap.new(range.last, range.first)
+
+      expect(subjects.last).to have_attributes(
+        find_next_gap_before: eq(gap),
+        find_next_gap_after: be_nil
+      )
+    end
+
+    it 'is possible to find the gap from the left' do
+      gap = Gitlab::RelativePositioning::Gap.new(range.first, range.last)
+
+      expect(subjects.first).to have_attributes(
+        find_next_gap_before: be_nil,
+        find_next_gap_after: eq(gap)
+      )
+    end
+  end
+
+  context 'there are several free spaces' do
+    let_it_be(:issues) { range.select(&:even?).map { |pos| create_issue(pos) } }
+    let_it_be(:gaps) do
+      range.select(&:odd?).map do |pos|
+        rhs = pos.succ.clamp(range.first, range.last)
+        lhs = pos.pred.clamp(range.first, range.last)
+
+        {
+          before: Gitlab::RelativePositioning::Gap.new(rhs, lhs),
+          after: Gitlab::RelativePositioning::Gap.new(lhs, rhs)
+        }
+      end
+    end
+
+    def issue_at(position)
+      issues.find { |i| i.relative_position == position }
+    end
+
+    where(:current_pos) { range.select(&:even?) }
+
+    with_them do
+      let(:subject) { subjects.find { |s| s.relative_position == current_pos } }
+      let(:siblings) { subjects.reject { |s| s.relative_position == current_pos } }
+
+      def covered_by_range(pos)
+        full_range.cover?(pos) ? pos : nil
+      end
+
+      it 'finds the closest gap' do
+        closest_gap_before = gaps
+          .map { |gap| gap[:before] }
+          .select { |gap| gap.start_pos <= subject.relative_position }
+          .max_by { |gap| gap.start_pos }
+        closest_gap_after = gaps
+          .map { |gap| gap[:after] }
+          .select { |gap| gap.start_pos >= subject.relative_position }
+          .min_by { |gap| gap.start_pos }
+
+        expect(subject).to have_attributes(
+          find_next_gap_before: closest_gap_before,
+          find_next_gap_after: closest_gap_after
+        )
+      end
+
+      it 'finds the neighbours' do
+        expect(subject).to have_attributes(
+          lhs_neighbour: subject.neighbour(issue_at(subject.relative_position - 2)),
+          rhs_neighbour: subject.neighbour(issue_at(subject.relative_position + 2))
+        )
+      end
+
+      it 'finds the next relative_positions' do
+        expect(subject).to have_attributes(
+          prev_relative_position: covered_by_range(subject.relative_position - 2),
+          next_relative_position: covered_by_range(subject.relative_position + 2)
+        )
+      end
+
+      it 'finds the min/max positions' do
+        expect(subject).to have_attributes(
+          min_relative_position: issues.first.relative_position,
+          max_relative_position: issues.last.relative_position
+        )
+      end
+
+      it 'finds the min/max siblings' do
+        expect(subject).to have_attributes(
+          min_sibling: siblings.first,
+          max_sibling: siblings.last
+        )
+      end
+    end
+  end
+
+  context 'there is at least one free space' do
+    where(:free_space) { range.to_a }
+
+    with_them do
+      let(:issues) { range.reject { |x| x == free_space }.map { |p| create_issue(p) } }
+      let(:gap_rhs) { free_space.succ.clamp(range.first, range.last) }
+      let(:gap_lhs) { free_space.pred.clamp(range.first, range.last) }
+
+      it 'can always find a gap before if there is space to the left' do
+        expected_gap = Gitlab::RelativePositioning::Gap.new(gap_rhs, gap_lhs)
+
+        to_the_right_of_gap = subjects.select { |s| free_space < s.relative_position }
+
+        expect(to_the_right_of_gap)
+          .to all(have_attributes(find_next_gap_before: eq(expected_gap), find_next_gap_after: be_nil))
+      end
+
+      it 'can always find a gap after if there is space to the right' do
+        expected_gap = Gitlab::RelativePositioning::Gap.new(gap_lhs, gap_rhs)
+
+        to_the_left_of_gap = subjects.select { |s| s.relative_position < free_space }
+
+        expect(to_the_left_of_gap)
+          .to all(have_attributes(find_next_gap_before: be_nil, find_next_gap_after: eq(expected_gap)))
+      end
+    end
+  end
+end

spec/lib/gitlab/relative_positioning/range_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::RelativePositioning::Range do
+  item_a = OpenStruct.new(relative_position: 100, object: :x, positioned?: true)
+  item_b = OpenStruct.new(relative_position: 200, object: :y, positioned?: true)
+
+  before do
+    allow(item_a).to receive(:lhs_neighbour) { nil }
+    allow(item_a).to receive(:rhs_neighbour) { item_b }
+
+    allow(item_b).to receive(:lhs_neighbour) { item_a }
+    allow(item_b).to receive(:rhs_neighbour) { nil }
+  end
+
+  describe 'RelativePositioning.range' do
+    it 'raises if lhs and rhs are nil' do
+      expect { Gitlab::RelativePositioning.range(nil, nil) }.to raise_error(ArgumentError)
+    end
+
+    it 'raises an error if there is no extent' do
+      expect { Gitlab::RelativePositioning.range(item_a, item_a) }.to raise_error(ArgumentError)
+    end
+
+    it 'constructs a closed range when both termini are provided' do
+      range = Gitlab::RelativePositioning.range(item_a, item_b)
+      expect(range).to be_a_kind_of(Gitlab::RelativePositioning::Range)
+      expect(range).to be_a_kind_of(Gitlab::RelativePositioning::ClosedRange)
+    end
+
+    it 'constructs a starting-from range when only the LHS is provided' do
+      range = Gitlab::RelativePositioning.range(item_a, nil)
+      expect(range).to be_a_kind_of(Gitlab::RelativePositioning::Range)
+      expect(range).to be_a_kind_of(Gitlab::RelativePositioning::StartingFrom)
+    end
+
+    it 'constructs an ending-at range when only the RHS is provided' do
+      range = Gitlab::RelativePositioning.range(nil, item_b)
+      expect(range).to be_a_kind_of(Gitlab::RelativePositioning::Range)
+      expect(range).to be_a_kind_of(Gitlab::RelativePositioning::EndingAt)
+    end
+  end
+
+  it 'infers neighbours correctly' do
+    starting_at_a = Gitlab::RelativePositioning.range(item_a, nil)
+    ending_at_b = Gitlab::RelativePositioning.range(nil, item_b)
+
+    expect(starting_at_a).to eq(ending_at_b)
+  end
+
+  describe '#open_on_left?' do
+    where(:lhs, :rhs, :expected_result) do
+      [
+        [item_a, item_b, false],
+        [item_a, nil, false],
+        [nil, item_b, false],
+        [item_b, nil, false],
+        [nil, item_a, true]
+      ]
+    end
+
+    with_them do
+      it 'is true if there is no LHS terminus' do
+        range = Gitlab::RelativePositioning.range(lhs, rhs)
+
+        expect(range.open_on_left?).to be(expected_result)
+      end
+    end
+  end
+
+  describe '#open_on_right?' do
+    where(:lhs, :rhs, :expected_result) do
+      [
+        [item_a, item_b, false],
+        [item_a, nil, false],
+        [nil, item_b, false],
+        [item_b, nil, true],
+        [nil, item_a, false]
+      ]
+    end
+
+    with_them do
+      it 'is true if there is no RHS terminus' do
+        range = Gitlab::RelativePositioning.range(lhs, rhs)
+
+        expect(range.open_on_right?).to be(expected_result)
+      end
+    end
+  end
+
+  describe '#cover?' do
+    item_c = OpenStruct.new(relative_position: 150, object: :z, positioned?: true)
+    item_d = OpenStruct.new(relative_position: 050, object: :w, positioned?: true)
+    item_e = OpenStruct.new(relative_position: 250, object: :r, positioned?: true)
+    item_f = OpenStruct.new(positioned?: false)
+    item_ax = OpenStruct.new(relative_position: 100, object: :not_x, positioned?: true)
+    item_bx = OpenStruct.new(relative_position: 200, object: :not_y, positioned?: true)
+
+    where(:lhs, :rhs, :item, :expected_result) do
+      [
+        [item_a, item_b, item_a, true],
+        [item_a, item_b, item_b, true],
+        [item_a, item_b, item_c, true],
+        [item_a, item_b, item_d, false],
+        [item_a, item_b, item_e, false],
+        [item_a, item_b, item_ax, false],
+        [item_a, item_b, item_bx, false],
+        [item_a, item_b, item_f, false],
+        [item_a, item_b, nil, false],
+
+        [nil, item_b, item_a, true],
+        [nil, item_b, item_b, true],
+        [nil, item_b, item_c, true],
+        [nil, item_b, item_d, false],
+        [nil, item_b, item_e, false],
+        [nil, item_b, item_ax, false],
+        [nil, item_b, item_bx, false],
+        [nil, item_b, item_f, false],
+        [nil, item_b, nil, false],
+
+        [item_a, nil, item_a, true],
+        [item_a, nil, item_b, true],
+        [item_a, nil, item_c, true],
+        [item_a, nil, item_d, false],
+        [item_a, nil, item_e, false],
+        [item_a, nil, item_ax, false],
+        [item_a, nil, item_bx, false],
+        [item_a, nil, item_f, false],
+        [item_a, nil, nil, false],
+
+        [nil, item_a, item_a, true],
+        [nil, item_a, item_b, false],
+        [nil, item_a, item_c, false],
+        [nil, item_a, item_d, true],
+        [nil, item_a, item_e, false],
+        [nil, item_a, item_ax, false],
+        [nil, item_a, item_bx, false],
+        [nil, item_a, item_f, false],
+        [nil, item_a, nil, false],
+
+        [item_b, nil, item_a, false],
+        [item_b, nil, item_b, true],
+        [item_b, nil, item_c, false],
+        [item_b, nil, item_d, false],
+        [item_b, nil, item_e, true],
+        [item_b, nil, item_ax, false],
+        [item_b, nil, item_bx, false],
+        [item_b, nil, item_f, false],
+        [item_b, nil, nil, false]
+      ]
+    end
+
+    with_them do
+      it 'is true when the object is within the bounds of the range' do
+        range = Gitlab::RelativePositioning.range(lhs, rhs)
+
+        expect(range.cover?(item)).to be(expected_result)
+      end
+    end
+  end
+end

spec/models/design_management/design_spec.rb

@@ -12,8 +12,10 @@ RSpec.describe DesignManagement::Design do
   let_it_be(:deleted_design) { create(:design, :with_versions, deleted: true) }
 
   it_behaves_like 'a class that supports relative positioning' do
+    let_it_be(:relative_parent) { create(:issue) }
+
     let(:factory) { :design }
-    let(:default_params) { { issue: issue } }
+    let(:default_params) { { issue: relative_parent } }
   end
 
   describe 'relations' do

spec/models/issue_spec.rb

@@ -1187,29 +1187,20 @@ RSpec.describe Issue do
 
   describe 'scheduling rebalancing' do
     before do
-      allow(issue).to receive(:find_next_gap) { raise ActiveRecord::QueryCanceled }
+      allow_next_instance_of(RelativePositioning::Mover) do |mover|
+        allow(mover).to receive(:move) { raise ActiveRecord::QueryCanceled }
+      end
     end
 
-    let(:project) { build(:project_empty_repo) }
+    let(:project) { build_stubbed(:project_empty_repo) }
     let(:issue) { build_stubbed(:issue, relative_position: 100, project: project) }
 
-    describe '#find_next_gap_before' do
-      it 'schedules rebalancing if we time-out when finding a gap' do
-        lhs = build_stubbed(:issue, relative_position: 99, project: project)
-        to_move = build(:issue, project: project)
-        expect(IssueRebalancingWorker).to receive(:perform_async).with(nil, project.id)
+    it 'schedules rebalancing if we time-out when moving' do
+      lhs = build_stubbed(:issue, relative_position: 99, project: project)
+      to_move = build(:issue, project: project)
+      expect(IssueRebalancingWorker).to receive(:perform_async).with(nil, project.id)
 
-        expect { to_move.move_between(lhs, issue) }.to raise_error(ActiveRecord::QueryCanceled)
-      end
-    end
-
-    describe '#find_next_gap_after' do
-      it 'schedules rebalancing if we time-out when finding a gap' do
-        allow(issue).to receive(:find_next_gap) { raise ActiveRecord::QueryCanceled }
-        expect(IssueRebalancingWorker).to receive(:perform_async).with(nil, project.id)
-
-        expect { issue.move_sequence_after }.to raise_error(ActiveRecord::QueryCanceled)
-      end
+      expect { to_move.move_between(lhs, issue) }.to raise_error(ActiveRecord::QueryCanceled)
     end
   end