Commit 4f641864 authored by Rémy Coutable's avatar Rémy Coutable

Merge branch 'ld-rename-query_limiting_whitelist-to-allowlist' into 'master'

Rename QueryLimiting.whitelist to .disable! [RUN AS-IF-FOSS]

See merge request gitlab-org/gitlab!55717
parents c8f44dbc 32c45159
......@@ -11,7 +11,7 @@ class Admin::ApplicationSettingsController < Admin::ApplicationController
# https://gitlab.com/gitlab-org/gitlab-foss/-/merge_requests/30233
before_action :set_application_setting, except: :integrations
before_action :whitelist_query_limiting, only: [:usage_data]
before_action :disable_query_limiting, only: [:usage_data]
before_action only: [:ci_cd] do
push_frontend_feature_flag(:ci_instance_variables_ui, default_enabled: true)
......@@ -194,8 +194,8 @@ class Admin::ApplicationSettingsController < Admin::ApplicationController
@plans = Plan.all
end
def whitelist_query_limiting
Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-foss/issues/63107')
def disable_query_limiting
Gitlab::QueryLimiting.disable!('https://gitlab.com/gitlab-org/gitlab-foss/issues/63107')
end
def application_setting_params
......
......@@ -4,7 +4,7 @@ class Admin::ServicesController < Admin::ApplicationController
include ServiceParams
before_action :service, only: [:edit, :update]
before_action :whitelist_query_limiting, only: [:index]
before_action :disable_query_limiting, only: [:index]
feature_category :integrations
......@@ -39,7 +39,7 @@ class Admin::ServicesController < Admin::ApplicationController
end
# rubocop: enable CodeReuse/ActiveRecord
def whitelist_query_limiting
Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab/-/issues/220357')
def disable_query_limiting
Gitlab::QueryLimiting.disable!('https://gitlab.com/gitlab-org/gitlab/-/issues/220357')
end
end
......@@ -13,7 +13,7 @@ module Boards
requires_cross_project_access if: -> { board&.group_board? }
before_action :whitelist_query_limiting, only: [:bulk_move]
before_action :disable_query_limiting, only: [:bulk_move]
before_action :authorize_read_issue, only: [:index]
before_action :authorize_create_issue, only: [:create]
before_action :authorize_update_issue, only: [:update]
......@@ -147,8 +147,8 @@ module Boards
serializer.represent(resource, opts)
end
def whitelist_query_limiting
Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab/issues/35174')
def disable_query_limiting
Gitlab::QueryLimiting.disable!('https://gitlab.com/gitlab-org/gitlab/issues/35174')
end
def validate_id_list
......
......@@ -4,7 +4,8 @@ class GraphqlController < ApplicationController
# Unauthenticated users have access to the API for public data
skip_before_action :authenticate_user!
WHITELIST_HEADER = 'HTTP_X_GITLAB_QUERY_WHITELIST_ISSUE'
# Header can be passed by tests to disable SQL query limits.
DISABLE_SQL_QUERY_LIMIT_HEADER = 'HTTP_X_GITLAB_DISABLE_SQL_QUERY_LIMIT'
# If a user is using their session to access GraphQL, we need to have session
# storage, since the admin-mode check is session wide.
......@@ -23,7 +24,7 @@ class GraphqlController < ApplicationController
before_action(only: [:execute]) { authenticate_sessionless_user!(:api) }
before_action :set_user_last_activity
before_action :track_vs_code_usage
before_action :whitelist_query!
before_action :disable_query_limiting
# Since we deactivate authentication from the main ApplicationController and
# defer it to :authorize_access_api!, we need to override the bypass session
......@@ -62,12 +63,14 @@ class GraphqlController < ApplicationController
private
# Tests may mark some queries as exempt from query limits
def whitelist_query!
whitelist_issue = request.headers[WHITELIST_HEADER]
return unless whitelist_issue
# Tests may mark some GraphQL queries as exempt from SQL query limits
def disable_query_limiting
return unless Gitlab::QueryLimiting.enabled_for_env?
Gitlab::QueryLimiting.whitelist(whitelist_issue)
disable_issue = request.headers[DISABLE_SQL_QUERY_LIMIT_HEADER]
return unless disable_issue
Gitlab::QueryLimiting.disable!(disable_issue)
end
def set_user_last_activity
......
......@@ -3,7 +3,7 @@
class Import::GitlabProjectsController < Import::BaseController
include WorkhorseAuthorization
before_action :whitelist_query_limiting, only: [:create]
before_action :disable_query_limiting, only: [:create]
before_action :verify_gitlab_project_import_enabled
def new
......@@ -42,8 +42,8 @@ class Import::GitlabProjectsController < Import::BaseController
)
end
def whitelist_query_limiting
Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-foss/issues/42437')
def disable_query_limiting
Gitlab::QueryLimiting.disable!('https://gitlab.com/gitlab-org/gitlab-foss/issues/42437')
end
def uploader_class
......
......@@ -3,7 +3,7 @@
class Import::ManifestController < Import::BaseController
extend ::Gitlab::Utils::Override
before_action :whitelist_query_limiting, only: [:create]
before_action :disable_query_limiting, only: [:create]
before_action :verify_import_enabled
before_action :ensure_import_vars, only: [:create, :status]
......@@ -115,7 +115,7 @@ class Import::ManifestController < Import::BaseController
render_404 unless manifest_import_enabled?
end
def whitelist_query_limiting
Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-foss/issues/48939')
def disable_query_limiting
Gitlab::QueryLimiting.disable!('https://gitlab.com/gitlab-org/gitlab-foss/issues/48939')
end
end
......@@ -8,7 +8,7 @@ class Projects::CommitsController < Projects::ApplicationController
prepend_before_action(only: [:show]) { authenticate_sessionless_user!(:rss) }
around_action :allow_gitaly_ref_name_caching
before_action :whitelist_query_limiting, except: :commits_root
before_action :disable_query_limiting, except: :commits_root
before_action :require_non_empty_project
before_action :assign_ref_vars, except: :commits_root
before_action :authorize_download_code!
......@@ -83,7 +83,7 @@ class Projects::CommitsController < Projects::ApplicationController
@commits = set_commits_for_rendering(@commits)
end
def whitelist_query_limiting
Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-foss/issues/42330')
def disable_query_limiting
Gitlab::QueryLimiting.disable!('https://gitlab.com/gitlab-org/gitlab-foss/issues/42330')
end
end
......@@ -7,7 +7,7 @@ class Projects::ForksController < Projects::ApplicationController
include Gitlab::Utils::StrongMemoize
# Authorize
before_action :whitelist_query_limiting, only: [:create]
before_action :disable_query_limiting, only: [:create]
before_action :require_non_empty_project
before_action :authorize_download_code!
before_action :authenticate_user!, only: [:new, :create]
......@@ -110,8 +110,8 @@ class Projects::ForksController < Projects::ApplicationController
access_denied! unless fork_namespace && fork_service.valid_fork_target?
end
def whitelist_query_limiting
Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-foss/issues/42335')
def disable_query_limiting
Gitlab::QueryLimiting.disable!('https://gitlab.com/gitlab-org/gitlab-foss/issues/42335')
end
def load_namespaces_with_associations
......
......@@ -18,7 +18,7 @@ class Projects::IssuesController < Projects::ApplicationController
prepend_before_action :authenticate_user!, only: [:new, :export_csv]
prepend_before_action :store_uri, only: [:new, :show, :designs]
before_action :whitelist_query_limiting, only: [:create, :create_merge_request, :move, :bulk_update]
before_action :disable_query_limiting, only: [:create, :create_merge_request, :move, :bulk_update]
before_action :check_issues_available!
before_action :issue, unless: ->(c) { ISSUES_EXCEPT_ACTIONS.include?(c.action_name.to_sym) }
after_action :log_issue_show, unless: ->(c) { ISSUES_EXCEPT_ACTIONS.include?(c.action_name.to_sym) }
......@@ -353,13 +353,13 @@ class Projects::IssuesController < Projects::ApplicationController
IssuesFinder
end
def whitelist_query_limiting
def disable_query_limiting
# Also see the following issues:
#
# 1. https://gitlab.com/gitlab-org/gitlab-foss/issues/42423
# 2. https://gitlab.com/gitlab-org/gitlab-foss/issues/42424
# 3. https://gitlab.com/gitlab-org/gitlab-foss/issues/42426
Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-foss/issues/42422')
Gitlab::QueryLimiting.disable!('https://gitlab.com/gitlab-org/gitlab-foss/issues/42422')
end
private
......
......@@ -6,7 +6,7 @@ class Projects::MergeRequests::CreationsController < Projects::MergeRequests::Ap
include RendersCommits
skip_before_action :merge_request
before_action :whitelist_query_limiting, only: [:create]
before_action :disable_query_limiting, only: [:create]
before_action :authorize_create_merge_request_from!
before_action :apply_diff_view_cookie!, only: [:diffs, :diff_for_path]
before_action :build_merge_request, except: [:create]
......@@ -133,8 +133,8 @@ class Projects::MergeRequests::CreationsController < Projects::MergeRequests::Ap
end
# rubocop: enable CodeReuse/ActiveRecord
def whitelist_query_limiting
Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-foss/issues/42384')
def disable_query_limiting
Gitlab::QueryLimiting.disable!('https://gitlab.com/gitlab-org/gitlab-foss/issues/42384')
end
def incr_count_webide_merge_request
......
......@@ -14,7 +14,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
skip_before_action :merge_request, only: [:index, :bulk_update, :export_csv]
before_action :apply_diff_view_cookie!, only: [:show]
before_action :whitelist_query_limiting, only: [:assign_related_issues, :update]
before_action :disable_query_limiting, only: [:assign_related_issues, :update]
before_action :authorize_update_issuable!, only: [:close, :edit, :update, :remove_wip, :sort]
before_action :authorize_read_actual_head_pipeline!, only: [
:test_reports,
......@@ -468,9 +468,9 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
access_denied! unless @merge_request.can_be_merged_by?(current_user)
end
def whitelist_query_limiting
def disable_query_limiting
# Also see https://gitlab.com/gitlab-org/gitlab-foss/issues/42441
Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-foss/issues/42438')
Gitlab::QueryLimiting.disable!('https://gitlab.com/gitlab-org/gitlab-foss/issues/42438')
end
def reports_response(report_comparison, pipeline = nil)
......
......@@ -4,7 +4,7 @@ class Projects::NetworkController < Projects::ApplicationController
include ExtractsPath
include ApplicationHelper
before_action :whitelist_query_limiting
before_action :disable_query_limiting
before_action :require_non_empty_project
before_action :assign_ref_vars
before_action :authorize_download_code!
......@@ -42,7 +42,7 @@ class Projects::NetworkController < Projects::ApplicationController
@commit = @repo.commit(@options[:extended_sha1])
end
def whitelist_query_limiting
Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-foss/issues/42333')
def disable_query_limiting
Gitlab::QueryLimiting.disable!('https://gitlab.com/gitlab-org/gitlab-foss/issues/42333')
end
end
......@@ -6,7 +6,7 @@ class Projects::NotesController < Projects::ApplicationController
include NotesHelper
include ToggleAwardEmoji
before_action :whitelist_query_limiting, only: [:create, :update]
before_action :disable_query_limiting, only: [:create, :update]
before_action :authorize_read_note!
before_action :authorize_create_note!, only: [:create]
before_action :authorize_resolve_note!, only: [:resolve, :unresolve]
......@@ -87,7 +87,7 @@ class Projects::NotesController < Projects::ApplicationController
access_denied! unless can?(current_user, :create_note, noteable)
end
def whitelist_query_limiting
Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-foss/issues/42383')
def disable_query_limiting
Gitlab::QueryLimiting.disable!('https://gitlab.com/gitlab-org/gitlab-foss/issues/42383')
end
end
......@@ -4,7 +4,7 @@ class Projects::PipelinesController < Projects::ApplicationController
include ::Gitlab::Utils::StrongMemoize
include Analytics::UniqueVisitsHelper
before_action :whitelist_query_limiting, only: [:create, :retry]
before_action :disable_query_limiting, only: [:create, :retry]
before_action :pipeline, except: [:index, :new, :create, :charts, :config_variables]
before_action :set_pipeline_path, only: [:show]
before_action :authorize_read_pipeline!
......@@ -92,7 +92,7 @@ class Projects::PipelinesController < Projects::ApplicationController
end
def show
Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab/-/issues/26657')
Gitlab::QueryLimiting.disable!('https://gitlab.com/gitlab-org/gitlab/-/issues/26657')
respond_to do |format|
format.html
......@@ -269,9 +269,9 @@ class Projects::PipelinesController < Projects::ApplicationController
&.present(current_user: current_user)
end
def whitelist_query_limiting
def disable_query_limiting
# Also see https://gitlab.com/gitlab-org/gitlab-foss/issues/42343
Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-foss/issues/42339')
Gitlab::QueryLimiting.disable!('https://gitlab.com/gitlab-org/gitlab-foss/issues/42339')
end
def authorize_update_pipeline!
......
......@@ -14,7 +14,7 @@ class ProjectsController < Projects::ApplicationController
around_action :allow_gitaly_ref_name_caching, only: [:index, :show]
before_action :whitelist_query_limiting, only: [:show, :create]
before_action :disable_query_limiting, only: [:show, :create]
before_action :authenticate_user!, except: [:index, :show, :activity, :refs, :resolve, :unfoldered_environment_names]
before_action :redirect_git_extension, only: [:show]
before_action :project, except: [:index, :new, :create, :resolve]
......@@ -510,8 +510,8 @@ class ProjectsController < Projects::ApplicationController
redirect_to(request.original_url.sub(%r{\.git/?\Z}, ''))
end
def whitelist_query_limiting
Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab/-/issues/20826')
def disable_query_limiting
Gitlab::QueryLimiting.disable!('https://gitlab.com/gitlab-org/gitlab/-/issues/20826')
end
def present_project
......
......@@ -9,7 +9,7 @@ class RegistrationsController < Devise::RegistrationsController
layout 'devise'
prepend_before_action :check_captcha, only: :create
before_action :whitelist_query_limiting, :ensure_destroy_prerequisites_met, only: [:destroy]
before_action :disable_query_limiting, :ensure_destroy_prerequisites_met, only: [:destroy]
before_action :load_recaptcha, only: :new
before_action :set_invite_params, only: :new
......@@ -162,8 +162,8 @@ class RegistrationsController < Devise::RegistrationsController
@devise_mapping ||= Devise.mappings[:user]
end
def whitelist_query_limiting
Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-foss/issues/42380')
def disable_query_limiting
Gitlab::QueryLimiting.disable!('https://gitlab.com/gitlab-org/gitlab-foss/issues/42380')
end
def load_recaptcha
......
......@@ -53,7 +53,7 @@ module Mutations
end
def resolve(board:, **args)
Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab/-/issues/247861')
Gitlab::QueryLimiting.disable!('https://gitlab.com/gitlab-org/gitlab/-/issues/247861')
raise_resource_not_available_error! unless board
authorize_board!(board)
......
......@@ -19,7 +19,7 @@ module Mutations
def resolve(project_path:, iid:, assignee_usernames:, operation_mode: Types::MutationOperationModeEnum.enum[:replace])
resource = authorized_find!(project_path: project_path, iid: iid)
Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab/issues/36098') if resource.is_a?(MergeRequest)
Gitlab::QueryLimiting.disable!('https://gitlab.com/gitlab-org/gitlab/issues/36098') if resource.is_a?(MergeRequest)
update_service_class.new(
resource.project,
......
......@@ -11,7 +11,7 @@ module Mutations
description: 'The project to move the issue to.'
def resolve(project_path:, iid:, target_project_path:)
Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab/-/issues/267762')
Gitlab::QueryLimiting.disable!('https://gitlab.com/gitlab-org/gitlab/-/issues/267762')
issue = authorized_find!(project_path: project_path, iid: iid)
source_project = issue.project
......
......@@ -42,7 +42,8 @@ module Mutations
description: 'Squash commits on the source branch before merge.'
def resolve(project_path:, iid:, **args)
Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-foss/issues/42317')
Gitlab::QueryLimiting.disable!('https://gitlab.com/gitlab-org/gitlab-foss/issues/42317')
merge_request = authorized_find!(project_path: project_path, iid: iid)
project = merge_request.target_project
merge_params = args.compact.with_indifferent_access
......
......@@ -48,7 +48,7 @@ class PostReceiveService
end
def process_mr_push_options(push_options, changes)
Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-foss/issues/61359')
Gitlab::QueryLimiting.disable!('https://gitlab.com/gitlab-org/gitlab-foss/issues/61359')
return unless repository
unless repository.repo_type.project?
......
# frozen_string_literal: true
if Gitlab::QueryLimiting.enable?
if Gitlab::QueryLimiting.enabled_for_env?
require_dependency 'gitlab/query_limiting/active_support_subscriber'
require_dependency 'gitlab/query_limiting/transaction'
require_dependency 'gitlab/query_limiting/middleware'
......
......@@ -15,30 +15,30 @@ When a test fails because it executes more than 100 SQL queries there are two
solutions to this problem:
- Reduce the number of SQL queries that are executed.
- Whitelist the controller or API endpoint.
- Disable query limiting for the controller or API endpoint.
You should only resort to whitelisting when an existing controller or endpoint
You should only resort to disabling query limits when an existing controller or endpoint
is to blame as in this case reducing the number of SQL queries can take a lot of
effort. Newly added controllers and endpoints are not allowed to execute more
than 100 SQL queries and no exceptions are made for this rule. _If_ a large
number of SQL queries is necessary to perform certain work it's best to have
this work performed by Sidekiq instead of doing this directly in a web request.
## Whitelisting
## Disable query limiting
In the event that you _have_ to whitelist a controller you must first
In the event that you _have_ to disable query limits for a controller, you must first
create an issue. This issue should (preferably in the title) mention the
controller or endpoint and include the appropriate labels (`database`,
`performance`, and at least a team specific label such as `Discussion`).
After the issue has been created you can whitelist the code in question. For
After the issue has been created, you can disable query limits on the code in question. For
Rails controllers it's best to create a `before_action` hook that runs as early
as possible. The called method in turn should call
`Gitlab::QueryLimiting.whitelist('issue URL here')`. For example:
`Gitlab::QueryLimiting.disable!('issue URL here')`. For example:
```ruby
class MyController < ApplicationController
before_action :whitelist_query_limiting, only: [:show]
before_action :disable_query_limiting, only: [:show]
def index
# ...
......@@ -48,8 +48,8 @@ class MyController < ApplicationController
# ...
end
def whitelist_query_limiting
Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/...')
def disable_query_limiting
Gitlab::QueryLimiting.disable!('https://gitlab.com/gitlab-org/...')
end
end
```
......@@ -63,7 +63,7 @@ call directly into the endpoint like so:
```ruby
get '/projects/:id/foo' do
Gitlab::QueryLimiting.whitelist('...')
Gitlab::QueryLimiting.disable!('...')
# ...
end
......
......@@ -9,8 +9,7 @@ module EE
prepended do
include DescriptionDiffActions
before_action :whitelist_query_limiting_ee, only: [:update]
before_action :disable_query_limiting_ee, only: [:update]
before_action only: [:new, :create] do
populate_vulnerability_id
end
......@@ -43,8 +42,8 @@ module EE
options.reject { |key| key == 'weight' }
end
def whitelist_query_limiting_ee
::Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab/issues/4794')
def disable_query_limiting_ee
::Gitlab::QueryLimiting.disable!('https://gitlab.com/gitlab-org/gitlab/issues/4794')
end
def issue_params
......
......@@ -16,7 +16,7 @@ module EE
push_frontend_feature_flag(:usage_data_i_testing_load_performance_widget_total, @project, default_enabled: true)
end
before_action :whitelist_query_limiting_ee_merge, only: [:merge]
before_action :disable_query_limiting_ee_merge, only: [:merge]
before_action :authorize_read_pipeline!, only: [:container_scanning_reports, :dependency_scanning_reports,
:sast_reports, :secret_detection_reports, :dast_reports,
:metrics_reports, :coverage_fuzzing_reports,
......@@ -64,8 +64,8 @@ module EE
private
def whitelist_query_limiting_ee_merge
::Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab/issues/4792')
def disable_query_limiting_ee_merge
::Gitlab::QueryLimiting.disable!('https://gitlab.com/gitlab-org/gitlab/issues/4792')
end
end
end
......
......@@ -115,7 +115,7 @@ module API
at_least_one_of :title, :description, :start_date_fixed, :start_date_is_fixed, :due_date_fixed, :due_date_is_fixed, :labels, :add_labels, :remove_labels, :state_event, :confidential
end
put ':id/(-/)epics/:epic_iid' do
Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab/issues/194104')
Gitlab::QueryLimiting.disable!('https://gitlab.com/gitlab-org/gitlab/issues/194104')
authorize_can_admin_epic!
......
......@@ -90,7 +90,7 @@ module EE
desc: 'Array of Group IDs to set as approvers.'
end
put 'approvers' do
::Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab/issues/8883')
::Gitlab::QueryLimiting.disable!('https://gitlab.com/gitlab-org/gitlab/issues/8883')
merge_request = find_merge_request_with_access(params[:merge_request_iid], :update_approvers)
......
......@@ -70,7 +70,7 @@ module API
optional :variables, Array, desc: 'Array of variables available in the pipeline'
end
post ':id/pipeline' do
Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-foss/issues/42124')
Gitlab::QueryLimiting.disable!('https://gitlab.com/gitlab-org/gitlab-foss/issues/42124')
authorize! :create_pipeline, user_project
......
......@@ -112,7 +112,7 @@ module API
end
def delete_group(group)
Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-foss/issues/46285')
Gitlab::QueryLimiting.disable!('https://gitlab.com/gitlab-org/gitlab-foss/issues/46285')
destroy_conditionally!(group) do |group|
::Groups::DestroyService.new(group, current_user).async_execute
end
......
......@@ -116,7 +116,7 @@ module API
end
def create_note(noteable, opts)
whitelist_query_limiting
disable_query_limiting
authorize!(:create_note, noteable)
parent = noteable_parent(noteable)
......@@ -144,8 +144,8 @@ module API
present discussion, with: Entities::Discussion
end
def whitelist_query_limiting
Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab/-/issues/211538')
def disable_query_limiting
Gitlab::QueryLimiting.disable!('https://gitlab.com/gitlab-org/gitlab/-/issues/211538')
end
end
end
......
......@@ -242,7 +242,7 @@ module API
use :issue_params
end
post ':id/issues' do
Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-foss/issues/42320')
Gitlab::QueryLimiting.disable!('https://gitlab.com/gitlab-org/gitlab-foss/issues/42320')
check_rate_limit! :issues_create, [current_user]
......@@ -288,7 +288,7 @@ module API
end
# rubocop: disable CodeReuse/ActiveRecord
put ':id/issues/:issue_iid' do
Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-foss/issues/42322')
Gitlab::QueryLimiting.disable!('https://gitlab.com/gitlab-org/gitlab-foss/issues/42322')
issue = user_project.issues.find_by!(iid: params.delete(:issue_iid))
authorize! :update_issue, issue
......@@ -346,7 +346,7 @@ module API
end
# rubocop: disable CodeReuse/ActiveRecord
post ':id/issues/:issue_iid/move' do
Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-foss/issues/42323')
Gitlab::QueryLimiting.disable!('https://gitlab.com/gitlab-org/gitlab-foss/issues/42323')
issue = user_project.issues.find_by(iid: params[:issue_iid])
not_found!('Issue') unless issue
......
......@@ -207,7 +207,7 @@ module API
use :optional_params
end
post ":id/merge_requests" do
Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-foss/issues/42316')
Gitlab::QueryLimiting.disable!('https://gitlab.com/gitlab-org/gitlab-foss/issues/42316')
authorize! :create_merge_request_from, user_project
......@@ -416,7 +416,7 @@ module API
at_least_one_of(*::API::MergeRequests.update_params_at_least_one_of)
end
put ':id/merge_requests/:merge_request_iid' do
Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-foss/issues/42318')
Gitlab::QueryLimiting.disable!('https://gitlab.com/gitlab-org/gitlab-foss/issues/42318')
merge_request = find_merge_request_with_access(params.delete(:merge_request_iid), :update_merge_request)
......@@ -445,7 +445,7 @@ module API
optional :squash, type: Grape::API::Boolean, desc: 'When true, the commits will be squashed into a single commit on merge'
end
put ':id/merge_requests/:merge_request_iid/merge' do
Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-foss/issues/42317')
Gitlab::QueryLimiting.disable!('https://gitlab.com/gitlab-org/gitlab-foss/issues/42317')
merge_request = find_project_merge_request(params[:merge_request_iid])
......
......@@ -67,7 +67,7 @@ module API
check_rate_limit! :project_import, [current_user, :project_import]
Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-foss/issues/42437')
Gitlab::QueryLimiting.disable!('https://gitlab.com/gitlab-org/gitlab-foss/issues/42437')
validate_file!
......
......@@ -215,7 +215,7 @@ module API
use :create_params
end
post do
Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab/issues/21139')
Gitlab::QueryLimiting.disable!('https://gitlab.com/gitlab-org/gitlab/issues/21139')
attrs = declared_params(include_missing: false)
attrs = translate_params_for_compatibility(attrs)
filter_attributes_using_license!(attrs)
......@@ -248,7 +248,7 @@ module API
end
# rubocop: disable CodeReuse/ActiveRecord
post "user/:user_id", feature_category: :projects do
Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab/issues/21139')
Gitlab::QueryLimiting.disable!('https://gitlab.com/gitlab-org/gitlab/issues/21139')
authenticated_as_admin!
user = User.find_by(id: params.delete(:user_id))
not_found!('User') unless user
......@@ -310,7 +310,7 @@ module API
optional :visibility, type: String, values: Gitlab::VisibilityLevel.string_values, desc: 'The visibility of the fork'
end
post ':id/fork', feature_category: :source_code_management do
Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-foss/issues/42284')
Gitlab::QueryLimiting.disable!('https://gitlab.com/gitlab-org/gitlab-foss/issues/42284')
not_found! unless can?(current_user, :fork_project, user_project)
......
......@@ -21,7 +21,7 @@ module API
optional :variables, type: Hash, desc: 'The list of variables to be injected into build'
end
post ":id/(ref/:ref/)trigger/pipeline", requirements: { ref: /.+/ } do
Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-foss/issues/42283')
Gitlab::QueryLimiting.disable!('https://gitlab.com/gitlab-org/gitlab-foss/issues/42283')
forbidden! if gitlab_pipeline_hook_request?
......
......@@ -571,7 +571,7 @@ module API
end
# rubocop: disable CodeReuse/ActiveRecord
delete ":id", feature_category: :users do
Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab/issues/20757')
Gitlab::QueryLimiting.disable!('https://gitlab.com/gitlab-org/gitlab/issues/20757')
authenticated_as_admin!
......
......@@ -6,28 +6,36 @@ module Gitlab
#
# This is only enabled in development and test to ensure we don't produce
# any errors that users of other environments can't do anything about themselves.
def self.enable?
def self.enabled_for_env?
Rails.env.development? || Rails.env.test?
end
def self.enabled?
enabled_for_env? &&
!Gitlab::SafeRequestStore[:query_limiting_disabled]
end
# Allows the current request to execute any number of SQL queries.
#
# This method should _only_ be used when there's a corresponding issue to
# reduce the number of queries.
#
# The issue URL is only meant to push developers into creating an issue
# instead of blindly whitelisting offending blocks of code.
def self.whitelist(issue_url)
return unless enable?
# instead of blindly disabling for offending blocks of code.
def self.disable!(issue_url)
unless issue_url.start_with?('https://')
raise(
ArgumentError,
'You must provide a valid issue URL in order to whitelist a block of code'
'You must provide a valid issue URL in order to allow a block of code'
)
end
Transaction&.current&.whitelisted = true
Gitlab::SafeRequestStore[:query_limiting_disabled] = true
end
# Enables query limiting for the request.
def self.enable!
Gitlab::SafeRequestStore[:query_limiting_disabled] = nil
end
end
end
......@@ -5,7 +5,7 @@ module Gitlab
class Transaction
THREAD_KEY = :__gitlab_query_counts_transaction
attr_accessor :count, :whitelisted
attr_accessor :count
# The name of the action (e.g. `UsersController#show`) that is being
# executed.
......@@ -45,7 +45,6 @@ module Gitlab
def initialize
@action = nil
@count = 0
@whitelisted = false
@sql_executed = []
end
......@@ -59,7 +58,7 @@ module Gitlab
end
def increment
@count += 1 unless whitelisted
@count += 1 if enabled?
end
def executed_sql(sql)
......@@ -83,6 +82,10 @@ module Gitlab
["#{header}: #{msg}", log, ellipsis].compact.join("\n")
end
def enabled?
::Gitlab::QueryLimiting.enabled?
end
end
end
end
......@@ -68,11 +68,15 @@ RSpec.describe Gitlab::QueryLimiting::Transaction do
it 'increments the number of executed queries' do
transaction = described_class.new
expect(transaction.count).to be_zero
expect { transaction.increment }.to change { transaction.count }.by(1)
end
it 'does not increment the number of executed queries when query limiting is disabled' do
transaction = described_class.new
transaction.increment
allow(transaction).to receive(:enabled?).and_return(false)
expect(transaction.count).to eq(1)
expect { transaction.increment }.not_to change { transaction.count }
end
end
......
......@@ -2,81 +2,85 @@
require 'spec_helper'
RSpec.describe Gitlab::QueryLimiting do
describe '.enable?' do
RSpec.describe Gitlab::QueryLimiting, :request_store do
describe '.enabled_for_env?' do
it 'returns true in a test environment' do
expect(described_class.enable?).to eq(true)
expect(described_class.enabled_for_env?).to eq(true)
end
it 'returns true in a development environment' do
stub_rails_env('development')
stub_rails_env('development')
expect(described_class.enable?).to eq(true)
expect(described_class.enabled_for_env?).to eq(true)
end
it 'returns false on GitLab.com' do
stub_rails_env('production')
allow(Gitlab).to receive(:com?).and_return(true)
expect(described_class.enable?).to eq(false)
expect(described_class.enabled_for_env?).to eq(false)
end
it 'returns false in a non GitLab.com' do
allow(Gitlab).to receive(:com?).and_return(false)
stub_rails_env('production')
expect(described_class.enable?).to eq(false)
expect(described_class.enabled_for_env?).to eq(false)
end
end
describe '.whitelist' do
it 'raises ArgumentError when an invalid issue URL is given' do
expect { described_class.whitelist('foo') }
.to raise_error(ArgumentError)
shared_context 'disable and enable' do |result|
let(:transaction) { Gitlab::QueryLimiting::Transaction.new }
let(:code) do
proc do
2.times { User.count }
end
end
context 'without a transaction' do
it 'does nothing' do
expect { described_class.whitelist('https://example.com') }
.not_to raise_error
end
before do
allow(Gitlab::QueryLimiting::Transaction)
.to receive(:current)
.and_return(transaction)
end
end
context 'with a transaction' do
let(:transaction) { Gitlab::QueryLimiting::Transaction.new }
describe '.disable!' do
include_context 'disable and enable'
before do
allow(Gitlab::QueryLimiting::Transaction)
.to receive(:current)
.and_return(transaction)
end
it 'raises an ArgumentError when an invalid issue URL is given' do
expect { described_class.disable!('foo') }
.to raise_error(ArgumentError)
end
it 'does not increment the number of SQL queries executed in the block' do
before = transaction.count
it 'stops the number of SQL queries from being incremented' do
described_class.disable!('https://example.com')
described_class.whitelist('https://example.com')
expect { code.call }.not_to change { transaction.count }
end
end
2.times do
User.count
end
describe '.enable!' do
include_context 'disable and enable'
expect(transaction.count).to eq(before)
end
it 'allows the number of SQL queries to be incremented' do
described_class.enable!
it 'whitelists when enabled' do
described_class.whitelist('https://example.com')
expect { code.call }.to change { transaction.count }.by(2)
end
end
expect(transaction.whitelisted).to eq(true)
end
describe '#enabled?' do
it 'returns true when enabled' do
Gitlab::SafeRequestStore[:query_limiting_disabled] = nil
it 'does not whitelist when disabled' do
allow(described_class).to receive(:enable?).and_return(false)
expect(described_class).to be_enabled
end
described_class.whitelist('https://example.com')
it 'returns false when disabled' do
Gitlab::SafeRequestStore[:query_limiting_disabled] = true
expect(transaction.whitelisted).to eq(false)
end
expect(described_class).not_to be_enabled
end
end
end
......@@ -47,10 +47,10 @@ RSpec.describe 'getting merge request listings nested in a project' do
end
before do
# We cannot call the whitelist here, since the transaction does not
# We cannot disable SQL query limiting here, since the transaction does not
# begin until we enter the controller.
headers = {
'X-GITLAB-QUERY-WHITELIST-ISSUE' => 'https://gitlab.com/gitlab-org/gitlab/-/issues/322979'
'X-GITLAB-DISABLE-SQL-QUERY-LIMIT' => 'https://gitlab.com/gitlab-org/gitlab/-/issues/322979'
}
post_graphql(query, current_user: current_user, headers: headers)
......
......@@ -359,6 +359,9 @@ RSpec.configure do |config|
# Reset all feature flag stubs to default for testing
stub_all_feature_flags
# Re-enable query limiting in case it was disabled
Gitlab::QueryLimiting.enable!
end
config.before(:example, :mailer) do
......
......@@ -15,20 +15,14 @@ end
# If Sidekiq::Testing.inline! is used, SQL transactions done inside
# Sidekiq worker are included in the SQL query limit (in a real
# deployment sidekiq worker is executed separately). To avoid
# increasing SQL limit counter, the request is marked as whitelisted
# during Sidekiq block
# deployment sidekiq worker is executed separately). To avoid increasing
# SQL limit counter, query limiting is disabled during Sidekiq block
class DisableQueryLimit
def call(worker_instance, msg, queue)
transaction = Gitlab::QueryLimiting::Transaction.current
if !transaction.respond_to?(:whitelisted) || transaction.whitelisted
yield
else
transaction.whitelisted = true
yield
transaction.whitelisted = false
end
::Gitlab::QueryLimiting.disable!('https://mock-issue')
yield
ensure
::Gitlab::QueryLimiting.enable!
end
end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment