Merge branch 'ajk-relative-positioning-safe-move-nulls' into 'master'

[RelativePositioning] Never fail to move nulls

See merge request gitlab-org/gitlab!40152

app/models/concerns/relative_positioning.rb

@@ -143,7 +143,7 @@ module RelativePositioning
       return 0 if objects.empty?
 
       representative = objects.first
-      number_of_gaps = objects.size + 1 # 1 at left, one between each, and one at right
+      number_of_gaps = objects.size # 1 to the nearest neighbour, and one between each
       position = if at_end
                    representative.max_relative_position
                  else
@@ -152,16 +152,21 @@ module RelativePositioning
 
       position ||= START_POSITION # If there are no positioned siblings, start from START_POSITION
 
-      gap, position = gap_size(representative, gaps: number_of_gaps, at_end: at_end, starting_from: position)
-
-      # Raise if we could not make enough space
-      raise NoSpaceLeft if gap < MIN_GAP
+      gap = 0
+      attempts = 10 # consolidate up to 10 gaps to find enough space
+      while gap < 1 && attempts > 0
+        gap, position = gap_size(representative, gaps: number_of_gaps, at_end: at_end, starting_from: position)
+        attempts -= 1
+      end
 
-      indexed = objects.each_with_index.to_a
-      starting_from = at_end ? position : position - (gap * number_of_gaps)
+      # Allow placing items next to each other, if we have to.
+      gap = 1 if gap < MIN_GAP
+      delta = at_end ? gap : -gap
+      indexed = (at_end ? objects : objects.reverse).each_with_index
 
       # Some classes are polymorphic, and not all siblings are in the same table.
       by_model = indexed.group_by { |pair| pair.first.class }
+      lower_bound, upper_bound = at_end ? [position, MAX_POSITION] : [MIN_POSITION, position]
 
       by_model.each do |model, pairs|
         model.transaction do
@@ -169,7 +174,8 @@ module RelativePositioning
             # These are known to be integers, one from the DB, and the other
             # calculated by us, and thus safe to interpolate
             values = batch.map do |obj, i|
-              pos = starting_from + gap * (i + 1)
+              desired_pos = position + delta * (i + 1)
+              pos = desired_pos.clamp(lower_bound, upper_bound)
               obj.relative_position = pos
               "(#{obj.id}, #{pos})"
             end.join(', ')

changelogs/unreleased/ajk-relative-positioning-safe-move-nulls.yml

+---
+title: Avoid raising errors when moving unpositioned items
+merge_request: 40152
+author:
+type: fixed

spec/support/shared_examples/models/relative_positioning_shared_examples.rb

@@ -70,6 +70,37 @@ RSpec.shared_examples 'a class that supports relative positioning' do
       expect(items.sort_by(&:relative_position)).to eq(items)
     end
 
+    it 'manages to move nulls to the end even if there is not enough space' do
+      run = run_at_end(20).to_a
+      bunch_a = create_items_with_positions(run[0..18])
+      bunch_b = create_items_with_positions([run.last])
+
+      nils = create_items_with_positions([nil] * 4)
+      described_class.move_nulls_to_end(nils)
+
+      items = [*bunch_a, *bunch_b, *nils]
+      items.each(&:reset)
+
+      expect(items.map(&:relative_position)).to all(be_valid_position)
+      expect(items.reverse.sort_by(&:relative_position)).to eq(items)
+    end
+
+    it 'manages to move nulls to the end, stacking if we cannot create enough space' do
+      run = run_at_end(40).to_a
+      bunch = create_items_with_positions(run.select(&:even?))
+
+      nils = create_items_with_positions([nil] * 20)
+      described_class.move_nulls_to_end(nils)
+
+      items = [*bunch, *nils]
+      items.each(&:reset)
+
+      expect(items.map(&:relative_position)).to all(be_valid_position)
+      expect(bunch.reverse.sort_by(&:relative_position)).to eq(bunch)
+      expect(nils.reverse.sort_by(&:relative_position)).not_to eq(nils)
+      expect(bunch.map(&:relative_position)).to all(be < nils.map(&:relative_position).min)
+    end
+
     it 'does not have an N+1 issue' do
       create_items_with_positions(10..12)
 
@@ -130,6 +161,37 @@ RSpec.shared_examples 'a class that supports relative positioning' do
       expect(described_class.move_nulls_to_start([item1])).to be(0)
       expect(item1.reload.relative_position).to be(1)
     end
+
+    it 'manages to move nulls to the start even if there is not enough space' do
+      run = run_at_start(20).to_a
+      bunch_a = create_items_with_positions([run.first])
+      bunch_b = create_items_with_positions(run[2..])
+
+      nils = create_items_with_positions([nil, nil, nil, nil])
+      described_class.move_nulls_to_start(nils)
+
+      items = [*nils, *bunch_a, *bunch_b]
+      items.each(&:reset)
+
+      expect(items.map(&:relative_position)).to all(be_valid_position)
+      expect(items.reverse.sort_by(&:relative_position)).to eq(items)
+    end
+
+    it 'manages to move nulls to the end, stacking if we cannot create enough space' do
+      run = run_at_start(40).to_a
+      bunch = create_items_with_positions(run.select(&:even?))
+
+      nils = create_items_with_positions([nil].cycle.take(20))
+      described_class.move_nulls_to_start(nils)
+
+      items = [*nils, *bunch]
+      items.each(&:reset)
+
+      expect(items.map(&:relative_position)).to all(be_valid_position)
+      expect(bunch.reverse.sort_by(&:relative_position)).to eq(bunch)
+      expect(nils.reverse.sort_by(&:relative_position)).not_to eq(nils)
+      expect(bunch.map(&:relative_position)).to all(be > nils.map(&:relative_position).max)
+    end
   end
 
   describe '#max_relative_position' do