Merge branch 'mmj-group-api-keyset-pagination' into 'master'

Introduce Keyset pagination for `GET /groups` API endpoint for unauthenticated users.

See merge request gitlab-org/gitlab!68346

config/feature_flags/development/keyset_pagination_for_groups_api.yml

+---
+name: keyset_pagination_for_groups_api
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/68346
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/339831
+milestone: '14.3'
+type: development
+group: group::access
+default_enabled: false

db/migrate/20210901065504_add_index_on_name_and_id_to_public_groups.rb

+# frozen_string_literal: true
+
+class AddIndexOnNameAndIdToPublicGroups < Gitlab::Database::Migration[1.0]
+  INDEX_NAME = 'index_namespaces_public_groups_name_id'
+  PUBLIC_VISIBILITY_LEVEL = 20
+
+  disable_ddl_transaction!
+
+  def up
+    add_concurrent_index :namespaces, [:name, :id], name: INDEX_NAME,
+      where: "type = 'Group' AND visibility_level = #{PUBLIC_VISIBILITY_LEVEL}"
+  end
+
+  def down
+    remove_concurrent_index_by_name :namespaces, INDEX_NAME
+  end
+end

db/schema_migrations/20210901065504

+9724a5fc1703418f9b1ea1d5375fc3b01834b30e5ff16c60537db5cb00bc210a
\ No newline at end of file

db/structure.sql

