diff --git a/doc/api/audit_events.md b/doc/api/audit_events.md index 4ddd851ebdaf2fffb240b984a76c07f5511c85b1..753e01a15aaf5967a566e65c962ec5a4825cb0b9 100644 --- a/doc/api/audit_events.md +++ b/doc/api/audit_events.md @@ -131,7 +131,8 @@ Example response: ## Group Audit Events -> [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/34078) in GitLab 12.5. +> - [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/34078) in GitLab 12.5. +> - [Support for keyset pagination added](https://gitlab.com/gitlab-org/gitlab/-/issues/333968) in GitLab 15.2. The Group Audit Events API allows you to retrieve [group audit events](../administration/audit_events.md#group-events). This API cannot retrieve project audit events. @@ -139,6 +140,10 @@ This API cannot retrieve project audit events. A user with a Owner role (or above) can retrieve group audit events of all users. A user with a Developer or Maintainer role is limited to group audit events based on their individual actions. +This endpoint optionally supports [keyset pagination](index.md#keyset-based-pagination): + +- When requesting consecutive pages of results, we recommend you use keyset pagination. + ### Retrieve all group audit events ```plaintext diff --git a/doc/api/index.md b/doc/api/index.md index 54d45ea7bf49382891c222804c81fc593ac01af3..1f1fe130e1ffb0af1b9ec335e829b53e3e8b7585 100644 --- a/doc/api/index.md +++ b/doc/api/index.md @@ -520,10 +520,11 @@ pagination headers. Keyset-based pagination is supported only for selected resources and ordering options: -| Resource | Options | Availability | -|:-------------------------|:---------------------------------|:----------------------------------------| -| [Projects](projects.md) | `order_by=id` only | Authenticated and unauthenticated users | -| [Groups](groups.md) | `order_by=name`, `sort=asc` only | Unauthenticated users only | +| Resource | Options | Availability | +|:---------------------------------------------------------|:---------------------------------|:------------------------------------------------------------------------------------------------------------| +| [Projects](projects.md) | `order_by=id` only | Authenticated and unauthenticated users | +| [Groups](groups.md) | `order_by=name`, `sort=asc` only | Unauthenticated users only | +| [Group audit events](audit_events.md#group-audit-events) | `order_by=id`, `sort=desc` only | Authenticated users only ([introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/333968) in GitLab 15.2 | ### Pagination response headers diff --git a/ee/lib/ee/api/groups.rb b/ee/lib/ee/api/groups.rb index e7dd2663dd8aef67e57ae7eb40ee2703989de91c..e6c7fb3ae75dc6119a3269fc500347038feca3aa 100644 --- a/ee/lib/ee/api/groups.rb +++ b/ee/lib/ee/api/groups.rb @@ -122,7 +122,7 @@ def delete_group(group) params: audit_event_finder_params ).execute - present paginate(audit_events), with: EE::API::Entities::AuditEvent + present paginate_with_strategies(audit_events), with: EE::API::Entities::AuditEvent end desc 'Get a specific audit event in this group.' do diff --git a/ee/spec/requests/api/audit_events_spec.rb b/ee/spec/requests/api/audit_events_spec.rb index 1ae193e80099f685b14c9ef1da94b71486ea15a9..022fe2964c1cf794f878257c5d3ca88674ac0c9f 100644 --- a/ee/spec/requests/api/audit_events_spec.rb +++ b/ee/spec/requests/api/audit_events_spec.rb @@ -154,6 +154,58 @@ end end end + + context 'keyset pagination' do + let_it_be(:current_user) { create(:admin) } + let_it_be(:group) { create(:group) } + let_it_be(:audit_event_1) { create(:group_audit_event, entity_id: group.id) } + let_it_be(:audit_event_2) { create(:group_audit_event, entity_id: group.id) } + + it 'paginates the records correctly' do + get api("/groups/#{group.id}/audit_events", current_user), params: { pagination: 'keyset', per_page: 1, order_by: 'id' } + + expect(response).to have_gitlab_http_status(:ok) + records = json_response + expect(records.size).to eq(1) + expect(records.first['id']).to eq(audit_event_2.id) + + params_for_next_page = params_for_next_page(response) + expect(params_for_next_page).to include('cursor') + + get api("/groups/#{group.id}/audit_events"), params: params_for_next_page + + expect(response).to have_gitlab_http_status(:ok) + records = Gitlab::Json.parse(response.body) + expect(records.size).to eq(1) + expect(records.first['id']).to eq(audit_event_1.id) + end + + def pagination_links(response) + link = response.headers['LINK'] + return unless link + + link.split(',').map do |link| + match = link.match(/<(?.*)>; rel="(?\w+)"/) + break nil unless match + + { url: match[:url], rel: match[:rel] } + end.compact + end + + def params_for_next_page(response) + next_url = pagination_links(response).find { |link| link[:rel] == 'next' }[:url] + Rack::Utils.parse_query(URI.parse(next_url).query) + end + + context 'on making requests with unsupported ordering structure' do + it 'returns error' do + get api("/groups/#{group.id}/audit_events", current_user), params: { pagination: 'keyset', per_page: 1, order_by: 'created_at', sort: 'asc' } + + expect(response).to have_gitlab_http_status(:method_not_allowed) + expect(json_response['error']).to eq('Keyset pagination is not yet available for this type of request') + end + end + end end describe 'GET /audit_events/:id' do diff --git a/lib/api/helpers/pagination_strategies.rb b/lib/api/helpers/pagination_strategies.rb index 8c2186768eaa9ce0eb55f7bb2602ffec31bc01f3..4e244ea589e3ab6fdf13d1cbdc69934a9a5d721d 100644 --- a/lib/api/helpers/pagination_strategies.rb +++ b/lib/api/helpers/pagination_strategies.rb @@ -49,6 +49,7 @@ def offset_paginator(relation, request_scope) offset_limit = limit_for_scope(request_scope) if (Gitlab::Pagination::Keyset.available_for_type?(relation) || cursor_based_keyset_pagination_supported?(relation)) && + cursor_based_keyset_pagination_enforced?(relation) && offset_limit_exceeded?(offset_limit) return error!("Offset pagination has a maximum allowed offset of #{offset_limit} " \ @@ -63,6 +64,10 @@ def cursor_based_keyset_pagination_supported?(relation) Gitlab::Pagination::CursorBasedKeyset.available_for_type?(relation) end + def cursor_based_keyset_pagination_enforced?(relation) + Gitlab::Pagination::CursorBasedKeyset.enforced_for_type?(relation) + end + def keyset_pagination_enabled? params[:pagination] == 'keyset' end diff --git a/lib/gitlab/pagination/cursor_based_keyset.rb b/lib/gitlab/pagination/cursor_based_keyset.rb index f19cdf06d9a4b2614672c1a843674a2f76234b19..2d9fb0a50fc01312185f894d404cc5632833a554 100644 --- a/lib/gitlab/pagination/cursor_based_keyset.rb +++ b/lib/gitlab/pagination/cursor_based_keyset.rb @@ -4,9 +4,18 @@ module Gitlab module Pagination module CursorBasedKeyset SUPPORTED_ORDERING = { - Group => { name: :asc } + Group => { name: :asc }, + AuditEvent => { id: :desc } }.freeze + # Relation types that are enforced in this list + # enforce the use of keyset pagination, thus erroring out requests + # made with offset pagination above a certain limit. + # + # In many cases this could introduce a breaking change + # so enforcement is optional. + ENFORCED_TYPES = [Group].freeze + def self.available_for_type?(relation) SUPPORTED_ORDERING.key?(relation.klass) end @@ -16,6 +25,10 @@ def self.available?(cursor_based_request_context, relation) order_satisfied?(relation, cursor_based_request_context) end + def self.enforced_for_type?(relation) + ENFORCED_TYPES.include?(relation.klass) + end + def self.order_satisfied?(relation, cursor_based_request_context) order_by_from_request = cursor_based_request_context.order_by diff --git a/spec/lib/api/helpers/pagination_strategies_spec.rb b/spec/lib/api/helpers/pagination_strategies_spec.rb index e8a4243b40725cdbd4b8b632a8f40ce192eecb34..16cc10182b047a33b385ecfd87905faabe8ece87 100644 --- a/spec/lib/api/helpers/pagination_strategies_spec.rb +++ b/spec/lib/api/helpers/pagination_strategies_spec.rb @@ -55,9 +55,10 @@ allow(subject).to receive(:keyset_pagination_enabled?).and_return(false) end - context 'when keyset pagination is available for the relation' do + context 'when keyset pagination is available and enforced for the relation' do before do allow(Gitlab::Pagination::Keyset).to receive(:available_for_type?).and_return(true) + allow(Gitlab::Pagination::CursorBasedKeyset).to receive(:enforced_for_type?).and_return(true) end context 'when a request scope is given' do @@ -70,6 +71,18 @@ subject.paginator(relation, request_scope) end + + context 'when keyset pagination is not enforced' do + before do + allow(Gitlab::Pagination::CursorBasedKeyset).to receive(:enforced_for_type?).and_return(false) + end + + it 'returns no errors' do + expect(subject).not_to receive(:error!) + + subject.paginator(relation, request_scope) + end + end end context 'when the scope limit is not exceeded' do diff --git a/spec/lib/gitlab/pagination/cursor_based_keyset_spec.rb b/spec/lib/gitlab/pagination/cursor_based_keyset_spec.rb index ac2695977c4e18e925041be64c5ac533cce542c0..879c874b134d7146ddd97c28cb13426acc6411f5 100644 --- a/spec/lib/gitlab/pagination/cursor_based_keyset_spec.rb +++ b/spec/lib/gitlab/pagination/cursor_based_keyset_spec.rb @@ -15,6 +15,22 @@ end end + describe '.enforced_for_type?' do + subject { described_class.enforced_for_type?(relation) } + + context 'when relation is Group' do + let(:relation) { Group.all } + + it { is_expected.to be true } + end + + context 'when relation is AuditEvent' do + let(:relation) { AuditEvent.all } + + it { is_expected.to be false } + end + end + describe '.available?' do let(:request_context) { double('request_context', params: { order_by: order_by, sort: sort }) } let(:cursor_based_request_context) { Gitlab::Pagination::Keyset::CursorBasedRequestContext.new(request_context) }