diff options
author | Lin Jen-Shin <godfat@godfat.org> | 2017-07-17 18:24:46 +0300 |
---|---|---|
committer | Lin Jen-Shin <godfat@godfat.org> | 2017-07-17 18:24:46 +0300 |
commit | 143fc48abac6e278dcda9be4b90cb3ca1291f4d9 (patch) | |
tree | 4792cf5eff9665810c52d27507f64ea17465527a /spec/lib/gitlab/cache | |
parent | 05329d4a364a5c55f2de9546871de1909b6be3f5 (diff) |
Add RequestStoreWrap to cache via RequestStore
I don't like the idea of `RequestStore` at all, because it's just a
global state which shouldn't be used at all. But we have a number of
places calling `ProtectedBranch.protected?` and `ProtectedTag.protected?`
in a loop for the same user, project, and ref whenever we're checking
against if the jobs for a given pipeline is accessible for a given user.
This means we're effectively making N queries for the same thing over
and over.
To properly fix this, we need to change how we check the permission,
and that could be a huge work. To solve this quickly, adding a cache
layer for the given request would be quite simple to do.
We're already doing this in Commit#author, and this is extending that
idea and make it generalized.
Diffstat (limited to 'spec/lib/gitlab/cache')
-rw-r--r-- | spec/lib/gitlab/cache/request_store_wrap_spec.rb | 113 |
1 files changed, 113 insertions, 0 deletions
diff --git a/spec/lib/gitlab/cache/request_store_wrap_spec.rb b/spec/lib/gitlab/cache/request_store_wrap_spec.rb new file mode 100644 index 00000000000..6a95239066b --- /dev/null +++ b/spec/lib/gitlab/cache/request_store_wrap_spec.rb @@ -0,0 +1,113 @@ +require 'spec_helper' + +describe Gitlab::Cache::RequestStoreWrap, :request_store do + let(:klass) do + Class.new do + extend Gitlab::Cache::RequestStoreWrap + + attr_accessor :id, :name, :result + + def self.name + 'ExpensiveAlgorithm' + end + + def initialize(id, name, result) + self.id = id + self.name = name + self.result = result + end + + request_store_wrap_key do + [id, name] + end + + request_store_wrap def compute(arg) + result << arg + end + + request_store_wrap def repute(arg) + result << arg + end + end + end + + let(:algorithm) { klass.new('id', 'name', []) } + + context 'when RequestStore is active' do + it 'does not compute twice for the same argument' do + result = algorithm.compute(true) + + expect(result).to eq([true]) + expect(algorithm.compute(true)).to eq(result) + expect(algorithm.result).to eq(result) + end + + it 'computes twice for the different argument' do + algorithm.compute(true) + result = algorithm.compute(false) + + expect(result).to eq([true, false]) + 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') + result = algorithm.compute(true) + + expect(result).to eq([true, true]) + expect(algorithm.result).to eq(result) + end + + it 'computes twice for the different method' do + algorithm.compute(true) + result = algorithm.repute(true) + + expect(result).to eq([true, true]) + expect(algorithm.result).to eq(result) + 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]) + expect(algorithm.result).to eq(result) + end + end + + context 'when RequestStore is inactive' do + before do + RequestStore.end! + end + + it 'computes twice even if everything is the same' do + algorithm.compute(true) + result = algorithm.compute(true) + + expect(result).to eq([true, true]) + expect(algorithm.result).to eq(result) + end + end +end |