Cache the number of open issues and merge requestscache-issue-and-mr-counts

Every project page displays a navigation menu that in turn displays the
number of open issues and merge requests. This means that for every
project page we run two COUNT(*) queries, each taking up roughly 30
milliseconds on GitLab.com. By caching these numbers and refreshing them
whenever necessary we can reduce loading times of all these pages by up
to roughly 60 milliseconds.

The number of open issues _includes_ confidential issues, even if you
can not see these issues. This is a trade-off to keep the code simple
and to ensure refreshing the data only needs 2 COUNT(*) queries instead
of 3. Further, by including all issues we don't need any additional work
to determine if the issue counter we display to a user should include
confidential issues or not. This does mean that if a project has 10
issues of which 5 are confidential, everybody (that can see the project)
will see the number 10.

Because we now have 3 similar counting service classes the code
previously used in Projects::ForksCountService has mostly been moved to
Projects::CountService, which in turn is reused by the various service
classes.

Fixes https://gitlab.com/gitlab-org/gitlab-ce/issues/36622

25 files changed, 279 insertions, 71 deletions

diff --git a/app/models/issue.rb b/app/models/issue.rb
index 1c948c8957e..9dbc018ed3e 100644
--- a/app/models/issue.rb
+++ b/app/models/issue.rb
@@ -54,6 +54,7 @@ class Issue < ActiveRecord::Base
   scope :preload_associations, -> { preload(:labels, project: :namespace) }
 
   after_save :expire_etag_cache
+  after_commit :update_project_counter_caches, on: :destroy
 
   attr_spammable :title, spam_title: true
   attr_spammable :description, spam_description: true
@@ -269,6 +270,10 @@ class Issue < ActiveRecord::Base
     end
   end
 
+  def update_project_counter_caches
+    Projects::OpenIssuesCountService.new(project).refresh_cache
+  end
+
   private
 
   # Returns `true` if the given User can read the current Issue.
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index ac08dc0ee1f..7fe7df75944 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -32,6 +32,7 @@ class MergeRequest < ActiveRecord::Base
 
   after_create :ensure_merge_request_diff, unless: :importing?
   after_update :reload_diff_if_branch_changed
+  after_commit :update_project_counter_caches, on: :destroy
 
   # When this attribute is true some MR validation is ignored
   # It allows us to close or modify broken merge requests
@@ -937,6 +938,10 @@ class MergeRequest < ActiveRecord::Base
     true
   end
 
+  def update_project_counter_caches
+    Projects::OpenMergeRequestsCountService.new(target_project).refresh_cache
+  end
+
   private
 
   def write_ref
diff --git a/app/models/project.rb b/app/models/project.rb
index be248bc99e1..ddef8b82dee 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -1158,7 +1158,11 @@ class Project < ActiveRecord::Base
   end
 
   def open_issues_count
-    issues.opened.count
+    Projects::OpenIssuesCountService.new(self).count
+  end
+
+  def open_merge_requests_count
+    Projects::OpenMergeRequestsCountService.new(self).count
   end
 
   def visibility_level_allowed_as_fork?(level = self.visibility_level)
diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb
index 4a4f2b91182..1486db046b5 100644
--- a/app/services/issuable_base_service.rb
+++ b/app/services/issuable_base_service.rb
@@ -192,6 +192,8 @@ class IssuableBaseService < BaseService
 
   def after_create(issuable)
     # To be overridden by subclasses
+
+    issuable.update_project_counter_caches
   end
 
   def before_update(issuable)
@@ -200,6 +202,8 @@ class IssuableBaseService < BaseService
 
   def after_update(issuable)
     # To be overridden by subclasses
+
+    issuable.update_project_counter_caches
   end
 
   def update(issuable)
diff --git a/app/services/issues/create_service.rb b/app/services/issues/create_service.rb
index 234fcbede03..0307634c0b6 100644
--- a/app/services/issues/create_service.rb
+++ b/app/services/issues/create_service.rb
@@ -27,6 +27,8 @@ module Issues
       todo_service.new_issue(issuable, current_user)
       user_agent_detail_service.create
       resolve_discussions_with_issue(issuable)
