Improve GraphQL pagination specs

The pagination tests are updated to be clearer, and a missing pagination
test is added.

The sort_enum is fixed - previously the upper and lowercase forms were
not synonymous (one would have string values, and the other had symbol
values). Now both have symbol values, and a case where this caused a bug
(in the requirements_finder) was detected and fixed.

Since the pagination specs now expect actual values rather than strings
everywhere, a number of sort_param values have been updated to be
symbols, which map to graphql Enums.

app/graphql/resolvers/issues_resolver.rb

@@ -10,7 +10,7 @@ module Resolvers
     argument :sort, Types::IssueSortEnum,
               description: 'Sort issues by this criteria',
               required: false,
-              default_value: 'created_desc'
+              default_value: :created_desc
 
     type Types::IssueType.connection_type, null: true
 

app/graphql/resolvers/merge_requests_resolver.rb

@@ -52,7 +52,7 @@ module Resolvers
     argument :sort, Types::MergeRequestSortEnum,
              description: 'Sort merge requests by this criteria',
              required: false,
-             default_value: 'created_desc'
+             default_value: :created_desc
 
     def self.single
       ::Resolvers::MergeRequestResolver

app/graphql/resolvers/users_resolver.rb

@@ -17,7 +17,7 @@ module Resolvers
     argument :sort, Types::SortEnum,
              description: 'Sort users by this criteria',
              required: false,
-             default_value: 'created_desc'
+             default_value: :created_desc
 
     argument :search, GraphQL::STRING_TYPE,
              required: false,

app/graphql/types/sort_enum.rb

@@ -7,10 +7,10 @@ module Types
 
     # Deprecated, as we prefer uppercase enums
     # https://gitlab.com/groups/gitlab-org/-/epics/1838
-    value 'updated_desc', 'Updated at descending order', deprecated: { reason: 'Use UPDATED_DESC', milestone: '13.5' }
-    value 'updated_asc', 'Updated at ascending order', deprecated: { reason: 'Use UPDATED_ASC', milestone: '13.5' }
-    value 'created_desc', 'Created at descending order', deprecated: { reason: 'Use CREATED_DESC', milestone: '13.5' }
-    value 'created_asc', 'Created at ascending order', deprecated: { reason: 'Use CREATED_ASC', milestone: '13.5' }
+    value 'updated_desc', 'Updated at descending order', value: :updated_desc, deprecated: { reason: 'Use UPDATED_DESC', milestone: '13.5' }
+    value 'updated_asc', 'Updated at ascending order', value: :updated_asc, deprecated: { reason: 'Use UPDATED_ASC', milestone: '13.5' }
+    value 'created_desc', 'Created at descending order', value: :created_desc, deprecated: { reason: 'Use CREATED_DESC', milestone: '13.5' }
+    value 'created_asc', 'Created at ascending order', value: :created_asc, deprecated: { reason: 'Use CREATED_ASC', milestone: '13.5' }
 
     value 'UPDATED_DESC', 'Updated at descending order', value: :updated_desc
     value 'UPDATED_ASC', 'Updated at ascending order', value: :updated_asc

ee/app/finders/requirements_management/requirements_finder.rb

@@ -79,7 +79,7 @@ module RequirementsManagement
 
     def sort(items)
       sorts = RequirementsManagement::Requirement.simple_sorts.keys
-      sort = sorts.include?(params[:sort]) ? params[:sort] : 'id_desc'
+      sort = sorts.include?(params[:sort]&.to_s) ? params[:sort] : 'id_desc'
 
       items.order_by(sort)
     end

