Remove issuable finder count cachingremove-finder-caching

We're going to cache the total open count separately, and then just perform
these counts on the list. We already do that to get the pagination information,
through Kaminari, and a future change will make Kaminari reuse the query results
from earlier in the request.

6 files changed, 9 insertions, 323 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..0cd27dcd42b 100644
--- a/app/helpers/issuables_helper.rb
+++ b/app/helpers/issuables_helper.rb
@@ -240,16 +240,12 @@ 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
+  def issuables_count_for_state(issuable_type, state)
+    finder = public_send("#{issuable_type}_finder") # rubocop:disable GitlabSecurity/PublicSend
 
     @counts ||= {}
-    @counts[cache_key] ||= Rails.cache.fetch(cache_key, expires_in: 2.minutes) do
-      finder.count_by_state
-    end
-
-    @counts[cache_key][state]
+    @counts[issuable_type] ||= finder.count_by_state
+    @counts[issuable_type][state]
   end
 
   def close_issuable_url(issuable)
diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb
index 1486db046b5..063f7bc28d0 100644
--- a/app/services/issuable_base_service.rb
+++ b/app/services/issuable_base_service.rb
@@ -244,9 +244,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')
@@ -340,18 +338,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