Merge branch 'remove-finder-caching' into 'master'

Remove issuable finder count caching

See merge request !13959

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

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)

app/helpers/issuables_helper.rb

@@ -240,18 +240,11 @@ 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
+  def issuables_count_for_state(issuable_type, state)
+    finder = public_send("#{issuable_type}_finder") # rubocop:disable GitlabSecurity/PublicSend
     finder.count_by_state
   end
 
-    @counts[cache_key][state]
-  end
-
   def close_issuable_url(issuable)
     issuable_url(issuable, close_reopen_params(issuable, :close))
   end

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

spec/features/projects/issuable_counts_caching_spec.rb

-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

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