diff --git a/app/services/base_count_service.rb b/app/services/base_count_service.rb index f2844854112eab1aa476715f87aefb56d4d88c9d..975e288301ce0abb04055ff084036d0de3e6497f 100644 --- a/app/services/base_count_service.rb +++ b/app/services/base_count_service.rb @@ -17,7 +17,7 @@ def count_stored? 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 @@ def cache_key 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 diff --git a/app/services/projects/count_service.rb b/app/services/projects/count_service.rb index 933829b557b298bcaf47ffac86efdebd301874b9..4c8e000928f9b75111b3c2a1f63dafa1a1d8fed4 100644 --- a/app/services/projects/count_service.rb +++ b/app/services/projects/count_service.rb @@ -22,8 +22,10 @@ def cache_key_name ) 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) diff --git a/app/services/projects/open_issues_count_service.rb b/app/services/projects/open_issues_count_service.rb index 0a004677417a0eb65ad479c1f20fb9d1bdec6eac..78b1477186af2bcba68dd2f6104cfff753bddb26 100644 --- a/app/services/projects/open_issues_count_service.rb +++ b/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 @@ def initialize(project, user = nil) 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 @@ def user_is_at_least_reporter? 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. diff --git a/changelogs/unreleased/issue_47729.yml b/changelogs/unreleased/issue_47729.yml new file mode 100644 index 0000000000000000000000000000000000000000..e27972af114b4069dfc7b8b890086395aedbb27f --- /dev/null +++ b/changelogs/unreleased/issue_47729.yml @@ -0,0 +1,5 @@ +--- +title: Fix refreshing cache keys for open issues count +merge_request: +author: +type: fixed diff --git a/spec/services/projects/batch_open_issues_count_service_spec.rb b/spec/services/projects/batch_open_issues_count_service_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..599aaf62080df2bc448de0630520425687e89d30 --- /dev/null +++ b/spec/services/projects/batch_open_issues_count_service_spec.rb @@ -0,0 +1,54 @@ +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 diff --git a/spec/services/projects/open_issues_count_service_spec.rb b/spec/services/projects/open_issues_count_service_spec.rb index 06b470849b361e42aa7ec6090bcf003f7eb9e61a..562c14a8df879fba1b466bdba6dadc6295e04045 100644 --- a/spec/services/projects/open_issues_count_service_spec.rb +++ b/spec/services/projects/open_issues_count_service_spec.rb @@ -50,5 +50,40 @@ 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