Merge branch 'ee-users-autocomplete' into 'master'

EE: Improve performance of searching for and auto completing of users

See merge request gitlab-org/gitlab-ee!4575

app/assets/javascripts/filtered_search/dropdown_user.js

@@ -14,7 +14,6 @@ export default class DropdownUser extends FilteredSearchDropdown {
         endpoint: `${gon.relative_url_root || ''}/autocomplete/users.json`,
         searchKey: 'search',
         params: {
-          per_page: 20,
           active: true,
           group_id: this.getGroupId(),
           project_id: this.getProjectId(),

app/assets/javascripts/users_select.js

@@ -39,7 +39,6 @@ function UsersSelect(currentUser, els, options = {}) {
       options.showCurrentUser = $dropdown.data('currentUser');
       options.todoFilter = $dropdown.data('todoFilter');
       options.todoStateFilter = $dropdown.data('todoStateFilter');
-      options.perPage = $dropdown.data('perPage');
       showNullUser = $dropdown.data('nullUser');
       defaultNullUser = $dropdown.data('nullUserDefault');
       showMenuAbove = $dropdown.data('showMenuAbove');
@@ -669,7 +668,6 @@ UsersSelect.prototype.users = function(query, options, callback) {
   const url = this.buildUrl(this.usersPath);
   const params = {
     search: query,
-    per_page: options.perPage || 20,
     active: true,
     project_id: options.projectId || null,
     group_id: options.groupId || null,

app/finders/autocomplete_users_finder.rb

 class AutocompleteUsersFinder
+  # The number of users to display in the results is hardcoded to 20, and
+  # pagination is not supported. This ensures that performance remains
+  # consistent and removes the need for implementing keyset pagination to ensure
+  # good performance.
+  LIMIT = 20
+
   attr_reader :current_user, :project, :group, :search, :skip_users,
-              :page, :per_page, :author_id, :params
+              :author_id, :params
 
   # EE
   attr_reader :skip_ldap
@@ -11,8 +17,6 @@ class AutocompleteUsersFinder
     @group = group
     @search = params[:search]
     @skip_users = params[:skip_users]
-    @page = params[:page]
-    @per_page = params[:per_page]
     @author_id = params[:author_id]
     @params = params
 
@@ -30,9 +34,10 @@ class AutocompleteUsersFinder
     items = items.reorder(:name)
     items = items.search(search) if search.present?
     items = items.where.not(id: skip_users) if skip_users.present?
+    items = items.limit(LIMIT)
 
     # EE
-    items = load_users_by_push_ability(items) || items.page(page).per(per_page)
+    items = load_users_by_push_ability(items)
 
     if params[:todo_filter].present? && current_user
       items = items.todo_authors(current_user.id, params[:todo_state_filter])
@@ -64,22 +69,25 @@ class AutocompleteUsersFinder
   end
 
   def users_from_project
-    user_ids = project.team.users.pluck(:id)
-    user_ids << author_id if author_id.present?
+    if author_id.present?
+      union = Gitlab::SQL::Union
+        .new([project.authorized_users, User.where(id: author_id)])
 
-    User.where(id: user_ids)
+      User.from("(#{union.to_sql}) #{User.table_name}")
+    else
+      project.authorized_users
+    end
   end
 
   # EE
   def load_users_by_push_ability(items)
-    return unless project
+    return items unless project
 
     ability = push_ability
-    return if ability.blank?
+    return items if ability.blank?
 
     items.to_a
       .select { |user| user.can?(ability, project) }
-      .take(per_page&.to_i || Kaminari.config.default_per_page)
   end
 
   def push_ability

app/models/user.rb

@@ -340,8 +340,8 @@ class User < ActiveRecord::Base
       SQL
 
       where(
-        fuzzy_arel_match(:name, query)
-          .or(fuzzy_arel_match(:username, query))
+        fuzzy_arel_match(:name, query, lower_exact_match: true)
+          .or(fuzzy_arel_match(:username, query, lower_exact_match: true))
           .or(arel_table[:email].eq(query))
       ).reorder(order % { query: ActiveRecord::Base.connection.quote(query) }, :name)
     end

changelogs/unreleased/users-autocomplete.yml

+---
+title: Improve performance of searching for and autocompleting of users
+merge_request:
+author:
+type: performance

db/migrate/20180215181245_users_name_lower_index.rb

+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class UsersNameLowerIndex < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  # Set this constant to true if this migration requires downtime.
+  DOWNTIME = false
+  INDEX_NAME = 'index_on_users_name_lower'
+
+  disable_ddl_transaction!
+
+  def up
+    return unless Gitlab::Database.postgresql?
+
+    # On GitLab.com this produces an index with a size of roughly 60 MB.
+    execute "CREATE INDEX CONCURRENTLY #{INDEX_NAME} ON users (LOWER(name))"
+  end
+
+  def down
+    return unless Gitlab::Database.postgresql?
+
+    if supports_drop_index_concurrently?
+      execute "DROP INDEX CONCURRENTLY IF EXISTS #{INDEX_NAME}"
+    else
+      execute "DROP INDEX IF EXISTS #{INDEX_NAME}"
+    end
+  end
+end

db/schema.rb

@@ -11,7 +11,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 20180215143644) do
+ActiveRecord::Schema.define(version: 20180215181245) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"

lib/gitlab/sql/pattern.rb

@@ -25,7 +25,11 @@ module Gitlab
           query.length >= MIN_CHARS_FOR_PARTIAL_MATCHING
         end
 
-        def fuzzy_arel_match(column, query)
+        # column - The column name to search in.
+        # query - The text to search for.
+        # lower_exact_match - When set to `true` we'll fall back to using
+        #                     `LOWER(column) = query` instead of using `ILIKE`.
+        def fuzzy_arel_match(column, query, lower_exact_match: false)
           query = query.squish
           return nil unless query.present?
 
@@ -36,7 +40,13 @@ module Gitlab
           else
             # No words of at least 3 chars, but we can search for an exact
             # case insensitive match with the query as a whole
-            arel_table[column].matches(sanitize_sql_like(query))
+            if lower_exact_match
+              Arel::Nodes::NamedFunction
+                .new('LOWER', [arel_table[column]])
+                .eq(query)
+            else
+              arel_table[column].matches(sanitize_sql_like(query))
+            end
           end
         end
 

lib/tasks/migrate/setup_postgresql.rake

@@ -8,6 +8,7 @@ task setup_postgresql: :environment do
   require Rails.root.join('db/migrate/20170503185032_index_redirect_routes_path_for_like')
   require Rails.root.join('db/migrate/20171220191323_add_index_on_namespaces_lower_name.rb')
   require Rails.root.join('db/migrate/20180113220114_rework_redirect_routes_indexes.rb')
+  require Rails.root.join('db/migrate/20180215181245_users_name_lower_index.rb')
 
   NamespacesProjectsPathLowerIndexes.new.up
   AddUsersLowerUsernameEmailIndexes.new.up
@@ -17,6 +18,7 @@ task setup_postgresql: :environment do
   IndexRedirectRoutesPathForLike.new.up
   AddIndexOnNamespacesLowerName.new.up
   ReworkRedirectRoutesIndexes.new.up
+  UsersNameLowerIndex.new.up
 end
 
 desc 'GitLab | Generate PostgreSQL Password Hash'

spec/controllers/autocomplete_controller_spec.rb

@@ -109,15 +109,17 @@ describe AutocompleteController do
     end
 
     context 'limited users per page' do
-      let(:per_page) { 2 }
-
       before do
+        25.times do
+          create(:user)
+        end
+
         sign_in(user)
-        get(:users, per_page: per_page)
+        get(:users)
       end
 
       it { expect(json_response).to be_kind_of(Array) }
-      it { expect(json_response.size).to eq(per_page) }
+      it { expect(json_response.size).to eq(20) }
     end
 
     context 'unauthenticated user' do

spec/lib/gitlab/sql/pattern_spec.rb

@@ -154,6 +154,12 @@ describe Gitlab::SQL::Pattern do
       it 'returns a single equality condition' do
         expect(fuzzy_arel_match.to_sql).to match(/title.*I?LIKE 'fo'/)
       end
+
+      it 'uses LOWER instead of ILIKE when LOWER is enabled' do
+        rel = Issue.fuzzy_arel_match(:title, query, lower_exact_match: true)
+
+        expect(rel.to_sql).to match(/LOWER\(.*title.*\).*=.*'fo'/)
+      end
     end
 
     context 'with two words both equal to 3 chars' do