summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorSean McGivern <sean@gitlab.com>2017-07-11 17:12:33 +0100
committerSean McGivern <sean@gitlab.com>2017-07-19 10:21:20 +0100
commit0e488ef70ab2608847423201e957693745f1e3b1 (patch)
tree1b7a329505c1a3026a777258040a3f5be9870561
parentb3a588bccaaa078a81e8ce322d75ee167f642e13 (diff)
downloadgitlab-ce-0e488ef70ab2608847423201e957693745f1e3b1.tar.gz
Clear issuable counter caches on updateclear-issuable-count-cache-for-states
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.
-rw-r--r--app/finders/issuable_finder.rb16
-rw-r--r--app/finders/issues_finder.rb10
-rw-r--r--app/services/boards/issues/list_service.rb5
-rw-r--r--app/services/issuable_base_service.rb23
-rw-r--r--app/services/issues/close_service.rb2
-rw-r--r--app/services/issues/reopen_service.rb2
-rw-r--r--app/services/merge_requests/close_service.rb2
-rw-r--r--app/services/merge_requests/post_merge_service.rb2
-rw-r--r--app/services/merge_requests/reopen_service.rb2
-rw-r--r--spec/features/dashboard/issues_spec.rb2
-rw-r--r--spec/features/projects/issuable_counts_caching_spec.rb132
11 files changed, 179 insertions, 19 deletions
diff --git a/app/finders/issuable_finder.rb b/app/finders/issuable_finder.rb
index d3010eff262..fc63e30c8fb 100644
--- a/app/finders/issuable_finder.rb
+++ b/app/finders/issuable_finder.rb
@@ -90,7 +90,13 @@ class IssuableFinder
end
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
def group
@@ -424,4 +430,12 @@ class IssuableFinder
['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
diff --git a/app/finders/issues_finder.rb b/app/finders/issues_finder.rb
index 295a64ef5b8..0ec42a4e6eb 100644
--- a/app/finders/issues_finder.rb
+++ b/app/finders/issues_finder.rb
@@ -84,6 +84,16 @@ class IssuesFinder < IssuableFinder
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
+
def by_assignee(items)
if assignee
items.assigned_to(assignee)
diff --git a/app/services/boards/issues/list_service.rb b/app/services/boards/issues/list_service.rb
index a1d67cbc244..eb345fead2d 100644
--- a/app/services/boards/issues/list_service.rb
+++ b/app/services/boards/issues/list_service.rb
@@ -33,17 +33,12 @@ module Boards
end
def filter_params
- set_default_scope
set_project
set_state
params
end
- def set_default_scope
- params[:scope] = 'all'
- end
-
def set_project
params[:project_id] = project.id
end
diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb
index a03a7abfeb1..9078b1f0983 100644
--- a/app/services/issuable_base_service.rb
+++ b/app/services/issuable_base_service.rb
@@ -183,7 +183,7 @@ class IssuableBaseService < BaseService
after_create(issuable)
issuable.create_cross_references!(current_user)
execute_hooks(issuable)
- invalidate_cache_counts(issuable.assignees, issuable)
+ invalidate_cache_counts(issuable, users: issuable.assignees)
end
issuable
@@ -240,12 +240,12 @@ class IssuableBaseService < BaseService
old_assignees: old_assignees
)
- if old_assignees != issuable.assignees
- new_assignees = issuable.assignees.to_a
- affected_assignees = (old_assignees + new_assignees) - (old_assignees & new_assignees)
- invalidate_cache_counts(affected_assignees.compact, issuable)
- end
+ new_assignees = issuable.assignees.to_a
+ affected_assignees = (old_assignees + new_assignees) - (old_assignees & new_assignees)
+ # 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)
issuable.create_new_cross_references!(current_user)
execute_hooks(issuable, 'update')
@@ -339,9 +339,18 @@ class IssuableBaseService < BaseService
create_labels_note(issuable, old_labels) if issuable.labels != old_labels
end
- def invalidate_cache_counts(users, issuable)
+ def invalidate_cache_counts(issuable, users: [], skip_project_cache: false)
users.each do |user|
user.public_send("invalidate_#{issuable.model_name.singular}_cache_counts")
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
diff --git a/app/services/issues/close_service.rb b/app/services/issues/close_service.rb
index 85c616ca576..ddef5281498 100644
--- a/app/services/issues/close_service.rb
+++ b/app/services/issues/close_service.rb
@@ -28,7 +28,7 @@ module Issues
notification_service.close_issue(issue, current_user) if notifications
todo_service.close_issue(issue, current_user)
execute_hooks(issue, 'close')
- invalidate_cache_counts(issue.assignees, issue)
+ invalidate_cache_counts(issue, users: issue.assignees)
end
issue
diff --git a/app/services/issues/reopen_service.rb b/app/services/issues/reopen_service.rb
index 80ea6312768..73b2e85cba3 100644
--- a/app/services/issues/reopen_service.rb
+++ b/app/services/issues/reopen_service.rb
@@ -8,7 +8,7 @@ module Issues
create_note(issue)
notification_service.reopen_issue(issue, current_user)
execute_hooks(issue, 'reopen')
- invalidate_cache_counts(issue.assignees, issue)
+ invalidate_cache_counts(issue, users: issue.assignees)
end
issue
diff --git a/app/services/merge_requests/close_service.rb b/app/services/merge_requests/close_service.rb
index 2ffc989ed71..c0ce01f7523 100644
--- a/app/services/merge_requests/close_service.rb
+++ b/app/services/merge_requests/close_service.rb
@@ -13,7 +13,7 @@ module MergeRequests
notification_service.close_mr(merge_request, current_user)
todo_service.close_merge_request(merge_request, current_user)
execute_hooks(merge_request, 'close')
- invalidate_cache_counts(merge_request.assignees, merge_request)
+ invalidate_cache_counts(merge_request, users: merge_request.assignees)
end
merge_request
diff --git a/app/services/merge_requests/post_merge_service.rb b/app/services/merge_requests/post_merge_service.rb
index f0d998731d7..261a8bfa200 100644
--- a/app/services/merge_requests/post_merge_service.rb
+++ b/app/services/merge_requests/post_merge_service.rb
@@ -13,7 +13,7 @@ module MergeRequests
create_note(merge_request)
notification_service.merge_mr(merge_request, current_user)
execute_hooks(merge_request, 'merge')
- invalidate_cache_counts(merge_request.assignees, merge_request)
+ invalidate_cache_counts(merge_request, users: merge_request.assignees)
end
private
diff --git a/app/services/merge_requests/reopen_service.rb b/app/services/merge_requests/reopen_service.rb
index f2fddf7f345..52f6d511f98 100644
--- a/app/services/merge_requests/reopen_service.rb
+++ b/app/services/merge_requests/reopen_service.rb
@@ -10,7 +10,7 @@ module MergeRequests
execute_hooks(merge_request, 'reopen')
merge_request.reload_diff(current_user)
merge_request.mark_as_unchecked
- invalidate_cache_counts(merge_request.assignees, merge_request)
+ invalidate_cache_counts(merge_request, users: merge_request.assignees)
end
merge_request
diff --git a/spec/features/dashboard/issues_spec.rb b/spec/features/dashboard/issues_spec.rb
index 86ac24ea06e..69c1a2ed89a 100644
--- a/spec/features/dashboard/issues_spec.rb
+++ b/spec/features/dashboard/issues_spec.rb
@@ -62,7 +62,7 @@ RSpec.describe 'Dashboard Issues', feature: true do
it 'state filter tabs work' do
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
it_behaves_like "it has an RSS button with current_user's RSS token"
diff --git a/spec/features/projects/issuable_counts_caching_spec.rb b/spec/features/projects/issuable_counts_caching_spec.rb
new file mode 100644
index 00000000000..703d1cbd327
--- /dev/null
+++ b/spec/features/projects/issuable_counts_caching_spec.rb
@@ -0,0 +1,132 @@
+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