diff options
| -rw-r--r-- | app/finders/issuable_finder.rb | 29 | ||||
| -rw-r--r-- | app/finders/issues_finder.rb | 38 | ||||
| -rw-r--r-- | app/helpers/issuables_helper.rb | 13 | ||||
| -rw-r--r-- | app/services/issuable_base_service.rb | 15 | ||||
| -rw-r--r-- | spec/features/projects/issuable_counts_caching_spec.rb | 132 | ||||
| -rw-r--r-- | spec/helpers/issuables_helper_spec.rb | 106 |
6 files changed, 8 insertions, 325 deletions
diff --git a/app/finders/issuable_finder.rb b/app/finders/issuable_finder.rb index 7e0d3b5c979..c8dd2275730 100644 --- a/app/finders/issuable_finder.rb +++ b/app/finders/issuable_finder.rb @@ -24,7 +24,6 @@ class IssuableFinder include CreatedAtFilter NONE = '0'.freeze - IRRELEVANT_PARAMS_FOR_CACHE_KEY = %i[utf8 sort page state].freeze attr_accessor :current_user, :params @@ -68,7 +67,7 @@ class IssuableFinder # grouping and counting within that query. # 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 finder = self.class.new(current_user, count_params) counts = Hash.new(0) @@ -91,16 +90,6 @@ class IssuableFinder execute.find_by!(*params) 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 return @group if defined?(@group) @@ -432,20 +421,4 @@ class IssuableFinder def current_user_related? params[:scope] == 'created-by-me' || params[:scope] == 'authored' || params[:scope] == 'assigned-to-me' 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 diff --git a/app/finders/issues_finder.rb b/app/finders/issues_finder.rb index 0ec42a4e6eb..aa9cef6b08c 100644 --- a/app/finders/issues_finder.rb +++ b/app/finders/issues_finder.rb @@ -54,44 +54,10 @@ class IssuesFinder < IssuableFinder project.team.max_member_access(current_user.id) >= CONFIDENTIAL_ACCESS_LEVEL end - # Anonymous users can't see any 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) + def user_cannot_see_confidential_issues? return false if user_can_see_all_confidential_issues? - current_user.blank? || for_counting || params[:for_counting] - 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] - ] + current_user.blank? end def by_assignee(items) diff --git a/app/helpers/issuables_helper.rb b/app/helpers/issuables_helper.rb index 2a748ce0a75..5d7d33da30a 100644 --- a/app/helpers/issuables_helper.rb +++ b/app/helpers/issuables_helper.rb @@ -240,16 +240,9 @@ module IssuablesHelper } end - def issuables_count_for_state(issuable_type, state, finder: nil) - 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 - end - - @counts[cache_key][state] + def issuables_count_for_state(issuable_type, state) + finder = public_send("#{issuable_type}_finder") # rubocop:disable GitlabSecurity/PublicSend + finder.count_by_state end def close_issuable_url(issuable) diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb index 7679389faf6..8b967b78052 100644 --- a/app/services/issuable_base_service.rb +++ b/app/services/issuable_base_service.rb @@ -245,9 +245,7 @@ class IssuableBaseService < BaseService 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) + invalidate_cache_counts(issuable, users: affected_assignees.compact) after_update(issuable) issuable.create_new_cross_references!(current_user) execute_hooks(issuable, 'update') @@ -341,18 +339,9 @@ class IssuableBaseService < BaseService create_labels_note(issuable, old_labels) if issuable.labels != old_labels end - def invalidate_cache_counts(issuable, users: [], skip_project_cache: false) + def invalidate_cache_counts(issuable, users: []) users.each do |user| user.public_send("invalidate_#{issuable.model_name.singular}_cache_counts") # rubocop:disable GitlabSecurity/PublicSend 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/spec/features/projects/issuable_counts_caching_spec.rb b/spec/features/projects/issuable_counts_caching_spec.rb deleted file mode 100644 index 1804d9dc244..00000000000 --- a/spec/features/projects/issuable_counts_caching_spec.rb +++ /dev/null @@ -1,132 +0,0 @@ -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 diff --git a/spec/helpers/issuables_helper_spec.rb b/spec/helpers/issuables_helper_spec.rb index 7789cfa3554..ead3e28438e 100644 --- a/spec/helpers/issuables_helper_spec.rb +++ b/spec/helpers/issuables_helper_spec.rb @@ -59,112 +59,6 @@ describe IssuablesHelper do .to eq('<span>All</span> <span class="badge">42</span>') 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 describe '#issuable_reference' do |