diff options
author | Stan Hu <stanhu@gmail.com> | 2018-12-07 22:16:45 +0300 |
---|---|---|
committer | Stan Hu <stanhu@gmail.com> | 2018-12-09 09:06:07 +0300 |
commit | cde78f7f4eaec2740c6e91c7d225d701d0097ec0 (patch) | |
tree | e62f795f3a92d77c2ac2991e3b95f6c6ded8c6a9 | |
parent | 7cb0dd98590e8fdd7483b9f61643a0daa23c2b67 (diff) |
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
-rw-r--r-- | app/models/broadcast_message.rb | 42 | ||||
-rw-r--r-- | changelogs/unreleased/sh-json-serialize-broadcast-messages.yml | 5 | ||||
-rw-r--r-- | spec/models/broadcast_message_spec.rb | 37 |
3 files changed, 80 insertions, 4 deletions
diff --git a/app/models/broadcast_message.rb b/app/models/broadcast_message.rb index baf8adb318b..277f7c2717c 100644 --- a/app/models/broadcast_message.rb +++ b/app/models/broadcast_message.rb @@ -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 diff --git a/changelogs/unreleased/sh-json-serialize-broadcast-messages.yml b/changelogs/unreleased/sh-json-serialize-broadcast-messages.yml new file mode 100644 index 00000000000..e8bee64f780 --- /dev/null +++ b/changelogs/unreleased/sh-json-serialize-broadcast-messages.yml @@ -0,0 +1,5 @@ +--- +title: Avoid caching BroadcastMessage as an ActiveRecord object +merge_request: 23662 +author: +type: fixed diff --git a/spec/models/broadcast_message_spec.rb b/spec/models/broadcast_message_spec.rb index 5326f9cb8c0..d6e5b557870 100644 --- a/spec/models/broadcast_message_spec.rb +++ b/spec/models/broadcast_message_spec.rb @@ -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 |