Improve performance of sorting milestone issuesmilestone-issue-sorting-performance

This cuts down the time it takes to sort issues of a milestone by about
10x. In the previous setup the code would run a SQL query for every
issue that had to be sorted. The new setup instead runs a single SQL
query to update all the given issues at once.

The attached benchmark used to run at around 60 iterations per second,
using the new setup this hovers around 600 iterations per second. Timing
wise a request to update a milestone with 40-something issues would take
about 760 ms, in the new setup this only takes about 130 ms.

Fixes #3066

5 files changed, 79 insertions, 5 deletions

diff --git a/CHANGELOG b/CHANGELOG
index e9ca5ddee5f..85106effa6c 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -1,6 +1,7 @@
 Please view this file on the master branch, on stable branches it's out of date.
 
 v 8.1.0 (unreleased)
+  - Improved performance of sorting milestone issues
   - Fix error preventing displaying of commit data for a directory with a leading dot (Stan Hu)
   - Speed up load times of issue detail pages by roughly 1.5x
   - Make diff file view easier to use on mobile screens (Stan Hu)
diff --git a/app/controllers/projects/milestones_controller.rb b/app/controllers/projects/milestones_controller.rb
index 86f4a02a6e9..15506bd677a 100644
--- a/app/controllers/projects/milestones_controller.rb
+++ b/app/controllers/projects/milestones_controller.rb
@@ -75,11 +75,7 @@ class Projects::MilestonesController < Projects::ApplicationController
   end
 
   def sort_issues
-    @issues = @milestone.issues.where(id: params['sortable_issue'])
-    @issues.each do |issue|
-      issue.position = params['sortable_issue'].index(issue.id.to_s) + 1
-      issue.save
-    end
+    @milestone.sort_issues(params['sortable_issue'].map(&:to_i))
 
     render json: { saved: true }
   end
diff --git a/app/models/milestone.rb b/app/models/milestone.rb
index 84acba30b6b..2ff16e2825c 100644
--- a/app/models/milestone.rb
+++ b/app/models/milestone.rb
@@ -105,4 +105,36 @@ class Milestone < ActiveRecord::Base
   def author_id
     nil
   end
+
+  # Sorts the issues for the given IDs.
+  #
+  # This method runs a single SQL query using a CASE statement to update the
+  # position of all issues in the current milestone (scoped to the list of IDs).
+  #
+  # Given the ids [10, 20, 30] this method produces a SQL query something like
+  # the following:
+  #
+  #     UPDATE issues
+  #     SET position = CASE
+  #       WHEN id = 10 THEN 1
+  #       WHEN id = 20 THEN 2
+  #       WHEN id = 30 THEN 3
+  #       ELSE position
+  #     END
+  #     WHERE id IN (10, 20, 30);
+  #
+  # This method expects that the IDs given in `ids` are already Fixnums.
+  def sort_issues(ids)
+    pairs = []
+
+    ids.each_with_index do |id, index|
+      pairs << id
+      pairs << index + 1
+    end
+
+    conditions = 'WHEN id = ? THEN ? ' * ids.length
+
+    issues.where(id: ids).
+      update_all(["position = CASE #{conditions} ELSE position END", *pairs])
+  end
 end
diff --git a/spec/benchmarks/models/milestone_spec.rb b/spec/benchmarks/models/milestone_spec.rb
new file mode 100644
index 00000000000..a94afc4c40d
--- /dev/null
+++ b/spec/benchmarks/models/milestone_spec.rb
@@ -0,0 +1,17 @@
+require 'spec_helper'
+
+describe Milestone, benchmark: true do
+  describe '#sort_issues' do
+    let(:milestone) { create(:milestone) }
+
+    let(:issue1) { create(:issue, milestone: milestone) }
+    let(:issue2) { create(:issue, milestone: milestone) }
+    let(:issue3) { create(:issue, milestone: milestone) }
+
+    let(:issue_ids) { [issue3.id, issue2.id, issue1.id] }
+
+    benchmark_subject { milestone.sort_issues(issue_ids) }
+
+    it { is_expected.to iterate_per_second(500) }
+  end
+end
diff --git a/spec/models/milestone_spec.rb b/spec/models/milestone_spec.rb
index c88d5349663..77c58627322 100644
--- a/spec/models/milestone_spec.rb
+++ b/spec/models/milestone_spec.rb
@@ -140,4 +140,32 @@ describe Milestone do
     end
   end
 
+  describe '#sort_issues' do
+    let(:milestone) { create(:milestone) }
+
+    let(:issue1) { create(:issue, milestone: milestone, position: 1) }
+    let(:issue2) { create(:issue, milestone: milestone, position: 2) }
+    let(:issue3) { create(:issue, milestone: milestone, position: 3) }
+    let(:issue4) { create(:issue, position: 42) }
+
+    it 'sorts the given issues' do
+      milestone.sort_issues([issue3.id, issue2.id, issue1.id])
+
+      issue1.reload
+      issue2.reload
+      issue3.reload
+
+      expect(issue1.position).to eq(3)
+      expect(issue2.position).to eq(2)
+      expect(issue3.position).to eq(1)
+    end
+
+    it 'ignores issues not part of the milestone' do
+      milestone.sort_issues([issue3.id, issue2.id, issue1.id, issue4.id])
+
+      issue4.reload
+
+      expect(issue4.position).to eq(42)
+    end
+  end
 end