Merge branch 'cache-issue-and-mr-counts' into 'master'

Cache the number of open issues and merge requests

Closes #36622

See merge request !13639

app/models/issue.rb

@@ -50,7 +50,10 @@ class Issue < ActiveRecord::Base
 
   scope :preload_associations, -> { preload(:labels, project: :namespace) }
 
+  scope :public_only, -> { where(confidential: false) }
+
   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
@@ -266,6 +269,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.

app/models/merge_request.rb

@@ -31,6 +31,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
@@ -936,6 +937,10 @@ class MergeRequest < ActiveRecord::Base
     true
   end
 
+  def update_project_counter_caches
+    Projects::OpenMergeRequestsCountService.new(target_project).refresh_cache
+  end
+
   private
 
   def write_ref

app/models/project.rb

@@ -1167,7 +1167,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)

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)

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)

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

app/services/projects/count_service.rb

+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

app/services/projects/forks_count_service.rb

 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

app/services/projects/open_issues_count_service.rb

+module Projects
+  # Service class for counting and caching the number of open issues of a
+  # project.
+  class OpenIssuesCountService < CountService
+    def relation_for_count
+      # We don't include confidential issues in this number since this would
+      # expose the number of confidential issues to non project members.
+      @project.issues.opened.public_only
+    end
+
+    def cache_key_name
+      'open_issues_count'
+    end
+  end
+end

app/services/projects/open_merge_requests_count_service.rb

+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

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

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

changelogs/unreleased/cache-issue-and-mr-counts.yml

+---
+title: Cache the number of open issues and merge requests
+merge_request:
+author:
+type: other

spec/models/issue_spec.rb

@@ -751,4 +751,22 @@ 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
+
+  describe '.public_only' do
+    it 'only returns public issues' do
+      public_issue = create(:issue)
+      create(:issue, confidential: true)
+
+      expect(described_class.public_only).to eq([public_issue])
+    end
+  end
 end

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

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)
 

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) }
 

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)

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

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
           }

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

spec/services/projects/count_service_spec.rb

+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

spec/services/projects/forks_count_service_spec.rb

 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

spec/services/projects/open_issues_count_service_spec.rb

+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
+
+    it 'does not include confidential issues in the issue count' do
+      project = create(:project)
+
+      create(:issue, :opened, project: project)
+      create(:issue, :opened, confidential: true, project: project)
+
+      expect(described_class.new(project).count).to eq(1)
+    end
+  end
+end

spec/services/projects/open_merge_requests_count_service_spec.rb

+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