Commit 723c6b04 authored by Sean McGivern's avatar Sean McGivern

Make finders responsible for counter cache keys

parent b060775f
...@@ -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
SCALAR_PARAMS = %i(scope state group_id project_id milestone_title assignee_id search label_name sort assignee_username author_id author_username authorized_only due_date iids non_archived weight).freeze SCALAR_PARAMS = %i(scope state group_id project_id milestone_title assignee_id search label_name sort assignee_username author_id author_username authorized_only due_date iids non_archived weight).freeze
ARRAY_PARAMS = { label_name: [], iids: [] }.freeze ARRAY_PARAMS = { label_name: [], iids: [] }.freeze
...@@ -91,6 +92,10 @@ class IssuableFinder ...@@ -91,6 +92,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)
...@@ -448,4 +453,13 @@ class IssuableFinder ...@@ -448,4 +453,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
...@@ -22,7 +22,7 @@ class IssuesFinder < IssuableFinder ...@@ -22,7 +22,7 @@ class IssuesFinder < IssuableFinder
Issue Issue
end end
def not_restricted_by_confidentiality def with_confidentiality_access_check
return Issue.all if user_can_see_all_confidential_issues? return Issue.all if user_can_see_all_confidential_issues?
return Issue.where('issues.confidential IS NOT TRUE') if user_cannot_see_confidential_issues? return Issue.where('issues.confidential IS NOT TRUE') if user_cannot_see_confidential_issues?
...@@ -36,7 +36,15 @@ class IssuesFinder < IssuableFinder ...@@ -36,7 +36,15 @@ class IssuesFinder < IssuableFinder
project_ids: current_user.authorized_projects(CONFIDENTIAL_ACCESS_LEVEL).select(:id)) project_ids: current_user.authorized_projects(CONFIDENTIAL_ACCESS_LEVEL).select(:id))
end end
private
def init_collection
with_confidentiality_access_check
end
def user_can_see_all_confidential_issues? 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 = false if current_user.blank?
return @user_can_see_all_confidential_issues = true if current_user.full_private_access? return @user_can_see_all_confidential_issues = true if current_user.full_private_access?
...@@ -46,16 +54,19 @@ class IssuesFinder < IssuableFinder ...@@ -46,16 +54,19 @@ class IssuesFinder < IssuableFinder
project.team.max_member_access(current_user.id) >= CONFIDENTIAL_ACCESS_LEVEL project.team.max_member_access(current_user.id) >= CONFIDENTIAL_ACCESS_LEVEL
end end
def user_cannot_see_confidential_issues? def user_cannot_see_confidential_issues?(for_counting: false)
return false if user_can_see_all_confidential_issues? return false if user_can_see_all_confidential_issues?
current_user.blank? || params[:for_counting] current_user.blank? || for_counting || params[:for_counting]
end end
private def state_counter_cache_key_components(state)
extra_components = [
user_can_see_all_confidential_issues?,
user_cannot_see_confidential_issues?(for_counting: true)
]
def init_collection super + extra_components
not_restricted_by_confidentiality
end end
def by_assignee(items) def by_assignee(items)
......
...@@ -261,7 +261,7 @@ module IssuablesHelper ...@@ -261,7 +261,7 @@ module IssuablesHelper
def issuables_count_for_state(issuable_type, state, finder: nil) def issuables_count_for_state(issuable_type, state, finder: nil)
finder ||= public_send("#{issuable_type}_finder") finder ||= public_send("#{issuable_type}_finder")
cache_key = issuables_state_counter_cache_key(issuable_type, finder, state) cache_key = finder.state_counter_cache_key(state)
@counts ||= {} @counts ||= {}
@counts[cache_key] ||= Rails.cache.fetch(cache_key, expires_in: 2.minutes) do @counts[cache_key] ||= Rails.cache.fetch(cache_key, expires_in: 2.minutes) do
...@@ -271,25 +271,6 @@ module IssuablesHelper ...@@ -271,25 +271,6 @@ module IssuablesHelper
@counts[cache_key][state] @counts[cache_key][state]
end 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, finder, state)
opts = params.with_indifferent_access
opts[:state] = state
opts.except!(*IRRELEVANT_PARAMS_FOR_CACHE_KEY)
opts.delete_if { |_, value| value.blank? }
key_components = ['issuables_count', issuable_type, opts.sort]
if issuable_type == :issues
key_components << finder.user_can_see_all_confidential_issues?
key_components << finder.user_cannot_see_confidential_issues?
end
hexdigest(key_components.flatten.join('-'))
end
def issuable_templates(issuable) def issuable_templates(issuable)
@issuable_templates ||= @issuable_templates ||=
case issuable case issuable
......
...@@ -311,7 +311,7 @@ describe IssuesFinder do ...@@ -311,7 +311,7 @@ describe IssuesFinder do
end end
end end
describe '#not_restricted_by_confidentiality' do describe '#with_confidentiality_access_check' do
let(:guest) { create(:user) } let(:guest) { create(:user) }
set(:authorized_user) { create(:user) } set(:authorized_user) { create(:user) }
let(:admin_user) { create(:user, :admin) } let(:admin_user) { create(:user, :admin) }
...@@ -324,7 +324,7 @@ describe IssuesFinder do ...@@ -324,7 +324,7 @@ describe IssuesFinder do
let(:params) { {} } let(:params) { {} }
context 'for an anonymous user' do context 'for an anonymous user' do
subject { described_class.new(nil, params).not_restricted_by_confidentiality } subject { described_class.new(nil, params).with_confidentiality_access_check }
it 'returns only public issues' do it 'returns only public issues' do
expect(subject).to include(public_issue) expect(subject).to include(public_issue)
...@@ -333,7 +333,7 @@ describe IssuesFinder do ...@@ -333,7 +333,7 @@ describe IssuesFinder do
end end
context 'for a user without project membership' do context 'for a user without project membership' do
subject { described_class.new(user, params).not_restricted_by_confidentiality } subject { described_class.new(user, params).with_confidentiality_access_check }
it 'returns only public issues' do it 'returns only public issues' do
expect(subject).to include(public_issue) expect(subject).to include(public_issue)
...@@ -342,7 +342,7 @@ describe IssuesFinder do ...@@ -342,7 +342,7 @@ describe IssuesFinder do
end end
context 'for a guest user' do context 'for a guest user' do
subject { described_class.new(guest, params).not_restricted_by_confidentiality } subject { described_class.new(guest, params).with_confidentiality_access_check }
before do before do
project.add_guest(guest) project.add_guest(guest)
...@@ -355,7 +355,7 @@ describe IssuesFinder do ...@@ -355,7 +355,7 @@ describe IssuesFinder do
end end
context 'for a project member with access to view confidential issues' do context 'for a project member with access to view confidential issues' do
subject { described_class.new(authorized_user, params).not_restricted_by_confidentiality } subject { described_class.new(authorized_user, params).with_confidentiality_access_check }
it 'returns all issues' do it 'returns all issues' do
expect(subject).to include(public_issue, confidential_issue) expect(subject).to include(public_issue, confidential_issue)
...@@ -363,7 +363,7 @@ describe IssuesFinder do ...@@ -363,7 +363,7 @@ describe IssuesFinder do
end end
context 'for an auditor' do context 'for an auditor' do
subject { described_class.new(auditor_user, params).not_restricted_by_confidentiality } subject { described_class.new(auditor_user, params).with_confidentiality_access_check }
it 'returns all issues' do it 'returns all issues' do
expect(subject).to include(public_issue, confidential_issue) expect(subject).to include(public_issue, confidential_issue)
...@@ -371,7 +371,7 @@ describe IssuesFinder do ...@@ -371,7 +371,7 @@ describe IssuesFinder do
end end
context 'for an admin' do context 'for an admin' do
subject { described_class.new(admin_user, params).not_restricted_by_confidentiality } subject { described_class.new(admin_user, params).with_confidentiality_access_check }
it 'returns all issues' do it 'returns all issues' do
expect(subject).to include(public_issue, confidential_issue) expect(subject).to include(public_issue, confidential_issue)
...@@ -383,7 +383,7 @@ describe IssuesFinder do ...@@ -383,7 +383,7 @@ describe IssuesFinder do
let(:params) { { project_id: project.id } } let(:params) { { project_id: project.id } }
context 'for an anonymous user' do context 'for an anonymous user' do
subject { described_class.new(nil, params).not_restricted_by_confidentiality } subject { described_class.new(nil, params).with_confidentiality_access_check }
it 'returns only public issues' do it 'returns only public issues' do
expect(subject).to include(public_issue) expect(subject).to include(public_issue)
...@@ -398,7 +398,7 @@ describe IssuesFinder do ...@@ -398,7 +398,7 @@ describe IssuesFinder do
end end
context 'for a user without project membership' do context 'for a user without project membership' do
subject { described_class.new(user, params).not_restricted_by_confidentiality } subject { described_class.new(user, params).with_confidentiality_access_check }
it 'returns only public issues' do it 'returns only public issues' do
expect(subject).to include(public_issue) expect(subject).to include(public_issue)
...@@ -413,7 +413,7 @@ describe IssuesFinder do ...@@ -413,7 +413,7 @@ describe IssuesFinder do
end end
context 'for a guest user' do context 'for a guest user' do
subject { described_class.new(guest, params).not_restricted_by_confidentiality } subject { described_class.new(guest, params).with_confidentiality_access_check }
before do before do
project.add_guest(guest) project.add_guest(guest)
...@@ -432,7 +432,7 @@ describe IssuesFinder do ...@@ -432,7 +432,7 @@ describe IssuesFinder do
end end
context 'for a project member with access to view confidential issues' do context 'for a project member with access to view confidential issues' do
subject { described_class.new(authorized_user, params).not_restricted_by_confidentiality } subject { described_class.new(authorized_user, params).with_confidentiality_access_check }
it 'returns all issues' do it 'returns all issues' do
expect(subject).to include(public_issue, confidential_issue) expect(subject).to include(public_issue, confidential_issue)
...@@ -446,7 +446,7 @@ describe IssuesFinder do ...@@ -446,7 +446,7 @@ describe IssuesFinder do
end end
context 'for an auditor' do context 'for an auditor' do
subject { described_class.new(auditor_user, params).not_restricted_by_confidentiality } subject { described_class.new(auditor_user, params).with_confidentiality_access_check }
it 'returns all issues' do it 'returns all issues' do
expect(subject).to include(public_issue, confidential_issue) expect(subject).to include(public_issue, confidential_issue)
...@@ -460,7 +460,7 @@ describe IssuesFinder do ...@@ -460,7 +460,7 @@ describe IssuesFinder do
end end
context 'for an admin' do context 'for an admin' do
subject { described_class.new(auditor_user, params).not_restricted_by_confidentiality } subject { described_class.new(auditor_user, params).with_confidentiality_access_check }
it 'returns all issues' do it 'returns all issues' do
expect(subject).to include(public_issue, confidential_issue) expect(subject).to include(public_issue, confidential_issue)
......
...@@ -77,59 +77,57 @@ describe IssuablesHelper do ...@@ -77,59 +77,57 @@ describe IssuablesHelper do
}.with_indifferent_access }.with_indifferent_access
end end
let(:finder) { double(:finder, user_cannot_see_confidential_issues?: true, user_can_see_all_confidential_issues?: false) } let(:issues_finder) { IssuesFinder.new(nil, params) }
let(:merge_requests_finder) { MergeRequestsFinder.new(nil, params) }
before do before do
allow(helper).to receive(:issues_finder).and_return(finder) allow(helper).to receive(:issues_finder).and_return(issues_finder)
allow(helper).to receive(:merge_requests_finder).and_return(finder) allow(helper).to receive(:merge_requests_finder).and_return(merge_requests_finder)
end 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(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(finder).not_to receive(:count_by_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 it 'takes confidential status into account when searching for issues' do
allow(helper).to receive(:params).and_return(params) expect(issues_finder).to receive(:count_by_state).and_return(opened: 42)
expect(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 include('42') .to include('42')
expect(finder).to receive(:user_cannot_see_confidential_issues?).and_return(false) expect(issues_finder).to receive(:user_cannot_see_confidential_issues?).twice.and_return(false)
expect(finder).to receive(:count_by_state).and_return(opened: 40) expect(issues_finder).to receive(:count_by_state).and_return(opened: 40)
expect(helper.issuables_state_counter_text(:issues, :opened)) expect(helper.issuables_state_counter_text(:issues, :opened))
.to include('40') .to include('40')
expect(finder).to receive(:user_can_see_all_confidential_issues?).and_return(true) expect(issues_finder).to receive(:user_can_see_all_confidential_issues?).and_return(true)
expect(finder).to receive(:count_by_state).and_return(opened: 45) expect(issues_finder).to receive(:count_by_state).and_return(opened: 45)
expect(helper.issuables_state_counter_text(:issues, :opened)) expect(helper.issuables_state_counter_text(:issues, :opened))
.to include('45') .to include('45')
end end
it 'does not take confidential status into account when searching for merge requests' do it 'does not take confidential status into account when searching for merge requests' do
allow(helper).to receive(:params).and_return(params) expect(merge_requests_finder).to receive(:count_by_state).and_return(opened: 42)
expect(finder).to receive(:count_by_state).and_return(opened: 42) expect(merge_requests_finder).not_to receive(:user_cannot_see_confidential_issues?)
expect(finder).not_to receive(:user_cannot_see_confidential_issues?) expect(merge_requests_finder).not_to receive(:user_can_see_all_confidential_issues?)
expect(finder).not_to receive(:user_can_see_all_confidential_issues?)
expect(helper.issuables_state_counter_text(:merge_requests, :opened)) expect(helper.issuables_state_counter_text(:merge_requests, :opened))
.to include('42') .to include('42')
end 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(finder).to receive(:count_by_state).and_return(opened: 42) expect(issues_finder).to receive(:count_by_state).and_return(opened: 42)
expect(helper).to receive(:params).and_return({ expect(issues_finder).to receive(:params).and_return({
author_id: '11', author_id: '11',
state: 'foo', state: 'foo',
sort: 'foo', sort: 'foo',
...@@ -140,8 +138,8 @@ describe IssuablesHelper do ...@@ -140,8 +138,8 @@ describe IssuablesHelper do
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(finder).not_to receive(:count_by_state) expect(issues_finder).not_to receive(:count_by_state)
expect(helper).to receive(:params).and_return({ expect(issues_finder).to receive(:params).and_return({
author_id: '11', author_id: '11',
state: 'bar', state: 'bar',
sort: 'bar', sort: 'bar',
...@@ -154,14 +152,14 @@ describe IssuablesHelper do ...@@ -154,14 +152,14 @@ describe IssuablesHelper do
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(finder).to receive(:count_by_state).and_return(opened: 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(finder).not_to receive(:count_by_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