Merge branch 'issue_281152' into 'master'

Fix backward keyset pagination for epics start_date/end_date

See merge request gitlab-org/gitlab!50579

ee/app/graphql/types/epic_sort_enum.rb

@@ -5,9 +5,9 @@ module Types
     graphql_name 'EpicSort'
     description 'Roadmap sort values'
 
-    value 'start_date_desc', 'Start date at descending order.'
-    value 'start_date_asc', 'Start date at ascending order.'
-    value 'end_date_desc', 'End date at descending order.'
-    value 'end_date_asc', 'End date at ascending order.'
+    value 'start_date_desc', 'Start date at descending order.', value: :start_date_desc
+    value 'start_date_asc', 'Start date at ascending order.', value: :start_date_asc
+    value 'end_date_desc', 'End date at descending order.', value: :end_date_desc
+    value 'end_date_asc', 'End date at ascending order.', value: :end_date_asc
   end
 end

ee/app/models/ee/epic.rb

@@ -88,19 +88,27 @@ module EE
       end
 
       scope :order_start_date_asc, -> do
-        reorder(::Gitlab::Database.nulls_last_order('start_date'), 'id DESC')
+        keyset_order = keyset_pagination_for(column_name: :start_date)
+
+        reorder(keyset_order)
+      end
+
+      scope :order_start_date_desc, -> do
+        keyset_order = keyset_pagination_for(column_name: :start_date, direction: 'DESC')
+
+        reorder(keyset_order)
       end
 
       scope :order_end_date_asc, -> do
-        reorder(::Gitlab::Database.nulls_last_order('end_date'), 'id DESC')
+        keyset_order = keyset_pagination_for(column_name: :end_date)
+
+        reorder(keyset_order)
       end
 
       scope :order_end_date_desc, -> do
-        reorder(::Gitlab::Database.nulls_last_order('end_date', 'DESC'), 'id DESC')
-      end
+        keyset_order = keyset_pagination_for(column_name: :end_date, direction: 'DESC')
 
-      scope :order_start_date_desc, -> do
-        reorder(::Gitlab::Database.nulls_last_order('start_date', 'DESC'), 'id DESC')
+        reorder(keyset_order)
       end
 
       scope :order_closed_date_desc, -> { reorder(closed_at: :desc) }
@@ -149,6 +157,20 @@ module EE
         select(selection).in_parents(node.parent_ids)
       end
 
+      # This is being overriden from Issuable to be able to use
+      # keyset pagination, allowing queries with these
+      # ordering statements to be reversible on GraphQL.
+      def self.sort_by_attribute(method, excluded_labels: [])
+        case method.to_s
+        when 'start_date_asc' then order_start_date_asc
+        when 'start_date_desc' then order_start_date_desc
+        when 'end_date_asc' then order_end_date_asc
+        when 'end_date_desc' then order_end_date_desc
+        else
+          super
+        end
+      end
+
       private
 
       def set_fixed_start_date
@@ -230,10 +252,10 @@ module EE
       def simple_sorts
         super.merge(
           {
-            'start_date_asc' => -> { order_start_date_asc.with_order_id_desc },
-            'start_date_desc' => -> { order_start_date_desc.with_order_id_desc },
-            'end_date_asc' => -> { order_end_date_asc.with_order_id_desc },
-            'end_date_desc' => -> { order_end_date_desc.with_order_id_desc }
+            'start_date_asc' => -> { order_start_date_asc },
+            'start_date_desc' => -> { order_start_date_desc },
+            'end_date_asc' => -> { order_end_date_asc },
+            'end_date_desc' => -> { order_end_date_desc }
           }
         )
       end
@@ -290,6 +312,26 @@ module EE
 
         records.map { |record| record.attributes.with_indifferent_access }
       end
+
+      def keyset_pagination_for(column_name:, direction: 'ASC')
+        reverse_direction = direction == 'ASC' ? 'DESC' : 'ASC'
+
+        ::Gitlab::Pagination::Keyset::Order.build([
+          ::Gitlab::Pagination::Keyset::ColumnOrderDefinition.new(
+            attribute_name: column_name.to_s,
+            column_expression: ::Epic.arel_table[column_name],
+            order_expression: ::Gitlab::Database.nulls_last_order(column_name, direction),
+            reversed_order_expression: ::Gitlab::Database.nulls_last_order(column_name, reverse_direction),
+            order_direction: direction,
+            distinct: false,
+            nullable: :nulls_last
+          ),
+          ::Gitlab::Pagination::Keyset::ColumnOrderDefinition.new(
+            attribute_name: 'id',
+            order_expression: ::Epic.arel_table[:id].desc
+          )
+        ])
+      end
     end
 
     def resource_parent

ee/changelogs/unreleased/issue_281152.yml

+---
+title: Fix backward keyset pagination for epics start_date/end_date
+merge_request: 50579
+author:
+type: fixed

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

@@ -7,13 +7,13 @@ require 'spec_helper'
 RSpec.describe 'Epics through GroupQuery' do
   include GraphqlHelpers
 
