Commit e7817fc1 authored by Sean McGivern's avatar Sean McGivern

Remove issuable finder count caching

We're going to cache the total open count separately, and then just perform
these counts on the list. We already do that to get the pagination information,
through Kaminari, and a future change will make Kaminari reuse the query results
from earlier in the request.
parent f35d7d7f
...@@ -24,7 +24,6 @@ class IssuableFinder ...@@ -24,7 +24,6 @@ class IssuableFinder
include CreatedAtFilter include CreatedAtFilter
NONE = '0'.freeze NONE = '0'.freeze
IRRELEVANT_PARAMS_FOR_CACHE_KEY = %i[utf8 sort page state].freeze
attr_accessor :current_user, :params attr_accessor :current_user, :params
...@@ -68,7 +67,7 @@ class IssuableFinder ...@@ -68,7 +67,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, for_counting: true) count_params = params.merge(state: nil, sort: nil)
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)
...@@ -91,16 +90,6 @@ class IssuableFinder ...@@ -91,16 +90,6 @@ class IssuableFinder
execute.find_by!(*params) execute.find_by!(*params)
end end
def state_counter_cache_key
cache_key(state_counter_cache_key_components)
end
def clear_caches!
state_counter_cache_key_components_permutations.each do |components|
Rails.cache.delete(cache_key(components))
end
end
def group def group
return @group if defined?(@group) return @group if defined?(@group)
...@@ -432,20 +421,4 @@ class IssuableFinder ...@@ -432,20 +421,4 @@ 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
opts = params.with_indifferent_access
opts.except!(*IRRELEVANT_PARAMS_FOR_CACHE_KEY)
opts.delete_if { |_, value| value.blank? }
['issuables_count', klass.to_ability_name, opts.sort]
end
def state_counter_cache_key_components_permutations
[state_counter_cache_key_components]
end
def cache_key(components)
Digest::SHA1.hexdigest(components.flatten.join('-'))
end
end end
...@@ -54,44 +54,10 @@ class IssuesFinder < IssuableFinder ...@@ -54,44 +54,10 @@ 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
# Anonymous users can't see any confidential issues. def user_cannot_see_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? return false if user_can_see_all_confidential_issues?
current_user.blank? || for_counting || params[:for_counting] current_user.blank?
end
def state_counter_cache_key_components
extra_components = [
user_can_see_all_confidential_issues?,
user_cannot_see_confidential_issues?(for_counting: true)
]
super + extra_components
end
def state_counter_cache_key_components_permutations
# Ignore the last two, as we'll provide both options for them.
components = super.first[0..-3]
[
components + [false, true],
components + [true, false]
]
end end
def by_assignee(items) def by_assignee(items)
......
...@@ -240,18 +240,11 @@ module IssuablesHelper ...@@ -240,18 +240,11 @@ module IssuablesHelper
} }
end end
def issuables_count_for_state(issuable_type, state, finder: nil) def issuables_count_for_state(issuable_type, state)
finder ||= public_send("#{issuable_type}_finder") # rubocop:disable GitlabSecurity/PublicSend finder = public_send("#{issuable_type}_finder") # rubocop:disable GitlabSecurity/PublicSend
cache_key = finder.state_counter_cache_key
@counts ||= {}
@counts[cache_key] ||= Rails.cache.fetch(cache_key, expires_in: 2.minutes) do
finder.count_by_state finder.count_by_state
end end
@counts[cache_key][state]
end
def close_issuable_url(issuable) def close_issuable_url(issuable)
issuable_url(issuable, close_reopen_params(issuable, :close)) issuable_url(issuable, close_reopen_params(issuable, :close))
end end
......
...@@ -244,9 +244,7 @@ class IssuableBaseService < BaseService ...@@ -244,9 +244,7 @@ class IssuableBaseService < BaseService
new_assignees = issuable.assignees.to_a new_assignees = issuable.assignees.to_a
affected_assignees = (old_assignees + new_assignees) - (old_assignees & new_assignees) affected_assignees = (old_assignees + new_assignees) - (old_assignees & new_assignees)
# Don't clear the project cache, because it will be handled by the invalidate_cache_counts(issuable, users: affected_assignees.compact)
# appropriate service (close / reopen / merge / etc.).
invalidate_cache_counts(issuable, users: affected_assignees.compact, skip_project_cache: true)
after_update(issuable) after_update(issuable)
issuable.create_new_cross_references!(current_user) issuable.create_new_cross_references!(current_user)
execute_hooks(issuable, 'update') execute_hooks(issuable, 'update')
...@@ -340,18 +338,9 @@ class IssuableBaseService < BaseService ...@@ -340,18 +338,9 @@ class IssuableBaseService < BaseService
create_labels_note(issuable, old_labels) if issuable.labels != old_labels create_labels_note(issuable, old_labels) if issuable.labels != old_labels
end end
def invalidate_cache_counts(issuable, users: [], skip_project_cache: false) def invalidate_cache_counts(issuable, users: [])
users.each do |user| users.each do |user|
user.public_send("invalidate_#{issuable.model_name.singular}_cache_counts") # rubocop:disable GitlabSecurity/PublicSend user.public_send("invalidate_#{issuable.model_name.singular}_cache_counts") # rubocop:disable GitlabSecurity/PublicSend
end end
unless skip_project_cache
case issuable
when Issue
IssuesFinder.new(nil, project_id: issuable.project_id).clear_caches!
when MergeRequest
MergeRequestsFinder.new(nil, project_id: issuable.target_project_id).clear_caches!
end
end
end end
end end
require 'spec_helper'
describe 'Issuable counts caching', :use_clean_rails_memory_store_caching do
let!(:member) { create(:user) }
let!(:member_2) { create(:user) }
let!(:non_member) { create(:user) }
let!(:project) { create(:project, :public) }
let!(:open_issue) { create(:issue, project: project) }
let!(:confidential_issue) { create(:issue, :confidential, project: project, author: non_member) }
let!(:closed_issue) { create(:issue, :closed, project: project) }
before do
project.add_developer(member)
project.add_developer(member_2)
end
it 'caches issuable counts correctly for non-members' do
# We can't use expect_any_instance_of because that uses a single instance.
counts = 0
allow_any_instance_of(IssuesFinder).to receive(:count_by_state).and_wrap_original do |m, *args|
counts += 1
m.call(*args)
end
aggregate_failures 'only counts once on first load with no params, and caches for later loads' do
expect { visit project_issues_path(project) }
.to change { counts }.by(1)
expect { visit project_issues_path(project) }
.not_to change { counts }
end
aggregate_failures 'uses counts from cache on load from non-member' do
sign_in(non_member)
expect { visit project_issues_path(project) }
.not_to change { counts }
sign_out(non_member)
end
aggregate_failures 'does not use the same cache for a member' do
sign_in(member)
expect { visit project_issues_path(project) }
.to change { counts }.by(1)
sign_out(member)
end
aggregate_failures 'uses the same cache for all members' do
sign_in(member_2)
expect { visit project_issues_path(project) }
.not_to change { counts }
sign_out(member_2)
end
aggregate_failures 'shares caches when params are passed' do
expect { visit project_issues_path(project, author_username: non_member.username) }
.to change { counts }.by(1)
sign_in(member)
expect { visit project_issues_path(project, author_username: non_member.username) }
.to change { counts }.by(1)
sign_in(non_member)
expect { visit project_issues_path(project, author_username: non_member.username) }
.not_to change { counts }
sign_in(member_2)
expect { visit project_issues_path(project, author_username: non_member.username) }
.not_to change { counts }
sign_out(member_2)
end
aggregate_failures 'resets caches on issue close' do
Issues::CloseService.new(project, member).execute(open_issue)
expect { visit project_issues_path(project) }
.to change { counts }.by(1)
sign_in(member)
expect { visit project_issues_path(project) }
.to change { counts }.by(1)
sign_in(non_member)
expect { visit project_issues_path(project) }
.not_to change { counts }
sign_in(member_2)
expect { visit project_issues_path(project) }
.not_to change { counts }
sign_out(member_2)
end
aggregate_failures 'does not reset caches on issue update' do
Issues::UpdateService.new(project, member, title: 'new title').execute(open_issue)
expect { visit project_issues_path(project) }
.not_to change { counts }
sign_in(member)
expect { visit project_issues_path(project) }
.not_to change { counts }
sign_in(non_member)
expect { visit project_issues_path(project) }
.not_to change { counts }
sign_in(member_2)
expect { visit project_issues_path(project) }
.not_to change { counts }
sign_out(member_2)
end
end
end
...@@ -59,112 +59,6 @@ describe IssuablesHelper do ...@@ -59,112 +59,6 @@ describe IssuablesHelper do
.to eq('<span>All</span> <span class="badge">42</span>') .to eq('<span>All</span> <span class="badge">42</span>')
end end
end end
describe 'counter caching based on issuable type and params', :use_clean_rails_memory_store_caching do
let(:params) do
{
scope: 'created-by-me',
state: 'opened',
utf8: '✓',
author_id: '11',
assignee_id: '18',
label_name: %w(bug discussion documentation),
milestone_title: 'v4.0',
sort: 'due_date_asc',
namespace_id: 'gitlab-org',
project_id: 'gitlab-ce',
page: 2
}.with_indifferent_access
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
expect(issues_finder).to receive(:count_by_state).and_return(opened: 42)
expect(helper.issuables_state_counter_text(:issues, :opened))
.to eq('<span>Open</span> <span class="badge">42</span>')
expect(issues_finder).not_to receive(:count_by_state)
expect(helper.issuables_state_counter_text(:issues, :opened))
.to eq('<span>Open</span> <span class="badge">42</span>')
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
expect(issues_finder).to receive(:count_by_state).and_return(opened: 42)
expect(issues_finder).to receive(:params).and_return({
author_id: '11',
state: 'foo',
sort: 'foo',
utf8: 'foo',
page: 'foo'
}.with_indifferent_access)
expect(helper.issuables_state_counter_text(:issues, :opened))
.to eq('<span>Open</span> <span class="badge">42</span>')
expect(issues_finder).not_to receive(:count_by_state)
expect(issues_finder).to receive(:params).and_return({
author_id: '11',
state: 'bar',
sort: 'bar',
utf8: 'bar',
page: 'bar'
}.with_indifferent_access)
expect(helper.issuables_state_counter_text(:issues, :opened))
.to eq('<span>Open</span> <span class="badge">42</span>')
end
it 'does not take params order into account in the cache key' do
expect(issues_finder).to receive(:params).and_return('author_id' => '11', 'state' => 'opened')
expect(issues_finder).to receive(:count_by_state).and_return(opened: 42)
expect(helper.issuables_state_counter_text(:issues, :opened))
.to eq('<span>Open</span> <span class="badge">42</span>')
expect(issues_finder).to receive(:params).and_return('state' => 'opened', 'author_id' => '11')
expect(issues_finder).not_to receive(:count_by_state)
expect(helper.issuables_state_counter_text(:issues, :opened))
.to eq('<span>Open</span> <span class="badge">42</span>')
end
end
end end
describe '#issuable_reference' do describe '#issuable_reference' do
......
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