Merge branch 'ajk-gql-pagination-offset-relation' into 'master'

Allow `Resolver.single` to work with offset pagination [RUN AS-IF-FOSS]

See merge request gitlab-org/gitlab!50368

app/graphql/resolvers/base_resolver.rb

@@ -118,7 +118,7 @@ module Resolvers
     end
 
     def offset_pagination(relation)
-      ::Gitlab::Graphql::Pagination::OffsetActiveRecordRelationConnection.new(relation)
+      ::Gitlab::Graphql::Pagination::OffsetPaginatedRelation.new(relation)
     end
 
     override :object

ee/app/graphql/mutations/iterations/update.rb

@@ -68,7 +68,7 @@ module Mutations
 
       def find_object(parent:, id:)
         ::Resolvers::IterationsResolver.new(object: parent, context: context, field: nil)
-          .resolve(id: id).items.first
+          .resolve(id: id).first
       end
 
       def validate_arguments!(args)

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

@@ -33,13 +33,13 @@ RSpec.describe Resolvers::BoardListIssuesResolver do
       end
 
       it 'accepts epic global id' do
-        result = resolve_board_list_issues({ filters: { epic_id: epic.to_global_id } }).items
+        result = resolve_board_list_issues({ filters: { epic_id: epic.to_global_id } })
 
         expect(result).to match_array([issue])
       end
 
       it 'accepts epic wildcard id' do
-        result = resolve_board_list_issues({ filters: { epic_wildcard_id: 'NONE' } }).items
+        result = resolve_board_list_issues({ filters: { epic_wildcard_id: 'NONE' } })
 
         expect(result).to match_array([])
       end
@@ -51,13 +51,13 @@ RSpec.describe Resolvers::BoardListIssuesResolver do
       let_it_be(:issue_without_iteration) { create(:issue, project: project, labels: [label]) }
 
       it 'accepts iteration title' do
-        result = resolve_board_list_issues({ filters: { iteration_title: iteration.title } }).items
+        result = resolve_board_list_issues({ filters: { iteration_title: iteration.title } })
 
         expect(result).to contain_exactly(issue_with_iteration)
       end
 
       it 'accepts iteration wildcard id' do
-        result = resolve_board_list_issues({ filters: { iteration_wildcard_id: 'NONE' } }).items
+        result = resolve_board_list_issues({ filters: { iteration_wildcard_id: 'NONE' } })
 
         expect(result).to contain_exactly(issue_without_iteration)
       end

ee/spec/graphql/ee/resolvers/board_lists_resolver_spec.rb

@@ -25,7 +25,7 @@ RSpec.describe Resolvers::BoardListsResolver do
     end
 
     it 'returns a list of board lists' do
-      lists = resolve_board_lists.items
+      lists = resolve_board_lists
 
       expect(lists.count).to eq 3
       expect(lists.map(&:list_type)).to eq %w(closed assignee milestone)

ee/spec/graphql/resolvers/boards/board_list_epics_resolver_spec.rb

@@ -36,7 +36,7 @@ RSpec.describe Resolvers::Boards::BoardListEpicsResolver do
     end
 
     it 'returns epics on the board list ordered by position on the board' do
-      expect(result.items).to eq([list1_epic2, list1_epic1])
+      expect(result.to_a).to eq([list1_epic2, list1_epic1])
     end
   end
 end

ee/spec/graphql/resolvers/boards/epic_boards_resolvers_spec.rb

@@ -42,7 +42,9 @@ RSpec.describe Resolvers::Boards::EpicBoardsResolver do
         end
 
         it 'returns epic boards in the group ordered by name' do
-          expect(result).to eq([epic_board2, epic_board1])
+          expect(result)
+            .to contain_exactly(epic_board2, epic_board1)
+            .and be_sorted(:name, :asc)
         end
 
         context 'when epic_boards flag is disabled' do

ee/spec/graphql/resolvers/boards/epic_lists_resolvers_spec.rb

@@ -17,8 +17,9 @@ RSpec.describe Resolvers::Boards::EpicListsResolver do
 
   describe '#resolve' do
     let(:args) { {} }
+    let(:resolver) { described_class }
 
-    subject(:result) { resolve(described_class, ctx: { current_user: user }, obj: epic_board, args: args) }
+    subject(:result) { resolve(resolver, ctx: { current_user: user }, obj: epic_board, args: args) }
 
     before do
       stub_licensed_features(epics: true)
@@ -34,14 +35,15 @@ RSpec.describe Resolvers::Boards::EpicListsResolver do
       end
 
       it 'returns epic lists for the board' do
-        expect(result.items).to match_array([epic_list1, epic_list2])
+        expect(result).to match_array([epic_list1, epic_list2])
       end
 
-      context 'when list ID param is set' do
+      context 'when resolving a single item' do
         let(:args) { { id: epic_list1.to_global_id } }
+        let(:resolver) { described_class.single }
 
         it 'returns an array with single epic list' do
-          expect(result.items).to match_array([epic_list1])
+          expect(result).to eq epic_list1
         end
       end
     end

ee/spec/graphql/types/project_type_spec.rb

@@ -216,7 +216,7 @@ RSpec.describe GitlabSchema.types['Project'] do
           resolve_field(:compliance_frameworks, p)
         end
       end
-      frameworks = results.flat_map(&:items)
+      frameworks = results.flat_map(&:to_a)
 
       expect(frameworks).to match_array(projects.flat_map(&:compliance_management_frameworks))
     end

lib/gitlab/graphql/pagination/connections.rb

@@ -5,6 +5,10 @@ module Gitlab
     module Pagination
       module Connections
         def self.use(schema)
+          schema.connections.add(
+            ::Gitlab::Graphql::Pagination::OffsetPaginatedRelation,
+            ::Gitlab::Graphql::Pagination::OffsetActiveRecordRelationConnection)
+
           schema.connections.add(
             ActiveRecord::Relation,
             Gitlab::Graphql::Pagination::Keyset::Connection)

lib/gitlab/graphql/pagination/offset_paginated_relation.rb

+# frozen_string_literal: true
+
+# Marker class to enable us to choose the correct
+# connection type during resolution
+module Gitlab
+  module Graphql
+    module Pagination
+      class OffsetPaginatedRelation < SimpleDelegator
+      end
+    end
+  end
+end

spec/graphql/resolvers/base_resolver_spec.rb

@@ -277,8 +277,8 @@ RSpec.describe Resolvers::BaseResolver do
   describe '#offset_pagination' do
     let(:instance) { resolver_instance(resolver) }
 
-    it 'is sugar for OffsetActiveRecordRelationConnection.new' do
-      expect(instance.offset_pagination(User.none)).to be_a(::Gitlab::Graphql::Pagination::OffsetActiveRecordRelationConnection)
+    it 'is sugar for OffsetPaginatedRelation.new' do
+      expect(instance.offset_pagination(User.none)).to be_a(::Gitlab::Graphql::Pagination::OffsetPaginatedRelation)
     end
   end
 end

spec/graphql/resolvers/board_list_issues_resolver_spec.rb

@@ -23,19 +23,19 @@ RSpec.describe Resolvers::BoardListIssuesResolver do
 
       it 'returns the issues in the correct order' do
         # by relative_position and then ID
-        issues = resolve_board_list_issues.items
+        issues = resolve_board_list_issues
 
         expect(issues.map(&:id)).to eq [issue3.id, issue1.id, issue2.id]
       end
 
       it 'finds only issues matching filters' do
-        result = resolve_board_list_issues(args: { filters: { label_name: [label.title], not: { label_name: [label2.title] } } }).items
+        result = resolve_board_list_issues(args: { filters: { label_name: [label.title], not: { label_name: [label2.title] } } })
 
         expect(result).to match_array([issue1, issue3])
       end
 
       it 'finds only issues matching search param' do
-        result = resolve_board_list_issues(args: { filters: { search: issue1.title } }).items
+        result = resolve_board_list_issues(args: { filters: { search: issue1.title } })
 
         expect(result).to match_array([issue1])
       end

spec/graphql/resolvers/board_lists_resolver_spec.rb

@@ -21,7 +21,7 @@ RSpec.describe Resolvers::BoardListsResolver do
     end
 
     it 'does not create the backlog list' do
-      lists = resolve_board_lists.items
+      lists = resolve_board_lists
 
       expect(lists.count).to eq 1
       expect(lists[0].list_type).to eq 'closed'
@@ -38,7 +38,7 @@ RSpec.describe Resolvers::BoardListsResolver do
       let!(:backlog_list) { create(:backlog_list, board: board) }
 
       it 'returns a list of board lists' do
-        lists = resolve_board_lists.items
+        lists = resolve_board_lists
 
         expect(lists.count).to eq 3
         expect(lists.map(&:list_type)).to eq %w(backlog label closed)
@@ -50,7 +50,7 @@ RSpec.describe Resolvers::BoardListsResolver do
         end
 
         it 'returns the complete list of board lists for this user' do
-          lists = resolve_board_lists.items
+          lists = resolve_board_lists
 
           expect(lists.count).to eq 3
         end
@@ -58,7 +58,7 @@ RSpec.describe Resolvers::BoardListsResolver do
 
       context 'when querying for a single list' do
         it 'returns specified list' do
-          list = resolve_board_lists(args: { id: global_id_of(label_list) }).items
+          list = resolve_board_lists(args: { id: global_id_of(label_list) })
 
           expect(list).to eq [label_list]
         end
@@ -69,13 +69,13 @@ RSpec.describe Resolvers::BoardListsResolver do
           external_label = create(:group_label, group: group)
           external_list = create(:list, board: external_board, label: external_label)
 
-          list = resolve_board_lists(args: { id: global_id_of(external_list) }).items
+          list = resolve_board_lists(args: { id: global_id_of(external_list) })
 
           expect(list).to eq List.none
         end
 
         it 'raises an argument error if list ID is not valid' do
-          expect { resolve_board_lists(args: { id: 'test' }).items }
+          expect { resolve_board_lists(args: { id: 'test' }) }
             .to raise_error(Gitlab::Graphql::Errors::ArgumentError)
         end
       end

spec/graphql/resolvers/issues_resolver_spec.rb

@@ -195,11 +195,11 @@ RSpec.describe Resolvers::IssuesResolver do
           let_it_be(:priority_issue4) { create(:issue, project: project) }
 
           it 'sorts issues ascending' do
-            expect(resolve_issues(sort: :priority_asc).items).to eq([priority_issue3, priority_issue1, priority_issue2, priority_issue4])
+            expect(resolve_issues(sort: :priority_asc).to_a).to eq([priority_issue3, priority_issue1, priority_issue2, priority_issue4])
           end
 
           it 'sorts issues descending' do
-            expect(resolve_issues(sort: :priority_desc).items).to eq([priority_issue1, priority_issue3, priority_issue2, priority_issue4])
+            expect(resolve_issues(sort: :priority_desc).to_a).to eq([priority_issue1, priority_issue3, priority_issue2, priority_issue4])
           end
         end
 
@@ -214,11 +214,11 @@ RSpec.describe Resolvers::IssuesResolver do
           let_it_be(:label_issue4) { create(:issue, project: project) }
 
           it 'sorts issues ascending' do
-            expect(resolve_issues(sort: :label_priority_asc).items).to eq([label_issue3, label_issue1, label_issue2, label_issue4])
+            expect(resolve_issues(sort: :label_priority_asc).to_a).to eq([label_issue3, label_issue1, label_issue2, label_issue4])
           end
 
           it 'sorts issues descending' do
-            expect(resolve_issues(sort: :label_priority_desc).items).to eq([label_issue2, label_issue3, label_issue1, label_issue4])
+            expect(resolve_issues(sort: :label_priority_desc).to_a).to eq([label_issue2, label_issue3, label_issue1, label_issue4])
           end
         end
 
@@ -231,11 +231,11 @@ RSpec.describe Resolvers::IssuesResolver do
           let_it_be(:milestone_issue3) { create(:issue, project: project, milestone: late_milestone) }
 
           it 'sorts issues ascending' do
-            expect(resolve_issues(sort: :milestone_due_asc).items).to eq([milestone_issue2, milestone_issue3, milestone_issue1])
+            expect(resolve_issues(sort: :milestone_due_asc).to_a).to eq([milestone_issue2, milestone_issue3, milestone_issue1])
           end
 
           it 'sorts issues descending' do
-            expect(resolve_issues(sort: :milestone_due_desc).items).to eq([milestone_issue3, milestone_issue2, milestone_issue1])
+            expect(resolve_issues(sort: :milestone_due_desc).to_a).to eq([milestone_issue3, milestone_issue2, milestone_issue1])
           end
         end
 

spec/graphql/resolvers/merge_requests_resolver_spec.rb

@@ -4,6 +4,7 @@ require 'spec_helper'
 
 RSpec.describe Resolvers::MergeRequestsResolver do
   include GraphqlHelpers
+  include SortingHelper
 
   let_it_be(:project) { create(:project, :repository) }
   let_it_be(:milestone) { create(:milestone, project: project) }
@@ -30,6 +31,16 @@ RSpec.describe Resolvers::MergeRequestsResolver do
   end
 
   describe '#resolve' do
+    # One for the initial auth, then MRs, and the load of project and project_feature (for further auth):
+    # SELECT MAX("project_authorizations"."access_level") AS maximum_access_level,
+    #   "project_authorizations"."user_id" AS project_authorizations_user_id
+    #   FROM "project_authorizations"
+    #   WHERE "project_authorizations"."project_id" = 2 AND "project_authorizations"."user_id" = 2
+    #   GROUP BY "project_authorizations"."user_id"
+    # SELECT "merge_requests".* FROM "merge_requests" WHERE "merge_requests"."target_project_id" = 2
+    #   AND "merge_requests"."iid" = 1 ORDER BY "merge_requests"."id" DESC
+    # SELECT "projects".* FROM "projects" WHERE "projects"."id" = 2
+    # SELECT "project_features".* FROM "project_features" WHERE "project_features"."project_id" = 2
     let(:queries_per_project) { 3 }
 
     context 'no arguments' do
@@ -72,15 +83,17 @@ RSpec.describe Resolvers::MergeRequestsResolver do
         expect(result).to contain_exactly(merge_request_1, merge_request_2, merge_request_3)
       end
 
-      it 'can batch-resolve merge requests from different projects' do
+      it 'can batch-resolve merge requests from different projects', :request_store, :use_clean_rails_memory_store_caching do
         # 2 queries for project_authorizations, and 2 for merge_requests
-        result = batch_sync(max_queries: queries_per_project * 2) do
-          resolve_mr(project, iids: [iid_1]) +
-            resolve_mr(project, iids: [iid_2]) +
-            resolve_mr(other_project, iids: [other_iid])
+        results = batch_sync(max_queries: queries_per_project * 2) do
+          a = resolve_mr(project, iids: [iid_1])
+          b = resolve_mr(project, iids: [iid_2])
+          c = resolve_mr(other_project, iids: [other_iid])
+
+          [a, b, c].flat_map(&:to_a)
         end
 
-        expect(result).to contain_exactly(merge_request_1, merge_request_2, other_merge_request)
+        expect(results).to contain_exactly(merge_request_1, merge_request_2, other_merge_request)
       end
 
       it 'resolves an unknown iid to be empty' do
@@ -134,9 +147,9 @@ RSpec.describe Resolvers::MergeRequestsResolver do
       it 'takes more than one argument' do
         mrs = [merge_request_3, merge_request_4]
         branches = mrs.map(&:target_branch)
-        result = resolve_mr(project, target_branches: branches )
+        result = resolve_mr(project, target_branches: branches)
 
-        expect(result.compact).to match_array(mrs)
+        expect(result).to match_array(mrs)
       end
     end
 
@@ -173,7 +186,7 @@ RSpec.describe Resolvers::MergeRequestsResolver do
       it 'returns merge requests merged between the given period' do
         result = resolve_mr(project, merged_after: 20.days.ago, merged_before: 5.days.ago)
 
-        expect(result).to eq([merge_request_1])
+        expect(result).to contain_exactly(merge_request_1)
       end
 
       it 'does not return anything' do
@@ -187,7 +200,7 @@ RSpec.describe Resolvers::MergeRequestsResolver do
       it 'filters merge requests by milestone title' do
         result = resolve_mr(project, milestone_title: milestone.title)
 
-        expect(result).to eq([merge_request_with_milestone])
+        expect(result).to contain_exactly(merge_request_with_milestone)
       end
 
       it 'does not find anything' do
@@ -203,18 +216,29 @@ RSpec.describe Resolvers::MergeRequestsResolver do
 
         result = resolve_mr(project, source_branches: [merge_request_4.source_branch], state: 'locked')
 
-        expect(result.compact).to contain_exactly(merge_request_4)
+        expect(result).to contain_exactly(merge_request_4)
       end
     end
 
     describe 'sorting' do
+      let(:mrs) do
+        [
+          merge_request_with_milestone, merge_request_6, merge_request_5, merge_request_4,
+          merge_request_3, merge_request_2, merge_request_1
+        ]
+      end
+
       context 'when sorting by created' do
         it 'sorts merge requests ascending' do
-          expect(resolve_mr(project, sort: 'created_asc')).to eq [merge_request_1, merge_request_2, merge_request_3, merge_request_4, merge_request_5, merge_request_6, merge_request_with_milestone]
+          expect(resolve_mr(project, sort: 'created_asc'))
+            .to match_array(mrs)
+            .and be_sorted(:created_at, :asc)
         end
 
         it 'sorts merge requests descending' do
-          expect(resolve_mr(project, sort: 'created_desc')).to eq [merge_request_with_milestone, merge_request_6, merge_request_5, merge_request_4, merge_request_3, merge_request_2, merge_request_1]
+          expect(resolve_mr(project, sort: 'created_desc'))
+            .to match_array(mrs)
+            .and be_sorted(:created_at, :desc)
         end
       end
 
@@ -225,11 +249,19 @@ RSpec.describe Resolvers::MergeRequestsResolver do
         end
 
         it 'sorts merge requests ascending' do
-          expect(resolve_mr(project, sort: :merged_at_asc)).to eq [merge_request_1, merge_request_3, merge_request_with_milestone, merge_request_6, merge_request_5, merge_request_4, merge_request_2]
+          expect(resolve_mr(project, sort: :merged_at_asc))
+            .to match_array(mrs)
+            .and be_sorted(->(mr) { [merged_at(mr), -mr.id] })
         end
 
         it 'sorts merge requests descending' do
-          expect(resolve_mr(project, sort: :merged_at_desc)).to eq [merge_request_3, merge_request_1, merge_request_with_milestone, merge_request_6, merge_request_5, merge_request_4, merge_request_2]
+          expect(resolve_mr(project, sort: :merged_at_desc))
+            .to match_array(mrs)
+            .and be_sorted(->(mr) { [-merged_at(mr), -mr.id] })
+        end
+
+        def merged_at(mr)
+          nils_last(mr.metrics.merged_at)
         end
 
         context 'when label filter is given and the optimized_issuable_label_filter feature flag is off' do

spec/graphql/resolvers/release_milestones_resolver_spec.rb

@@ -12,12 +12,12 @@ RSpec.describe Resolvers::ReleaseMilestonesResolver do
   end
 
   describe '#resolve' do
-    it "returns an OffsetActiveRecordRelationConnection" do
-      expect(resolved).to be_a(::Gitlab::Graphql::Pagination::OffsetActiveRecordRelationConnection)
+    it "uses offset-pagination" do
+      expect(resolved).to be_a(::Gitlab::Graphql::Pagination::OffsetPaginatedRelation)
     end
 
     it "includes the release's milestones in the returned OffsetActiveRecordRelationConnection" do
-      expect(resolved.items).to eq(release.milestones.order_by_dates_and_title)
+      expect(resolved.to_a).to eq(release.milestones.order_by_dates_and_title)
     end
   end
 end

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

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+# Tests that our connections are correctly mapped.
+RSpec.describe ::Gitlab::Graphql::Pagination::Connections do
+  include GraphqlHelpers
+
+  before(:all) do
+    ActiveRecord::Schema.define do
+      create_table :testing_pagination_nodes, force: true do |t|
+        t.integer :value, null: false
+      end
+    end
+  end
+
+  after(:all) do
+    ActiveRecord::Schema.define do
+      drop_table :testing_pagination_nodes, force: true
+    end
+  end
+
+  let_it_be(:node_model) do
+    Class.new(ActiveRecord::Base) do
+      self.table_name = 'testing_pagination_nodes'
+    end
+  end
+
+  let(:query_string) { 'query { items(first: 2) { nodes { value } } }' }
+  let(:user) { nil }
+
+  let(:node) { Struct.new(:value) }
+  let(:node_type) do
+    Class.new(::GraphQL::Schema::Object) do
+      graphql_name 'Node'
+      field :value, GraphQL::INT_TYPE, null: false
+    end
+  end
+
+  let(:query_type) do
+    item_values = nodes
+
+    query_factory do |t|
+      t.field :items, node_type.connection_type, null: true
+
+      t.define_method :items do
+        item_values
+      end
+    end
+  end
+
+  shared_examples 'it maps to a specific connection class' do |connection_type|
+    let(:raw_values) { [1, 7, 42] }
+
+    it "maps to #{connection_type.name}" do
+      expect(connection_type).to receive(:new).and_call_original
+
+      results = execute_query(query_type).to_h
+
+      expect(graphql_dig_at(results, :data, :items, :nodes, :value)).to eq [1, 7]
+    end
+  end
+
+  describe 'OffsetPaginatedRelation' do
+    before do
+      # Expect to be ordered by an explicit ordering.
+      raw_values.each_with_index { |value, id| node_model.create!(id: id, value: value) }
+    end
+
+    let(:nodes) { ::Gitlab::Graphql::Pagination::OffsetPaginatedRelation.new(node_model.order(value: :asc)) }
+
+    include_examples 'it maps to a specific connection class', Gitlab::Graphql::Pagination::OffsetActiveRecordRelationConnection
+  end
+
+  describe 'ActiveRecord::Relation' do
+    before do
+      # Expect to be ordered by ID descending
+      [3, 2, 1].zip(raw_values) { |id, value| node_model.create!(id: id, value: value) }
+    end
+
+    let(:nodes) { node_model.all }
+
+    include_examples 'it maps to a specific connection class', Gitlab::Graphql::Pagination::Keyset::Connection
+  end
+
+  describe 'ExternallyPaginatedArray' do
+    let(:nodes) { ::Gitlab::Graphql::ExternallyPaginatedArray.new(nil, nil, node.new(1), node.new(7)) }
+
+    include_examples 'it maps to a specific connection class', Gitlab::Graphql::Pagination::ExternallyPaginatedArrayConnection
+  end
+
+  describe 'Array' do
+    let(:nodes) { raw_values.map { |x| node.new(x) } }
+
+    include_examples 'it maps to a specific connection class', Gitlab::Graphql::Pagination::ArrayConnection
+  end
+end

spec/support/helpers/sorting_helper.rb

@@ -17,4 +17,35 @@ module SortingHelper
       click_link value
     end
   end
+
+  def nils_last(value)
+    NilsLast.new(value)
+  end
+
+  class NilsLast
+    include Comparable
+
+    attr_reader :value
+    delegate :==, :eql?, :hash, to: :value
+
+    def initialize(value)
+      @value = value
+      @reverse = false
+    end
+
+    def <=>(other)
+      return unless other.is_a?(self.class)
+      return 0 if value.nil? && other.value.nil?
+      return 1 if value.nil?
+      return -1 if other.value.nil?
+
+      int = value <=> other.value
+      @reverse ? -int : int
+    end
+
+    def -@
+      @reverse = true
+      self
+    end
+  end
 end