From 833eadad8cac85b99871842854c9a676a607e2da Mon Sep 17 00:00:00 2001 From: GitLab Bot Date: Tue, 29 Oct 2019 09:06:10 +0000 Subject: [PATCH] Add latest changes from gitlab-org/gitlab@master --- app/helpers/dashboard_helper.rb | 13 +- app/views/admin/dashboard/index.html.haml | 37 +- ...ble-escaping-in-tableflip-quick-action.yml | 5 + .../unreleased/dz-improve-admin-features.yml | 5 + doc/api/audit_events.md | 2 +- lib/api/helpers/pagination.rb | 249 +---------- lib/gitlab/pagination/base.rb | 32 ++ lib/gitlab/pagination/offset_pagination.rb | 77 ++++ lib/gitlab/quick_actions/issuable_actions.rb | 2 +- lib/gitlab/serializer/pagination.rb | 5 +- locale/gitlab.pot | 10 +- spec/helpers/dashboard_helper_spec.rb | 63 ++- spec/lib/api/helpers/pagination_spec.rb | 401 +----------------- .../pagination/offset_pagination_spec.rb | 215 ++++++++++ 14 files changed, 441 insertions(+), 675 deletions(-) create mode 100644 changelogs/unreleased/24082-double-escaping-in-tableflip-quick-action.yml create mode 100644 changelogs/unreleased/dz-improve-admin-features.yml create mode 100644 lib/gitlab/pagination/base.rb create mode 100644 lib/gitlab/pagination/offset_pagination.rb create mode 100644 spec/lib/gitlab/pagination/offset_pagination_spec.rb diff --git a/app/helpers/dashboard_helper.rb b/app/helpers/dashboard_helper.rb index 518cb7c9714..679622897aa 100644 --- a/app/helpers/dashboard_helper.rb +++ b/app/helpers/dashboard_helper.rb @@ -27,16 +27,25 @@ module DashboardHelper false end - def feature_entry(title, href: nil, enabled: true) + def feature_entry(title, href: nil, enabled: true, doc_href: nil) enabled_text = enabled ? 'on' : 'off' label = "#{title}: status #{enabled_text}" link_or_title = href && enabled ? tag.a(title, href: href) : title tag.p(aria: { label: label }) do concat(link_or_title) + concat(tag.span(class: ['light', 'float-right']) do - concat(boolean_to_icon(enabled)) + boolean_to_icon(enabled) end) + + if doc_href.present? + link_to_doc = link_to(sprite_icon('question', size: 16), doc_href, + class: 'prepend-left-5', title: _('Documentation'), + target: '_blank', rel: 'noopener noreferrer') + + concat(link_to_doc) + end end end diff --git a/app/views/admin/dashboard/index.html.haml b/app/views/admin/dashboard/index.html.haml index 41147950c40..e5a3c0df9bf 100644 --- a/app/views/admin/dashboard/index.html.haml +++ b/app/views/admin/dashboard/index.html.haml @@ -41,17 +41,38 @@ .info-well .well-segment.admin-well.admin-well-features %h4 Features - = feature_entry(_('Sign up'), href: admin_application_settings_path(anchor: 'js-signup-settings'), enabled: allow_signup?) - = feature_entry(_('LDAP'), enabled: Gitlab.config.ldap.enabled) - = feature_entry(_('Gravatar'), href: admin_application_settings_path(anchor: 'js-account-settings'), enabled: gravatar_enabled?) - = feature_entry(_('OmniAuth'), href: admin_application_settings_path(anchor: 'js-signin-settings'), enabled: Gitlab::Auth.omniauth_enabled?) - = feature_entry(_('Reply by email'), enabled: Gitlab::IncomingEmail.enabled?) + = feature_entry(_('Sign up'), + href: admin_application_settings_path(anchor: 'js-signup-settings'), + enabled: allow_signup?) + + = feature_entry(_('LDAP'), + enabled: Gitlab.config.ldap.enabled) + + = feature_entry(_('Gravatar'), + href: admin_application_settings_path(anchor: 'js-account-settings'), + enabled: gravatar_enabled?) + + = feature_entry(_('OmniAuth'), + href: admin_application_settings_path(anchor: 'js-signin-settings'), + enabled: Gitlab::Auth.omniauth_enabled?) + + = feature_entry(_('Reply by email'), + enabled: Gitlab::IncomingEmail.enabled?) = render_if_exists 'admin/dashboard/elastic_and_geo' - = feature_entry(_('Container Registry'), href: ci_cd_admin_application_settings_path(anchor: 'js-registry-settings'), enabled: Gitlab.config.registry.enabled) - = feature_entry(_('Gitlab Pages'), href: help_instance_configuration_url, enabled: Gitlab.config.pages.enabled) - = feature_entry(_('Shared Runners'), href: admin_runners_path, enabled: Gitlab.config.gitlab_ci.shared_runners_enabled) + = feature_entry(_('Container Registry'), + href: ci_cd_admin_application_settings_path(anchor: 'js-registry-settings'), + enabled: Gitlab.config.registry.enabled, + doc_href: help_page_path('user/packages/container_registry/index')) + + = feature_entry(_('Gitlab Pages'), + enabled: Gitlab.config.pages.enabled, + doc_href: help_instance_configuration_url) + + = feature_entry(_('Shared Runners'), + href: admin_runners_path, + enabled: Gitlab.config.gitlab_ci.shared_runners_enabled) .col-md-4 .info-well .well-segment.admin-well diff --git a/changelogs/unreleased/24082-double-escaping-in-tableflip-quick-action.yml b/changelogs/unreleased/24082-double-escaping-in-tableflip-quick-action.yml new file mode 100644 index 00000000000..b7a2a41c93e --- /dev/null +++ b/changelogs/unreleased/24082-double-escaping-in-tableflip-quick-action.yml @@ -0,0 +1,5 @@ +--- +title: Fix double escaping in /tableflip quick action +merge_request: 19271 +author: Brian T +type: fixed diff --git a/changelogs/unreleased/dz-improve-admin-features.yml b/changelogs/unreleased/dz-improve-admin-features.yml new file mode 100644 index 00000000000..a15b593d04e --- /dev/null +++ b/changelogs/unreleased/dz-improve-admin-features.yml @@ -0,0 +1,5 @@ +--- +title: Improve admin dashboard features +merge_request: 18666 +author: +type: changed diff --git a/doc/api/audit_events.md b/doc/api/audit_events.md index aca221cf990..0b8351062e5 100644 --- a/doc/api/audit_events.md +++ b/doc/api/audit_events.md @@ -15,7 +15,7 @@ GET /audit_events | `created_after` | string | no | Return audit events created on or after the given time. Format: ISO 8601 YYYY-MM-DDTHH:MM:SSZ | | `created_before` | string | no | Return audit events created on or before the given time. Format: ISO 8601 YYYY-MM-DDTHH:MM:SSZ | | `entity_type` | string | no | Return audit events for the given entity type. Valid values are: `User`, `Group`, or `Project`. | -| `entity_id` | boolean | no | Return audit events for the given entity ID. Requires `entity_type` attribute to be present. | +| `entity_id` | integer | no | Return audit events for the given entity ID. Requires `entity_type` attribute to be present. | By default, `GET` requests return 20 results at a time because the API results are paginated. diff --git a/lib/api/helpers/pagination.rb b/lib/api/helpers/pagination.rb index 71bbc218f94..9c5b355e823 100644 --- a/lib/api/helpers/pagination.rb +++ b/lib/api/helpers/pagination.rb @@ -4,254 +4,7 @@ module API module Helpers module Pagination def paginate(relation) - strategy = if params[:pagination] == 'keyset' && Feature.enabled?('api_keyset_pagination') - KeysetPaginationStrategy - else - DefaultPaginationStrategy - end - - strategy.new(self).paginate(relation) - end - - class Base - private - - def per_page - @per_page ||= params[:per_page] - end - - def base_request_uri - @base_request_uri ||= URI.parse(request.url).tap do |uri| - uri.host = Gitlab.config.gitlab.host - uri.port = Gitlab.config.gitlab.port - end - end - - def build_page_url(query_params:) - base_request_uri.tap do |uri| - uri.query = query_params - end.to_s - end - - def page_href(next_page_params = {}) - query_params = params.merge(**next_page_params, per_page: per_page).to_query - - build_page_url(query_params: query_params) - end - end - - class KeysetPaginationInfo - attr_reader :relation, :request_context - - def initialize(relation, request_context) - # This is because it's rather complex to support multiple values with possibly different sort directions - # (and we don't need this in the API) - if relation.order_values.size > 1 - raise "Pagination only supports ordering by a single column." \ - "The following columns were given: #{relation.order_values.map { |v| v.expr.name }}" - end - - @relation = relation - @request_context = request_context - end - - def fields - keys.zip(values).reject { |_, v| v.nil? }.to_h - end - - def column_for_order_by(relation) - relation.order_values.first&.expr&.name - end - - # Sort direction (`:asc` or `:desc`) - def sort - @sort ||= if order_by_primary_key? - # Default order is by id DESC - :desc - else - # API defaults to DESC order if param `sort` not present - request_context.params[:sort]&.to_sym || :desc - end - end - - # Do we only sort by primary key? - def order_by_primary_key? - keys.size == 1 && keys.first == primary_key - end - - def primary_key - relation.model.primary_key.to_sym - end - - def sort_ascending? - sort == :asc - end - - # Build hash of request parameters for a given record (relevant to pagination) - def params_for(record) - return {} unless record - - keys.each_with_object({}) do |key, h| - h["ks_prev_#{key}".to_sym] = record.attributes[key.to_s] - end - end - - private - - # All values present in request parameters that correspond to #keys. - def values - @values ||= keys.map do |key| - request_context.params["ks_prev_#{key}".to_sym] - end - end - - # All keys relevant to pagination. - # This always includes the primary key. Optionally, the `order_by` key is prepended. - def keys - @keys ||= [column_for_order_by(relation), primary_key].compact.uniq - end - end - - class KeysetPaginationStrategy < Base - attr_reader :request_context - delegate :params, :header, :request, to: :request_context - - def initialize(request_context) - @request_context = request_context - end - - # rubocop: disable CodeReuse/ActiveRecord - def paginate(relation) - pagination = KeysetPaginationInfo.new(relation, request_context) - - paged_relation = relation.limit(per_page) - - if conds = conditions(pagination) - paged_relation = paged_relation.where(*conds) - end - - # In all cases: sort by primary key (possibly in addition to another sort column) - paged_relation = paged_relation.order(pagination.primary_key => pagination.sort) - - add_default_pagination_headers - - if last_record = paged_relation.last - next_page_params = pagination.params_for(last_record) - add_navigation_links(next_page_params) - end - - paged_relation - end - # rubocop: enable CodeReuse/ActiveRecord - - private - - def conditions(pagination) - fields = pagination.fields - - return if fields.empty? - - placeholder = fields.map { '?' } - - comp = if pagination.sort_ascending? - '>' - else - '<' - end - - [ - # Row value comparison: - # (A, B) < (a, b) <=> (A < a) OR (A = a AND B < b) - # <=> A <= a AND ((A < a) OR (A = a AND B < b)) - "(#{fields.keys.join(',')}) #{comp} (#{placeholder.join(',')})", - *fields.values - ] - end - - def add_default_pagination_headers - header 'X-Per-Page', per_page.to_s - end - - def add_navigation_links(next_page_params) - header 'X-Next-Page', page_href(next_page_params) - header 'Link', link_for('next', next_page_params) - end - - def link_for(rel, next_page_params) - %(<#{page_href(next_page_params)}>; rel="#{rel}") - end - end - - class DefaultPaginationStrategy < Base - attr_reader :request_context - delegate :params, :header, :request, to: :request_context - - def initialize(request_context) - @request_context = request_context - end - - def paginate(relation) - paginate_with_limit_optimization(add_default_order(relation)).tap do |data| - add_pagination_headers(data) - end - end - - private - - def paginate_with_limit_optimization(relation) - pagination_data = relation.page(params[:page]).per(params[:per_page]) - return pagination_data unless pagination_data.is_a?(ActiveRecord::Relation) - return pagination_data unless Feature.enabled?(:api_kaminari_count_with_limit) - - limited_total_count = pagination_data.total_count_with_limit - if limited_total_count > Kaminari::ActiveRecordRelationMethods::MAX_COUNT_LIMIT - # The call to `total_count_with_limit` memoizes `@arel` because of a call to `references_eager_loaded_tables?` - # We need to call `reset` because `without_count` relies on `@arel` being unmemoized - pagination_data.reset.without_count - else - pagination_data - end - end - - def add_default_order(relation) - if relation.is_a?(ActiveRecord::Relation) && relation.order_values.empty? - relation = relation.order(:id) # rubocop: disable CodeReuse/ActiveRecord - end - - relation - end - - def add_pagination_headers(paginated_data) - header 'X-Per-Page', paginated_data.limit_value.to_s - header 'X-Page', paginated_data.current_page.to_s - header 'X-Next-Page', paginated_data.next_page.to_s - header 'X-Prev-Page', paginated_data.prev_page.to_s - header 'Link', pagination_links(paginated_data) - - return if data_without_counts?(paginated_data) - - header 'X-Total', paginated_data.total_count.to_s - header 'X-Total-Pages', total_pages(paginated_data).to_s - end - - def pagination_links(paginated_data) - [].tap do |links| - links << %(<#{page_href(page: paginated_data.prev_page)}>; rel="prev") if paginated_data.prev_page - links << %(<#{page_href(page: paginated_data.next_page)}>; rel="next") if paginated_data.next_page - links << %(<#{page_href(page: 1)}>; rel="first") - - links << %(<#{page_href(page: total_pages(paginated_data))}>; rel="last") unless data_without_counts?(paginated_data) - end.join(', ') - end - - def total_pages(paginated_data) - # Ensure there is in total at least 1 page - [paginated_data.total_pages, 1].max - end - - def data_without_counts?(paginated_data) - paginated_data.is_a?(Kaminari::PaginatableWithoutCount) - end + ::Gitlab::Pagination::OffsetPagination.new(self).paginate(relation) end end end diff --git a/lib/gitlab/pagination/base.rb b/lib/gitlab/pagination/base.rb new file mode 100644 index 00000000000..90fa1f8d1ec --- /dev/null +++ b/lib/gitlab/pagination/base.rb @@ -0,0 +1,32 @@ +# frozen_string_literal: true + +module Gitlab + module Pagination + class Base + private + + def per_page + @per_page ||= params[:per_page] + end + + def base_request_uri + @base_request_uri ||= URI.parse(request.url).tap do |uri| + uri.host = Gitlab.config.gitlab.host + uri.port = Gitlab.config.gitlab.port + end + end + + def build_page_url(query_params:) + base_request_uri.tap do |uri| + uri.query = query_params + end.to_s + end + + def page_href(next_page_params = {}) + query_params = params.merge(**next_page_params, per_page: per_page).to_query + + build_page_url(query_params: query_params) + end + end + end +end diff --git a/lib/gitlab/pagination/offset_pagination.rb b/lib/gitlab/pagination/offset_pagination.rb new file mode 100644 index 00000000000..bf31f252a6b --- /dev/null +++ b/lib/gitlab/pagination/offset_pagination.rb @@ -0,0 +1,77 @@ +# frozen_string_literal: true + +module Gitlab + module Pagination + class OffsetPagination < Base + attr_reader :request_context + delegate :params, :header, :request, to: :request_context + + def initialize(request_context) + @request_context = request_context + end + + def paginate(relation) + paginate_with_limit_optimization(add_default_order(relation)).tap do |data| + add_pagination_headers(data) + end + end + + private + + def paginate_with_limit_optimization(relation) + pagination_data = relation.page(params[:page]).per(params[:per_page]) + return pagination_data unless pagination_data.is_a?(ActiveRecord::Relation) + return pagination_data unless Feature.enabled?(:api_kaminari_count_with_limit) + + limited_total_count = pagination_data.total_count_with_limit + if limited_total_count > Kaminari::ActiveRecordRelationMethods::MAX_COUNT_LIMIT + # The call to `total_count_with_limit` memoizes `@arel` because of a call to `references_eager_loaded_tables?` + # We need to call `reset` because `without_count` relies on `@arel` being unmemoized + pagination_data.reset.without_count + else + pagination_data + end + end + + def add_default_order(relation) + if relation.is_a?(ActiveRecord::Relation) && relation.order_values.empty? + relation = relation.order(:id) # rubocop: disable CodeReuse/ActiveRecord + end + + relation + end + + def add_pagination_headers(paginated_data) + header 'X-Per-Page', paginated_data.limit_value.to_s + header 'X-Page', paginated_data.current_page.to_s + header 'X-Next-Page', paginated_data.next_page.to_s + header 'X-Prev-Page', paginated_data.prev_page.to_s + header 'Link', pagination_links(paginated_data) + + return if data_without_counts?(paginated_data) + + header 'X-Total', paginated_data.total_count.to_s + header 'X-Total-Pages', total_pages(paginated_data).to_s + end + + def pagination_links(paginated_data) + [].tap do |links| + links << %(<#{page_href(page: paginated_data.prev_page)}>; rel="prev") if paginated_data.prev_page + links << %(<#{page_href(page: paginated_data.next_page)}>; rel="next") if paginated_data.next_page + links << %(<#{page_href(page: 1)}>; rel="first") + + links << %(<#{page_href(page: total_pages(paginated_data))}>; rel="last") unless data_without_counts?(paginated_data) + end.join(', ') + end + + def total_pages(paginated_data) + # Ensure there is in total at least 1 page + [paginated_data.total_pages, 1].max + end + + def data_without_counts?(paginated_data) + paginated_data.is_a?(Kaminari::PaginatableWithoutCount) + end + end + end +end diff --git a/lib/gitlab/quick_actions/issuable_actions.rb b/lib/gitlab/quick_actions/issuable_actions.rb index 340ec75c5f1..942f90e8040 100644 --- a/lib/gitlab/quick_actions/issuable_actions.rb +++ b/lib/gitlab/quick_actions/issuable_actions.rb @@ -234,7 +234,7 @@ module Gitlab "#{comment} #{SHRUG}" end - desc _("Append the comment with %{TABLEFLIP}") % { tableflip: TABLEFLIP } + desc _("Append the comment with %{tableflip}") % { tableflip: TABLEFLIP } params '' types Issuable substitution :tableflip do |comment| diff --git a/lib/gitlab/serializer/pagination.rb b/lib/gitlab/serializer/pagination.rb index eb242cc7c20..bb7571dd66a 100644 --- a/lib/gitlab/serializer/pagination.rb +++ b/lib/gitlab/serializer/pagination.rb @@ -4,7 +4,6 @@ module Gitlab module Serializer class Pagination InvalidResourceError = Class.new(StandardError) - include ::API::Helpers::Pagination def initialize(request, response) @request = request @@ -13,13 +12,13 @@ module Gitlab def paginate(resource) if resource.respond_to?(:page) - super(resource) + ::Gitlab::Pagination::OffsetPagination.new(self).paginate(resource) else raise InvalidResourceError end end - # Methods needed by `API::Helpers::Pagination` + # Methods needed by `Gitlab::Pagination::OffsetPagination` # attr_reader :request diff --git a/locale/gitlab.pot b/locale/gitlab.pot index dd02a55d4c9..9e19792f52e 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -1761,10 +1761,10 @@ msgstr "" msgid "Appearance was successfully updated." msgstr "" -msgid "Append the comment with %{TABLEFLIP}" +msgid "Append the comment with %{shrug}" msgstr "" -msgid "Append the comment with %{shrug}" +msgid "Append the comment with %{tableflip}" msgstr "" msgid "Application" @@ -5742,6 +5742,9 @@ msgstr "" msgid "Dockerfile" msgstr "" +msgid "Documentation" +msgstr "" + msgid "Documentation for popular identity providers" msgstr "" @@ -10502,6 +10505,9 @@ msgstr "" msgid "Metrics|There was an error fetching the environments data, please try again" msgstr "" +msgid "Metrics|There was an error fetching the logs, please try again" +msgstr "" + msgid "Metrics|There was an error getting deployment information." msgstr "" diff --git a/spec/helpers/dashboard_helper_spec.rb b/spec/helpers/dashboard_helper_spec.rb index c899c2d9853..8a4ea33ac7c 100644 --- a/spec/helpers/dashboard_helper_spec.rb +++ b/spec/helpers/dashboard_helper_spec.rb @@ -25,39 +25,62 @@ describe DashboardHelper do end describe '#feature_entry' do - context 'when implicitly enabled' do - it 'considers feature enabled by default' do - entry = feature_entry('Demo', href: 'demo.link') + shared_examples "a feature is enabled" do + it { is_expected.to include('

