diff options
| -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 |