summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorDouwe Maan <douwe@gitlab.com>2018-02-23 13:47:43 +0000
committerDouwe Maan <douwe@gitlab.com>2018-02-23 13:47:43 +0000
commitbb0fe96f75c6a39e57ac5e9f1895a85f8453e3a5 (patch)
treefe269a7d864843602b7fe88cfad8650dd4e0b4a8
parent58a312f5097b30a93100de93d06427402d514b48 (diff)
parent090eeb581b3809ab83d52f7baa2bcfbd63b1c2ba (diff)
downloadgitlab-ce-bb0fe96f75c6a39e57ac5e9f1895a85f8453e3a5.tar.gz
Merge branch 'users-autocomplete' into 'master'
Improve performance of searching for and auto completing of users See merge request gitlab-org/gitlab-ce!17158
-rw-r--r--app/assets/javascripts/filtered_search/dropdown_user.js1
-rw-r--r--app/assets/javascripts/users_select.js2
-rw-r--r--app/finders/autocomplete_users_finder.rb22
-rw-r--r--app/models/user.rb4
-rw-r--r--changelogs/unreleased/users-autocomplete.yml5
-rw-r--r--db/migrate/20180215181245_users_name_lower_index.rb29
-rw-r--r--db/schema.rb2
-rw-r--r--lib/gitlab/sql/pattern.rb14
-rw-r--r--lib/tasks/migrate/setup_postgresql.rake2
-rw-r--r--spec/controllers/autocomplete_controller_spec.rb10
-rw-r--r--spec/lib/gitlab/sql/pattern_spec.rb6
11 files changed, 78 insertions, 19 deletions
diff --git a/app/assets/javascripts/filtered_search/dropdown_user.js b/app/assets/javascripts/filtered_search/dropdown_user.js
index 22421fc4868..d36f38a70b5 100644
--- a/app/assets/javascripts/filtered_search/dropdown_user.js
+++ b/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(),
diff --git a/app/assets/javascripts/users_select.js b/app/assets/javascripts/users_select.js
index 8958534689c..3385aba0279 100644
--- a/app/assets/javascripts/users_select.js
+++ b/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,
diff --git a/app/finders/autocomplete_users_finder.rb b/app/finders/autocomplete_users_finder.rb
index c3f5358b577..e8a03947f59 100644
--- a/app/finders/autocomplete_users_finder.rb
+++ b/app/finders/autocomplete_users_finder.rb
@@ -1,6 +1,12 @@
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
def initialize(params:, current_user:, project:, group:)
@current_user = current_user
@@ -8,8 +14,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
end
@@ -20,7 +24,7 @@ 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.page(page).per(per_page)
+ items = items.limit(LIMIT)
if params[:todo_filter].present? && current_user
items = items.todo_authors(current_user.id, params[:todo_state_filter])
@@ -52,9 +56,13 @@ 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
end
diff --git a/app/models/user.rb b/app/models/user.rb
index f5eeba27572..8610ca27b7f 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -327,8 +327,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
diff --git a/changelogs/unreleased/users-autocomplete.yml b/changelogs/unreleased/users-autocomplete.yml
new file mode 100644
index 00000000000..2cb078a3a7c
--- /dev/null
+++ b/changelogs/unreleased/users-autocomplete.yml
@@ -0,0 +1,5 @@
+---
+title: Improve performance of searching for and autocompleting of users
+merge_request:
+author:
+type: performance
diff --git a/db/migrate/20180215181245_users_name_lower_index.rb b/db/migrate/20180215181245_users_name_lower_index.rb
new file mode 100644
index 00000000000..d3f68cb7d45
--- /dev/null
+++ b/db/migrate/20180215181245_users_name_lower_index.rb
@@ -0,0 +1,29 @@
+# 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
diff --git a/db/schema.rb b/db/schema.rb
index 409d1ac7644..c0ce87302cf 100644
--- a/db/schema.rb
+++ b/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: 20180213131630) do
+ActiveRecord::Schema.define(version: 20180215181245) do
# These are extensions that must be enabled in order to support this database
enable_extension "plpgsql"
diff --git a/lib/gitlab/sql/pattern.rb b/lib/gitlab/sql/pattern.rb
index 5f0c98cb5a4..53744bad1f4 100644
--- a/lib/gitlab/sql/pattern.rb
+++ b/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
diff --git a/lib/tasks/migrate/setup_postgresql.rake b/lib/tasks/migrate/setup_postgresql.rake
index 31cbd651edb..1c7a8a90f5c 100644
--- a/lib/tasks/migrate/setup_postgresql.rake
+++ b/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,4 +18,5 @@ task setup_postgresql: :environment do
IndexRedirectRoutesPathForLike.new.up
AddIndexOnNamespacesLowerName.new.up
ReworkRedirectRoutesIndexes.new.up
+ UsersNameLowerIndex.new.up
end
diff --git a/spec/controllers/autocomplete_controller_spec.rb b/spec/controllers/autocomplete_controller_spec.rb
index 73fff6eb5ca..b7257fac608 100644
--- a/spec/controllers/autocomplete_controller_spec.rb
+++ b/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
diff --git a/spec/lib/gitlab/sql/pattern_spec.rb b/spec/lib/gitlab/sql/pattern_spec.rb
index ef51e3cc8df..5b5052de372 100644
--- a/spec/lib/gitlab/sql/pattern_spec.rb
+++ b/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