diff options
author | GitLab Bot <gitlab-bot@gitlab.com> | 2019-11-29 21:06:24 +0300 |
---|---|---|
committer | GitLab Bot <gitlab-bot@gitlab.com> | 2019-11-29 21:06:24 +0300 |
commit | 8263f6ee3131cdea3c6041785c32771a6af0b24f (patch) | |
tree | 3dde0ed2466b10fa223eacbd51c78beb32009fbd /spec | |
parent | eac0da9a47f0c7b8b970833d7d5b96cfee057bf7 (diff) |
Add latest changes from gitlab-org/gitlab@master
Diffstat (limited to 'spec')
-rw-r--r-- | spec/lib/api/helpers/pagination_spec.rb | 44 | ||||
-rw-r--r-- | spec/lib/feature_spec.rb | 1 | ||||
-rw-r--r-- | spec/lib/gitlab/pagination/keyset/page_spec.rb | 66 | ||||
-rw-r--r-- | spec/lib/gitlab/pagination/keyset/pager_spec.rb | 68 | ||||
-rw-r--r-- | spec/lib/gitlab/pagination/keyset/request_context_spec.rb | 115 | ||||
-rw-r--r-- | spec/lib/gitlab/pagination/keyset_spec.rb | 61 | ||||
-rw-r--r-- | spec/lib/marginalia_spec.rb | 173 | ||||
-rw-r--r-- | spec/requests/api/projects_spec.rb | 81 | ||||
-rw-r--r-- | spec/serializers/pipeline_serializer_spec.rb | 6 |
9 files changed, 607 insertions, 8 deletions
diff --git a/spec/lib/api/helpers/pagination_spec.rb b/spec/lib/api/helpers/pagination_spec.rb index 040ff1a8ebe..d61341ed3e5 100644 --- a/spec/lib/api/helpers/pagination_spec.rb +++ b/spec/lib/api/helpers/pagination_spec.rb @@ -10,13 +10,47 @@ describe API::Helpers::Pagination do let(:offset_pagination) { double("offset pagination") } let(:expected_result) { double("result") } - 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) + before do + allow(subject).to receive(:params).and_return(params) + end + + context 'for offset pagination' do + let(:params) { {} } + + 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) + + result = subject.paginate(relation) + + expect(result).to eq(expected_result) + end + end + + context 'for keyset pagination' do + let(:params) { { pagination: 'keyset' } } + let(:request_context) { double('request context') } + + before do + allow(Gitlab::Pagination::Keyset::RequestContext).to receive(:new).with(subject).and_return(request_context) + allow(Gitlab::Pagination::Keyset).to receive(:available?).and_return(true) + end + + it 'delegates to KeysetPagination' do + expect(Gitlab::Pagination::Keyset).to receive(:paginate).with(request_context, relation).and_return(expected_result) + + result = subject.paginate(relation) + + expect(result).to eq(expected_result) + end - result = subject.paginate(relation) + it 'renders a 501 error if keyset pagination isnt available yet' do + expect(Gitlab::Pagination::Keyset).to receive(:available?).with(request_context, relation).and_return(false) + expect(Gitlab::Pagination::Keyset).not_to receive(:paginate) + expect(subject).to receive(:error!).with(/not yet available/, 501) - expect(result).to eq(expected_result) + subject.paginate(relation) + end end end end diff --git a/spec/lib/feature_spec.rb b/spec/lib/feature_spec.rb index 3d59b1f35a9..7254981d09a 100644 --- a/spec/lib/feature_spec.rb +++ b/spec/lib/feature_spec.rb @@ -175,6 +175,7 @@ describe Feature do let(:flag) { :some_feature_flag } before do + stub_feature_flags(Gitlab::Marginalia::MARGINALIA_FEATURE_FLAG => false) described_class.flipper.memoize = false described_class.enabled?(flag) end diff --git a/spec/lib/gitlab/pagination/keyset/page_spec.rb b/spec/lib/gitlab/pagination/keyset/page_spec.rb new file mode 100644 index 00000000000..bda9e6ecd13 --- /dev/null +++ b/spec/lib/gitlab/pagination/keyset/page_spec.rb @@ -0,0 +1,66 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Gitlab::Pagination::Keyset::Page do + describe '#per_page' do + it 'limits to a maximum of 20 records per page' do + per_page = described_class.new(per_page: 21).per_page + + expect(per_page).to eq(described_class::DEFAULT_PAGE_SIZE) + end + + it 'uses default value when given 0' do + per_page = described_class.new(per_page: 0).per_page + + expect(per_page).to eq(described_class::DEFAULT_PAGE_SIZE) + end + + it 'uses default value when given negative values' do + per_page = described_class.new(per_page: -1).per_page + + expect(per_page).to eq(described_class::DEFAULT_PAGE_SIZE) + end + + it 'uses the given value if it is within range' do + per_page = described_class.new(per_page: 10).per_page + + expect(per_page).to eq(10) + end + end + + describe '#next' do + let(:page) { described_class.new(order_by: order_by, lower_bounds: lower_bounds, per_page: per_page, end_reached: end_reached) } + subject { page.next(new_lower_bounds, new_end_reached) } + + let(:order_by) { { id: :desc } } + let(:lower_bounds) { { id: 42 } } + let(:per_page) { 10 } + let(:end_reached) { false } + + let(:new_lower_bounds) { { id: 21 } } + let(:new_end_reached) { true } + + it 'copies over order_by' do + expect(subject.order_by).to eq(page.order_by) + end + + it 'copies over per_page' do + expect(subject.per_page).to eq(page.per_page) + end + + it 'dups the instance' do + expect(subject).not_to eq(page) + end + + it 'sets lower_bounds only on new instance' do + expect(subject.lower_bounds).to eq(new_lower_bounds) + expect(page.lower_bounds).to eq(lower_bounds) + end + + it 'sets end_reached only on new instance' do + expect(subject.end_reached?).to eq(new_end_reached) + expect(page.end_reached?).to eq(end_reached) + end + end +end diff --git a/spec/lib/gitlab/pagination/keyset/pager_spec.rb b/spec/lib/gitlab/pagination/keyset/pager_spec.rb new file mode 100644 index 00000000000..6d23fe2adcc --- /dev/null +++ b/spec/lib/gitlab/pagination/keyset/pager_spec.rb @@ -0,0 +1,68 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Gitlab::Pagination::Keyset::Pager do + let(:relation) { Project.all.order(id: :asc) } + let(:request) { double('request', page: page, apply_headers: nil) } + let(:page) { Gitlab::Pagination::Keyset::Page.new(order_by: { id: :asc }, per_page: 3) } + let(:next_page) { double('next page') } + + before_all do + create_list(:project, 7) + end + + describe '#paginate' do + subject { described_class.new(request).paginate(relation) } + + it 'loads the result relation only once' do + expect do + subject + end.not_to exceed_query_limit(1) + end + + it 'passes information about next page to request' do + lower_bounds = relation.limit(page.per_page).last.slice(:id) + expect(page).to receive(:next).with(lower_bounds, false).and_return(next_page) + expect(request).to receive(:apply_headers).with(next_page) + + subject + end + + context 'when retrieving the last page' do + let(:relation) { Project.where('id > ?', Project.maximum(:id) - page.per_page).order(id: :asc) } + + it 'indicates this is the last page' do + expect(request).to receive(:apply_headers) do |next_page| + expect(next_page.end_reached?).to be_truthy + end + + subject + end + end + + context 'when retrieving an empty page' do + let(:relation) { Project.where('id > ?', Project.maximum(:id) + 1).order(id: :asc) } + + it 'indicates this is the last page' do + expect(request).to receive(:apply_headers) do |next_page| + expect(next_page.end_reached?).to be_truthy + end + + subject + end + end + + it 'returns an array with the loaded records' do + expect(subject).to eq(relation.limit(page.per_page).to_a) + end + + context 'validating the order clause' do + let(:page) { Gitlab::Pagination::Keyset::Page.new(order_by: { created_at: :asc }, per_page: 3) } + + it 'raises an error if has a different order clause than the page' do + expect { subject }.to raise_error(ArgumentError, /order_by does not match/) + end + end + end +end diff --git a/spec/lib/gitlab/pagination/keyset/request_context_spec.rb b/spec/lib/gitlab/pagination/keyset/request_context_spec.rb new file mode 100644 index 00000000000..344ef90efa3 --- /dev/null +++ b/spec/lib/gitlab/pagination/keyset/request_context_spec.rb @@ -0,0 +1,115 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Gitlab::Pagination::Keyset::RequestContext do + let(:request) { double('request', params: params) } + + describe '#page' do + subject { described_class.new(request).page } + + context 'with only order_by given' do + let(:params) { { order_by: :id } } + + it 'extracts order_by/sorting information' do + page = subject + + expect(page.order_by).to eq(id: :desc) + end + end + + context 'with order_by and sort given' do + let(:params) { { order_by: :created_at, sort: :desc } } + + it 'extracts order_by/sorting information and adds tie breaker' do + page = subject + + expect(page.order_by).to eq(created_at: :desc, id: :desc) + end + end + + context 'with no order_by information given' do + let(:params) { {} } + + it 'defaults to tie breaker' do + page = subject + + expect(page.order_by).to eq({ id: :desc }) + end + end + + context 'with per_page params given' do + let(:params) { { per_page: 10 } } + + it 'extracts per_page information' do + page = subject + + expect(page.per_page).to eq(params[:per_page]) + end + end + end + + describe '#apply_headers' do + let(:request) { double('request', url: "http://#{Gitlab.config.gitlab.host}/api/v4/projects?foo=bar") } + let(:params) { { foo: 'bar' } } + let(:request_context) { double('request context', params: params, request: request) } + let(:next_page) { double('next page', order_by: { id: :asc }, lower_bounds: { id: 42 }, end_reached?: false) } + + subject { described_class.new(request_context).apply_headers(next_page) } + + it 'sets Links header with same host/path as the original request' do + orig_uri = URI.parse(request_context.request.url) + + expect(request_context).to receive(:header) do |name, header| + expect(name).to eq('Links') + + first_link, _ = /<([^>]+)>; rel="next"/.match(header).captures + + uri = URI.parse(first_link) + + expect(uri.host).to eq(orig_uri.host) + expect(uri.path).to eq(orig_uri.path) + end + + subject + end + + it 'sets Links header with a link to the next page' do + orig_uri = URI.parse(request_context.request.url) + + expect(request_context).to receive(:header) do |name, header| + expect(name).to eq('Links') + + first_link, _ = /<([^>]+)>; rel="next"/.match(header).captures + + query = CGI.parse(URI.parse(first_link).query) + + expect(query.except('id_after')).to eq(CGI.parse(orig_uri.query).except('id_after')) + expect(query['id_after']).to eq(['42']) + end + + subject + end + + context 'with descending order' do + let(:next_page) { double('next page', order_by: { id: :desc }, lower_bounds: { id: 42 }, end_reached?: false) } + + it 'sets Links header with a link to the next page' do + orig_uri = URI.parse(request_context.request.url) + + expect(request_context).to receive(:header) do |name, header| + expect(name).to eq('Links') + + first_link, _ = /<([^>]+)>; rel="next"/.match(header).captures + + query = CGI.parse(URI.parse(first_link).query) + + expect(query.except('id_before')).to eq(CGI.parse(orig_uri.query).except('id_before')) + expect(query['id_before']).to eq(['42']) + end + + subject + end + end + end +end diff --git a/spec/lib/gitlab/pagination/keyset_spec.rb b/spec/lib/gitlab/pagination/keyset_spec.rb new file mode 100644 index 00000000000..755c422c46a --- /dev/null +++ b/spec/lib/gitlab/pagination/keyset_spec.rb @@ -0,0 +1,61 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Gitlab::Pagination::Keyset do + describe '.paginate' do + subject { described_class.paginate(request_context, relation) } + + let(:request_context) { double } + let(:relation) { double } + let(:pager) { double } + let(:result) { double } + + it 'uses Pager to paginate the relation' do + expect(Gitlab::Pagination::Keyset::Pager).to receive(:new).with(request_context).and_return(pager) + expect(pager).to receive(:paginate).with(relation).and_return(result) + + expect(subject).to eq(result) + end + end + + describe '.available?' do + subject { described_class } + + let(:request_context) { double("request context", page: page)} + let(:page) { double("page", order_by: order_by) } + + shared_examples_for 'keyset pagination is available' do + it 'returns true for Project' do + expect(subject.available?(request_context, Project.all)).to be_truthy + end + + it 'return false for other types of relations' do + expect(subject.available?(request_context, User.all)).to be_falsey + end + end + + context 'with order-by id asc' do + let(:order_by) { { id: :asc } } + + it_behaves_like 'keyset pagination is available' + end + + context 'with order-by id desc' do + let(:order_by) { { id: :desc } } + + it_behaves_like 'keyset pagination is available' + end + + context 'with other order-by columns' do + let(:order_by) { { created_at: :desc, id: :desc } } + it 'returns false for Project' do + expect(subject.available?(request_context, Project.all)).to be_falsey + end + + it 'return false for other types of relations' do + expect(subject.available?(request_context, User.all)).to be_falsey + end + end + end +end diff --git a/spec/lib/marginalia_spec.rb b/spec/lib/marginalia_spec.rb new file mode 100644 index 00000000000..2a1dbd94a9e --- /dev/null +++ b/spec/lib/marginalia_spec.rb @@ -0,0 +1,173 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe 'Marginalia spec' do + class MarginaliaTestController < ActionController::Base + def first_user + User.first + render body: nil + end + end + + class MarginaliaTestJob + include Sidekiq::Worker + + def perform + User.first + end + end + + class MarginaliaTestMailer < BaseMailer + def first_user + User.first + end + end + + def add_sidekiq_middleware + # Reference: https://github.com/mperham/sidekiq/wiki/Testing#testing-server-middlewaresidekiq + # Sidekiq test harness fakes worker without its server middlewares, so include instrumentation to 'Sidekiq::Testing' server middleware. + Sidekiq::Testing.server_middleware do |chain| + chain.add Marginalia::SidekiqInstrumentation::Middleware + end + end + + def remove_sidekiq_middleware + Sidekiq::Testing.server_middleware do |chain| + chain.remove Marginalia::SidekiqInstrumentation::Middleware + end + end + + def stub_feature(value) + stub_feature_flags(Gitlab::Marginalia::MARGINALIA_FEATURE_FLAG => value) + end + + def make_request(correlation_id) + request_env = Rack::MockRequest.env_for('/') + + ::Labkit::Correlation::CorrelationId.use_id(correlation_id) do + MarginaliaTestController.action(:first_user).call(request_env) + end + end + + describe 'For rails web requests' do + let(:correlation_id) { SecureRandom.uuid } + let(:recorded) { ActiveRecord::QueryRecorder.new { make_request(correlation_id) } } + + let(:component_map) do + { + "application" => "test", + "controller" => "marginalia_test", + "action" => "first_user", + "line" => "/spec/support/helpers/query_recorder.rb", + "correlation_id" => correlation_id + } + end + + context 'when the feature is enabled' do + before do + stub_feature(true) + end + + it 'generates a query that includes the component and value' do + component_map.each do |component, value| + expect(recorded.log.last).to include("#{component}:#{value}") + end + end + end + + context 'when the feature is disabled' do + before do + stub_feature(false) + end + + it 'excludes annotations in generated queries' do + expect(recorded.log.last).not_to include("/*") + expect(recorded.log.last).not_to include("*/") + end + end + end + + describe 'for Sidekiq worker jobs' do + before(:all) do + add_sidekiq_middleware + + # Because of faking, 'Sidekiq.server?' does not work so implicitly set application name which is done in config/initializers/0_marginalia.rb + Marginalia.application_name = "sidekiq" + end + + after(:all) do + MarginaliaTestJob.clear + remove_sidekiq_middleware + end + + around do |example| + Sidekiq::Testing.fake! { example.run } + end + + before do + MarginaliaTestJob.perform_async + end + + let(:sidekiq_job) { MarginaliaTestJob.jobs.first } + let(:recorded) { ActiveRecord::QueryRecorder.new { MarginaliaTestJob.drain } } + + let(:component_map) do + { + "application" => "sidekiq", + "job_class" => "MarginaliaTestJob", + "line" => "/spec/support/sidekiq_middleware.rb", + "correlation_id" => sidekiq_job['correlation_id'], + "jid" => sidekiq_job['jid'] + } + end + + context 'when the feature is enabled' do + before do + stub_feature(true) + end + + it 'generates a query that includes the component and value' do + component_map.each do |component, value| + expect(recorded.log.last).to include("#{component}:#{value}") + end + end + + describe 'for ActionMailer delivery jobs' do + let(:delivery_job) { MarginaliaTestMailer.first_user.deliver_later } + + let(:recorded) do + ActiveRecord::QueryRecorder.new do + delivery_job.perform_now + end + end + + let(:component_map) do + { + "application" => "sidekiq", + "line" => "/lib/gitlab/i18n.rb", + "jid" => delivery_job.job_id, + "job_class" => delivery_job.arguments.first + } + end + + it 'generates a query that includes the component and value' do + component_map.each do |component, value| + expect(recorded.log.last).to include("#{component}:#{value}") + end + end + end + end + + context 'when the feature is disabled' do + before do + stub_feature(false) + end + + it 'excludes annotations in generated queries' do + expect(recorded.log.last).not_to include("/*") + expect(recorded.log.last).not_to include("*/") + end + end + end +end diff --git a/spec/requests/api/projects_spec.rb b/spec/requests/api/projects_spec.rb index cda2dd7d2f4..4c1db4f0d18 100644 --- a/spec/requests/api/projects_spec.rb +++ b/spec/requests/api/projects_spec.rb @@ -570,6 +570,87 @@ describe API::Projects do let(:projects) { Project.all } end end + + context 'with keyset pagination' do + let(:current_user) { user } + let(:projects) { [public_project, project, project2, project3] } + + context 'headers and records' do + let(:params) { { pagination: 'keyset', order_by: :id, sort: :asc, per_page: 1 } } + + it 'includes a pagination header with link to the next page' do + get api('/projects', current_user), params: params + + expect(response.header).to include('Links') + expect(response.header['Links']).to include('pagination=keyset') + expect(response.header['Links']).to include("id_after=#{public_project.id}") + end + + it 'contains only the first project with per_page = 1' do + get api('/projects', current_user), params: params + + expect(response).to have_gitlab_http_status(200) + expect(json_response).to be_an Array + expect(json_response.map { |p| p['id'] }).to contain_exactly(public_project.id) + end + + it 'does not include a link if the end has reached and there is no more data' do + get api('/projects', current_user), params: params.merge(id_after: project2.id) + + expect(response.header).not_to include('Links') + end + + it 'responds with 501 if order_by is different from id' do + get api('/projects', current_user), params: params.merge(order_by: :created_at) + + expect(response).to have_gitlab_http_status(501) + end + end + + context 'with descending sorting' do + let(:params) { { pagination: 'keyset', order_by: :id, sort: :desc, per_page: 1 } } + + it 'includes a pagination header with link to the next page' do + get api('/projects', current_user), params: params + + expect(response.header).to include('Links') + expect(response.header['Links']).to include('pagination=keyset') + expect(response.header['Links']).to include("id_before=#{project3.id}") + end + + it 'contains only the last project with per_page = 1' do + get api('/projects', current_user), params: params + + expect(response).to have_gitlab_http_status(200) + expect(json_response).to be_an Array + expect(json_response.map { |p| p['id'] }).to contain_exactly(project3.id) + end + end + + context 'retrieving the full relation' do + let(:params) { { pagination: 'keyset', order_by: :id, sort: :desc, per_page: 2 } } + + it 'returns all projects' do + url = '/projects' + requests = 0 + ids = [] + + while url && requests <= 5 # circuit breaker + requests += 1 + get api(url, current_user), params: params + + links = response.header['Links'] + url = links&.match(/<[^>]+(\/projects\?[^>]+)>; rel="next"/) do |match| + match[1] + end + + ids += JSON.parse(response.body).map { |p| p['id'] } + end + + expect(ids).to contain_exactly(*projects.map(&:id)) + end + end + end end describe 'POST /projects' do diff --git a/spec/serializers/pipeline_serializer_spec.rb b/spec/serializers/pipeline_serializer_spec.rb index 7661c8acc13..f1f761a6fd0 100644 --- a/spec/serializers/pipeline_serializer_spec.rb +++ b/spec/serializers/pipeline_serializer_spec.rb @@ -130,10 +130,10 @@ describe PipelineSerializer do it 'preloads related merge requests' do recorded = ActiveRecord::QueryRecorder.new { subject } + expected_query = "SELECT \"merge_requests\".* FROM \"merge_requests\" " \ + "WHERE \"merge_requests\".\"id\" IN (#{merge_request_1.id}, #{merge_request_2.id})" - expect(recorded.log) - .to include("SELECT \"merge_requests\".* FROM \"merge_requests\" " \ - "WHERE \"merge_requests\".\"id\" IN (#{merge_request_1.id}, #{merge_request_2.id})") + expect(recorded.log).to include(a_string_starting_with(expected_query)) end end |