') } + end - expect(entry).to include('

') - expect(entry).to include('Demo') + shared_examples "a feature is disabled" do + it { is_expected.to include('

') } + end + + shared_examples "a feature without link" do + it do + is_expected.not_to have_link('Demo') + is_expected.not_to have_link('Documentation') end end - context 'when explicitly enabled' do - it 'returns a link' do - entry = feature_entry('Demo', href: 'demo.link', enabled: true) + shared_examples "a feature with configuration" do + it { is_expected.to have_link('Demo', href: 'demo.link') } + end - expect(entry).to include('

') - expect(entry).to include('Demo') + shared_examples "a feature with documentation" do + it { is_expected.to have_link('Documentation', href: 'doc.link') } + end + + context 'when implicitly enabled' do + subject { feature_entry('Demo') } + + it_behaves_like 'a feature is enabled' + end + + context 'when explicitly enabled' do + context 'without links' do + subject { feature_entry('Demo', enabled: true) } + + it_behaves_like 'a feature is enabled' + it_behaves_like 'a feature without link' end - it 'returns text if href is not provided' do - entry = feature_entry('Demo', enabled: true) + context 'with configure link' do + subject { feature_entry('Demo', href: 'demo.link', enabled: true) } - expect(entry).to include('

') - expect(entry).not_to match(/]+>/) + it_behaves_like 'a feature with configuration' + end + + context 'with configure and documentation links' do + subject { feature_entry('Demo', href: 'demo.link', doc_href: 'doc.link', enabled: true) } + + it_behaves_like 'a feature with configuration' + it_behaves_like 'a feature with documentation' end end context 'when disabled' do - it 'returns text without link' do - entry = feature_entry('Demo', href: 'demo.link', enabled: false) + subject { feature_entry('Demo', href: 'demo.link', enabled: false) } - expect(entry).to include('

