From 778a16be3cf772975343072346874bb12428f1ac Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Wed, 19 Jul 2017 15:54:09 +0000 Subject: Merge branch 'clear-issuable-count-cache-for-states' into 'master' Clear issuable count cache on update Closes #34772 See merge request !12795 --- app/controllers/concerns/issuable_collections.rb | 8 +- app/finders/issuable_finder.rb | 25 +++- app/finders/issues_finder.rb | 12 +- app/helpers/issuables_helper.rb | 2 +- app/services/boards/issues/list_service.rb | 5 - app/services/issuable_base_service.rb | 23 ++-- app/services/issues/close_service.rb | 2 +- app/services/issues/reopen_service.rb | 2 +- app/services/merge_requests/close_service.rb | 2 +- app/services/merge_requests/post_merge_service.rb | 2 +- app/services/merge_requests/reopen_service.rb | 2 +- spec/features/dashboard/issues_spec.rb | 2 +- .../projects/issuable_counts_caching_spec.rb | 132 +++++++++++++++++++++ 13 files changed, 187 insertions(+), 32 deletions(-) create mode 100644 spec/features/projects/issuable_counts_caching_spec.rb diff --git a/app/controllers/concerns/issuable_collections.rb b/app/controllers/concerns/issuable_collections.rb index e18778cdf80..b43b2c5621f 100644 --- a/app/controllers/concerns/issuable_collections.rb +++ b/app/controllers/concerns/issuable_collections.rb @@ -32,10 +32,10 @@ module IssuableCollections def filter_params set_sort_order_from_cookie - set_default_scope set_default_state - @filter_params = params.dup + # Skip irrelevant Rails routing params + @filter_params = params.dup.except(:controller, :action, :namespace_id) @filter_params[:sort] ||= default_sort_order @sort = @filter_params[:sort] @@ -55,10 +55,6 @@ module IssuableCollections @filter_params end - def set_default_scope - params[:scope] = 'all' if params[:scope].blank? - end - def set_default_state params[:state] = 'opened' if params[:state].blank? end diff --git a/app/finders/issuable_finder.rb b/app/finders/issuable_finder.rb index 2e5a6493134..fc63e30c8fb 100644 --- a/app/finders/issuable_finder.rb +++ b/app/finders/issuable_finder.rb @@ -20,9 +20,9 @@ # class IssuableFinder include CreatedAtFilter - + NONE = '0'.freeze - IRRELEVANT_PARAMS_FOR_CACHE_KEY = %i[utf8 sort page].freeze + IRRELEVANT_PARAMS_FOR_CACHE_KEY = %i[utf8 sort page state].freeze attr_accessor :current_user, :params @@ -89,8 +89,14 @@ class IssuableFinder execute.find_by!(*params) end - def state_counter_cache_key(state) - Digest::SHA1.hexdigest(state_counter_cache_key_components(state).flatten.join('-')) + 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 @@ -417,12 +423,19 @@ class IssuableFinder params[:scope] == 'created-by-me' || params[:scope] == 'authored' || params[:scope] == 'assigned-to-me' end - def state_counter_cache_key_components(state) + def state_counter_cache_key_components 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 + + 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 85230ff1293..0ec42a4e6eb 100644 --- a/app/finders/issues_finder.rb +++ b/app/finders/issues_finder.rb @@ -75,7 +75,7 @@ class IssuesFinder < IssuableFinder current_user.blank? || for_counting || params[:for_counting] end - def state_counter_cache_key_components(state) + def state_counter_cache_key_components extra_components = [ user_can_see_all_confidential_issues?, user_cannot_see_confidential_issues?(for_counting: true) @@ -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/helpers/issuables_helper.rb b/app/helpers/issuables_helper.rb index d0c518f81f7..425af547330 100644 --- a/app/helpers/issuables_helper.rb +++ b/app/helpers/issuables_helper.rb @@ -235,7 +235,7 @@ module IssuablesHelper def issuables_count_for_state(issuable_type, state, finder: nil) finder ||= public_send("#{issuable_type}_finder") - cache_key = finder.state_counter_cache_key(state) + cache_key = finder.state_counter_cache_key @counts ||= {} @counts[cache_key] ||= Rails.cache.fetch(cache_key, expires_in: 2.minutes) do 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 -- cgit v1.2.1