ee/spec/requests/api/graphql/namespace/projects_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe 'Namespace.projects' do
+  include GraphqlHelpers
+
+  describe 'sorting and pagination' do
+    let_it_be(:ns) { create(:group) }
+    let_it_be(:current_user) { create(:user) }
+
+    let!(:project_1) { create(:project, namespace: ns, name: 'Project', path: 'project') }
+    let!(:project_2) { create(:project, namespace: ns, name: 'Test Project', path: 'test-project') }
+    let!(:project_3) { create(:project, namespace: ns, name: 'Test', path: 'test') }
+    let!(:project_4) { create(:project, namespace: ns, name: 'Test Project Other', path: 'other-test-project') }
+
+    let(:data_path) { [:namespace, :projects] }
+
+    let(:ns_args) { graphql_args(full_path: ns.full_path) }
+    let(:project_args) { graphql_args(include_subgroups: true, search: 'test') }
+
+    before do
+      ns.add_owner(current_user)
+    end
+
+    def pagination_query(params, page_info)
+      graphql_query_for(:namespace, ns_args,
+        query_graphql_field(:projects, params + project_args, "#{page_info} edges { node { name } }"))
+    end
+
+    def pagination_results_data(data)
+      data.map { |project| project.dig('node', 'name') }
+    end
+
+    context 'when sorting by STORAGE' do
+      before do
+        project_4.statistics.update!(lfs_objects_size: 1, repository_size: 4.gigabytes)
+        project_2.statistics.update!(lfs_objects_size: 1, repository_size: 2.gigabytes)
+        project_3.statistics.update!(lfs_objects_size: 2, repository_size: 1.gigabytes)
+      end
+
+      it_behaves_like 'sorted paginated query' do
+        let(:sort_param)       { :STORAGE }
+        let(:first_param)      { 2 }
+        let(:expected_results) { [project_4, project_2, project_3].map(&:name) }
+      end
+
+      it_behaves_like 'sorted pagable query' do
+        let!(:project_5) { create(:project, namespace: ns, name: 'Test 5') }
+        let!(:project_6) { create(:project, namespace: ns, name: 'Test 6') }
+        let!(:project_7) { create(:project, namespace: ns, name: 'Test 7') }
+        let!(:project_8) { create(:project, namespace: ns, name: 'Test 8') }
+
+        let(:all_results) { expected_order.map { |p| { 'name' => p.name } } }
+        let(:sort_value)  { :STORAGE }
+        let(:expected_order) do
+          # Some jumbled random order, to ensure we aren't testing ID desc or asc
+          [project_5, project_3, project_7, project_8, project_2, project_6, project_4]
+        end
+
+        before do
+          expected_order.reverse.each_with_index do |p, i|
+            n = i + 1
+            p.statistics.update!(lfs_objects_size: n, repository_size: n.gigabytes)
+          end
+        end
+
+        def paging_query(params)
+          graphql_query_for(:namespace, ns_args,
+            query_graphql_field(:projects, params + project_args, "#{page_info} nodes { name }"))
+        end
+      end
+    end
+  end
+end

ee/spec/requests/api/graphql/project/issues_spec.rb

@@ -32,7 +32,7 @@ RSpec.describe 'getting an issue list for a project' do
 
       context 'when ascending' do
         it_behaves_like 'sorted paginated query' do
-          let(:sort_param)       { 'WEIGHT_ASC' }
+          let(:sort_param)       { :WEIGHT_ASC }
           let(:first_param)      { 2 }
           let(:expected_results) { [weight_issue3.iid, weight_issue5.iid, weight_issue1.iid, weight_issue4.iid, weight_issue2.iid] }
         end
@@ -40,7 +40,7 @@ RSpec.describe 'getting an issue list for a project' do
 
       context 'when descending' do
         it_behaves_like 'sorted paginated query' do
-          let(:sort_param)       { 'WEIGHT_DESC' }
+          let(:sort_param)       { :WEIGHT_DESC }
           let(:first_param)      { 2 }
           let(:expected_results) { [weight_issue1.iid, weight_issue5.iid, weight_issue3.iid, weight_issue4.iid, weight_issue2.iid] }
         end

ee/spec/requests/api/graphql/project/requirements_management/requirements_spec.rb

@@ -183,7 +183,7 @@ RSpec.describe 'getting a requirement list for a project' do
 
         context 'when ascending' do
           it_behaves_like 'sorted paginated query' do
-            let(:sort_param)       { 'created_asc' }
+            let(:sort_param)       { :CREATED_ASC }
             let(:first_param)      { 2 }
             let(:expected_results) { [requirement4.iid, requirement3.iid, requirement5.iid, requirement1.iid, requirement2.iid] }
           end
@@ -191,7 +191,7 @@ RSpec.describe 'getting a requirement list for a project' do
 
         context 'when descending' do
           it_behaves_like 'sorted paginated query' do
-            let(:sort_param)       { 'created_desc' }
+            let(:sort_param)       { :CREATED_DESC }
             let(:first_param)      { 2 }
             let(:expected_results) { [requirement2.iid, requirement1.iid, requirement5.iid, requirement3.iid, requirement4.iid] }
           end

ee/spec/requests/api/graphql/project/requirements_management/test_reports_spec.rb

@@ -75,15 +75,23 @@ RSpec.describe 'getting test reports of a requirement' do
         data.map { |test_report| test_report.dig('node', 'id') }
       end
 