') - expect(entry).not_to match(/]+>/) - expect(entry).to include('Demo') - end + it_behaves_like 'a feature is disabled' + it_behaves_like 'a feature without link' end end diff --git a/spec/lib/api/helpers/pagination_spec.rb b/spec/lib/api/helpers/pagination_spec.rb index b57adb46385..040ff1a8ebe 100644 --- a/spec/lib/api/helpers/pagination_spec.rb +++ b/spec/lib/api/helpers/pagination_spec.rb @@ -3,399 +3,20 @@ require 'spec_helper' describe API::Helpers::Pagination do - let(:resource) { Project.all } - let(:custom_port) { 8080 } - let(:incoming_api_projects_url) { "#{Gitlab.config.gitlab.url}:#{custom_port}/api/v4/projects" } + subject { Class.new.include(described_class).new } - before do - stub_config_setting(port: custom_port) - end + describe '#paginate' do + let(:relation) { double("relation") } + let(:offset_pagination) { double("offset pagination") } + let(:expected_result) { double("result") } - subject do - Class.new.include(described_class).new - end + it 'delegates to OffsetPagination' do + expect(::Gitlab::Pagination::OffsetPagination).to receive(:new).with(subject).and_return(offset_pagination) + expect(offset_pagination).to receive(:paginate).with(relation).and_return(expected_result) - describe '#paginate (keyset pagination)' do - let(:value) { spy('return value') } - let(:base_query) do - { - pagination: 'keyset', - foo: 'bar', - bar: 'baz' - } + result = subject.paginate(relation) + + expect(result).to eq(expected_result) end - let(:query) { base_query } - - before do - allow(subject).to receive(:header).and_return(value) - allow(subject).to receive(:params).and_return(query) - allow(subject).to receive(:request).and_return(double(url: "#{incoming_api_projects_url}?#{query.to_query}")) - end - - context 'when resource can be paginated' do - let!(:projects) do - [ - create(:project, name: 'One'), - create(:project, name: 'Two'), - create(:project, name: 'Three') - ].sort_by { |e| -e.id } # sort by id desc (this is the default sort order for the API) - end - - describe 'first page' do - let(:query) { base_query.merge(per_page: 2) } - - it 'returns appropriate amount of resources' do - expect(subject.paginate(resource).count).to eq 2 - end - - it 'returns the first two records (by id desc)' do - expect(subject.paginate(resource)).to eq(projects[0..1]) - end - - it 'adds appropriate headers' do - expect_header('X-Per-Page', '2') - expect_header('X-Next-Page', "#{incoming_api_projects_url}?#{query.merge(ks_prev_id: projects[1].id).to_query}") - - expect_header('Link', anything) do |_key, val| - expect(val).to include('rel="next"') - end - - subject.paginate(resource) - end - end - - describe 'second page' do - let(:query) { base_query.merge(per_page: 2, ks_prev_id: projects[1].id) } - - it 'returns appropriate amount of resources' do - expect(subject.paginate(resource).count).to eq 1 - end - - it 'returns the third record' do - expect(subject.paginate(resource)).to eq(projects[2..2]) - end - - it 'adds appropriate headers' do - expect_header('X-Per-Page', '2') - expect_header('X-Next-Page', "#{incoming_api_projects_url}?#{query.merge(ks_prev_id: projects[2].id).to_query}") - - expect_header('Link', anything) do |_key, val| - expect(val).to include('rel="next"') - end - - subject.paginate(resource) - end - end - - describe 'third page' do - let(:query) { base_query.merge(per_page: 2, ks_prev_id: projects[2].id) } - - it 'returns appropriate amount of resources' do - expect(subject.paginate(resource).count).to eq 0 - end - - it 'adds appropriate headers' do - expect_header('X-Per-Page', '2') - expect_no_header('X-Next-Page') - expect(subject).not_to receive(:header).with('Link') - - subject.paginate(resource) - end - end - - context 'if order' do - context 'is not present' do - let(:query) { base_query.merge(per_page: 2) } - - it 'is not present it adds default order(:id) desc' do - resource.order_values = [] - - paginated_relation = subject.paginate(resource) - - expect(resource.order_values).to be_empty - expect(paginated_relation.order_values).to be_present - expect(paginated_relation.order_values.size).to eq(1) - expect(paginated_relation.order_values.first).to be_descending - expect(paginated_relation.order_values.first.expr.name).to eq 'id' - end - end - - context 'is present' do - let(:resource) { Project.all.order(name: :desc) } - let!(:projects) do - [ - create(:project, name: 'One'), - create(:project, name: 'Two'), - create(:project, name: 'Three'), - create(:project, name: 'Three'), # Note the duplicate name - create(:project, name: 'Four'), - create(:project, name: 'Five'), - create(:project, name: 'Six') - ] - - # if we sort this by name descending, id descending, this yields: - # { - # 2 => "Two", - # 4 => "Three", - # 3 => "Three", - # 7 => "Six", - # 1 => "One", - # 5 => "Four", - # 6 => "Five" - # } - # - # (key is the id) - end - - it 'also orders by primary key' do - paginated_relation = subject.paginate(resource) - - expect(paginated_relation.order_values).to be_present - expect(paginated_relation.order_values.size).to eq(2) - expect(paginated_relation.order_values.first).to be_descending - expect(paginated_relation.order_values.first.expr.name).to eq 'name' - expect(paginated_relation.order_values.second).to be_descending - expect(paginated_relation.order_values.second.expr.name).to eq 'id' - end - - it 'returns the right records (first page)' do - result = subject.paginate(resource) - - expect(result.first).to eq(projects[1]) - expect(result.second).to eq(projects[3]) - end - - describe 'second page' do - let(:query) { base_query.merge(ks_prev_id: projects[3].id, ks_prev_name: projects[3].name, per_page: 2) } - - it 'returns the right records (second page)' do - result = subject.paginate(resource) - - expect(result.first).to eq(projects[2]) - expect(result.second).to eq(projects[6]) - end - - it 'returns the right link to the next page' do - expect_header('X-Per-Page', '2') - expect_header('X-Next-Page', "#{incoming_api_projects_url}?#{query.merge(ks_prev_id: projects[6].id, ks_prev_name: projects[6].name).to_query}") - expect_header('Link', anything) do |_key, val| - expect(val).to include('rel="next"') - end - - subject.paginate(resource) - end - end - - describe 'third page' do - let(:query) { base_query.merge(ks_prev_id: projects[6].id, ks_prev_name: projects[6].name, per_page: 5) } - - it 'returns the right records (third page), note increased per_page' do - result = subject.paginate(resource) - - expect(result.size).to eq(3) - expect(result.first).to eq(projects[0]) - expect(result.second).to eq(projects[4]) - expect(result.last).to eq(projects[5]) - end - end - end - end - end - end - - describe '#paginate (default offset-based pagination)' do - let(:value) { spy('return value') } - let(:base_query) { { foo: 'bar', bar: 'baz' } } - let(:query) { base_query } - - before do - allow(subject).to receive(:header).and_return(value) - allow(subject).to receive(:params).and_return(query) - allow(subject).to receive(:request).and_return(double(url: "#{incoming_api_projects_url}?#{query.to_query}")) - end - - context 'when resource can be paginated' do - before do - create_list(:project, 3) - end - - describe 'first page' do - shared_examples 'response with pagination headers' do - it 'adds appropriate headers' do - expect_header('X-Total', '3') - expect_header('X-Total-Pages', '2') - expect_header('X-Per-Page', '2') - expect_header('X-Page', '1') - expect_header('X-Next-Page', '2') - expect_header('X-Prev-Page', '') - - expect_header('Link', anything) do |_key, val| - expect(val).to include(%Q(<#{incoming_api_projects_url}?#{query.merge(page: 1).to_query}>; rel="first")) - expect(val).to include(%Q(<#{incoming_api_projects_url}?#{query.merge(page: 2).to_query}>; rel="last")) - expect(val).to include(%Q(<#{incoming_api_projects_url}?#{query.merge(page: 2).to_query}>; rel="next")) - expect(val).not_to include('rel="prev"') - end - - subject.paginate(resource) - end - end - - shared_examples 'paginated response' do - it 'returns appropriate amount of resources' do - expect(subject.paginate(resource).count).to eq 2 - end - - it 'executes only one SELECT COUNT query' do - expect { subject.paginate(resource) }.to make_queries_matching(/SELECT COUNT/, 1) - end - end - - let(:query) { base_query.merge(page: 1, per_page: 2) } - - context 'when the api_kaminari_count_with_limit feature flag is unset' do - it_behaves_like 'paginated response' - it_behaves_like 'response with pagination headers' - end - - context 'when the api_kaminari_count_with_limit feature flag is disabled' do - before do - stub_feature_flags(api_kaminari_count_with_limit: false) - end - - it_behaves_like 'paginated response' - it_behaves_like 'response with pagination headers' - end - - context 'when the api_kaminari_count_with_limit feature flag is enabled' do - before do - stub_feature_flags(api_kaminari_count_with_limit: true) - end - - context 'when resources count is less than MAX_COUNT_LIMIT' do - before do - stub_const("::Kaminari::ActiveRecordRelationMethods::MAX_COUNT_LIMIT", 4) - end - - it_behaves_like 'paginated response' - it_behaves_like 'response with pagination headers' - end - - context 'when resources count is more than MAX_COUNT_LIMIT' do - before do - stub_const("::Kaminari::ActiveRecordRelationMethods::MAX_COUNT_LIMIT", 2) - end - - it_behaves_like 'paginated response' - - it 'does not return the X-Total and X-Total-Pages headers' do - expect_no_header('X-Total') - expect_no_header('X-Total-Pages') - expect_header('X-Per-Page', '2') - expect_header('X-Page', '1') - expect_header('X-Next-Page', '2') - expect_header('X-Prev-Page', '') - - expect_header('Link', anything) do |_key, val| - expect(val).to include(%Q(<#{incoming_api_projects_url}?#{query.merge(page: 1).to_query}>; rel="first")) - expect(val).to include(%Q(<#{incoming_api_projects_url}?#{query.merge(page: 2).to_query}>; rel="next")) - expect(val).not_to include('rel="last"') - expect(val).not_to include('rel="prev"') - end - - subject.paginate(resource) - end - end - end - end - - describe 'second page' do - let(:query) { base_query.merge(page: 2, per_page: 2) } - - it 'returns appropriate amount of resources' do - expect(subject.paginate(resource).count).to eq 1 - end - - it 'adds appropriate headers' do - expect_header('X-Total', '3') - expect_header('X-Total-Pages', '2') - expect_header('X-Per-Page', '2') - expect_header('X-Page', '2') - expect_header('X-Next-Page', '') - expect_header('X-Prev-Page', '1') - - expect_header('Link', anything) do |_key, val| - expect(val).to include(%Q(<#{incoming_api_projects_url}?#{query.merge(page: 1).to_query}>; rel="first")) - expect(val).to include(%Q(<#{incoming_api_projects_url}?#{query.merge(page: 2).to_query}>; rel="last")) - expect(val).to include(%Q(<#{incoming_api_projects_url}?#{query.merge(page: 1).to_query}>; rel="prev")) - expect(val).not_to include('rel="next"') - end - - subject.paginate(resource) - end - end - - context 'if order' do - it 'is not present it adds default order(:id) if no order is present' do - resource.order_values = [] - - paginated_relation = subject.paginate(resource) - - expect(resource.order_values).to be_empty - expect(paginated_relation.order_values).to be_present - expect(paginated_relation.order_values.first).to be_ascending - expect(paginated_relation.order_values.first.expr.name).to eq 'id' - end - - it 'is present it does not add anything' do - paginated_relation = subject.paginate(resource.order(created_at: :desc)) - - expect(paginated_relation.order_values).to be_present - expect(paginated_relation.order_values.first).to be_descending - expect(paginated_relation.order_values.first.expr.name).to eq 'created_at' - end - end - end - - context 'when resource empty' do - describe 'first page' do - let(:query) { base_query.merge(page: 1, per_page: 2) } - - it 'returns appropriate amount of resources' do - expect(subject.paginate(resource).count).to eq 0 - end - - it 'adds appropriate headers' do - expect_header('X-Total', '0') - expect_header('X-Total-Pages', '1') - expect_header('X-Per-Page', '2') - expect_header('X-Page', '1') - expect_header('X-Next-Page', '') - expect_header('X-Prev-Page', '') - - expect_header('Link', anything) do |_key, val| - expect(val).to include(%Q(<#{incoming_api_projects_url}?#{query.merge(page: 1).to_query}>; rel="first")) - expect(val).to include(%Q(<#{incoming_api_projects_url}?#{query.merge(page: 1).to_query}>; rel="last")) - expect(val).not_to include('rel="prev"') - expect(val).not_to include('rel="next"') - expect(val).not_to include('page=0') - end - - subject.paginate(resource) - end - end - end - end - - def expect_header(*args, &block) - expect(subject).to receive(:header).with(*args, &block) - end - - def expect_no_header(*args, &block) - expect(subject).not_to receive(:header).with(*args) - end - - def expect_message(method) - expect(subject).to receive(method) - .at_least(:once).and_return(value) end end diff --git a/spec/lib/gitlab/pagination/offset_pagination_spec.rb b/spec/lib/gitlab/pagination/offset_pagination_spec.rb new file mode 100644 index 00000000000..9c7dd385726 --- /dev/null +++ b/spec/lib/gitlab/pagination/offset_pagination_spec.rb @@ -0,0 +1,215 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Gitlab::Pagination::OffsetPagination do + let(:resource) { Project.all } + let(:custom_port) { 8080 } + let(:incoming_api_projects_url) { "#{Gitlab.config.gitlab.url}:#{custom_port}/api/v4/projects" } + + before do + stub_config_setting(port: custom_port) + end + + let(:request_context) { double("request_context") } + + subject do + described_class.new(request_context) + end + + describe '#paginate' do + let(:value) { spy('return value') } + let(:base_query) { { foo: 'bar', bar: 'baz' } } + let(:query) { base_query } + + before do + allow(request_context).to receive(:header).and_return(value) + allow(request_context).to receive(:params).and_return(query) + allow(request_context).to receive(:request).and_return(double(url: "#{incoming_api_projects_url}?#{query.to_query}")) + end + + context 'when resource can be paginated' do + before do + create_list(:project, 3) + end + + describe 'first page' do + shared_examples 'response with pagination headers' do + it 'adds appropriate headers' do + expect_header('X-Total', '3') + expect_header('X-Total-Pages', '2') + expect_header('X-Per-Page', '2') + expect_header('X-Page', '1') + expect_header('X-Next-Page', '2') + expect_header('X-Prev-Page', '') + + expect_header('Link', anything) do |_key, val| + expect(val).to include(%Q(<#{incoming_api_projects_url}?#{query.merge(page: 1).to_query}>; rel="first")) + expect(val).to include(%Q(<#{incoming_api_projects_url}?#{query.merge(page: 2).to_query}>; rel="last")) + expect(val).to include(%Q(<#{incoming_api_projects_url}?#{query.merge(page: 2).to_query}>; rel="next")) + expect(val).not_to include('rel="prev"') + end + + subject.paginate(resource) + end + end + + shared_examples 'paginated response' do + it 'returns appropriate amount of resources' do + expect(subject.paginate(resource).count).to eq 2 + end + + it 'executes only one SELECT COUNT query' do + expect { subject.paginate(resource) }.to make_queries_matching(/SELECT COUNT/, 1) + end + end + + let(:query) { base_query.merge(page: 1, per_page: 2) } + + context 'when the api_kaminari_count_with_limit feature flag is unset' do + it_behaves_like 'paginated response' + it_behaves_like 'response with pagination headers' + end + + context 'when the api_kaminari_count_with_limit feature flag is disabled' do + before do + stub_feature_flags(api_kaminari_count_with_limit: false) + end + + it_behaves_like 'paginated response' + it_behaves_like 'response with pagination headers' + end + + context 'when the api_kaminari_count_with_limit feature flag is enabled' do + before do + stub_feature_flags(api_kaminari_count_with_limit: true) + end + + context 'when resources count is less than MAX_COUNT_LIMIT' do + before do + stub_const("::Kaminari::ActiveRecordRelationMethods::MAX_COUNT_LIMIT", 4) + end + + it_behaves_like 'paginated response' + it_behaves_like 'response with pagination headers' + end + + context 'when resources count is more than MAX_COUNT_LIMIT' do + before do + stub_const("::Kaminari::ActiveRecordRelationMethods::MAX_COUNT_LIMIT", 2) + end + + it_behaves_like 'paginated response' + + it 'does not return the X-Total and X-Total-Pages headers' do + expect_no_header('X-Total') + expect_no_header('X-Total-Pages') + expect_header('X-Per-Page', '2') + expect_header('X-Page', '1') + expect_header('X-Next-Page', '2') + expect_header('X-Prev-Page', '') + + expect_header('Link', anything) do |_key, val| + expect(val).to include(%Q(<#{incoming_api_projects_url}?#{query.merge(page: 1).to_query}>; rel="first")) + expect(val).to include(%Q(<#{incoming_api_projects_url}?#{query.merge(page: 2).to_query}>; rel="next")) + expect(val).not_to include('rel="last"') + expect(val).not_to include('rel="prev"') + end + + subject.paginate(resource) + end + end + end + end + + describe 'second page' do + let(:query) { base_query.merge(page: 2, per_page: 2) } + + it 'returns appropriate amount of resources' do + expect(subject.paginate(resource).count).to eq 1 + end + + it 'adds appropriate headers' do + expect_header('X-Total', '3') + expect_header('X-Total-Pages', '2') + expect_header('X-Per-Page', '2') + expect_header('X-Page', '2') + expect_header('X-Next-Page', '') + expect_header('X-Prev-Page', '1') + + expect_header('Link', anything) do |_key, val| + expect(val).to include(%Q(<#{incoming_api_projects_url}?#{query.merge(page: 1).to_query}>; rel="first")) + expect(val).to include(%Q(<#{incoming_api_projects_url}?#{query.merge(page: 2).to_query}>; rel="last")) + expect(val).to include(%Q(<#{incoming_api_projects_url}?#{query.merge(page: 1).to_query}>; rel="prev")) + expect(val).not_to include('rel="next"') + end + + subject.paginate(resource) + end + end + + context 'if order' do + it 'is not present it adds default order(:id) if no order is present' do + resource.order_values = [] + + paginated_relation = subject.paginate(resource) + + expect(resource.order_values).to be_empty + expect(paginated_relation.order_values).to be_present + expect(paginated_relation.order_values.first).to be_ascending + expect(paginated_relation.order_values.first.expr.name).to eq 'id' + end + + it 'is present it does not add anything' do + paginated_relation = subject.paginate(resource.order(created_at: :desc)) + + expect(paginated_relation.order_values).to be_present + expect(paginated_relation.order_values.first).to be_descending + expect(paginated_relation.order_values.first.expr.name).to eq 'created_at' + end + end + end + + context 'when resource empty' do + describe 'first page' do + let(:query) { base_query.merge(page: 1, per_page: 2) } + + it 'returns appropriate amount of resources' do + expect(subject.paginate(resource).count).to eq 0 + end + + it 'adds appropriate headers' do + expect_header('X-Total', '0') + expect_header('X-Total-Pages', '1') + expect_header('X-Per-Page', '2') + expect_header('X-Page', '1') + expect_header('X-Next-Page', '') + expect_header('X-Prev-Page', '') + + expect_header('Link', anything) do |_key, val| + expect(val).to include(%Q(<#{incoming_api_projects_url}?#{query.merge(page: 1).to_query}>; rel="first")) + expect(val).to include(%Q(<#{incoming_api_projects_url}?#{query.merge(page: 1).to_query}>; rel="last")) + expect(val).not_to include('rel="prev"') + expect(val).not_to include('rel="next"') + expect(val).not_to include('page=0') + end + + subject.paginate(resource) + end + end + end + end + + def expect_header(*args, &block) + expect(subject).to receive(:header).with(*args, &block) + end + + def expect_no_header(*args, &block) + expect(subject).not_to receive(:header).with(*args) + end + + def expect_message(method) + expect(subject).to receive(method) + .at_least(:once).and_return(value) + end +end