Commit 0e488ef7 authored by Sean McGivern's avatar Sean McGivern

Clear issuable counter caches on update

When an issuable's state changes, or one is created, we should clear the cache
counts for a user's assigned issuables, and also the project-wide caches for
this user type.
parent b3a588bc
...@@ -90,7 +90,13 @@ class IssuableFinder ...@@ -90,7 +90,13 @@ class IssuableFinder
end end
def state_counter_cache_key def state_counter_cache_key
Digest::SHA1.hexdigest(state_counter_cache_key_components.flatten.join('-')) 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 end
def group def group
...@@ -424,4 +430,12 @@ class IssuableFinder ...@@ -424,4 +430,12 @@ class IssuableFinder
['issuables_count', klass.to_ability_name, opts.sort] ['issuables_count', klass.to_ability_name, opts.sort]
end 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
...@@ -84,6 +84,16 @@ class IssuesFinder < IssuableFinder ...@@ -84,6 +84,16 @@ class IssuesFinder < IssuableFinder
super + extra_components super + extra_components
end 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
def by_assignee(items) def by_assignee(items)
if assignee if assignee
items.assigned_to(assignee) items.assigned_to(assignee)
......
...@@ -33,17 +33,12 @@ module Boards ...@@ -33,17 +33,12 @@ module Boards
end end
def filter_params def filter_params
set_default_scope
set_project set_project
set_state set_state
params params
end end
def set_default_scope
params[:scope] = 'all'
end
def set_project def set_project
params[:project_id] = project.id params[:project_id] = project.id
end end
......
...@@ -183,7 +183,7 @@ class IssuableBaseService < BaseService ...@@ -183,7 +183,7 @@ class IssuableBaseService < BaseService
after_create(issuable) after_create(issuable)
issuable.create_cross_references!(current_user) issuable.create_cross_references!(current_user)
execute_hooks(issuable) execute_hooks(issuable)
invalidate_cache_counts(issuable.assignees, issuable) invalidate_cache_counts(issuable, users: issuable.assignees)
end end
issuable issuable
...@@ -240,12 +240,12 @@ class IssuableBaseService < BaseService ...@@ -240,12 +240,12 @@ class IssuableBaseService < BaseService
old_assignees: old_assignees old_assignees: old_assignees
) )
if old_assignees != issuable.assignees
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)
invalidate_cache_counts(affected_assignees.compact, issuable)
end
# Don't clear the project cache, because it will be handled by the
# 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')
...@@ -339,9 +339,18 @@ class IssuableBaseService < BaseService ...@@ -339,9 +339,18 @@ 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(users, issuable) def invalidate_cache_counts(issuable, users: [], skip_project_cache: false)
users.each do |user| users.each do |user|
user.public_send("invalidate_#{issuable.model_name.singular}_cache_counts") user.public_send("invalidate_#{issuable.model_name.singular}_cache_counts")
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
...@@ -28,7 +28,7 @@ module Issues ...@@ -28,7 +28,7 @@ module Issues
notification_service.close_issue(issue, current_user) if notifications notification_service.close_issue(issue, current_user) if notifications
todo_service.close_issue(issue, current_user) todo_service.close_issue(issue, current_user)
execute_hooks(issue, 'close') execute_hooks(issue, 'close')
invalidate_cache_counts(issue.assignees, issue) invalidate_cache_counts(issue, users: issue.assignees)
end end
issue issue
......
...@@ -8,7 +8,7 @@ module Issues ...@@ -8,7 +8,7 @@ module Issues
create_note(issue) create_note(issue)
notification_service.reopen_issue(issue, current_user) notification_service.reopen_issue(issue, current_user)
execute_hooks(issue, 'reopen') execute_hooks(issue, 'reopen')
invalidate_cache_counts(issue.assignees, issue) invalidate_cache_counts(issue, users: issue.assignees)
end end
issue issue
......
...@@ -13,7 +13,7 @@ module MergeRequests ...@@ -13,7 +13,7 @@ module MergeRequests
notification_service.close_mr(merge_request, current_user) notification_service.close_mr(merge_request, current_user)
todo_service.close_merge_request(merge_request, current_user) todo_service.close_merge_request(merge_request, current_user)
execute_hooks(merge_request, 'close') execute_hooks(merge_request, 'close')
invalidate_cache_counts(merge_request.assignees, merge_request) invalidate_cache_counts(merge_request, users: merge_request.assignees)
end end
merge_request merge_request
......
...@@ -13,7 +13,7 @@ module MergeRequests ...@@ -13,7 +13,7 @@ module MergeRequests
create_note(merge_request) create_note(merge_request)
notification_service.merge_mr(merge_request, current_user) notification_service.merge_mr(merge_request, current_user)
execute_hooks(merge_request, 'merge') execute_hooks(merge_request, 'merge')
invalidate_cache_counts(merge_request.assignees, merge_request) invalidate_cache_counts(merge_request, users: merge_request.assignees)
end end
private private
......
...@@ -10,7 +10,7 @@ module MergeRequests ...@@ -10,7 +10,7 @@ module MergeRequests
execute_hooks(merge_request, 'reopen') execute_hooks(merge_request, 'reopen')
merge_request.reload_diff(current_user) merge_request.reload_diff(current_user)
merge_request.mark_as_unchecked merge_request.mark_as_unchecked
invalidate_cache_counts(merge_request.assignees, merge_request) invalidate_cache_counts(merge_request, users: merge_request.assignees)
end end
merge_request merge_request
......
...@@ -62,7 +62,7 @@ RSpec.describe 'Dashboard Issues', feature: true do ...@@ -62,7 +62,7 @@ RSpec.describe 'Dashboard Issues', feature: true do
it 'state filter tabs work' do it 'state filter tabs work' do
find('#state-closed').click find('#state-closed').click
expect(page).to have_current_path(issues_dashboard_url(assignee_id: current_user.id, scope: 'all', state: 'closed'), url: true) expect(page).to have_current_path(issues_dashboard_url(assignee_id: current_user.id, state: 'closed'), url: true)
end end
it_behaves_like "it has an RSS button with current_user's RSS token" it_behaves_like "it has an RSS button with current_user's RSS token"
......
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(:empty_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
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