+      let(:in_creation_order) do
+        [test_report_3, test_report_2, test_report_1]
+      end
+
+      it_behaves_like 'sorted paginated query' do
+        let(:sort_param)       { :CREATED_ASC }
+        let(:first_param)      { 2 }
+        let(:expected_results) do
+          in_creation_order.map { |r| global_id_of(r) }
+        end
+      end
+
       it_behaves_like 'sorted paginated query' do
-        let(:sort_param)       { 'created_asc' }
+        let(:sort_param)       { :CREATED_DESC }
         let(:first_param)      { 2 }
         let(:expected_results) do
-          [
-            test_report_3.to_global_id.to_s,
-            test_report_2.to_global_id.to_s,
-            test_report_1.to_global_id.to_s
-          ]
+          in_creation_order.reverse.map { |r| global_id_of(r) }
         end
       end
     end

spec/requests/api/graphql/namespace/projects_spec.rb

@@ -80,22 +80,28 @@ RSpec.describe 'getting projects' do
   end
 
   describe 'sorting and pagination' do
+    let_it_be(:ns) { create(:group) }
+    let_it_be(:current_user) { create(:user) }
+    let_it_be(:project_1) { create(:project, name: 'Project', path: 'project', namespace: ns) }
+    let_it_be(:project_2) { create(:project, name: 'Test Project', path: 'test-project', namespace: ns) }
+    let_it_be(:project_3) { create(:project, name: 'Test', path: 'test', namespace: ns) }
+    let_it_be(:project_4) { create(:project, name: 'Test Project Other', path: 'other-test-project', namespace: ns) }
+
     let(:data_path) { [:namespace, :projects] }
 
+    let(:ns_args) { { full_path: ns.full_path } }
+    let(:search) { 'test' }
+
+    before do
+      ns.add_owner(current_user)
+    end
+
     def pagination_query(params, page_info)
-      graphql_query_for(
-        'namespace',
-        { 'fullPath' => subject.full_path },
-        <<~QUERY
-        projects(includeSubgroups: #{include_subgroups}, search: "#{search}", #{params}) {
-          #{page_info} edges {
-            node {
-              #{all_graphql_fields_for('Project')}
-            }
-          }
-        }
-        QUERY
-      )
+      arguments = params.merge(include_subgroups: include_subgroups, search: search)
+      graphql_query_for(:namespace, ns_args, query_graphql_field(:projects, arguments, <<~GQL))
+        #{page_info}
+        edges { node { name } }
+      GQL
     end
 
     def pagination_results_data(data)
@@ -103,15 +109,8 @@ RSpec.describe 'getting projects' do
     end
 
     context 'when sorting by similarity' do
-      let!(:project_1) { create(:project, name: 'Project', path: 'project', namespace: subject) }
-      let!(:project_2) { create(:project, name: 'Test Project', path: 'test-project', namespace: subject) }
-      let!(:project_3) { create(:project, name: 'Test', path: 'test', namespace: subject) }
-      let!(:project_4) { create(:project, name: 'Test Project Other', path: 'other-test-project', namespace: subject) }
-      let(:search) { 'test' }
-      let(:current_user) { user }
-
       it_behaves_like 'sorted paginated query' do
-        let(:sort_param)       { 'SIMILARITY' }
+        let(:sort_param)       { :SIMILARITY }
         let(:first_param)      { 2 }
         let(:expected_results) { [project_3.name, project_2.name, project_4.name] }
       end

spec/requests/api/graphql/project/issues_spec.rb

@@ -164,7 +164,7 @@ RSpec.describe 'getting an issue list for a project' do
 
       context 'when ascending' do
         it_behaves_like 'sorted paginated query' do
-          let(:sort_param)       { 'DUE_DATE_ASC' }
+          let(:sort_param)       { :DUE_DATE_ASC }
           let(:first_param)      { 2 }
           let(:expected_results) { [due_issue3.iid, due_issue5.iid, due_issue1.iid, due_issue4.iid, due_issue2.iid] }
         end
@@ -172,7 +172,7 @@ RSpec.describe 'getting an issue list for a project' do
 
       context 'when descending' do
         it_behaves_like 'sorted paginated query' do
-          let(:sort_param)       { 'DUE_DATE_DESC' }
+          let(:sort_param)       { :DUE_DATE_DESC }
           let(:first_param)      { 2 }
           let(:expected_results) { [due_issue1.iid, due_issue5.iid, due_issue3.iid, due_issue4.iid, due_issue2.iid] }
         end
