Merge branch 'bw-commonmark-cached-markdown-version' into 'master'

Ensure that cached_markdown_version is handled correctly for CommonMark

See merge request gitlab-org/gitlab-ce!18431

app/models/concerns/cache_markdown_field.rb

@@ -11,7 +11,9 @@ module CacheMarkdownField
   extend ActiveSupport::Concern
 
   # Increment this number every time the renderer changes its output
-  CACHE_VERSION = 3
+  CACHE_REDCARPET_VERSION         = 3
+  CACHE_COMMONMARK_VERSION_START  = 10
+  CACHE_COMMONMARK_VERSION        = 11
 
   # changes to these attributes cause the cache to be invalidates
   INVALIDATED_BY = %w[author project].freeze
@@ -55,6 +57,8 @@ module CacheMarkdownField
     # Banzai is less strict about authors, so don't always have an author key
     context[:author] = self.author if self.respond_to?(:author)
 
+    context[:markdown_engine] = markdown_engine
+
     context
   end
 
@@ -69,7 +73,7 @@ module CacheMarkdownField
         Banzai::Renderer.cacheless_render_field(self, markdown_field, options)
       ]
     end.to_h
-    updates['cached_markdown_version'] = CacheMarkdownField::CACHE_VERSION
+    updates['cached_markdown_version'] = latest_cached_markdown_version
 
     updates.each {|html_field, data| write_attribute(html_field, data) }
   end
@@ -90,7 +94,7 @@ module CacheMarkdownField
     markdown_changed = attribute_changed?(markdown_field) || false
     html_changed = attribute_changed?(html_field) || false
 
-    CacheMarkdownField::CACHE_VERSION == cached_markdown_version &&
+    latest_cached_markdown_version == cached_markdown_version &&
       (html_changed || markdown_changed == html_changed)
   end
 
@@ -109,6 +113,24 @@ module CacheMarkdownField
     __send__(cached_markdown_fields.html_field(markdown_field)) # rubocop:disable GitlabSecurity/PublicSend
   end
 
+  def latest_cached_markdown_version
+    return CacheMarkdownField::CACHE_REDCARPET_VERSION unless cached_markdown_version
+
+    if cached_markdown_version < CacheMarkdownField::CACHE_COMMONMARK_VERSION_START
+      CacheMarkdownField::CACHE_REDCARPET_VERSION
+    else
+      CacheMarkdownField::CACHE_COMMONMARK_VERSION
+    end
+  end
+
+  def markdown_engine
+    if latest_cached_markdown_version < CacheMarkdownField::CACHE_COMMONMARK_VERSION_START
+      :redcarpet
+    else
+      :common_mark
+    end
+  end
+
   included do
     cattr_reader :cached_markdown_fields do
       FieldData.new

spec/lib/banzai/object_renderer_spec.rb

@@ -11,7 +11,7 @@ describe Banzai::ObjectRenderer do
     )
   end
 
-  let(:object) { Note.new(note: 'hello', note_html: '<p dir="auto">hello</p>', cached_markdown_version: CacheMarkdownField::CACHE_VERSION) }
+  let(:object) { Note.new(note: 'hello', note_html: '<p dir="auto">hello</p>', cached_markdown_version: CacheMarkdownField::CACHE_COMMONMARK_VERSION) }
 
   describe '#render' do
     context 'with cache' do

spec/models/concerns/cache_markdown_field_spec.rb

@@ -72,7 +72,7 @@ describe CacheMarkdownField do
   let(:updated_markdown) { '`Bar`' }
   let(:updated_html) { '<p dir="auto"><code>Bar</code></p>' }
 
-  let(:thing) { ThingWithMarkdownFields.new(foo: markdown, foo_html: html, cached_markdown_version: CacheMarkdownField::CACHE_VERSION) }
+  let(:thing) { ThingWithMarkdownFields.new(foo: markdown, foo_html: html, cached_markdown_version: CacheMarkdownField::CACHE_COMMONMARK_VERSION) }
 
   describe '.attributes' do
     it 'excludes cache attributes' do
@@ -89,17 +89,24 @@ describe CacheMarkdownField do
     it { expect(thing.foo).to eq(markdown) }
     it { expect(thing.foo_html).to eq(html) }
     it { expect(thing.foo_html_changed?).not_to be_truthy }
-    it { expect(thing.cached_markdown_version).to eq(CacheMarkdownField::CACHE_VERSION) }
+    it { expect(thing.cached_markdown_version).to eq(CacheMarkdownField::CACHE_COMMONMARK_VERSION) }
   end
 
   context 'a changed markdown field' do
+    shared_examples 'with cache version' do |cache_version|
+      let(:thing) { ThingWithMarkdownFields.new(foo: markdown, foo_html: html, cached_markdown_version: cache_version) }
+
       before do
         thing.foo = updated_markdown
         thing.save
       end
 
       it { expect(thing.foo_html).to eq(updated_html) }
-    it { expect(thing.cached_markdown_version).to eq(CacheMarkdownField::CACHE_VERSION) }
+      it { expect(thing.cached_markdown_version).to eq(cache_version) }
+    end
+
+    it_behaves_like 'with cache version', CacheMarkdownField::CACHE_REDCARPET_VERSION
+    it_behaves_like 'with cache version', CacheMarkdownField::CACHE_COMMONMARK_VERSION
   end
 
   context 'when a markdown field is set repeatedly to an empty string' do
@@ -123,6 +130,9 @@ describe CacheMarkdownField do
   end
 
   context 'a non-markdown field changed' do
+    shared_examples 'with cache version' do |cache_version|
+      let(:thing) { ThingWithMarkdownFields.new(foo: markdown, foo_html: html, cached_markdown_version: cache_version) }
+
       before do
         thing.bar = 'OK'
         thing.save
@@ -131,7 +141,11 @@ describe CacheMarkdownField do
       it { expect(thing.bar).to eq('OK') }
       it { expect(thing.foo).to eq(markdown) }
       it { expect(thing.foo_html).to eq(html) }
-    it { expect(thing.cached_markdown_version).to eq(CacheMarkdownField::CACHE_VERSION) }
+      it { expect(thing.cached_markdown_version).to eq(cache_version) }
+    end
+
+    it_behaves_like 'with cache version', CacheMarkdownField::CACHE_REDCARPET_VERSION
+    it_behaves_like 'with cache version', CacheMarkdownField::CACHE_COMMONMARK_VERSION
   end
 
   context 'version is out of date' do
@@ -142,10 +156,13 @@ describe CacheMarkdownField do
     end
 
     it { expect(thing.foo_html).to eq(updated_html) }
-    it { expect(thing.cached_markdown_version).to eq(CacheMarkdownField::CACHE_VERSION) }
+    it { expect(thing.cached_markdown_version).to eq(CacheMarkdownField::CACHE_REDCARPET_VERSION) }
   end
 
   describe '#cached_html_up_to_date?' do
+    shared_examples 'with cache version' do |cache_version|
+      let(:thing) { ThingWithMarkdownFields.new(foo: markdown, foo_html: html, cached_markdown_version: cache_version) }
+
       subject { thing.cached_html_up_to_date?(:foo) }
 
       it 'returns false when the version is absent' do
@@ -167,7 +184,7 @@ describe CacheMarkdownField do
       end
 
       it 'returns true when the version is just right' do
-      thing.cached_markdown_version = CacheMarkdownField::CACHE_VERSION
+        thing.cached_markdown_version = cache_version
 
         is_expected.to be_truthy
       end
@@ -198,6 +215,29 @@ describe CacheMarkdownField do
       end
     end
 
+    it_behaves_like 'with cache version', CacheMarkdownField::CACHE_REDCARPET_VERSION
+    it_behaves_like 'with cache version', CacheMarkdownField::CACHE_COMMONMARK_VERSION
+  end
+
+  describe '#latest_cached_markdown_version' do
+    subject { thing.latest_cached_markdown_version }
+
+    it 'returns redcarpet version' do
+      thing.cached_markdown_version = CacheMarkdownField::CACHE_COMMONMARK_VERSION_START - 1
+      is_expected.to eq(CacheMarkdownField::CACHE_REDCARPET_VERSION)
+    end
+
+    it 'returns commonmark version' do
+      thing.cached_markdown_version = CacheMarkdownField::CACHE_COMMONMARK_VERSION_START + 1
+      is_expected.to eq(CacheMarkdownField::CACHE_COMMONMARK_VERSION)
+    end
+
+    it 'returns default version when version is nil' do
+      thing.cached_markdown_version = nil
+      is_expected.to eq(CacheMarkdownField::CACHE_REDCARPET_VERSION)
+    end
+  end
+
   describe '#refresh_markdown_cache' do
     before do
       thing.foo = updated_markdown
@@ -221,11 +261,14 @@ describe CacheMarkdownField do
       thing.cached_markdown_version = nil
       thing.refresh_markdown_cache
 
-      expect(thing.cached_markdown_version).to eq(CacheMarkdownField::CACHE_VERSION)
+      expect(thing.cached_markdown_version).to eq(CacheMarkdownField::CACHE_REDCARPET_VERSION)
     end
   end
 
   describe '#refresh_markdown_cache!' do
+    shared_examples 'with cache version' do |cache_version|
+      let(:thing) { ThingWithMarkdownFields.new(foo: markdown, foo_html: html, cached_markdown_version: cache_version) }
+
       before do
         thing.foo = updated_markdown
       end
@@ -248,12 +291,16 @@ 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" => CacheMarkdownField::CACHE_VERSION)
+          .with("foo_html" => updated_html, "baz_html" => "", "cached_markdown_version" => cache_version)
 
         thing.refresh_markdown_cache!
       end
     end
 
+    it_behaves_like 'with cache version', CacheMarkdownField::CACHE_REDCARPET_VERSION
+    it_behaves_like 'with cache version', CacheMarkdownField::CACHE_COMMONMARK_VERSION
+  end
+
   describe '#banzai_render_context' do
     subject(:context) { thing.banzai_render_context(:foo) }
 
@@ -299,7 +346,7 @@ describe CacheMarkdownField do
 
         expect(thing.foo_html).to eq(updated_html)
         expect(thing.baz_html).to eq(updated_html)
-        expect(thing.cached_markdown_version).to eq(CacheMarkdownField::CACHE_VERSION)
+        expect(thing.cached_markdown_version).to eq(CacheMarkdownField::CACHE_REDCARPET_VERSION)
       end
     end
 
@@ -319,7 +366,7 @@ describe CacheMarkdownField do
 
         expect(thing.foo_html).to eq(updated_html)
         expect(thing.baz_html).to eq(updated_html)
-        expect(thing.cached_markdown_version).to eq(CacheMarkdownField::CACHE_VERSION)
+        expect(thing.cached_markdown_version).to eq(CacheMarkdownField::CACHE_REDCARPET_VERSION)
       end
     end
   end