Merge branch 'id-n-1-for-epic-issues' into 'master'

Reduce SQL requests number for epic issues

See merge request gitlab-org/gitlab!57352

doc/api/epic_issues.md

@@ -14,6 +14,11 @@ results in a `404` status code.
 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.
 
+## 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
 
 Gets all issues that are assigned to an epic and the authenticated user has access to.

ee/app/models/ee/epic.rb

@@ -285,10 +285,7 @@ module EE
       end
 
       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')
-          .joins(:epic_issue)
-          .preload(preload)
-          .order('epic_issues.relative_position, epic_issues.id')
+        items = ::Issue.preload(preload).sorted_by_epic_position
 
         return items unless ids
 

ee/app/models/ee/issue.rb

@@ -30,6 +30,7 @@ module EE
       scope :any_epic, -> { joins(:epic_issue) }
       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 :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 :any_iteration, -> { where.not(sprint_id: nil) }
       scope :in_iterations, ->(iterations) { where(sprint_id: iterations) }

ee/changelogs/unreleased/id-n-1-for-epic-issues.yml

+---
+title: Reduce SQL requests number for epic issues
+merge_request: 57352
+author:
+type: performance

ee/lib/api/epic_issues.rb

@@ -2,6 +2,8 @@
 
 module API
   class EpicIssues < ::API::Base
+    include PaginationParams
+
     feature_category :epics
 
     before do
@@ -15,6 +17,12 @@ module API
       def link
         @link ||= epic.epic_issues.find(params[:epic_issue_id])
       end
+
+      def related_issues(epic)
+        IssuesFinder.new(current_user, { epic_id: epic.id }).execute
+          .with_api_entity_associations
+          .sorted_by_epic_position
+      end
     end
 
     params do
@@ -29,6 +37,7 @@ module API
         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_after_id, type: Integer, desc: 'The ID of the epic issue association that should be positioned after the actual issue'
+        use :pagination
       end
       put ':id/(-/)epics/:epic_iid/issues/:epic_issue_id' do
         authorize_can_admin_epic!
@@ -40,10 +49,8 @@ module API
 
         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
-          present epic.issues_readable_by(current_user),
+          present paginate(related_issues(epic)),
             with: EE::API::Entities::EpicIssue,
             current_user: current_user
         else
@@ -56,12 +63,13 @@ module API
       end
       params do
         requires :epic_iid, type: Integer, desc: 'The IID of the epic'
+        use :pagination
       end
       [':id/epics/:epic_iid/issues', ':id/-/epics/:epic_iid/issues'].each do |path|
         get path do
           authorize_can_read!
 
-          present epic.issues_readable_by(current_user),
+          present paginate(related_issues(epic)),
             with: EE::API::Entities::EpicIssue,
             current_user: current_user
         end

ee/spec/models/issue_spec.rb

@@ -75,8 +75,8 @@ RSpec.describe Issue do
     context 'epics' do
       let_it_be(:epic1) { create(:epic) }
       let_it_be(:epic2) { create(:epic) }
-      let_it_be(:epic_issue1) { create(:epic_issue, epic: epic1) }
-      let_it_be(:epic_issue2) { create(:epic_issue, epic: epic2) }
+      let_it_be(:epic_issue1) { create(:epic_issue, epic: epic1, relative_position: 2) }
+      let_it_be(:epic_issue2) { create(:epic_issue, epic: epic2, relative_position: 1) }
       let_it_be(:issue_no_epic) { create(:issue) }
 
       before do
@@ -128,6 +128,12 @@ RSpec.describe Issue do
           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
 
     context 'iterations' do

ee/spec/requests/api/epic_issues_spec.rb

@@ -48,16 +48,38 @@ RSpec.describe API::EpicIssues do
         let!(:epic_issue1) { create(:epic_issue, epic: epic, issue: issues[0]) }
         let!(:epic_issue2) { create(:epic_issue, epic: epic, issue: issues[1]) }
 
-        before do
-          get api(url, user)
+        def perform_request(params = {})
+          get api(url, user), params: params
         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 match_response_schema('public_api/v4/epic_issues', dir: 'ee')
+          expect(response.parsed_body.size).to eq(2)
         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.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