From a2baaf29b851185d1bacbcb00428e705a4021657 Mon Sep 17 00:00:00 2001 From: Max Woolf Date: Tue, 5 Jul 2022 14:44:53 +0100 Subject: [PATCH 1/3] Add group audit event keyset pagination Adds optional support for keyset pagination in group audit event REST API. EE: true Changelog: added --- doc/api/audit_events.md | 7 ++- doc/api/index.md | 9 ++-- ee/lib/ee/api/groups.rb | 2 +- ee/spec/requests/api/audit_events_spec.rb | 52 ++++++++++++++++++++ lib/gitlab/pagination/cursor_based_keyset.rb | 3 +- 5 files changed, 66 insertions(+), 7 deletions(-) diff --git a/doc/api/audit_events.md b/doc/api/audit_events.md index 4ddd851ebdaf2f..753e01a15aaf59 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 54d45ea7bf4938..1457a45dd69eaa 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=created_at`, `sort=desc` only | Authenticated users only | ### Pagination response headers diff --git a/ee/lib/ee/api/groups.rb b/ee/lib/ee/api/groups.rb index e7dd2663dd8aef..e6c7fb3ae75dc6 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 1ae193e80099f6..a6472cdc65d228 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 'pagination strategies' 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: 'created_at' } + + 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/gitlab/pagination/cursor_based_keyset.rb b/lib/gitlab/pagination/cursor_based_keyset.rb index f19cdf06d9a4b2..09837950554917 100644 --- a/lib/gitlab/pagination/cursor_based_keyset.rb +++ b/lib/gitlab/pagination/cursor_based_keyset.rb @@ -4,7 +4,8 @@ module Gitlab module Pagination module CursorBasedKeyset SUPPORTED_ORDERING = { - Group => { name: :asc } + Group => { name: :asc }, + AuditEvent => { created_at: :desc } }.freeze def self.available_for_type?(relation) -- GitLab From e51a90ffc2a80e7a9ab677898fcc3fe4d511335a Mon Sep 17 00:00:00 2001 From: Max Woolf Date: Thu, 7 Jul 2022 15:24:12 +0100 Subject: [PATCH 2/3] Backend reviewer suggestions --- doc/api/index.md | 10 +++++----- ee/spec/requests/api/audit_events_spec.rb | 2 +- lib/api/helpers/pagination_strategies.rb | 5 +++++ lib/gitlab/pagination/cursor_based_keyset.rb | 14 +++++++++++++- .../api/helpers/pagination_strategies_spec.rb | 15 ++++++++++++++- .../pagination/cursor_based_keyset_spec.rb | 16 ++++++++++++++++ 6 files changed, 54 insertions(+), 8 deletions(-) diff --git a/doc/api/index.md b/doc/api/index.md index 1457a45dd69eaa..1f1fe130e1ffb0 100644 --- a/doc/api/index.md +++ b/doc/api/index.md @@ -520,11 +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 | -| [Group audit events](audit_events.md#group-audit-events) | `order_by=created_at`, `sort=desc` only | Authenticated 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/spec/requests/api/audit_events_spec.rb b/ee/spec/requests/api/audit_events_spec.rb index a6472cdc65d228..9361bcae1f16a2 100644 --- a/ee/spec/requests/api/audit_events_spec.rb +++ b/ee/spec/requests/api/audit_events_spec.rb @@ -162,7 +162,7 @@ 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: 'created_at' } + 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 diff --git a/lib/api/helpers/pagination_strategies.rb b/lib/api/helpers/pagination_strategies.rb index 8c2186768eaa9c..4e244ea589e3ab 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 09837950554917..611c2c4f1f3e08 100644 --- a/lib/gitlab/pagination/cursor_based_keyset.rb +++ b/lib/gitlab/pagination/cursor_based_keyset.rb @@ -5,9 +5,17 @@ module Pagination module CursorBasedKeyset SUPPORTED_ORDERING = { Group => { name: :asc }, - AuditEvent => { created_at: :desc } + AuditEvent => { id: :desc } }.freeze + # Relation types that are enforced in this list + # enforce the use of keyset pagination above a limit + # that could cause issues with offset pagination. + # + # 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 @@ -17,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 e8a4243b40725c..16cc10182b047a 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 ac2695977c4e18..879c874b134d71 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) } -- GitLab From 8b053eac64e2cd98a3acc5062128acbcda7089dd Mon Sep 17 00:00:00 2001 From: Manoj M J Date: Fri, 8 Jul 2022 12:25:08 +0000 Subject: [PATCH 3/3] Apply 2 suggestion(s) to 2 file(s) --- ee/spec/requests/api/audit_events_spec.rb | 2 +- lib/gitlab/pagination/cursor_based_keyset.rb | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/ee/spec/requests/api/audit_events_spec.rb b/ee/spec/requests/api/audit_events_spec.rb index 9361bcae1f16a2..022fe2964c1cf7 100644 --- a/ee/spec/requests/api/audit_events_spec.rb +++ b/ee/spec/requests/api/audit_events_spec.rb @@ -155,7 +155,7 @@ end end - context 'pagination strategies' do + 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) } diff --git a/lib/gitlab/pagination/cursor_based_keyset.rb b/lib/gitlab/pagination/cursor_based_keyset.rb index 611c2c4f1f3e08..2d9fb0a50fc013 100644 --- a/lib/gitlab/pagination/cursor_based_keyset.rb +++ b/lib/gitlab/pagination/cursor_based_keyset.rb @@ -9,8 +9,8 @@ module CursorBasedKeyset }.freeze # Relation types that are enforced in this list - # enforce the use of keyset pagination above a limit - # that could cause issues with offset pagination. + # 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. -- GitLab