Merge branch 'jp-epic-issues-list' into 'master'

Remove CachingArrayResolver from epic issues

See merge request gitlab-org/gitlab!80635

ee/app/graphql/resolvers/epic_issues_resolver.rb

@@ -2,30 +2,16 @@
 
 module Resolvers
   class EpicIssuesResolver < BaseResolver
-    include CachingArrayResolver
-
     type Types::EpicIssueType.connection_type, null: true
 
     alias_method :epic, :object
 
-    def model_class
-      ::Issue
-    end
-
-    def allowed?(issue)
-      DeclarativePolicy.user_scope { issue.visible_to_user?(current_user) }
-    end
-
-    def query_input(**args)
-      epic.id
-    end
-
-    def query_for(id)
-      ::Epic.related_issues(ids: id)
-    end
-
-    def preload
-      { project: [:namespace, :project_feature] }
+    # because epic issues are ordered by EpicIssue's relative position,
+    # we can not use batch loading to load epic issues for multiple epics at once
+    # (assuming we don't load all issues for each epic but only a single page)
+    def resolve
+      issues = Epic.related_issues(ids: epic.id, preload: { project: [:namespace, :project_feature] })
+      offset_pagination(issues)
     end
   end
 end

ee/spec/graphql/resolvers/epic_issues_resolver_spec.rb

@@ -5,8 +5,8 @@ require 'spec_helper'
 RSpec.describe Resolvers::EpicIssuesResolver do
   include GraphqlHelpers
 
-  let_it_be(:current_user) { create(:user) }
-  let_it_be(:user) { create(:user) }
+  let_it_be(:developer) { create(:user) }
+  let_it_be(:guest) { create(:user) }
   let_it_be(:group) { create(:group, :public) }
   let_it_be(:project1) { create(:project, :public, group: group) }
   let_it_be(:project2) { create(:project, :private, group: group) }
@@ -16,13 +16,21 @@ RSpec.describe Resolvers::EpicIssuesResolver do
   let_it_be(:issue2) { create(:issue, project: project1, confidential: true) }
   let_it_be(:issue3) { create(:issue, project: project2) }
   let_it_be(:issue4) { create(:issue, project: project2) }
+  let_it_be(:issue5) { create(:issue, project: project1) }
   let_it_be(:epic_issue1) { create(:epic_issue, epic: epic1, issue: issue1, relative_position: 3) }
   let_it_be(:epic_issue2) { create(:epic_issue, epic: epic1, issue: issue2, relative_position: 2) }
   let_it_be(:epic_issue3) { create(:epic_issue, epic: epic2, issue: issue3, relative_position: 1) }
   let_it_be(:epic_issue4) { create(:epic_issue, epic: epic2, issue: issue4, relative_position: nil) }
+  let_it_be(:epic_issue5) { create(:epic_issue, epic: epic1, issue: issue5, relative_position: nil) }
+
+  let(:schema) do
+    Class.new(GitlabSchema) do
+      default_max_page_size 100
+    end
+  end
 
   before do
-    group.add_developer(current_user)
+    group.add_developer(developer)
     stub_licensed_features(epics: true)
   end
 
@@ -31,28 +39,31 @@ RSpec.describe Resolvers::EpicIssuesResolver do
   end
 
   describe '#resolve' do
-    let(:epics) { [epic1, epic2] }
-
-    it 'finds all epic issues' do
-      result = epics.map { |epic| resolve_epic_issues(epic).to_a }
+    using RSpec::Parameterized::TableSyntax
 
-      expect(result).to eq [[issue2, issue1], [issue3, issue4]]
+    where(:epic, :user, :max_page_size, :has_next_page, :issues) do
+      ref(:epic1) | ref(:developer) | 100 | false | lazy { [issue2, issue1, issue5] }
+      ref(:epic1) | ref(:developer) | 2   | true  | lazy { [issue2, issue1] }
+      ref(:epic1) | ref(:guest)     | 100 | false | lazy { [issue1, issue5] }
+      ref(:epic2) | ref(:developer) | 100 | false | lazy { [issue3, issue4] }
+      ref(:epic2) | ref(:guest)     | 100 | false | lazy { [] }
     end
 
-    it 'finds only epic issues that user can read' do
-      guest = create(:user)
+    with_them do
+      it 'returns only a page of issues user can read' do
+        result = resolve_epic_issues(epic, user, max_page_size)
 
-      result =
-        [
-          resolve_epic_issues(epic1, user: guest).to_a,
-          resolve_epic_issues(epic2, user: guest).to_a
-        ]
-
-      expect(result).to eq([[issue1], []])
+        expect(result.to_a).to eq issues
+        expect(result.has_next_page).to eq has_next_page
+      end
     end
   end
 
-  def resolve_epic_issues(object, user: current_user)
-    force(resolve(described_class, obj: object, ctx: { current_user: user }))
+  def resolve_epic_issues(object, user, max_page_size)
+    resolver = described_class
+    opts = resolver.field_options
+    allow(resolver).to receive(:field_options).and_return(opts.merge(max_page_size: max_page_size))
+
+    force(resolve(resolver, obj: object, ctx: { current_user: user }))
   end
 end

ee/spec/requests/api/graphql/group/epic/epic_issues_spec.rb

@@ -124,17 +124,24 @@ RSpec.describe 'Getting issues for an epic' do
         expect(result[epic2.iid]).to match_array [issue2.to_global_id.to_s]
       end
 
-      it 'avoids N+1 queries' do
-        user_1 = create(:user, developer_projects: [project])
-        user_2 = create(:user, developer_projects: [project])
+      it 'does limited number of N+1 queries' do
+        # extra queries:
+        # epic_issues - for each epic issues are loaded ordered by relatve_position
+        # issue_assignees - issue policy checks if user is between issue assignees
+        # when https://gitlab.com/gitlab-org/gitlab/-/issues/353375 is fixed, we can
+        # preload also issue assignees
+        extra_queries_count = 2
+
+        # warm-up query
+        post_graphql(epic_query(iid: epic.iid), current_user: user)
 
         control_count = ActiveRecord::QueryRecorder.new(query_recorder_debug: true) do
-          post_graphql(epic_query(iid: epic.iid), current_user: user_1)
+          post_graphql(epic_query(iid: epic.iid), current_user: user)
         end
 
         expect do
-          post_graphql(epic_query(params), current_user: user_2)
-        end.not_to exceed_query_limit(control_count).ignoring(/FROM "namespaces"/)
+          post_graphql(epic_query(params), current_user: user)
+        end.not_to exceed_query_limit(control_count).with_threshold(extra_queries_count)
 
         expect(graphql_errors).to be_nil
       end