Commit cde78f7f authored by Stan Hu's avatar Stan Hu

Avoid caching BroadcastMessage as an ActiveRecord object

When a Rails 4 host serializes a BroadcastMessage, it will serialize
`ActiveRecord::ConnectionAdapters::PostgreSQL::OID::Integer`, which does
not exist in Rails 5. This will cause Error 500s on a Rails 5 reading
from this cache.

To make Rails 4 and 5 play well together, store the data as JSON and
construct the ActiveRecord objects from JSON.

Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/55034
parent 7cb0dd98
......@@ -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
messages = decode_messages(raw_messages)
return messages if messages.empty?
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
---
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
......
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