diff options
author | Lin Jen-Shin <godfat@godfat.org> | 2017-07-18 01:18:20 +0800 |
---|---|---|
committer | Lin Jen-Shin <godfat@godfat.org> | 2017-07-18 01:18:20 +0800 |
commit | aada5273fa260cbd2441e8f1a0c95d951b4977fc (patch) | |
tree | 756f82f4492659adc61ee47f12b9c6a780cb5077 | |
parent | 143fc48abac6e278dcda9be4b90cb3ca1291f4d9 (diff) | |
download | gitlab-ce-aada5273fa260cbd2441e8f1a0c95d951b4977fc.tar.gz |
Use RequestStoreWrap for Commit#author
We also try to use instance variable to cache the result if
RequestStore is not available, so we could keep the same logic,
using the same cache key. Also introduce a way to specify method
specific cache key
-rw-r--r-- | app/models/commit.rb | 19 | ||||
-rw-r--r-- | lib/gitlab/cache/request_store_wrap.rb | 46 | ||||
-rw-r--r-- | spec/lib/gitlab/cache/request_store_wrap_spec.rb | 78 | ||||
-rw-r--r-- | spec/models/commit_spec.rb | 8 |
4 files changed, 93 insertions, 58 deletions
diff --git a/app/models/commit.rb b/app/models/commit.rb index c7f62617c4c..337236b30d5 100644 --- a/app/models/commit.rb +++ b/app/models/commit.rb @@ -1,5 +1,6 @@ class Commit extend ActiveModel::Naming + extend Gitlab::Cache::RequestStoreWrap 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_store_wrap(: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/lib/gitlab/cache/request_store_wrap.rb b/lib/gitlab/cache/request_store_wrap.rb index 3e0a5f06b53..1cb442444bb 100644 --- a/lib/gitlab/cache/request_store_wrap.rb +++ b/lib/gitlab/cache/request_store_wrap.rb @@ -37,23 +37,45 @@ module Gitlab end end - def request_store_wrap(method_name) - const_get(:RequestStoreWrapExtension) - .send(:define_method, method_name) do |*args| - return super(*args) unless RequestStore.active? + def request_store_wrap(method_name, &method_key_block) + const_get(:RequestStoreWrapExtension).module_eval do + define_method(method_name) do |*args| + store = + if RequestStore.active? + RequestStore.store + else + ivar_name = # ! and ? cannot be used as ivar name + "@#{method_name.to_s.tr('!', "\u2605").tr('?', "\u2606")}" - klass = self.class - key = [klass.name, - method_name, - *instance_exec(&klass.request_store_wrap_key), - *args].join(':') + instance_variable_get(ivar_name) || + instance_variable_set(ivar_name, {}) + end + + key = send("#{method_name}_cache_key", args) - if RequestStore.store.key?(key) - RequestStore.store[key] + if store.key?(key) + store[key] else - RequestStore.store[key] = super(*args) + store[key] = super(*args) end end + + cache_key_method_name = "#{method_name}_cache_key" + + define_method(cache_key_method_name) do |args| + klass = self.class + + instance_key = instance_exec(&klass.request_store_wrap_key) if + klass.request_store_wrap_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 diff --git a/spec/lib/gitlab/cache/request_store_wrap_spec.rb b/spec/lib/gitlab/cache/request_store_wrap_spec.rb index 6a95239066b..d63d958900a 100644 --- a/spec/lib/gitlab/cache/request_store_wrap_spec.rb +++ b/spec/lib/gitlab/cache/request_store_wrap_spec.rb @@ -5,20 +5,17 @@ describe Gitlab::Cache::RequestStoreWrap, :request_store do Class.new do extend Gitlab::Cache::RequestStoreWrap - attr_accessor :id, :name, :result + attr_accessor :id, :name, :result, :extra def self.name 'ExpensiveAlgorithm' end - def initialize(id, name, result) + def initialize(id, name, result, extra = nil) self.id = id self.name = name self.result = result - end - - request_store_wrap_key do - [id, name] + self.extra = nil end request_store_wrap def compute(arg) @@ -28,6 +25,11 @@ describe Gitlab::Cache::RequestStoreWrap, :request_store do request_store_wrap def repute(arg) result << arg end + + def dispute(arg) + result << arg + end + request_store_wrap(:dispute) { extra } end end @@ -50,24 +52,6 @@ describe Gitlab::Cache::RequestStoreWrap, :request_store do expect(algorithm.result).to eq(result) 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]) - expect(algorithm.result).to eq(result) - 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]) - expect(algorithm.result).to eq(result) - end - it 'computes twice for the different class name' do algorithm.compute(true) allow(klass).to receive(:name).and_return('CheapAlgo') @@ -95,6 +79,42 @@ describe Gitlab::Cache::RequestStoreWrap, :request_store do expect(result).to eq([true, true]) expect(algorithm.result).to eq(result) end + + context 'when request_store_wrap_key is provided' do + before do + klass.request_store_wrap_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]) + expect(algorithm.result).to eq(result) + 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]) + expect(algorithm.result).to eq(result) + 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]) + expect(algorithm.result).to eq(result) + end + end end context 'when RequestStore is inactive' do @@ -102,10 +122,18 @@ describe Gitlab::Cache::RequestStoreWrap, :request_store do RequestStore.end! end - it 'computes twice even if everything is the same' do + it 'computes only once if it is the same instance for the same key' do algorithm.compute(true) result = algorithm.compute(true) + expect(result).to eq([true]) + expect(algorithm.result).to eq(result) + end + + 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]) expect(algorithm.result).to eq(result) 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 |