From ae7c50798599ca4fb902c41491b03911a37ea3fa Mon Sep 17 00:00:00 2001 From: c_fons Date: Fri, 31 Oct 2025 18:09:07 +0000 Subject: [PATCH 1/9] Add keyset pagination for data management API This adds the necessary parameters and methods so that models included in the data management API support keyset pagination EE: true Changelog: added --- ee/app/models/concerns/orderable.rb | 5 +++ ee/spec/models/concerns/orderable_spec.rb | 54 +++++++++++++++++++++++ 2 files changed, 59 insertions(+) create mode 100644 ee/spec/models/concerns/orderable_spec.rb diff --git a/ee/app/models/concerns/orderable.rb b/ee/app/models/concerns/orderable.rb index d0724d9d1ba973..5c8fd08925bed4 100644 --- a/ee/app/models/concerns/orderable.rb +++ b/ee/app/models/concerns/orderable.rb @@ -5,5 +5,10 @@ module Orderable # rubocop:disable Gitlab/BoundedContexts -- general purpose con included do scope :order_by_primary_key, -> { order(*Array(primary_key).map { |key| arel_table[key] }) } + + # By default, only support generic ordering on primary keys as they're always indexed + def self.supported_keyset_orderings + Array(primary_key).index_with { |_k| [:asc, :desc] } + end end end diff --git a/ee/spec/models/concerns/orderable_spec.rb b/ee/spec/models/concerns/orderable_spec.rb new file mode 100644 index 00000000000000..d9383c1e3034bd --- /dev/null +++ b/ee/spec/models/concerns/orderable_spec.rb @@ -0,0 +1,54 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Orderable, feature_category: :shared do + let_it_be(:dummy_model_class) do + Class.new(ActiveRecord::Base) do + self.table_name = '_test_dummy_models' + include Orderable + end + end + + context 'when primary_key is a single column' do + before do + allow(dummy_model_class).to receive(:primary_key).and_return('id') + end + + describe '.supported_keyset_orderings' do + it 'returns a hash with the primary key and asc/desc orderings' do + expect(dummy_model_class.supported_keyset_orderings).to eq({ 'id' => [:asc, :desc] }) + end + end + + describe '.order_by_primary_key' do + it 'orders by primary key in ascending order' do + result = dummy_model_class.order_by_primary_key + + expect(result.to_sql).to include("ORDER BY \"_test_dummy_models\".\"id\"") + end + end + end + + context 'when primary_key is an array of columns (composite primary key)' do + before do + allow(dummy_model_class).to receive(:primary_key).and_return(%w[id partition_id]) + end + + describe '.supported_keyset_orderings' do + it 'returns a hash with each primary key and asc/desc orderings' do + expect(dummy_model_class.supported_keyset_orderings).to eq({ 'id' => [:asc, :desc], + 'partition_id' => [:asc, :desc] }) + end + end + + describe '.order_by_primary_key' do + it 'orders by primary key in ascending order' do + expected_order = "ORDER BY \"_test_dummy_models\".\"id\", \"_test_dummy_models\".\"partition_id\"" + result = dummy_model_class.order_by_primary_key + + expect(result.to_sql).to include(expected_order) + end + end + end +end -- GitLab From 9d76cb36312304d8a8474af12be1e5915f1dd25e Mon Sep 17 00:00:00 2001 From: c_fons Date: Tue, 4 Nov 2025 17:58:27 +0000 Subject: [PATCH 2/9] Add a new scope to support keyset ordering Use the existing Orderable concern to create a new keyset ordering scope --- ee/app/models/concerns/orderable.rb | 13 +++++- ee/lib/api/admin/data_management.rb | 15 ++++++- ee/spec/models/concerns/orderable_spec.rb | 24 ++++++++-- .../api/admin/data_management_spec.rb | 45 +++++++++++++++++-- 4 files changed, 88 insertions(+), 9 deletions(-) diff --git a/ee/app/models/concerns/orderable.rb b/ee/app/models/concerns/orderable.rb index 5c8fd08925bed4..f441b0b40ebaa1 100644 --- a/ee/app/models/concerns/orderable.rb +++ b/ee/app/models/concerns/orderable.rb @@ -5,10 +5,21 @@ module Orderable # rubocop:disable Gitlab/BoundedContexts -- general purpose con included do scope :order_by_primary_key, -> { order(*Array(primary_key).map { |key| arel_table[key] }) } + scope :keyset_order_by_primary_key, ->(sort_order = 'asc') { + keyset_order = Gitlab::Pagination::Keyset::Order.build( + Array(primary_key).map do |key| + Gitlab::Pagination::Keyset::ColumnOrderDefinition.new( + attribute_name: key, + order_expression: sort_order == 'asc' ? arel_table[key.to_sym].asc : arel_table[key.to_sym].desc) + end + ) + + order(keyset_order) + } # By default, only support generic ordering on primary keys as they're always indexed def self.supported_keyset_orderings - Array(primary_key).index_with { |_k| [:asc, :desc] } + Array(primary_key).index_with { |_k| [:asc, :desc] }.symbolize_keys end end end diff --git a/ee/lib/api/admin/data_management.rb b/ee/lib/api/admin/data_management.rb index dbc58076dec720..dea6e5c22ce564 100644 --- a/ee/lib/api/admin/data_management.rb +++ b/ee/lib/api/admin/data_management.rb @@ -11,6 +11,7 @@ class DataManagement < ::API::Base AVAILABLE_MODEL_NAMES = Gitlab::Geo::ModelMapper.available_model_names.freeze VERIFICATION_STATES = %w[pending started succeeded failed disabled].freeze + # AVAILABLE_MODEL_ID_FIELDS = Gitlab::Geo::ModelMapper.available_models.flat_map(&:primary_key).uniq.freeze before do authenticated_as_admin! @@ -153,6 +154,8 @@ def decoded_identifiers(identifier_array) type: String, desc: 'The checksum status of the records to filter by', values: VERIFICATION_STATES + optional :cursor, type: String, desc: 'Cursor for obtaining the next set of records' + optional :sort, type: String, values: %w[asc desc], default: 'asc', desc: 'Order of sorting' end get do model_class = Gitlab::Geo::ModelMapper.find_from_name(params[:model_name]) @@ -168,9 +171,17 @@ def decoded_identifiers(identifier_array) relation = relation.with_verification_state(:"verification_#{params[:checksum_state]}") end - relation = relation.order_by_primary_key + if params[:pagination] == 'keyset' + # the order_by parameter's default value is the primary key column of the model + # TODO: not sure it will have an influence on the result... seems like the scope is what matters + model_pk = model_class.primary_key + params[:order_by] = model_pk.is_a?(Array) ? model_pk.first : model_pk - present paginate(relation, without_count: true), with: Entities::Admin::Model + present paginate_with_strategies(relation.keyset_order_by_primary_key(params[:sort])), + with: Entities::Admin::Model + else + present paginate(relation.order_by_primary_key), with: Entities::Admin::Model + end end # Example request: diff --git a/ee/spec/models/concerns/orderable_spec.rb b/ee/spec/models/concerns/orderable_spec.rb index d9383c1e3034bd..c2a25d028ecfbc 100644 --- a/ee/spec/models/concerns/orderable_spec.rb +++ b/ee/spec/models/concerns/orderable_spec.rb @@ -17,7 +17,7 @@ describe '.supported_keyset_orderings' do it 'returns a hash with the primary key and asc/desc orderings' do - expect(dummy_model_class.supported_keyset_orderings).to eq({ 'id' => [:asc, :desc] }) + expect(dummy_model_class.supported_keyset_orderings).to eq({ id: [:asc, :desc] }) end end @@ -28,6 +28,14 @@ expect(result.to_sql).to include("ORDER BY \"_test_dummy_models\".\"id\"") end end + + describe '.keyset_order_by_primary_key' do + it 'orders by primary key in ascending order' do + result = dummy_model_class.keyset_order_by_primary_key + + expect(result.to_sql).to include("ORDER BY \"_test_dummy_models\".\"id\" ASC") + end + end end context 'when primary_key is an array of columns (composite primary key)' do @@ -37,8 +45,8 @@ describe '.supported_keyset_orderings' do it 'returns a hash with each primary key and asc/desc orderings' do - expect(dummy_model_class.supported_keyset_orderings).to eq({ 'id' => [:asc, :desc], - 'partition_id' => [:asc, :desc] }) + expect(dummy_model_class.supported_keyset_orderings).to eq({ id: [:asc, :desc], + partition_id: [:asc, :desc] }) end end @@ -50,5 +58,15 @@ expect(result.to_sql).to include(expected_order) end end + + describe '.keyset_order_by_primary_key' do + it 'orders by primary key in ascending order' do + expected_sql = "ORDER BY \"_test_dummy_models\".\"id\" ASC, \"_test_dummy_models\".\"partition_id\" ASC" + + result = dummy_model_class.keyset_order_by_primary_key + + expect(result.to_sql).to include(expected_sql) + end + end end end diff --git a/ee/spec/requests/api/admin/data_management_spec.rb b/ee/spec/requests/api/admin/data_management_spec.rb index 1db63ab22209d6..87fa9d702dba31 100644 --- a/ee/spec/requests/api/admin/data_management_spec.rb +++ b/ee/spec/requests/api/admin/data_management_spec.rb @@ -13,10 +13,12 @@ context 'with feature flag enabled' do context 'when authenticated as admin' do context 'with valid model name' do + let_it_be(:api_path) { "/admin/data_management/snippet_repository" } + it 'returns matching object data' do expected_model = create(:snippet_repository) - get api("/admin/data_management/snippet_repository", admin, admin_mode: true) + get api(api_path, admin, admin_mode: true) expect(response).to have_gitlab_http_status(:ok) expect(json_response.first).to include('record_identifier' => expected_model.id, @@ -27,7 +29,7 @@ it 'paginates results correctly' do create_list(:snippet_repository, 9) - get api("/admin/data_management/snippet_repository?per_page=5", admin, admin_mode: true) + get api("#{api_path}?per_page=5", admin, admin_mode: true) expect(response).to have_gitlab_http_status(:ok) expect(json_response.size).to eq(5) @@ -35,7 +37,7 @@ expect(response.headers['X-Page']).to eq('1') expect(response.headers['X-Per-Page']).to eq('5') - get api("/admin/data_management/snippet_repository?per_page=5&page=2", admin, admin_mode: true) + get api("#{api_path}?per_page=5&page=2", admin, admin_mode: true) expect(response).to have_gitlab_http_status(:ok) expect(json_response.size).to eq(4) # Remaining 4 records @@ -95,6 +97,43 @@ end end + context 'with keyset pagination and ordering' do + let_it_be(:list) { create_list(:snippet_repository, 3) } + + it 'returns first page with cursor to next page' do + get api(api_path, admin, admin_mode: true), params: { pagination: 'keyset', per_page: 2 } + + expect(response).to have_gitlab_http_status(:ok) + expect(json_response.size).to eq(2) + expect(json_response.pluck('record_identifier')).to contain_exactly(list.first.id, list.second.id) + expect(response.headers["Link"]).to include("cursor") + next_cursor = response.headers["Link"].match("(?cursor=.*?)&")["cursor_data"] + + get api(api_path, admin, admin_mode: true), + params: { pagination: 'keyset', per_page: 2 }.merge(Rack::Utils.parse_query(next_cursor)) + + expect(json_response.size).to eq(1) + expect(json_response.pluck('record_identifier')).to contain_exactly(list.last.id) + expect(response.headers).not_to include("Link") + end + + it 'orders descending results correctly' do + get api(api_path, admin, admin_mode: true), params: { pagination: 'keyset', per_page: 3, sort: 'desc' } + + expect(response).to have_gitlab_http_status(:ok) + expect(json_response.size).to eq(3) + expect(json_response.pluck('record_identifier')).to eq(list.map(&:id).reverse) + end + + it 'orders ascending results correctly' do + get api(api_path, admin, admin_mode: true), params: { pagination: 'keyset', per_page: 3, sort: 'asc' } + + expect(response).to have_gitlab_http_status(:ok) + expect(json_response.size).to eq(3) + expect(json_response.pluck('record_identifier')).to eq(list.map(&:id)) + end + end + context 'with filtering based on ids' do context 'with integer ids' do let_it_be(:list) { create_list(:project, 3) } -- GitLab From 8ba4cf84afdc083cfab031d54d7e92866bd8bbbe Mon Sep 17 00:00:00 2001 From: c_fons Date: Thu, 6 Nov 2025 18:01:16 +0100 Subject: [PATCH 3/9] Add tests for composite PKs --- ee/lib/api/admin/data_management.rb | 4 +- .../api/admin/data_management_spec.rb | 174 ++++++++++-------- 2 files changed, 99 insertions(+), 79 deletions(-) diff --git a/ee/lib/api/admin/data_management.rb b/ee/lib/api/admin/data_management.rb index dea6e5c22ce564..4076533e4fde25 100644 --- a/ee/lib/api/admin/data_management.rb +++ b/ee/lib/api/admin/data_management.rb @@ -172,8 +172,8 @@ def decoded_identifiers(identifier_array) end if params[:pagination] == 'keyset' - # the order_by parameter's default value is the primary key column of the model - # TODO: not sure it will have an influence on the result... seems like the scope is what matters + # the order_by parameter's default value is the primary key column of the model - + # using the first column for composite keys model_pk = model_class.primary_key params[:order_by] = model_pk.is_a?(Array) ? model_pk.first : model_pk diff --git a/ee/spec/requests/api/admin/data_management_spec.rb b/ee/spec/requests/api/admin/data_management_spec.rb index 87fa9d702dba31..b2f77c145d7d5a 100644 --- a/ee/spec/requests/api/admin/data_management_spec.rb +++ b/ee/spec/requests/api/admin/data_management_spec.rb @@ -21,7 +21,7 @@ get api(api_path, admin, admin_mode: true) expect(response).to have_gitlab_http_status(:ok) - expect(json_response.first).to include('record_identifier' => expected_model.id, + expect(json_response.first).to include('record_identifier' => record_identifier(expected_model), 'model_class' => expected_model.class.name) end @@ -59,42 +59,6 @@ expect(second_page_ids).to eq(second_page_ids.sort) expect(first_page_ids.last).to be < second_page_ids.first end - - context 'with composite IDs' do - let_it_be(:list) { create_list(:virtual_registries_packages_maven_cache_entry, 9) } - # We're using this Entry model because it will be the first model with composite PKs supported by Geo. - # The model isn't Geo-ready yet, so we need to mock its interface in this test to simulate its future - # implementation. - let_it_be(:orderable_klass) do - Class.new(list.first.class) do - include Orderable - end - end - - before do - # The VirtualRegistries::Packages::Maven::Cache::Entry model is not in the allowed list yet. - # This is why we need to force the ModelMapper to return the stubbed class instead of the model - # passed as parameters. - allow(Gitlab::Geo::ModelMapper).to receive(:find_from_name).with('project').and_return(orderable_klass) - end - - it 'paginates results' do - get api("/admin/data_management/project?per_page=5", admin, admin_mode: true) - - expect(response).to have_gitlab_http_status(:ok) - expect(json_response.size).to eq(5) - expect(response.headers['X-Next-Page']).to eq('2') - expect(response.headers['X-Page']).to eq('1') - expect(response.headers['X-Per-Page']).to eq('5') - - get api("/admin/data_management/project?per_page=5&page=2", admin, admin_mode: true) - - expect(response).to have_gitlab_http_status(:ok) - expect(json_response.size).to eq(4) # Remaining 4 records - expect(response.headers['X-Page']).to eq('2') - expect(response.headers['X-Next-Page']).to be_empty - end - end end context 'with keyset pagination and ordering' do @@ -134,55 +98,100 @@ end end - context 'with filtering based on ids' do - context 'with integer ids' do - let_it_be(:list) { create_list(:project, 3) } + context 'with composite ids' do + let_it_be(:list) { create_list(:virtual_registries_packages_maven_cache_entry, 9) } + let_it_be(:ids_list) { list.map { |model| record_identifier(model) } } + # We're using this Entry model because it will be the first model with composite PKs supported by Geo. + # The model isn't Geo-ready yet, so we need to mock its interface in this test to simulate its future + # implementation. + let_it_be(:orderable_klass) do + Class.new(list.first.class) do + include Orderable + end + end - it 'filters passed ids' do - get api("/admin/data_management/project?identifiers[]=#{list.first.id}&identifiers[]=#{list.last.id}", - admin, - admin_mode: true) + before do + # The VirtualRegistries::Packages::Maven::Cache::Entry model is not in the allowed list yet. + # This is why we need to force the ModelMapper to return the stubbed class instead of the model + # passed as parameters. + allow(Gitlab::Geo::ModelMapper).to receive(:find_from_name) + .with('snippet_repository') + .and_return(orderable_klass) + end - expect(response).to have_gitlab_http_status(:ok) - expect(json_response.pluck('record_identifier')).to eq([list.first.id, list.last.id]) - expect(json_response.size).to eq(2) - end + it 'filters passed ids' do + get api("#{api_path}?identifiers[]=#{ids_list.first}&identifiers[]=#{ids_list.last}", + admin, + admin_mode: true) + + expect(response).to have_gitlab_http_status(:ok) + expect(json_response.pluck('record_identifier')).to eq([ids_list.first, ids_list.last]) + expect(json_response.size).to eq(2) end - context 'with composite ids' do - let_it_be(:list) { create_list(:virtual_registries_packages_maven_cache_entry, 3) } - # We're using this Entry model because it will be the first model with composite PKs supported by Geo. - # The model isn't Geo-ready yet, so we need to mock its interface in this test to simulate its future - # implementation. - let_it_be(:orderable_klass) do - Class.new(list.first.class) do - include Orderable - end - end + it 'paginates results' do + get api("#{api_path}?per_page=5", admin, admin_mode: true) - let_it_be(:ids_list) do - list.map do |model| - Base64.urlsafe_encode64(orderable_klass - .primary_key - .map { |field| model.read_attribute_before_type_cast(field) } - .join(' ')) - end - end + expect(response).to have_gitlab_http_status(:ok) + expect(json_response.size).to eq(5) + expect(response.headers['X-Next-Page']).to eq('2') + expect(response.headers['X-Page']).to eq('1') + expect(response.headers['X-Per-Page']).to eq('5') - before do - # The VirtualRegistries::Packages::Maven::Cache::Entry model is not in the allowed list yet. - # This is why we need to force the ModelMapper to return the stubbed class instead of the model - # passed as parameters. - allow(Gitlab::Geo::ModelMapper).to receive(:find_from_name).with('project').and_return(orderable_klass) - end + get api("#{api_path}?per_page=5&page=2", admin, admin_mode: true) + + expect(response).to have_gitlab_http_status(:ok) + expect(json_response.size).to eq(4) # Remaining 4 records + expect(response.headers['X-Page']).to eq('2') + expect(response.headers['X-Next-Page']).to be_empty + end + + it 'returns first page with cursor to next page' do + get api(api_path, admin, admin_mode: true), params: { pagination: 'keyset', per_page: 5 } + + expect(response).to have_gitlab_http_status(:ok) + expect(json_response.size).to eq(5) + + expect(json_response.pluck('record_identifier')).to match_array(ids_list.first(5)) + expect(response.headers["Link"]).to include("cursor") + next_cursor = response.headers["Link"].match("(?cursor=.*?)&")["cursor_data"] + + get api(api_path, admin, admin_mode: true), + params: { pagination: 'keyset', per_page: 5 }.merge(Rack::Utils.parse_query(next_cursor)) + + expect(json_response.size).to eq(4) + expect(json_response.pluck('record_identifier')).to match_array(ids_list.last(4)) + expect(response.headers).not_to include("Link") + end + + it 'orders descending results correctly' do + get api(api_path, admin, admin_mode: true), params: { pagination: 'keyset', per_page: 9, sort: 'desc' } + + expect(response).to have_gitlab_http_status(:ok) + expect(json_response.size).to eq(9) + expect(json_response.pluck('record_identifier')).to eq(ids_list.reverse) + end + + it 'orders ascending results correctly' do + get api(api_path, admin, admin_mode: true), params: { pagination: 'keyset', per_page: 9, sort: 'asc' } + + expect(response).to have_gitlab_http_status(:ok) + expect(json_response.size).to eq(9) + expect(json_response.pluck('record_identifier')).to eq(ids_list) + end + end + + context 'with filtering based on ids' do + context 'with integer ids' do + let_it_be(:list) { create_list(:project, 3) } it 'filters passed ids' do - get api("/admin/data_management/project?identifiers[]=#{ids_list.first}&identifiers[]=#{ids_list.last}", + get api("/admin/data_management/project?identifiers[]=#{list.first.id}&identifiers[]=#{list.last.id}", admin, admin_mode: true) expect(response).to have_gitlab_http_status(:ok) - expect(json_response.pluck('record_identifier')).to eq([ids_list.first, ids_list.last]) + expect(json_response.pluck('record_identifier')).to eq([list.first.id, list.last.id]) expect(json_response.size).to eq(2) end end @@ -824,9 +833,10 @@ def build_record_for_given_state(state) describe 'PUT /admin/data_management/:model_name/checksum' do let(:list) { create_list(factory_name(model_classes), 3, :verification_failed) } + let(:ids_list) { list.map { |model| record_identifier(model) } } it 'handles parameters appropriately' do - query_params = "identifiers[]=#{list.first.id}&identifiers[]=#{list.last.id}&checksum_state=failed" + query_params = "identifiers[]=#{ids_list.first}&identifiers[]=#{ids_list.last}&checksum_state=failed" expected_ids = [list.first.verification_state_object.id, list.last.verification_state_object.id] expected_params = { checksum_state: "verification_failed", identifiers: expected_ids } @@ -862,14 +872,24 @@ def build_record_for_given_state(state) end describe 'GET /admin/data_management/:model_name/:record_identifier' do + let(:identifier) { record_identifier(expected_record) } + it 'handles all known replicable model names' do - get api("#{api_path}/#{expected_record.id}", admin, admin_mode: true) + get api("#{api_path}/#{identifier}", admin, admin_mode: true) expect(response).to have_gitlab_http_status(:ok) - expect(json_response).to include('record_identifier' => expected_record.id, + expect(json_response).to include('record_identifier' => identifier, 'model_class' => expected_record.class.name) end end end end + + def record_identifier(model_record) + return model_record.id unless model_record.class.primary_key.is_a?(Array) + + Base64.urlsafe_encode64(model_record.class.primary_key + .map { |field| model_record.read_attribute_before_type_cast(field) } + .join(' ')) + end end -- GitLab From 38b071eb16d3869f46e8b831a1f1a20ac93723b5 Mon Sep 17 00:00:00 2001 From: c_fons Date: Thu, 6 Nov 2025 18:05:13 +0100 Subject: [PATCH 4/9] Remove commented code --- ee/lib/api/admin/data_management.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/ee/lib/api/admin/data_management.rb b/ee/lib/api/admin/data_management.rb index 4076533e4fde25..cd0372a5e9d849 100644 --- a/ee/lib/api/admin/data_management.rb +++ b/ee/lib/api/admin/data_management.rb @@ -11,7 +11,6 @@ class DataManagement < ::API::Base AVAILABLE_MODEL_NAMES = Gitlab::Geo::ModelMapper.available_model_names.freeze VERIFICATION_STATES = %w[pending started succeeded failed disabled].freeze - # AVAILABLE_MODEL_ID_FIELDS = Gitlab::Geo::ModelMapper.available_models.flat_map(&:primary_key).uniq.freeze before do authenticated_as_admin! -- GitLab From 13042d60f968b87233255fd5d1ddd52a3e4e725b Mon Sep 17 00:00:00 2001 From: c_fons Date: Fri, 7 Nov 2025 11:28:58 +0100 Subject: [PATCH 5/9] Re-order tests for more clarity --- .../api/admin/data_management_spec.rb | 76 ++++++++++--------- 1 file changed, 40 insertions(+), 36 deletions(-) diff --git a/ee/spec/requests/api/admin/data_management_spec.rb b/ee/spec/requests/api/admin/data_management_spec.rb index b2f77c145d7d5a..c5c19164f2afb9 100644 --- a/ee/spec/requests/api/admin/data_management_spec.rb +++ b/ee/spec/requests/api/admin/data_management_spec.rb @@ -129,55 +129,59 @@ expect(json_response.size).to eq(2) end - it 'paginates results' do - get api("#{api_path}?per_page=5", admin, admin_mode: true) + context 'with offset pagination' do + it 'paginates results' do + get api("#{api_path}?per_page=5", admin, admin_mode: true) - expect(response).to have_gitlab_http_status(:ok) - expect(json_response.size).to eq(5) - expect(response.headers['X-Next-Page']).to eq('2') - expect(response.headers['X-Page']).to eq('1') - expect(response.headers['X-Per-Page']).to eq('5') + expect(response).to have_gitlab_http_status(:ok) + expect(json_response.size).to eq(5) + expect(response.headers['X-Next-Page']).to eq('2') + expect(response.headers['X-Page']).to eq('1') + expect(response.headers['X-Per-Page']).to eq('5') - get api("#{api_path}?per_page=5&page=2", admin, admin_mode: true) + get api("#{api_path}?per_page=5&page=2", admin, admin_mode: true) - expect(response).to have_gitlab_http_status(:ok) - expect(json_response.size).to eq(4) # Remaining 4 records - expect(response.headers['X-Page']).to eq('2') - expect(response.headers['X-Next-Page']).to be_empty + expect(response).to have_gitlab_http_status(:ok) + expect(json_response.size).to eq(4) # Remaining 4 records + expect(response.headers['X-Page']).to eq('2') + expect(response.headers['X-Next-Page']).to be_empty + end end - it 'returns first page with cursor to next page' do - get api(api_path, admin, admin_mode: true), params: { pagination: 'keyset', per_page: 5 } + context 'with keyset pagination and ordering' do + it 'returns first page with cursor to next page' do + get api(api_path, admin, admin_mode: true), params: { pagination: 'keyset', per_page: 5 } - expect(response).to have_gitlab_http_status(:ok) - expect(json_response.size).to eq(5) + expect(response).to have_gitlab_http_status(:ok) + expect(json_response.size).to eq(5) - expect(json_response.pluck('record_identifier')).to match_array(ids_list.first(5)) - expect(response.headers["Link"]).to include("cursor") - next_cursor = response.headers["Link"].match("(?cursor=.*?)&")["cursor_data"] + expect(json_response.pluck('record_identifier')).to match_array(ids_list.first(5)) + expect(response.headers["Link"]).to include("cursor") + next_cursor = response.headers["Link"].match("(?cursor=.*?)&")["cursor_data"] - get api(api_path, admin, admin_mode: true), - params: { pagination: 'keyset', per_page: 5 }.merge(Rack::Utils.parse_query(next_cursor)) + get api(api_path, admin, admin_mode: true), + params: { pagination: 'keyset', per_page: 5 }.merge(Rack::Utils.parse_query(next_cursor)) - expect(json_response.size).to eq(4) - expect(json_response.pluck('record_identifier')).to match_array(ids_list.last(4)) - expect(response.headers).not_to include("Link") - end + expect(json_response.size).to eq(4) + expect(json_response.pluck('record_identifier')).to match_array(ids_list.last(4)) + expect(response.headers).not_to include("Link") + end - it 'orders descending results correctly' do - get api(api_path, admin, admin_mode: true), params: { pagination: 'keyset', per_page: 9, sort: 'desc' } + it 'orders descending results correctly' do + get api(api_path, admin, admin_mode: true), params: { pagination: 'keyset', per_page: 9, sort: 'desc' } - expect(response).to have_gitlab_http_status(:ok) - expect(json_response.size).to eq(9) - expect(json_response.pluck('record_identifier')).to eq(ids_list.reverse) - end + expect(response).to have_gitlab_http_status(:ok) + expect(json_response.size).to eq(9) + expect(json_response.pluck('record_identifier')).to eq(ids_list.reverse) + end - it 'orders ascending results correctly' do - get api(api_path, admin, admin_mode: true), params: { pagination: 'keyset', per_page: 9, sort: 'asc' } + it 'orders ascending results correctly' do + get api(api_path, admin, admin_mode: true), params: { pagination: 'keyset', per_page: 9, sort: 'asc' } - expect(response).to have_gitlab_http_status(:ok) - expect(json_response.size).to eq(9) - expect(json_response.pluck('record_identifier')).to eq(ids_list) + expect(response).to have_gitlab_http_status(:ok) + expect(json_response.size).to eq(9) + expect(json_response.pluck('record_identifier')).to eq(ids_list) + end end end -- GitLab From 427ae58b47a609d72140ca0da70ba4f007b140e9 Mon Sep 17 00:00:00 2001 From: Chloe Fons Date: Fri, 7 Nov 2025 10:30:41 +0000 Subject: [PATCH 6/9] Apply 2 suggestion(s) to 1 file(s) Co-authored-by: DANGER_GITLAB_API_TOKEN --- ee/spec/requests/api/admin/data_management_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ee/spec/requests/api/admin/data_management_spec.rb b/ee/spec/requests/api/admin/data_management_spec.rb index c5c19164f2afb9..ee741f01d7070a 100644 --- a/ee/spec/requests/api/admin/data_management_spec.rb +++ b/ee/spec/requests/api/admin/data_management_spec.rb @@ -125,7 +125,7 @@ admin_mode: true) expect(response).to have_gitlab_http_status(:ok) - expect(json_response.pluck('record_identifier')).to eq([ids_list.first, ids_list.last]) + expect(json_response.pluck('record_identifier')).to match_array([ids_list.first, ids_list.last]) expect(json_response.size).to eq(2) end @@ -195,7 +195,7 @@ admin_mode: true) expect(response).to have_gitlab_http_status(:ok) - expect(json_response.pluck('record_identifier')).to eq([list.first.id, list.last.id]) + expect(json_response.pluck('record_identifier')).to match_array([list.first.id, list.last.id]) expect(json_response.size).to eq(2) end end -- GitLab From 55a0acdea38b4ffdce6bca921a63d3db9abb0a7c Mon Sep 17 00:00:00 2001 From: c_fons Date: Fri, 7 Nov 2025 11:57:48 +0100 Subject: [PATCH 7/9] Add line about supporting keyset pagination --- doc/api/admin/data_management.md | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/doc/api/admin/data_management.md b/doc/api/admin/data_management.md index f33556179263e3..165b135c94863a 100644 --- a/doc/api/admin/data_management.md +++ b/doc/api/admin/data_management.md @@ -62,11 +62,13 @@ The `:model_name` parameter must be one of: Supported attributes: -| Attribute | Type | Required | Description | -|-------------------|--------|----------|-----------------------------------------------------------------------------------------------------------------------------| -| `model_name` | string | Yes | The name of the requested model. Must belong to the `:model_name` list above. | -| `checksum_state` | string | No | Search by checksum status. Allowed values: pending, started, succeeded, failed, disabled. | -| `identifiers` | array | No | Filter results with an array of unique identifiers of the requested model, which can be integers or base64 encoded strings. | +| Attribute | Type | Required | Description | +|------------------|--------|----------|-----------------------------------------------------------------------------------------------------------------------------| +| `model_name` | string | Yes | The name of the requested model. Must belong to the `:model_name` list above. | +| `checksum_state` | string | No | Search by checksum status. Allowed values: pending, started, succeeded, failed, disabled. | +| `identifiers` | array | No | Filter results with an array of unique identifiers of the requested model, which can be integers or base64 encoded strings. | + +This endpoint supports [keyset pagination](../rest/_index.md#keyset-based-pagination) on the model's primary key, with sorting in ascending or descending order. If successful, returns [`200`](../rest/troubleshooting.md#status-codes) and information about the model. It includes the following response attributes: -- GitLab From c168a86428f44f2dd1a9fb98113a9e1a41fab71e Mon Sep 17 00:00:00 2001 From: c_fons Date: Wed, 12 Nov 2025 12:30:08 +0100 Subject: [PATCH 8/9] Add tests for keyset ordering --- ee/spec/models/concerns/orderable_spec.rb | 38 ++++++++++++++++++----- 1 file changed, 30 insertions(+), 8 deletions(-) diff --git a/ee/spec/models/concerns/orderable_spec.rb b/ee/spec/models/concerns/orderable_spec.rb index c2a25d028ecfbc..f0b5f937033686 100644 --- a/ee/spec/models/concerns/orderable_spec.rb +++ b/ee/spec/models/concerns/orderable_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Orderable, feature_category: :shared do +RSpec.describe Orderable, feature_category: :api do let_it_be(:dummy_model_class) do Class.new(ActiveRecord::Base) do self.table_name = '_test_dummy_models' @@ -30,11 +30,21 @@ end describe '.keyset_order_by_primary_key' do - it 'orders by primary key in ascending order' do - result = dummy_model_class.keyset_order_by_primary_key + shared_examples 'orders by primary key with given order' do |order| + it 'orders by primary key' do + result = if order + dummy_model_class.keyset_order_by_primary_key(order) + else + dummy_model_class.keyset_order_by_primary_key + end - expect(result.to_sql).to include("ORDER BY \"_test_dummy_models\".\"id\" ASC") + expect(result.to_sql).to include("ORDER BY \"_test_dummy_models\".\"id\" #{order&.upcase || 'ASC'}") + end end + + it_behaves_like 'orders by primary key with given order' + it_behaves_like 'orders by primary key with given order', 'asc' + it_behaves_like 'orders by primary key with given order', 'desc' end end @@ -60,13 +70,25 @@ end describe '.keyset_order_by_primary_key' do - it 'orders by primary key in ascending order' do - expected_sql = "ORDER BY \"_test_dummy_models\".\"id\" ASC, \"_test_dummy_models\".\"partition_id\" ASC" + shared_examples 'orders by primary key with given order' do |order| + it 'orders by primary key' do + expected_order = order&.upcase || 'ASC' + first_column_sql = "ORDER BY \"_test_dummy_models\".\"id\" #{expected_order}," + second_column_sql = " \"_test_dummy_models\".\"partition_id\" #{expected_order}" - result = dummy_model_class.keyset_order_by_primary_key + result = if order + dummy_model_class.keyset_order_by_primary_key(order) + else + dummy_model_class.keyset_order_by_primary_key + end - expect(result.to_sql).to include(expected_sql) + expect(result.to_sql).to include(first_column_sql + second_column_sql) + end end + + it_behaves_like 'orders by primary key with given order' + it_behaves_like 'orders by primary key with given order', 'asc' + it_behaves_like 'orders by primary key with given order', 'desc' end end end -- GitLab From 9fe93a69e3d3c83554f36fe7492e49b0f2555a04 Mon Sep 17 00:00:00 2001 From: c_fons Date: Fri, 14 Nov 2025 19:00:41 +0000 Subject: [PATCH 9/9] Add a param to paginate_with_strategies The param allows to force the use of non-cursor based keyset pagination when false. This allows the project API to continue to rely on non-cursor based keyset pagination while ensuring all other models can use cursor based pagination instead --- lib/api/helpers/pagination_strategies.rb | 12 ++-- lib/api/projects.rb | 2 +- .../api/helpers/pagination_strategies_spec.rb | 65 ++++++++++++++++++- 3 files changed, 70 insertions(+), 9 deletions(-) diff --git a/lib/api/helpers/pagination_strategies.rb b/lib/api/helpers/pagination_strategies.rb index 4353ba0e99adfe..fad34101aa6270 100644 --- a/lib/api/helpers/pagination_strategies.rb +++ b/lib/api/helpers/pagination_strategies.rb @@ -4,8 +4,8 @@ module API module Helpers module PaginationStrategies # paginator_params are only currently supported with offset pagination - def paginate_with_strategies(relation, request_scope = nil, paginator_params: {}) - paginator = paginator(relation, request_scope) + def paginate_with_strategies(relation, request_scope = nil, paginator_params: {}, use_cursor: true) + paginator = paginator(relation, request_scope, use_cursor) result = if block_given? yield(paginator.paginate(relation, **paginator_params)) @@ -18,16 +18,16 @@ def paginate_with_strategies(relation, request_scope = nil, paginator_params: {} end end - def paginator(relation, request_scope = nil) - return keyset_paginator(relation) if keyset_pagination_enabled? + def paginator(relation, request_scope = nil, use_cursor = true) + return keyset_paginator(relation, use_cursor) if keyset_pagination_enabled? offset_paginator(relation, request_scope) end private - def keyset_paginator(relation) - if cursor_based_keyset_pagination_supported?(relation) + def keyset_paginator(relation, use_cursor) + if cursor_based_keyset_pagination_supported?(relation) && use_cursor request_context_class = Gitlab::Pagination::Keyset::CursorBasedRequestContext paginator_class = Gitlab::Pagination::Keyset::CursorPager availability_checker = Gitlab::Pagination::CursorBasedKeyset diff --git a/lib/api/projects.rb b/lib/api/projects.rb index 6270eda286cc81..d209c42013d732 100644 --- a/lib/api/projects.rb +++ b/lib/api/projects.rb @@ -214,7 +214,7 @@ def present_projects(projects, options = {}) projects = reorder_projects(projects) unless order_by_similarity?(allow_unauthorized: false) projects = apply_filters(projects) - records, options = paginate_with_strategies(projects, options[:request_scope]) do |projects| + records, options = paginate_with_strategies(projects, options[:request_scope], use_project_cursor: false) do |projects| projects, options = with_custom_attributes(projects, options) options = options.reverse_merge( diff --git a/spec/lib/api/helpers/pagination_strategies_spec.rb b/spec/lib/api/helpers/pagination_strategies_spec.rb index 7ba185e13feee8..1e5189e8b69219 100644 --- a/spec/lib/api/helpers/pagination_strategies_spec.rb +++ b/spec/lib/api/helpers/pagination_strategies_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe API::Helpers::PaginationStrategies do +RSpec.describe API::Helpers::PaginationStrategies, feature_category: :api do subject { Class.new.include(described_class).new } let(:expected_result) { double("result") } @@ -17,7 +17,7 @@ let(:paginator) { double("paginator", paginate: expected_result, finalize: nil) } before do - allow(subject).to receive(:paginator).with(relation, nil).and_return(paginator) + allow(subject).to receive(:paginator).with(relation, nil, true).and_return(paginator) end it 'yields paginated relation' do @@ -174,6 +174,67 @@ subject.paginator(relation) end end + + context 'when cursor based keyset pagination is available' do + before do + allow(subject).to receive(:cursor_based_keyset_pagination_supported?).and_return(true) + end + + context 'when use_cursor is true - default' do + before do + allow(Gitlab::Pagination::CursorBasedKeyset).to receive(:available?).and_return(true) + allow(Gitlab::Pagination::Keyset::CursorBasedRequestContext).to receive(:new) + .with(subject) + .and_return(request_context) + allow(Gitlab::Pagination::Keyset::CursorPager).to receive(:new).with(request_context).and_return(pager) + end + + it 'delegates to CursorPager' do + expect(subject.paginator(relation)).to eq(pager) + end + end + + context 'when use_cursor is false' do + before do + allow(Gitlab::Pagination::Keyset).to receive(:available?).and_return(true) + allow(Gitlab::Pagination::Keyset::Pager).to receive(:new).with(request_context).and_return(pager) + end + + it 'delegates to Pager' do + expect(subject.paginator(relation, nil, false)).to eq(pager) + end + end + end + + context 'when cursor based keyset pagination is not supported' do + before do + allow(subject).to receive(:cursor_based_keyset_pagination_supported?).and_return(false) + allow(Gitlab::Pagination::Keyset).to receive(:available?).and_return(true) + allow(Gitlab::Pagination::Keyset::Pager).to receive(:new).with(request_context).and_return(pager) + end + + it 'delegate to keyset Pager' do + expect(subject.paginator(relation)).to eq(pager) + end + end + + context 'when cursor based keyset pagination is not available' do + before do + allow(subject).to receive(:cursor_based_keyset_pagination_supported?).and_return(true) + allow(Gitlab::Pagination::CursorBasedKeyset).to receive(:available?) + .with(request_context, relation) + .and_return(false) + allow(Gitlab::Pagination::Keyset::CursorBasedRequestContext).to receive(:new) + .with(subject) + .and_return(request_context) + end + + it 'renders a 501 error' do + expect(subject).to receive(:error!).with(/not yet available/, 405) + + subject.paginator(relation) + end + end end end end -- GitLab