Commit bbf01ed5 authored by Igor Drozdov's avatar Igor Drozdov

Reduce SQL requests number for epic issues

When API request performed for multiple milestone issues
the number of SQL requests depends on the number of issues.

There is still an N + 1 for calculating subscribed? field,
but we'll tackle it in:
https://gitlab.com/gitlab-org/gitlab/-/issues/325898

Also pagination is added to limit the returned results
parent c1d45f9c
...@@ -14,6 +14,11 @@ results in a `404` status code. ...@@ -14,6 +14,11 @@ results in a `404` status code.
Epics are available only in GitLab [Premium and higher](https://about.gitlab.com/pricing/). Epics are available only in GitLab [Premium and higher](https://about.gitlab.com/pricing/).
If the Epics feature is not available, a `403` status code is returned. If the Epics feature is not available, a `403` status code is returned.
## Epic Issues pagination
API results [are paginated](README.md#pagination). Requests that return
multiple issues default to returning 20 results at a time.
## List issues for an epic ## List issues for an epic
Gets all issues that are assigned to an epic and the authenticated user has access to. Gets all issues that are assigned to an epic and the authenticated user has access to.
......
...@@ -285,10 +285,7 @@ module EE ...@@ -285,10 +285,7 @@ module EE
end end
def related_issues(ids: nil, preload: nil) def related_issues(ids: nil, preload: nil)
items = ::Issue.select('issues.*, epic_issues.id as epic_issue_id, epic_issues.relative_position, epic_issues.epic_id as epic_id') items = ::Issue.preload(preload).sorted_by_epic_position
.joins(:epic_issue)
.preload(preload)
.order('epic_issues.relative_position, epic_issues.id')
return items unless ids return items unless ids
......
...@@ -30,6 +30,7 @@ module EE ...@@ -30,6 +30,7 @@ module EE
scope :any_epic, -> { joins(:epic_issue) } scope :any_epic, -> { joins(:epic_issue) }
scope :in_epics, ->(epics) { joins(:epic_issue).where(epic_issues: { epic_id: epics }) } scope :in_epics, ->(epics) { joins(:epic_issue).where(epic_issues: { epic_id: epics }) }
scope :not_in_epics, ->(epics) { left_outer_joins(:epic_issue).where('epic_issues.epic_id NOT IN (?) OR epic_issues.epic_id IS NULL', epics) } scope :not_in_epics, ->(epics) { left_outer_joins(:epic_issue).where('epic_issues.epic_id NOT IN (?) OR epic_issues.epic_id IS NULL', epics) }
scope :sorted_by_epic_position, -> { joins(:epic_issue).select('issues.*, epic_issues.id as epic_issue_id, epic_issues.relative_position, epic_issues.epic_id as epic_id').order('epic_issues.relative_position, epic_issues.id') }
scope :no_iteration, -> { where(sprint_id: nil) } scope :no_iteration, -> { where(sprint_id: nil) }
scope :any_iteration, -> { where.not(sprint_id: nil) } scope :any_iteration, -> { where.not(sprint_id: nil) }
scope :in_iterations, ->(iterations) { where(sprint_id: iterations) } scope :in_iterations, ->(iterations) { where(sprint_id: iterations) }
......
---
title: Reduce SQL requests number for epic issues
merge_request: 57352
author:
type: performance
...@@ -2,6 +2,8 @@ ...@@ -2,6 +2,8 @@
module API module API
class EpicIssues < ::API::Base class EpicIssues < ::API::Base
include PaginationParams
feature_category :epics feature_category :epics
before do before do
...@@ -15,6 +17,12 @@ module API ...@@ -15,6 +17,12 @@ module API
def link def link
@link ||= epic.epic_issues.find(params[:epic_issue_id]) @link ||= epic.epic_issues.find(params[:epic_issue_id])
end end
def related_issues(epic)
IssuesFinder.new(current_user, { epic_id: epic.id }).execute
.with_api_entity_associations
.sorted_by_epic_position
end
end end
params do params do
...@@ -29,6 +37,7 @@ module API ...@@ -29,6 +37,7 @@ module API
requires :epic_issue_id, type: Integer, desc: 'The ID of the epic issue association to update' requires :epic_issue_id, type: Integer, desc: 'The ID of the epic issue association to update'
optional :move_before_id, type: Integer, desc: 'The ID of the epic issue association that should be positioned before the actual issue' optional :move_before_id, type: Integer, desc: 'The ID of the epic issue association that should be positioned before the actual issue'
optional :move_after_id, type: Integer, desc: 'The ID of the epic issue association that should be positioned after the actual issue' optional :move_after_id, type: Integer, desc: 'The ID of the epic issue association that should be positioned after the actual issue'
use :pagination
end end
put ':id/(-/)epics/:epic_iid/issues/:epic_issue_id' do put ':id/(-/)epics/:epic_iid/issues/:epic_issue_id' do
authorize_can_admin_epic! authorize_can_admin_epic!
...@@ -40,10 +49,8 @@ module API ...@@ -40,10 +49,8 @@ module API
result = ::EpicIssues::UpdateService.new(link, current_user, update_params).execute result = ::EpicIssues::UpdateService.new(link, current_user, update_params).execute
# For now we return empty body
# The issues list in the correct order in body will be returned as part of #4250
if result if result
present epic.issues_readable_by(current_user), present paginate(related_issues(epic)),
with: EE::API::Entities::EpicIssue, with: EE::API::Entities::EpicIssue,
current_user: current_user current_user: current_user
else else
...@@ -56,12 +63,13 @@ module API ...@@ -56,12 +63,13 @@ module API
end end
params do params do
requires :epic_iid, type: Integer, desc: 'The IID of the epic' requires :epic_iid, type: Integer, desc: 'The IID of the epic'
use :pagination
end end
[':id/epics/:epic_iid/issues', ':id/-/epics/:epic_iid/issues'].each do |path| [':id/epics/:epic_iid/issues', ':id/-/epics/:epic_iid/issues'].each do |path|
get path do get path do
authorize_can_read! authorize_can_read!
present epic.issues_readable_by(current_user), present paginate(related_issues(epic)),
with: EE::API::Entities::EpicIssue, with: EE::API::Entities::EpicIssue,
current_user: current_user current_user: current_user
end end
......
...@@ -75,8 +75,8 @@ RSpec.describe Issue do ...@@ -75,8 +75,8 @@ RSpec.describe Issue do
context 'epics' do context 'epics' do
let_it_be(:epic1) { create(:epic) } let_it_be(:epic1) { create(:epic) }
let_it_be(:epic2) { create(:epic) } let_it_be(:epic2) { create(:epic) }
let_it_be(:epic_issue1) { create(:epic_issue, epic: epic1) } let_it_be(:epic_issue1) { create(:epic_issue, epic: epic1, relative_position: 2) }
let_it_be(:epic_issue2) { create(:epic_issue, epic: epic2) } let_it_be(:epic_issue2) { create(:epic_issue, epic: epic2, relative_position: 1) }
let_it_be(:issue_no_epic) { create(:issue) } let_it_be(:issue_no_epic) { create(:issue) }
before do before do
...@@ -128,6 +128,12 @@ RSpec.describe Issue do ...@@ -128,6 +128,12 @@ RSpec.describe Issue do
end end
end end
end end
describe '.sorted_by_epic_position' do
it 'sorts by epic relative position' do
expect(described_class.sorted_by_epic_position.ids).to eq([epic_issue2.issue_id, epic_issue1.issue_id])
end
end
end end
context 'iterations' do context 'iterations' do
......
...@@ -48,16 +48,38 @@ RSpec.describe API::EpicIssues do ...@@ -48,16 +48,38 @@ RSpec.describe API::EpicIssues do
let!(:epic_issue1) { create(:epic_issue, epic: epic, issue: issues[0]) } let!(:epic_issue1) { create(:epic_issue, epic: epic, issue: issues[0]) }
let!(:epic_issue2) { create(:epic_issue, epic: epic, issue: issues[1]) } let!(:epic_issue2) { create(:epic_issue, epic: epic, issue: issues[1]) }
before do def perform_request(params = {})
get api(url, user) get api(url, user), params: params
end end
it 'returns 200 status' do it 'responds 200 and matches the response schema' do
perform_request
expect(response).to have_gitlab_http_status(:ok) expect(response).to have_gitlab_http_status(:ok)
expect(response).to match_response_schema('public_api/v4/epic_issues', dir: 'ee')
expect(response.parsed_body.size).to eq(2)
end end
it 'matches the response schema' do it 'accepts pagination params' do
perform_request({ per_page: 1 })
expect(response).to have_gitlab_http_status(:ok)
expect(response).to match_response_schema('public_api/v4/epic_issues', dir: 'ee') expect(response).to match_response_schema('public_api/v4/epic_issues', dir: 'ee')
expect(response.parsed_body.size).to eq(1)
end
context 'returns multiple issues without performing N + 1' do
it 'returns multiple issues without performing N + 1' do
perform_request
control_count = ActiveRecord::QueryRecorder.new { perform_request }.count
issue = create(:issue, project: project)
create(:epic_issue, epic: epic, issue: issue)
# Existing N + 1 for calculating subscribed? field: https://gitlab.com/gitlab-org/gitlab/-/issues/325898
expect { perform_request }.not_to exceed_query_limit(control_count + 2)
end
end end
end end
end end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment