Merge branch 'sh-json-serialize-broadcast-messages' into 'master'
Avoid caching BroadcastMessage as an ActiveRecord object Closes #55034 See merge request gitlab-org/gitlab-ce!23662
This commit is contained in:
commit
ec76f4fd90
3 changed files with 80 additions and 4 deletions
|
@ -16,14 +16,20 @@ class BroadcastMessage < ActiveRecord::Base
|
|||
default_value_for :color, '#E75E40'
|
||||
default_value_for :font, '#FFFFFF'
|
||||
|
||||
CACHE_KEY = 'broadcast_message_current'.freeze
|
||||
CACHE_KEY = 'broadcast_message_current_json'.freeze
|
||||
LEGACY_CACHE_KEY = 'broadcast_message_current'.freeze
|
||||
|
||||
after_commit :flush_redis_cache
|
||||
|
||||
def self.current
|
||||
messages = Rails.cache.fetch(CACHE_KEY, expires_in: cache_expires_in) { current_and_future_messages.to_a }
|
||||
raw_messages = Rails.cache.fetch(CACHE_KEY, expires_in: cache_expires_in) do
|
||||
remove_legacy_cache_key
|
||||
current_and_future_messages.to_json
|
||||
end
|
||||
|
||||
return messages if messages.empty?
|
||||
messages = decode_messages(raw_messages)
|
||||
|
||||
return [] unless messages&.present?
|
||||
|
||||
now_or_future = messages.select(&:now_or_future?)
|
||||
|
||||
|
@ -34,6 +40,27 @@ class BroadcastMessage < ActiveRecord::Base
|
|||
now_or_future.select(&:now?)
|
||||
end
|
||||
|
||||
def self.decode_messages(raw_messages)
|
||||
return unless raw_messages&.present?
|
||||
|
||||
message_list = ActiveSupport::JSON.decode(raw_messages)
|
||||
|
||||
return unless message_list.is_a?(Array)
|
||||
|
||||
valid_attr = BroadcastMessage.attribute_names
|
||||
|
||||
message_list.map do |raw|
|
||||
BroadcastMessage.new(raw) if valid_cache_entry?(raw, valid_attr)
|
||||
end.compact
|
||||
rescue ActiveSupport::JSON.parse_error
|
||||
end
|
||||
|
||||
def self.valid_cache_entry?(raw, valid_attr)
|
||||
return false unless raw.is_a?(Hash)
|
||||
|
||||
(raw.keys - valid_attr).empty?
|
||||
end
|
||||
|
||||
def self.current_and_future_messages
|
||||
where('ends_at > :now', now: Time.zone.now).order_id_asc
|
||||
end
|
||||
|
@ -42,6 +69,14 @@ class BroadcastMessage < ActiveRecord::Base
|
|||
nil
|
||||
end
|
||||
|
||||
# This can be removed in GitLab 12.0+
|
||||
# The old cache key had an indefinite lifetime, and in an HA
|
||||
# environment a one-shot migration would not work because the cache
|
||||
# would be repopulated by a node that has not been upgraded.
|
||||
def self.remove_legacy_cache_key
|
||||
Rails.cache.delete(LEGACY_CACHE_KEY)
|
||||
end
|
||||
|
||||
def active?
|
||||
started? && !ended?
|
||||
end
|
||||
|
@ -68,5 +103,6 @@ class BroadcastMessage < ActiveRecord::Base
|
|||
|
||||
def flush_redis_cache
|
||||
Rails.cache.delete(CACHE_KEY)
|
||||
self.class.remove_legacy_cache_key
|
||||
end
|
||||
end
|
||||
|
|
|
@ -0,0 +1,5 @@
|
|||
---
|
||||
title: Avoid caching BroadcastMessage as an ActiveRecord object
|
||||
merge_request: 23662
|
||||
author:
|
||||
type: fixed
|
|
@ -58,6 +58,12 @@ describe BroadcastMessage do
|
|||
end
|
||||
end
|
||||
|
||||
it 'does not create new records' do
|
||||
create(:broadcast_message)
|
||||
|
||||
expect { described_class.current }.not_to change { described_class.count }
|
||||
end
|
||||
|
||||
it 'includes messages that need to be displayed in the future' do
|
||||
create(:broadcast_message)
|
||||
|
||||
|
@ -77,9 +83,37 @@ describe BroadcastMessage do
|
|||
it 'does not clear the cache if only a future message should be displayed' do
|
||||
create(:broadcast_message, :future)
|
||||
|
||||
expect(Rails.cache).not_to receive(:delete)
|
||||
expect(Rails.cache).not_to receive(:delete).with(described_class::CACHE_KEY)
|
||||
expect(described_class.current.length).to eq(0)
|
||||
end
|
||||
|
||||
it 'clears the legacy cache key' do
|
||||
create(:broadcast_message, :future)
|
||||
|
||||
expect(Rails.cache).to receive(:delete).with(described_class::LEGACY_CACHE_KEY)
|
||||
expect(described_class.current.length).to eq(0)
|
||||
end
|
||||
|
||||
it 'gracefully handles bad cache entry' do
|
||||
allow(described_class).to receive(:current_and_future_messages).and_return('{')
|
||||
|
||||
expect(described_class.current).to be_empty
|
||||
end
|
||||
|
||||
it 'gracefully handles an empty hash' do
|
||||
allow(described_class).to receive(:current_and_future_messages).and_return('{}')
|
||||
|
||||
expect(described_class.current).to be_empty
|
||||
end
|
||||
|
||||
it 'gracefully handles unknown attributes' do
|
||||
message = create(:broadcast_message)
|
||||
|
||||
allow(described_class).to receive(:current_and_future_messages)
|
||||
.and_return([{ bad_attr: 1 }, message])
|
||||
|
||||
expect(described_class.current).to eq([message])
|
||||
end
|
||||
end
|
||||
|
||||
describe '#active?' do
|
||||
|
@ -143,6 +177,7 @@ describe BroadcastMessage do
|
|||
message = create(:broadcast_message)
|
||||
|
||||
expect(Rails.cache).to receive(:delete).with(described_class::CACHE_KEY)
|
||||
expect(Rails.cache).to receive(:delete).with(described_class::LEGACY_CACHE_KEY)
|
||||
|
||||
message.flush_redis_cache
|
||||
end
|
||||
|
|
Loading…
Reference in a new issue