Commit a886532c authored by Rémy Coutable's avatar Rémy Coutable

Revert to caching the AR object in CacheableAttributes

Caching the attributes as JSON and manually instantiating the record
ended-up very complex since the edge-cases such as upload fields,
serialized fields, and fields with custom accessors had to be handled.

To ensure 3 points out of 4 are checked from https://gitlab.com/gitlab-org/gitlab-ce/issues/45175
we now include the Rails version in the cache key.
Signed-off-by: Rémy Coutable's avatarRémy Coutable <remy@rymai.me>
parent e206e328
......@@ -6,15 +6,16 @@ module CacheableAttributes
end
class_methods do
def cache_key
"#{name}:#{Gitlab::VERSION}:#{Gitlab.migrations_hash}:#{Rails.version}".freeze
end
# Can be overriden
def current_without_cache
last
end
def cache_key
"#{name}:#{Gitlab::VERSION}:#{Gitlab.migrations_hash}:json".freeze
end
# Can be overriden
def defaults
{}
end
......@@ -24,10 +25,14 @@ module CacheableAttributes
end
def cached
json_attributes = Rails.cache.read(cache_key)
return nil unless json_attributes.present?
retrieve_from_cache
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
def current
......@@ -35,7 +40,12 @@ module CacheableAttributes
return cached_record if cached_record.present?
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)
current_without_cache
end
......@@ -46,9 +56,15 @@ module CacheableAttributes
# Gracefully handle when Redis is not available. For example,
# omnibus may fail here during gitlab:assets:compile.
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
def cache!
Rails.cache.write(self.class.cache_key, attributes.to_json)
Rails.cache.write(self.class.cache_key, self)
end
end
......@@ -22,7 +22,7 @@ describe CacheableAttributes do
attr_accessor :attributes
def initialize(attrs = {})
def initialize(attrs = {}, *)
@attributes = attrs
end
end
......@@ -52,7 +52,7 @@ describe CacheableAttributes do
describe '.cache_key' 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
......@@ -75,47 +75,106 @@ describe CacheableAttributes do
context 'without any attributes given' 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
context 'without attributes given' do
context 'with attributes given' 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')
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
describe '.current', :use_clean_rails_memory_store_caching do
context 'redis unavailable' do
it 'returns an uncached record' do
before do
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
expect(minimal_test_class.current).to eq(:last)
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)
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
context 'when a record is not yet present' do
it 'does not cache nil object' do
# 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(Rails.cache.exist?(minimal_test_class.cache_key)).to be(false)
expect(ApplicationSetting.current).to be_nil
expect(Rails.cache.exist?(ApplicationSetting.cache_key)).to be(false)
end
it 'cache non-nil object' do
# when the settings are set the method returns a valid object
allow(minimal_test_class).to receive(:last).and_call_original
it 'caches non-nil object' do
create(:application_setting)
expect(minimal_test_class.current).to eq(minimal_test_class.last)
expect(Rails.cache.exist?(minimal_test_class.cache_key)).to be(true)
expect(ApplicationSetting.current).to eq(ApplicationSetting.last)
expect(Rails.cache.exist?(ApplicationSetting.cache_key)).to be(true)
# subsequent calls retrieve the record from the cache
last_record = minimal_test_class.last
expect(minimal_test_class).not_to receive(:last)
expect(minimal_test_class.current.attributes).to eq(last_record.attributes)
last_record = ApplicationSetting.last
expect(ApplicationSetting).not_to receive(:current_without_cache)
expect(ApplicationSetting.current.attributes).to eq(last_record.attributes)
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
end
......@@ -127,27 +186,36 @@ describe CacheableAttributes do
end
end
context 'when cached settings do not include the latest defaults' do
context 'when cached is warm' do
before do
Rails.cache.write(minimal_test_class.cache_key, { bar: 'b', baz: 'c' }.to_json)
minimal_test_class.define_singleton_method(:defaults) do
{ foo: 'a', bar: 'b', baz: 'c' }
end
# Warm up the cache
create(:appearance).cache!
end
it 'includes attributes from defaults' do
expect(minimal_test_class.cached.attributes[:foo]).to eq(minimal_test_class.defaults[:foo])
it 'retrieves the record from cache' do
expect(ActiveRecord::QueryRecorder.new { Appearance.cached }.count).to eq(0)
expect(Appearance.cached).to eq(Appearance.current_without_cache)
end
end
end
describe '#cache!', :use_clean_rails_memory_store_caching do
let(:appearance_record) { create(:appearance) }
let(:record) { create(:appearance) }
it 'caches the attributes' do
appearance_record.cache!
record.cache!
expect(Rails.cache.read(Appearance.cache_key)).to eq(record)
end
describe 'edge cases' do
let(:record) { create(:appearance) }
expect(Rails.cache.read(Appearance.cache_key)).to eq(appearance_record.attributes.to_json)
it 'caches the attributes' do
record.cache!
expect(Rails.cache.read(Appearance.cache_key)).to eq(record)
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