diff options
| -rw-r--r-- | app/finders/notes_finder.rb | 12 | ||||
| -rw-r--r-- | app/views/search/_category.html.haml | 8 | ||||
| -rw-r--r-- | changelogs/unreleased/jprovazn-scoped-limit.yml | 6 | ||||
| -rw-r--r-- | lib/gitlab/project_search_results.rb | 29 | ||||
| -rw-r--r-- | lib/gitlab/search_results.rb | 16 | ||||
| -rw-r--r-- | spec/finders/notes_finder_spec.rb | 12 | ||||
| -rw-r--r-- | spec/lib/gitlab/project_search_results_spec.rb | 41 | ||||
| -rw-r--r-- | spec/lib/gitlab/search_results_spec.rb | 36 |
8 files changed, 97 insertions, 63 deletions
diff --git a/app/finders/notes_finder.rb b/app/finders/notes_finder.rb index 33ee1e975b9..35f4ff2f62f 100644 --- a/app/finders/notes_finder.rb +++ b/app/finders/notes_finder.rb @@ -48,11 +48,23 @@ class NotesFinder def init_collection if target notes_on_target + elsif target_type + notes_of_target_type else notes_of_any_type end end + def notes_of_target_type + notes = notes_for_type(target_type) + + search(notes) + end + + def target_type + @params[:target_type] + end + def notes_of_any_type types = %w(commit issue merge_request snippet) note_relations = types.map { |t| notes_for_type(t) } diff --git a/app/views/search/_category.html.haml b/app/views/search/_category.html.haml index 915e648a5d3..7d43fd61081 100644 --- a/app/views/search/_category.html.haml +++ b/app/views/search/_category.html.haml @@ -14,25 +14,25 @@ = link_to search_filter_path(scope: 'issues') do Issues %span.badge - = @search_results.issues_count + = limited_count(@search_results.limited_issues_count) - if project_search_tabs?(:merge_requests) %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) - if project_search_tabs?(:milestones) %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) - if project_search_tabs?(:notes) %li{ class: active_when(@scope == 'notes') } = link_to search_filter_path(scope: 'notes') do Comments %span.badge - = @search_results.notes_count + = limited_count(@search_results.limited_notes_count) - if project_search_tabs?(:wiki) %li{ class: active_when(@scope == 'wiki_blobs') } = link_to search_filter_path(scope: 'wiki_blobs') do diff --git a/changelogs/unreleased/jprovazn-scoped-limit.yml b/changelogs/unreleased/jprovazn-scoped-limit.yml new file mode 100644 index 00000000000..45724bb3479 --- /dev/null +++ b/changelogs/unreleased/jprovazn-scoped-limit.yml @@ -0,0 +1,6 @@ +--- +title: Optimize search queries on the search page by setting a limit for matching + records in project scope +merge_request: +author: +type: performance diff --git a/lib/gitlab/project_search_results.rb b/lib/gitlab/project_search_results.rb index cf0935dbd9a..29277ec6481 100644 --- a/lib/gitlab/project_search_results.rb +++ b/lib/gitlab/project_search_results.rb @@ -29,8 +29,18 @@ module Gitlab @blobs_count ||= blobs.count end - def notes_count - @notes_count ||= notes.count + def limited_notes_count + return @limited_notes_count if defined?(@limited_notes_count) + + types = %w(issue merge_request commit snippet) + @limited_notes_count = 0 + + types.each do |type| + @limited_notes_count += notes_finder(type).limit(count_limit).count + break if @limited_notes_count >= count_limit + end + + @limited_notes_count end def wiki_blobs_count @@ -72,11 +82,12 @@ module Gitlab end def single_commit_result? - commits_count == 1 && total_result_count == 1 - end + return false if commits_count != 1 - def total_result_count - issues_count + merge_requests_count + milestones_count + notes_count + blobs_count + wiki_blobs_count + commits_count + counts = %i(limited_milestones_count limited_notes_count + limited_merge_requests_count limited_issues_count + blobs_count wiki_blobs_count) + counts.all? { |count_method| public_send(count_method).zero? } # rubocop:disable GitlabSecurity/PublicSend end private @@ -106,7 +117,11 @@ module Gitlab end def notes - @notes ||= NotesFinder.new(project, @current_user, search: query).execute.user.order('updated_at DESC') + @notes ||= notes_finder(nil) + end + + def notes_finder(type) + NotesFinder.new(project, @current_user, search: query, target_type: type).execute.user.order('updated_at DESC') end def commits diff --git a/lib/gitlab/search_results.rb b/lib/gitlab/search_results.rb index 781783f4d97..757ef71b95a 100644 --- a/lib/gitlab/search_results.rb +++ b/lib/gitlab/search_results.rb @@ -62,22 +62,6 @@ module Gitlab without_count ? collection.without_count : collection end - def projects_count - @projects_count ||= projects.count - end - - def issues_count - @issues_count ||= issues.count - end - - def merge_requests_count - @merge_requests_count ||= merge_requests.count - end - - def milestones_count - @milestones_count ||= milestones.count - end - def limited_projects_count @limited_projects_count ||= projects.limit(count_limit).count end diff --git a/spec/finders/notes_finder_spec.rb b/spec/finders/notes_finder_spec.rb index 7b43494eea2..f1ae2c7ab65 100644 --- a/spec/finders/notes_finder_spec.rb +++ b/spec/finders/notes_finder_spec.rb @@ -75,6 +75,18 @@ describe NotesFinder do end end + context 'for target type' do + let(:project) { create(:project, :repository) } + let!(:note1) { create :note_on_issue, project: project } + let!(:note2) { create :note_on_commit, project: project } + + it 'finds only notes for the selected type' do + notes = described_class.new(project, user, target_type: 'issue').execute + + expect(notes).to eq([note1]) + end + end + context 'for target' do let(:project) { create(:project, :repository) } let(:note1) { create :note_on_commit, project: project } diff --git a/spec/lib/gitlab/project_search_results_spec.rb b/spec/lib/gitlab/project_search_results_spec.rb index d8250e4b4c6..c46bb8edebf 100644 --- a/spec/lib/gitlab/project_search_results_spec.rb +++ b/spec/lib/gitlab/project_search_results_spec.rb @@ -217,7 +217,7 @@ describe Gitlab::ProjectSearchResults do expect(issues).to include issue expect(issues).not_to include security_issue_1 expect(issues).not_to include security_issue_2 - expect(results.issues_count).to eq 1 + expect(results.limited_issues_count).to eq 1 end it 'does not list project confidential issues for project members with guest role' do @@ -229,7 +229,7 @@ describe Gitlab::ProjectSearchResults do expect(issues).to include issue expect(issues).not_to include security_issue_1 expect(issues).not_to include security_issue_2 - expect(results.issues_count).to eq 1 + expect(results.limited_issues_count).to eq 1 end it 'lists project confidential issues for author' do @@ -239,7 +239,7 @@ describe Gitlab::ProjectSearchResults do expect(issues).to include issue expect(issues).to include security_issue_1 expect(issues).not_to include security_issue_2 - expect(results.issues_count).to eq 2 + expect(results.limited_issues_count).to eq 2 end it 'lists project confidential issues for assignee' do @@ -249,7 +249,7 @@ describe Gitlab::ProjectSearchResults do expect(issues).to include issue expect(issues).not_to include security_issue_1 expect(issues).to include security_issue_2 - expect(results.issues_count).to eq 2 + expect(results.limited_issues_count).to eq 2 end it 'lists project confidential issues for project members' do @@ -261,7 +261,7 @@ describe Gitlab::ProjectSearchResults do expect(issues).to include issue expect(issues).to include security_issue_1 expect(issues).to include security_issue_2 - expect(results.issues_count).to eq 3 + expect(results.limited_issues_count).to eq 3 end it 'lists all project issues for admin' do @@ -271,7 +271,7 @@ describe Gitlab::ProjectSearchResults do expect(issues).to include issue expect(issues).to include security_issue_1 expect(issues).to include security_issue_2 - expect(results.issues_count).to eq 3 + expect(results.limited_issues_count).to eq 3 end end @@ -304,6 +304,35 @@ describe Gitlab::ProjectSearchResults do end end + describe '#limited_notes_count' do + let(:project) { create(:project, :public) } + let(:note) { create(:note_on_issue, project: project) } + let(:results) { described_class.new(user, project, note.note) } + + context 'when count_limit is lower than total amount' do + before do + allow(results).to receive(:count_limit).and_return(1) + end + + it 'calls note finder once to get the limited amount of notes' do + expect(results).to receive(:notes_finder).once.and_call_original + expect(results.limited_notes_count).to eq(1) + end + end + + context 'when count_limit is higher than total amount' do + it 'calls note finder multiple times to get the limited amount of notes' do + project = create(:project, :public) + note = create(:note_on_issue, project: project) + + results = described_class.new(user, project, note.note) + + expect(results).to receive(:notes_finder).exactly(4).times.and_call_original + expect(results.limited_notes_count).to eq(1) + end + end + end + # Examples for commit access level test # # params: diff --git a/spec/lib/gitlab/search_results_spec.rb b/spec/lib/gitlab/search_results_spec.rb index 9dbab95f70e..87288baedb0 100644 --- a/spec/lib/gitlab/search_results_spec.rb +++ b/spec/lib/gitlab/search_results_spec.rb @@ -29,30 +29,6 @@ describe Gitlab::SearchResults do end end - describe '#projects_count' do - it 'returns the total amount of projects' do - expect(results.projects_count).to eq(1) - end - end - - describe '#issues_count' do - it 'returns the total amount of issues' do - expect(results.issues_count).to eq(1) - end - end - - describe '#merge_requests_count' do - it 'returns the total amount of merge requests' do - expect(results.merge_requests_count).to eq(1) - end - end - - describe '#milestones_count' do - it 'returns the total amount of milestones' do - expect(results.milestones_count).to eq(1) - end - end - context "when count_limit is lower than total amount" do before do allow(results).to receive(:count_limit).and_return(1) @@ -183,7 +159,7 @@ describe Gitlab::SearchResults do expect(issues).not_to include security_issue_3 expect(issues).not_to include security_issue_4 expect(issues).not_to include security_issue_5 - expect(results.issues_count).to eq 1 + expect(results.limited_issues_count).to eq 1 end it 'does not list confidential issues for project members with guest role' do @@ -199,7 +175,7 @@ describe Gitlab::SearchResults do expect(issues).not_to include security_issue_3 expect(issues).not_to include security_issue_4 expect(issues).not_to include security_issue_5 - expect(results.issues_count).to eq 1 + expect(results.limited_issues_count).to eq 1 end it 'lists confidential issues for author' do @@ -212,7 +188,7 @@ describe Gitlab::SearchResults do expect(issues).to include security_issue_3 expect(issues).not_to include security_issue_4 expect(issues).not_to include security_issue_5 - expect(results.issues_count).to eq 3 + expect(results.limited_issues_count).to eq 3 end it 'lists confidential issues for assignee' do @@ -225,7 +201,7 @@ describe Gitlab::SearchResults do expect(issues).not_to include security_issue_3 expect(issues).to include security_issue_4 expect(issues).not_to include security_issue_5 - expect(results.issues_count).to eq 3 + expect(results.limited_issues_count).to eq 3 end it 'lists confidential issues for project members' do @@ -241,7 +217,7 @@ describe Gitlab::SearchResults do expect(issues).to include security_issue_3 expect(issues).not_to include security_issue_4 expect(issues).not_to include security_issue_5 - expect(results.issues_count).to eq 4 + expect(results.limited_issues_count).to eq 4 end it 'lists all issues for admin' do @@ -254,7 +230,7 @@ describe Gitlab::SearchResults do expect(issues).to include security_issue_3 expect(issues).to include security_issue_4 expect(issues).not_to include security_issue_5 - expect(results.issues_count).to eq 5 + expect(results.limited_issues_count).to eq 5 end end |