From 57b96eb6db9b860991b035714e65ffd557327b6f Mon Sep 17 00:00:00 2001 From: Yorick Peterse Date: Tue, 19 Sep 2017 13:55:56 +0200 Subject: [PATCH] Fix refreshing of issues/MR count caches This ensures the open issues/MR count caches are refreshed properly when creating new issues or MRs. This MR also includes a change to the cache keys to ensure all caches are rebuilt on the fly. This particular problem was not caught in the test suite due to a null cache being used, resulting in all calls that would use a cache using the underlying data directly. In production the code would fail because a newly saved record returns an empty hash in #changes meaning checks such as `state_changed? || confidential_changed?` would return false for new rows, thus never updating the counters. Fixes https://gitlab.com/gitlab-org/gitlab-ce/issues/38061 --- app/models/issue.rb | 2 -- app/models/merge_request.rb | 2 -- app/services/issuable_base_service.rb | 11 +++++++---- app/services/issues/close_service.rb | 1 + app/services/merge_requests/close_service.rb | 1 + app/services/projects/count_service.rb | 7 ++++++- spec/services/issues/close_service_spec.rb | 2 +- spec/services/issues/create_service_spec.rb | 2 +- spec/services/issues/update_service_spec.rb | 7 +++++++ spec/services/merge_requests/close_service_spec.rb | 2 +- spec/services/merge_requests/create_service_spec.rb | 2 +- spec/services/projects/count_service_spec.rb | 4 ++-- 12 files changed, 28 insertions(+), 15 deletions(-) diff --git a/app/models/issue.rb b/app/models/issue.rb index cd5056aae5e..92a454300af 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -275,8 +275,6 @@ class Issue < ActiveRecord::Base end def update_project_counter_caches - return unless update_project_counter_caches? - Projects::OpenIssuesCountService.new(project).refresh_cache end diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 31bd130dcc2..8d9a30397a9 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -958,8 +958,6 @@ class MergeRequest < ActiveRecord::Base end def update_project_counter_caches - return unless update_project_counter_caches? - Projects::OpenMergeRequestsCountService.new(target_project).refresh_cache end diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb index 8b967b78052..12604e7eb5d 100644 --- a/app/services/issuable_base_service.rb +++ b/app/services/issuable_base_service.rb @@ -182,6 +182,7 @@ class IssuableBaseService < BaseService after_create(issuable) execute_hooks(issuable) invalidate_cache_counts(issuable, users: issuable.assignees) + issuable.update_project_counter_caches end issuable @@ -193,8 +194,6 @@ class IssuableBaseService < BaseService def after_create(issuable) # To be overridden by subclasses - - issuable.update_project_counter_caches end def before_update(issuable) @@ -203,8 +202,6 @@ class IssuableBaseService < BaseService def after_update(issuable) # To be overridden by subclasses - - issuable.update_project_counter_caches end def update(issuable) @@ -229,6 +226,10 @@ class IssuableBaseService < BaseService before_update(issuable) + # We have to perform this check before saving the issuable as Rails resets + # the changed fields upon calling #save. + update_project_counters = issuable.update_project_counter_caches? + if issuable.with_transaction_returning_status { issuable.save } # We do not touch as it will affect a update on updated_at field ActiveRecord::Base.no_touching do @@ -249,6 +250,8 @@ class IssuableBaseService < BaseService after_update(issuable) issuable.create_new_cross_references!(current_user) execute_hooks(issuable, 'update') + + issuable.update_project_counter_caches if update_project_counters end end diff --git a/app/services/issues/close_service.rb b/app/services/issues/close_service.rb index 74459c3342c..0c5cf2c62ad 100644 --- a/app/services/issues/close_service.rb +++ b/app/services/issues/close_service.rb @@ -29,6 +29,7 @@ module Issues todo_service.close_issue(issue, current_user) execute_hooks(issue, 'close') invalidate_cache_counts(issue, users: issue.assignees) + issue.update_project_counter_caches end issue diff --git a/app/services/merge_requests/close_service.rb b/app/services/merge_requests/close_service.rb index c0ce01f7523..40213c99014 100644 --- a/app/services/merge_requests/close_service.rb +++ b/app/services/merge_requests/close_service.rb @@ -14,6 +14,7 @@ module MergeRequests todo_service.close_merge_request(merge_request, current_user) execute_hooks(merge_request, 'close') invalidate_cache_counts(merge_request, users: merge_request.assignees) + merge_request.update_project_counter_caches end merge_request diff --git a/app/services/projects/count_service.rb b/app/services/projects/count_service.rb index 5e633c37bf8..aa034315280 100644 --- a/app/services/projects/count_service.rb +++ b/app/services/projects/count_service.rb @@ -2,6 +2,11 @@ module Projects # Base class for the various service classes that count project data (e.g. # issues or forks). class CountService + # The version of the cache format. This should be bumped whenever the + # underlying logic changes. This removes the need for explicitly flushing + # all caches. + VERSION = 1 + def initialize(project) @project = project end @@ -37,7 +42,7 @@ module Projects end def cache_key - ['projects', @project.id, cache_key_name] + ['projects', 'count_service', VERSION, @project.id, cache_key_name] end end end diff --git a/spec/services/issues/close_service_spec.rb b/spec/services/issues/close_service_spec.rb index 171f70c32a8..5c27e8fd561 100644 --- a/spec/services/issues/close_service_spec.rb +++ b/spec/services/issues/close_service_spec.rb @@ -42,7 +42,7 @@ describe Issues::CloseService do service.execute(issue) end - it 'refreshes the number of open issues' do + it 'refreshes the number of open issues', :use_clean_rails_memory_store_caching do expect { service.execute(issue) } .to change { project.open_issues_count }.from(1).to(0) end diff --git a/spec/services/issues/create_service_spec.rb b/spec/services/issues/create_service_spec.rb index cc3d648c340..d86da244520 100644 --- a/spec/services/issues/create_service_spec.rb +++ b/spec/services/issues/create_service_spec.rb @@ -35,7 +35,7 @@ describe Issues::CreateService do expect(issue.due_date).to eq Date.tomorrow end - it 'refreshes the number of open issues' do + it 'refreshes the number of open issues', :use_clean_rails_memory_store_caching do expect { issue }.to change { project.open_issues_count }.from(0).to(1) end diff --git a/spec/services/issues/update_service_spec.rb b/spec/services/issues/update_service_spec.rb index 15a50b85f19..a8a8aeed1bd 100644 --- a/spec/services/issues/update_service_spec.rb +++ b/spec/services/issues/update_service_spec.rb @@ -64,6 +64,13 @@ describe Issues::UpdateService, :mailer do expect(issue.due_date).to eq Date.tomorrow end + it 'refreshes the number of open issues when the issue is made confidential', :use_clean_rails_memory_store_caching do + issue # make sure the issue is created first so our counts are correct. + + expect { update_issue(confidential: true) } + .to change { project.open_issues_count }.from(1).to(0) + end + it 'updates open issue counter for assignees when issue is reassigned' do update_issue(assignee_ids: [user2.id]) diff --git a/spec/services/merge_requests/close_service_spec.rb b/spec/services/merge_requests/close_service_spec.rb index 7e65369762c..b3886987316 100644 --- a/spec/services/merge_requests/close_service_spec.rb +++ b/spec/services/merge_requests/close_service_spec.rb @@ -52,7 +52,7 @@ describe MergeRequests::CloseService do end end - it 'refreshes the number of open merge requests for a valid MR' do + it 'refreshes the number of open merge requests for a valid MR', :use_clean_rails_memory_store_caching do service = described_class.new(project, user, {}) expect { service.execute(merge_request) } diff --git a/spec/services/merge_requests/create_service_spec.rb b/spec/services/merge_requests/create_service_spec.rb index d6409c0d625..a047f891ab2 100644 --- a/spec/services/merge_requests/create_service_spec.rb +++ b/spec/services/merge_requests/create_service_spec.rb @@ -37,7 +37,7 @@ describe MergeRequests::CreateService do expect(service).to have_received(:execute_hooks).with(merge_request) end - it 'refreshes the number of open merge requests' do + it 'refreshes the number of open merge requests', :use_clean_rails_memory_store_caching do expect { service.execute } .to change { project.open_merge_requests_count }.from(0).to(1) end diff --git a/spec/services/projects/count_service_spec.rb b/spec/services/projects/count_service_spec.rb index 79b01e7620e..cc496501bad 100644 --- a/spec/services/projects/count_service_spec.rb +++ b/spec/services/projects/count_service_spec.rb @@ -66,8 +66,8 @@ describe Projects::CountService do 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']) + allow(service).to receive(:cache_key_name).and_return('foo') + expect(service.cache_key).to eq(['projects', 'count_service', described_class::VERSION, 1, 'foo']) end end end