Merge branch 'fix-backward-keyset-pagination-for-merged-at' into 'master'

Fix backward keyset pagination for merged_at

See merge request gitlab-org/gitlab!43701

changelogs/unreleased/fix-backward-keyset-pagination-for-merged-at.yml

+---
+title: Fix GraphQL backward pagination when merge requests are ordered by merged_at
+merge_request: 43701
+author:
+type: fixed

lib/gitlab/graphql/pagination/keyset/connection.rb

@@ -110,8 +110,7 @@ module Gitlab
               end
 
               if last
-                # grab one more than we need
-                paginated_nodes = sliced_nodes.last(limit_value + 1)
+                paginated_nodes = LastItems.take_items(sliced_nodes, limit_value + 1)
 
                 # there is an extra node, so there is a previous page
                 @has_previous_page = paginated_nodes.count > limit_value

lib/gitlab/graphql/pagination/keyset/last_items.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Graphql
+    module Pagination
+      module Keyset
+        # This class handles the last(N) ActiveRecord call even if a special ORDER BY configuration is present.
+        # For the last(N) call, ActiveRecord calls reverse_order, however for some cases it raises
+        # ActiveRecord::IrreversibleOrderError error.
+        class LastItems
+          # rubocop: disable CodeReuse/ActiveRecord
+          def self.take_items(scope, count)
+            if custom_order = lookup_custom_reverse_order(scope.order_values)
+              items = scope.reorder(*custom_order).first(count) # returns a single record when count is nil
+              items.is_a?(Array) ? items.reverse : items
+            else
+              scope.last(count)
+            end
+          end
+          # rubocop: enable CodeReuse/ActiveRecord
+
+          # Detect special ordering and provide the reversed order
+          def self.lookup_custom_reverse_order(order_values)
+            if ordering_by_merged_at_and_mr_id_desc?(order_values)
+              [
+                Gitlab::Database.nulls_first_order('merge_request_metrics.merged_at', 'ASC'), # reversing the order
+                MergeRequest.arel_table[:id].asc
+              ]
+            elsif ordering_by_merged_at_and_mr_id_asc?(order_values)
+              [
+                Gitlab::Database.nulls_first_order('merge_request_metrics.merged_at', 'DESC'),
+                MergeRequest.arel_table[:id].asc
+              ]
+            end
+          end
+
+          def self.ordering_by_merged_at_and_mr_id_desc?(order_values)
+            order_values.size == 2 &&
+              order_values.first.to_s == Gitlab::Database.nulls_last_order('merge_request_metrics.merged_at', 'DESC') &&
+              order_values.last.is_a?(Arel::Nodes::Descending) &&
+              order_values.last.to_sql == MergeRequest.arel_table[:id].desc.to_sql
+          end
+
+          def self.ordering_by_merged_at_and_mr_id_asc?(order_values)
+            order_values.size == 2 &&
+              order_values.first.to_s == Gitlab::Database.nulls_last_order('merge_request_metrics.merged_at', 'ASC') &&
+              order_values.last.is_a?(Arel::Nodes::Descending) &&
+              order_values.last.to_sql == MergeRequest.arel_table[:id].desc.to_sql
+          end
+
+          private_class_method :ordering_by_merged_at_and_mr_id_desc?
+          private_class_method :ordering_by_merged_at_and_mr_id_asc?
+        end
+      end
+    end
+  end
+end

spec/lib/gitlab/graphql/pagination/keyset/last_items_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::Graphql::Pagination::Keyset::LastItems do
+  let_it_be(:merge_request) { create(:merge_request) }
+  let(:scope) { MergeRequest.order_merged_at_asc.with_order_id_desc }
+
+  subject { described_class.take_items(*args) }
+
+  context 'when the `count` parameter is nil' do
+    let(:args) { [scope, nil] }
+
+    it 'returns a single record' do
+      expect(subject).to eq(merge_request)
+    end
+  end
+
+  context 'when the `count` parameter is given' do
+    let(:args) { [scope, 1] }
+
+    it 'returns an array' do
+      expect(subject).to eq([merge_request])
+    end
+  end
+end

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

@@ -13,6 +13,7 @@ RSpec.describe 'getting merge request listings nested in a project' do
   let_it_be(:merge_request_b) { create(:merge_request, :closed, :unique_branches, source_project: project) }
   let_it_be(:merge_request_c) { create(:labeled_merge_request, :closed, :unique_branches, source_project: project, labels: [label]) }
   let_it_be(:merge_request_d) { create(:merge_request, :locked, :unique_branches, source_project: project) }
+  let_it_be(:merge_request_e) { create(:merge_request, :unique_branches, source_project: project) }
 
   let(:results) { graphql_data.dig('project', 'mergeRequests', 'nodes') }
 
@@ -118,7 +119,7 @@ RSpec.describe 'getting merge request listings nested in a project' do
 
   context 'there are no search params' do
     let(:search_params) { nil }
-    let(:mrs) { [merge_request_a, merge_request_b, merge_request_c, merge_request_d] }
+    let(:mrs) { [merge_request_a, merge_request_b, merge_request_c, merge_request_d, merge_request_e] }
 
     it_behaves_like 'searching with parameters'
   end
@@ -241,16 +242,50 @@ RSpec.describe 'getting merge request listings nested in a project' do
         let(:expected_results) do
           [
             merge_request_b,
-            merge_request_c,
             merge_request_d,
+            merge_request_c,
+            merge_request_e,
             merge_request_a
           ].map(&:to_gid).map(&:to_s)
         end
 
         before do
-          merge_request_c.metrics.update!(merged_at: 5.days.ago)
+          five_days_ago = 5.days.ago
+
+          merge_request_d.metrics.update!(merged_at: five_days_ago)
+
+          # same merged_at, the second order column will decide (merge_request.id)
+          merge_request_c.metrics.update!(merged_at: five_days_ago)
+
           merge_request_b.metrics.update!(merged_at: 1.day.ago)
         end
+
+        context 'when paginating backwards' do
+          let(:params) { 'first: 2, sort: MERGED_AT_DESC' }
+          let(:page_info) { 'pageInfo { startCursor endCursor }' }
+
+          before do
+            post_graphql(pagination_query(params, page_info), current_user: current_user)
+          end
+
+          it 'paginates backwards correctly' do
+            # first page
+            first_page_response_data = graphql_dig_at(Gitlab::Json.parse(response.body), :data, *data_path, :edges)
+            end_cursor = graphql_dig_at(Gitlab::Json.parse(response.body), :data, :project, :mergeRequests, :pageInfo, :endCursor)
+
+            # second page
+            params = "first: 2, after: \"#{end_cursor}\", sort: MERGED_AT_DESC"
+            post_graphql(pagination_query(params, page_info), current_user: current_user)
+            start_cursor = graphql_dig_at(Gitlab::Json.parse(response.body), :data, :project, :mergeRequests, :pageInfo, :start_cursor)
+
+            # going back to the first page
+
+            params = "last: 2, before: \"#{start_cursor}\", sort: MERGED_AT_DESC"
+            post_graphql(pagination_query(params, page_info), current_user: current_user)
+            backward_paginated_response_data = graphql_dig_at(Gitlab::Json.parse(response.body), :data, *data_path, :edges)
+            expect(first_page_response_data).to eq(backward_paginated_response_data)
+          end
+        end
       end
     end
   end