Make finders responsible for counter cache keys

5 files changed, 63 insertions, 59 deletions

diff --git a/app/finders/issuable_finder.rb b/app/finders/issuable_finder.rb
index e8605f3d5b3..7bc2117f61e 100644
--- a/app/finders/issuable_finder.rb
+++ b/app/finders/issuable_finder.rb
@@ -20,6 +20,7 @@
 #
 class IssuableFinder
   NONE = '0'.freeze
+  IRRELEVANT_PARAMS_FOR_CACHE_KEY = %i[utf8 sort page].freeze
 
   attr_accessor :current_user, :params
 
@@ -86,6 +87,10 @@ class IssuableFinder
     execute.find_by!(*params)
   end
 
+  def state_counter_cache_key(state)
+    Digest::SHA1.hexdigest(state_counter_cache_key_components(state).flatten.join('-'))
+  end
+
   def group
     return @group if defined?(@group)
 
@@ -418,4 +423,13 @@ 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(state)
+    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
 end
diff --git a/app/finders/issues_finder.rb b/app/finders/issues_finder.rb
index b213a7aebfd..d20f4475a03 100644
--- a/app/finders/issues_finder.rb
+++ b/app/finders/issues_finder.rb
@@ -22,7 +22,7 @@ class IssuesFinder < IssuableFinder
     Issue
   end
 
-  def not_restricted_by_confidentiality
+  def with_confidentiality_access_check
     return Issue.all if user_can_see_all_confidential_issues?
     return Issue.where('issues.confidential IS NOT TRUE') if user_cannot_see_confidential_issues?
 
@@ -36,7 +36,15 @@ class IssuesFinder < IssuableFinder
       project_ids: current_user.authorized_projects(CONFIDENTIAL_ACCESS_LEVEL).select(:id))
   end
 
+  private
+
+  def init_collection
+    with_confidentiality_access_check
+  end
+
   def user_can_see_all_confidential_issues?
+    return @user_can_see_all_confidential_issues if defined?(@user_can_see_all_confidential_issues)
+
     return @user_can_see_all_confidential_issues = false if current_user.blank?
     return @user_can_see_all_confidential_issues = true if current_user.full_private_access?
 
@@ -46,16 +54,19 @@ class IssuesFinder < IssuableFinder
       project.team.max_member_access(current_user.id) >= CONFIDENTIAL_ACCESS_LEVEL
   end
 
-  def user_cannot_see_confidential_issues?
+  def user_cannot_see_confidential_issues?(for_counting: false)
     return false if user_can_see_all_confidential_issues?
 
-    current_user.blank? || params[:for_counting]
+    current_user.blank? || for_counting || params[:for_counting]
   end
 
-  private
+  def state_counter_cache_key_components(state)
+    extra_components = [
+      user_can_see_all_confidential_issues?,
+      user_cannot_see_confidential_issues?(for_counting: true)
+    ]
 
-  def init_collection
-    not_restricted_by_confidentiality
+    super + extra_components
   end
 
   def by_assignee(items)
diff --git a/app/helpers/issuables_helper.rb b/app/helpers/issuables_helper.rb
index d99a9bab12f..5385ada6dc4 100644
--- a/app/helpers/issuables_helper.rb
+++ b/app/helpers/issuables_helper.rb
@@ -253,7 +253,7 @@ module IssuablesHelper
 
   def issuables_count_for_state(issuable_type, state, finder: nil)
     finder ||= public_send("#{issuable_type}_finder")
-    cache_key = issuables_state_counter_cache_key(issuable_type, finder, state)
+    cache_key = finder.state_counter_cache_key(state)
 
     @counts ||= {}
     @counts[cache_key] ||= Rails.cache.fetch(cache_key, expires_in: 2.minutes) do
@@ -263,25 +263,6 @@ module IssuablesHelper
     @counts[cache_key][state]
   end
 
-  IRRELEVANT_PARAMS_FOR_CACHE_KEY = %i[utf8 sort page].freeze
-  private_constant :IRRELEVANT_PARAMS_FOR_CACHE_KEY
-
-  def issuables_state_counter_cache_key(issuable_type, finder, state)
-    opts = params.with_indifferent_access
-    opts[:state] = state
-    opts.except!(*IRRELEVANT_PARAMS_FOR_CACHE_KEY)
-    opts.delete_if { |_, value| value.blank? }
-
-    key_components = ['issuables_count', issuable_type, opts.sort]
-
-    if issuable_type == :issues
-      key_components << finder.user_can_see_all_confidential_issues?
-      key_components << finder.user_cannot_see_confidential_issues?
-    end
-
-    hexdigest(key_components.flatten.join('-'))
-  end
-
   def issuable_templates(issuable)
     @issuable_templates ||=
       case issuable
diff --git a/spec/finders/issues_finder_spec.rb b/spec/finders/issues_finder_spec.rb
index dfa15e859a4..4a52f0d5c58 100644
--- a/spec/finders/issues_finder_spec.rb
+++ b/spec/finders/issues_finder_spec.rb
@@ -295,7 +295,7 @@ describe IssuesFinder do
     end
   end
 
-  describe '#not_restricted_by_confidentiality' do
+  describe '#with_confidentiality_access_check' do
     let(:guest) { create(:user) }
     set(:authorized_user) { create(:user) }
     set(:project) { create(:empty_project, namespace: authorized_user.namespace) }
@@ -306,7 +306,7 @@ describe IssuesFinder do
       let(:params) { {} }
 
       context 'for an anonymous user' do
