Commit 72702275 authored by Douwe Maan's avatar Douwe Maan

Merge branch...

Merge branch '46913-appearance-uploader-fields-and-description-html-are-missing-in-cached-version' into 'master'

Resolve "Appearance uploader fields and description HTML are missing in cached version"

Closes #46913

See merge request gitlab-org/gitlab-ce!19203
parents 4b0ff7c7 4eda09e3
...@@ -6,15 +6,16 @@ module CacheableAttributes ...@@ -6,15 +6,16 @@ module CacheableAttributes
end end
class_methods do class_methods do
def cache_key
"#{name}:#{Gitlab::VERSION}:#{Gitlab.migrations_hash}:#{Rails.version}".freeze
end
# Can be overriden # Can be overriden
def current_without_cache def current_without_cache
last last
end end
def cache_key # Can be overriden
"#{name}:#{Gitlab::VERSION}:#{Gitlab.migrations_hash}:json".freeze
end
def defaults def defaults
{} {}
end end
...@@ -24,10 +25,18 @@ module CacheableAttributes ...@@ -24,10 +25,18 @@ module CacheableAttributes
end end
def cached def cached
json_attributes = Rails.cache.read(cache_key) if RequestStore.active?
return nil unless json_attributes.present? RequestStore[:"#{name}_cached_attributes"] ||= retrieve_from_cache
else
retrieve_from_cache
end
end
def retrieve_from_cache
record = Rails.cache.read(cache_key)
ensure_cache_setup if record.present?
build_from_defaults(JSON.parse(json_attributes)) record
end end
def current def current
...@@ -35,7 +44,12 @@ module CacheableAttributes ...@@ -35,7 +44,12 @@ module CacheableAttributes
return cached_record if cached_record.present? return cached_record if cached_record.present?
current_without_cache.tap { |current_record| current_record&.cache! } current_without_cache.tap { |current_record| current_record&.cache! }
rescue rescue => e
if Rails.env.production?
Rails.logger.warn("Cached record for #{name} couldn't be loaded, falling back to uncached record: #{e}")
else
raise e
end
# Fall back to an uncached value if there are any problems (e.g. Redis down) # Fall back to an uncached value if there are any problems (e.g. Redis down)
current_without_cache current_without_cache
end end
...@@ -46,9 +60,15 @@ module CacheableAttributes ...@@ -46,9 +60,15 @@ module CacheableAttributes
# Gracefully handle when Redis is not available. For example, # Gracefully handle when Redis is not available. For example,
# omnibus may fail here during gitlab:assets:compile. # omnibus may fail here during gitlab:assets:compile.
end end
def ensure_cache_setup
# This is a workaround for a Rails bug that causes attribute methods not
# to be loaded when read from cache: https://github.com/rails/rails/issues/27348
define_attribute_methods
end
end end
def cache! def cache!
Rails.cache.write(self.class.cache_key, attributes.to_json) Rails.cache.write(self.class.cache_key, self)
end end
end end
...@@ -22,7 +22,7 @@ describe CacheableAttributes do ...@@ -22,7 +22,7 @@ describe CacheableAttributes do
attr_accessor :attributes attr_accessor :attributes
def initialize(attrs = {}) def initialize(attrs = {}, *)
@attributes = attrs @attributes = attrs
end end
end end
...@@ -52,7 +52,7 @@ describe CacheableAttributes do ...@@ -52,7 +52,7 @@ describe CacheableAttributes do
describe '.cache_key' do describe '.cache_key' do
it 'excludes cache attributes' do it 'excludes cache attributes' do
expect(minimal_test_class.cache_key).to eq("TestClass:#{Gitlab::VERSION}:#{Gitlab.migrations_hash}:json") expect(minimal_test_class.cache_key).to eq("TestClass:#{Gitlab::VERSION}:#{Gitlab.migrations_hash}:#{Rails.version}")
end end
end end
...@@ -75,49 +75,117 @@ describe CacheableAttributes do ...@@ -75,49 +75,117 @@ describe CacheableAttributes do
context 'without any attributes given' do context 'without any attributes given' do
it 'intializes a new object with the defaults' do it 'intializes a new object with the defaults' do
expect(minimal_test_class.build_from_defaults).not_to be_persisted expect(minimal_test_class.build_from_defaults.attributes).to eq(minimal_test_class.defaults)
end end
end end
context 'without attributes given' do context 'with attributes given' do
it 'intializes a new object with the given attributes merged into the defaults' do it 'intializes a new object with the given attributes merged into the defaults' do
expect(minimal_test_class.build_from_defaults(foo: 'd').attributes[:foo]).to eq('d') expect(minimal_test_class.build_from_defaults(foo: 'd').attributes[:foo]).to eq('d')
end end
end end
describe 'edge cases on concrete implementations' do
describe '.build_from_defaults' do
context 'without any attributes given' do
it 'intializes all attributes even if they are nil' do
record = ApplicationSetting.build_from_defaults
expect(record).not_to be_persisted
expect(record.sign_in_text).to be_nil
end
end
end
end
end end
describe '.current', :use_clean_rails_memory_store_caching do describe '.current', :use_clean_rails_memory_store_caching do
context 'redis unavailable' do context 'redis unavailable' do
it 'returns an uncached record' do before do
allow(minimal_test_class).to receive(:last).and_return(:last) allow(minimal_test_class).to receive(:last).and_return(:last)
expect(Rails.cache).to receive(:read).and_raise(Redis::BaseError) expect(Rails.cache).to receive(:read).with(minimal_test_class.cache_key).and_raise(Redis::BaseError)
end
context 'in production environment' do
before do
expect(Rails.env).to receive(:production?).and_return(true)
end
it 'returns an uncached record and logs a warning' do
expect(Rails.logger).to receive(:warn).with("Cached record for TestClass couldn't be loaded, falling back to uncached record: Redis::BaseError")
expect(minimal_test_class.current).to eq(:last) expect(minimal_test_class.current).to eq(:last)
end
end
context 'in other environments' do
before do
expect(Rails.env).to receive(:production?).and_return(false)
end
it 'returns an uncached record and logs a warning' do
expect(Rails.logger).not_to receive(:warn)
expect { minimal_test_class.current }.to raise_error(Redis::BaseError)
end
end end
end end
context 'when a record is not yet present' do context 'when a record is not yet present' do
it 'does not cache nil object' do it 'does not cache nil object' do
# when missing settings a nil object is returned, but not cached # when missing settings a nil object is returned, but not cached
allow(minimal_test_class).to receive(:last).twice.and_return(nil) allow(ApplicationSetting).to receive(:current_without_cache).twice.and_return(nil)
expect(minimal_test_class.current).to be_nil expect(ApplicationSetting.current).to be_nil
expect(Rails.cache.exist?(minimal_test_class.cache_key)).to be(false) expect(Rails.cache.exist?(ApplicationSetting.cache_key)).to be(false)
end end
it 'cache non-nil object' do it 'caches non-nil object' do
# when the settings are set the method returns a valid object create(:application_setting)
allow(minimal_test_class).to receive(:last).and_call_original
expect(minimal_test_class.current).to eq(minimal_test_class.last) expect(ApplicationSetting.current).to eq(ApplicationSetting.last)
expect(Rails.cache.exist?(minimal_test_class.cache_key)).to be(true) expect(Rails.cache.exist?(ApplicationSetting.cache_key)).to be(true)
# subsequent calls retrieve the record from the cache # subsequent calls retrieve the record from the cache
last_record = minimal_test_class.last last_record = ApplicationSetting.last
expect(minimal_test_class).not_to receive(:last) expect(ApplicationSetting).not_to receive(:current_without_cache)
expect(minimal_test_class.current.attributes).to eq(last_record.attributes) expect(ApplicationSetting.current.attributes).to eq(last_record.attributes)
end end
end end
describe 'edge cases' do
describe 'caching behavior', :use_clean_rails_memory_store_caching do
it 'retrieves upload fields properly' do
ar_record = create(:appearance, :with_logo)
ar_record.cache!
cache_record = Appearance.current
expect(cache_record).to be_persisted
expect(cache_record.logo).to be_an(AttachmentUploader)
expect(cache_record.logo.url).to end_with('/dk.png')
end
it 'retrieves markdown fields properly' do
ar_record = create(:appearance, description: '**Hello**')
ar_record.cache!
cache_record = Appearance.current
expect(cache_record.description).to eq('**Hello**')
expect(cache_record.description_html).to eq('<p dir="auto"><strong>Hello</strong></p>')
end
end
end
it 'uses RequestStore in addition to Rails.cache', :request_store do
# Warm up the cache
create(:application_setting).cache!
expect(Rails.cache).to receive(:read).with(ApplicationSetting.cache_key).once.and_call_original
2.times { ApplicationSetting.current }
end
end end
describe '.cached', :use_clean_rails_memory_store_caching do describe '.cached', :use_clean_rails_memory_store_caching do
...@@ -127,27 +195,36 @@ describe CacheableAttributes do ...@@ -127,27 +195,36 @@ describe CacheableAttributes do
end end
end end
context 'when cached settings do not include the latest defaults' do context 'when cached is warm' do
before do before do
Rails.cache.write(minimal_test_class.cache_key, { bar: 'b', baz: 'c' }.to_json) # Warm up the cache
minimal_test_class.define_singleton_method(:defaults) do create(:appearance).cache!
{ foo: 'a', bar: 'b', baz: 'c' }
end
end end
it 'includes attributes from defaults' do it 'retrieves the record from cache' do
expect(minimal_test_class.cached.attributes[:foo]).to eq(minimal_test_class.defaults[:foo]) expect(ActiveRecord::QueryRecorder.new { Appearance.cached }.count).to eq(0)
expect(Appearance.cached).to eq(Appearance.current_without_cache)
end end
end end
end end
describe '#cache!', :use_clean_rails_memory_store_caching do describe '#cache!', :use_clean_rails_memory_store_caching do
let(:appearance_record) { create(:appearance) } let(:record) { create(:appearance) }
it 'caches the attributes' do it 'caches the attributes' do
appearance_record.cache! record.cache!
expect(Rails.cache.read(Appearance.cache_key)).to eq(appearance_record.attributes.to_json) expect(Rails.cache.read(Appearance.cache_key)).to eq(record)
end
describe 'edge cases' do
let(:record) { create(:appearance) }
it 'caches the attributes' do
record.cache!
expect(Rails.cache.read(Appearance.cache_key)).to eq(record)
end
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