diff options
author | Rémy Coutable <remy@rymai.me> | 2017-07-04 16:15:24 +0200 |
---|---|---|
committer | Rémy Coutable <remy@rymai.me> | 2017-07-06 11:18:26 +0200 |
commit | 00ac76cc4ce87954d770abae411c54eb8bf23360 (patch) | |
tree | e9727ab4149ecd6ece8e3ebe692998e53821b3c8 | |
parent | 75bd0c3a444cb58f1a7ed5f0c540dddbde6f09ed (diff) | |
download | gitlab-ce-00ac76cc4ce87954d770abae411c54eb8bf23360.tar.gz |
Cache the allowed user IDs for the performance bar, in Redis for 10 minutes
Signed-off-by: Rémy Coutable <remy@rymai.me>
-rw-r--r-- | lib/gitlab/performance_bar.rb | 52 | ||||
-rw-r--r-- | spec/lib/gitlab/performance_bar_spec.rb | 52 |
2 files changed, 63 insertions, 41 deletions
diff --git a/lib/gitlab/performance_bar.rb b/lib/gitlab/performance_bar.rb index 60c8ba5063e..85f4371ec23 100644 --- a/lib/gitlab/performance_bar.rb +++ b/lib/gitlab/performance_bar.rb @@ -1,35 +1,53 @@ module Gitlab module PerformanceBar + ALLOWED_USER_IDS_KEY = 'performance_bar_allowed_user_ids'.freeze + # The time (in seconds) after which a set of allowed user IDs is expired + # automatically. + ALLOWED_USER_IDS_TIME_TO_LIVE = 10.minutes.to_i + def self.enabled?(current_user = nil) Feature.enabled?(:gitlab_performance_bar, current_user) end def self.allowed_user?(user) - return false unless allowed_group + return false unless allowed_group_name - if RequestStore.active? - RequestStore.fetch('performance_bar:user_member_of_allowed_group') do - user_member_of_allowed_group?(user) - end - else - user_member_of_allowed_group?(user) - end + allowed_user_ids.include?(user.id) end - def self.allowed_group - return nil unless Gitlab.config.performance_bar.allowed_group + def self.allowed_group_name + Gitlab.config.performance_bar.allowed_group + end + + def self.allowed_user_ids + Gitlab::Redis.with do |redis| + if redis.exists(cache_key) + redis.smembers(cache_key).map(&:to_i) + else + group = Group.find_by_full_path(allowed_group_name) + # Redis#sadd doesn't accept an empty array, but we still want to use + # Redis to let us know that no users are allowed, so we set the + # array to [-1] in this case. + user_ids = + if group + GroupMembersFinder.new(group).execute + .pluck(:user_id).presence || [-1] + else + [-1] + end + + redis.multi do + redis.sadd(cache_key, user_ids) + redis.expire(cache_key, ALLOWED_USER_IDS_TIME_TO_LIVE) + end - if RequestStore.active? - RequestStore.fetch('performance_bar:allowed_group') do - Group.find_by_full_path(Gitlab.config.performance_bar.allowed_group) + user_ids end - else - Group.find_by_full_path(Gitlab.config.performance_bar.allowed_group) end end - def self.user_member_of_allowed_group?(user) - GroupMembersFinder.new(allowed_group).execute.exists?(user_id: user.id) + def self.cache_key + "#{ALLOWED_USER_IDS_KEY}:#{allowed_group_name}" end end end diff --git a/spec/lib/gitlab/performance_bar_spec.rb b/spec/lib/gitlab/performance_bar_spec.rb index cab267cde30..ecdecc03304 100644 --- a/spec/lib/gitlab/performance_bar_spec.rb +++ b/spec/lib/gitlab/performance_bar_spec.rb @@ -25,6 +25,27 @@ describe Gitlab::PerformanceBar do end end + shared_examples 'allowed user IDs are cached in Redis for 10 minutes' do + before do + # Warm the Redis cache + described_class.allowed_user?(user) + end + + it 'caches the allowed user IDs in Redis', :redis do + expect do + expect(described_class.allowed_user?(user)).to be_truthy + end.not_to exceed_query_limit(0) + end + + it 'caches the allowed user IDs for 10 minutes', :redis do + ttl_cached_user_ids = Gitlab::Redis.with do |redis| + redis.ttl(described_class.cache_key) + end + + expect(ttl_cached_user_ids).to be <= 10.minutes + end + end + describe '.allowed_user?' do let(:user) { create(:user) } @@ -45,6 +66,8 @@ describe Gitlab::PerformanceBar do it 'returns false' do expect(described_class.allowed_user?(user)).to be_falsy end + + it_behaves_like 'allowed user IDs are cached in Redis for 10 minutes' end context 'when user is a member of the allowed group' do @@ -55,28 +78,8 @@ describe Gitlab::PerformanceBar do it 'returns true' do expect(described_class.allowed_user?(user)).to be_truthy end - end - end - end - describe '.allowed_group' do - before do - stub_performance_bar_setting(allowed_group: 'my-group') - end - - context 'when allowed group does not exist' do - it 'returns false' do - expect(described_class.allowed_group).to be_falsy - end - end - - context 'when allowed group exists' do - let!(:my_group) { create(:group, path: 'my-group') } - - context 'when user is not a member of the allowed group' do - it 'returns the group' do - expect(described_class.allowed_group).to eq(my_group) - end + it_behaves_like 'allowed user IDs are cached in Redis for 10 minutes' end end @@ -85,21 +88,22 @@ describe Gitlab::PerformanceBar do before do create(:group, path: 'my-group') + nested_my_group.add_developer(user) stub_performance_bar_setting(allowed_group: 'my-org/my-group') end it 'returns the nested group' do - expect(described_class.allowed_group).to eq(nested_my_group) + expect(described_class.allowed_user?(user)).to be_truthy end end context 'when a nested group has the same path', :nested_groups do before do - create(:group, :nested, path: 'my-group') + create(:group, :nested, path: 'my-group').add_developer(user) end it 'returns false' do - expect(described_class.allowed_group).to be_falsy + expect(described_class.allowed_user?(user)).to be_falsy end end end |