Move nulls safely even when there is not enough space

This is motivated by errors such as
https://sentry.gitlab.net/gitlab/gitlabcom/issues/1766076, where
attempts to move nulls fail even with rebalancing due to the fact that
the gap found is not big enough for all the nulls.

We address this in two ways:

- we try to consolidate multiple gaps. Up to 10 gaps will be found and
consolidated to make enough room. This is expected to be effective in
most cases.

- if we still cannot make enough space, rather than failing, we stack
all overflow elements at the edge. So if we need to move 10 nulls to the
end, and we only have space for 5, all the extras (5 of them) will be
placed at the maximum position.

Adds a changelog entry since this is a bug-fix

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