diff options
| -rw-r--r-- | .gitlab/merge_request_templates/Database changes.md | 50 | ||||
| -rw-r--r-- | app/controllers/search_controller.rb | 25 | ||||
| -rw-r--r-- | config/locales/en.yml | 2 | ||||
| -rw-r--r-- | danger/database/Dangerfile | 4 | ||||
| -rw-r--r-- | doc/development/README.md | 1 | ||||
| -rw-r--r-- | doc/development/database_merge_request_checklist.md | 15 | ||||
| -rw-r--r-- | doc/development/database_review.md | 60 | ||||
| -rw-r--r-- | doc/user/search/index.md | 1 | ||||
| -rw-r--r-- | spec/controllers/search_controller_spec.rb | 42 |
9 files changed, 121 insertions, 79 deletions
diff --git a/.gitlab/merge_request_templates/Database changes.md b/.gitlab/merge_request_templates/Database changes.md deleted file mode 100644 index 89c8c7a5d07..00000000000 --- a/.gitlab/merge_request_templates/Database changes.md +++ /dev/null @@ -1,50 +0,0 @@ -## What does this MR do? - -<!-- -Describe in detail what your merge request does, why it does that, etc. Merge -requests without an adequate description will not be reviewed until one is -added. - -Please also keep this description up-to-date with any discussion that takes -place so that reviewers can understand your intent. This is especially -important if they didn't participate in the discussion. - -Make sure to remove this comment when you are done. ---> - -Add a description of your merge request here. - -## Database checklist - -- [ ] Conforms to the [database guides](https://docs.gitlab.com/ee/development/README.html#database-guides) - -When adding migrations: - -- [ ] Updated `db/schema.rb` -- [ ] Added a `down` method so the migration can be reverted -- [ ] Added the output of the migration(s) to the MR body -- [ ] Added tests for the migration in `spec/migrations` if necessary (e.g. when migrating data) -- [ ] Added rollback procedure. Include either a rollback procedure or description how to rollback changes - -When adding or modifying queries to improve performance: - -- [ ] Included data that shows the performance improvement, preferably in the form of a benchmark -- [ ] Included the output of `EXPLAIN (ANALYZE, BUFFERS)` of the relevant queries - -When adding foreign keys to existing tables: - -- [ ] Included a migration to remove orphaned rows in the source table before adding the foreign key -- [ ] Removed any instances of `dependent: ...` that may no longer be necessary - -When adding tables: - -- [ ] Ordered columns based on the [Ordering Table Columns](https://docs.gitlab.com/ee/development/ordering_table_columns.html) guidelines -- [ ] Added foreign keys to any columns pointing to data in other tables -- [ ] Added indexes for fields that are used in statements such as `WHERE`, `ORDER BY`, `GROUP BY`, and `JOIN`s - -When removing columns, tables, indexes or other structures: - -- [ ] Removed these in a post-deployment migration -- [ ] Made sure the application no longer uses (or ignores) these structures - -/label ~database ~"database::review pending" diff --git a/app/controllers/search_controller.rb b/app/controllers/search_controller.rb index e30935be4b6..b6e24a450e8 100644 --- a/app/controllers/search_controller.rb +++ b/app/controllers/search_controller.rb @@ -5,6 +5,9 @@ class SearchController < ApplicationController include SearchHelper include RendersCommits + NON_ES_SEARCH_TERM_LIMIT = 64 + NON_ES_SEARCH_CHAR_LIMIT = 4096 + around_action :allow_gitaly_ref_name_caching skip_before_action :authenticate_user! @@ -21,6 +24,8 @@ class SearchController < ApplicationController return if params[:search].blank? + return unless search_term_valid? + @search_term = params[:search] @scope = search_service.scope @@ -62,6 +67,26 @@ class SearchController < ApplicationController private + def search_term_valid? + return true if Gitlab::CurrentSettings.elasticsearch_search? + + chars_count = params[:search].length + if chars_count > NON_ES_SEARCH_CHAR_LIMIT + flash[:alert] = t('errors.messages.search_chars_too_long', count: NON_ES_SEARCH_CHAR_LIMIT) + + return false + end + + search_terms_count = params[:search].split.count { |word| word.length >= 3 } + if search_terms_count > NON_ES_SEARCH_TERM_LIMIT + flash[:alert] = t('errors.messages.search_terms_too_long', count: NON_ES_SEARCH_TERM_LIMIT) + + return false + end + + true + end + def render_commits @search_objects = prepare_commits_for_rendering(@search_objects) end diff --git a/config/locales/en.yml b/config/locales/en.yml index 950529f0355..dabcefba169 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -195,6 +195,8 @@ en: wrong_length: one: is the wrong length (should be 1 character) other: is the wrong length (should be %{count} characters) + search_chars_too_long: Search query is too long (maximum is %{count} characters) + search_terms_too_long: Search query is too long (maximum is %{count} terms) other_than: must be other than %{count} template: body: 'There were problems with the following fields:' diff --git a/danger/database/Dangerfile b/danger/database/Dangerfile index 56624c0b897..16740cb867d 100644 --- a/danger/database/Dangerfile +++ b/danger/database/Dangerfile @@ -20,8 +20,8 @@ changes are reviewed, take the following steps: 1. Ensure the merge request has ~database and ~"database::review pending" labels. If the merge request modifies database files, Danger will do this for you. -1. Use the [Database changes checklist](https://gitlab.com/gitlab-org/gitlab/blob/master/.gitlab/merge_request_templates/Database%20changes.md) - template or add the appropriate items to the MR description. +1. Prepare your MR for database review according to the + [docs](https://docs.gitlab.com/ee/development/database_review.html#how-to-prepare-the-merge-request-for-a-database-review). 1. Assign and mention the database reviewer suggested by Reviewer Roulette. MSG diff --git a/doc/development/README.md b/doc/development/README.md index 53133366c78..684f6d01d12 100644 --- a/doc/development/README.md +++ b/doc/development/README.md @@ -111,7 +111,6 @@ description: 'Learn how to contribute to GitLab.' ### Best practices -- [Merge Request checklist](database_merge_request_checklist.md) - [Adding database indexes](adding_database_indexes.md) - [Foreign keys & associations](foreign_keys.md) - [Single table inheritance](single_table_inheritance.md) diff --git a/doc/development/database_merge_request_checklist.md b/doc/development/database_merge_request_checklist.md deleted file mode 100644 index 09dece27e8d..00000000000 --- a/doc/development/database_merge_request_checklist.md +++ /dev/null @@ -1,15 +0,0 @@ -# Merge Request Checklist - -When creating a merge request that performs database related changes (schema -changes, adjusting queries to optimize performance, etc) you should use the -merge request template called "Database changes". This template contains a -checklist of steps to follow to make sure the changes are up to snuff. - -To use the checklist, create a new merge request and click on the "Choose a -template" dropdown, then click "Database changes". - -An example of this checklist can be found at -<https://gitlab.com/gitlab-org/gitlab-foss/merge_requests/12463>. - -The source code of the checklist can be found in at -<https://gitlab.com/gitlab-org/gitlab/blob/master/.gitlab/merge_request_templates/Database%20changes.md> diff --git a/doc/development/database_review.md b/doc/development/database_review.md index b1c3ed47976..5ca77579eec 100644 --- a/doc/development/database_review.md +++ b/doc/development/database_review.md @@ -32,12 +32,10 @@ for review. ### Roles and process -A Merge Request author's role is to: +A Merge Request **author**'s role is to: - Decide whether a database review is needed. - If database review is needed, add the ~database label. -- Use the [database changes](https://gitlab.com/gitlab-org/gitlab/blob/master/.gitlab/merge_request_templates/Database%20changes.md) - merge request template, or include the appropriate items in the MR description. - [Prepare the merge request for a database review](#how-to-prepare-the-merge-request-for-a-database-review). A database **reviewer**'s role is to: @@ -78,15 +76,54 @@ make sure you have applied the ~database label and rerun the ### How to prepare the merge request for a database review -In order to make reviewing easier and therefore faster, please consider preparing a comment -and details for a database reviewer: +In order to make reviewing easier and therefore faster, please take +the following preparations into account. -- Provide queries in SQL form rather than ActiveRecord. -- Format any queries with a SQL query formatter, for example with [sqlformat.darold.net](http://sqlformat.darold.net). -- Consider providing query plans via a link to [explain.depesz.com](https://explain.depesz.com) or another tool instead of textual form. -- For query changes, it is best to provide the SQL query along with a plan *before* and *after* the change. This helps to spot differences quickly. -- When providing query plans, make sure to use good parameter values, so that the query executed is a good example and also hits enough data. - - Usually, the `gitlab-org` namespace (`namespace_id = 9970`) and the `gitlab-org/gitlab-foss` (`project_id = 13083`) or the `gitlab-org/gitlab` (`project_id = 278964`) projects provide enough data to serve as a good example. +#### Preparation when adding migrations + +- Ensure `db/schema.rb` is updated. +- Make migrations reversible by using the `change` method or include a `down` method when using `up`. + - Include either a rollback procedure or describe how to rollback changes. +- Add the output of the migration(s) to the MR description. +- Add tests for the migration in `spec/migrations` if necessary. See [Testing Rails migrations at GitLab](testing_guide/testing_migrations_guide.html) for more details. + +#### Preparation when adding or modifying queries + +- Write the raw SQL in the MR description. Preferably formatted + nicely with [sqlformat.darold.net](http://sqlformat.darold.net) or + [paste.depesz.com](https://paste.depesz.com). +- Include the output of `EXPLAIN (ANALYZE, BUFFERS)` of the relevant + queries in the description. If the output is too long, wrap it in + `<details>` blocks, paste it in a GitLab Snippet, or provide the + link to the plan at: [explain.depesz.com](https://explain.depesz.com). +- When providing query plans, make sure it hits enough data: + - You can use a GitLab production replica to test your queries on a large scale, + through the `#database-lab` Slack channel or through [chatops](understanding_explain_plans.md#chatops). + - Usually, the `gitlab-org` namespace (`namespace_id = 9970`) and the + `gitlab-org/gitlab-foss` (`project_id = 13083`) or the `gitlab-org/gitlab` (`project_id = 278964`) + projects provide enough data to serve as a good example. +- For query changes, it is best to provide the SQL query along with a + plan _before_ and _after_ the change. This helps to spot differences + quickly. +- Include data that shows the performance improvement, preferably in + the form of a benchmark. + +#### Preparation when adding foreign keys to existing tables + +- Include a migration to remove orphaned rows in the source table **before** adding the foreign key. +- Remove any instances of `dependent: ...` that may no longer be necessary. + +#### Preparation when adding tables + +- Order columns based on the [Ordering Table Columns](ordering_table_columns.md) guidelines. +- Add foreign keys to any columns pointing to data in other tables, including [an index](migration_style_guide.md#adding-foreign-key-constraints). +- Add indexes for fields that are used in statements such as `WHERE`, `ORDER BY`, `GROUP BY`, and `JOIN`s. + +#### Preparation when removing columns, tables, indexes or other structures + +- Follow the [guidelines on dropping columns](what_requires_downtime.md#dropping-columns). +- Generally it's best practice, but not a hard rule, to remove indexes and foreign keys in a post-deployment migration. + - Exceptions include removing indexes and foreign keys for small tables. ### How to review for database @@ -136,6 +173,7 @@ and details for a database reviewer: (eg indexes, columns), you can use a [one-off instance from the restore pipeline](https://ops.gitlab.net/gitlab-com/gl-infra/gitlab-restore/postgres-gprd) in order to establish a proper testing environment. + - Avoid N+1 problems and minimalize the [query count](merge_request_performance_guidelines.md#query-counts). ### Timing guidelines for migrations diff --git a/doc/user/search/index.md b/doc/user/search/index.md index bc31052b758..68aef567270 100644 --- a/doc/user/search/index.md +++ b/doc/user/search/index.md @@ -62,6 +62,7 @@ You can filter issues and merge requests by specific terms included in titles or - Limitation - For performance reasons, terms shorter than 3 chars are ignored. E.g.: searching issues for `included in titles` is same as `included titles` + - Search is limited to 4096 characters and 64 terms per query.  diff --git a/spec/controllers/search_controller_spec.rb b/spec/controllers/search_controller_spec.rb index 3dcafae295a..1559a50d016 100644 --- a/spec/controllers/search_controller_spec.rb +++ b/spec/controllers/search_controller_spec.rb @@ -92,6 +92,7 @@ describe SearchController do end context 'global search' do + using RSpec::Parameterized::TableSyntax render_views it 'omits pipeline status from load' do @@ -102,6 +103,47 @@ describe SearchController do expect(assigns[:search_objects].first).to eq project end + + context 'check search term length' do + let(:search_queries) do + char_limit = controller.class::NON_ES_SEARCH_CHAR_LIMIT + term_limit = controller.class::NON_ES_SEARCH_TERM_LIMIT + { + chars_under_limit: ('a' * (char_limit - 1)), + chars_over_limit: ('a' * (char_limit + 1)), + terms_under_limit: ('abc ' * (term_limit - 1)), + terms_over_limit: ('abc ' * (term_limit + 1)) + } + end + + where(:es_enabled, :string_name, :expectation) do + true | :chars_under_limit | :not_to_set_flash + true | :chars_over_limit | :not_to_set_flash + true | :terms_under_limit | :not_to_set_flash + true | :terms_over_limit | :not_to_set_flash + false | :chars_under_limit | :not_to_set_flash + false | :chars_over_limit | :set_chars_flash + false | :terms_under_limit | :not_to_set_flash + false | :terms_over_limit | :set_terms_flash + end + + with_them do + it do + allow(Gitlab::CurrentSettings).to receive(:elasticsearch_search?).and_return(es_enabled) + + get :show, params: { scope: 'projects', search: search_queries[string_name] } + + case expectation + when :not_to_set_flash + expect(controller).not_to set_flash[:alert] + when :set_chars_flash + expect(controller).to set_flash[:alert].to(/characters/) + when :set_terms_flash + expect(controller).to set_flash[:alert].to(/terms/) + end + end + end + end end it 'finds issue comments' do |