Commit a349a426 authored by Yorick Peterse's avatar Yorick Peterse

Merge branch 'search-100' into 'master'

Use limit for search count queries

See merge request gitlab-org/gitlab-ce!16502
parents f35bac57 090ca9c3
......@@ -15,6 +15,7 @@
# label_name: string
# sort: string
# my_reaction_emoji: string
# public_only: boolean
#
class IssuesFinder < IssuableFinder
CONFIDENTIAL_ACCESS_LEVEL = Gitlab::Access::REPORTER
......@@ -40,8 +41,16 @@ class IssuesFinder < IssuableFinder
private
def init_collection
if public_only?
Issue.public_only
else
with_confidentiality_access_check
end
end
def public_only?
params.fetch(:public_only, false)
end
def user_can_see_all_confidential_issues?
return @user_can_see_all_confidential_issues if defined?(@user_can_see_all_confidential_issues)
......
......@@ -170,4 +170,8 @@ module SearchHelper
# Truncato's filtered_tags and filtered_attributes are not quite the same
sanitize(html, tags: %w(a p ol ul li pre code))
end
def limited_count(count, limit = 1000)
count > limit ? "#{limit}+" : count
end
end
......@@ -57,25 +57,24 @@
Titles and Filenames
%span.badge
= @search_results.snippet_titles_count
- else
%li{ class: active_when(@scope == 'projects') }
= link_to search_filter_path(scope: 'projects') do
Projects
%span.badge
= @search_results.projects_count
= limited_count(@search_results.limited_projects_count)
%li{ class: active_when(@scope == 'issues') }
= link_to search_filter_path(scope: 'issues') do
Issues
%span.badge
= @search_results.issues_count
= limited_count(@search_results.limited_issues_count)
%li{ class: active_when(@scope == 'merge_requests') }
= link_to search_filter_path(scope: 'merge_requests') do
Merge requests
%span.badge
= @search_results.merge_requests_count
= limited_count(@search_results.limited_merge_requests_count)
%li{ class: active_when(@scope == 'milestones') }
= link_to search_filter_path(scope: 'milestones') do
Milestones
%span.badge
= @search_results.milestones_count
= limited_count(@search_results.limited_milestones_count)
......@@ -2,6 +2,7 @@
= render partial: "search/results/empty"
- else
.row-content-block
- unless @search_objects.is_a?(Kaminari::PaginatableWithoutCount)
= search_entries_info(@search_objects, @scope, @search_term)
- unless @show_snippets
- if @project
......@@ -22,4 +23,4 @@
= render partial: "search/results/#{@scope.singularize}", collection: @search_objects
- if @scope != 'projects'
= paginate(@search_objects, theme: 'gitlab')
= paginate_collection(@search_objects)
---
title: Optimize search queries on the search page by setting a limit for matching records.
merge_request:
author:
type: performance
class AddIndexUpdatedAtToIssues < ActiveRecord::Migration
include Gitlab::Database::MigrationHelpers
DOWNTIME = false
disable_ddl_transaction!
def up
add_concurrent_index :issues, :updated_at
end
def down
remove_concurrent_index :issues, :updated_at
end
end
......@@ -11,7 +11,7 @@
#
# It's strongly recommended that you check this file into your version control system.
ActiveRecord::Schema.define(version: 20180113220114) do
ActiveRecord::Schema.define(version: 20180115201419) do
# These are extensions that must be enabled in order to support this database
enable_extension "plpgsql"
......@@ -886,6 +886,7 @@ ActiveRecord::Schema.define(version: 20180113220114) do
add_index "issues", ["relative_position"], name: "index_issues_on_relative_position", using: :btree
add_index "issues", ["state"], name: "index_issues_on_state", using: :btree
add_index "issues", ["title"], name: "index_issues_on_title_trigram", using: :gin, opclasses: {"title"=>"gin_trgm_ops"}
add_index "issues", ["updated_at"], name: "index_issues_on_updated_at", using: :btree
add_index "issues", ["updated_by_id"], name: "index_issues_on_updated_by_id", where: "(updated_by_id IS NOT NULL)", using: :btree
create_table "keys", force: :cascade do |t|
......
......@@ -175,7 +175,7 @@ module API
end
get "/search/:query", requirements: { query: /[^\/]+/ } do
search_service = Search::GlobalService.new(current_user, search: params[:query]).execute
projects = search_service.objects('projects', params[:page])
projects = search_service.objects('projects', params[:page], false)
projects = projects.reorder(params[:order_by] => params[:sort])
present paginate(projects), with: ::API::V3::Entities::Project
......
......@@ -20,7 +20,7 @@ module Gitlab
when 'commits'
Kaminari.paginate_array(commits).page(page).per(per_page)
else
super
super(scope, page, false)
end
end
......
......@@ -40,8 +40,8 @@ module Gitlab
@default_project_filter = default_project_filter
end
def objects(scope, page = nil)
case scope
def objects(scope, page = nil, without_count = true)
collection = case scope
when 'projects'
projects.page(page).per(per_page)
when 'issues'
......@@ -53,6 +53,8 @@ module Gitlab
else
Kaminari.paginate_array([]).page(page).per(per_page)
end
without_count ? collection.without_count : collection
end
def projects_count
......@@ -71,18 +73,46 @@ module Gitlab
@milestones_count ||= milestones.count
end
def limited_projects_count
@limited_projects_count ||= projects.limit(count_limit).count
end
def limited_issues_count
return @limited_issues_count if @limited_issues_count
# By default getting limited count (e.g. 1000+) is fast on issuable
# collections except for issues, where filtering both not confidential
# and confidential issues user has access to, is too complex.
# It's faster to try to fetch all public issues first, then only
# if necessary try to fetch all issues.
sum = issues(public_only: true).limit(count_limit).count
@limited_issues_count = sum < count_limit ? issues.limit(count_limit).count : sum
end
def limited_merge_requests_count
@limited_merge_requests_count ||= merge_requests.limit(count_limit).count
end
def limited_milestones_count
@limited_milestones_count ||= milestones.limit(count_limit).count
end
def single_commit_result?
false
end
def count_limit
1001
end
private
def projects
limit_projects.search(query)
end
def issues
issues = IssuesFinder.new(current_user).execute
def issues(finder_params = {})
issues = IssuesFinder.new(current_user, finder_params).execute
unless default_project_filter
issues = issues.where(project_id: project_ids_relation)
end
......@@ -94,13 +124,13 @@ module Gitlab
issues.full_search(query)
end
issues.order('updated_at DESC')
issues.reorder('updated_at DESC')
end
def milestones
milestones = Milestone.where(project_id: project_ids_relation)
milestones = milestones.search(query)
milestones.order('updated_at DESC')
milestones.reorder('updated_at DESC')
end
def merge_requests
......@@ -116,7 +146,7 @@ module Gitlab
merge_requests.full_search(query)
end
merge_requests.order('updated_at DESC')
merge_requests.reorder('updated_at DESC')
end
def default_scope
......
......@@ -16,7 +16,7 @@ module Gitlab
when 'snippet_blobs'
snippet_blobs.page(page).per(per_page)
else
super
super(scope, nil, false)
end
end
......
......@@ -22,7 +22,7 @@ feature 'Global search' do
click_button "Go"
select_filter("Issues")
expect(page).to have_selector('.gl-pagination .page', count: 2)
expect(page).to have_selector('.gl-pagination .next')
end
end
end
......@@ -19,6 +19,12 @@ describe Gitlab::SearchResults do
project.add_developer(user)
end
describe '#objects' do
it 'returns without_page collection by default' do
expect(results.objects('projects')).to be_kind_of(Kaminari::PaginatableWithoutCount)
end
end
describe '#projects_count' do
it 'returns the total amount of projects' do
expect(results.projects_count).to eq(1)
......@@ -43,6 +49,58 @@ describe Gitlab::SearchResults do
end
end
context "when count_limit is lower than total amount" do
before do
allow(results).to receive(:count_limit).and_return(1)
end
describe '#limited_projects_count' do
it 'returns the limited amount of projects' do
create(:project, name: 'foo2')
expect(results.limited_projects_count).to eq(1)
end
end
describe '#limited_merge_requests_count' do
it 'returns the limited amount of merge requests' do
create(:merge_request, :simple, source_project: project, title: 'foo2')
expect(results.limited_merge_requests_count).to eq(1)
end
end
describe '#limited_milestones_count' do
it 'returns the limited amount of milestones' do
create(:milestone, project: project, title: 'foo2')
expect(results.limited_milestones_count).to eq(1)
end
end
describe '#limited_issues_count' do
it 'runs single SQL query to get the limited amount of issues' do
create(:milestone, project: project, title: 'foo2')
expect(results).to receive(:issues).with(public_only: true).and_call_original
expect(results).not_to receive(:issues).with(no_args).and_call_original
expect(results.limited_issues_count).to eq(1)
end
end
end
context "when count_limit is higher than total amount" do
describe '#limited_issues_count' do
it 'runs multiple queries to get the limited amount of issues' do
expect(results).to receive(:issues).with(public_only: true).and_call_original
expect(results).to receive(:issues).with(no_args).and_call_original
expect(results.limited_issues_count).to eq(1)
end
end
end
it 'includes merge requests from source and target projects' do
forked_project = fork_project(project, user)
merge_request_2 = create(:merge_request, target_project: project, source_project: forked_project, title: 'foo')
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment