summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
-rw-r--r--app/models/concerns/cacheable_attributes.rb38
-rw-r--r--spec/models/concerns/cacheable_attributes_spec.rb133
2 files changed, 134 insertions, 37 deletions
diff --git a/app/models/concerns/cacheable_attributes.rb b/app/models/concerns/cacheable_attributes.rb
index b32459fdabf..d58d7165969 100644
--- a/app/models/concerns/cacheable_attributes.rb
+++ b/app/models/concerns/cacheable_attributes.rb
@@ -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,18 @@ module CacheableAttributes
end
def cached
- json_attributes = Rails.cache.read(cache_key)
- return nil unless json_attributes.present?
+ if RequestStore.active?
+ 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
def current
@@ -35,7 +44,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 +60,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
diff --git a/spec/models/concerns/cacheable_attributes_spec.rb b/spec/models/concerns/cacheable_attributes_spec.rb
index 49e4b23ebc7..c6331c5ec15 100644
--- a/spec/models/concerns/cacheable_attributes_spec.rb
+++ b/spec/models/concerns/cacheable_attributes_spec.rb
@@ -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,49 +75,117 @@ 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
+
+ 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
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
+
+ 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
describe '.cached', :use_clean_rails_memory_store_caching do
@@ -127,27 +195,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(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