Merge branch 'ccr/43034_issues_controller_100_queries' into 'master'

Add preload for routes and namespaces for issues controller.

Closes #43034

See merge request gitlab-org/gitlab-ce!21651

app/controllers/boards/issues_controller.rb

@@ -18,10 +18,15 @@ module Boards
       list_service = Boards::Issues::ListService.new(board_parent, current_user, filter_params)
       issues = list_service.execute
       issues = issues.page(params[:page]).per(params[:per] || 20).without_count
-      make_sure_position_is_set(issues) if Gitlab::Database.read_write?
-      issues = issues.preload(:project,
-                              :milestone,
+      Issue.move_to_end(issues) if Gitlab::Database.read_write?
+      issues = issues.preload(:milestone,
                               :assignees,
+                              project: [
+                                  :route,
+                                  {
+                                      namespace: [:route]
+                                  }
+                              ],
                               labels: [:priorities],
                               notes: [:award_emoji, :author]
                              )
@@ -60,12 +65,6 @@ module Boards
       render json: data
     end
 
-    def make_sure_position_is_set(issues)
-      issues.each do |issue|
-        issue.move_to_end && issue.save unless issue.relative_position
-      end
-    end
-
     def issue
       @issue ||= issues_finder.find(params[:id])
     end

app/models/concerns/relative_positioning.rb

@@ -12,6 +12,49 @@ module RelativePositioning
     after_save :save_positionable_neighbours
   end
 
+  class_methods do
+    def move_to_end(objects)
+      parent_ids = objects.map(&:parent_ids).flatten.uniq
+      max_relative_position = in_parents(parent_ids).maximum(:relative_position) || START_POSITION
+      objects = objects.reject(&:relative_position)
+
+      self.transaction do
+        objects.each do |object|
+          relative_position = position_between(max_relative_position, MAX_POSITION)
+          object.relative_position = relative_position
+          max_relative_position = relative_position
+          object.save
+        end
+      end
+    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 are incrementing position by IDEAL_DISTANCE * 2 every time
+    # when we have enough space. If distance is less then IDEAL_DISTANCE we are calculating an average number
+    def position_between(pos_before, pos_after)
+      pos_before ||= MIN_POSITION
+      pos_after ||= MAX_POSITION
+
+      pos_before, pos_after = [pos_before, pos_after].sort
+
+      halfway = (pos_after + pos_before) / 2
+      distance_to_halfway = pos_after - halfway
+
+      if distance_to_halfway < IDEAL_DISTANCE
+        halfway
+      else
+        if pos_before == MIN_POSITION
+          pos_after - IDEAL_DISTANCE
+        elsif pos_after == MAX_POSITION
+          pos_before + IDEAL_DISTANCE
+        else
+          halfway
+        end
+      end
+    end
+  end
+
   def min_relative_position
     self.class.in_parents(parent_ids).minimum(:relative_position)
   end
@@ -57,7 +100,7 @@ module RelativePositioning
       @positionable_neighbours = [before] # rubocop:disable Gitlab/ModuleWithInstanceVariables
     end
 
-    self.relative_position = position_between(before.relative_position, after.relative_position)
+    self.relative_position = self.class.position_between(before.relative_position, after.relative_position)
   end
 
   def move_after(before = self)
@@ -72,7 +115,7 @@ module RelativePositioning
       pos_after = issue_to_move.relative_position
     end
 
-    self.relative_position = position_between(pos_before, pos_after)
+    self.relative_position = self.class.position_between(pos_before, pos_after)
   end
 
   def move_before(after = self)
@@ -87,15 +130,15 @@ module RelativePositioning
       pos_before = issue_to_move.relative_position
     end
 
-    self.relative_position = position_between(pos_before, pos_after)
+    self.relative_position = self.class.position_between(pos_before, pos_after)
   end
 
   def move_to_end
-    self.relative_position = position_between(max_relative_position || START_POSITION, MAX_POSITION)
+    self.relative_position = self.class.position_between(max_relative_position || START_POSITION, MAX_POSITION)
   end
 
   def move_to_start
-    self.relative_position = position_between(min_relative_position || START_POSITION, MIN_POSITION)
+    self.relative_position = self.class.position_between(min_relative_position || START_POSITION, MIN_POSITION)
   end
 
   # Indicates if there is an issue that should be shifted to free the place
@@ -112,32 +155,6 @@ module RelativePositioning
 
   private
 
-  # 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 are incrementing position by IDEAL_DISTANCE * 2 every time
-  # when we have enough space. If distance is less then IDEAL_DISTANCE we are calculating an average number
-  def position_between(pos_before, pos_after)
-    pos_before ||= MIN_POSITION
-    pos_after ||= MAX_POSITION
-
-    pos_before, pos_after = [pos_before, pos_after].sort
-
-    halfway = (pos_after + pos_before) / 2
-    distance_to_halfway = pos_after - halfway
-
-    if distance_to_halfway < IDEAL_DISTANCE
-      halfway
-    else
-      if pos_before == MIN_POSITION
-        pos_after - IDEAL_DISTANCE
-      elsif pos_after == MAX_POSITION
-        pos_before + IDEAL_DISTANCE
-      else
-        halfway
-      end
-    end
-  end
-
   # rubocop:disable Gitlab/ModuleWithInstanceVariables
   def save_positionable_neighbours
     return unless @positionable_neighbours

app/models/list.rb

@@ -15,6 +15,7 @@ class List < ActiveRecord::Base
 
   scope :destroyable, -> { where(list_type: list_types.slice(*destroyable_types).values) }
   scope :movable, -> { where(list_type: list_types.slice(*movable_types).values) }
+  scope :preload_associations, -> { preload(:board, :label) }
 
   class << self
     def destroyable_types

app/services/boards/lists/list_service.rb

@@ -6,7 +6,7 @@ module Boards
       def execute(board)
         board.lists.create(list_type: :backlog) unless board.lists.backlog.exists?
 
-        board.lists
+        board.lists.preload_associations
       end
     end
   end

changelogs/unreleased/ccr-43034_issues_controller_100_queries.yml

+---
+title: Add preload for routes and namespaces for issues controller.
+merge_request: 21651
+author:
+type: performance

spec/controllers/boards/issues_controller_spec.rb

@@ -30,6 +30,15 @@ describe Boards::IssuesController do
 
     context 'when list id is present' do
       context 'with valid list id' do
+        let(:group) { create(:group, :private, projects: [project]) }
+        let(:group_board) { create(:board, group: group) }
+        let!(:list3) { create(:list, board: group_board, label: development, position: 2) }
+        let(:sub_group_1) { create(:group, :private, parent: group) }
+
+        before do
+          group.add_maintainer(user)
+        end
+
         it 'returns issues that have the list label applied' do
           issue = create(:labeled_issue, project: project, labels: [planning])
           create(:labeled_issue, project: project, labels: [planning])
@@ -56,6 +65,39 @@ describe Boards::IssuesController do
 
           expect { list_issues(user: user, board: board, list: list2) }.not_to exceed_query_limit(control_count)
         end
+
+        it 'avoids N+1 database queries when adding a project', :request_store do
+          create(:labeled_issue, project: project, labels: [development])
+          control_count = ActiveRecord::QueryRecorder.new { list_issues(user: user, board: group_board, list: list3) }.count
+
+          2.times do
+            p = create(:project, group: group)
+            create(:labeled_issue, project: p, labels: [development])
+          end
+
+          project_2 = create(:project, group: group)
+          create(:labeled_issue, project: project_2, labels: [development], assignees: [johndoe])
+
+          # because each issue without relative_position must be updated with
+          # a different value, we have 8 extra queries per issue
+          expect { list_issues(user: user, board: group_board, list: list3) }.not_to exceed_query_limit(control_count + (2 * 8 - 1))
+        end
+
+        it 'avoids N+1 database queries when adding a subgroup, project, and issue', :nested_groups do
+          create(:project, group: sub_group_1)
+          create(:labeled_issue, project: project, labels: [development])
+          control_count = ActiveRecord::QueryRecorder.new { list_issues(user: user, board: group_board, list: list3) }.count
+          project_2 = create(:project, group: group)
+
+          2.times do
+            p = create(:project, group: sub_group_1)
+            create(:labeled_issue, project: p, labels: [development])
+          end
+
+          create(:labeled_issue, project: project_2, labels: [development], assignees: [johndoe])
+
+          expect { list_issues(user: user, board: group_board, list: list3) }.not_to exceed_query_limit(control_count + (2 * 8 - 1))
+        end
       end
 
       context 'with invalid list id' do
@@ -102,12 +144,15 @@ describe Boards::IssuesController do
       sign_in(user)
 
       params = {
-        namespace_id: project.namespace.to_param,
-        project_id: project,
         board_id: board.to_param,
         list_id: list.try(:to_param)
       }
 
+      unless board.try(:parent)&.is_a?(Group)
+        params[:namespace_id] = project.namespace.to_param
+        params[:project_id] = project
+      end
+
       get :index, params.compact
     end
   end

spec/models/concerns/relative_positioning_spec.rb

@@ -6,9 +6,13 @@ describe RelativePositioning do
   let(:issue1) { create(:issue, project: project) }
   let(:new_issue) { create(:issue, project: project) }
 
-  before do
-    [issue, issue1].each do |issue|
-      issue.move_to_end && issue.save
+  describe '.move_to_end' do
+    it 'moves the object to the end' do
+      Issue.move_to_end([issue, issue1])
+
+      expect(issue1.prev_relative_position).to eq issue.relative_position
+      expect(issue.prev_relative_position).to eq nil
+      expect(issue1.next_relative_position).to eq nil
     end
   end
 
@@ -59,6 +63,12 @@ describe RelativePositioning do
   end
 
   describe '#move_to_end' do
+    before do
+      [issue, issue1].each do |issue|
+        issue.move_to_end && issue.save
+      end
+    end
+
     it 'moves issue to the end' do
       new_issue.move_to_end
 
@@ -67,6 +77,12 @@ describe RelativePositioning do
   end
 
   describe '#shift_after?' do
+    before do
+      [issue, issue1].each do |issue|
+        issue.move_to_end && issue.save
+      end
+    end
+
     it 'returns true' do
       issue.update(relative_position: issue1.relative_position - 1)
 
@@ -81,6 +97,12 @@ describe RelativePositioning do
   end
 
   describe '#shift_before?' do
+    before do
+      [issue, issue1].each do |issue|
+        issue.move_to_end && issue.save
+      end
+    end
+
     it 'returns true' do
       issue.update(relative_position: issue1.relative_position + 1)
 
@@ -95,6 +117,12 @@ describe RelativePositioning do
   end
 
   describe '#move_between' do
+    before do
+      [issue, issue1].each do |issue|
+        issue.move_to_end && issue.save
+      end
+    end
+
     it 'positions issue between two other' do
       new_issue.move_between(issue, issue1)