Test error conditions

This involves a refactoring of the service so that it is less of a
single procedural thicket.

app/services/issue_rebalancing_service.rb

@@ -4,45 +4,39 @@ class IssueRebalancingService
   MAX_ISSUE_COUNT = 10_000
   TooManyIssues = Class.new(StandardError)
 
-  attr_reader :issue
-
   def initialize(issue)
     @issue = issue
+    @base = Issue.relative_positioning_query_base(issue)
   end
 
-  # rubocop: disable CodeReuse/ActiveRecord
   def execute
     gates = [issue.project, issue.project.group].compact
     return unless gates.any? { |gate| Feature.enabled?(:rebalance_issues, gate) }
 
-    base = Issue.relative_positioning_query_base(issue)
+    raise TooManyIssues, "#{issue_count} issues" unless gap_size.present?
 
-    n = base.count
+    start = RelativePositioning::START_POSITION - (gaps / 2) * gap_size
 
-    if n > MAX_ISSUE_COUNT
-      raise TooManyIssues, "#{n} issues"
+    Issue.transaction do
+      indexed_ids.each_slice(500) { |pairs| assign_positions(start, pairs) }
     end
-
-    range = RelativePositioning::MAX_POSITION - RelativePositioning::MIN_POSITION
-    gaps = n - 1
-    gap_size = 0
-    ratio = 0.5
-
-    while gap_size < RelativePositioning::MIN_GAP && ratio < 1
-      gap_size = [RelativePositioning::IDEAL_DISTANCE, (ratio * range) / gaps].min.floor
-      ratio += 0.1
   end
 
-    # If there are 4 billion issues, then we cannot rebalance them
-    raise TooManyIssues if gap_size < RelativePositioning::MIN_GAP
+  private
 
-    start = RelativePositioning::START_POSITION - (gaps / 2) * gap_size
+  attr_reader :issue, :base
 
-    Issue.transaction do
-      indexed = base.reorder(:relative_position, :id).pluck(:id).each_with_index
+  FULL_RANGE = RelativePositioning::MAX_POSITION - RelativePositioning::MIN_POSITION
+
+  # rubocop: disable CodeReuse/ActiveRecord
+  def indexed_ids
+    base.reorder(:relative_position, :id).pluck(:id).each_with_index
+  end
+  # rubocop: enable CodeReuse/ActiveRecord
 
-      indexed.each_slice(500) do |pairs|
-        values = pairs.map do |id, index|
+  # rubocop: disable CodeReuse/ActiveRecord
+  def assign_positions(start, positions)
+    values = positions.map do |id, index|
       "(#{id}, #{start + (index * gap_size)})"
     end.join(', ')
 
@@ -57,7 +51,30 @@ class IssueRebalancingService
       WHERE cte_id = id
     SQL
   end
+  # rubocop: enable CodeReuse/ActiveRecord
+
+  def issue_count
+    @issue_count ||= base.count
   end
+
+  def gaps
+    issue_count - 1
+  end
+
+  def gap_size
+    @gap_size ||= begin
+      return if issue_count > MAX_ISSUE_COUNT
+
+      (0.4..0.9).step(0.1)
+        .map { |ratio| ratio * FULL_RANGE }
+        .map { |scaled_range| gap_width(scaled_range) }
+        .select { |gap| gap >= RelativePositioning::MIN_GAP }
+        .max
+    end
+  end
+
+  # What gap width do we need to use to spread our N gaps out over a given range?
+  def gap_width(range)
+    [RelativePositioning::IDEAL_DISTANCE, range / gaps].min.floor
   end
-  # rubocop: enable CodeReuse/ActiveRecord
 end

app/workers/issue_rebalancing_worker.rb

@@ -10,17 +10,8 @@ class IssueRebalancingWorker
   def perform(issue_id)
     issue = Issue.find(issue_id)
 
-    rebalance(issue)
+    IssueRebalancingService.new(issue).execute
   rescue ActiveRecord::RecordNotFound, IssueRebalancingService::TooManyIssues => e
     Gitlab::ErrorTracking.log_exception(e, issue_id: issue_id)
   end
-
-  private
-
-  def rebalance(issue)
-    gates = [issue.project, issue.project.group].compact
-    return unless gates.any? { |gate| Feature.enabled?(:rebalance_issues, gate) }
-
-    IssueRebalancingService.new(issue).execute
-  end
 end

spec/services/issue_rebalancing_service_spec.rb

@@ -89,4 +89,22 @@ RSpec.describe IssueRebalancingService do
 
     expect { service.execute }.to change { issue.reload.relative_position }
   end
+
+  it 'aborts if there are too many issues' do
+    issue = project.issues.first
+    base = double(count: 10_001)
+
+    allow(Issue).to receive(:relative_positioning_query_base).with(issue).and_return(base)
+
+    expect { described_class.new(issue).execute }.to raise_error(described_class::TooManyIssues)
+  end
+
+  it 'aborts if we cannot compute a suitable gap' do
+    issue = project.issues.first
+    service = described_class.new(issue)
+
+    allow(service).to receive(:gap_width).and_return(0)
+
+    expect { service.execute }.to raise_error(described_class::TooManyIssues)
+  end
 end

spec/workers/issue_rebalancing_worker_spec.rb

@@ -4,13 +4,28 @@ require 'spec_helper'
 
 RSpec.describe IssueRebalancingWorker do
   describe '#perform' do
+    let_it_be(:issue) { create(:issue) }
     it 'runs an instance of IssueRebalancingService' do
-      issue = create(:issue)
-
       service = double(execute: nil)
       expect(IssueRebalancingService).to receive(:new).with(issue).and_return(service)
 
       described_class.new.perform(issue.id)
     end
+
+    it 'anticipates the inability to find the issue' do
+      expect(Gitlab::ErrorTracking).to receive(:log_exception).with(ActiveRecord::RecordNotFound, include(issue_id: -1))
+      expect(IssueRebalancingService).not_to receive(:new)
+
+      described_class.new.perform(-1)
+    end
+
+    it 'anticipates there being too many issues' do
+      service = double
+      allow(service).to receive(:execute) { raise IssueRebalancingService::TooManyIssues }
+      expect(IssueRebalancingService).to receive(:new).with(issue).and_return(service)
+      expect(Gitlab::ErrorTracking).to receive(:log_exception).with(IssueRebalancingService::TooManyIssues, include(issue_id: issue.id))
+
+      described_class.new.perform(issue.id)
+    end
   end
 end