Commit 3e561736 authored by Yorick Peterse's avatar Yorick Peterse

Cache the number of user SSH keys

By caching the number of personal SSH keys we reduce the number of
queries necessary on pages such as ProjectsController#show (which can
end up querying this data multiple times).

The cache is refreshed/flushed whenever an SSH key is added, removed, or
when a user is removed.
parent 81e94ce1
...@@ -27,8 +27,10 @@ class Key < ActiveRecord::Base ...@@ -27,8 +27,10 @@ class Key < ActiveRecord::Base
after_commit :add_to_shell, on: :create after_commit :add_to_shell, on: :create
after_create :post_create_hook after_create :post_create_hook
after_create :refresh_user_cache
after_commit :remove_from_shell, on: :destroy after_commit :remove_from_shell, on: :destroy
after_destroy :post_destroy_hook after_destroy :post_destroy_hook
after_destroy :refresh_user_cache
def key=(value) def key=(value)
value&.delete!("\n\r") value&.delete!("\n\r")
...@@ -76,6 +78,12 @@ class Key < ActiveRecord::Base ...@@ -76,6 +78,12 @@ class Key < ActiveRecord::Base
) )
end end
def refresh_user_cache
return unless user
Users::KeysCountService.new(user).refresh_cache
end
def post_destroy_hook def post_destroy_hook
SystemHooksService.new.execute_hooks_for(self, :destroy) SystemHooksService.new.execute_hooks_for(self, :destroy)
end end
......
...@@ -170,6 +170,7 @@ class User < ActiveRecord::Base ...@@ -170,6 +170,7 @@ class User < ActiveRecord::Base
after_save :ensure_namespace_correct after_save :ensure_namespace_correct
after_update :username_changed_hook, if: :username_changed? after_update :username_changed_hook, if: :username_changed?
after_destroy :post_destroy_hook after_destroy :post_destroy_hook
after_destroy :remove_key_cache
after_commit :update_emails_with_primary_email, on: :update, if: -> { previous_changes.key?('email') } after_commit :update_emails_with_primary_email, on: :update, if: -> { previous_changes.key?('email') }
after_commit :update_invalid_gpg_signatures, on: :update, if: -> { previous_changes.key?('email') } after_commit :update_invalid_gpg_signatures, on: :update, if: -> { previous_changes.key?('email') }
...@@ -624,7 +625,9 @@ class User < ActiveRecord::Base ...@@ -624,7 +625,9 @@ class User < ActiveRecord::Base
end end
def require_ssh_key? def require_ssh_key?
keys.count == 0 && Gitlab::ProtocolAccess.allowed?('ssh') count = Users::KeysCountService.new(self).count
count.zero? && Gitlab::ProtocolAccess.allowed?('ssh')
end end
def require_password_creation? def require_password_creation?
...@@ -886,6 +889,10 @@ class User < ActiveRecord::Base ...@@ -886,6 +889,10 @@ class User < ActiveRecord::Base
system_hook_service.execute_hooks_for(self, :destroy) system_hook_service.execute_hooks_for(self, :destroy)
end end
def remove_key_cache
Users::KeysCountService.new(self).delete_cache
end
def delete_async(deleted_by:, params: {}) def delete_async(deleted_by:, params: {})
block if params[:hard_delete] block if params[:hard_delete]
DeleteUserWorker.perform_async(deleted_by.id, id, params) DeleteUserWorker.perform_async(deleted_by.id, id, params)
......
# Base class for services that count a single resource such as the number of
# issues for a project.
class BaseCountService
def relation_for_count
raise(
NotImplementedError,
'"relation_for_count" must be implemented and return an ActiveRecord::Relation'
)
end
def count
Rails.cache.fetch(cache_key, raw: raw?) { uncached_count }.to_i
end
def refresh_cache
Rails.cache.write(cache_key, uncached_count, raw: raw?)
end
def uncached_count
relation_for_count.count
end
def delete_cache
Rails.cache.delete(cache_key)
end
def raw?
false
end
def cache_key
raise NotImplementedError, 'cache_key must be implemented and return a String'
end
end
module Projects 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 < BaseCountService
# The version of the cache format. This should be bumped whenever the # The version of the cache format. This should be bumped whenever the
# underlying logic changes. This removes the need for explicitly flushing # underlying logic changes. This removes the need for explicitly flushing
# all caches. # all caches.
...@@ -11,29 +11,6 @@ module Projects ...@@ -11,29 +11,6 @@ module Projects
@project = project @project = project
end end
def relation_for_count
raise(
NotImplementedError,
'"relation_for_count" must be implemented and return an ActiveRecord::Relation'
)
end
def count
Rails.cache.fetch(cache_key) { uncached_count }
end
def refresh_cache
Rails.cache.write(cache_key, uncached_count)
end
def uncached_count
relation_for_count.count
end
def delete_cache
Rails.cache.delete(cache_key)
end
def cache_key_name def cache_key_name
raise( raise(
NotImplementedError, NotImplementedError,
......
module Projects module Projects
# Service class for getting and caching the number of forks of a project. # Service class for getting and caching the number of forks of a project.
class ForksCountService < CountService class ForksCountService < Projects::CountService
def relation_for_count def relation_for_count
@project.forks @project.forks
end end
......
module Projects module Projects
# Service class for counting and caching the number of open issues of a # Service class for counting and caching the number of open issues of a
# project. # project.
class OpenIssuesCountService < CountService class OpenIssuesCountService < Projects::CountService
def relation_for_count def relation_for_count
# We don't include confidential issues in this number since this would # We don't include confidential issues in this number since this would
# expose the number of confidential issues to non project members. # expose the number of confidential issues to non project members.
......
module Projects module Projects
# Service class for counting and caching the number of open merge requests of # Service class for counting and caching the number of open merge requests of
# a project. # a project.
class OpenMergeRequestsCountService < CountService class OpenMergeRequestsCountService < Projects::CountService
def relation_for_count def relation_for_count
@project.merge_requests.opened @project.merge_requests.opened
end end
......
# frozen_string_literal: true
module Users
# Service class for getting the number of SSH keys that belong to a user.
class KeysCountService < BaseCountService
attr_reader :user
# user - The User for which to get the number of SSH keys.
def initialize(user)
@user = user
end
def relation_for_count
user.keys
end
def raw?
# Since we're storing simple integers we don't need all of the additional
# Marshal data Rails includes by default.
true
end
def cache_key
"users/key-count-service/#{user.id}"
end
end
end
---
title: Cache the number of user SSH keys
merge_request:
author:
type: performance
...@@ -166,4 +166,27 @@ describe Key, :mailer do ...@@ -166,4 +166,27 @@ describe Key, :mailer do
expect(key.public_key.key_text).to eq(valid_key) expect(key.public_key.key_text).to eq(valid_key)
end end
end end
describe '#refresh_user_cache', :use_clean_rails_memory_store_caching do
context 'when the key belongs to a user' do
it 'refreshes the keys count cache for the user' do
expect_any_instance_of(Users::KeysCountService)
.to receive(:refresh_cache)
.and_call_original
key = create(:personal_key)
expect(Users::KeysCountService.new(key.user).count).to eq(1)
end
end
context 'when the key does not belong to a user' do
it 'does nothing' do
expect_any_instance_of(Users::KeysCountService)
.not_to receive(:refresh_cache)
create(:key)
end
end
end
end end
...@@ -828,7 +828,7 @@ describe User do ...@@ -828,7 +828,7 @@ describe User do
end end
end end
describe '#require_ssh_key?' do describe '#require_ssh_key?', :use_clean_rails_memory_store_caching do
protocol_and_expectation = { protocol_and_expectation = {
'http' => false, 'http' => false,
'ssh' => true, 'ssh' => true,
...@@ -843,6 +843,12 @@ describe User do ...@@ -843,6 +843,12 @@ describe User do
expect(user.require_ssh_key?).to eq(expected) expect(user.require_ssh_key?).to eq(expected)
end end
end end
it 'returns false when the user has 1 or more SSH keys' do
key = create(:personal_key)
expect(key.user.require_ssh_key?).to eq(false)
end
end end
end end
......
require 'spec_helper'
describe BaseCountService, :use_clean_rails_memory_store_caching do
let(:service) { described_class.new }
describe '#relation_for_count' do
it 'raises NotImplementedError' do
expect { service.relation_for_count }.to raise_error(NotImplementedError)
end
end
describe '#count' do
it 'returns the number of values' do
expect(service)
.to receive(:cache_key)
.and_return('foo')
expect(service)
.to receive(:uncached_count)
.and_return(5)
expect(service.count).to eq(5)
end
end
describe '#uncached_count' do
it 'returns the uncached number of values' do
expect(service)
.to receive(:relation_for_count)
.and_return(double(:relation, count: 5))
expect(service.uncached_count).to eq(5)
end
end
describe '#refresh_cache' do
it 'refreshes the cache' do
allow(service)
.to receive(:cache_key)
.and_return('foo')
allow(service)
.to receive(:uncached_count)
.and_return(4)
service.refresh_cache
expect(Rails.cache.fetch(service.cache_key, raw: service.raw?)).to eq(4)
end
end
describe '#delete_cache' do
it 'deletes the cache' do
allow(service)
.to receive(:cache_key)
.and_return('foo')
allow(service)
.to receive(:uncached_count)
.and_return(4)
service.refresh_cache
service.delete_cache
expect(Rails.cache.fetch(service.cache_key, raw: service.raw?)).to be_nil
end
end
describe '#raw?' do
it 'returns false' do
expect(service.raw?).to eq(false)
end
end
describe '#cache_key' do
it 'raises NotImplementedError' do
expect { service.cache_key }.to raise_error(NotImplementedError)
end
end
end
# frozen_string_literal: true
require 'spec_helper'
describe Users::KeysCountService, :use_clean_rails_memory_store_caching do
let(:user) { create(:user) }
let(:service) { described_class.new(user) }
describe '#count' do
before do
create(:personal_key, user: user)
end
it 'returns the number of SSH keys as an Integer' do
expect(service.count).to eq(1)
end
it 'caches the number of keys in Redis' do
service.delete_cache
recorder = ActiveRecord::QueryRecorder.new do
2.times { service.count }
end
expect(recorder.count).to eq(1)
end
end
describe '#refresh_cache' do
it 'refreshes the Redis cache' do
Rails.cache.write(service.cache_key, 10)
service.refresh_cache
expect(Rails.cache.fetch(service.cache_key, raw: true)).to be_zero
end
end
describe '#delete_cache' do
it 'removes the cache' do
service.count
service.delete_cache
expect(Rails.cache.fetch(service.cache_key, raw: true)).to be_nil
end
end
describe '#uncached_count' do
it 'returns the number of SSH keys' do
expect(service.uncached_count).to be_zero
end
it 'does not cache the number of keys' do
recorder = ActiveRecord::QueryRecorder.new do
2.times { service.uncached_count }
end
expect(recorder.count).to be > 0
end
end
describe '#cache_key' do
it 'returns the cache key' do
expect(service.cache_key).to eq("users/key-count-service/#{user.id}")
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