Skip to content
Snippets Groups Projects
Commit d4f5f229 authored by Vasilii Iakliushin's avatar Vasilii Iakliushin :two:
Browse files

Use cached values from `ProtectedBranch` cache

* Contributes to #366724
* Follow-up for:
!92934

* Feature flag: #368279

**Problem**

After testing new Protected branch cache with `dry_run: true` option, we
can start relying on it.

**Solution**

* Remove `dry_run` code
* Use `ProtectedBranch` cache if FF is on
parent c975f738
No related branches found
No related tags found
No related merge requests found
......@@ -37,7 +37,7 @@ def self.protected?(project, ref_name)
def self.new_cache(project, ref_name)
if Feature.enabled?(:hash_based_cache_for_protected_branches, project)
ProtectedBranches::CacheService.new(project).fetch(ref_name, dry_run: true) do # rubocop: disable CodeReuse/ServiceClass
ProtectedBranches::CacheService.new(project).fetch(ref_name) do # rubocop: disable CodeReuse/ServiceClass
self.matching(ref_name, protected_refs: protected_refs(project)).present?
end
end
......
......@@ -7,29 +7,16 @@ class CacheService < ProtectedBranches::BaseService
CACHE_EXPIRE_IN = 1.day
CACHE_LIMIT = 1000
def fetch(ref_name, dry_run: false)
def fetch(ref_name)
record = OpenSSL::Digest::SHA256.hexdigest(ref_name)
Gitlab::Redis::Cache.with do |redis|
cached_result = redis.hget(redis_key, record)
decoded_result = Gitlab::Redis::Boolean.decode(cached_result) unless cached_result.nil?
break decoded_result unless dry_run || decoded_result.nil?
break Gitlab::Redis::Boolean.decode(cached_result) unless cached_result.nil?
value = yield
if dry_run && !decoded_result.nil? && decoded_result != value
encoded_ref_name = Gitlab::EncodingHelper.encode_utf8_with_replacement_character(ref_name)
log_error(
'class' => self.class.name,
'message' => "Cache mismatch '#{encoded_ref_name}': cached value: #{decoded_result}, real value: #{value}",
'project_id' => @project.id,
'project_path' => @project.full_path
)
end
redis.hset(redis_key, record, Gitlab::Redis::Boolean.encode(value))
# If the cache record has too many elements, then something went wrong and
......
......@@ -7,7 +7,7 @@
# for example, adding quick actions when creating the issue and checking DateTime formats on UI.
# Because this kind of spec takes more time to run there is no need to add new ones
# for each existing quick action unless they test something not tested by existing tests.
RSpec.describe 'Merge request > User uses quick actions', :js do
RSpec.describe 'Merge request > User uses quick actions', :js, :clean_gitlab_redis_cache do
include Spec::Support::Helpers::Features::NotesHelpers
let(:project) { create(:project, :public, :repository) }
......
......@@ -167,7 +167,7 @@
expect(described_class.protected?(project, nil)).to eq(false)
end
context 'with caching', :use_clean_rails_memory_store_caching do
context 'with caching', :clean_gitlab_redis_cache do
let_it_be(:project) { create(:project, :repository) }
let_it_be(:protected_branch) { create(:protected_branch, project: project, name: "“jawn”") }
......@@ -195,8 +195,7 @@
end
context 'when project is updated' do
# Expected to fail, because of fetch(dry_run: true)
xit 'does not invalidate a cache' do
it 'does not invalidate a cache' do
expect(described_class).not_to receive(:matching).with(protected_branch.name, protected_refs: anything)
project.touch
......@@ -206,8 +205,7 @@
end
context 'when other project protected branch is updated' do
# Expected to fail, because of fetch(dry_run: true)
xit 'does not invalidate the current project cache' do
it 'does not invalidate the current project cache' do
expect(described_class).not_to receive(:matching).with(protected_branch.name, protected_refs: anything)
another_project = create(:project)
......@@ -217,13 +215,12 @@
end
end
# Expected to fail, because of fetch(dry_run: true)
xit 'correctly uses the cached version' do
it 'correctly uses the cached version' do
expect(described_class).not_to receive(:matching)
expect(described_class.protected?(project, protected_branch.name)).to eq(true)
end
context 'when feature flag is off' do
context 'when feature flag is off', :use_clean_rails_memory_store_caching do
let(:feature_flag) { false }
it 'correctly invalidates a cache' do
......
......@@ -193,7 +193,7 @@
end
end
describe "Webhooks" do
describe "Webhooks", :clean_gitlab_redis_cache do
before do
create(:project_hook, push_events: true, project: project)
end
......
......@@ -61,38 +61,6 @@
expect(service.fetch('not-found') { true }).to eq(true)
end
end
context 'when dry_run is on' do
it 'does not use cached value' do
expect(service.fetch('main', dry_run: true) { true }).to eq(true)
expect(service.fetch('main', dry_run: true) { false }).to eq(false)
end
context 'when cache mismatch' do
it 'logs an error' do
expect(service.fetch('main', dry_run: true) { true }).to eq(true)
expect(Gitlab::AppLogger).to receive(:error).with(
'class' => described_class.name,
'message' => /Cache mismatch/,
'project_id' => project.id,
'project_path' => project.full_path
)
expect(service.fetch('main', dry_run: true) { false }).to eq(false)
end
end
context 'when cache matches' do
it 'does not log an error' do
expect(service.fetch('main', dry_run: true) { true }).to eq(true)
expect(Gitlab::AppLogger).not_to receive(:error)
expect(service.fetch('main', dry_run: true) { true }).to eq(true)
end
end
end
end
describe '#refresh' do
......
0% Loading or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment