Merge branch 'issue_47729' into 'master'

Fix refresh cache for open issues count service

Closes #47945

See merge request gitlab-org/gitlab-ce!19904

app/services/base_count_service.rb

@@ -17,7 +17,7 @@ class BaseCountService
   end
 
   def refresh_cache(&block)
-    Rails.cache.write(cache_key, block_given? ? yield : uncached_count, raw: raw?)
+    update_cache_for_key(cache_key, &block)
   end
 
   def uncached_count
@@ -41,4 +41,8 @@ class BaseCountService
   def cache_options
     { raw: raw? }
   end
+
+  def update_cache_for_key(key, &block)
+    Rails.cache.write(key, block_given? ? yield : uncached_count, raw: raw?)
+  end
 end

app/services/projects/count_service.rb

@@ -22,8 +22,10 @@ module Projects
       )
     end
 
-    def cache_key
-      ['projects', 'count_service', VERSION, @project.id, cache_key_name]
+    def cache_key(key = nil)
+      cache_key = key || cache_key_name
+
+      ['projects', 'count_service', VERSION, @project.id, cache_key]
     end
 
     def self.query(project_ids)

app/services/projects/open_issues_count_service.rb

@@ -4,6 +4,10 @@ module Projects
   class OpenIssuesCountService < Projects::CountService
     include Gitlab::Utils::StrongMemoize
 
+    # Cache keys used to store issues count
+    PUBLIC_COUNT_KEY = 'public_open_issues_count'.freeze
+    TOTAL_COUNT_KEY = 'total_open_issues_count'.freeze
+
     def initialize(project, user = nil)
       @user = user
 
@@ -11,7 +15,7 @@ module Projects
     end
 
     def cache_key_name
-      public_only? ? 'public_open_issues_count' : 'total_open_issues_count'
+      public_only? ? PUBLIC_COUNT_KEY : TOTAL_COUNT_KEY
     end
 
     def public_only?
@@ -28,6 +32,32 @@ module Projects
       end
     end
 
+    def public_count_cache_key
+      cache_key(PUBLIC_COUNT_KEY)
+    end
+
+    def total_count_cache_key
+      cache_key(TOTAL_COUNT_KEY)
+    end
+
+    def refresh_cache(&block)
+      if block_given?
+        super(&block)
+      else
+        count_grouped_by_confidential = self.class.query(@project, public_only: false).group(:confidential).count
+        public_count = count_grouped_by_confidential[false] || 0
+        total_count = public_count + (count_grouped_by_confidential[true] || 0)
+
+        update_cache_for_key(public_count_cache_key) do
+          public_count
+        end
+
+        update_cache_for_key(total_count_cache_key) do
+          total_count
+        end
+      end
+    end
+
     # We only show total issues count for reporters
     # which are allowed to view confidential issues
     # This will still show a discrepancy on issues number but should be less than before.

changelogs/unreleased/issue_47729.yml

+---
+title: Fix refreshing cache keys for open issues count
+merge_request:
+author:
+type: fixed

spec/services/projects/batch_open_issues_count_service_spec.rb

+require 'spec_helper'
+
+describe Projects::BatchOpenIssuesCountService do
+  let!(:project_1) { create(:project) }
+  let!(:project_2) { create(:project) }
+
+  let(:subject) { described_class.new([project_1, project_2]) }
+
+  context '#refresh_cache', :use_clean_rails_memory_store_caching do
+    before do
+      create(:issue, project: project_1)
+      create(:issue, project: project_1, confidential: true)
+
+      create(:issue, project: project_2)
+      create(:issue, project: project_2, confidential: true)
+    end
+
+    context 'when cache is clean' do
+      it 'refreshes cache keys correctly' do
+        subject.refresh_cache
+
+        # It does not update total issues cache
+        expect(Rails.cache.read(get_cache_key(subject, project_1))).to eq(nil)
+        expect(Rails.cache.read(get_cache_key(subject, project_2))).to eq(nil)
+
+        expect(Rails.cache.read(get_cache_key(subject, project_1, true))).to eq(1)
+        expect(Rails.cache.read(get_cache_key(subject, project_1, true))).to eq(1)
+      end
+    end
+
+    context 'when issues count is already cached' do
+      before do
+        create(:issue, project: project_2)
+        subject.refresh_cache
+      end
+
+      it 'does update cache again' do
+        expect(Rails.cache).not_to receive(:write)
+
+        subject.refresh_cache
+      end
+    end
+  end
+
+  def get_cache_key(subject, project, public_key = false)
+    service = subject.count_service.new(project)
+
+    if public_key
+      service.cache_key(service.class::PUBLIC_COUNT_KEY)
+    else
+      service.cache_key(service.class::TOTAL_COUNT_KEY)
+    end
+  end
+end

spec/services/projects/open_issues_count_service_spec.rb

@@ -50,5 +50,40 @@ describe Projects::OpenIssuesCountService do
         end
       end
     end
+
+    context '#refresh_cache', :use_clean_rails_memory_store_caching do
+      let(:subject) { described_class.new(project) }
+
+      before do
+        create(:issue, :opened, project: project)
+        create(:issue, :opened, project: project)
+        create(:issue, :opened, confidential: true, project: project)
+      end
+
+      context 'when cache is empty' do
+        it 'refreshes cache keys correctly' do
+          subject.refresh_cache
+
+          expect(Rails.cache.read(subject.cache_key(described_class::PUBLIC_COUNT_KEY))).to eq(2)
+          expect(Rails.cache.read(subject.cache_key(described_class::TOTAL_COUNT_KEY))).to eq(3)
+        end
+      end
+
+      context 'when cache is outdated' do
+        before do
+          subject.refresh_cache
+        end
+
+        it 'refreshes cache keys correctly' do
+          create(:issue, :opened, project: project)
+          create(:issue, :opened, confidential: true, project: project)
+
+          subject.refresh_cache
+
+          expect(Rails.cache.read(subject.cache_key(described_class::PUBLIC_COUNT_KEY))).to eq(3)
+          expect(Rails.cache.read(subject.cache_key(described_class::TOTAL_COUNT_KEY))).to eq(5)
+        end
+      end
+    end
   end
 end