@@ -189,7 +189,7 @@ RSpec.describe 'getting an issue list for a project' do
 
       context 'when ascending' do
         it_behaves_like 'sorted paginated query' do
-          let(:sort_param)       { 'RELATIVE_POSITION_ASC' }
+          let(:sort_param)       { :RELATIVE_POSITION_ASC }
           let(:first_param)      { 2 }
           let(:expected_results) { [relative_issue5.iid, relative_issue3.iid, relative_issue1.iid, relative_issue4.iid, relative_issue2.iid] }
         end
@@ -209,7 +209,7 @@ RSpec.describe 'getting an issue list for a project' do
 
       context 'when ascending' do
         it_behaves_like 'sorted paginated query' do
-          let(:sort_param)       { 'PRIORITY_ASC' }
+          let(:sort_param)       { :PRIORITY_ASC }
           let(:first_param)      { 2 }
           let(:expected_results) { [priority_issue3.iid, priority_issue1.iid, priority_issue2.iid, priority_issue4.iid] }
         end
@@ -217,7 +217,7 @@ RSpec.describe 'getting an issue list for a project' do
 
       context 'when descending' do
         it_behaves_like 'sorted paginated query' do
-          let(:sort_param)       { 'PRIORITY_DESC' }
+          let(:sort_param)       { :PRIORITY_DESC }
           let(:first_param)      { 2 }
           let(:expected_results) { [priority_issue1.iid, priority_issue3.iid, priority_issue2.iid, priority_issue4.iid] }
         end
@@ -236,7 +236,7 @@ RSpec.describe 'getting an issue list for a project' do
 
       context 'when ascending' do
         it_behaves_like 'sorted paginated query' do
-          let(:sort_param)       { 'LABEL_PRIORITY_ASC' }
+          let(:sort_param)       { :LABEL_PRIORITY_ASC }
           let(:first_param)      { 2 }
           let(:expected_results) { [label_issue3.iid, label_issue1.iid, label_issue2.iid, label_issue4.iid] }
         end
@@ -244,7 +244,7 @@ RSpec.describe 'getting an issue list for a project' do
 
       context 'when descending' do
         it_behaves_like 'sorted paginated query' do
-          let(:sort_param)       { 'LABEL_PRIORITY_DESC' }
+          let(:sort_param)       { :LABEL_PRIORITY_DESC }
           let(:first_param)      { 2 }
           let(:expected_results) { [label_issue2.iid, label_issue3.iid, label_issue1.iid, label_issue4.iid] }
         end
@@ -261,7 +261,7 @@ RSpec.describe 'getting an issue list for a project' do
 
       context 'when ascending' do
         it_behaves_like 'sorted paginated query' do
-          let(:sort_param)       { 'MILESTONE_DUE_ASC' }
+          let(:sort_param)       { :MILESTONE_DUE_ASC }
           let(:first_param)      { 2 }
           let(:expected_results) { [milestone_issue2.iid, milestone_issue3.iid, milestone_issue1.iid] }
         end
@@ -269,7 +269,7 @@ RSpec.describe 'getting an issue list for a project' do
 
       context 'when descending' do
         it_behaves_like 'sorted paginated query' do
-          let(:sort_param)       { 'MILESTONE_DUE_DESC' }
+          let(:sort_param)       { :MILESTONE_DUE_DESC }
           let(:first_param)      { 2 }
           let(:expected_results) { [milestone_issue3.iid, milestone_issue2.iid, milestone_issue1.iid] }
         end

spec/requests/api/graphql/project/merge_requests_spec.rb

@@ -281,7 +281,7 @@ RSpec.describe 'getting merge request listings nested in a project' do
 
     context 'when sorting by merged_at DESC' do
       it_behaves_like 'sorted paginated query' do
-        let(:sort_param) { 'MERGED_AT_DESC' }
+        let(:sort_param) { :MERGED_AT_DESC }
         let(:first_param) { 2 }
 
         let(:expected_results) do

spec/requests/api/graphql/users_spec.rb

@@ -72,7 +72,7 @@ RSpec.describe 'Users' do
 
       context 'when ascending' do
         it_behaves_like 'sorted paginated query' do
-          let(:sort_param)       { 'created_asc' }
+          let(:sort_param)       { :CREATED_ASC }
           let(:first_param)      { 1 }
           let(:expected_results) { ascending_users }
         end
