Display epic issues with null relative position

This removes batch loading for epic issues because its ordering
does not include objects with nil position.

For more information check:
 https://gitlab.com/gitlab-org/gitlab/-/issues/207898.

ee/app/graphql/resolvers/epic_issues_resolver.rb

@@ -6,20 +6,8 @@ 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 executing any authorization checks in earlier
-    # phase
-    def self.skip_authorizations?
-      true
-    end
-
     def resolve(**args)
-      filter = proc do |issues|
-        Ability.issues_readable_by_user(issues, context[:current_user])
-      end
-
-      Gitlab::Graphql::Loaders::BatchEpicIssuesLoader.new(epic.id, filter).find
+      epic.issues_readable_by(context[:current_user], preload: { project: [:namespace, :project_feature] })
     end
   end
 end

ee/app/graphql/types/epic_type.rb

@@ -121,7 +121,7 @@ module Types
     field :issues,
           Types::EpicIssueType.connection_type,
           null: true,
-          complexity: 2,
+          complexity: 5,
           description: 'A list of issues associated with the epic',
           resolver: Resolvers::EpicIssuesResolver
 

ee/changelogs/unreleased/issue_207898.yml

+---
+title: Display epic issues with null relative position
+merge_request: 33105
+author:
+type: fixed

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
-        BATCH_SIZE = 1_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|
-            load_issues(loader, ids)
-          end
-        end
-
-        private
-
-        def load_issues(loader, ids)
-          issues = ::EpicIssue.related_issues_for_batches(ids)
-          issues.each_batch(of: BATCH_SIZE, column: 'relative_position') do |batch, idx|
-            process_batch(loader, batch, idx)
-          end
-        end
-
-        def process_batch(loader, batch, idx)
-          Epic.related_issues(preload: { project: [:namespace, :project_feature] })
-            .merge(batch.except(:select)).each do |issue|
-            ensure_limit_not_exceeded!(idx)
-
-            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
-
-        def ensure_limit_not_exceeded!(current_index)
-          if current_index * BATCH_SIZE > MAX_LOADED_ISSUES
-            raise Gitlab::Graphql::Errors::ArgumentError, 'Too many epic issues requested.'
-          end
-        end
-      end
-    end
-  end
-end

ee/spec/graphql/resolvers/epic_issues_resolver_spec.rb

@@ -7,17 +7,19 @@ RSpec.describe Resolvers::EpicIssuesResolver do
 
   let_it_be(:current_user) { create(:user) }
   let_it_be(:user) { create(:user) }
-  let_it_be(:group) { create(:group) }
+  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) }
   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(: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(: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) }
 
   before do
     group.add_developer(current_user)
@@ -26,17 +28,21 @@ RSpec.describe Resolvers::EpicIssuesResolver do
 
   describe '#resolve' do
     it 'finds all epic issues' do
-      result = batch_sync(max_queries: 6) { resolve_epic_issues(epic1) }
+      result = [resolve_epic_issues(epic1), resolve_epic_issues(epic2)]
 
-      expect(result).to contain_exactly(issue1, issue2)
+      expect(result).to contain_exactly([issue2, issue1], [issue3, issue4])
     end
 
-    it 'can batch-resolve epic issues from different epics' do
-      result = batch_sync(max_queries: 6) do
-        [resolve_epic_issues(epic1), resolve_epic_issues(epic2)]
-      end
+    it 'finds only epic issues that user can read' do
+      guest = create(:user)
 
-      expect(result).to contain_exactly([issue2, issue1], [issue3])
+      result =
+        [
+          resolve_epic_issues(epic1, {}, { current_user: guest }),
+          resolve_epic_issues(epic2, {}, { current_user: guest })
+        ]
+
+      expect(result).to contain_exactly([], [issue1])
     end
   end
 

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

-# frozen_string_literal: true
-
-require 'spec_helper'
-
-RSpec.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(:issue3) { create(:issue, project: project2) }
-    let_it_be(:epic_issue1) { create(:epic_issue, epic: epic1, issue: issue1, relative_position: 1000) }
-    let_it_be(:epic_issue2) { create(:epic_issue, epic: epic2, issue: issue2, relative_position: 99999) }
-    let_it_be(:epic_issue3) { create(:epic_issue, epic: epic2, issue: issue3, relative_position: 1) }
-    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
-      # 6 queries are done: 2 queries in EachBatch and then getting issues
-      # and getting projects, project_features and groups for these issues
-      expect { subject }.not_to exceed_query_limit(6)
-    end
-
-    it 'returns all epic issues ordered by relative position' do
-      expect(subject).to eq [[issue1], [issue3, 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', 2)
-
-      expect { subject }.to raise_error Gitlab::Graphql::Errors::ArgumentError, 'Too many epic issues requested.'
-    end
-  end
-end

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

@@ -157,7 +157,10 @@ RSpec.describe 'Getting issues for an epic' do
         expect(result[epic2.iid]).to eq [issue2.to_global_id.to_s]
       end
 
-      it 'avoids N+1 queries' do
+      # TODO remove the pending state of this spec when
+      # we have an efficient way of preloading data on GraphQL.
+      # For more information check: https://gitlab.com/gitlab-org/gitlab/-/issues/207898
+      xit 'avoids N+1 queries' do
         control_count = ActiveRecord::QueryRecorder.new do
           post_graphql(epic_query(iid: epic.iid), current_user: user)
         end.count

lib/gitlab/graphql/authorize/instrumentation.rb

@@ -9,16 +9,12 @@ module Gitlab
         def instrument(_type, field)
           service = AuthorizeFieldService.new(field)
 
-          if service.authorizations? && !resolver_skips_authorizations?(field)
+          if service.authorizations?
             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/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

lib/gitlab/graphql/pagination/connections.rb

@@ -9,10 +9,6 @@ module Gitlab
             ActiveRecord::Relation,
             Gitlab::Graphql::Pagination::Keyset::Connection)
 
-          schema.connections.add(
-            Gitlab::Graphql::FilterableArray,
-            Gitlab::Graphql::Pagination::FilterableArrayConnection)
-
           schema.connections.add(
             Gitlab::Graphql::ExternallyPaginatedArray,
             Gitlab::Graphql::Pagination::ExternallyPaginatedArrayConnection)

lib/gitlab/graphql/pagination/filterable_array_connection.rb

-# frozen_string_literal: true
-
-module Gitlab
-  module Graphql
-    module Pagination
-      # 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::Pagination::ArrayConnection
-        def nodes
-          @nodes ||= items.filter_callback.call(super)
-        end
-      end
-    end
-  end
-end

spec/graphql/gitlab_schema_spec.rb

@@ -46,12 +46,6 @@ describe GitlabSchema do
     expect(connection).to eq(Gitlab::Graphql::Pagination::ExternallyPaginatedArrayConnection)
   end
 
-  it 'paginates FilterableArray using `Pagination::FilterableArrayConnection`' do
-    connection = connections[Gitlab::Graphql::FilterableArray]
-
-    expect(connection).to eq(Gitlab::Graphql::Pagination::FilterableArrayConnection)
-  end
-
   describe '.execute' do
     context 'for different types of users' do
       context 'when no context' do

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

-# frozen_string_literal: true
-
-require 'spec_helper'
-
-describe Gitlab::Graphql::Pagination::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, { max_page_size: 3 }.merge(arguments))
-  end
-
-  describe '#nodes' do
-    let(:paged_nodes) { subject.nodes }
-
-    it_behaves_like 'connection with paged nodes' do
-      let(:paged_nodes_size) { 3 }
-    end
-
-    context 'when callback filters some nodes' do
-      let(:callback) { proc { |nodes| nodes[1..-1] } }
-
-      it 'does not return filtered elements' do
-        expect(subject.nodes).to contain_exactly(all_nodes[1], all_nodes[2])
-      end
-    end
-  end
-end