Commit 9db15d2a authored by Rémy Coutable's avatar Rémy Coutable

Merge branch 'speed-up-issue-counting-for-a-project' into 'master'

Speed up issue counting for a project

Closes #33913

See merge request !12457
parents d52034b2 4e38985b
...@@ -20,6 +20,7 @@ ...@@ -20,6 +20,7 @@
# #
class IssuableFinder class IssuableFinder
NONE = '0'.freeze NONE = '0'.freeze
IRRELEVANT_PARAMS_FOR_CACHE_KEY = %i[utf8 sort page].freeze
attr_accessor :current_user, :params attr_accessor :current_user, :params
...@@ -62,7 +63,7 @@ class IssuableFinder ...@@ -62,7 +63,7 @@ class IssuableFinder
# grouping and counting within that query. # grouping and counting within that query.
# #
def count_by_state def count_by_state
count_params = params.merge(state: nil, sort: nil) count_params = params.merge(state: nil, sort: nil, for_counting: true)
labels_count = label_names.any? ? label_names.count : 1 labels_count = label_names.any? ? label_names.count : 1
finder = self.class.new(current_user, count_params) finder = self.class.new(current_user, count_params)
counts = Hash.new(0) counts = Hash.new(0)
...@@ -86,6 +87,10 @@ class IssuableFinder ...@@ -86,6 +87,10 @@ class IssuableFinder
execute.find_by!(*params) execute.find_by!(*params)
end end
def state_counter_cache_key(state)
Digest::SHA1.hexdigest(state_counter_cache_key_components(state).flatten.join('-'))
end
def group def group
return @group if defined?(@group) return @group if defined?(@group)
...@@ -418,4 +423,13 @@ class IssuableFinder ...@@ -418,4 +423,13 @@ class IssuableFinder
def current_user_related? def current_user_related?
params[:scope] == 'created-by-me' || params[:scope] == 'authored' || params[:scope] == 'assigned-to-me' params[:scope] == 'created-by-me' || params[:scope] == 'authored' || params[:scope] == 'assigned-to-me'
end end
def state_counter_cache_key_components(state)
opts = params.with_indifferent_access
opts[:state] = state
opts.except!(*IRRELEVANT_PARAMS_FOR_CACHE_KEY)
opts.delete_if { |_, value| value.blank? }
['issuables_count', klass.to_ability_name, opts.sort]
end
end end
...@@ -16,14 +16,72 @@ ...@@ -16,14 +16,72 @@
# sort: string # sort: string
# #
class IssuesFinder < IssuableFinder class IssuesFinder < IssuableFinder
CONFIDENTIAL_ACCESS_LEVEL = Gitlab::Access::REPORTER
def klass def klass
Issue Issue
end end
def with_confidentiality_access_check
return Issue.all if user_can_see_all_confidential_issues?
return Issue.where('issues.confidential IS NOT TRUE') if user_cannot_see_confidential_issues?
Issue.where('
issues.confidential IS NOT TRUE
OR (issues.confidential = TRUE
AND (issues.author_id = :user_id
OR EXISTS (SELECT TRUE FROM issue_assignees WHERE user_id = :user_id AND issue_id = issues.id)
OR issues.project_id IN(:project_ids)))',
user_id: current_user.id,
project_ids: current_user.authorized_projects(CONFIDENTIAL_ACCESS_LEVEL).select(:id))
end
private private
def init_collection def init_collection
IssuesFinder.not_restricted_by_confidentiality(current_user) with_confidentiality_access_check
end
def user_can_see_all_confidential_issues?
return @user_can_see_all_confidential_issues if defined?(@user_can_see_all_confidential_issues)
return @user_can_see_all_confidential_issues = false if current_user.blank?
return @user_can_see_all_confidential_issues = true if current_user.full_private_access?
@user_can_see_all_confidential_issues =
project? &&
project &&
project.team.max_member_access(current_user.id) >= CONFIDENTIAL_ACCESS_LEVEL
end
# Anonymous users can't see any confidential issues.
#
# Users without access to see _all_ confidential issues (as in
# `user_can_see_all_confidential_issues?`) are more complicated, because they
# can see confidential issues where:
# 1. They are an assignee.
# 2. They are an author.
#
# That's fine for most cases, but if we're just counting, we need to cache
# effectively. If we cached this accurately, we'd have a cache key for every
# authenticated user without sufficient access to the project. Instead, when
# we are counting, we treat them as if they can't see any confidential issues.
#
# This does mean the counts may be wrong for those users, but avoids an
# explosion in cache keys.
def user_cannot_see_confidential_issues?(for_counting: false)
return false if user_can_see_all_confidential_issues?
current_user.blank? || for_counting || params[:for_counting]
end
def state_counter_cache_key_components(state)
extra_components = [
user_can_see_all_confidential_issues?,
user_cannot_see_confidential_issues?(for_counting: true)
]
super + extra_components
end end
def by_assignee(items) def by_assignee(items)
...@@ -38,21 +96,6 @@ class IssuesFinder < IssuableFinder ...@@ -38,21 +96,6 @@ class IssuesFinder < IssuableFinder
end end
end end
def self.not_restricted_by_confidentiality(user)
return Issue.where('issues.confidential IS NOT TRUE') if user.blank?
return Issue.all if user.full_private_access?
Issue.where('
issues.confidential IS NOT TRUE
OR (issues.confidential = TRUE
AND (issues.author_id = :user_id
OR EXISTS (SELECT TRUE FROM issue_assignees WHERE user_id = :user_id AND issue_id = issues.id)
OR issues.project_id IN(:project_ids)))',
user_id: user.id,
project_ids: user.authorized_projects(Gitlab::Access::REPORTER).select(:id))
end
def item_project_ids(items) def item_project_ids(items)
items&.reorder(nil)&.select(:project_id) items&.reorder(nil)&.select(:project_id)
end end
......
...@@ -165,11 +165,7 @@ module IssuablesHelper ...@@ -165,11 +165,7 @@ module IssuablesHelper
} }
state_title = titles[state] || state.to_s.humanize state_title = titles[state] || state.to_s.humanize
count = issuables_count_for_state(issuable_type, state)
count =
Rails.cache.fetch(issuables_state_counter_cache_key(issuable_type, state), expires_in: 2.minutes) do
issuables_count_for_state(issuable_type, state)
end
html = content_tag(:span, state_title) html = content_tag(:span, state_title)
html << " " << content_tag(:span, number_with_delimiter(count), class: 'badge') html << " " << content_tag(:span, number_with_delimiter(count), class: 'badge')
...@@ -237,6 +233,18 @@ module IssuablesHelper ...@@ -237,6 +233,18 @@ module IssuablesHelper
} }
end end
def issuables_count_for_state(issuable_type, state, finder: nil)
finder ||= public_send("#{issuable_type}_finder")
cache_key = finder.state_counter_cache_key(state)
@counts ||= {}
@counts[cache_key] ||= Rails.cache.fetch(cache_key, expires_in: 2.minutes) do
finder.count_by_state
end
@counts[cache_key][state]
end
private private
def sidebar_gutter_collapsed? def sidebar_gutter_collapsed?
...@@ -255,24 +263,6 @@ module IssuablesHelper ...@@ -255,24 +263,6 @@ module IssuablesHelper
end end
end end
def issuables_count_for_state(issuable_type, state)
@counts ||= {}
@counts[issuable_type] ||= public_send("#{issuable_type}_finder").count_by_state
@counts[issuable_type][state]
end
IRRELEVANT_PARAMS_FOR_CACHE_KEY = %i[utf8 sort page].freeze
private_constant :IRRELEVANT_PARAMS_FOR_CACHE_KEY
def issuables_state_counter_cache_key(issuable_type, state)
opts = params.with_indifferent_access
opts[:state] = state
opts.except!(*IRRELEVANT_PARAMS_FOR_CACHE_KEY)
opts.delete_if { |_, value| value.blank? }
hexdigest(['issuables_count', issuable_type, opts.sort].flatten.join('-'))
end
def issuable_templates(issuable) def issuable_templates(issuable)
@issuable_templates ||= @issuable_templates ||=
case issuable case issuable
......
...@@ -28,7 +28,7 @@ ...@@ -28,7 +28,7 @@
%span %span
Issues Issues
- if @project.default_issues_tracker? - if @project.default_issues_tracker?
%span.badge.count.issue_counter= number_with_delimiter(IssuesFinder.new(current_user, project_id: @project.id).execute.opened.count) %span.badge.count.issue_counter= number_with_delimiter(issuables_count_for_state(:issues, :opened, finder: IssuesFinder.new(current_user, project_id: @project.id)))
- if project_nav_tab? :merge_requests - if project_nav_tab? :merge_requests
- controllers = [:merge_requests, 'projects/merge_requests/conflicts'] - controllers = [:merge_requests, 'projects/merge_requests/conflicts']
...@@ -37,7 +37,7 @@ ...@@ -37,7 +37,7 @@
= link_to namespace_project_merge_requests_path(@project.namespace, @project), title: 'Merge Requests', class: 'shortcuts-merge_requests' do = link_to namespace_project_merge_requests_path(@project.namespace, @project), title: 'Merge Requests', class: 'shortcuts-merge_requests' do
%span %span
Merge Requests Merge Requests
%span.badge.count.merge_counter.js-merge-counter= number_with_delimiter(MergeRequestsFinder.new(current_user, project_id: @project.id).execute.opened.count) %span.badge.count.merge_counter.js-merge-counter= number_with_delimiter(issuables_count_for_state(:merge_requests, :opened, finder: MergeRequestsFinder.new(current_user, project_id: @project.id)))
- if project_nav_tab? :pipelines - if project_nav_tab? :pipelines
= nav_link(controller: [:pipelines, :builds, :environments, :artifacts]) do = nav_link(controller: [:pipelines, :builds, :environments, :artifacts]) do
......
---
title: Cache open issue and merge request counts for project tabs to speed up project
pages
merge_request: 12457
author:
...@@ -295,22 +295,121 @@ describe IssuesFinder do ...@@ -295,22 +295,121 @@ describe IssuesFinder do
end end
end end
describe '.not_restricted_by_confidentiality' do describe '#with_confidentiality_access_check' do
let(:authorized_user) { create(:user) } let(:guest) { create(:user) }
let(:project) { create(:empty_project, namespace: authorized_user.namespace) } set(:authorized_user) { create(:user) }
let!(:public_issue) { create(:issue, project: project) } set(:project) { create(:empty_project, namespace: authorized_user.namespace) }
let!(:confidential_issue) { create(:issue, project: project, confidential: true) } set(:public_issue) { create(:issue, project: project) }
set(:confidential_issue) { create(:issue, project: project, confidential: true) }
context 'when no project filter is given' do
let(:params) { {} }
context 'for an anonymous user' do
subject { described_class.new(nil, params).with_confidentiality_access_check }
it 'returns only public issues' do
expect(subject).to include(public_issue)
expect(subject).not_to include(confidential_issue)
end
end
context 'for a user without project membership' do
subject { described_class.new(user, params).with_confidentiality_access_check }
it 'returns only public issues' do
expect(subject).to include(public_issue)
expect(subject).not_to include(confidential_issue)
end
end
context 'for a guest user' do
subject { described_class.new(guest, params).with_confidentiality_access_check }
before do
project.add_guest(guest)
end
it 'returns only public issues' do
expect(subject).to include(public_issue)
expect(subject).not_to include(confidential_issue)
end
end
context 'for a project member with access to view confidential issues' do
subject { described_class.new(authorized_user, params).with_confidentiality_access_check }
it 'returns all issues' do
expect(subject).to include(public_issue, confidential_issue)
end
end
end
context 'when searching within a specific project' do
let(:params) { { project_id: project.id } }
context 'for an anonymous user' do
subject { described_class.new(nil, params).with_confidentiality_access_check }
it 'returns only public issues' do
expect(subject).to include(public_issue)
expect(subject).not_to include(confidential_issue)
end
it 'does not filter by confidentiality' do
expect(Issue).not_to receive(:where).with(a_string_matching('confidential'), anything)
it 'returns non confidential issues for nil user' do subject
expect(described_class.send(:not_restricted_by_confidentiality, nil)).to include(public_issue)
end end
end
context 'for a user without project membership' do
subject { described_class.new(user, params).with_confidentiality_access_check }
it 'returns non confidential issues for user not authorized for the issues projects' do it 'returns only public issues' do
expect(described_class.send(:not_restricted_by_confidentiality, user)).to include(public_issue) expect(subject).to include(public_issue)
expect(subject).not_to include(confidential_issue)
end end
it 'returns all issues for user authorized for the issues projects' do it 'filters by confidentiality' do
expect(described_class.send(:not_restricted_by_confidentiality, authorized_user)).to include(public_issue, confidential_issue) expect(Issue).to receive(:where).with(a_string_matching('confidential'), anything)
subject
end
end
context 'for a guest user' do
subject { described_class.new(guest, params).with_confidentiality_access_check }
before do
project.add_guest(guest)
end
it 'returns only public issues' do
expect(subject).to include(public_issue)
expect(subject).not_to include(confidential_issue)
end
it 'filters by confidentiality' do
expect(Issue).to receive(:where).with(a_string_matching('confidential'), anything)
subject
end
end
context 'for a project member with access to view confidential issues' do
subject { described_class.new(authorized_user, params).with_confidentiality_access_check }
it 'returns all issues' do
expect(subject).to include(public_issue, confidential_issue)
end
it 'does not filter by confidentiality' do
expect(Issue).not_to receive(:where).with(a_string_matching('confidential'), anything)
subject
end
end
end end
end end
end end
...@@ -77,54 +77,89 @@ describe IssuablesHelper do ...@@ -77,54 +77,89 @@ describe IssuablesHelper do
}.with_indifferent_access }.with_indifferent_access
end end
let(:issues_finder) { IssuesFinder.new(nil, params) }
let(:merge_requests_finder) { MergeRequestsFinder.new(nil, params) }
before do
allow(helper).to receive(:issues_finder).and_return(issues_finder)
allow(helper).to receive(:merge_requests_finder).and_return(merge_requests_finder)
end
it 'returns the cached value when called for the same issuable type & with the same params' do it 'returns the cached value when called for the same issuable type & with the same params' do
expect(helper).to receive(:params).twice.and_return(params) expect(issues_finder).to receive(:count_by_state).and_return(opened: 42)
expect(helper).to receive(:issuables_count_for_state).with(:issues, :opened).and_return(42)
expect(helper.issuables_state_counter_text(:issues, :opened)) expect(helper.issuables_state_counter_text(:issues, :opened))
.to eq('<span>Open</span> <span class="badge">42</span>') .to eq('<span>Open</span> <span class="badge">42</span>')
expect(helper).not_to receive(:issuables_count_for_state) expect(issues_finder).not_to receive(:count_by_state)
expect(helper.issuables_state_counter_text(:issues, :opened)) expect(helper.issuables_state_counter_text(:issues, :opened))
.to eq('<span>Open</span> <span class="badge">42</span>') .to eq('<span>Open</span> <span class="badge">42</span>')
end end
it 'takes confidential status into account when searching for issues' do
expect(issues_finder).to receive(:count_by_state).and_return(opened: 42)
expect(helper.issuables_state_counter_text(:issues, :opened))
.to include('42')
expect(issues_finder).to receive(:user_cannot_see_confidential_issues?).twice.and_return(false)
expect(issues_finder).to receive(:count_by_state).and_return(opened: 40)
expect(helper.issuables_state_counter_text(:issues, :opened))
.to include('40')
expect(issues_finder).to receive(:user_can_see_all_confidential_issues?).and_return(true)
expect(issues_finder).to receive(:count_by_state).and_return(opened: 45)
expect(helper.issuables_state_counter_text(:issues, :opened))
.to include('45')
end
it 'does not take confidential status into account when searching for merge requests' do
expect(merge_requests_finder).to receive(:count_by_state).and_return(opened: 42)
expect(merge_requests_finder).not_to receive(:user_cannot_see_confidential_issues?)
expect(merge_requests_finder).not_to receive(:user_can_see_all_confidential_issues?)
expect(helper.issuables_state_counter_text(:merge_requests, :opened))
.to include('42')
end
it 'does not take some keys into account in the cache key' do it 'does not take some keys into account in the cache key' do
expect(helper).to receive(:params).and_return({ expect(issues_finder).to receive(:count_by_state).and_return(opened: 42)
expect(issues_finder).to receive(:params).and_return({
author_id: '11', author_id: '11',
state: 'foo', state: 'foo',
sort: 'foo', sort: 'foo',
utf8: 'foo', utf8: 'foo',
page: 'foo' page: 'foo'
}.with_indifferent_access) }.with_indifferent_access)
expect(helper).to receive(:issuables_count_for_state).with(:issues, :opened).and_return(42)
expect(helper.issuables_state_counter_text(:issues, :opened)) expect(helper.issuables_state_counter_text(:issues, :opened))
.to eq('<span>Open</span> <span class="badge">42</span>') .to eq('<span>Open</span> <span class="badge">42</span>')
expect(helper).to receive(:params).and_return({ expect(issues_finder).not_to receive(:count_by_state)
expect(issues_finder).to receive(:params).and_return({
author_id: '11', author_id: '11',
state: 'bar', state: 'bar',
sort: 'bar', sort: 'bar',
utf8: 'bar', utf8: 'bar',
page: 'bar' page: 'bar'
}.with_indifferent_access) }.with_indifferent_access)
expect(helper).not_to receive(:issuables_count_for_state)
expect(helper.issuables_state_counter_text(:issues, :opened)) expect(helper.issuables_state_counter_text(:issues, :opened))
.to eq('<span>Open</span> <span class="badge">42</span>') .to eq('<span>Open</span> <span class="badge">42</span>')
end end
it 'does not take params order into account in the cache key' do it 'does not take params order into account in the cache key' do
expect(helper).to receive(:params).and_return('author_id' => '11', 'state' => 'opened') expect(issues_finder).to receive(:params).and_return('author_id' => '11', 'state' => 'opened')
expect(helper).to receive(:issuables_count_for_state).with(:issues, :opened).and_return(42) expect(issues_finder).to receive(:count_by_state).and_return(opened: 42)
expect(helper.issuables_state_counter_text(:issues, :opened)) expect(helper.issuables_state_counter_text(:issues, :opened))
.to eq('<span>Open</span> <span class="badge">42</span>') .to eq('<span>Open</span> <span class="badge">42</span>')
expect(helper).to receive(:params).and_return('state' => 'opened', 'author_id' => '11') expect(issues_finder).to receive(:params).and_return('state' => 'opened', 'author_id' => '11')
expect(helper).not_to receive(:issuables_count_for_state) expect(issues_finder).not_to receive(:count_by_state)
expect(helper.issuables_state_counter_text(:issues, :opened)) expect(helper.issuables_state_counter_text(:issues, :opened))
.to eq('<span>Open</span> <span class="badge">42</span>') .to eq('<span>Open</span> <span class="badge">42</span>')
......
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