@@ -80,7 +80,7 @@ RSpec.describe 'Users' do
 
       context 'when descending' do
         it_behaves_like 'sorted paginated query' do
-          let(:sort_param)       { 'created_desc' }
+          let(:sort_param)       { :CREATED_DESC }
           let(:first_param)      { 1 }
           let(:expected_results) { ascending_users.reverse }
         end

spec/support/shared_examples/graphql/sorted_paginated_query_shared_examples.rb

@@ -47,12 +47,8 @@ RSpec.shared_examples 'sorted paginated query' do
   end
 
   describe do
-    let(:sort_argument)  { "sort: #{sort_param}" if sort_param.present? }
-    let(:first_argument) { "first: #{first_param}" if first_param.present? }
+    let(:sort_argument)  { graphql_args(sort: sort_param) }
     let(:params)         { sort_argument }
-    let(:start_cursor)   { graphql_data_at(*data_path, :pageInfo, :startCursor) }
-    let(:end_cursor)     { graphql_data_at(*data_path, :pageInfo, :endCursor) }
-    let(:sorted_edges)   { graphql_data_at(*data_path, :edges) }
     let(:page_info)      { "pageInfo { startCursor endCursor }" }
 
     def pagination_query(params, page_info)
@@ -67,29 +63,86 @@ RSpec.shared_examples 'sorted paginated query' do
       super(data)
     end
 
+    def results
+      edges = graphql_dig_at(graphql_data(fresh_response_data), *data_path, :edges)
+      pagination_results_data(edges)
+    end
+
+    def end_cursor
+      graphql_dig_at(graphql_data(fresh_response_data), *data_path, :page_info, :end_cursor)
+    end
+
+    let(:query) { pagination_query(params, page_info) }
+
     before do
-      post_graphql(pagination_query(params, page_info), current_user: current_user)
+      post_graphql(query, current_user: current_user)
     end
 
     context 'when sorting' do
       it 'sorts correctly' do
-        expect(pagination_results_data(sorted_edges)).to eq expected_results
+        expect(results).to eq expected_results
       end
 
       context 'when paginating' do
-        let(:params) { [sort_argument, first_argument].compact.join(',') }
+        let(:params) { sort_argument.merge(first: first_param) }
+        let(:first_page) { expected_results.first(first_param) }
+        let(:rest) { expected_results.drop(first_param) }
 
         it 'paginates correctly' do
-          expect(pagination_results_data(sorted_edges)).to eq expected_results.first(first_param)
+          expect(results).to eq first_page
 
-          cursored_query = pagination_query([sort_argument, "after: \"#{end_cursor}\""].compact.join(','), page_info)
+          cursored_query = pagination_query(sort_argument.merge(after: end_cursor), page_info)
           post_graphql(cursored_query, current_user: current_user)
 
-          expect(response).to have_gitlab_http_status(:ok)
+          expect(results).to eq rest
+        end
+      end
+    end
+  end
+end
+
+RSpec.shared_examples 'sorted pagable query' do
+  let(:sort_argument)  { graphql_args(sort: sort_value) }
+  let(:page_info)      { "pageInfo { startCursor endCursor }" }
+
+  def paging_query(params)
+    raise('paging_query(params) must be defined in the test, see example in comment') unless defined?(super)
+
+    super
+  end
+
+  def nodes
+    graphql_dig_at(graphql_data(fresh_response_data), *data_path, :nodes)
+  end
+
+  def end_cursor
+    graphql_dig_at(graphql_data(fresh_response_data), *data_path, :page_info, :end_cursor)
+  end
+
+  context 'when sorting' do
+    it 'sorts correctly' do
+      post_graphql(paging_query(sort_argument), current_user: current_user)
+
+      expect(nodes).to eq all_results
+    end
+
+    it 'has at least 5 items' do
+      # We need to page a few times - this makes sure we can page at least twice
+      expect(all_results.size).to be >= 5
+    end
+
+    context 'when paginating' do
+      let(:page_size) { 2 }
+
+      it 'paginates correctly' do
+        all_results.in_groups_of(page_size, false).reduce(nil) do |cursor, group|
+          q = paging_query(sort_argument.merge(first: page_size, after: cursor))
+
+          post_graphql(q, current_user: current_user)
 
-          response_data = graphql_dig_at(Gitlab::Json.parse(response.body), :data, *data_path, :edges)
+          expect(nodes).to eq(group)
 
-          expect(pagination_results_data(response_data)).to eq expected_results.drop(first_param)
+          end_cursor
         end
       end
     end