diff options
58 files changed, 884 insertions, 69 deletions
diff --git a/.flayignore b/.flayignore index acac0ce14c9..87cb3507b05 100644 --- a/.flayignore +++ b/.flayignore @@ -6,3 +6,5 @@ app/models/concerns/relative_positioning.rb app/workers/stuck_merge_jobs_worker.rb lib/gitlab/redis/*.rb lib/gitlab/gitaly_client/operation_service.rb +lib/gitlab/background_migration/* +app/models/project_services/kubernetes_service.rb diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index b4afa953175..9c3556f5cce 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -627,6 +627,8 @@ codequality: sast: <<: *except-docs image: registry.gitlab.com/gitlab-org/gl-sast:latest + variables: + CONFIDENCE_LEVEL: 2 before_script: [] script: - /app/bin/run . diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index b366ae6f069..ed56da0353d 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -50,6 +50,9 @@ _This notice should stay as the first item in the CONTRIBUTING.md file._ ## Contribute to GitLab +For a first-time step-by-step guide to the contribution process, see +["Contributing to GitLab"](https://about.gitlab.com/contributing/). + Thank you for your interest in contributing to GitLab. This guide details how to contribute to GitLab in a way that is efficient for everyone. diff --git a/GITALY_SERVER_VERSION b/GITALY_SERVER_VERSION index 4a7076db09a..bd14e8533ef 100644 --- a/GITALY_SERVER_VERSION +++ b/GITALY_SERVER_VERSION @@ -1 +1 @@ -0.77.0 +0.78.0 @@ -349,7 +349,7 @@ group :development, :test do gem 'scss_lint', '~> 0.56.0', require: false gem 'haml_lint', '~> 0.26.0', require: false gem 'simplecov', '~> 0.14.0', require: false - gem 'flay', '~> 2.8.0', require: false + gem 'flay', '~> 2.10.0', require: false gem 'bundler-audit', '~> 0.5.0', require: false gem 'benchmark-ips', '~> 2.3.0', require: false diff --git a/Gemfile.lock b/Gemfile.lock index 2ddf8221a06..cf9f160499d 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -211,7 +211,7 @@ GEM fast_gettext (1.4.0) ffaker (2.4.0) ffi (1.9.18) - flay (2.8.1) + flay (2.10.0) erubis (~> 2.7.0) path_expander (~> 1.0) ruby_parser (~> 3.0) @@ -588,7 +588,7 @@ GEM ast (~> 2.3) parslet (1.5.0) blankslate (~> 2.0) - path_expander (1.0.1) + path_expander (1.0.2) peek (1.0.1) concurrent-ruby (>= 0.9.0) concurrent-ruby-ext (>= 0.9.0) @@ -1037,7 +1037,7 @@ DEPENDENCIES faraday (~> 0.12) fast_blank ffaker (~> 2.4) - flay (~> 2.8.0) + flay (~> 2.10.0) flipper (~> 0.11.0) flipper-active_record (~> 0.11.0) flipper-active_support_cache_store (~> 0.11.0) diff --git a/app/controllers/admin/services_controller.rb b/app/controllers/admin/services_controller.rb index 4c3d336b3af..a7025b62ad7 100644 --- a/app/controllers/admin/services_controller.rb +++ b/app/controllers/admin/services_controller.rb @@ -1,6 +1,7 @@ class Admin::ServicesController < Admin::ApplicationController include ServiceParams + before_action :whitelist_query_limiting, only: [:index] before_action :service, only: [:edit, :update] def index @@ -37,4 +38,8 @@ class Admin::ServicesController < Admin::ApplicationController def service @service ||= Service.where(id: params[:id], template: true).first end + + def whitelist_query_limiting + Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-ce/issues/42430') + end end diff --git a/app/controllers/boards/issues_controller.rb b/app/controllers/boards/issues_controller.rb index f8049b20b9f..ee23ee0bcc3 100644 --- a/app/controllers/boards/issues_controller.rb +++ b/app/controllers/boards/issues_controller.rb @@ -2,6 +2,7 @@ module Boards class IssuesController < Boards::ApplicationController include BoardsResponses + before_action :whitelist_query_limiting, only: [:index, :update] before_action :authorize_read_issue, only: [:index] before_action :authorize_create_issue, only: [:create] before_action :authorize_update_issue, only: [:update] @@ -92,5 +93,10 @@ module Boards } ) end + + def whitelist_query_limiting + # Also see https://gitlab.com/gitlab-org/gitlab-ce/issues/42439 + Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-ce/issues/42428') + end end end diff --git a/app/controllers/import/gitlab_projects_controller.rb b/app/controllers/import/gitlab_projects_controller.rb index 567957ba2cb..f22df992fe9 100644 --- a/app/controllers/import/gitlab_projects_controller.rb +++ b/app/controllers/import/gitlab_projects_controller.rb @@ -1,4 +1,5 @@ class Import::GitlabProjectsController < Import::BaseController + before_action :whitelist_query_limiting, only: [:create] before_action :verify_gitlab_project_import_enabled def new @@ -40,4 +41,8 @@ class Import::GitlabProjectsController < Import::BaseController :path, :namespace_id, :file ) end + + def whitelist_query_limiting + Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-ce/issues/42437') + end end diff --git a/app/controllers/projects/commits_controller.rb b/app/controllers/projects/commits_controller.rb index 0a40c67368f..1d910e461b1 100644 --- a/app/controllers/projects/commits_controller.rb +++ b/app/controllers/projects/commits_controller.rb @@ -4,6 +4,7 @@ class Projects::CommitsController < Projects::ApplicationController include ExtractsPath include RendersCommits + before_action :whitelist_query_limiting before_action :require_non_empty_project before_action :assign_ref_vars before_action :authorize_download_code! @@ -65,4 +66,8 @@ class Projects::CommitsController < Projects::ApplicationController @commits = @commits.with_pipeline_status @commits = prepare_commits_for_rendering(@commits) end + + def whitelist_query_limiting + Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-ce/issues/42330') + end end diff --git a/app/controllers/projects/cycle_analytics_controller.rb b/app/controllers/projects/cycle_analytics_controller.rb index 88ac3ad046b..d1b8fd80c4e 100644 --- a/app/controllers/projects/cycle_analytics_controller.rb +++ b/app/controllers/projects/cycle_analytics_controller.rb @@ -3,6 +3,7 @@ class Projects::CycleAnalyticsController < Projects::ApplicationController include ActionView::Helpers::TextHelper include CycleAnalyticsParams + before_action :whitelist_query_limiting, only: [:show] before_action :authorize_read_cycle_analytics! def show @@ -31,4 +32,8 @@ class Projects::CycleAnalyticsController < Projects::ApplicationController permissions: @cycle_analytics.permissions(user: current_user) } end + + def whitelist_query_limiting + Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-ce/issues/42671') + end end diff --git a/app/controllers/projects/forks_controller.rb b/app/controllers/projects/forks_controller.rb index 68978f8fdd1..f43bba18d81 100644 --- a/app/controllers/projects/forks_controller.rb +++ b/app/controllers/projects/forks_controller.rb @@ -2,6 +2,7 @@ class Projects::ForksController < Projects::ApplicationController include ContinueParams # Authorize + before_action :whitelist_query_limiting, only: [:create] before_action :require_non_empty_project before_action :authorize_download_code! before_action :authenticate_user!, only: [:new, :create] @@ -54,4 +55,8 @@ class Projects::ForksController < Projects::ApplicationController render :error end end + + def whitelist_query_limiting + Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-ce/issues/42335') + end end diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb index 384f18b316c..515cb08f1fc 100644 --- a/app/controllers/projects/issues_controller.rb +++ b/app/controllers/projects/issues_controller.rb @@ -8,6 +8,7 @@ class Projects::IssuesController < Projects::ApplicationController prepend_before_action :authenticate_user!, only: [:new] + before_action :whitelist_query_limiting, only: [:create, :create_merge_request, :move, :bulk_update] before_action :check_issues_available! before_action :issue, except: [:index, :new, :create, :bulk_update] before_action :set_issuables_index, only: [:index] @@ -247,4 +248,13 @@ class Projects::IssuesController < Projects::ApplicationController @finder_type = IssuesFinder super end + + def whitelist_query_limiting + # Also see the following issues: + # + # 1. https://gitlab.com/gitlab-org/gitlab-ce/issues/42423 + # 2. https://gitlab.com/gitlab-org/gitlab-ce/issues/42424 + # 3. https://gitlab.com/gitlab-org/gitlab-ce/issues/42426 + Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-ce/issues/42422') + end end diff --git a/app/controllers/projects/merge_requests/creations_controller.rb b/app/controllers/projects/merge_requests/creations_controller.rb index 0df80fa700f..a5a2d54ba82 100644 --- a/app/controllers/projects/merge_requests/creations_controller.rb +++ b/app/controllers/projects/merge_requests/creations_controller.rb @@ -4,6 +4,7 @@ class Projects::MergeRequests::CreationsController < Projects::MergeRequests::Ap include RendersCommits skip_before_action :merge_request + before_action :whitelist_query_limiting, only: [:create] before_action :authorize_create_merge_request! before_action :apply_diff_view_cookie!, only: [:diffs, :diff_for_path] before_action :build_merge_request, except: [:create] @@ -125,4 +126,8 @@ class Projects::MergeRequests::CreationsController < Projects::MergeRequests::Ap @project.forked_from_project end end + + def whitelist_query_limiting + Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-ce/issues/42384') + end end diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index 2e8a738b6d9..8af4e379f0a 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -7,6 +7,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo include IssuableCollections skip_before_action :merge_request, only: [:index, :bulk_update] + before_action :whitelist_query_limiting, only: [:assign_related_issues, :update] before_action :authorize_update_issuable!, only: [:close, :edit, :update, :remove_wip, :sort] before_action :set_issuables_index, only: [:index] before_action :authenticate_user!, only: [:assign_related_issues] @@ -339,4 +340,9 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo access_denied! unless access_check end + + def whitelist_query_limiting + # Also see https://gitlab.com/gitlab-org/gitlab-ce/issues/42441 + Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-ce/issues/42438') + end end diff --git a/app/controllers/projects/network_controller.rb b/app/controllers/projects/network_controller.rb index fb68dd771a1..3b10a93e97f 100644 --- a/app/controllers/projects/network_controller.rb +++ b/app/controllers/projects/network_controller.rb @@ -2,6 +2,7 @@ class Projects::NetworkController < Projects::ApplicationController include ExtractsPath include ApplicationHelper + before_action :whitelist_query_limiting before_action :require_non_empty_project before_action :assign_ref_vars before_action :authorize_download_code! @@ -35,4 +36,8 @@ class Projects::NetworkController < Projects::ApplicationController @options[:extended_sha1] = params[:extended_sha1] @commit = @repo.commit(@options[:extended_sha1]) end + + def whitelist_query_limiting + Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-ce/issues/42333') + end end diff --git a/app/controllers/projects/notes_controller.rb b/app/controllers/projects/notes_controller.rb index 5940fae8dd0..4f8978c93c3 100644 --- a/app/controllers/projects/notes_controller.rb +++ b/app/controllers/projects/notes_controller.rb @@ -2,6 +2,7 @@ class Projects::NotesController < Projects::ApplicationController include NotesActions include ToggleAwardEmoji + before_action :whitelist_query_limiting, only: [:create] before_action :authorize_read_note! before_action :authorize_create_note!, only: [:create] before_action :authorize_resolve_note!, only: [:resolve, :unresolve] @@ -79,4 +80,8 @@ 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-ce/issues/42383') + end end diff --git a/app/controllers/projects/pipelines_controller.rb b/app/controllers/projects/pipelines_controller.rb index e146d0d3cd5..78d109cf33e 100644 --- a/app/controllers/projects/pipelines_controller.rb +++ b/app/controllers/projects/pipelines_controller.rb @@ -1,4 +1,5 @@ class Projects::PipelinesController < Projects::ApplicationController + before_action :whitelist_query_limiting, only: [:create, :retry] before_action :pipeline, except: [:index, :new, :create, :charts] before_action :commit, only: [:show, :builds, :failures] before_action :authorize_read_pipeline! @@ -166,4 +167,9 @@ class Projects::PipelinesController < Projects::ApplicationController def commit @commit ||= @pipeline.commit end + + def whitelist_query_limiting + # Also see https://gitlab.com/gitlab-org/gitlab-ce/issues/42343 + Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-ce/issues/42339') + end end diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb index 86923909d07..72573e0765d 100644 --- a/app/controllers/projects_controller.rb +++ b/app/controllers/projects_controller.rb @@ -3,6 +3,7 @@ class ProjectsController < Projects::ApplicationController include ExtractsPath include PreviewMarkdown + before_action :whitelist_query_limiting, only: [:create] before_action :authenticate_user!, except: [:index, :show, :activity, :refs] before_action :redirect_git_extension, only: [:show] before_action :project, except: [:index, :new, :create] @@ -405,4 +406,8 @@ class ProjectsController < Projects::ApplicationController # redirect_to request.original_url.sub(%r{\.git/?\Z}, '') if params[:format] == 'git' end + + def whitelist_query_limiting + Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-ce/issues/42440') + end end diff --git a/app/controllers/registrations_controller.rb b/app/controllers/registrations_controller.rb index d9142311b6f..1848c806c41 100644 --- a/app/controllers/registrations_controller.rb +++ b/app/controllers/registrations_controller.rb @@ -1,6 +1,8 @@ class RegistrationsController < Devise::RegistrationsController include Recaptcha::Verify + before_action :whitelist_query_limiting, only: [:destroy] + def new redirect_to(new_user_session_path) end @@ -83,4 +85,8 @@ class RegistrationsController < Devise::RegistrationsController def devise_mapping @devise_mapping ||= Devise.mappings[:user] end + + def whitelist_query_limiting + Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-ce/issues/42380') + end end diff --git a/app/services/system_note_service.rb b/app/services/system_note_service.rb index 06b23cd7076..2253d638e93 100644 --- a/app/services/system_note_service.rb +++ b/app/services/system_note_service.rb @@ -22,8 +22,7 @@ module SystemNoteService commits_text = "#{total_count} commit".pluralize(total_count) body = "added #{commits_text}\n\n" - body << existing_commit_summary(noteable, existing_commits, oldrev) - body << new_commit_summary(new_commits).join("\n") + body << commits_list(noteable, new_commits, existing_commits, oldrev) body << "\n\n[Compare with previous version](#{diff_comparison_url(noteable, project, oldrev)})" create_note(NoteSummary.new(noteable, project, author, body, action: 'commit', commit_count: total_count)) @@ -481,7 +480,7 @@ module SystemNoteService # Returns an Array of Strings def new_commit_summary(new_commits) new_commits.collect do |commit| - "* #{commit.short_id} - #{escape_html(commit.title)}" + content_tag('li', "#{commit.short_id} - #{commit.title}") end end @@ -604,6 +603,16 @@ module SystemNoteService "#{cross_reference_note_prefix}#{gfm_reference}" end + # Builds a list of existing and new commits according to existing_commits and + # new_commits methods. + # Returns a String wrapped in `ul` and `li` tags. + def commits_list(noteable, new_commits, existing_commits, oldrev) + existing_commit_summary = existing_commit_summary(noteable, existing_commits, oldrev) + new_commit_summary = new_commit_summary(new_commits).join + + content_tag('ul', "#{existing_commit_summary}#{new_commit_summary}".html_safe) + end + # Build a single line summarizing existing commits being added in a merge # request # @@ -640,11 +649,8 @@ module SystemNoteService branch = noteable.target_branch branch = "#{noteable.target_project_namespace}:#{branch}" if noteable.for_fork? - "* #{commit_ids} - #{commits_text} from branch `#{branch}`\n" - end - - def escape_html(text) - Rack::Utils.escape_html(text) + branch_name = content_tag('code', branch) + content_tag('li', "#{commit_ids} - #{commits_text} from branch #{branch_name}".html_safe) end def url_helpers @@ -661,4 +667,8 @@ module SystemNoteService start_sha: oldrev ) end + + def content_tag(*args) + ActionController::Base.helpers.content_tag(*args) + end end diff --git a/changelogs/unreleased/25327-coverage-badge-rounding.yml b/changelogs/unreleased/25327-coverage-badge-rounding.yml new file mode 100644 index 00000000000..ea985689484 --- /dev/null +++ b/changelogs/unreleased/25327-coverage-badge-rounding.yml @@ -0,0 +1,5 @@ +--- +title: Show coverage to two decimal points in coverage badge +merge_request: 10083 +author: Jeff Stubler +type: changed diff --git a/changelogs/unreleased/osw-markdown-bypass-for-commit-messages.yml b/changelogs/unreleased/osw-markdown-bypass-for-commit-messages.yml new file mode 100644 index 00000000000..b2c1cd9710a --- /dev/null +++ b/changelogs/unreleased/osw-markdown-bypass-for-commit-messages.yml @@ -0,0 +1,5 @@ +--- +title: Bypass commits title markdown on notes +merge_request: +author: +type: fixed diff --git a/changelogs/unreleased/query-counts.yml b/changelogs/unreleased/query-counts.yml new file mode 100644 index 00000000000..e01ff8a4ad8 --- /dev/null +++ b/changelogs/unreleased/query-counts.yml @@ -0,0 +1,5 @@ +--- +title: Track and act upon the number of executed queries +merge_request: +author: +type: added diff --git a/config/initializers/query_limiting.rb b/config/initializers/query_limiting.rb new file mode 100644 index 00000000000..66864d1898e --- /dev/null +++ b/config/initializers/query_limiting.rb @@ -0,0 +1,9 @@ +if Gitlab::QueryLimiting.enable? + require_dependency 'gitlab/query_limiting/active_support_subscriber' + require_dependency 'gitlab/query_limiting/transaction' + require_dependency 'gitlab/query_limiting/middleware' + + Gitlab::Application.configure do |config| + config.middleware.use(Gitlab::QueryLimiting::Middleware) + end +end diff --git a/doc/development/README.md b/doc/development/README.md index 12cca9f84b7..45e9565f9a7 100644 --- a/doc/development/README.md +++ b/doc/development/README.md @@ -75,6 +75,7 @@ comments: false - [Ordering table columns](ordering_table_columns.md) - [Verifying database capabilities](verifying_database_capabilities.md) - [Database Debugging and Troubleshooting](database_debugging.md) +- [Query Count Limits](query_count_limits.md) ## Testing guides diff --git a/doc/development/query_count_limits.md b/doc/development/query_count_limits.md new file mode 100644 index 00000000000..ebb6e0c2dac --- /dev/null +++ b/doc/development/query_count_limits.md @@ -0,0 +1,65 @@ +# Query Count Limits + +Each controller or API endpoint is allowed to execute up to 100 SQL queries. In +a production environment we'll only log an error in case this threshold is +exceeded, but in a test environment we'll raise an error instead. + +## Solving Failing Tests + +When a test fails because it executes more than 100 SQL queries there are two +solutions to this problem: + +1. Reduce the number of SQL queries that are executed. +2. Whitelist the controller or API endpoint. + +You should only resort to whitelisting 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 will be 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 + +In the event that you _have_ to whitelist a controller you'll first need to +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`). + +Once the issue has been created you can whitelist 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: + +```ruby +class MyController < ApplicationController + before_action :whitelist_query_limiting, only: [:show] + + def index + # ... + end + + def show + # ... + end + + def whitelist_query_limiting + Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/...') + end +end +``` + +By using a `before_action` you don't have to modify the controller method in +question, reducing the likelihood of merge conflicts. + +For Grape API endpoints there unfortunately is not a reliable way of running a +hook before a specific endpoint. This means that you have to add the whitelist +call directly into the endpoint like so: + +```ruby +get '/projects/:id/foo' do + Gitlab::QueryLimiting.whitelist('...') + + # ... +end +``` diff --git a/lib/api/branches.rb b/lib/api/branches.rb index 0791a110c39..1794207e29b 100644 --- a/lib/api/branches.rb +++ b/lib/api/branches.rb @@ -29,6 +29,8 @@ module API use :pagination end get ':id/repository/branches' do + Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-ce/issues/42329') + repository = user_project.repository branches = ::Kaminari.paginate_array(repository.branches.sort_by(&:name)) merged_branch_names = repository.merged_branch_names(branches.map(&:name)) diff --git a/lib/api/issues.rb b/lib/api/issues.rb index c99fe3ab5b3..b6c278c89d0 100644 --- a/lib/api/issues.rb +++ b/lib/api/issues.rb @@ -161,6 +161,8 @@ module API use :issue_params end post ':id/issues' do + Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-ce/issues/42320') + authorize! :create_issue, user_project # Setting created_at time only allowed for admins and project owners @@ -201,6 +203,8 @@ module API :labels, :created_at, :due_date, :confidential, :state_event end put ':id/issues/:issue_iid' do + Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-ce/issues/42322') + issue = user_project.issues.find_by!(iid: params.delete(:issue_iid)) authorize! :update_issue, issue @@ -234,6 +238,8 @@ module API requires :to_project_id, type: Integer, desc: 'The ID of the new project' end post ':id/issues/:issue_iid/move' do + Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-ce/issues/42323') + issue = user_project.issues.find_by(iid: params[:issue_iid]) not_found!('Issue') unless issue diff --git a/lib/api/merge_requests.rb b/lib/api/merge_requests.rb index 420aaf1c964..719afa09295 100644 --- a/lib/api/merge_requests.rb +++ b/lib/api/merge_requests.rb @@ -152,6 +152,8 @@ module API use :optional_params end post ":id/merge_requests" do + Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-ce/issues/42316') + authorize! :create_merge_request, user_project mr_params = declared_params(include_missing: false) @@ -256,6 +258,8 @@ module API at_least_one_of(*at_least_one_of_ce) end put ':id/merge_requests/:merge_request_iid' do + Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-ce/issues/42318') + merge_request = find_merge_request_with_access(params.delete(:merge_request_iid), :update_merge_request) mr_params = declared_params(include_missing: false) @@ -283,6 +287,8 @@ module API optional :sha, type: String, desc: 'When present, must have the HEAD SHA of the source branch' end put ':id/merge_requests/:merge_request_iid/merge' do + Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-ce/issues/42317') + merge_request = find_project_merge_request(params[:merge_request_iid]) merge_when_pipeline_succeeds = to_boolean(params[:merge_when_pipeline_succeeds]) diff --git a/lib/api/pipelines.rb b/lib/api/pipelines.rb index 675c963bae2..d2b8b832e4e 100644 --- a/lib/api/pipelines.rb +++ b/lib/api/pipelines.rb @@ -42,6 +42,8 @@ module API requires :ref, type: String, desc: 'Reference' end post ':id/pipeline' do + Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-ce/issues/42124') + authorize! :create_pipeline, user_project new_pipeline = Ci::CreatePipelineService.new(user_project, diff --git a/lib/api/projects.rb b/lib/api/projects.rb index 8b5e4f8edcc..5b481121a10 100644 --- a/lib/api/projects.rb +++ b/lib/api/projects.rb @@ -210,6 +210,8 @@ module API optional :namespace, type: String, desc: 'The ID or name of the namespace that the project will be forked into' end post ':id/fork' do + Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-ce/issues/42284') + fork_params = declared_params(include_missing: false) namespace_id = fork_params[:namespace] diff --git a/lib/api/triggers.rb b/lib/api/triggers.rb index dd6801664b1..b3709455bc3 100644 --- a/lib/api/triggers.rb +++ b/lib/api/triggers.rb @@ -15,6 +15,8 @@ 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-ce/issues/42283') + # validate variables params[:variables] = params[:variables].to_h unless params[:variables].all? { |key, value| key.is_a?(String) && value.is_a?(String) } diff --git a/lib/api/users.rb b/lib/api/users.rb index e5de31ad51b..c7c2aa280d5 100644 --- a/lib/api/users.rb +++ b/lib/api/users.rb @@ -383,6 +383,8 @@ module API optional :hard_delete, type: Boolean, desc: "Whether to remove a user's contributions" end delete ":id" do + Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-ce/issues/42279') + authenticated_as_admin! user = User.find_by(id: params[:id]) diff --git a/lib/api/v3/branches.rb b/lib/api/v3/branches.rb index b201bf77667..25176c5b38e 100644 --- a/lib/api/v3/branches.rb +++ b/lib/api/v3/branches.rb @@ -14,6 +14,8 @@ module API success ::API::Entities::Branch end get ":id/repository/branches" do + Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-ce/issues/42276') + repository = user_project.repository branches = repository.branches.sort_by(&:name) merged_branch_names = repository.merged_branch_names(branches.map(&:name)) diff --git a/lib/api/v3/issues.rb b/lib/api/v3/issues.rb index cb371fdbab8..b59947d81d9 100644 --- a/lib/api/v3/issues.rb +++ b/lib/api/v3/issues.rb @@ -134,6 +134,8 @@ module API use :issue_params end post ':id/issues' do + Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-ce/issues/42131') + # Setting created_at time only allowed for admins and project owners unless current_user.admin? || user_project.owner == current_user params.delete(:created_at) @@ -169,6 +171,8 @@ module API :labels, :created_at, :due_date, :confidential, :state_event end put ':id/issues/:issue_id' do + Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-ce/issues/42132') + issue = user_project.issues.find(params.delete(:issue_id)) authorize! :update_issue, issue @@ -201,6 +205,8 @@ module API requires :to_project_id, type: Integer, desc: 'The ID of the new project' end post ':id/issues/:issue_id/move' do + Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-ce/issues/42133') + issue = user_project.issues.find_by(id: params[:issue_id]) not_found!('Issue') unless issue diff --git a/lib/api/v3/merge_requests.rb b/lib/api/v3/merge_requests.rb index 0a24fea52a3..ce216497996 100644 --- a/lib/api/v3/merge_requests.rb +++ b/lib/api/v3/merge_requests.rb @@ -91,6 +91,8 @@ module API use :optional_params end post ":id/merge_requests" do + Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-ce/issues/42126') + authorize! :create_merge_request, user_project mr_params = declared_params(include_missing: false) @@ -167,6 +169,8 @@ module API :remove_source_branch end put path do + Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-ce/issues/42127') + merge_request = find_merge_request_with_access(params.delete(:merge_request_id), :update_merge_request) mr_params = declared_params(include_missing: false) diff --git a/lib/api/v3/pipelines.rb b/lib/api/v3/pipelines.rb index c48cbd2b765..6d31c12f572 100644 --- a/lib/api/v3/pipelines.rb +++ b/lib/api/v3/pipelines.rb @@ -19,6 +19,8 @@ module API desc: 'Either running, branches, or tags' end get ':id/pipelines' do + Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-ce/issues/42123') + authorize! :read_pipeline, user_project pipelines = PipelinesFinder.new(user_project, scope: params[:scope]).execute diff --git a/lib/api/v3/triggers.rb b/lib/api/v3/triggers.rb index 534911fde5c..34f07dfb486 100644 --- a/lib/api/v3/triggers.rb +++ b/lib/api/v3/triggers.rb @@ -16,6 +16,8 @@ module API optional :variables, type: Hash, desc: 'The list of variables to be injected into build' end post ":id/(ref/:ref/)trigger/builds", requirements: { ref: /.+/ } do + Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-ce/issues/42121') + # validate variables params[:variables] = params[:variables].to_h unless params[:variables].all? { |key, value| key.is_a?(String) && value.is_a?(String) } diff --git a/lib/gitlab/badge/coverage/report.rb b/lib/gitlab/badge/coverage/report.rb index 9a0482306b7..778d78185ff 100644 --- a/lib/gitlab/badge/coverage/report.rb +++ b/lib/gitlab/badge/coverage/report.rb @@ -23,7 +23,7 @@ module Gitlab @coverage ||= raw_coverage return unless @coverage - @coverage.to_i + @coverage.to_f.round(2) end def metadata diff --git a/lib/gitlab/badge/coverage/template.rb b/lib/gitlab/badge/coverage/template.rb index fcecb1d9665..afbf9dd17e3 100644 --- a/lib/gitlab/badge/coverage/template.rb +++ b/lib/gitlab/badge/coverage/template.rb @@ -25,7 +25,7 @@ module Gitlab end def value_text - @status ? "#{@status}%" : 'unknown' + @status ? ("%.2f%%" % @status) : 'unknown' end def key_width @@ -33,7 +33,7 @@ module Gitlab end def value_width - @status ? 36 : 58 + @status ? 54 : 58 end def value_color diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb index 35eb4a097e9..ab1362a3bb0 100644 --- a/lib/gitlab/git/repository.rb +++ b/lib/gitlab/git/repository.rb @@ -1222,33 +1222,13 @@ module Gitlab end def squash(user, squash_id, branch:, start_sha:, end_sha:, author:, message:) - squash_path = worktree_path(SQUASH_WORKTREE_PREFIX, squash_id) - env = git_env_for_user(user).merge( - 'GIT_AUTHOR_NAME' => author.name, - 'GIT_AUTHOR_EMAIL' => author.email - ) - diff_range = "#{start_sha}...#{end_sha}" - diff_files = run_git!( - %W(diff --name-only --diff-filter=a --binary #{diff_range}) - ).chomp - - with_worktree(squash_path, branch, sparse_checkout_files: diff_files, env: env) do - # Apply diff of the `diff_range` to the worktree - diff = run_git!(%W(diff --binary #{diff_range})) - run_git!(%w(apply --index), chdir: squash_path, env: env) do |stdin| - stdin.write(diff) + gitaly_migrate(:squash) do |is_enabled| + if is_enabled + gitaly_operation_client.user_squash(user, squash_id, branch, + start_sha, end_sha, author, message) + else + git_squash(user, squash_id, branch, start_sha, end_sha, author, message) end - - # Commit the `diff_range` diff - run_git!(%W(commit --no-verify --message #{message}), chdir: squash_path, env: env) - - # Return the squash sha. May print a warning for ambiguous refs, but - # we can ignore that with `--quiet` and just take the SHA, if present. - # HEAD here always refers to the current HEAD commit, even if there is - # another ref called HEAD. - run_git!( - %w(rev-parse --quiet --verify HEAD), chdir: squash_path, env: env - ).chomp end end @@ -2182,6 +2162,37 @@ module Gitlab end end + def git_squash(user, squash_id, branch, start_sha, end_sha, author, message) + squash_path = worktree_path(SQUASH_WORKTREE_PREFIX, squash_id) + env = git_env_for_user(user).merge( + 'GIT_AUTHOR_NAME' => author.name, + 'GIT_AUTHOR_EMAIL' => author.email + ) + diff_range = "#{start_sha}...#{end_sha}" + diff_files = run_git!( + %W(diff --name-only --diff-filter=a --binary #{diff_range}) + ).chomp + + with_worktree(squash_path, branch, sparse_checkout_files: diff_files, env: env) do + # Apply diff of the `diff_range` to the worktree + diff = run_git!(%W(diff --binary #{diff_range})) + run_git!(%w(apply --index), chdir: squash_path, env: env) do |stdin| + stdin.write(diff) + end + + # Commit the `diff_range` diff + run_git!(%W(commit --no-verify --message #{message}), chdir: squash_path, env: env) + + # Return the squash sha. May print a warning for ambiguous refs, but + # we can ignore that with `--quiet` and just take the SHA, if present. + # HEAD here always refers to the current HEAD commit, even if there is + # another ref called HEAD. + run_git!( + %w(rev-parse --quiet --verify HEAD), chdir: squash_path, env: env + ).chomp + end + end + def local_fetch_ref(source_path, source_ref:, target_ref:) args = %W(fetch --no-tags -f #{source_path} #{source_ref}:#{target_ref}) run_git(args) diff --git a/lib/gitlab/gitaly_client/operation_service.rb b/lib/gitlab/gitaly_client/operation_service.rb index cd2734b5a07..831cfd1e014 100644 --- a/lib/gitlab/gitaly_client/operation_service.rb +++ b/lib/gitlab/gitaly_client/operation_service.rb @@ -183,6 +183,32 @@ module Gitlab end end + def user_squash(user, squash_id, branch, start_sha, end_sha, author, message) + request = Gitaly::UserSquashRequest.new( + repository: @gitaly_repo, + user: Gitlab::Git::User.from_gitlab(user).to_gitaly, + squash_id: squash_id.to_s, + branch: encode_binary(branch), + start_sha: start_sha, + end_sha: end_sha, + author: Gitlab::Git::User.from_gitlab(author).to_gitaly, + commit_message: encode_binary(message) + ) + + response = GitalyClient.call( + @repository.storage, + :operation_service, + :user_squash, + request + ) + + if response.git_error.presence + raise Gitlab::Git::Repository::GitError, response.git_error + end + + response.squash_sha + end + def user_commit_files( user, branch_name, commit_message, actions, author_email, author_name, start_branch_name, start_repository) diff --git a/lib/gitlab/query_limiting.rb b/lib/gitlab/query_limiting.rb new file mode 100644 index 00000000000..f64f1757144 --- /dev/null +++ b/lib/gitlab/query_limiting.rb @@ -0,0 +1,36 @@ +module Gitlab + module QueryLimiting + # Returns true if we should enable tracking of query counts. + # + # This is only enabled in production/staging if we're running on GitLab.com. + # This ensures we don't produce any errors that users can't do anything + # about themselves. + def self.enable? + Gitlab.com? || Rails.env.development? || Rails.env.test? + 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_whitelist? + + unless issue_url.start_with?('https://') + raise( + ArgumentError, + 'You must provide a valid issue URL in order to whitelist a block of code' + ) + end + + Transaction&.current&.whitelisted = true + end + + def self.enable_whitelist? + Rails.env.development? || Rails.env.test? + end + end +end diff --git a/lib/gitlab/query_limiting/active_support_subscriber.rb b/lib/gitlab/query_limiting/active_support_subscriber.rb new file mode 100644 index 00000000000..66049c94ec6 --- /dev/null +++ b/lib/gitlab/query_limiting/active_support_subscriber.rb @@ -0,0 +1,11 @@ +module Gitlab + module QueryLimiting + class ActiveSupportSubscriber < ActiveSupport::Subscriber + attach_to :active_record + + def sql(*) + Transaction.current&.increment + end + end + end +end diff --git a/lib/gitlab/query_limiting/middleware.rb b/lib/gitlab/query_limiting/middleware.rb new file mode 100644 index 00000000000..949ae79a047 --- /dev/null +++ b/lib/gitlab/query_limiting/middleware.rb @@ -0,0 +1,55 @@ +# frozen_string_literal: true + +module Gitlab + module QueryLimiting + # Middleware for reporting (or raising) when a request performs more than a + # certain amount of database queries. + class Middleware + CONTROLLER_KEY = 'action_controller.instance'.freeze + ENDPOINT_KEY = 'api.endpoint'.freeze + + def initialize(app) + @app = app + end + + def call(env) + transaction, retval = Transaction.run do + @app.call(env) + end + + transaction.action = action_name(env) + transaction.act_upon_results + + retval + end + + def action_name(env) + if env[CONTROLLER_KEY] + action_for_rails(env) + elsif env[ENDPOINT_KEY] + action_for_grape(env) + end + end + + private + + def action_for_rails(env) + controller = env[CONTROLLER_KEY] + action = "#{controller.class.name}##{controller.action_name}" + + if controller.content_type == 'text/html' + action + else + "#{action} (#{controller.content_type})" + end + end + + def action_for_grape(env) + endpoint = env[ENDPOINT_KEY] + route = endpoint.route rescue nil + + "#{route.request_method} #{route.path}" if route + end + end + end +end diff --git a/lib/gitlab/query_limiting/transaction.rb b/lib/gitlab/query_limiting/transaction.rb new file mode 100644 index 00000000000..3cbafadb0d0 --- /dev/null +++ b/lib/gitlab/query_limiting/transaction.rb @@ -0,0 +1,83 @@ +module Gitlab + module QueryLimiting + class Transaction + THREAD_KEY = :__gitlab_query_counts_transaction + + attr_accessor :count, :whitelisted + + # The name of the action (e.g. `UsersController#show`) that is being + # executed. + attr_accessor :action + + # The maximum number of SQL queries that can be executed in a request. For + # the sake of keeping things simple we hardcode this value here, it's not + # supposed to be changed very often anyway. + THRESHOLD = 100 + + # Error that is raised whenever exceeding the maximum number of queries. + ThresholdExceededError = Class.new(StandardError) + + def self.current + Thread.current[THREAD_KEY] + end + + # Starts a new transaction and returns it and the blocks' return value. + # + # Example: + # + # transaction, retval = Transaction.run do + # 10 + # end + # + # retval # => 10 + def self.run + transaction = new + Thread.current[THREAD_KEY] = transaction + + [transaction, yield] + ensure + Thread.current[THREAD_KEY] = nil + end + + def initialize + @action = nil + @count = 0 + @whitelisted = false + end + + # Sends a notification based on the number of executed SQL queries. + def act_upon_results + return unless threshold_exceeded? + + error = ThresholdExceededError.new(error_message) + + if raise_error? + raise(error) + else + # Raven automatically logs to the Rails log if disabled, thus we don't + # need to manually log anything in case Sentry support is not enabled. + Raven.capture_exception(error) + end + end + + def increment + @count += 1 unless whitelisted + end + + def raise_error? + Rails.env.test? + end + + def threshold_exceeded? + count > THRESHOLD + end + + def error_message + header = 'Too many SQL queries were executed' + header += " in #{action}" if action + + "#{header}: a maximum of #{THRESHOLD} is allowed but #{count} SQL queries were executed" + end + end + end +end diff --git a/lib/tasks/flay.rake b/lib/tasks/flay.rake index b1e012e70c5..4b4881cecb8 100644 --- a/lib/tasks/flay.rake +++ b/lib/tasks/flay.rake @@ -2,7 +2,7 @@ desc 'Code duplication analyze via flay' task :flay do output = `bundle exec flay --mass 35 app/ lib/gitlab/ 2> #{File::NULL}` - if output.include? "Similar code found" + if output.include?("Similar code found") || output.include?("IDENTICAL code found") puts output exit 1 end diff --git a/spec/features/projects/badges/coverage_spec.rb b/spec/features/projects/badges/coverage_spec.rb index 821ce88a402..f51001edcd7 100644 --- a/spec/features/projects/badges/coverage_spec.rb +++ b/spec/features/projects/badges/coverage_spec.rb @@ -18,7 +18,7 @@ feature 'test coverage badge' do show_test_coverage_badge - expect_coverage_badge('95%') + expect_coverage_badge('95.00%') end scenario 'user requests coverage badge for specific job' do @@ -30,7 +30,7 @@ feature 'test coverage badge' do show_test_coverage_badge(job: 'coverage') - expect_coverage_badge('85%') + expect_coverage_badge('85.00%') end scenario 'user requests coverage badge for pipeline without coverage' do diff --git a/spec/lib/gitlab/badge/coverage/template_spec.rb b/spec/lib/gitlab/badge/coverage/template_spec.rb index 383bae6e087..d9c21a22590 100644 --- a/spec/lib/gitlab/badge/coverage/template_spec.rb +++ b/spec/lib/gitlab/badge/coverage/template_spec.rb @@ -1,7 +1,7 @@ require 'spec_helper' describe Gitlab::Badge::Coverage::Template do - let(:badge) { double(entity: 'coverage', status: 90) } + let(:badge) { double(entity: 'coverage', status: 90.00) } let(:template) { described_class.new(badge) } describe '#key_text' do @@ -13,7 +13,17 @@ describe Gitlab::Badge::Coverage::Template do describe '#value_text' do context 'when coverage is known' do it 'returns coverage percentage' do - expect(template.value_text).to eq '90%' + expect(template.value_text).to eq '90.00%' + end + end + + context 'when coverage is known to many digits' do + before do + allow(badge).to receive(:status).and_return(92.349) + end + + it 'returns rounded coverage percentage' do + expect(template.value_text).to eq '92.35%' end end @@ -37,7 +47,7 @@ describe Gitlab::Badge::Coverage::Template do describe '#value_width' do context 'when coverage is known' do it 'is narrower when coverage is known' do - expect(template.value_width).to eq 36 + expect(template.value_width).to eq 54 end end @@ -113,7 +123,7 @@ describe Gitlab::Badge::Coverage::Template do describe '#width' do context 'when coverage is known' do it 'returns the key width plus value width' do - expect(template.width).to eq 98 + expect(template.width).to eq 116 end end diff --git a/spec/lib/gitlab/git/repository_spec.rb b/spec/lib/gitlab/git/repository_spec.rb index df9f8a84aa5..ec1c7a96f92 100644 --- a/spec/lib/gitlab/git/repository_spec.rb +++ b/spec/lib/gitlab/git/repository_spec.rb @@ -2196,7 +2196,7 @@ describe Gitlab::Git::Repository, seed_helper: true do repository.squash(user, squash_id, opts) end - context 'sparse checkout' do + context 'sparse checkout', :skip_gitaly_mock do let(:expected_files) { %w(files files/js files/js/application.js) } before do diff --git a/spec/lib/gitlab/gitaly_client/operation_service_spec.rb b/spec/lib/gitlab/gitaly_client/operation_service_spec.rb index d9ec28ab02e..9fbdd73ee0e 100644 --- a/spec/lib/gitlab/gitaly_client/operation_service_spec.rb +++ b/spec/lib/gitlab/gitaly_client/operation_service_spec.rb @@ -123,4 +123,53 @@ describe Gitlab::GitalyClient::OperationService do expect(subject.branch_created).to be(false) end end + + describe '#user_squash' do + let(:branch_name) { 'my-branch' } + let(:squash_id) { '1' } + let(:start_sha) { 'b83d6e391c22777fca1ed3012fce84f633d7fed0' } + let(:end_sha) { '54cec5282aa9f21856362fe321c800c236a61615' } + let(:commit_message) { 'Squash message' } + let(:request) do + Gitaly::UserSquashRequest.new( + repository: repository.gitaly_repository, + user: gitaly_user, + squash_id: squash_id.to_s, + branch: branch_name, + start_sha: start_sha, + end_sha: end_sha, + author: gitaly_user, + commit_message: commit_message + ) + end + let(:squash_sha) { 'f00' } + let(:response) { Gitaly::UserSquashResponse.new(squash_sha: squash_sha) } + + subject do + client.user_squash(user, squash_id, branch_name, start_sha, end_sha, user, commit_message) + end + + it 'sends a user_squash message and returns the squash sha' do + expect_any_instance_of(Gitaly::OperationService::Stub) + .to receive(:user_squash).with(request, kind_of(Hash)) + .and_return(response) + + expect(subject).to eq(squash_sha) + end + + context "when git_error is present" do + let(:response) do + Gitaly::UserSquashResponse.new(git_error: "something failed") + end + + it "throws a PreReceive exception" do + expect_any_instance_of(Gitaly::OperationService::Stub) + .to receive(:user_squash).with(request, kind_of(Hash)) + .and_return(response) + + expect { subject }.to raise_error( + Gitlab::Git::Repository::GitError, "something failed") + end + end + end end diff --git a/spec/lib/gitlab/query_limiting/active_support_subscriber_spec.rb b/spec/lib/gitlab/query_limiting/active_support_subscriber_spec.rb new file mode 100644 index 00000000000..b49bc5c328c --- /dev/null +++ b/spec/lib/gitlab/query_limiting/active_support_subscriber_spec.rb @@ -0,0 +1,19 @@ +require 'spec_helper' + +describe Gitlab::QueryLimiting::ActiveSupportSubscriber do + describe '#sql' do + it 'increments the number of executed SQL queries' do + transaction = double(:transaction) + + allow(Gitlab::QueryLimiting::Transaction) + .to receive(:current) + .and_return(transaction) + + expect(transaction) + .to receive(:increment) + .at_least(:once) + + User.count + end + end +end diff --git a/spec/lib/gitlab/query_limiting/middleware_spec.rb b/spec/lib/gitlab/query_limiting/middleware_spec.rb new file mode 100644 index 00000000000..a04bcdecb4b --- /dev/null +++ b/spec/lib/gitlab/query_limiting/middleware_spec.rb @@ -0,0 +1,72 @@ +require 'spec_helper' + +describe Gitlab::QueryLimiting::Middleware do + describe '#call' do + it 'runs the application with query limiting in place' do + middleware = described_class.new(-> (env) { env }) + + expect_any_instance_of(Gitlab::QueryLimiting::Transaction) + .to receive(:act_upon_results) + + expect(middleware.call({ number: 10 })) + .to eq({ number: 10 }) + end + end + + describe '#action_name' do + let(:middleware) { described_class.new(-> (env) { env }) } + + context 'using a Rails request' do + it 'returns the name of the controller and action' do + env = { + described_class::CONTROLLER_KEY => double( + :controller, + action_name: 'show', + class: double(:class, name: 'UsersController'), + content_type: 'text/html' + ) + } + + expect(middleware.action_name(env)).to eq('UsersController#show') + end + + it 'includes the content type if this is not text/html' do + env = { + described_class::CONTROLLER_KEY => double( + :controller, + action_name: 'show', + class: double(:class, name: 'UsersController'), + content_type: 'application/json' + ) + } + + expect(middleware.action_name(env)) + .to eq('UsersController#show (application/json)') + end + end + + context 'using a Grape API request' do + it 'returns the name of the request method and endpoint path' do + env = { + described_class::ENDPOINT_KEY => double( + :endpoint, + route: double(:route, request_method: 'GET', path: '/foo') + ) + } + + expect(middleware.action_name(env)).to eq('GET /foo') + end + + it 'returns nil if the route can not be retrieved' do + endpoint = double(:endpoint) + env = { described_class::ENDPOINT_KEY => endpoint } + + allow(endpoint) + .to receive(:route) + .and_raise(RuntimeError) + + expect(middleware.action_name(env)).to be_nil + end + end + end +end diff --git a/spec/lib/gitlab/query_limiting/transaction_spec.rb b/spec/lib/gitlab/query_limiting/transaction_spec.rb new file mode 100644 index 00000000000..b4231fcd0fa --- /dev/null +++ b/spec/lib/gitlab/query_limiting/transaction_spec.rb @@ -0,0 +1,144 @@ +require 'spec_helper' + +describe Gitlab::QueryLimiting::Transaction do + after do + Thread.current[described_class::THREAD_KEY] = nil + end + + describe '.current' do + it 'returns nil when there is no transaction' do + expect(described_class.current).to be_nil + end + + it 'returns the transaction when present' do + Thread.current[described_class::THREAD_KEY] = described_class.new + + expect(described_class.current).to be_an_instance_of(described_class) + end + end + + describe '.run' do + it 'runs a transaction and returns it and its return value' do + trans, ret = described_class.run do + 10 + end + + expect(trans).to be_an_instance_of(described_class) + expect(ret).to eq(10) + end + + it 'removes the transaction from the current thread upon completion' do + described_class.run do + 10 + end + + expect(Thread.current[described_class::THREAD_KEY]).to be_nil + end + end + + describe '#act_upon_results' do + context 'when the query threshold is not exceeded' do + it 'does nothing' do + trans = described_class.new + + expect(trans).not_to receive(:raise) + + trans.act_upon_results + end + end + + context 'when the query threshold is exceeded' do + let(:transaction) do + trans = described_class.new + trans.count = described_class::THRESHOLD + 1 + + trans + end + + it 'raises an error when this is enabled' do + expect { transaction.act_upon_results } + .to raise_error(described_class::ThresholdExceededError) + end + + it 'reports the error in Sentry if raising an error is disabled' do + expect(transaction) + .to receive(:raise_error?) + .and_return(false) + + expect(Raven) + .to receive(:capture_exception) + .with(an_instance_of(described_class::ThresholdExceededError)) + + transaction.act_upon_results + end + end + end + + describe '#increment' do + it 'increments the number of executed queries' do + transaction = described_class.new + + expect(transaction.count).to be_zero + + transaction.increment + + expect(transaction.count).to eq(1) + end + end + + describe '#raise_error?' do + it 'returns true in a test environment' do + transaction = described_class.new + + expect(transaction.raise_error?).to eq(true) + end + + it 'returns false in a production environment' do + transaction = described_class.new + + expect(Rails.env) + .to receive(:test?) + .and_return(false) + + expect(transaction.raise_error?).to eq(false) + end + end + + describe '#threshold_exceeded?' do + it 'returns false when the threshold is not exceeded' do + transaction = described_class.new + + expect(transaction.threshold_exceeded?).to eq(false) + end + + it 'returns true when the threshold is exceeded' do + transaction = described_class.new + transaction.count = described_class::THRESHOLD + 1 + + expect(transaction.threshold_exceeded?).to eq(true) + end + end + + describe '#error_message' do + it 'returns the error message to display when the threshold is exceeded' do + transaction = described_class.new + transaction.count = max = described_class::THRESHOLD + + expect(transaction.error_message).to eq( + "Too many SQL queries were executed: a maximum of #{max} " \ + "is allowed but #{max} SQL queries were executed" + ) + end + + it 'includes the action name in the error message when present' do + transaction = described_class.new + transaction.count = max = described_class::THRESHOLD + transaction.action = 'UsersController#show' + + expect(transaction.error_message).to eq( + "Too many SQL queries were executed in UsersController#show: " \ + "a maximum of #{max} is allowed but #{max} SQL queries were executed" + ) + end + end +end diff --git a/spec/lib/gitlab/query_limiting_spec.rb b/spec/lib/gitlab/query_limiting_spec.rb new file mode 100644 index 00000000000..2eddab0b8c3 --- /dev/null +++ b/spec/lib/gitlab/query_limiting_spec.rb @@ -0,0 +1,65 @@ +require 'spec_helper' + +describe Gitlab::QueryLimiting do + describe '.enable?' do + it 'returns true in a test environment' do + expect(described_class.enable?).to eq(true) + end + + it 'returns true in a development environment' do + allow(Rails.env).to receive(:development?).and_return(true) + + expect(described_class.enable?).to eq(true) + end + + it 'returns true on GitLab.com' do + allow(Gitlab).to receive(:com?).and_return(true) + + expect(described_class.enable?).to eq(true) + end + + it 'returns true in a non GitLab.com' do + expect(Gitlab).to receive(:com?).and_return(false) + expect(Rails.env).to receive(:development?).and_return(false) + expect(Rails.env).to receive(:test?).and_return(false) + + expect(described_class.enable?).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) + end + + context 'without a transaction' do + it 'does nothing' do + expect { described_class.whitelist('https://example.com') } + .not_to raise_error + end + end + + context 'with a transaction' do + let(:transaction) { Gitlab::QueryLimiting::Transaction.new } + + before do + allow(Gitlab::QueryLimiting::Transaction) + .to receive(:current) + .and_return(transaction) + end + + it 'does not increment the number of SQL queries executed in the block' do + before = transaction.count + + described_class.whitelist('https://example.com') + + 2.times do + User.count + end + + expect(transaction.count).to eq(before) + end + end + end +end diff --git a/spec/requests/api/groups_spec.rb b/spec/requests/api/groups_spec.rb index 3c0b4728dc2..bb0034e3237 100644 --- a/spec/requests/api/groups_spec.rb +++ b/spec/requests/api/groups_spec.rb @@ -30,6 +30,21 @@ describe API::Groups do expect(json_response) .to satisfy_one { |group| group['name'] == group1.name } end + + it 'avoids N+1 queries' do + # Establish baseline + get api("/groups", admin) + + control = ActiveRecord::QueryRecorder.new do + get api("/groups", admin) + end + + create(:group) + + expect do + get api("/groups", admin) + end.not_to exceed_query_limit(control) + end end context "when authenticated as user" do diff --git a/spec/services/system_note_service_spec.rb b/spec/services/system_note_service_spec.rb index ab3aa18cf4e..5b5edc1aa0d 100644 --- a/spec/services/system_note_service_spec.rb +++ b/spec/services/system_note_service_spec.rb @@ -54,10 +54,11 @@ describe SystemNoteService do expect(note_lines[0]).to eq "added #{new_commits.size} commits" end - it 'adds a message line for each commit' do - new_commits.each_with_index do |commit, i| - # Skip the header - expect(HTMLEntities.new.decode(note_lines[i + 1])).to eq "* #{commit.short_id} - #{commit.title}" + it 'adds a message for each commit' do + decoded_note_content = HTMLEntities.new.decode(subject.note) + + new_commits.each do |commit| + expect(decoded_note_content).to include("<li>#{commit.short_id} - #{commit.title}</li>") end end end @@ -69,7 +70,7 @@ describe SystemNoteService do let(:old_commits) { [noteable.commits.last] } it 'includes the existing commit' do - expect(summary_line).to eq "* #{old_commits.first.short_id} - 1 commit from branch `feature`" + expect(summary_line).to start_with("<ul><li>#{old_commits.first.short_id} - 1 commit from branch <code>feature</code>") end end @@ -79,22 +80,16 @@ describe SystemNoteService do context 'with oldrev' do let(:oldrev) { noteable.commits[2].id } - it 'includes a commit range' do - expect(summary_line).to start_with "* #{Commit.truncate_sha(oldrev)}...#{old_commits.last.short_id}" - end - - it 'includes a commit count' do - expect(summary_line).to end_with " - 26 commits from branch `feature`" + it 'includes a commit range and count' do + expect(summary_line) + .to start_with("<ul><li>#{Commit.truncate_sha(oldrev)}...#{old_commits.last.short_id} - 26 commits from branch <code>feature</code>") end end context 'without oldrev' do - it 'includes a commit range' do - expect(summary_line).to start_with "* #{old_commits[0].short_id}..#{old_commits[-1].short_id}" - end - - it 'includes a commit count' do - expect(summary_line).to end_with " - 26 commits from branch `feature`" + it 'includes a commit range and count' do + expect(summary_line) + .to start_with("<ul><li>#{old_commits[0].short_id}..#{old_commits[-1].short_id} - 26 commits from branch <code>feature</code>") end end @@ -104,7 +99,7 @@ describe SystemNoteService do end it 'includes the project namespace' do - expect(summary_line).to end_with "`#{noteable.target_project_namespace}:feature`" + expect(summary_line).to include("<code>#{noteable.target_project_namespace}:feature</code>") end end end @@ -693,7 +688,7 @@ describe SystemNoteService do describe '.new_commit_summary' do it 'escapes HTML titles' do commit = double(title: '<pre>This is a test</pre>', short_id: '12345678') - escaped = '<pre>This is a test</pre>' + escaped = '<pre>This is a test</pre>' expect(described_class.new_commit_summary([commit])).to all(match(/- #{escaped}/)) end |