Add IssuesFieldExtension to set relative positions

IssuesFieldExtension is a field extension for
the field issues of the type BoardList.
The extension is used to check and set
relative positions of returned issues.

Because IssuesFieldExtension needs to pass
a paginated list of issues to
::Boards::Issues::ListService.initialize_relative_positions
for performance, the extension needs to execute after
the underlying issues relation has been wrapped
by the default pagination connection extension.
We achieve this by utilizing 'late_extensions'
on initializing the field.

Previously we incorrectly called initialize_relative_position
by passing an underlying AR relation rather than
a paginated result when resolving the field in BoardListIssuesResolver.
This produced DB queries without `LIMIT` clause -
causing performance degradations.

Changelog: fixed

app/graphql/resolvers/board_list_issues_resolver.rb

@@ -18,11 +18,8 @@ module Resolvers
 
       filter_params = filters.merge(board_id: list.board.id, id: list.id)
       service = ::Boards::Issues::ListService.new(list.board.resource_parent, context[:current_user], filter_params)
-      pagination_connections = Gitlab::Graphql::Pagination::Keyset::Connection.new(service.execute)
 
-      ::Boards::Issues::ListService.initialize_relative_positions(list.board, current_user, pagination_connections.items)
-
-      pagination_connections
+      service.execute
     end
 
     # https://gitlab.com/gitlab-org/gitlab/-/issues/235681

app/graphql/types/base_field.rb

@@ -21,6 +21,7 @@ module Types
       @feature_flag = kwargs[:feature_flag]
       kwargs = check_feature_flag(kwargs)
       @deprecation = gitlab_deprecation(kwargs)
+      after_connection_extensions = kwargs.delete(:late_extensions) || []
 
       super(**kwargs, &block)
 
@@ -28,6 +29,8 @@ module Types
       extension ::Gitlab::Graphql::CallsGitaly::FieldExtension if Gitlab.dev_or_test_env?
       extension ::Gitlab::Graphql::Present::FieldExtension
       extension ::Gitlab::Graphql::Authorize::ConnectionFilterExtension
+
+      after_connection_extensions.each { extension _1 } if after_connection_extensions.any?
     end
 
     def may_call_gitaly?

app/graphql/types/board_list_type.rb

@@ -27,6 +27,7 @@ module Types
 
     field :issues, ::Types::IssueType.connection_type, null: true,
           description: 'Board issues.',
+          late_extensions: [Gitlab::Graphql::Board::IssuesConnectionExtension],
           resolver: ::Resolvers::BoardListIssuesResolver
 
     def issues_count

ee/spec/graphql/ee/resolvers/board_list_issues_resolver_spec.rb

@@ -134,6 +134,6 @@ RSpec.describe Resolvers::BoardListIssuesResolver do
   end
 
   def resolve_board_list_issues(args)
-    resolve(described_class, obj: list, args: args, ctx: { current_user: user }).items
+    resolve(described_class, obj: list, args: args, ctx: { current_user: user })
   end
 end

lib/gitlab/graphql/board/issues_connection_extension.rb

+# frozen_string_literal: true
+module Gitlab
+  module Graphql
+    module Board
+      class IssuesConnectionExtension < GraphQL::Schema::Field::ConnectionExtension
+        def after_resolve(value:, object:, context:, **rest)
+          ::Boards::Issues::ListService
+            .initialize_relative_positions(object.list.board, context[:current_user], value.nodes)
+
+          value
+        end
+      end
+    end
+  end
+end

lib/gitlab/graphql/connection_collection_methods.rb

@@ -6,7 +6,7 @@ module Gitlab
       extend ActiveSupport::Concern
 
       included do
-        delegate :to_a, :size, :include?, :empty?, to: :nodes
+        delegate :to_a, :size, :map, :include?, :empty?, to: :nodes
       end
     end
   end

spec/graphql/resolvers/board_list_issues_resolver_spec.rb

@@ -31,12 +31,11 @@ RSpec.describe Resolvers::BoardListIssuesResolver do
         end.to raise_error(Gitlab::Graphql::Errors::ArgumentError)
       end
 
-      it 'returns issues in the correct order with non-nil relative positions', :aggregate_failures do
+      it 'returns the issues in the correct order' do
         # by relative_position and then ID
         result = resolve_board_list_issues
 
         expect(result.map(&:id)).to eq [issue1.id, issue3.id, issue2.id, issue4.id]
-        expect(result.map(&:relative_position)).not_to include(nil)
       end
 
       it 'finds only issues matching filters' do
@@ -119,6 +118,6 @@ RSpec.describe Resolvers::BoardListIssuesResolver do
   end
 
   def resolve_board_list_issues(args: {}, current_user: user)
-    resolve(described_class, obj: list, args: args, ctx: { current_user: current_user }).items
+    resolve(described_class, obj: list, args: args, ctx: { current_user: current_user })
   end
 end

spec/graphql/types/base_field_spec.rb

@@ -154,6 +154,17 @@ RSpec.describe Types::BaseField do
     end
   end
 
+  describe '#resolve' do
+    context "late_extensions is given" do
+      it 'registers the late extensions after the regular extensions' do
+        extension_class = Class.new(GraphQL::Schema::Field::ConnectionExtension)
+        field = described_class.new(name: 'test', type: GraphQL::Types::String.connection_type, null: true, late_extensions: [extension_class])
+
+        expect(field.extensions.last.class).to be(extension_class)
+      end
+    end
+  end
+
   describe '#description' do
     context 'feature flag given' do
       let(:field) { described_class.new(name: 'test', type: GraphQL::Types::String, feature_flag: flag, null: false, description: 'Test description.') }

spec/graphql/types/board_list_type_spec.rb

@@ -10,4 +10,12 @@ RSpec.describe GitlabSchema.types['BoardList'] do
 
     expect(described_class).to include_graphql_fields(*expected_fields)
   end
+
+  describe 'issues field' do
+    subject { described_class.fields['issues'] }
+
+    it 'has a correct extension' do
+      is_expected.to have_graphql_extension(Gitlab::Graphql::Board::IssuesConnectionExtension)
+    end
+  end
 end

spec/requests/api/graphql/boards/board_list_issues_query_spec.rb

@@ -30,7 +30,7 @@ RSpec.describe 'get board lists' do
           nodes {
             lists {
               nodes {
-                issues(filters: {labelName: "#{label2.title}"}) {
+                issues(filters: {labelName: "#{label2.title}"}, first: 3) {
                   count
                   nodes {
                     #{all_graphql_fields_for('issues'.classify)}
@@ -44,6 +44,10 @@ RSpec.describe 'get board lists' do
     )
   end
 
+  def issue_id
+    issues_data.map { |i| i['id'] }
+  end
+
   def issue_titles
     issues_data.map { |i| i['title'] }
   end
@@ -60,6 +64,7 @@ RSpec.describe 'get board lists' do
     let!(:issue3) { create(:issue, project: issue_project, labels: [label, label2], relative_position: nil) }
     let!(:issue4) { create(:issue, project: issue_project, labels: [label], relative_position: 9) }
     let!(:issue5) { create(:issue, project: issue_project, labels: [label2], relative_position: 432) }
+    let!(:issue6) { create(:issue, project: issue_project, labels: [label, label2], relative_position: nil) }
 
     context 'when the user does not have access to the board' do
       it 'returns nil' do
@@ -72,14 +77,19 @@ RSpec.describe 'get board lists' do
     context 'when user can read the board' do
       before do
         board_parent.add_reporter(user)
+        post_graphql(query("id: \"#{global_id_of(label_list)}\""), current_user: user)
       end
 
       it 'can access the issues', :aggregate_failures do
-        post_graphql(query("id: \"#{global_id_of(label_list)}\""), current_user: user)
-
+        # ties for relative positions are broken by id in ascending order by default
         expect(issue_titles).to eq([issue2.title, issue1.title, issue3.title])
         expect(issue_relative_positions).not_to include(nil)
       end
+
+      it 'does not set the relative positions of the issues not being returned', :aggregate_failures do
+        expect(issue_id).not_to include(issue6.id)
+        expect(issue3.relative_position).to be_nil
+      end
     end
   end
 

spec/support/shared_examples/graphql/connection_shared_examples.rb

 # frozen_string_literal: true
 
 RSpec.shared_examples 'a connection with collection methods' do
-  %i[to_a size include? empty?].each do |method_name|
+  %i[to_a size map include? empty?].each do |method_name|
     it "responds to #{method_name}" do
       expect(connection).to respond_to(method_name)
     end