Commit d4e46919 authored by Douwe Maan's avatar Douwe Maan

Merge branch 'fix-counts-caching' into 'master'

Fix refreshing of issues/MR count caches

Closes #38061

See merge request gitlab-org/gitlab-ce!14363
parents 95de7d50 57b96eb6
...@@ -275,8 +275,6 @@ class Issue < ActiveRecord::Base ...@@ -275,8 +275,6 @@ class Issue < ActiveRecord::Base
end end
def update_project_counter_caches def update_project_counter_caches
return unless update_project_counter_caches?
Projects::OpenIssuesCountService.new(project).refresh_cache Projects::OpenIssuesCountService.new(project).refresh_cache
end end
......
...@@ -958,8 +958,6 @@ class MergeRequest < ActiveRecord::Base ...@@ -958,8 +958,6 @@ class MergeRequest < ActiveRecord::Base
end end
def update_project_counter_caches def update_project_counter_caches
return unless update_project_counter_caches?
Projects::OpenMergeRequestsCountService.new(target_project).refresh_cache Projects::OpenMergeRequestsCountService.new(target_project).refresh_cache
end end
......
...@@ -182,6 +182,7 @@ class IssuableBaseService < BaseService ...@@ -182,6 +182,7 @@ class IssuableBaseService < BaseService
after_create(issuable) after_create(issuable)
execute_hooks(issuable) execute_hooks(issuable)
invalidate_cache_counts(issuable, users: issuable.assignees) invalidate_cache_counts(issuable, users: issuable.assignees)
issuable.update_project_counter_caches
end end
issuable issuable
...@@ -193,8 +194,6 @@ class IssuableBaseService < BaseService ...@@ -193,8 +194,6 @@ class IssuableBaseService < BaseService
def after_create(issuable) def after_create(issuable)
# To be overridden by subclasses # To be overridden by subclasses
issuable.update_project_counter_caches
end end
def before_update(issuable) def before_update(issuable)
...@@ -203,8 +202,6 @@ class IssuableBaseService < BaseService ...@@ -203,8 +202,6 @@ class IssuableBaseService < BaseService
def after_update(issuable) def after_update(issuable)
# To be overridden by subclasses # To be overridden by subclasses
issuable.update_project_counter_caches
end end
def update(issuable) def update(issuable)
...@@ -229,6 +226,10 @@ class IssuableBaseService < BaseService ...@@ -229,6 +226,10 @@ class IssuableBaseService < BaseService
before_update(issuable) 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 } if issuable.with_transaction_returning_status { issuable.save }
# We do not touch as it will affect a update on updated_at field # We do not touch as it will affect a update on updated_at field
ActiveRecord::Base.no_touching do ActiveRecord::Base.no_touching do
...@@ -249,6 +250,8 @@ class IssuableBaseService < BaseService ...@@ -249,6 +250,8 @@ class IssuableBaseService < BaseService
after_update(issuable) after_update(issuable)
issuable.create_new_cross_references!(current_user) issuable.create_new_cross_references!(current_user)
execute_hooks(issuable, 'update') execute_hooks(issuable, 'update')
issuable.update_project_counter_caches if update_project_counters
end end
end end
......
...@@ -29,6 +29,7 @@ module Issues ...@@ -29,6 +29,7 @@ module Issues
todo_service.close_issue(issue, current_user) todo_service.close_issue(issue, current_user)
execute_hooks(issue, 'close') execute_hooks(issue, 'close')
invalidate_cache_counts(issue, users: issue.assignees) invalidate_cache_counts(issue, users: issue.assignees)
issue.update_project_counter_caches
end end
issue issue
......
...@@ -14,6 +14,7 @@ module MergeRequests ...@@ -14,6 +14,7 @@ module MergeRequests
todo_service.close_merge_request(merge_request, current_user) todo_service.close_merge_request(merge_request, current_user)
execute_hooks(merge_request, 'close') execute_hooks(merge_request, 'close')
invalidate_cache_counts(merge_request, users: merge_request.assignees) invalidate_cache_counts(merge_request, users: merge_request.assignees)
merge_request.update_project_counter_caches
end end
merge_request merge_request
......
...@@ -2,6 +2,11 @@ module Projects ...@@ -2,6 +2,11 @@ module Projects
# Base class for the various service classes that count project data (e.g. # Base class for the various service classes that count project data (e.g.
# issues or forks). # issues or forks).
class CountService 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) def initialize(project)
@project = project @project = project
end end
...@@ -37,7 +42,7 @@ module Projects ...@@ -37,7 +42,7 @@ module Projects
end end
def cache_key def cache_key
['projects', @project.id, cache_key_name] ['projects', 'count_service', VERSION, @project.id, cache_key_name]
end end
end end
end end
...@@ -42,7 +42,7 @@ describe Issues::CloseService do ...@@ -42,7 +42,7 @@ describe Issues::CloseService do
service.execute(issue) service.execute(issue)
end 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) } expect { service.execute(issue) }
.to change { project.open_issues_count }.from(1).to(0) .to change { project.open_issues_count }.from(1).to(0)
end end
......
...@@ -35,7 +35,7 @@ describe Issues::CreateService do ...@@ -35,7 +35,7 @@ describe Issues::CreateService do
expect(issue.due_date).to eq Date.tomorrow expect(issue.due_date).to eq Date.tomorrow
end 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) expect { issue }.to change { project.open_issues_count }.from(0).to(1)
end end
......
...@@ -64,6 +64,13 @@ describe Issues::UpdateService, :mailer do ...@@ -64,6 +64,13 @@ describe Issues::UpdateService, :mailer do
expect(issue.due_date).to eq Date.tomorrow expect(issue.due_date).to eq Date.tomorrow
end 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 it 'updates open issue counter for assignees when issue is reassigned' do
update_issue(assignee_ids: [user2.id]) update_issue(assignee_ids: [user2.id])
......
...@@ -52,7 +52,7 @@ describe MergeRequests::CloseService do ...@@ -52,7 +52,7 @@ describe MergeRequests::CloseService do
end end
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, {}) service = described_class.new(project, user, {})
expect { service.execute(merge_request) } expect { service.execute(merge_request) }
......
...@@ -37,7 +37,7 @@ describe MergeRequests::CreateService do ...@@ -37,7 +37,7 @@ describe MergeRequests::CreateService do
expect(service).to have_received(:execute_hooks).with(merge_request) expect(service).to have_received(:execute_hooks).with(merge_request)
end 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 } expect { service.execute }
.to change { project.open_merge_requests_count }.from(0).to(1) .to change { project.open_merge_requests_count }.from(0).to(1)
end end
......
...@@ -66,8 +66,8 @@ describe Projects::CountService do ...@@ -66,8 +66,8 @@ describe Projects::CountService do
describe '#cache_key' do describe '#cache_key' do
it 'returns the cache key as an Array' do it 'returns the cache key as an Array' do
allow(service).to receive(:cache_key_name).and_return('count_service') allow(service).to receive(:cache_key_name).and_return('foo')
expect(service.cache_key).to eq(['projects', 1, 'count_service']) expect(service.cache_key).to eq(['projects', 'count_service', described_class::VERSION, 1, 'foo'])
end end
end end
end end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment