From 3fd8e612719dbeb9d4790aef6875b005d9dd6aff Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Wed, 20 Mar 2019 15:34:30 -0300 Subject: [PATCH 1/4] Add option to not exclude _html fields from attributes --- app/models/concerns/cache_markdown_field.rb | 21 +++++++++++++++---- .../concerns/cache_markdown_field_spec.rb | 14 +++++++++---- 2 files changed, 27 insertions(+), 8 deletions(-) diff --git a/app/models/concerns/cache_markdown_field.rb b/app/models/concerns/cache_markdown_field.rb index 1a8570b80c3..4212224b66a 100644 --- a/app/models/concerns/cache_markdown_field.rb +++ b/app/models/concerns/cache_markdown_field.rb @@ -7,6 +7,7 @@ # cache_markdown_field :foo # cache_markdown_field :bar # cache_markdown_field :baz, pipeline: :single_line +# cache_markdown_field :baz, hidden: false # # Corresponding foo_html, bar_html and baz_html fields should exist. module CacheMarkdownField @@ -37,7 +38,15 @@ module CacheMarkdownField end def html_fields - markdown_fields.map {|field| html_field(field) } + markdown_fields.map { |field| html_field(field) } + end + + def hidden_html_fields + markdown_fields.each_with_object([]) do |field, fields| + if @data[field].fetch(:hidden, true) + fields << html_field(field) + end + end end end @@ -149,13 +158,17 @@ module CacheMarkdownField alias_method :attributes_before_markdown_cache, :attributes def attributes attrs = attributes_before_markdown_cache + html_fields = cached_markdown_fields.html_fields + hidden_html_fields = cached_markdown_fields.hidden_html_fields - attrs.delete('cached_markdown_version') - - cached_markdown_fields.html_fields.each do |field| + hidden_html_fields.each do |field| attrs.delete(field) end + if (html_fields - hidden_html_fields).empty? + attrs.delete('cached_markdown_version') + end + attrs end diff --git a/spec/models/concerns/cache_markdown_field_spec.rb b/spec/models/concerns/cache_markdown_field_spec.rb index 447279f19a8..a2e19f7d687 100644 --- a/spec/models/concerns/cache_markdown_field_spec.rb +++ b/spec/models/concerns/cache_markdown_field_spec.rb @@ -23,6 +23,7 @@ describe CacheMarkdownField do include CacheMarkdownField cache_markdown_field :foo cache_markdown_field :baz, pipeline: :single_line + cache_markdown_field :zoo, hidden: false def self.add_attr(name) self.attribute_names += [name] @@ -35,7 +36,7 @@ describe CacheMarkdownField do add_attr :cached_markdown_version - [:foo, :foo_html, :bar, :baz, :baz_html].each do |name| + [:foo, :foo_html, :bar, :baz, :baz_html, :zoo, :zoo_html].each do |name| add_attr(name) end @@ -84,8 +85,8 @@ describe CacheMarkdownField do end describe '.attributes' do - it 'excludes cache attributes' do - expect(thing.attributes.keys.sort).to eq(%w[bar baz foo]) + it 'excludes cache attributes that is hidden by default' do + expect(thing.attributes.keys.sort).to eq(%w[bar baz cached_markdown_version foo zoo zoo_html]) end end @@ -297,7 +298,12 @@ describe CacheMarkdownField do it 'saves the changes using #update_columns' do expect(thing).to receive(:persisted?).and_return(true) expect(thing).to receive(:update_columns) - .with("foo_html" => updated_html, "baz_html" => "", "cached_markdown_version" => cache_version) + .with( + "foo_html" => updated_html, + "baz_html" => "", + "zoo_html" => "", + "cached_markdown_version" => cache_version + ) thing.refresh_markdown_cache! end -- GitLab From bcc988a6c6c9896450b7c40d2718a1aa9dbd689f Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Wed, 20 Mar 2019 15:43:40 -0300 Subject: [PATCH 2/4] Does not exclude message_html from attributes --- app/models/broadcast_message.rb | 2 +- spec/models/broadcast_message_spec.rb | 6 ++++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/app/models/broadcast_message.rb b/app/models/broadcast_message.rb index 2d237383e60..e77a1236c39 100644 --- a/app/models/broadcast_message.rb +++ b/app/models/broadcast_message.rb @@ -4,7 +4,7 @@ class BroadcastMessage < ActiveRecord::Base include CacheMarkdownField include Sortable - cache_markdown_field :message, pipeline: :broadcast_message + cache_markdown_field :message, pipeline: :broadcast_message, hidden: false validates :message, presence: true validates :starts_at, presence: true diff --git a/spec/models/broadcast_message_spec.rb b/spec/models/broadcast_message_spec.rb index 89839709131..30ca07d5d2c 100644 --- a/spec/models/broadcast_message_spec.rb +++ b/spec/models/broadcast_message_spec.rb @@ -95,6 +95,12 @@ describe BroadcastMessage do end end + describe '#attributes' do + it 'includes message_html field' do + expect(subject.attributes.keys).to include("cached_markdown_version", "message_html") + end + end + describe '#active?' do it 'is truthy when started and not ended' do message = build(:broadcast_message) -- GitLab From 69dc893da3f19d465c12c6c183270daf38df14b3 Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Wed, 20 Mar 2019 15:44:05 -0300 Subject: [PATCH 3/4] Fix spec for Gitlab::JsonCache --- spec/lib/gitlab/json_cache_spec.rb | 30 +++++++++++++++++++++++------- 1 file changed, 23 insertions(+), 7 deletions(-) diff --git a/spec/lib/gitlab/json_cache_spec.rb b/spec/lib/gitlab/json_cache_spec.rb index b7dc8234bdf..b82c09af306 100644 --- a/spec/lib/gitlab/json_cache_spec.rb +++ b/spec/lib/gitlab/json_cache_spec.rb @@ -146,6 +146,18 @@ describe Gitlab::JsonCache do expect(cache.read(key, BroadcastMessage)).to be_nil end + + it 'gracefully handles excluded fields from attributes during serialization' do + allow(backend).to receive(:read) + .with(expanded_key) + .and_return(broadcast_message.attributes.except("message_html").to_json) + + result = cache.read(key, BroadcastMessage) + + BroadcastMessage.cached_markdown_fields.html_fields.each do |field| + expect(result.public_send(field)).to be_nil + end + end end context 'when the cached value is an array' do @@ -327,7 +339,9 @@ describe Gitlab::JsonCache do .with(expanded_key) .and_return('{') - expect(cache.read(key, BroadcastMessage)).to be_nil + result = cache.fetch(key, as: BroadcastMessage) { 'block result' } + + expect(result).to eq 'block result' end it 'gracefully handles an empty hash' do @@ -335,7 +349,7 @@ describe Gitlab::JsonCache do .with(expanded_key) .and_return('{}') - expect(cache.read(key, BroadcastMessage)).to be_a(BroadcastMessage) + expect(cache.fetch(key, as: BroadcastMessage)).to be_a(BroadcastMessage) end it 'gracefully handles unknown attributes' do @@ -343,17 +357,19 @@ describe Gitlab::JsonCache do .with(expanded_key) .and_return(broadcast_message.attributes.merge(unknown_attribute: 1).to_json) - expect(cache.read(key, BroadcastMessage)).to be_nil + result = cache.fetch(key, as: BroadcastMessage) { 'block result' } + + expect(result).to eq 'block result' end it 'gracefully handles excluded fields from attributes during serialization' do - backend.write(expanded_key, broadcast_message.to_json) + allow(backend).to receive(:read) + .with(expanded_key) + .and_return(broadcast_message.attributes.except("message_html").to_json) result = cache.fetch(key, as: BroadcastMessage) { 'block result' } - excluded_fields = BroadcastMessage.cached_markdown_fields.html_fields - - (excluded_fields + ['cached_markdown_version']).each do |field| + BroadcastMessage.cached_markdown_fields.html_fields.each do |field| expect(result.public_send(field)).to be_nil end end -- GitLab From 6264694d572c48f4517f8999a61cc6e0fb32ccae Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Wed, 20 Mar 2019 17:01:27 -0300 Subject: [PATCH 4/4] Rename the hidden option to whitelisted --- app/models/broadcast_message.rb | 2 +- app/models/concerns/cache_markdown_field.rb | 13 +++++++------ spec/models/concerns/cache_markdown_field_spec.rb | 4 ++-- 3 files changed, 10 insertions(+), 9 deletions(-) diff --git a/app/models/broadcast_message.rb b/app/models/broadcast_message.rb index e77a1236c39..1c95abdd9ee 100644 --- a/app/models/broadcast_message.rb +++ b/app/models/broadcast_message.rb @@ -4,7 +4,7 @@ class BroadcastMessage < ActiveRecord::Base include CacheMarkdownField include Sortable - cache_markdown_field :message, pipeline: :broadcast_message, hidden: false + cache_markdown_field :message, pipeline: :broadcast_message, whitelisted: true validates :message, presence: true validates :starts_at, presence: true diff --git a/app/models/concerns/cache_markdown_field.rb b/app/models/concerns/cache_markdown_field.rb index 4212224b66a..15d8d58b9b5 100644 --- a/app/models/concerns/cache_markdown_field.rb +++ b/app/models/concerns/cache_markdown_field.rb @@ -7,7 +7,7 @@ # cache_markdown_field :foo # cache_markdown_field :bar # cache_markdown_field :baz, pipeline: :single_line -# cache_markdown_field :baz, hidden: false +# cache_markdown_field :baz, whitelisted: true # # Corresponding foo_html, bar_html and baz_html fields should exist. module CacheMarkdownField @@ -41,9 +41,9 @@ module CacheMarkdownField markdown_fields.map { |field| html_field(field) } end - def hidden_html_fields + def html_fields_whitelisted markdown_fields.each_with_object([]) do |field, fields| - if @data[field].fetch(:hidden, true) + if @data[field].fetch(:whitelisted, false) fields << html_field(field) end end @@ -159,13 +159,14 @@ module CacheMarkdownField def attributes attrs = attributes_before_markdown_cache html_fields = cached_markdown_fields.html_fields - hidden_html_fields = cached_markdown_fields.hidden_html_fields + whitelisted = cached_markdown_fields.html_fields_whitelisted + exclude_fields = html_fields - whitelisted - hidden_html_fields.each do |field| + exclude_fields.each do |field| attrs.delete(field) end - if (html_fields - hidden_html_fields).empty? + if whitelisted.empty? attrs.delete('cached_markdown_version') end diff --git a/spec/models/concerns/cache_markdown_field_spec.rb b/spec/models/concerns/cache_markdown_field_spec.rb index a2e19f7d687..7d555f15e39 100644 --- a/spec/models/concerns/cache_markdown_field_spec.rb +++ b/spec/models/concerns/cache_markdown_field_spec.rb @@ -23,7 +23,7 @@ describe CacheMarkdownField do include CacheMarkdownField cache_markdown_field :foo cache_markdown_field :baz, pipeline: :single_line - cache_markdown_field :zoo, hidden: false + cache_markdown_field :zoo, whitelisted: true def self.add_attr(name) self.attribute_names += [name] @@ -85,7 +85,7 @@ describe CacheMarkdownField do end describe '.attributes' do - it 'excludes cache attributes that is hidden by default' do + it 'excludes cache attributes that is blacklisted by default' do expect(thing.attributes.keys.sort).to eq(%w[bar baz cached_markdown_version foo zoo zoo_html]) end end -- GitLab