From c871faa3e42edf7c1d92669cf18a5b0de7c84ace Mon Sep 17 00:00:00 2001 From: Chantal Rollison Date: Mon, 10 Sep 2018 10:26:33 -0700 Subject: Add preload in issues controller --- app/controllers/boards/issues_controller.rb | 17 +++-- app/models/concerns/relative_positioning.rb | 79 +++++++++++++--------- app/models/list.rb | 1 + app/services/boards/lists/list_service.rb | 2 +- .../ccr-43034_issues_controller_100_queries.yml | 5 ++ spec/controllers/boards/issues_controller_spec.rb | 49 +++++++++++++- spec/models/concerns/relative_positioning_spec.rb | 34 +++++++++- 7 files changed, 141 insertions(+), 46 deletions(-) create mode 100644 changelogs/unreleased/ccr-43034_issues_controller_100_queries.yml diff --git a/app/controllers/boards/issues_controller.rb b/app/controllers/boards/issues_controller.rb index 4f3d737e3ce..7f874687212 100644 --- a/app/controllers/boards/issues_controller.rb +++ b/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 diff --git a/app/models/concerns/relative_positioning.rb b/app/models/concerns/relative_positioning.rb index 85229cded5d..045bf392ac8 100644 --- a/app/models/concerns/relative_positioning.rb +++ b/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 diff --git a/app/models/list.rb b/app/models/list.rb index 1a30acc83cf..029685be927 100644 --- a/app/models/list.rb +++ b/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 diff --git a/app/services/boards/lists/list_service.rb b/app/services/boards/lists/list_service.rb index e10eb52e041..5cf5f14a55b 100644 --- a/app/services/boards/lists/list_service.rb +++ b/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 diff --git a/changelogs/unreleased/ccr-43034_issues_controller_100_queries.yml b/changelogs/unreleased/ccr-43034_issues_controller_100_queries.yml new file mode 100644 index 00000000000..d92c0e74c07 --- /dev/null +++ b/changelogs/unreleased/ccr-43034_issues_controller_100_queries.yml @@ -0,0 +1,5 @@ +--- +title: Add preload for routes and namespaces for issues controller. +merge_request: 21651 +author: +type: performance diff --git a/spec/controllers/boards/issues_controller_spec.rb b/spec/controllers/boards/issues_controller_spec.rb index d98e6ff0df8..c365988a100 100644 --- a/spec/controllers/boards/issues_controller_spec.rb +++ b/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 diff --git a/spec/models/concerns/relative_positioning_spec.rb b/spec/models/concerns/relative_positioning_spec.rb index 729056b6abc..66c1f47d12b 100644 --- a/spec/models/concerns/relative_positioning_spec.rb +++ b/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) -- cgit v1.2.1