-        subject { described_class.new(nil, params).not_restricted_by_confidentiality }
+        subject { described_class.new(nil, params).with_confidentiality_access_check }
 
         it 'returns only public issues' do
           expect(subject).to include(public_issue)
@@ -315,7 +315,7 @@ describe IssuesFinder do
       end
 
       context 'for a user without project membership' do
-        subject { described_class.new(user, params).not_restricted_by_confidentiality }
+        subject { described_class.new(user, params).with_confidentiality_access_check }
 
         it 'returns only public issues' do
           expect(subject).to include(public_issue)
@@ -324,7 +324,7 @@ describe IssuesFinder do
       end
 
       context 'for a guest user' do
-        subject { described_class.new(guest, params).not_restricted_by_confidentiality }
+        subject { described_class.new(guest, params).with_confidentiality_access_check }
 
         before do
           project.add_guest(guest)
@@ -337,7 +337,7 @@ describe IssuesFinder do
       end
 
       context 'for a project member with access to view confidential issues' do
-        subject { described_class.new(authorized_user, params).not_restricted_by_confidentiality }
+        subject { described_class.new(authorized_user, params).with_confidentiality_access_check }
 
         it 'returns all issues' do
           expect(subject).to include(public_issue, confidential_issue)
@@ -349,7 +349,7 @@ describe IssuesFinder do
       let(:params) { { project_id: project.id } }
 
       context 'for an anonymous user' do
-        subject { described_class.new(nil, params).not_restricted_by_confidentiality }
+        subject { described_class.new(nil, params).with_confidentiality_access_check }
 
         it 'returns only public issues' do
           expect(subject).to include(public_issue)
@@ -364,7 +364,7 @@ describe IssuesFinder do
       end
 
       context 'for a user without project membership' do
-        subject { described_class.new(user, params).not_restricted_by_confidentiality }
+        subject { described_class.new(user, params).with_confidentiality_access_check }
 
         it 'returns only public issues' do
           expect(subject).to include(public_issue)
@@ -379,7 +379,7 @@ describe IssuesFinder do
       end
 
       context 'for a guest user' do
-        subject { described_class.new(guest, params).not_restricted_by_confidentiality }
+        subject { described_class.new(guest, params).with_confidentiality_access_check }
 
         before do
           project.add_guest(guest)
@@ -398,7 +398,7 @@ describe IssuesFinder do
       end
 
       context 'for a project member with access to view confidential issues' do
-        subject { described_class.new(authorized_user, params).not_restricted_by_confidentiality }
+        subject { described_class.new(authorized_user, params).with_confidentiality_access_check }
 
         it 'returns all issues' do
           expect(subject).to include(public_issue, confidential_issue)
diff --git a/spec/helpers/issuables_helper_spec.rb b/spec/helpers/issuables_helper_spec.rb
index 7dfda388de4..d2e918ef014 100644
--- a/spec/helpers/issuables_helper_spec.rb
+++ b/spec/helpers/issuables_helper_spec.rb
@@ -77,59 +77,57 @@ describe IssuablesHelper do
         }.with_indifferent_access
       end
 
-      let(:finder) { double(:finder, user_cannot_see_confidential_issues?: true, user_can_see_all_confidential_issues?: false) }
+      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(finder)
-        allow(helper).to receive(:merge_requests_finder).and_return(finder)
+        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(helper).to receive(:params).twice.and_return(params)
-        expect(finder).to receive(:count_by_state).and_return(opened: 42)
+        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(finder).not_to receive(:count_by_state)
+        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
-        allow(helper).to receive(:params).and_return(params)
-        expect(finder).to receive(:count_by_state).and_return(opened: 42)
+        expect(issues_finder).to receive(:count_by_state).and_return(opened: 42)
 
         expect(helper.issuables_state_counter_text(:issues, :opened))
           .to include('42')
 
-        expect(finder).to receive(:user_cannot_see_confidential_issues?).and_return(false)
-        expect(finder).to receive(:count_by_state).and_return(opened: 40)
+        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(finder).to receive(:user_can_see_all_confidential_issues?).and_return(true)
-        expect(finder).to receive(:count_by_state).and_return(opened: 45)
+        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
-        allow(helper).to receive(:params).and_return(params)
-        expect(finder).to receive(:count_by_state).and_return(opened: 42)
-        expect(finder).not_to receive(:user_cannot_see_confidential_issues?)
-        expect(finder).not_to receive(:user_can_see_all_confidential_issues?)
+        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(finder).to receive(:count_by_state).and_return(opened: 42)
-        expect(helper).to receive(:params).and_return({
+        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',
@@ -140,8 +138,8 @@ describe IssuablesHelper do
         expect(helper.issuables_state_counter_text(:issues, :opened))
           .to eq('<span>Open</span> <span class="badge">42</span>')
 
-        expect(finder).not_to receive(:count_by_state)
-        expect(helper).to receive(:params).and_return({
+        expect(issues_finder).not_to receive(:count_by_state)
+        expect(issues_finder).to receive(:params).and_return({
           author_id: '11',
           state: 'bar',
           sort: 'bar',
@@ -154,14 +152,14 @@ describe IssuablesHelper do
       end
 
       it 'does not take params order into account in the cache key' do
-        expect(helper).to receive(:params).and_return('author_id' => '11', 'state' => 'opened')
-        expect(finder).to receive(:count_by_state).and_return(opened: 42)
+        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(helper).to receive(:params).and_return('state' => 'opened', 'author_id' => '11')
-        expect(finder).not_to receive(:count_by_state)
+        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>')