diff options
author | Rémy Coutable <remy@rymai.me> | 2017-07-18 12:09:47 +0000 |
---|---|---|
committer | Rémy Coutable <remy@rymai.me> | 2017-07-18 12:09:47 +0000 |
commit | f3e682c03fa84adea99d55ac74e32d53164cd99b (patch) | |
tree | 695c197151fe0ee2330db86328440fa6b390a49d | |
parent | f48264555563a906472795bc9fbccd09be4b6a47 (diff) | |
parent | f69c0f801f3db8cc534bc9652fbdb01bb15a5340 (diff) | |
download | gitlab-ce-f3e682c03fa84adea99d55ac74e32d53164cd99b.tar.gz |
Merge branch 'request-store-wrap' into 'master'
Add RequestCache to cache via RequestStore
See merge request !12920
-rw-r--r-- | app/models/commit.rb | 19 | ||||
-rw-r--r-- | app/models/note.rb | 2 | ||||
-rw-r--r-- | changelogs/unreleased/request-store-wrap.yml | 4 | ||||
-rw-r--r-- | lib/gitlab/cache/request_cache.rb | 94 | ||||
-rw-r--r-- | lib/gitlab/user_access.rb | 14 | ||||
-rw-r--r-- | spec/factories/commits.rb | 9 | ||||
-rw-r--r-- | spec/features/participants_autocomplete_spec.rb | 3 | ||||
-rw-r--r-- | spec/lib/gitlab/cache/request_cache_spec.rb | 133 | ||||
-rw-r--r-- | spec/models/commit_spec.rb | 8 | ||||
-rw-r--r-- | spec/services/notification_service_spec.rb | 2 |
10 files changed, 258 insertions, 30 deletions
diff --git a/app/models/commit.rb b/app/models/commit.rb index 21b906e1110..1e19f00106a 100644 --- a/app/models/commit.rb +++ b/app/models/commit.rb @@ -1,5 +1,6 @@ class Commit extend ActiveModel::Naming + extend Gitlab::Cache::RequestCache include ActiveModel::Conversion include Noteable @@ -169,19 +170,9 @@ class Commit end def author - if RequestStore.active? - key = "commit_author:#{author_email.downcase}" - # nil is a valid value since no author may exist in the system - if RequestStore.store.key?(key) - @author = RequestStore.store[key] - else - @author = find_author_by_any_email - RequestStore.store[key] = @author - end - else - @author ||= find_author_by_any_email - end + User.find_by_any_email(author_email.downcase) end + request_cache(:author) { author_email.downcase } def committer @committer ||= User.find_by_any_email(committer_email.downcase) @@ -368,10 +359,6 @@ class Commit end end - def find_author_by_any_email - User.find_by_any_email(author_email.downcase) - end - def repo_changes changes = { added: [], modified: [], removed: [] } diff --git a/app/models/note.rb b/app/models/note.rb index 3d39047d32f..d0e3bc0bfed 100644 --- a/app/models/note.rb +++ b/app/models/note.rb @@ -190,7 +190,7 @@ class Note < ActiveRecord::Base # override to return commits, which are not active record def noteable if for_commit? - project.commit(commit_id) + @commit ||= project.commit(commit_id) else super end diff --git a/changelogs/unreleased/request-store-wrap.yml b/changelogs/unreleased/request-store-wrap.yml new file mode 100644 index 00000000000..8017054b77b --- /dev/null +++ b/changelogs/unreleased/request-store-wrap.yml @@ -0,0 +1,4 @@ +--- +title: Add RequestCache which makes caching with RequestStore easier +merge_request: 12920 +author: diff --git a/lib/gitlab/cache/request_cache.rb b/lib/gitlab/cache/request_cache.rb new file mode 100644 index 00000000000..f1a04affd38 --- /dev/null +++ b/lib/gitlab/cache/request_cache.rb @@ -0,0 +1,94 @@ +module Gitlab + module Cache + # This module provides a simple way to cache values in RequestStore, + # and the cache key would be based on the class name, method name, + # optionally customized instance level values, optionally customized + # method level values, and optional method arguments. + # + # A simple example: + # + # class UserAccess + # extend Gitlab::Cache::RequestCache + # + # request_cache_key do + # [user&.id, project&.id] + # end + # + # request_cache def can_push_to_branch?(ref) + # # ... + # end + # end + # + # This way, the result of `can_push_to_branch?` would be cached in + # `RequestStore.store` based on the cache key. If RequestStore is not + # currently active, then it would be stored in a hash saved in an + # instance variable, so the cache logic would be the same. + # Here's another example using customized method level values: + # + # class Commit + # extend Gitlab::Cache::RequestCache + # + # def author + # User.find_by_any_email(author_email.downcase) + # end + # request_cache(:author) { author_email.downcase } + # end + # + # So that we could have different strategies for different methods + # + module RequestCache + def self.extended(klass) + return if klass < self + + extension = Module.new + klass.const_set(:RequestCacheExtension, extension) + klass.prepend(extension) + end + + def request_cache_key(&block) + if block_given? + @request_cache_key = block + else + @request_cache_key + end + end + + def request_cache(method_name, &method_key_block) + const_get(:RequestCacheExtension).module_eval do + cache_key_method_name = "#{method_name}_cache_key" + + define_method(method_name) do |*args| + store = + if RequestStore.active? + RequestStore.store + else + ivar_name = # ! and ? cannot be used as ivar name + "@cache_#{method_name.to_s.tr('!?', "\u2605\u2606")}" + + instance_variable_get(ivar_name) || + instance_variable_set(ivar_name, {}) + end + + key = __send__(cache_key_method_name, args) + + store.fetch(key) { store[key] = super(*args) } + end + + define_method(cache_key_method_name) do |args| + klass = self.class + + instance_key = instance_exec(&klass.request_cache_key) if + klass.request_cache_key + + method_key = instance_exec(&method_key_block) if method_key_block + + [klass.name, method_name, *instance_key, *method_key, *args] + .join(':') + end + + private cache_key_method_name + end + end + end + end +end diff --git a/lib/gitlab/user_access.rb b/lib/gitlab/user_access.rb index 3b922da7ced..8e91ee7287c 100644 --- a/lib/gitlab/user_access.rb +++ b/lib/gitlab/user_access.rb @@ -1,5 +1,11 @@ module Gitlab class UserAccess + extend Gitlab::Cache::RequestCache + + request_cache_key do + [user&.id, project&.id] + end + attr_reader :user, :project def initialize(user, project: nil) @@ -28,7 +34,7 @@ module Gitlab true end - def can_create_tag?(ref) + request_cache def can_create_tag?(ref) return false unless can_access_git? if ProtectedTag.protected?(project, ref) @@ -38,7 +44,7 @@ module Gitlab end end - def can_delete_branch?(ref) + request_cache def can_delete_branch?(ref) return false unless can_access_git? if ProtectedBranch.protected?(project, ref) @@ -48,7 +54,7 @@ module Gitlab end end - def can_push_to_branch?(ref) + request_cache def can_push_to_branch?(ref) return false unless can_access_git? if ProtectedBranch.protected?(project, ref) @@ -60,7 +66,7 @@ module Gitlab end end - def can_merge_to_branch?(ref) + request_cache def can_merge_to_branch?(ref) return false unless can_access_git? if ProtectedBranch.protected?(project, ref) diff --git a/spec/factories/commits.rb b/spec/factories/commits.rb index 36b9645438a..89e260cf65b 100644 --- a/spec/factories/commits.rb +++ b/spec/factories/commits.rb @@ -4,14 +4,19 @@ FactoryGirl.define do factory :commit do git_commit RepoHelpers.sample_commit project factory: :empty_project - author { build(:author) } initialize_with do new(git_commit, project) end + after(:build) do |commit| + allow(commit).to receive(:author).and_return build(:author) + end + trait :without_author do - author nil + after(:build) do |commit| + allow(commit).to receive(:author).and_return nil + end end end end diff --git a/spec/features/participants_autocomplete_spec.rb b/spec/features/participants_autocomplete_spec.rb index 382d83ca051..81b0a2f541b 100644 --- a/spec/features/participants_autocomplete_spec.rb +++ b/spec/features/participants_autocomplete_spec.rb @@ -54,7 +54,8 @@ feature 'Member autocomplete', :js do let(:note) { create(:note_on_commit, project: project, commit_id: project.commit.id) } before do - allow_any_instance_of(Commit).to receive(:author).and_return(author) + allow(User).to receive(:find_by_any_email) + .with(noteable.author_email.downcase).and_return(author) visit project_commit_path(project, noteable) end diff --git a/spec/lib/gitlab/cache/request_cache_spec.rb b/spec/lib/gitlab/cache/request_cache_spec.rb new file mode 100644 index 00000000000..5b82c216a13 --- /dev/null +++ b/spec/lib/gitlab/cache/request_cache_spec.rb @@ -0,0 +1,133 @@ +require 'spec_helper' + +describe Gitlab::Cache::RequestCache do + let(:klass) do + Class.new do + extend Gitlab::Cache::RequestCache + + attr_accessor :id, :name, :result, :extra + + def self.name + 'ExpensiveAlgorithm' + end + + def initialize(id, name, result, extra = nil) + self.id = id + self.name = name + self.result = result + self.extra = nil + end + + request_cache def compute(arg) + result << arg + end + + request_cache def repute(arg) + result << arg + end + + def dispute(arg) + result << arg + end + request_cache(:dispute) { extra } + end + end + + let(:algorithm) { klass.new('id', 'name', []) } + + shared_examples 'cache for the same instance' do + it 'does not compute twice for the same argument' do + algorithm.compute(true) + result = algorithm.compute(true) + + expect(result).to eq([true]) + end + + it 'computes twice for the different argument' do + algorithm.compute(true) + result = algorithm.compute(false) + + expect(result).to eq([true, false]) + end + + it 'computes twice for the different class name' do + algorithm.compute(true) + allow(klass).to receive(:name).and_return('CheapAlgo') + result = algorithm.compute(true) + + expect(result).to eq([true, true]) + end + + it 'computes twice for the different method' do + algorithm.compute(true) + result = algorithm.repute(true) + + expect(result).to eq([true, true]) + end + + context 'when request_cache_key is provided' do + before do + klass.request_cache_key do + [id, name] + end + end + + it 'computes twice for the different keys, id' do + algorithm.compute(true) + algorithm.id = 'ad' + result = algorithm.compute(true) + + expect(result).to eq([true, true]) + end + + it 'computes twice for the different keys, name' do + algorithm.compute(true) + algorithm.name = 'same' + result = algorithm.compute(true) + + expect(result).to eq([true, true]) + end + + it 'uses extra method cache key if provided' do + algorithm.dispute(true) # miss + algorithm.extra = true + algorithm.dispute(true) # miss + result = algorithm.dispute(true) # hit + + expect(result).to eq([true, true]) + end + end + end + + context 'when RequestStore is active', :request_store do + it_behaves_like 'cache for the same instance' + + it 'computes once for different instances when keys are the same' do + algorithm.compute(true) + result = klass.new('id', 'name', algorithm.result).compute(true) + + expect(result).to eq([true]) + end + + it 'computes twice if RequestStore starts over' do + algorithm.compute(true) + RequestStore.end! + RequestStore.clear! + RequestStore.begin! + result = algorithm.compute(true) + + expect(result).to eq([true, true]) + end + end + + context 'when RequestStore is inactive' do + it_behaves_like 'cache for the same instance' + + it 'computes twice for different instances even if keys are the same' do + algorithm.compute(true) + result = klass.new('id', 'name', algorithm.result).compute(true) + + expect(result).to eq([true, true]) + end + end +end diff --git a/spec/models/commit_spec.rb b/spec/models/commit_spec.rb index 6056d78da4e..528b211c9d6 100644 --- a/spec/models/commit_spec.rb +++ b/spec/models/commit_spec.rb @@ -19,17 +19,15 @@ describe Commit, models: true do expect(commit.author).to eq(user) end - it 'caches the author' do - allow(RequestStore).to receive(:active?).and_return(true) + it 'caches the author', :request_store do user = create(:user, email: commit.author_email) - expect_any_instance_of(Commit).to receive(:find_author_by_any_email).and_call_original + expect(User).to receive(:find_by_any_email).and_call_original expect(commit.author).to eq(user) - key = "commit_author:#{commit.author_email}" + key = "Commit:author:#{commit.author_email.downcase}" expect(RequestStore.store[key]).to eq(user) expect(commit.author).to eq(user) - RequestStore.store.clear end end diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb index f1e00c1163b..4fc5eb0a527 100644 --- a/spec/services/notification_service_spec.rb +++ b/spec/services/notification_service_spec.rb @@ -383,7 +383,7 @@ describe NotificationService, services: true do before do build_team(note.project) reset_delivered_emails! - allow_any_instance_of(Commit).to receive(:author).and_return(@u_committer) + allow(note.noteable).to receive(:author).and_return(@u_committer) update_custom_notification(:new_note, @u_guest_custom, resource: project) update_custom_notification(:new_note, @u_custom_global) end |