+
+      super
     end
 
     def resolve_discussions_with_issue(issue)
diff --git a/app/services/merge_requests/create_service.rb b/app/services/merge_requests/create_service.rb
index 194413bf321..3d53fe0646b 100644
--- a/app/services/merge_requests/create_service.rb
+++ b/app/services/merge_requests/create_service.rb
@@ -28,6 +28,8 @@ module MergeRequests
       todo_service.new_merge_request(issuable, current_user)
       issuable.cache_merge_request_closes_issues!(current_user)
       update_merge_requests_head_pipeline(issuable)
+
+      super
     end
 
     private
diff --git a/app/services/projects/count_service.rb b/app/services/projects/count_service.rb
new file mode 100644
index 00000000000..5e633c37bf8
--- /dev/null
+++ b/app/services/projects/count_service.rb
@@ -0,0 +1,43 @@
+module Projects
+  # Base class for the various service classes that count project data (e.g.
+  # issues or forks).
+  class CountService
+    def initialize(project)
+      @project = project
+    end
+
+    def relation_for_count
+      raise(
+        NotImplementedError,
+        '"relation_for_count" must be implemented and return an ActiveRecord::Relation'
+      )
+    end
+
+    def count
+      Rails.cache.fetch(cache_key) { uncached_count }
+    end
+
+    def refresh_cache
+      Rails.cache.write(cache_key, uncached_count)
+    end
+
+    def uncached_count
+      relation_for_count.count
+    end
+
+    def delete_cache
+      Rails.cache.delete(cache_key)
+    end
+
+    def cache_key_name
+      raise(
+        NotImplementedError,
+        '"cache_key_name" must be implemented and return a String'
+      )
+    end
+
+    def cache_key
+      ['projects', @project.id, cache_key_name]
+    end
+  end
+end
diff --git a/app/services/projects/forks_count_service.rb b/app/services/projects/forks_count_service.rb
index e2e2b1da91d..3a0fa84b868 100644
--- a/app/services/projects/forks_count_service.rb
+++ b/app/services/projects/forks_count_service.rb
@@ -1,30 +1,12 @@
 module Projects
   # Service class for getting and caching the number of forks of a project.
-  class ForksCountService
-    def initialize(project)
-      @project = project
+  class ForksCountService < CountService
+    def relation_for_count
+      @project.forks
     end
 
-    def count
-      Rails.cache.fetch(cache_key) { uncached_count }
-    end
-
-    def refresh_cache
-      Rails.cache.write(cache_key, uncached_count)
-    end
-
-    def delete_cache
-      Rails.cache.delete(cache_key)
-    end
-
-    private
-
-    def uncached_count
-      @project.forks.count
-    end
-
-    def cache_key
-      ['projects', @project.id, 'forks_count']
+    def cache_key_name
+      'forks_count'
     end
   end
 end
diff --git a/app/services/projects/open_issues_count_service.rb b/app/services/projects/open_issues_count_service.rb
new file mode 100644
index 00000000000..a1dce31cf33
--- /dev/null
+++ b/app/services/projects/open_issues_count_service.rb
@@ -0,0 +1,13 @@
+module Projects
+  # Service class for counting and caching the number of open issues of a
+  # project.
+  class OpenIssuesCountService < CountService
+    def relation_for_count
+      @project.issues.opened
+    end
+
+    def cache_key_name
+      'open_issues_count'
+    end
+  end
+end
diff --git a/app/services/projects/open_merge_requests_count_service.rb b/app/services/projects/open_merge_requests_count_service.rb
new file mode 100644
index 00000000000..2a90f78b90d
--- /dev/null
+++ b/app/services/projects/open_merge_requests_count_service.rb
@@ -0,0 +1,13 @@
+module Projects
+  # Service class for counting and caching the number of open merge requests of
+  # a project.
+  class OpenMergeRequestsCountService < CountService
+    def relation_for_count
+      @project.merge_requests.opened
+    end
+
+    def cache_key_name
+      'open_merge_requests_count'
+    end
+  end
+end
diff --git a/app/views/layouts/nav/_new_project_sidebar.html.haml b/app/views/layouts/nav/_new_project_sidebar.html.haml
index 0ef81375c3a..81c84756ff8 100644
--- a/app/views/layouts/nav/_new_project_sidebar.html.haml
+++ b/app/views/layouts/nav/_new_project_sidebar.html.haml
@@ -86,7 +86,8 @@
             %span.nav-item-name
               Issues
             - if @project.issues_enabled?
-              %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(@project.open_issues_count)
 
           %ul.sidebar-sub-level-items
             = nav_link(controller: :issues) do
@@ -116,7 +117,8 @@
               = custom_icon('mr_bold')
             %span.nav-item-name
               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(@project.open_merge_requests_count)
 
       - if project_nav_tab? :pipelines
         = nav_link(controller: [:pipelines, :builds, :jobs, :pipeline_schedules, :environments, :artifacts]) do
diff --git a/app/views/layouts/nav/_project.html.haml b/app/views/layouts/nav/_project.html.haml
index 924cd2e9681..b88465848e3 100644
--- a/app/views/layouts/nav/_project.html.haml
+++ b/app/views/layouts/nav/_project.html.haml
@@ -28,7 +28,8 @@
           %span
             Issues
             - if @project.issues_enabled?
-              %span.badge.count.issue_counter= number_with_delimiter(issuables_count_for_state(:issues, :opened, finder: IssuesFinder.new(current_user, project_id: @project.id)))
+              %span.badge.count.issue_counter
+                = number_with_delimiter(@project.open_issues_count)
 
     - if project_nav_tab? :merge_requests
       - controllers = [:merge_requests, 'projects/merge_requests/conflicts']
@@ -37,7 +38,8 @@
         = link_to project_merge_requests_path(@project), title: 'Merge Requests', class: 'shortcuts-merge_requests' do
           %span
             Merge Requests
-            %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)))
+            %span.badge.count.merge_counter.js-merge-counter
+              = number_with_delimiter(@project.open_merge_requests_count)
 
     - if project_nav_tab? :pipelines
       = nav_link(controller: [:pipelines, :builds, :environments, :artifacts]) do
diff --git a/changelogs/unreleased/cache-issue-and-mr-counts.yml b/changelogs/unreleased/cache-issue-and-mr-counts.yml
new file mode 100644
index 00000000000..fe3fe3be976
--- /dev/null
+++ b/changelogs/unreleased/cache-issue-and-mr-counts.yml
@@ -0,0 +1,5 @@
+---
+title: Cache the number of open issues and merge requests
+merge_request:
+author:
+type: other
diff --git a/spec/models/issue_spec.rb b/spec/models/issue_spec.rb
index 9203f6562f2..6fdeaf44627 100644
--- a/spec/models/issue_spec.rb
+++ b/spec/models/issue_spec.rb
@@ -751,4 +751,13 @@ describe Issue do
       end
     end
   end
+
+  describe 'removing an issue' do
+    it 'refreshes the number of open issues of the project' do
+      project = subject.project
+
+      expect { subject.destroy }
+        .to change { project.open_issues_count }.from(1).to(0)
+    end
+  end
 end
diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index 026bdbd26d1..1c72513520d 100644
--- a/spec/models/merge_request_spec.rb
+++ b/spec/models/merge_request_spec.rb
@@ -1692,4 +1692,13 @@ describe MergeRequest do
       expect(subject.ref_fetched?).to be_falsey
     end
   end
+
+  describe 'removing a merge request' do
+    it 'refreshes the number of open merge requests of the target project' do
+      project = subject.target_project
+
+      expect { subject.destroy }
+        .to change { project.open_merge_requests_count }.from(1).to(0)
+    end
+  end
 end
diff --git a/spec/services/issues/close_service_spec.rb b/spec/services/issues/close_service_spec.rb
index a03f68434de..171f70c32a8 100644
--- a/spec/services/issues/close_service_spec.rb
+++ b/spec/services/issues/close_service_spec.rb
@@ -42,6 +42,11 @@ describe Issues::CloseService do
       service.execute(issue)
     end
 
+    it 'refreshes the number of open issues' do
+      expect { service.execute(issue) }
+        .to change { project.open_issues_count }.from(1).to(0)
+    end
+
     it 'invalidates counter cache for assignees' do
       expect_any_instance_of(User).to receive(:invalidate_issue_cache_counts)
 
diff --git a/spec/services/issues/create_service_spec.rb b/spec/services/issues/create_service_spec.rb
index 9b2d9e79f4f..78b11cd7991 100644
--- a/spec/services/issues/create_service_spec.rb
+++ b/spec/services/issues/create_service_spec.rb
@@ -35,6 +35,10 @@ describe Issues::CreateService do
         expect(issue.due_date).to eq Date.tomorrow
       end
 
+      it 'refreshes the number of open issues' do
+        expect { issue }.to change { project.open_issues_count }.from(0).to(1)
+      end
+
       context 'when current user cannot admin issues in the project' do
         let(:guest) { create(:user) }
 
diff --git a/spec/services/issues/reopen_service_spec.rb b/spec/services/issues/reopen_service_spec.rb
index 205e9ebd237..48fc98b3b2f 100644
--- a/spec/services/issues/reopen_service_spec.rb
+++ b/spec/services/issues/reopen_service_spec.rb
@@ -34,6 +34,13 @@ describe Issues::ReopenService do
         described_class.new(project, user).execute(issue)
       end
 
+      it 'refreshes the number of opened issues' do
+        service = described_class.new(project, user)
+
+        expect { service.execute(issue) }
+          .to change { project.open_issues_count }.from(0).to(1)
+      end
+
       context 'when issue is not confidential' do
         it 'executes issue hooks' do
           expect(project).to receive(:execute_hooks).with(an_instance_of(Hash), :issue_hooks)
diff --git a/spec/services/merge_requests/close_service_spec.rb b/spec/services/merge_requests/close_service_spec.rb
index 04bf267d167..7e65369762c 100644
--- a/spec/services/merge_requests/close_service_spec.rb
+++ b/spec/services/merge_requests/close_service_spec.rb
@@ -52,6 +52,13 @@ describe MergeRequests::CloseService do
       end
     end
 
+    it 'refreshes the number of open merge requests for a valid MR' do
+      service = described_class.new(project, user, {})
+
+      expect { service.execute(merge_request) }
+        .to change { project.open_merge_requests_count }.from(1).to(0)
+    end
+
     context 'current user is not authorized to close merge request' do
       before do
         perform_enqueued_jobs do
diff --git a/spec/services/merge_requests/create_service_spec.rb b/spec/services/merge_requests/create_service_spec.rb
index a1f3bec42cc..d6409c0d625 100644
--- a/spec/services/merge_requests/create_service_spec.rb
+++ b/spec/services/merge_requests/create_service_spec.rb
@@ -18,31 +18,35 @@ describe MergeRequests::CreateService do
       end
 
       let(:service) { described_class.new(project, user, opts) }
+      let(:merge_request) { service.execute }
 
       before do
         project.team << [user, :master]
         project.team << [assignee, :developer]
         allow(service).to receive(:execute_hooks)
-
-        @merge_request = service.execute
       end
 
       it 'creates an MR' do
-        expect(@merge_request).to be_valid
-        expect(@merge_request.title).to eq('Awesome merge_request')
-        expect(@merge_request.assignee).to be_nil
-        expect(@merge_request.merge_params['force_remove_source_branch']).to eq('1')
+        expect(merge_request).to be_valid
+        expect(merge_request.title).to eq('Awesome merge_request')
+        expect(merge_request.assignee).to be_nil
+        expect(merge_request.merge_params['force_remove_source_branch']).to eq('1')
       end
 
       it 'executes hooks with default action' do
-        expect(service).to have_received(:execute_hooks).with(@merge_request)
+        expect(service).to have_received(:execute_hooks).with(merge_request)
+      end
+
+      it 'refreshes the number of open merge requests' do
+        expect { service.execute }
+          .to change { project.open_merge_requests_count }.from(0).to(1)
       end
 
       it 'does not creates todos' do
         attributes = {
           project: project,
-          target_id: @merge_request.id,
-          target_type: @merge_request.class.name
+          target_id: merge_request.id,
+          target_type: merge_request.class.name
         }
 
         expect(Todo.where(attributes).count).to be_zero
@@ -51,8 +55,8 @@ describe MergeRequests::CreateService do
       it 'creates exactly 1 create MR event' do
         attributes = {
           action: Event::CREATED,
-          target_id: @merge_request.id,
-          target_type: @merge_request.class.name
+          target_id: merge_request.id,
+          target_type: merge_request.class.name
         }
 
         expect(Event.where(attributes).count).to eq(1)
@@ -69,15 +73,15 @@ describe MergeRequests::CreateService do
           }
         end
 
-        it { expect(@merge_request.assignee).to eq assignee }
+        it { expect(merge_request.assignee).to eq assignee }
 
         it 'creates a todo for new assignee' do
           attributes = {
             project: project,
             author: user,
             user: assignee,
-            target_id: @merge_request.id,
-            target_type: @merge_request.class.name,
+            target_id: merge_request.id,
+            target_type: merge_request.class.name,
             action: Todo::ASSIGNED,
             state: :pending
           }
diff --git a/spec/services/merge_requests/reopen_service_spec.rb b/spec/services/merge_requests/reopen_service_spec.rb
index f02af0c582e..fa652611c6b 100644
--- a/spec/services/merge_requests/reopen_service_spec.rb
+++ b/spec/services/merge_requests/reopen_service_spec.rb
@@ -47,6 +47,13 @@ describe MergeRequests::ReopenService do
       end
     end
 
+    it 'refreshes the number of open merge requests for a valid MR' do
+      service = described_class.new(project, user, {})
+
+      expect { service.execute(merge_request) }
+        .to change { project.open_merge_requests_count }.from(0).to(1)
+    end
+
     context 'current user is not authorized to reopen merge request' do
       before do
         perform_enqueued_jobs do
diff --git a/spec/services/projects/count_service_spec.rb b/spec/services/projects/count_service_spec.rb
new file mode 100644
index 00000000000..79b01e7620e
--- /dev/null
+++ b/spec/services/projects/count_service_spec.rb
@@ -0,0 +1,73 @@
+require 'spec_helper'
+
+describe Projects::CountService do
+  let(:project) { build(:project, id: 1) }
+  let(:service) { described_class.new(project) }
+
+  describe '#relation_for_count' do
+    it 'raises NotImplementedError' do
+      expect { service.relation_for_count }.to raise_error(NotImplementedError)
+    end
+  end
+
+  describe '#count' do
+    before do
+      allow(service).to receive(:cache_key_name).and_return('count_service')
+    end
+
+    it 'returns the number of rows' do
+      allow(service).to receive(:uncached_count).and_return(1)
+
+      expect(service.count).to eq(1)
+    end
+
+    it 'caches the number of rows', :use_clean_rails_memory_store_caching do
+      expect(service).to receive(:uncached_count).once.and_return(1)
+
+      2.times do
+        expect(service.count).to eq(1)
+      end
+    end
+  end
+
+  describe '#refresh_cache', :use_clean_rails_memory_store_caching do
+    before do
+      allow(service).to receive(:cache_key_name).and_return('count_service')
+    end
+
+    it 'refreshes the cache' do
+      expect(service).to receive(:uncached_count).once.and_return(1)
+
+      service.refresh_cache
+
+      expect(service.count).to eq(1)
+    end
+  end
+
+  describe '#delete_cache', :use_clean_rails_memory_store_caching do
+    before do
+      allow(service).to receive(:cache_key_name).and_return('count_service')
+    end
+
+    it 'removes the cache' do
+      expect(service).to receive(:uncached_count).twice.and_return(1)
+
+      service.count
+      service.delete_cache
+      service.count
+    end
+  end
+
+  describe '#cache_key_name' do
+    it 'raises NotImplementedError' do
+      expect { service.cache_key_name }.to raise_error(NotImplementedError)
+    end
+  end
+
+  describe '#cache_key' do
+    it 'returns the cache key as an Array' do
+      allow(service).to receive(:cache_key_name).and_return('count_service')
+      expect(service.cache_key).to eq(['projects', 1, 'count_service'])
+    end
+  end
+end
diff --git a/spec/services/projects/forks_count_service_spec.rb b/spec/services/projects/forks_count_service_spec.rb
index cf299c5d09b..9f8e7ee18a8 100644
--- a/spec/services/projects/forks_count_service_spec.rb
+++ b/spec/services/projects/forks_count_service_spec.rb
@@ -1,40 +1,14 @@
 require 'spec_helper'
 
 describe Projects::ForksCountService do
-  let(:project) { build(:project, id: 42) }
-  let(:service) { described_class.new(project) }
-
   describe '#count' do
     it 'returns the number of forks' do
-      allow(service).to receive(:uncached_count).and_return(1)
-
-      expect(service.count).to eq(1)
-    end
-
-    it 'caches the forks count', :use_clean_rails_memory_store_caching do
-      expect(service).to receive(:uncached_count).once.and_return(1)
+      project = build(:project, id: 42)
+      service = described_class.new(project)
 
-      2.times { service.count }
-    end
-  end
-
-  describe '#refresh_cache', :use_clean_rails_memory_store_caching do
-    it 'refreshes the cache' do
-      expect(service).to receive(:uncached_count).once.and_return(1)
-
-      service.refresh_cache
+      allow(service).to receive(:uncached_count).and_return(1)
 
       expect(service.count).to eq(1)
     end
   end
-
-  describe '#delete_cache', :use_clean_rails_memory_store_caching do
-    it 'removes the cache' do
-      expect(service).to receive(:uncached_count).twice.and_return(1)
-
-      service.count
-      service.delete_cache
-      service.count
-    end
-  end
 end
diff --git a/spec/services/projects/open_issues_count_service_spec.rb b/spec/services/projects/open_issues_count_service_spec.rb
new file mode 100644
index 00000000000..27c4596bc88
--- /dev/null
+++ b/spec/services/projects/open_issues_count_service_spec.rb
@@ -0,0 +1,12 @@
+require 'spec_helper'
+
+describe Projects::OpenIssuesCountService do
+  describe '#count' do
+    it 'returns the number of open issues' do
+      project = create(:project)
+      create(:issue, :opened, project: project)
+
+      expect(described_class.new(project).count).to eq(1)
+    end
+  end
+end
diff --git a/spec/services/projects/open_merge_requests_count_service_spec.rb b/spec/services/projects/open_merge_requests_count_service_spec.rb
new file mode 100644
index 00000000000..9f49b9ec6a2
--- /dev/null
+++ b/spec/services/projects/open_merge_requests_count_service_spec.rb
@@ -0,0 +1,15 @@
+require 'spec_helper'
+
+describe Projects::OpenMergeRequestsCountService do
+  describe '#count' do
+    it 'returns the number of open merge requests' do
+      project = create(:project)
+      create(:merge_request,
+             :opened,
+             source_project: project,
+             target_project: project)
+
+      expect(described_class.new(project).count).to eq(1)
+    end
+  end
+end