Merge branch 'epic-issues-loading' into 'master'

Use batch loader for loading epic issues

See merge request gitlab-org/gitlab!19141

doc/api/graphql/reference/gitlab_schema.graphql

@@ -1219,6 +1219,10 @@ type Epic implements Noteable {
   hasIssues: Boolean!
   id: ID!
   iid: ID!
+
+  """
+  A list of issues associated with the epic
+  """
   issues(
     """
     Returns the elements in the list that come after the specified cursor.

doc/api/graphql/reference/gitlab_schema.json

@@ -3751,7 +3751,7 @@
             },
             {
               "name": "issues",
-              "description": null,
+              "description": "A list of issues associated with the epic",
               "args": [
                 {
                   "name": "after",

ee/app/graphql/resolvers/epic_issues_resolver.rb

@@ -6,8 +6,20 @@ module Resolvers
 
     alias_method :epic, :object
 
+    # When using EpicIssuesResolver then epic's issues are authorized when
+    # rendering lazy-loaded issues, we explicitly ignore any inherited
+    # type_authorizations to avoid excuting any authorization checks in earlier
+    # phase
+    def self.skip_authorizations?
+      true
+    end
+
     def resolve(**args)
-      epic.issues_readable_by(context[:current_user])
+      filter = proc do |issues|
+        Ability.issues_readable_by_user(issues, context[:current_user])
+      end
+
+      Gitlab::Graphql::Loaders::BatchEpicIssuesLoader.new(epic.id, filter).find
     end
   end
 end

ee/app/graphql/types/epic_issue_type.rb

@@ -14,7 +14,7 @@ module Types
       issue.group_epic_issue_path(ctx[:current_user])
     end
 
-    field :id, GraphQL::ID_TYPE, null: true, resolve: -> (issue) do
+    field :id, GraphQL::ID_TYPE, null: true, resolve: -> (issue, args, ctx) do
       issue.to_global_id
     end, description: 'The global id of the epic-issue relation'
 

ee/app/graphql/types/epic_type.rb

@@ -67,9 +67,11 @@ module Types
       complexity: 5,
       description: 'Boolean flag for whether the currently logged in user is subscribed to this epic'
 
-    field :issues, # rubocop:disable Graphql/Descriptions
+    field :issues,
           Types::EpicIssueType.connection_type,
           null: true,
+          complexity: 2,
+          description: 'A list of issues associated with the epic',
           resolver: Resolvers::EpicIssuesResolver
 
     field :descendant_counts, Types::EpicDescendantCountType, null: true, complexity: 10,

ee/app/models/ee/epic.rb

@@ -203,6 +203,14 @@ module EE
           groups.select { |g| Ability.allowed?(user, :read_epic, g) }
         end
       end
+
+      def related_issues(ids:, preload: nil)
+        ::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)
+          .where("epic_issues.epic_id": ids)
+          .order('epic_issues.relative_position, epic_issues.id')
+      end
     end
 
     def resource_parent
@@ -337,11 +345,7 @@ module EE
     end
 
     def issues_readable_by(current_user, preload: nil)
-      related_issues = ::Issue.select('issues.*, epic_issues.id as epic_issue_id, epic_issues.relative_position')
-        .joins(:epic_issue)
-        .preload(preload)
-        .where("epic_issues.epic_id = #{id}")
-        .order('epic_issues.relative_position, epic_issues.id')
+      related_issues = self.class.related_issues(ids: id, preload: preload)
 
       Ability.issues_readable_by_user(related_issues, current_user)
     end

ee/lib/gitlab/graphql/loaders/batch_epic_issues_loader.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Graphql
+    module Loaders
+      class BatchEpicIssuesLoader
+        # this will assure that no more than 100 queries will be done to fetch issues
+        MAX_LOADED_ISSUES = 100_000
+
+        def initialize(model_id, authorization_filter)
+          @model_id = model_id
+          @authorization_filter = authorization_filter
+        end
+
+        def find
+          BatchLoader::GraphQL.for(@model_id).batch(default_value: []) do |ids, loader|
+            issues = ::Epic.related_issues(ids: ids, preload: { project: [:namespace, :project_feature] })
+            load_issues(loader, issues)
+          end
+        end
+
+        private
+
+        # rubocop: disable CodeReuse/ActiveRecord
+        def load_issues(loader, issues)
+          issues.find_each(batch_size: 1000).with_index do |issue, idx|
+            if idx > MAX_LOADED_ISSUES
+              raise Gitlab::Graphql::Errors::ArgumentError, 'Too many epic issues requested.'
+            end
+
+            loader.call(issue.epic_id) do |memo|
+              unless memo.is_a?(Gitlab::Graphql::FilterableArray)
+                # memo is an empty array by default
+                memo = Gitlab::Graphql::FilterableArray.new(@authorization_filter)
+              end
+
+              memo << issue
+            end
+          end
+        end
+        # rubocop: enable CodeReuse/ActiveRecord
+      end
+    end
+  end
+end

ee/spec/graphql/resolvers/epic_issues_resolver_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Resolvers::EpicIssuesResolver do
+  include GraphqlHelpers
+
+  let_it_be(:current_user) { create(:user) }
+  let_it_be(:user) { create(:user) }
+  let_it_be(:group) { create(:group) }
+  let_it_be(:project1) { create(:project, :public, group: group) }
+  let_it_be(:project2) { create(:project, :private, group: group) }
+  let_it_be(:epic1) { create(:epic, group: group) }
+  let_it_be(:epic2) { create(:epic, group: group) }
+  let_it_be(:issue1) { create(:issue, project: project1) }
+  let_it_be(:issue2) { create(:issue, project: project1) }
+  let_it_be(:issue3) { create(:issue, project: project2) }
+  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) }
+
+  before do
+    group.add_developer(current_user)
+    stub_licensed_features(epics: true)
+  end
+
+  describe '#resolve' do
+    it 'finds all epic issues' do
+      result = batch_sync(max_queries: 4) { resolve_epic_issues(epic1) }
+
+      expect(result).to contain_exactly(issue1, issue2)
+    end
+
+    it 'can batch-resolve epic issues from different epics' do
+      result = batch_sync(max_queries: 4) do
+        [resolve_epic_issues(epic1), resolve_epic_issues(epic2)]
+      end
+
+      expect(result).to contain_exactly([issue1, issue2], [issue3])
+    end
+  end
+
+  def resolve_epic_issues(object, args = {}, context = { current_user: current_user })
+    resolve(described_class, obj: object, args: args, ctx: context)
+  end
+end

ee/spec/lib/gitlab/graphql/loaders/batch_epic_issues_loader_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::Graphql::Loaders::BatchEpicIssuesLoader do
+  describe '#find' do
+    let_it_be(:user) { create(:user) }
+    let_it_be(:group) { create(:group) }
+    let_it_be(:project1) { create(:project, :public, group: group) }
+    let_it_be(:project2) { create(:project, :private, group: group) }
+    let_it_be(:epic1) { create(:epic, group: group) }
+    let_it_be(:epic2) { create(:epic, group: group) }
+    let_it_be(:issue1) { create(:issue, project: project1) }
+    let_it_be(:issue2) { create(:issue, project: project2) }
+    let_it_be(:epic_issue1) { create(:epic_issue, epic: epic1, issue: issue1) }
+    let_it_be(:epic_issue2) { create(:epic_issue, epic: epic2, issue: issue2) }
+    let(:filter) { proc {} }
+
+    subject do
+      [
+        described_class.new(epic1.id, filter).find,
+        described_class.new(epic2.id, filter).find
+      ].map(&:sync)
+    end
+
+    it 'only queries once for epic issues' do
+      # 4 queries are done: getting issues and getting projects,
+      # project_features and groups for these issues
+      expect { subject }.not_to exceed_query_limit(4)
+    end
+
+    it 'returns all epic issues' do
+      expect(subject).to eq [[issue1], [issue2]]
+    end
+
+    it 'returns an instance of FilterableArray' do
+      expect(subject.all?(Gitlab::Graphql::FilterableArray)).to be_truthy
+    end
+
+    it 'raises an error if too many issues are loaded' do
+      stub_const('Gitlab::Graphql::Loaders::BatchEpicIssuesLoader::MAX_LOADED_ISSUES', 0)
+
+      expect { subject }.to raise_error Gitlab::Graphql::Errors::ArgumentError, 'Too many epic issues requested.'
+    end
+  end
+end

ee/spec/models/epic_spec.rb

@@ -580,5 +580,21 @@ describe Epic do
     end
   end
 
+  describe '.related_issues' do
+    it 'returns epic issues ordered by relative position' do
+      epic1 = create(:epic, group: group)
+      epic2 = create(:epic, group: group)
+      issue1 = create(:issue, project: project)
+      issue2 = create(:issue, project: project)
+      create(:issue, project: project)
+      create(:epic_issue, epic: epic1, issue: issue1, relative_position: 5)
+      create(:epic_issue, epic: epic2, issue: issue2, relative_position: 2)
+
+      result = described_class.related_issues(ids: [epic1.id, epic2.id])
+
+      expect(result.pluck(:id)).to eq [issue2.id, issue1.id]
+    end
+  end
+
   it_behaves_like 'versioned description'
 end

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

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe 'Getting issues for an epic' do
+  include GraphqlHelpers
+
+  let_it_be(:current_user) { create(:user) }
+  let_it_be(:user) { create(:user) }
+  let_it_be(:group) { create(:group, :public) }
+  let_it_be(:epic) { create(:epic, group: group) }
+  let_it_be(:project) { create(:project, :private, group: group) }
+  let_it_be(:issue) { create(:issue, project: project) }
+  let_it_be(:confidential_issue) { create(:issue, :confidential, project: project) }
+  let_it_be(:epic_issue) { create(:epic_issue, epic: epic, issue: issue, relative_position: 3) }
+  let_it_be(:epic_issue2) { create(:epic_issue, epic: epic, issue: confidential_issue, relative_position: 5) }
+
+  let(:epics_data) { graphql_data['group']['epics']['edges'] }
+
+  let(:epic_node) do
+    <<~NODE
+      edges {
+        node {
+          iid
+          issues {
+            edges {
+              node {
+                id
+              }
+            }
+          }
+        }
+      }
+    NODE
+  end
+
+  def epic_query(params = {})
+    graphql_query_for("group", { "fullPath" => group.full_path },
+                       query_graphql_field("epics", params, epic_node)
+    )
+  end
+
+  def issue_ids
+    node_array(epics_data).each_with_object({}) do |node, result|
+      result[node['iid'].to_i] = node_array(node['issues']['edges'], 'id')
+    end
+  end
+
+  def first_epic_issues_page_info
+    epics_data.first['node']['issues']['pageInfo']
+  end
+
+  context 'when epics are enabled' do
+    before do
+      stub_licensed_features(epics: true)
+    end
+
+    it 'does not return inaccessible issues' do
+      post_graphql(epic_query(iid: epic.iid), current_user: user)
+
+      expect(response).to have_gitlab_http_status(:success)
+      expect(issue_ids[epic.iid]).to be_empty
+    end
+
+    context 'when user has access to the issue project' do
+      before do
+        project.add_developer(user)
+      end
+
+      it 'returns issues in this project' do
+        post_graphql(epic_query(iid: epic.iid), current_user: user)
+
+        expect(response).to have_gitlab_http_status(:success)
+        expect(issue_ids[epic.iid]).to eq [issue.to_global_id.to_s, confidential_issue.to_global_id.to_s]
+      end
+
+      context 'pagination' do
+        let(:after_cursor) { '' }
+        let(:epic_node) do
+          <<~NODE
+            edges {
+              node {
+                iid
+                issues(first: 1, after: "#{after_cursor}") {
+                  pageInfo {
+                    hasNextPage
+                    hasPreviousPage
+                    startCursor
+                    endCursor
+                  },
+                  edges {
+                    node {
+                      id
+                    }
+                  }
+                }
+              }
+            }
+          NODE
+        end
+
+        context 'without a cursor' do
+          it 'return first page of issues' do
+            post_graphql(epic_query(iid: epic.iid), current_user: user)
+
+            expect(response).to have_gitlab_http_status(:success)
+            expect(first_epic_issues_page_info['hasNextPage']).to be_truthy
+            expect(first_epic_issues_page_info['endCursor']).to eq 'MQ'
+            expect(issue_ids[epic.iid]).to eq [issue.to_global_id.to_s]
+          end
+        end
+
+        context 'with an after cursor' do
+          let(:after_cursor) { 'MQ' }
+
+          it 'return first page after the cursor' do
+            post_graphql(epic_query(iid: epic.iid), current_user: user)
+
+            expect(response).to have_gitlab_http_status(:success)
+            expect(first_epic_issues_page_info['hasNextPage']).to be_falsey
+            expect(first_epic_issues_page_info['endCursor']).to eq 'Mg'
+            expect(issue_ids[epic.iid]).to eq [confidential_issue.to_global_id.to_s]
+          end
+        end
+      end
+    end
+
+    context 'when user is guest' do
+      before do
+        project.add_guest(user)
+      end
+
+      it 'filters out confidential issues' do
+        post_graphql(epic_query(iid: epic.iid), current_user: user)
+
+        expect(response).to have_gitlab_http_status(:success)
+        expect(issue_ids[epic.iid]).to eq [issue.to_global_id.to_s]
+      end
+    end
+
+    context 'when issues from multiple epics are queried' do
+      let_it_be(:epic2) { create(:epic, group: group) }
+      let_it_be(:issue2) { create(:issue, project: project) }
+      let_it_be(:epic_issue3) { create(:epic_issue, epic: epic2, issue: issue2, relative_position: 3) }
+      let(:params) { { iids: [epic.iid, epic2.iid] } }
+
+      before do
+        project.add_developer(user)
+      end
+
+      it 'returns issues for each epic' do
+        post_graphql(epic_query(params), current_user: user)
+
+        expect(response).to have_gitlab_http_status(:success)
+        result = issue_ids
+        expect(result[epic.iid]).to eq [issue.to_global_id.to_s, confidential_issue.to_global_id.to_s]
+        expect(result[epic2.iid]).to eq [issue2.to_global_id.to_s]
+      end
+
+      it 'avoids N+1 queries' do
+        control_count = ActiveRecord::QueryRecorder.new do
+          post_graphql(epic_query(iid: epic.iid), current_user: user)
+        end.count
+
+        expect do
+          post_graphql(epic_query(params), current_user: user)
+        end.not_to exceed_query_limit(control_count)
+
+        expect(graphql_errors).to be_nil
+      end
+    end
+  end
+
+  context 'when epics are disabled' do
+    before do
+      stub_licensed_features(epics: false)
+    end
+
+    it 'does not find the epic' do
+      post_graphql(epic_query(iid: epic.iid), current_user: user)
+
+      expect(response).to have_gitlab_http_status(:success)
+      expect(graphql_errors).to be_nil
+      expect(graphql_data['group']['epic']).to be_nil
+    end
+  end
+end

ee/spec/requests/api/graphql/group/epics_spec.rb

@@ -50,7 +50,7 @@ describe 'Epics through GroupQuery' do
       it 'returns epics successfully' do
         expect(response).to have_gitlab_http_status(200)
         expect(graphql_errors).to be_nil
-        expect(node_array('id').first).to eq epic.to_global_id.to_s
+        expect(epic_node_array('id').first).to eq epic.to_global_id.to_s
         expect(graphql_data['group']['epicsEnabled']).to be_truthy
       end
     end
@@ -75,7 +75,7 @@ describe 'Epics through GroupQuery' do
           it 'returns false for adminEpic' do
             post_graphql(query, current_user: user)
 
-            expect(node_array('userPermissions')).to all(include('adminEpic' => false))
+            expect(epic_node_array('userPermissions')).to all(include('adminEpic' => false))
           end
         end
 
@@ -87,7 +87,7 @@ describe 'Epics through GroupQuery' do
           it 'returns true for adminEpic' do
             post_graphql(query, current_user: user)
 
-            expect(node_array('userPermissions')).to all(include('adminEpic' => true))
+            expect(epic_node_array('userPermissions')).to all(include('adminEpic' => true))
           end
         end
       end
@@ -156,12 +156,10 @@ describe 'Epics through GroupQuery' do
   def expect_array_response(items)
     expect(response).to have_gitlab_http_status(:success)
     expect(epics_data).to be_an Array
-    expect(node_array('id')).to eq(Array(items))
+    expect(epic_node_array('id')).to eq(Array(items))
   end
 
-  def node_array(extract_attribute = nil)
-    epics_data.map do |item|
-      extract_attribute ? item['node'][extract_attribute] : item['node']
-    end
+  def epic_node_array(extract_attribute = nil)
+    node_array(epics_data, extract_attribute)
   end
 end

lib/gitlab/graphql/authorize/instrumentation.rb

@@ -9,12 +9,16 @@ module Gitlab
         def instrument(_type, field)
           service = AuthorizeFieldService.new(field)
 
-          if service.authorizations?
+          if service.authorizations? && !resolver_skips_authorizations?(field)
             field.redefine { resolve(service.authorized_resolve) }
           else
             field
           end
         end
+
+        def resolver_skips_authorizations?(field)
+          field.metadata[:resolver].try(:skip_authorizations?)
+        end
       end
     end
   end

lib/gitlab/graphql/connections.rb

@@ -8,6 +8,10 @@ module Gitlab
           ActiveRecord::Relation,
           Gitlab::Graphql::Connections::Keyset::Connection
         )
+        GraphQL::Relay::BaseConnection.register_connection_implementation(
+          Gitlab::Graphql::FilterableArray,
+          Gitlab::Graphql::Connections::FilterableArrayConnection
+        )
       end
     end
   end

lib/gitlab/graphql/connections/filterable_array_connection.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Graphql
+    module Connections
+      # FilterableArrayConnection is useful especially for lazy-loaded values.
+      # It allows us to call a callback only on the slice of array being
+      # rendered in the "after loaded" phase.  For example we can check
+      # permissions only on a small subset of items.
+      class FilterableArrayConnection < GraphQL::Relay::ArrayConnection
+        def paged_nodes
+          @filtered_nodes ||= nodes.filter_callback.call(super)
+        end
+      end
+    end
+  end
+end

lib/gitlab/graphql/filterable_array.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Graphql
+    class FilterableArray < Array
+      attr_reader :filter_callback
+
+      def initialize(filter_callback, *args)
+        super(args)
+        @filter_callback = filter_callback
+      end
+    end
+  end
+end

spec/lib/gitlab/graphql/connections/filterable_array_connection_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::Graphql::Connections::FilterableArrayConnection do
+  let(:callback) { proc { |nodes| nodes } }
+  let(:all_nodes) { Gitlab::Graphql::FilterableArray.new(callback, 1, 2, 3, 4, 5) }
+  let(:arguments) { {} }
+  subject(:connection) do
+    described_class.new(all_nodes, arguments, max_page_size: 3)
+  end
+
+  describe '#paged_nodes' do
+    let(:paged_nodes) { subject.paged_nodes }
+
+    it_behaves_like "connection with paged nodes"
+
+    context 'when callback filters some nodes' do
+      let(:callback) { proc { |nodes| nodes[1..-1] } }
+
+      it 'does not return filtered elements' do
+        expect(subject.paged_nodes).to contain_exactly(all_nodes[1], all_nodes[2])
+      end
+    end
+  end
+end

spec/lib/gitlab/graphql/connections/keyset/connection_spec.rb

@@ -240,38 +240,16 @@ describe Gitlab::Graphql::Connections::Keyset::Connection do
   end
 
   describe '#paged_nodes' do
-    let!(:projects) { create_list(:project, 5) }
+    let_it_be(:all_nodes) { create_list(:project, 5) }
+    let(:paged_nodes) { subject.paged_nodes }
 
-    it 'returns the collection limited to max page size' do
-      expect(subject.paged_nodes.size).to eq(3)
-    end
-
-    it 'is a loaded memoized array' do
-      expect(subject.paged_nodes).to be_an(Array)
-      expect(subject.paged_nodes.object_id).to eq(subject.paged_nodes.object_id)
-    end
-
-    context 'when `first` is passed' do
-      let(:arguments) { { first: 2 } }
-
-      it 'returns only the first elements' do
-        expect(subject.paged_nodes).to contain_exactly(projects.first, projects.second)
-      end
-    end
-
-    context 'when `last` is passed' do
-      let(:arguments) { { last: 2 } }
-
-      it 'returns only the last elements' do
-        expect(subject.paged_nodes).to contain_exactly(projects[3], projects[4])
-      end
-    end
+    it_behaves_like "connection with paged nodes"
 
     context 'when both are passed' do
       let(:arguments) { { first: 2, last: 2 } }
 
       it 'raises an error' do
-        expect { subject.paged_nodes }.to raise_error(Gitlab::Graphql::Errors::ArgumentError)
+        expect { paged_nodes }.to raise_error(Gitlab::Graphql::Errors::ArgumentError)
       end
     end
 

spec/support/helpers/graphql_helpers.rb

@@ -37,9 +37,12 @@ module GraphqlHelpers
   # BatchLoader::GraphQL returns a wrapper, so we need to :sync in order
   # to get the actual values
   def batch_sync(max_queries: nil, &blk)
-    result = batch(max_queries: nil, &blk)
+    wrapper = proc do
+      lazy_vals = yield
+      lazy_vals.is_a?(Array) ? lazy_vals.map(&:sync) : lazy_vals&.sync
+    end
 
-    result.is_a?(Array) ? result.map(&:sync) : result&.sync
+    batch(max_queries: max_queries, &wrapper)
   end
 
   def graphql_query_for(name, attributes = {}, fields = nil)
@@ -157,7 +160,13 @@ module GraphqlHelpers
 
   def attributes_to_graphql(attributes)
     attributes.map do |name, value|
-      "#{GraphqlHelpers.fieldnamerize(name.to_s)}: \"#{value}\""
+      value_str = if value.is_a?(Array)
+                    '["' + value.join('","') + '"]'
+                  else
+                    "\"#{value}\""
+                  end
+
+      "#{GraphqlHelpers.fieldnamerize(name.to_s)}: #{value_str}"
     end.join(", ")
   end
 
@@ -282,6 +291,12 @@ module GraphqlHelpers
   def allow_high_graphql_recursion
     allow_any_instance_of(Gitlab::Graphql::QueryAnalyzers::RecursionAnalyzer).to receive(:recursion_threshold).and_return 1000
   end
+
+  def node_array(data, extract_attribute = nil)
+    data.map do |item|
+      extract_attribute ? item['node'][extract_attribute] : item['node']
+    end
+  end
 end
 
 # This warms our schema, doing this as part of loading the helpers to avoid

spec/support/shared_examples/graphql/connection_paged_nodes.rb

+# frozen_string_literal: true
+
+RSpec.shared_examples 'connection with paged nodes' do
+  it 'returns the collection limited to max page size' do
+    expect(paged_nodes.size).to eq(3)
+  end
+
+  it 'is a loaded memoized array' do
+    expect(paged_nodes).to be_an(Array)
+    expect(paged_nodes.object_id).to eq(paged_nodes.object_id)
+  end
+
+  context 'when `first` is passed' do
+    let(:arguments) { { first: 2 } }
+
+    it 'returns only the first elements' do
+      expect(paged_nodes).to contain_exactly(all_nodes.first, all_nodes.second)
+    end
+  end
+
+  context 'when `last` is passed' do
+    let(:arguments) { { last: 2 } }
+
+    it 'returns only the last elements' do
+      expect(paged_nodes).to contain_exactly(all_nodes[3], all_nodes[4])
+    end
+  end
+end