Commit cbbe0349 authored by Stan Hu's avatar Stan Hu

Fix error 500 loading branch with UTF-8 characters with performance bar

When we added caching for protected branch names in
https://gitlab.com/gitlab-org/gitlab/-/merge_requests/64738, we did not
account for UTF-8 characters being used in the branch name.

The cache key would be encoded in ASCII-8BIT due to the branch name
coming from Gitaly, but when the performance bar were enabled the Redis
activity would be dumped to JSON. Since this string wasn't UTF-8, Ruby
would throw up a `Encoding::UndefinedConversionError`.

This commit fixes the problem by hashing the ref name with SHA-1 to
avoid needing to deal with any encoding issues.

Relates to https://gitlab.com/gitlab-org/gitlab/-/issues/343698

Changelog: fixed
parent 817d5d87
...@@ -27,12 +27,17 @@ class ProtectedBranch < ApplicationRecord ...@@ -27,12 +27,17 @@ class ProtectedBranch < ApplicationRecord
# Check if branch name is marked as protected in the system # Check if branch name is marked as protected in the system
def self.protected?(project, ref_name) def self.protected?(project, ref_name)
return true if project.empty_repo? && project.default_branch_protected? return true if project.empty_repo? && project.default_branch_protected?
return false if ref_name.blank?
Rails.cache.fetch("protected_ref-#{ref_name}-#{project.cache_key}") do Rails.cache.fetch(protected_ref_cache_key(project, ref_name)) do
self.matching(ref_name, protected_refs: protected_refs(project)).present? self.matching(ref_name, protected_refs: protected_refs(project)).present?
end end
end end
def self.protected_ref_cache_key(project, ref_name)
"protected_ref-#{project.cache_key}-#{Digest::SHA1.hexdigest(ref_name)}"
end
def self.allow_force_push?(project, ref_name) def self.allow_force_push?(project, ref_name)
project.protected_branches.allowing_force_push.matching(ref_name).any? project.protected_branches.allowing_force_push.matching(ref_name).any?
end end
......
...@@ -163,27 +163,32 @@ RSpec.describe ProtectedBranch do ...@@ -163,27 +163,32 @@ RSpec.describe ProtectedBranch do
expect(described_class.protected?(project, 'staging/some-branch')).to eq(false) expect(described_class.protected?(project, 'staging/some-branch')).to eq(false)
end end
it 'returns false when branch name is nil' do
expect(described_class.protected?(project, nil)).to eq(false)
end
context 'with caching', :use_clean_rails_memory_store_caching do context 'with caching', :use_clean_rails_memory_store_caching do
let_it_be(:project) { create(:project, :repository) } let_it_be(:project) { create(:project, :repository) }
let_it_be(:protected_branch) { create(:protected_branch, project: project, name: "jawn") } let_it_be(:protected_branch) { create(:protected_branch, project: project, name: "“jawn”") }
before do before do
allow(described_class).to receive(:matching).once.and_call_original allow(described_class).to receive(:matching).with(protected_branch.name, protected_refs: anything).once.and_call_original
# the original call works and warms the cache # the original call works and warms the cache
described_class.protected?(project, 'jawn') described_class.protected?(project, protected_branch.name)
end end
it 'correctly invalidates a cache' do it 'correctly invalidates a cache' do
expect(described_class).to receive(:matching).once.and_call_original expect(described_class).to receive(:matching).with(protected_branch.name, protected_refs: anything).once.and_call_original
create(:protected_branch, project: project, name: "bar") create(:protected_branch, project: project, name: "bar")
# the cache is invalidated because the project has been "updated" # the cache is invalidated because the project has been "updated"
expect(described_class.protected?(project, 'jawn')).to eq(true) expect(described_class.protected?(project, protected_branch.name)).to eq(true)
end end
it 'correctly uses the cached version' do it 'correctly uses the cached version' do
expect(described_class).not_to receive(:matching) expect(described_class).not_to receive(:matching)
expect(described_class.protected?(project, 'jawn')).to eq(true) expect(described_class.protected?(project, protected_branch.name)).to eq(true)
end end
end end
end end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment