From 29b0a90c208f29606a05d1391a72b9ff7ff843b1 Mon Sep 17 00:00:00 2001 From: Andreas Brandl Date: Wed, 4 Apr 2018 17:14:19 +0200 Subject: [PATCH] Cache personal projects count. Closes #37462. --- app/models/user.rb | 21 +++++++++++++++---- app/services/projects/create_service.rb | 2 ++ app/services/projects/destroy_service.rb | 2 ++ app/services/projects/transfer_service.rb | 2 ++ ...ab-37462-cache-personal-projects-count.yml | 5 +++++ spec/models/user_spec.rb | 20 ++++++++++++++---- spec/services/projects/create_service_spec.rb | 8 +++++++ .../services/projects/destroy_service_spec.rb | 6 ++++++ .../projects/transfer_service_spec.rb | 6 ++++++ 9 files changed, 64 insertions(+), 8 deletions(-) create mode 100644 changelogs/unreleased/ab-37462-cache-personal-projects-count.yml diff --git a/app/models/user.rb b/app/models/user.rb index ce56b39b1c8..a14aefc61d2 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -700,10 +700,6 @@ class User < ActiveRecord::Base projects_limit - personal_projects_count end - def personal_projects_count - @personal_projects_count ||= personal_projects.count - end - def recent_push(project = nil) service = Users::LastPushEventService.new(self) @@ -1046,6 +1042,18 @@ class User < ActiveRecord::Base end end + def personal_projects_count(force: false) + Rails.cache.fetch(['users', id, 'personal_projects_count'], force: force, expires_in: 24.hours, raw: true) do + personal_projects.count + end.to_i + end + + def update_cache_counts + assigned_open_merge_requests_count(force: true) + assigned_open_issues_count(force: true) + personal_projects_count(force: true) + end + def update_todos_count_cache todos_done_count(force: true) todos_pending_count(force: true) @@ -1056,6 +1064,7 @@ class User < ActiveRecord::Base invalidate_merge_request_cache_counts invalidate_todos_done_count invalidate_todos_pending_count + invalidate_personal_projects_count end def invalidate_issue_cache_counts @@ -1074,6 +1083,10 @@ class User < ActiveRecord::Base Rails.cache.delete(['users', id, 'todos_pending_count']) end + def invalidate_personal_projects_count + Rails.cache.delete(['users', id, 'personal_projects_count']) + end + # This is copied from Devise::Models::Lockable#valid_for_authentication?, as our auth # flow means we don't call that automatically (and can't conveniently do so). # diff --git a/app/services/projects/create_service.rb b/app/services/projects/create_service.rb index 633e2c8236c..d361d070993 100644 --- a/app/services/projects/create_service.rb +++ b/app/services/projects/create_service.rb @@ -96,6 +96,8 @@ module Projects system_hook_service.execute_hooks_for(@project, :create) setup_authorizations + + current_user.invalidate_personal_projects_count end # Refresh the current user's authorizations inline (so they can access the diff --git a/app/services/projects/destroy_service.rb b/app/services/projects/destroy_service.rb index 4b8f955ae69..114762c208e 100644 --- a/app/services/projects/destroy_service.rb +++ b/app/services/projects/destroy_service.rb @@ -34,6 +34,8 @@ module Projects system_hook_service.execute_hooks_for(project, :destroy) log_info("Project \"#{project.full_path}\" was removed") + current_user.invalidate_personal_projects_count + true rescue => error attempt_rollback(project, error.message) diff --git a/app/services/projects/transfer_service.rb b/app/services/projects/transfer_service.rb index 26765e5c3f3..5a23f0f0a62 100644 --- a/app/services/projects/transfer_service.rb +++ b/app/services/projects/transfer_service.rb @@ -24,6 +24,8 @@ module Projects transfer(project) + current_user.invalidate_personal_projects_count + true rescue Projects::TransferService::TransferError => ex project.reload diff --git a/changelogs/unreleased/ab-37462-cache-personal-projects-count.yml b/changelogs/unreleased/ab-37462-cache-personal-projects-count.yml new file mode 100644 index 00000000000..55069b1f2d2 --- /dev/null +++ b/changelogs/unreleased/ab-37462-cache-personal-projects-count.yml @@ -0,0 +1,5 @@ +--- +title: Cache personal projects count. +merge_request: 18197 +author: +type: performance diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index a600987d0bf..73266c0085f 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -2234,6 +2234,20 @@ describe User do end end + context '#invalidate_personal_projects_count' do + let(:user) { build_stubbed(:user) } + + it 'invalidates cache for personal projects counter' do + cache_mock = double + + expect(cache_mock).to receive(:delete).with(['users', user.id, 'personal_projects_count']) + + allow(Rails).to receive(:cache).and_return(cache_mock) + + user.invalidate_personal_projects_count + end + end + describe '#allow_password_authentication_for_web?' do context 'regular user' do let(:user) { build(:user) } @@ -2283,11 +2297,9 @@ describe User do user = build(:user) projects = double(:projects, count: 1) - expect(user).to receive(:personal_projects).once.and_return(projects) + expect(user).to receive(:personal_projects).and_return(projects) - 2.times do - expect(user.personal_projects_count).to eq(1) - end + expect(user.personal_projects_count).to eq(1) end end diff --git a/spec/services/projects/create_service_spec.rb b/spec/services/projects/create_service_spec.rb index 2cacb97a293..e35f0f6337a 100644 --- a/spec/services/projects/create_service_spec.rb +++ b/spec/services/projects/create_service_spec.rb @@ -28,6 +28,14 @@ describe Projects::CreateService, '#execute' do end end + describe 'after create actions' do + it 'invalidate personal_projects_count caches' do + expect(user).to receive(:invalidate_personal_projects_count) + + create_project(user, opts) + end + end + context "admin creates project with other user's namespace_id" do it 'sets the correct permissions' do admin = create(:admin) diff --git a/spec/services/projects/destroy_service_spec.rb b/spec/services/projects/destroy_service_spec.rb index 0bec2054f50..9bb1cda565c 100644 --- a/spec/services/projects/destroy_service_spec.rb +++ b/spec/services/projects/destroy_service_spec.rb @@ -66,6 +66,12 @@ describe Projects::DestroyService do end it_behaves_like 'deleting the project' + + it 'invalidates personal_project_count cache' do + expect(user).to receive(:invalidate_personal_projects_count) + + destroy_project(project, user) + end end context 'Sidekiq fake' do diff --git a/spec/services/projects/transfer_service_spec.rb b/spec/services/projects/transfer_service_spec.rb index 95a6771c59d..ff9b2372a35 100644 --- a/spec/services/projects/transfer_service_spec.rb +++ b/spec/services/projects/transfer_service_spec.rb @@ -37,6 +37,12 @@ describe Projects::TransferService do transfer_project(project, user, group) end + it 'invalidates the user\'s personal_project_count cache' do + expect(user).to receive(:invalidate_personal_projects_count) + + transfer_project(project, user, group) + end + it 'executes system hooks' do transfer_project(project, user, group) do |service| expect(service).to receive(:execute_system_hooks)