diff --git a/config/initializers/active_record_keyset_pagination.rb b/config/initializers/active_record_keyset_pagination.rb index f5692c95276b473fe36f0be45f82e95038e33fbc..7f830cafd31251eca2e39e8eb04836809c159155 100644 --- a/config/initializers/active_record_keyset_pagination.rb +++ b/config/initializers/active_record_keyset_pagination.rb @@ -1,10 +1,36 @@ # frozen_string_literal: true module PaginatorExtension + KEYSET_ORDER_PLACEHOLDER = Object.new + # This method loads the records for the requested page and returns a keyset paginator object. def keyset_paginate(cursor: nil, per_page: 20, keyset_order_options: {}) Gitlab::Pagination::Keyset::Paginator.new(scope: self.dup, cursor: cursor, per_page: per_page, keyset_order_options: keyset_order_options) end + + # This modifies `reverse_sql_order` so that it is aware of Gitlab::Pagination::Keyset::Order which + # can reverse order clauses with NULLS LAST because we provide it a `reversed_order_expression`. + # This allows us to use `#last` on these relations. + # + # Overrides https://github.com/rails/rails/blob/v6.1.6.1/activerecord/lib/active_record/relation/query_methods.rb#L1331-L1358 + def reverse_sql_order(order_query) + return super if order_query.empty? + + keyset_order_values = [] + + order_query_without_keyset = order_query.flat_map do |o| + next o unless o.is_a?(Gitlab::Pagination::Keyset::Order) + + keyset_order_values << o + KEYSET_ORDER_PLACEHOLDER + end + + super(order_query_without_keyset).map do |o| + next o unless o == KEYSET_ORDER_PLACEHOLDER + + keyset_order_values.shift.reversed_order + end + end end ActiveSupport.on_load(:active_record) do diff --git a/lib/gitlab/graphql/pagination/keyset/connection.rb b/lib/gitlab/graphql/pagination/keyset/connection.rb index b074c27399697da0e8e1e7a3cbd349f68038acac..b5f2c5ee6e8cdbb382129b8f37634c6e628ec8ff 100644 --- a/lib/gitlab/graphql/pagination/keyset/connection.rb +++ b/lib/gitlab/graphql/pagination/keyset/connection.rb @@ -116,7 +116,7 @@ def limited_nodes end if last - paginated_nodes = LastItems.take_items(sliced_nodes, limit_value + 1) + paginated_nodes = sliced_nodes.last(limit_value + 1) # there is an extra node, so there is a previous page @has_previous_page = paginated_nodes.count > limit_value diff --git a/lib/gitlab/graphql/pagination/keyset/last_items.rb b/lib/gitlab/graphql/pagination/keyset/last_items.rb deleted file mode 100644 index 960567a6fbc26d6219b09ba262c93951e687a496..0000000000000000000000000000000000000000 --- a/lib/gitlab/graphql/pagination/keyset/last_items.rb +++ /dev/null @@ -1,25 +0,0 @@ -# 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 Gitlab::Pagination::Keyset::Order.keyset_aware?(scope) - order = Gitlab::Pagination::Keyset::Order.extract_keyset_order_object(scope) - items = scope.reorder(order.reversed_order).first(count) - items.is_a?(Array) ? items.reverse : items - else - scope.last(count) - end - end - end - end - end - end -end diff --git a/spec/lib/gitlab/graphql/pagination/keyset/last_items_spec.rb b/spec/lib/gitlab/graphql/pagination/keyset/last_items_spec.rb deleted file mode 100644 index 792cb03e8c7aa716ab9098609dbf75aea2c6e0f9..0000000000000000000000000000000000000000 --- a/spec/lib/gitlab/graphql/pagination/keyset/last_items_spec.rb +++ /dev/null @@ -1,27 +0,0 @@ -# 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 } - - 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