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

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