Fix refreshing of issues/MR count cachesfix-counts-caching

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

6 files changed, 15 insertions, 9 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