From 42ccb5981a8425216f9d69372754c52510f0cade Mon Sep 17 00:00:00 2001
From: Sean McGivern <sean@gitlab.com>
Date: Tue, 20 Jun 2017 16:38:14 +0100
Subject: Only do complicated confidentiality checks when necessary

When we are filtering by a single project, and the current user has access to
see confidential issues on that project, we don't need to filter by
confidentiality at all - just as if the user were an admin.

The filter by confidentiality often picks a non-optimal query plan: for
instance, AND-ing the results of all issues in the project (a relatively small
set), and all issues in the states requested (a huge set), rather than just
starting small and winnowing further.
---
 app/finders/issues_finder.rb             |  42 +++++++----
 app/views/layouts/nav/_project.html.haml |   4 +-
 spec/finders/issues_finder_spec.rb       | 125 +++++++++++++++++++++++++++----
 3 files changed, 142 insertions(+), 29 deletions(-)

diff --git a/app/finders/issues_finder.rb b/app/finders/issues_finder.rb
index 3da5508aefd..3455a75b8bc 100644
--- a/app/finders/issues_finder.rb
+++ b/app/finders/issues_finder.rb
@@ -16,14 +16,30 @@
 #     sort: string
 #
 class IssuesFinder < IssuableFinder
+  CONFIDENTIAL_ACCESS_LEVEL = Gitlab::Access::REPORTER
+
   def klass
     Issue
   end
 
+  def not_restricted_by_confidentiality
+    return Issue.where('issues.confidential IS NOT TRUE') if user_cannot_see_confidential_issues?
+    return Issue.all if user_can_see_all_confidential_issues?
+
+    Issue.where('
+      issues.confidential IS NOT TRUE
+      OR (issues.confidential = TRUE
+        AND (issues.author_id = :user_id
+          OR EXISTS (SELECT TRUE FROM issue_assignees WHERE user_id = :user_id AND issue_id = issues.id)
+          OR issues.project_id IN(:project_ids)))',
+      user_id: current_user.id,
+      project_ids: current_user.authorized_projects(CONFIDENTIAL_ACCESS_LEVEL).select(:id))
+  end
+
   private
 
   def init_collection
-    IssuesFinder.not_restricted_by_confidentiality(current_user)
+    not_restricted_by_confidentiality
   end
 
   def by_assignee(items)
@@ -38,22 +54,20 @@ class IssuesFinder < IssuableFinder
     end
   end
 
-  def self.not_restricted_by_confidentiality(user)
-    return Issue.where('issues.confidential IS NOT TRUE') if user.blank?
+  def item_project_ids(items)
+    items&.reorder(nil)&.select(:project_id)
+  end
 
-    return Issue.all if user.full_private_access?
+  def user_can_see_all_confidential_issues?
+    return false unless current_user
+    return true if current_user.full_private_access?
 
-    Issue.where('
-      issues.confidential IS NOT TRUE
-      OR (issues.confidential = TRUE
-        AND (issues.author_id = :user_id
-          OR EXISTS (SELECT TRUE FROM issue_assignees WHERE user_id = :user_id AND issue_id = issues.id)
-          OR issues.project_id IN(:project_ids)))',
-      user_id: user.id,
-      project_ids: user.authorized_projects(Gitlab::Access::REPORTER).select(:id))
+    project? &&
+      project &&
+      project.team.max_member_access(current_user.id) >= CONFIDENTIAL_ACCESS_LEVEL
   end
 
-  def item_project_ids(items)
-    items&.reorder(nil)&.select(:project_id)
+  def user_cannot_see_confidential_issues?
+    current_user.blank?
   end
 end
diff --git a/app/views/layouts/nav/_project.html.haml b/app/views/layouts/nav/_project.html.haml
index 68024d782a6..a2c6e44425a 100644
--- a/app/views/layouts/nav/_project.html.haml
+++ b/app/views/layouts/nav/_project.html.haml
@@ -28,7 +28,7 @@
           %span
             Issues
             - if @project.default_issues_tracker?
-              %span.badge.count.issue_counter= number_with_delimiter(IssuesFinder.new(current_user, project_id: @project.id).execute.opened.count)
+              %span.badge.count.issue_counter= number_with_delimiter(IssuesFinder.new(current_user, project_id: @project.id, state: :opened).execute.count)
 
     - if project_nav_tab? :merge_requests
       - controllers = [:merge_requests, 'projects/merge_requests/conflicts']
@@ -37,7 +37,7 @@
         = link_to namespace_project_merge_requests_path(@project.namespace, @project), title: 'Merge Requests', class: 'shortcuts-merge_requests' do
           %span
             Merge Requests
-            %span.badge.count.merge_counter.js-merge-counter= number_with_delimiter(MergeRequestsFinder.new(current_user, project_id: @project.id).execute.opened.count)
+            %span.badge.count.merge_counter.js-merge-counter= number_with_delimiter(MergeRequestsFinder.new(current_user, project_id: @project.id, state: :opened).execute.count)
 
     - if project_nav_tab? :pipelines
       = nav_link(controller: [:pipelines, :builds, :environments, :artifacts]) do
diff --git a/spec/finders/issues_finder_spec.rb b/spec/finders/issues_finder_spec.rb
index 8ace1fb5751..dfa15e859a4 100644
--- a/spec/finders/issues_finder_spec.rb
+++ b/spec/finders/issues_finder_spec.rb
@@ -295,22 +295,121 @@ describe IssuesFinder do
     end
   end
 
-  describe '.not_restricted_by_confidentiality' do
-    let(:authorized_user) { create(:user) }
-    let(:project) { create(:empty_project, namespace: authorized_user.namespace) }
-    let!(:public_issue) { create(:issue, project: project) }
-    let!(:confidential_issue) { create(:issue, project: project, confidential: true) }
-
-    it 'returns non confidential issues for nil user' do
-      expect(described_class.send(:not_restricted_by_confidentiality, nil)).to include(public_issue)
-    end
+  describe '#not_restricted_by_confidentiality' do
+    let(:guest) { create(:user) }
+    set(:authorized_user) { create(:user) }
+    set(:project) { create(:empty_project, namespace: authorized_user.namespace) }
+    set(:public_issue) { create(:issue, project: project) }
+    set(:confidential_issue) { create(:issue, project: project, confidential: true) }
+
+    context 'when no project filter is given' do
+      let(:params) { {} }
+
+      context 'for an anonymous user' do
+        subject { described_class.new(nil, params).not_restricted_by_confidentiality }
+
+        it 'returns only public issues' do
+          expect(subject).to include(public_issue)
+          expect(subject).not_to include(confidential_issue)
+        end
+      end
+
+      context 'for a user without project membership' do
+        subject { described_class.new(user, params).not_restricted_by_confidentiality }
+
+        it 'returns only public issues' do
+          expect(subject).to include(public_issue)
+          expect(subject).not_to include(confidential_issue)
+        end
+      end
+
+      context 'for a guest user' do
+        subject { described_class.new(guest, params).not_restricted_by_confidentiality }
+
+        before do
+          project.add_guest(guest)
+        end
+
+        it 'returns only public issues' do
+          expect(subject).to include(public_issue)
+          expect(subject).not_to include(confidential_issue)
+        end
+      end
+
+      context 'for a project member with access to view confidential issues' do
+        subject { described_class.new(authorized_user, params).not_restricted_by_confidentiality }
 
-    it 'returns non confidential issues for user not authorized for the issues projects' do
-      expect(described_class.send(:not_restricted_by_confidentiality, user)).to include(public_issue)
+        it 'returns all issues' do
+          expect(subject).to include(public_issue, confidential_issue)
+        end
+      end
     end
 
-    it 'returns all issues for user authorized for the issues projects' do
-      expect(described_class.send(:not_restricted_by_confidentiality, authorized_user)).to include(public_issue, confidential_issue)
+    context 'when searching within a specific project' do
+      let(:params) { { project_id: project.id } }
+
+      context 'for an anonymous user' do
+        subject { described_class.new(nil, params).not_restricted_by_confidentiality }
+
+        it 'returns only public issues' do
+          expect(subject).to include(public_issue)
+          expect(subject).not_to include(confidential_issue)
+        end
+
+        it 'does not filter by confidentiality' do
+          expect(Issue).not_to receive(:where).with(a_string_matching('confidential'), anything)
+
+          subject
+        end
+      end
+
+      context 'for a user without project membership' do
+        subject { described_class.new(user, params).not_restricted_by_confidentiality }
+
+        it 'returns only public issues' do
+          expect(subject).to include(public_issue)
+          expect(subject).not_to include(confidential_issue)
+        end
+
+        it 'filters by confidentiality' do
+          expect(Issue).to receive(:where).with(a_string_matching('confidential'), anything)
+
+          subject
+        end
+      end
+
+      context 'for a guest user' do
+        subject { described_class.new(guest, params).not_restricted_by_confidentiality }
+
+        before do
+          project.add_guest(guest)
+        end
+
+        it 'returns only public issues' do
+          expect(subject).to include(public_issue)
+          expect(subject).not_to include(confidential_issue)
+        end
+
+        it 'filters by confidentiality' do
+          expect(Issue).to receive(:where).with(a_string_matching('confidential'), anything)
+
+          subject
+        end
+      end
+
+      context 'for a project member with access to view confidential issues' do
+        subject { described_class.new(authorized_user, params).not_restricted_by_confidentiality }
+
+        it 'returns all issues' do
+          expect(subject).to include(public_issue, confidential_issue)
+        end
+
+        it 'does not filter by confidentiality' do
+          expect(Issue).not_to receive(:where).with(a_string_matching('confidential'), anything)
+
+          subject
+        end
+      end
     end
   end
 end
-- 
cgit v1.2.1


From 20bb678d91715817f3da04c7a1b73db84295accd Mon Sep 17 00:00:00 2001
From: Sean McGivern <sean@gitlab.com>
Date: Thu, 22 Jun 2017 20:58:20 +0100
Subject: Cache total issue / MR counts for project by user type

This runs a slightly slower query to get the issue and MR counts in the
navigation, but caches by user type (can see all / none confidential issues) for
two minutes.
---
 app/finders/issues_finder.rb             | 26 ++++++++---------
 app/helpers/issuables_helper.rb          | 29 ++++++++++++-------
 app/views/layouts/nav/_project.html.haml |  4 +--
 spec/helpers/issuables_helper_spec.rb    | 49 ++++++++++++++++++++++++++++----
 4 files changed, 77 insertions(+), 31 deletions(-)

diff --git a/app/finders/issues_finder.rb b/app/finders/issues_finder.rb
index 3455a75b8bc..328198c026a 100644
--- a/app/finders/issues_finder.rb
+++ b/app/finders/issues_finder.rb
@@ -36,6 +36,19 @@ class IssuesFinder < IssuableFinder
       project_ids: current_user.authorized_projects(CONFIDENTIAL_ACCESS_LEVEL).select(:id))
   end
 
+  def user_can_see_all_confidential_issues?
+    return false unless current_user
+    return true if current_user.full_private_access?
+
+    project? &&
+      project &&
+      project.team.max_member_access(current_user.id) >= CONFIDENTIAL_ACCESS_LEVEL
+  end
+
+  def user_cannot_see_confidential_issues?
+    current_user.blank?
+  end
+
   private
 
   def init_collection
@@ -57,17 +70,4 @@ class IssuesFinder < IssuableFinder
   def item_project_ids(items)
     items&.reorder(nil)&.select(:project_id)
   end
-
-  def user_can_see_all_confidential_issues?
-    return false unless current_user
-    return true if current_user.full_private_access?
-
-    project? &&
-      project &&
-      project.team.max_member_access(current_user.id) >= CONFIDENTIAL_ACCESS_LEVEL
-  end
-
-  def user_cannot_see_confidential_issues?
-    current_user.blank?
-  end
 end
diff --git a/app/helpers/issuables_helper.rb b/app/helpers/issuables_helper.rb
index 3259a9c1933..d99a9bab12f 100644
--- a/app/helpers/issuables_helper.rb
+++ b/app/helpers/issuables_helper.rb
@@ -165,11 +165,7 @@ module IssuablesHelper
     }
 
     state_title = titles[state] || state.to_s.humanize
-
-    count =
-      Rails.cache.fetch(issuables_state_counter_cache_key(issuable_type, state), expires_in: 2.minutes) do
-        issuables_count_for_state(issuable_type, state)
-      end
+    count = issuables_count_for_state(issuable_type, state)
 
     html = content_tag(:span, state_title)
     html << " " << content_tag(:span, number_with_delimiter(count), class: 'badge')
@@ -255,22 +251,35 @@ module IssuablesHelper
     end
   end
 
-  def issuables_count_for_state(issuable_type, state)
+  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)
+
     @counts ||= {}
-    @counts[issuable_type] ||= public_send("#{issuable_type}_finder").count_by_state
-    @counts[issuable_type][state]
+    @counts[cache_key] ||= Rails.cache.fetch(cache_key, expires_in: 2.minutes) do
+      finder.count_by_state
+    end
+
+    @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, state)
+  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? }
 
-    hexdigest(['issuables_count', issuable_type, opts.sort].flatten.join('-'))
+    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)
diff --git a/app/views/layouts/nav/_project.html.haml b/app/views/layouts/nav/_project.html.haml
index a2c6e44425a..b095adcfe7e 100644
--- a/app/views/layouts/nav/_project.html.haml
+++ b/app/views/layouts/nav/_project.html.haml
@@ -28,7 +28,7 @@
           %span
             Issues
             - if @project.default_issues_tracker?
-              %span.badge.count.issue_counter= number_with_delimiter(IssuesFinder.new(current_user, project_id: @project.id, state: :opened).execute.count)
+              %span.badge.count.issue_counter= number_with_delimiter(issuables_count_for_state(:issues, :opened, finder: IssuesFinder.new(current_user, project_id: @project.id)))
 
     - if project_nav_tab? :merge_requests
       - controllers = [:merge_requests, 'projects/merge_requests/conflicts']
@@ -37,7 +37,7 @@
         = link_to namespace_project_merge_requests_path(@project.namespace, @project), title: 'Merge Requests', class: 'shortcuts-merge_requests' do
           %span
             Merge Requests
-            %span.badge.count.merge_counter.js-merge-counter= number_with_delimiter(MergeRequestsFinder.new(current_user, project_id: @project.id, state: :opened).execute.count)
+            %span.badge.count.merge_counter.js-merge-counter= number_with_delimiter(issuables_count_for_state(:merge_requests, :opened, finder: MergeRequestsFinder.new(current_user, project_id: @project.id)))
 
     - if project_nav_tab? :pipelines
       = nav_link(controller: [:pipelines, :builds, :environments, :artifacts]) do
diff --git a/spec/helpers/issuables_helper_spec.rb b/spec/helpers/issuables_helper_spec.rb
index 15cb620199d..7dfda388de4 100644
--- a/spec/helpers/issuables_helper_spec.rb
+++ b/spec/helpers/issuables_helper_spec.rb
@@ -77,20 +77,58 @@ describe IssuablesHelper do
         }.with_indifferent_access
       end
 
+      let(:finder) { double(:finder, user_cannot_see_confidential_issues?: true, user_can_see_all_confidential_issues?: false) }
+
+      before do
+        allow(helper).to receive(:issues_finder).and_return(finder)
+        allow(helper).to receive(:merge_requests_finder).and_return(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(helper).to receive(:issuables_count_for_state).with(:issues, :opened).and_return(42)
+        expect(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).not_to receive(:issuables_count_for_state)
+        expect(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(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(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(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(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({
           author_id: '11',
           state: 'foo',
@@ -98,11 +136,11 @@ describe IssuablesHelper do
           utf8: 'foo',
           page: 'foo'
         }.with_indifferent_access)
-        expect(helper).to receive(:issuables_count_for_state).with(:issues, :opened).and_return(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(helper).to receive(:params).and_return({
           author_id: '11',
           state: 'bar',
@@ -110,7 +148,6 @@ describe IssuablesHelper do
           utf8: 'bar',
           page: 'bar'
         }.with_indifferent_access)
-        expect(helper).not_to receive(:issuables_count_for_state)
 
         expect(helper.issuables_state_counter_text(:issues, :opened))
           .to eq('<span>Open</span> <span class="badge">42</span>')
@@ -118,13 +155,13 @@ describe IssuablesHelper do
 
       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(helper).to receive(:issuables_count_for_state).with(:issues, :opened).and_return(42)
+        expect(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(helper).not_to receive(:issuables_count_for_state)
+        expect(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>')
-- 
cgit v1.2.1


From c400030d0f51c71f32990ab0ecfebfa245ed663e Mon Sep 17 00:00:00 2001
From: Sean McGivern <sean@gitlab.com>
Date: Fri, 23 Jun 2017 12:50:33 +0100
Subject: Don't count any confidential issues for non-project-members

---
 app/finders/issuable_finder.rb |  2 +-
 app/finders/issues_finder.rb   | 13 ++++++++-----
 2 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/app/finders/issuable_finder.rb b/app/finders/issuable_finder.rb
index 558f8b5e2e5..e8605f3d5b3 100644
--- a/app/finders/issuable_finder.rb
+++ b/app/finders/issuable_finder.rb
@@ -62,7 +62,7 @@ class IssuableFinder
   # grouping and counting within that query.
   #
   def count_by_state
-    count_params = params.merge(state: nil, sort: nil)
+    count_params = params.merge(state: nil, sort: nil, for_counting: true)
     labels_count = label_names.any? ? label_names.count : 1
     finder = self.class.new(current_user, count_params)
     counts = Hash.new(0)
diff --git a/app/finders/issues_finder.rb b/app/finders/issues_finder.rb
index 328198c026a..b213a7aebfd 100644
--- a/app/finders/issues_finder.rb
+++ b/app/finders/issues_finder.rb
@@ -23,8 +23,8 @@ class IssuesFinder < IssuableFinder
   end
 
   def not_restricted_by_confidentiality
-    return Issue.where('issues.confidential IS NOT TRUE') if user_cannot_see_confidential_issues?
     return Issue.all if user_can_see_all_confidential_issues?
+    return Issue.where('issues.confidential IS NOT TRUE') if user_cannot_see_confidential_issues?
 
     Issue.where('
       issues.confidential IS NOT TRUE
@@ -37,16 +37,19 @@ class IssuesFinder < IssuableFinder
   end
 
   def user_can_see_all_confidential_issues?
-    return false unless current_user
-    return true if current_user.full_private_access?
+    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?
 
-    project? &&
+    @user_can_see_all_confidential_issues =
+      project? &&
       project &&
       project.team.max_member_access(current_user.id) >= CONFIDENTIAL_ACCESS_LEVEL
   end
 
   def user_cannot_see_confidential_issues?
-    current_user.blank?
+    return false if user_can_see_all_confidential_issues?
+
+    current_user.blank? || params[:for_counting]
   end
 
   private
-- 
cgit v1.2.1


From 8deece32478aaa83354fcfff7b5d6f3250d55844 Mon Sep 17 00:00:00 2001
From: Sean McGivern <sean@gitlab.com>
Date: Mon, 26 Jun 2017 15:48:27 +0100
Subject: Add changelog entry for issue / MR tab counting optimisations

---
 changelogs/unreleased/speed-up-issue-counting-for-a-project.yml | 5 +++++
 1 file changed, 5 insertions(+)
 create mode 100644 changelogs/unreleased/speed-up-issue-counting-for-a-project.yml

diff --git a/changelogs/unreleased/speed-up-issue-counting-for-a-project.yml b/changelogs/unreleased/speed-up-issue-counting-for-a-project.yml
new file mode 100644
index 00000000000..493ecbcb77a
--- /dev/null
+++ b/changelogs/unreleased/speed-up-issue-counting-for-a-project.yml
@@ -0,0 +1,5 @@
+---
+title: Cache open issue and merge request counts for project tabs to speed up project
+  pages
+merge_request:
+author:
-- 
cgit v1.2.1


From 0c6cdd07829668e04012219eb21cc60db8c1eabc Mon Sep 17 00:00:00 2001
From: Sean McGivern <sean@gitlab.com>
Date: Thu, 29 Jun 2017 12:43:56 +0100
Subject: Make finders responsible for counter cache keys

---
 app/finders/issuable_finder.rb        | 14 +++++++++++
 app/finders/issues_finder.rb          | 23 +++++++++++++-----
 app/helpers/issuables_helper.rb       | 21 +---------------
 spec/finders/issues_finder_spec.rb    | 18 +++++++-------
 spec/helpers/issuables_helper_spec.rb | 46 +++++++++++++++++------------------
 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>')
-- 
cgit v1.2.1


From cb30edfae5c3557686463ca22eca7ef572c3ac33 Mon Sep 17 00:00:00 2001
From: Sean McGivern <sean@gitlab.com>
Date: Thu, 29 Jun 2017 17:15:49 +0100
Subject: Clarify counter caching for users without project access

---
 app/finders/issues_finder.rb | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/app/finders/issues_finder.rb b/app/finders/issues_finder.rb
index d20f4475a03..18f60f9a2b6 100644
--- a/app/finders/issues_finder.rb
+++ b/app/finders/issues_finder.rb
@@ -54,6 +54,21 @@ 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. The 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)
     return false if user_can_see_all_confidential_issues?
 
-- 
cgit v1.2.1


From 4bd17d4f7427b2f5f950e601d1a21042b30f69b1 Mon Sep 17 00:00:00 2001
From: Sean McGivern <sean@gitlab.com>
Date: Fri, 30 Jun 2017 11:55:27 +0100
Subject: Make issuables_count_for_state public

This doesn't need to be public in CE, but EE uses it as such.
---
 app/helpers/issuables_helper.rb | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/app/helpers/issuables_helper.rb b/app/helpers/issuables_helper.rb
index 5385ada6dc4..05177e58c5a 100644
--- a/app/helpers/issuables_helper.rb
+++ b/app/helpers/issuables_helper.rb
@@ -233,6 +233,18 @@ module IssuablesHelper
     }
   end
 
+  def issuables_count_for_state(issuable_type, state, finder: nil)
+    finder ||= public_send("#{issuable_type}_finder")
+    cache_key = finder.state_counter_cache_key(state)
+
+    @counts ||= {}
+    @counts[cache_key] ||= Rails.cache.fetch(cache_key, expires_in: 2.minutes) do
+      finder.count_by_state
+    end
+
+    @counts[cache_key][state]
+  end
+
   private
 
   def sidebar_gutter_collapsed?
@@ -251,18 +263,6 @@ module IssuablesHelper
     end
   end
 
-  def issuables_count_for_state(issuable_type, state, finder: nil)
-    finder ||= public_send("#{issuable_type}_finder")
-    cache_key = finder.state_counter_cache_key(state)
-
-    @counts ||= {}
-    @counts[cache_key] ||= Rails.cache.fetch(cache_key, expires_in: 2.minutes) do
-      finder.count_by_state
-    end
-
-    @counts[cache_key][state]
-  end
-
   def issuable_templates(issuable)
     @issuable_templates ||=
       case issuable
-- 
cgit v1.2.1


From 4e38985b1cc159c4e1582ab34c29d7ec151aef35 Mon Sep 17 00:00:00 2001
From: Sean McGivern <sean@gitlab.com>
Date: Fri, 30 Jun 2017 15:44:21 +0100
Subject: Fix typo in IssuesFinder comment

[ci skip]
---
 app/finders/issues_finder.rb                                    | 2 +-
 changelogs/unreleased/speed-up-issue-counting-for-a-project.yml | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/app/finders/issues_finder.rb b/app/finders/issues_finder.rb
index 18f60f9a2b6..85230ff1293 100644
--- a/app/finders/issues_finder.rb
+++ b/app/finders/issues_finder.rb
@@ -60,7 +60,7 @@ class IssuesFinder < IssuableFinder
   # `user_can_see_all_confidential_issues?`) are more complicated, because they
   # can see confidential issues where:
   # 1. They are an assignee.
-  # 2. The are an author.
+  # 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
diff --git a/changelogs/unreleased/speed-up-issue-counting-for-a-project.yml b/changelogs/unreleased/speed-up-issue-counting-for-a-project.yml
index 493ecbcb77a..6bf03d9a382 100644
--- a/changelogs/unreleased/speed-up-issue-counting-for-a-project.yml
+++ b/changelogs/unreleased/speed-up-issue-counting-for-a-project.yml
@@ -1,5 +1,5 @@
 ---
 title: Cache open issue and merge request counts for project tabs to speed up project
   pages
-merge_request:
+merge_request: 12457
 author:
-- 
cgit v1.2.1