Add latest changes from gitlab-org/gitlab@master

.gitlab/merge_request_templates/Database changes.md

-## 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"

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

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:'

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

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)

doc/development/database_merge_request_checklist.md

-# 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>

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:
-- 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.
+In order to make reviewing easier and therefore faster, please take
+the following preparations into account.
+#### 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

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.
 ![filter issues by specific terms](img/issue_search_by_term.png)

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