@@ -25691,6 +25691,8 @@ CREATE INDEX index_namespaces_on_traversal_ids ON namespaces USING gin (traversa
 
 CREATE INDEX index_namespaces_on_type_and_id_partial ON namespaces USING btree (type, id) WHERE (type IS NOT NULL);
 
+CREATE INDEX index_namespaces_public_groups_name_id ON namespaces USING btree (name, id) WHERE (((type)::text = 'Group'::text) AND (visibility_level = 20));
+
 CREATE INDEX index_non_requested_project_members_on_source_id_and_type ON members USING btree (source_id, source_type) WHERE ((requested_at IS NULL) AND ((type)::text = 'ProjectMember'::text));
 
 CREATE UNIQUE INDEX index_note_diff_files_on_diff_note_id ON note_diff_files USING btree (diff_note_id);

doc/api/groups.md

@@ -13,6 +13,11 @@ authentication, only public groups are returned.
 
 By default, this request returns 20 results at a time because the API results [are paginated](index.md#pagination).
 
+When accessed without authentication, this endpoint also supports [keyset pagination](index.md#keyset-based-pagination):
+
+- When requesting consecutive pages of results, we recommend you use keyset pagination.
+- Beyond a specific offset limit (specified by [max offset allowed by the REST API for offset-based pagination](../administration/instance_limits.md#max-offset-allowed-by-the-rest-api-for-offset-based-pagination)), offset pagination is unavailable.
+
 Parameters:
 
 | Attribute                | Type              | Required | Description |

doc/api/index.md

@@ -462,22 +462,43 @@ The response header includes a link to the next page. For example:
 ```http
 HTTP/1.1 200 OK
 ...
-Links: <https://gitlab.example.com/api/v4/projects?pagination=keyset&per_page=50&order_by=id&sort=asc&id_after=42>; rel="next"
 Link: <https://gitlab.example.com/api/v4/projects?pagination=keyset&per_page=50&order_by=id&sort=asc&id_after=42>; rel="next"
 Status: 200 OK
 ...
 ```
 
+The link to the next page contains an additional filter `id_after=42` that
+excludes already-retrieved records.
+
+As another example, the following request lists 50 [groups](groups.md) per page ordered
+by `name` ascending using keyset pagination:
+
+```shell
+curl --request GET --header "PRIVATE-TOKEN: <your_access_token>" "https://gitlab.example.com/api/v4/groups?pagination=keyset&per_page=50&order_by=name&sort=asc"
+```
+
+The response header includes a link to the next page:
+
+```http
+HTTP/1.1 200 OK
+...
+Link: <https://gitlab.example.com/api/v4/groups?pagination=keyset&per_page=50&order_by=name&sort=asc&cursor=eyJuYW1lIjoiRmxpZ2h0anMiLCJpZCI6IjI2IiwiX2tkIjoibiJ9>; rel="next"
+Status: 200 OK
+...
+```
+
+The link to the next page contains an additional filter `cursor=eyJuYW1lIjoiRmxpZ2h0anMiLCJpZCI6IjI2IiwiX2tkIjoibiJ9` that
+excludes already-retrieved records.
+
+The type of filter depends on the
+`order_by` option used, and we can have more than one additional filter.
+
 WARNING:
-The `Links` header is scheduled to be removed in GitLab 14.0 to be aligned with the
+The `Links` header was removed in GitLab 14.0 to be aligned with the
 [W3C `Link` specification](https://www.w3.org/wiki/LinkHeader). The `Link`
 header was [added in GitLab 13.1](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/33714)
 and should be used instead.
 
-The link to the next page contains an additional filter `id_after=42` that
-excludes already-retrieved records. The type of filter depends on the
-`order_by` option used, and we may have more than one additional filter.
-
 When the end of the collection is reached and there are no additional
 records to retrieve, the `Link` header is absent and the resulting array is
 empty.
@@ -489,9 +510,10 @@ pagination headers.
 Keyset-based pagination is supported only for selected resources and ordering
 options:
 
-| Resource                | Order |
-|-------------------------|-------|
-| [Projects](projects.md) | `order_by=id` 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              |
 
 ## Path parameters
 

lib/api/groups.rb

@@ -108,6 +108,20 @@ module API
         present paginate(groups), options
       end
 
+      def present_groups_with_pagination_strategies(params, groups)
+        return present_groups(params, groups) if current_user.present? || Feature.disabled?(:keyset_pagination_for_groups_api)
+
+        options = {
+          with: Entities::Group,
+          current_user: nil,
+          statistics: false
+        }
+
+        groups, options = with_custom_attributes(groups, options)
+
+        present paginate_with_strategies(groups), options
+      end
+
       def delete_group(group)
         destroy_conditionally!(group) do |group|
           ::Groups::DestroyService.new(group, current_user).async_execute
@@ -168,7 +182,7 @@ module API
       end
       get do
         groups = find_groups(declared_params(include_missing: false), params[:id])
-        present_groups params, groups
+        present_groups_with_pagination_strategies params, groups
       end
 
       desc 'Create a group. Available only for users who can create groups.' do

lib/api/helpers/pagination_strategies.rb

@@ -3,10 +3,16 @@
 module API
   module Helpers
     module PaginationStrategies
-      def paginate_with_strategies(relation, request_scope)
+      def paginate_with_strategies(relation, request_scope = nil)
         paginator = paginator(relation, request_scope)
 
-        yield(paginator.paginate(relation)).tap do |records, _|
+        result = if block_given?
+                   yield(paginator.paginate(relation))
+                 else
+                   paginator.paginate(relation)
+                 end
+
+        result.tap do |records, _|
           paginator.finalize(records)
         end
       end
@@ -20,17 +26,31 @@ module API
       private
 
       def keyset_paginator(relation)
-        request_context = Gitlab::Pagination::Keyset::RequestContext.new(self)
-        unless Gitlab::Pagination::Keyset.available?(request_context, relation)
+        if cursor_based_keyset_pagination_supported?(relation)
+          request_context_class = Gitlab::Pagination::Keyset::CursorBasedRequestContext
+          paginator_class = Gitlab::Pagination::Keyset::CursorPager
+          availability_checker = Gitlab::Pagination::CursorBasedKeyset
+        else
+          request_context_class = Gitlab::Pagination::Keyset::RequestContext
+          paginator_class = Gitlab::Pagination::Keyset::Pager
+          availability_checker = Gitlab::Pagination::Keyset
+        end
+
+        request_context = request_context_class.new(self)
+
+        unless availability_checker.available?(request_context, relation)
           return error!('Keyset pagination is not yet available for this type of request', 405)
         end
 
-        Gitlab::Pagination::Keyset::Pager.new(request_context)
+        paginator_class.new(request_context)
       end
 
       def offset_paginator(relation, request_scope)
         offset_limit = limit_for_scope(request_scope)
-        if Gitlab::Pagination::Keyset.available_for_type?(relation) && offset_limit_exceeded?(offset_limit)
+        if (Gitlab::Pagination::Keyset.available_for_type?(relation) ||
+            cursor_based_keyset_pagination_supported?(relation)) &&
+            offset_limit_exceeded?(offset_limit)
+
           return error!("Offset pagination has a maximum allowed offset of #{offset_limit} " \
             "for requests that return objects of type #{relation.klass}. " \
             "Remaining records can be retrieved using keyset pagination.", 405)
@@ -39,6 +59,10 @@ module API
         Gitlab::Pagination::OffsetPagination.new(self)
       end
 
+      def cursor_based_keyset_pagination_supported?(relation)
+        Gitlab::Pagination::CursorBasedKeyset.available_for_type?(relation)
+      end
+
       def keyset_pagination_enabled?
         params[:pagination] == 'keyset'
       end

lib/gitlab/pagination/cursor_based_keyset.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Pagination
+    module CursorBasedKeyset
+      SUPPORTED_ORDERING = {
+        Group => { name: :asc }
+      }.freeze
+
+      def self.available_for_type?(relation)
+        SUPPORTED_ORDERING.key?(relation.klass)
+      end
+
+      def self.available?(cursor_based_request_context, relation)
+        available_for_type?(relation) &&
+        order_satisfied?(relation, cursor_based_request_context)
+      end
+
+      def self.order_satisfied?(relation, cursor_based_request_context)
+        order_by_from_request = cursor_based_request_context.order_by
+
+        SUPPORTED_ORDERING[relation.klass] == order_by_from_request
+      end
+      private_class_method :order_satisfied?
+    end
+  end
+end

lib/gitlab/pagination/keyset/cursor_based_request_context.rb

@@ -4,11 +4,12 @@ module Gitlab
   module Pagination
     module Keyset
       class CursorBasedRequestContext
-        attr_reader :request
-        delegate :params, :header, to: :request
+        DEFAULT_SORT_DIRECTION = :desc
+        attr_reader :request_context
+        delegate :params, to: :request_context
 
-        def initialize(request)
-          @request = request
+        def initialize(request_context)
+          @request_context = request_context
         end
 
         def per_page
@@ -21,9 +22,13 @@ module Gitlab
 
         def apply_headers(cursor_for_next_page)
           Gitlab::Pagination::Keyset::HeaderBuilder
-            .new(self)
+            .new(request_context)
             .add_next_page_header({ cursor: cursor_for_next_page })
         end
+
+        def order_by
+          { params[:order_by].to_sym => params[:sort]&.to_sym || DEFAULT_SORT_DIRECTION }
+        end
       end
     end
   end

spec/lib/gitlab/pagination/cursor_based_keyset_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::Pagination::CursorBasedKeyset do
+  subject { described_class }
+
+  describe '.available_for_type?' do
+    it 'returns true for Group' do
+      expect(subject.available_for_type?(Group.all)).to be_truthy
+    end
+
+    it 'return false for other types of relations' do
+      expect(subject.available_for_type?(User.all)).to be_falsey
+    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) }
+
+    context 'with order-by name asc' do
+      let(:order_by) { :name }
+      let(:sort) { :asc }
+
+      it 'returns true for Group' do
+        expect(subject.available?(cursor_based_request_context, Group.all)).to be_truthy
+      end
+
+      it 'return false for other types of relations' do
+        expect(subject.available?(cursor_based_request_context, User.all)).to be_falsey
+      end
+    end
+
+    context 'with other order-by columns' do
+      let(:order_by) { :path }
+      let(:sort) { :asc }
+
+      it 'returns false for Group' do
+        expect(subject.available?(cursor_based_request_context, Group.all)).to be_falsey
+      end
+
+      it 'return false for other types of relations' do
+        expect(subject.available?(cursor_based_request_context, User.all)).to be_falsey
+      end
+    end
+  end
+end

spec/lib/gitlab/pagination/keyset/cursor_based_request_context_spec.rb

@@ -3,32 +3,40 @@
 require 'spec_helper'
 
 RSpec.describe Gitlab::Pagination::Keyset::CursorBasedRequestContext do
-  let(:params) { { per_page: 2, cursor: 'eyJuYW1lIjoiR2l0TGFiIEluc3RhbmNlIiwiaWQiOiI1MiIsIl9rZCI6Im4ifQ==' } }
-  let(:request) { double('request', params: params) }
+  let(:params) { { per_page: 2, cursor: 'eyJuYW1lIjoiR2l0TGFiIEluc3RhbmNlIiwiaWQiOiI1MiIsIl9rZCI6Im4ifQ==', order_by: :name, sort: :asc } }
+  let(:request) { double('request', url: 'http://localhost') }
+  let(:request_context) { double('request_context', header: nil, params: params, request: request) }
 
   describe '#per_page' do
-    subject(:per_page) { described_class.new(request).per_page }
+    subject(:per_page) { described_class.new(request_context).per_page }
 
     it { is_expected.to eq 2 }
   end
 
   describe '#cursor' do
-    subject(:cursor) { described_class.new(request).cursor }
+    subject(:cursor) { described_class.new(request_context).cursor }
 
     it { is_expected.to eq 'eyJuYW1lIjoiR2l0TGFiIEluc3RhbmNlIiwiaWQiOiI1MiIsIl9rZCI6Im4ifQ==' }
   end
 
+  describe '#order_by' do
+    subject(:order_by) { described_class.new(request_context).order_by }
+
+    it { is_expected.to eq({ name: :asc }) }
+  end
+
   describe '#apply_headers' do
-    let(:request) { double('request', url: "http://#{Gitlab.config.gitlab.host}/api/v4/projects?per_page=3", params: params) }
+    let(:request) { double('request', url: "http://#{Gitlab.config.gitlab.host}/api/v4/projects?per_page=3") }
     let(:params) { { per_page: 3 } }
+    let(:request_context) { double('request_context', header: nil, params: params, request: request) }
     let(:cursor_for_next_page) { 'eyJuYW1lIjoiSDVicCIsImlkIjoiMjgiLCJfa2QiOiJuIn0=' }
 
-    subject(:apply_headers) { described_class.new(request).apply_headers(cursor_for_next_page) }
+    subject(:apply_headers) { described_class.new(request_context).apply_headers(cursor_for_next_page) }
 
     it 'sets Link header with same host/path as the original request' do
-      orig_uri = URI.parse(request.url)
+      orig_uri = URI.parse(request_context.request.url)
 
-      expect(request).to receive(:header).once do |name, header|
+      expect(request_context).to receive(:header).once do |name, header|
         first_link, _ = /<([^>]+)>; rel="next"/.match(header).captures
 
         uri = URI.parse(first_link)
@@ -42,9 +50,9 @@ RSpec.describe Gitlab::Pagination::Keyset::CursorBasedRequestContext do
     end
 
     it 'sets Link header with a cursor to the next page' do
-      orig_uri = URI.parse(request.url)
+      orig_uri = URI.parse(request_context.request.url)
 
-      expect(request).to receive(:header).once do |name, header|
+      expect(request_context).to receive(:header).once do |name, header|
         first_link, _ = /<([^>]+)>; rel="next"/.match(header).captures
 
         query = CGI.parse(URI.parse(first_link).query)

spec/lib/gitlab/pagination/keyset/cursor_pager_spec.rb

@@ -6,8 +6,8 @@ RSpec.describe Gitlab::Pagination::Keyset::CursorPager do
   let(:relation) { Group.all.order(:name, :id) }
   let(:per_page) { 3 }
   let(:params) { { cursor: nil, per_page: per_page } }
-  let(:request) { double('request', params: params) }
-  let(:cursor_based_request_context) { Gitlab::Pagination::Keyset::CursorBasedRequestContext.new(request) }
+  let(:request_context) { double('request_context', params: params) }
+  let(:cursor_based_request_context) { Gitlab::Pagination::Keyset::CursorBasedRequestContext.new(request_context) }
 
   before_all do
     create_list(:group, 7)
@@ -33,7 +33,7 @@ RSpec.describe Gitlab::Pagination::Keyset::CursorPager do
     it 'passes information about next page to request' do
       cursor_for_next_page = relation.keyset_paginate(**params).cursor_for_next_page
 
-      expect_next_instance_of(Gitlab::Pagination::Keyset::HeaderBuilder, cursor_based_request_context) do |builder|
+      expect_next_instance_of(Gitlab::Pagination::Keyset::HeaderBuilder, request_context) do |builder|
         expect(builder).to receive(:add_next_page_header).with({ cursor: cursor_for_next_page })
       end
 

spec/requests/api/groups_spec.rb

@@ -158,6 +158,127 @@ RSpec.describe API::Groups do
       end
     end
 
+    context 'pagination strategies' do
+      let_it_be(:group_1) { create(:group, name: '1_group') }
+      let_it_be(:group_2) { create(:group, name: '2_group') }
+
+      context 'when the user is anonymous' do
+        context 'offset pagination' do
+          context 'on making requests beyond the allowed offset pagination threshold' do
+            it 'returns error and suggests to use keyset pagination' do
+              get api('/groups'), params: { page: 3000, per_page: 25 }
+
+              expect(response).to have_gitlab_http_status(:method_not_allowed)
+              expect(json_response['error']).to eq(
+                'Offset pagination has a maximum allowed offset of 50000 for requests that return objects of type Group. '\
+                'Remaining records can be retrieved using keyset pagination.'
+              )
+            end
+
+            context 'when the feature flag `keyset_pagination_for_groups_api` is disabled' do
+              before do
+                stub_feature_flags(keyset_pagination_for_groups_api: false)
+              end
+
+              it 'returns successful response' do
+                get api('/groups'), params: { page: 3000, per_page: 25 }
+
+                expect(response).to have_gitlab_http_status(:ok)
+              end
+            end
+          end
+
+          context 'on making requests below the allowed offset pagination threshold' do
+            it 'paginates the records' do
+              get api('/groups'), params: { page: 1, per_page: 1 }
+
+              expect(response).to have_gitlab_http_status(:ok)
+              records = json_response
+              expect(records.size).to eq(1)
+              expect(records.first['id']).to eq(group_1.id)
+
+              # next page
+
+              get api('/groups'), params: { page: 2, per_page: 1 }
+
+              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(group_2.id)
+            end
+          end
+        end
+
+        context 'keyset pagination' do
+          def pagination_links(response)
+            link = response.headers['LINK']
+            return unless link
+
+            link.split(',').map do |link|
+              match = link.match(/<(?<url>.*)>; rel="(?<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 supported ordering structure' do
+            it 'paginates the records correctly' do
+              # first page
+              get api('/groups'), params: { pagination: 'keyset', per_page: 1 }
+
+              expect(response).to have_gitlab_http_status(:ok)
+              records = json_response
+              expect(records.size).to eq(1)
+              expect(records.first['id']).to eq(group_1.id)
+
+              params_for_next_page = params_for_next_page(response)
+              expect(params_for_next_page).to include('cursor')
+
+              get api('/groups'), 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(group_2.id)
+            end
+
+            context 'when the feature flag `keyset_pagination_for_groups_api` is disabled' do
+              before do
+                stub_feature_flags(keyset_pagination_for_groups_api: false)
+              end
+
+              it 'ignores the keyset pagination params and performs offset pagination' do
+                get api('/groups'), params: { pagination: 'keyset', per_page: 1 }
+
+                expect(response).to have_gitlab_http_status(:ok)
+                records = json_response
+                expect(records.size).to eq(1)
+                expect(records.first['id']).to eq(group_1.id)
+
+                params_for_next_page = params_for_next_page(response)
+                expect(params_for_next_page).not_to include('cursor')
+              end
+            end
+          end
+
+          context 'on making requests with unsupported ordering structure' do
+            it 'returns error' do
+              get api('/groups'), params: { pagination: 'keyset', per_page: 1, order_by: 'path', sort: 'desc' }
+
+              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
+    end
+
     context "when authenticated as admin" do
       it "admin: returns an array of all groups" do
         get api("/groups", admin)