-  let(:user)       { create(:user) }
-  let(:group)      { create(:group) }
-  let(:project)    { create(:project, :public, group: group) }
-  let(:label)      { create(:label) }
-  let(:epic)       { create(:labeled_epic, group: group, labels: [label]) }
-  let(:epics_data) { graphql_data['group']['epics']['edges'] }
-  let(:epic_data)  { graphql_data['group']['epic'] }
+  let(:user)        { create(:user) }
+  let_it_be(:group) { create(:group) }
+  let(:project)     { create(:project, :public, group: group) }
+  let(:label)       { create(:label) }
+  let(:epic)        { create(:labeled_epic, group: group, labels: [label]) }
+  let(:epics_data)  { graphql_data['group']['epics']['edges'] }
+  let(:epic_data)   { graphql_data['group']['epic'] }
 
   # similar to GET /groups/:id/epics
   describe 'Get list of epics from a group' do
@@ -60,14 +60,69 @@ RSpec.describe 'Epics through GroupQuery' do
     end
 
     context 'with multiple epics' do
-      let(:user2)  { create(:user) }
-      let!(:epic)  { create(:epic, group: group, state: :closed, created_at: 3.days.ago, updated_at: 2.days.ago) }
-      let!(:epic2) { create(:epic, author: user2, group: group, title: 'foo', description: 'bar', created_at: 2.days.ago, updated_at: 3.days.ago) }
+      let_it_be(:user2) { create(:user) }
+      let_it_be(:epic)  { create(:epic, group: group, state: :closed, created_at: 3.days.ago, updated_at: 2.days.ago, start_date: 2.days.ago, end_date: 4.days.ago) }
+      let_it_be(:epic2) { create(:epic, author: user2, group: group, title: 'foo', description: 'bar', created_at: 2.days.ago, updated_at: 3.days.ago, start_date: 3.days.ago, end_date: 3.days.ago ) }
 
       before do
         stub_licensed_features(epics: true)
       end
 
+      context 'with sort and pagination' do
+        let_it_be(:epic3) { create(:epic, group: group, start_date: 4.days.ago, end_date: 7.days.ago ) }
+        let_it_be(:epic4) { create(:epic, group: group, start_date: 5.days.ago, end_date: 6.days.ago ) }
+        let(:current_user) { user }
+        let(:data_path) { [:group, :epics] }
+
+        def pagination_query(params)
+          query =
+            <<~QUERY
+            epics(#{params}) {
+              #{page_info}
+              nodes { id }
+            }
+            QUERY
+
+          graphql_query_for('group', { 'fullPath' => group.full_path }, ['epicsEnabled', query])
+        end
+
+        def global_ids(*epics)
+          epics.map { |epic| global_id_of(epic) }
+        end
+
+        context 'with start_date_asc' do
+          it_behaves_like 'sorted paginated query', is_reversible: true do
+            let(:sort_param) { :start_date_asc }
+            let(:first_param) { 2 }
+            let(:expected_results) { global_ids(epic4, epic3, epic2, epic) }
+          end
+        end
+
+        context 'with start_date_desc' do
+          it_behaves_like 'sorted paginated query', is_reversible: true do
+            let(:sort_param) { :start_date_desc }
+            let(:first_param) { 2 }
+            let(:expected_results) { global_ids(epic, epic2, epic3, epic4) }
+          end
+        end
+
+        context 'with end_date_asc' do
+          it_behaves_like 'sorted paginated query', is_reversible: true do
+            let(:sort_param) { :end_date_asc }
+            let(:first_param) { 2 }
+            let(:expected_results) { global_ids(epic3, epic4, epic, epic2) }
+          end
+        end
+
+        context 'with end_date_desc' do
+          it_behaves_like 'sorted paginated query', is_reversible: true do
+            let(:sort_param) { :end_date_desc }
+            let(:first_param) { 2 }
+            let(:expected_results) { global_ids(epic2, epic, epic4, epic3) }
+          end
+        end
+      end
+
       it 'sorts by created_at descending by default' do
         post_graphql(query, current_user: user)
 

lib/gitlab/pagination/keyset/order.rb

@@ -55,14 +55,14 @@ module Gitlab
       #   scope :created_at_ordered, -> {
       #     keyset_order = Gitlab::Pagination::Keyset::Order.build([
       #       Gitlab::Pagination::Keyset::ColumnOrderDefinition.new(
-      #         attribute: :created_at,
+      #         attribute_name: :created_at,
       #         column_expression: Project.arel_table[:created_at],
       #         order_expression: Project.arel_table[:created_at].asc,
       #         distinct: false, # values in the column are not unique
       #         nullable: :nulls_last # we might see NULL values (bottom)
       #       ),
       #       Gitlab::Pagination::Keyset::ColumnOrderDefinition.new(
-      #         attribute: :id,
+      #         attribute_name: :id,
       #         order_expression: Project.arel_table[:id].asc
       #       )
       #     ])

spec/support/shared_examples/graphql/sorted_paginated_query_shared_examples.rb

@@ -44,7 +44,7 @@
 #         end
 #       end
 #
-RSpec.shared_examples 'sorted paginated query' do
+RSpec.shared_examples 'sorted paginated query' do |conditions = {}|
   # Provided as a convenience when constructing queries using string concatenation
   let(:page_info) { 'pageInfo { startCursor endCursor }' }
   # Convenience for using default implementation of pagination_results_data
@@ -123,6 +123,16 @@ RSpec.shared_examples 'sorted paginated query' do
           expect(results).to eq first_page
         end
       end
+
+      context 'when last and sort params are present', if: conditions[:is_reversible] do
+        let(:params) { sort_argument.merge(last: 1) }
+
+        it 'fetches last elements without error' do
+          post_graphql(pagination_query(params), current_user: current_user)
+
+          expect(results.first).to eq(expected_results.last)
+        end
+      end
     end
   end
 end