Merge branch '54140-non-ar-cache-commit-markdown' into 'master'

Use Redis for CacheMarkDownField on non AR models

Closes #54140

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

app/models/commit.rb

@@ -13,6 +13,7 @@ class Commit
   include StaticModel
   include Presentable
   include ::Gitlab::Utils::StrongMemoize
+  include CacheMarkdownField
 
   attr_mentionable :safe_message, pipeline: :single_line
 
@@ -37,13 +38,9 @@ class Commit
   # Used by GFM to match and present link extensions on node texts and hrefs.
   LINK_EXTENSION_PATTERN = /(patch)/.freeze
 
-  def banzai_render_context(field)
-    pipeline = field == :description ? :commit_description : :single_line
-    context = { pipeline: pipeline, project: self.project }
-    context[:author] = self.author if self.author
-
-    context
-  end
+  cache_markdown_field :title, pipeline: :single_line
+  cache_markdown_field :full_title, pipeline: :single_line
+  cache_markdown_field :description, pipeline: :commit_description
 
   class << self
     def decorate(commits, project)

app/models/concerns/cache_markdown_field.rb

@@ -13,43 +13,9 @@
 module CacheMarkdownField
   extend ActiveSupport::Concern
 
-  # Increment this number every time the renderer changes its output
-  CACHE_COMMONMARK_VERSION_START  = 10
-  CACHE_COMMONMARK_VERSION        = 16
-
   # changes to these attributes cause the cache to be invalidates
   INVALIDATED_BY = %w[author project].freeze
 
-  # Knows about the relationship between markdown and html field names, and
-  # stores the rendering contexts for the latter
-  class FieldData
-    def initialize
-      @data = {}
-    end
-
-    delegate :[], :[]=, to: :@data
-
-    def markdown_fields
-      @data.keys
-    end
-
-    def html_field(markdown_field)
-      "#{markdown_field}_html"
-    end
-
-    def html_fields
-      markdown_fields.map { |field| html_field(field) }
-    end
-
-    def html_fields_whitelisted
-      markdown_fields.each_with_object([]) do |field, fields|
-        if @data[field].fetch(:whitelisted, false)
-          fields << html_field(field)
-        end
-      end
-    end
-  end
-
   def skip_project_check?
     false
   end
@@ -85,24 +51,22 @@ module CacheMarkdownField
     end.to_h
     updates['cached_markdown_version'] = latest_cached_markdown_version
 
-    updates.each {|html_field, data| write_attribute(html_field, data) }
+    updates.each { |field, data| write_markdown_field(field, data) }
   end
 
   def refresh_markdown_cache!
     updates = refresh_markdown_cache
 
-    return unless persisted? && Gitlab::Database.read_write?
-
-    update_columns(updates)
+    save_markdown(updates)
   end
 
   def cached_html_up_to_date?(markdown_field)
-    html_field = cached_markdown_fields.html_field(markdown_field)
+    return false if cached_html_for(markdown_field).nil? && __send__(markdown_field).present? # rubocop:disable GitlabSecurity/PublicSend
 
-    return false if cached_html_for(markdown_field).nil? && !__send__(markdown_field).nil? # rubocop:disable GitlabSecurity/PublicSend
+    html_field = cached_markdown_fields.html_field(markdown_field)
 
-    markdown_changed = attribute_changed?(markdown_field) || false
-    html_changed = attribute_changed?(html_field) || false
+    markdown_changed = markdown_field_changed?(markdown_field)
+    html_changed = markdown_field_changed?(html_field)
 
     latest_cached_markdown_version == cached_markdown_version &&
       (html_changed || markdown_changed == html_changed)
@@ -117,21 +81,21 @@ module CacheMarkdownField
   end
 
   def cached_html_for(markdown_field)
-    raise ArgumentError.new("Unknown field: #{field}") unless
+    raise ArgumentError.new("Unknown field: #{markdown_field}") unless
       cached_markdown_fields.markdown_fields.include?(markdown_field)
 
     __send__(cached_markdown_fields.html_field(markdown_field)) # rubocop:disable GitlabSecurity/PublicSend
   end
 
   def latest_cached_markdown_version
-    @latest_cached_markdown_version ||= (CacheMarkdownField::CACHE_COMMONMARK_VERSION << 16) | local_version
+    @latest_cached_markdown_version ||= (Gitlab::MarkdownCache::CACHE_COMMONMARK_VERSION << 16) | local_version
   end
 
   def local_version
     # because local_markdown_version is stored in application_settings which
     # uses cached_markdown_version too, we check explicitly to avoid
     # endless loop
-    return local_markdown_version if has_attribute?(:local_markdown_version)
+    return local_markdown_version if respond_to?(:has_attribute?) && has_attribute?(:local_markdown_version)
 
     settings = Gitlab::CurrentSettings.current_application_settings
 
@@ -150,32 +114,14 @@ module CacheMarkdownField
 
   included do
     cattr_reader :cached_markdown_fields do
-      FieldData.new
+      Gitlab::MarkdownCache::FieldData.new
     end
 
-    # Always exclude _html fields from attributes (including serialization).
-    # They contain unredacted HTML, which would be a security issue
-    alias_method :attributes_before_markdown_cache, :attributes
-    def attributes
-      attrs = attributes_before_markdown_cache
-      html_fields = cached_markdown_fields.html_fields
-      whitelisted = cached_markdown_fields.html_fields_whitelisted
-      exclude_fields = html_fields - whitelisted
-
-      exclude_fields.each do |field|
-        attrs.delete(field)
-      end
-
-      if whitelisted.empty?
-        attrs.delete('cached_markdown_version')
-      end
-
-      attrs
+    if self < ActiveRecord::Base
+      include Gitlab::MarkdownCache::ActiveRecord::Extension
+    else
+      prepend Gitlab::MarkdownCache::Redis::Extension
     end
-
-    # Using before_update here conflicts with elasticsearch-model somehow
-    before_create :refresh_markdown_cache, if: :invalidated_markdown_cache?
-    before_update :refresh_markdown_cache, if: :invalidated_markdown_cache?
   end
 
   class_methods do
@@ -193,10 +139,8 @@ module CacheMarkdownField
       # The HTML becomes invalid if any dependent fields change. For now, assume
       # author and project invalidate the cache in all circumstances.
       define_method(invalidation_method) do
-        changed_fields = changed_attributes.keys
-        invalidations  = changed_fields & [markdown_field.to_s, *INVALIDATED_BY]
-        invalidations.delete(markdown_field.to_s) if changed_fields.include?("#{markdown_field}_html")
-
+        invalidations = changed_markdown_fields & [markdown_field.to_s, *INVALIDATED_BY]
+        invalidations.delete(markdown_field.to_s) if changed_markdown_fields.include?("#{markdown_field}_html")
         !invalidations.empty? || !cached_html_up_to_date?(markdown_field)
       end
     end

app/views/projects/tree/_tree_commit_column.html.haml

+- full_title = markdown_field(commit, :full_title)
 %span.str-truncated
-  = link_to_html commit.redacted_full_title_html, project_commit_path(@project, commit.id), title: commit.redacted_full_title_html, class: 'tree-commit-link'
+  = link_to_html full_title, project_commit_path(@project, commit.id), title: full_title, class: 'tree-commit-link'

changelogs/unreleased/54140-non-ar-cache-commit-markdown.yml

+---
+title: Use Redis for CacheMarkDownField on non AR models
+merge_request: 29054
+author:
+type: performance

lib/banzai/commit_renderer.rb

@@ -2,7 +2,7 @@
 
 module Banzai
   module CommitRenderer
-    ATTRIBUTES = [:description, :title].freeze
+    ATTRIBUTES = [:description, :title, :full_title].freeze
 
     def self.render(commits, project, user = nil)
       obj_renderer = ObjectRenderer.new(user: user, default_project: project)

lib/gitlab/markdown_cache.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module MarkdownCache
+    # Increment this number every time the renderer changes its output
+    CACHE_COMMONMARK_VERSION_START  = 10
+    CACHE_COMMONMARK_VERSION        = 16
+
+    BaseError = Class.new(StandardError)
+    UnsupportedClassError = Class.new(BaseError)
+  end
+end

lib/gitlab/markdown_cache/active_record/extension.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module MarkdownCache
+    module ActiveRecord
+      module Extension
+        extend ActiveSupport::Concern
+
+        included do
+          # Using before_update here conflicts with elasticsearch-model somehow
+          before_create :refresh_markdown_cache, if: :invalidated_markdown_cache?
+          before_update :refresh_markdown_cache, if: :invalidated_markdown_cache?
+        end
+
+        # Always exclude _html fields from attributes (including serialization).
+        # They contain unredacted HTML, which would be a security issue
+        def attributes
+          attrs = super
+          html_fields = cached_markdown_fields.html_fields
+          whitelisted = cached_markdown_fields.html_fields_whitelisted
+          exclude_fields = html_fields - whitelisted
+
+          attrs.except!(*exclude_fields)
+          attrs.delete('cached_markdown_version') if whitelisted.empty?
+
+          attrs
+        end
+
+        def changed_markdown_fields
+          changed_attributes.keys.map(&:to_s) & cached_markdown_fields.markdown_fields.map(&:to_s)
+        end
+
+        def write_markdown_field(field_name, value)
+          write_attribute(field_name, value)
+        end
+
+        def markdown_field_changed?(field_name)
+          attribute_changed?(field_name)
+        end
+
+        def save_markdown(updates)
+          return unless persisted? && Gitlab::Database.read_write?
+
+          update_columns(updates)
+        end
+      end
+    end
+  end
+end

lib/gitlab/markdown_cache/field_data.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module MarkdownCache
+    # Knows about the relationship between markdown and html field names, and
+    # stores the rendering contexts for the latter
+    class FieldData
+      def initialize
+        @data = {}
+      end
+
+      delegate :[], :[]=, to: :@data
+
+      def markdown_fields
+        @data.keys
+      end
+
+      def html_field(markdown_field)
+        "#{markdown_field}_html"
+      end
+
+      def html_fields
+        @html_fields ||= markdown_fields.map { |field| html_field(field) }
+      end
+
+      def html_fields_whitelisted
+        markdown_fields.each_with_object([]) do |field, fields|
+          if @data[field].fetch(:whitelisted, false)
+            fields << html_field(field)
+          end
+        end
+      end
+    end
+  end
+end

lib/gitlab/markdown_cache/redis/extension.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module MarkdownCache
+    module Redis
+      module Extension
+        extend ActiveSupport::Concern
+
+        attr_reader :cached_markdown_version
+
+        class_methods do
+          def cache_markdown_field(markdown_field, context = {})
+            super
+
+            # define the `[field]_html` accessor
+            html_field = cached_markdown_fields.html_field(markdown_field)
+            define_method(html_field) do
+              load_cached_markdown unless markdown_data_loaded?
+
+              instance_variable_get("@#{html_field}")
+            end
+          end
+        end
+
+        private
+
+        def save_markdown(updates)
+          markdown_store.save(updates)
+        end
+
+        def write_markdown_field(field_name, value)
+          instance_variable_set("@#{field_name}", value)
+        end
+
+        def markdown_field_changed?(field_name)
+          false
+        end
+
+        def changed_markdown_fields
+          []
+        end
+
+        def cached_markdown
+          @cached_data ||= markdown_store.read
+        end
+
+        def load_cached_markdown
+          cached_markdown.each do |field_name, value|
+            write_markdown_field(field_name, value)
+          end
+        end
+
+        def markdown_data_loaded?
+          cached_markdown_version.present? || markdown_store.loaded?
+        end
+
+        def markdown_store
+          @store ||= Gitlab::MarkdownCache::Redis::Store.new(self)
+        end
+      end
+    end
+  end
+end

lib/gitlab/markdown_cache/redis/store.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module MarkdownCache
+    module Redis
+      class Store
+        EXPIRES_IN = 1.day
+
+        def initialize(subject)
+          @subject = subject
+          @loaded = false
+        end
+
+        def save(updates)
+          @loaded = false
+
+          Gitlab::Redis::Cache.with do |r|
+            r.mapped_hmset(markdown_cache_key, updates)
+            r.expire(markdown_cache_key, EXPIRES_IN)
+          end
+        end
+
+        def read
+          @loaded = true
+
+          results = Gitlab::Redis::Cache.with do |r|
+            r.mapped_hmget(markdown_cache_key, *fields)
+          end
+          # The value read from redis is a string, so we're converting it back
+          # to an int.
+          results[:cached_markdown_version] = results[:cached_markdown_version].to_i
+          results
+        end
+
+        def loaded?
+          @loaded
+        end
+
+        private
+
+        def fields
+          @fields ||= @subject.cached_markdown_fields.html_fields + [:cached_markdown_version]
+        end
+
+        def markdown_cache_key
+          unless @subject.respond_to?(:cache_key)
+            raise Gitlab::MarkdownCache::UnsupportedClassError,
+                  "This class has no cache_key to use for caching"
+          end
+
+          "markdown_cache:#{@subject.cache_key}"
+        end
+      end
+    end
+  end
+end

spec/features/markdown/gitlab_flavored_markdown_spec.rb

@@ -20,8 +20,7 @@ describe "GitLab Flavored Markdown" do
     let(:commit) { project.commit }
 
     before do
-      allow_any_instance_of(Commit).to receive(:title)
-        .and_return("fix #{issue.to_reference}\n\nask #{fred.to_reference} for details")
+      create_commit("fix #{issue.to_reference}\n\nask #{fred.to_reference} for details", project, user, 'master')
     end
 
     it "renders title in commits#index" do

spec/helpers/markup_helper_spec.rb

@@ -78,7 +78,8 @@ describe MarkupHelper do
     let(:link)    { '/commits/0a1b2c3d' }
     let(:issues)  { create_list(:issue, 2, project: project) }
 
-    it 'handles references nested in links with all the text' do
+    # Clean the cache to make sure the title is re-rendered from the stubbed one
+    it 'handles references nested in links with all the text', :clean_gitlab_redis_cache do
       allow(commit).to receive(:title).and_return("This should finally fix #{issues[0].to_reference} and #{issues[1].to_reference} for real")
 
       actual = helper.link_to_markdown_field(commit, :title, link)

spec/lib/banzai/commit_renderer_spec.rb

 require 'spec_helper'
 
 describe Banzai::CommitRenderer do
-  describe '.render' do
+  describe '.render', :clean_gitlab_redis_cache do
     it 'renders a commit description and title' do
       user = build(:user)
       project = create(:project, :repository)
@@ -13,7 +13,7 @@ describe Banzai::CommitRenderer do
 
       described_class::ATTRIBUTES.each do |attr|
         expect_any_instance_of(Banzai::ObjectRenderer).to receive(:render).with([project.commit], attr).once.and_call_original
-        expect(Banzai::Renderer).to receive(:cacheless_render_field).with(project.commit, attr, {})
+        expect(Banzai::Renderer).to receive(:cacheless_render_field).with(project.commit, attr, { skip_project_check: false }).and_call_original
       end
 
       described_class.render([project.commit], project, user)

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_COMMONMARK_VERSION << 16) }
+  let(:object) { Note.new(note: 'hello', note_html: '<p dir="auto">hello</p>', cached_markdown_version: Gitlab::MarkdownCache::CACHE_COMMONMARK_VERSION << 16) }
 
   describe '#render' do
     context 'with cache' do
@@ -60,24 +60,38 @@ describe Banzai::ObjectRenderer do
     end
 
     context 'without cache' do
-      let(:commit) { project.commit }
+      let(:cacheless_class) do
+        Class.new do
+          attr_accessor :title, :redacted_title_html, :project
+
+          def banzai_render_context(field)
+            { project: project, pipeline: :single_line }
+          end
+        end
+      end
+      let(:cacheless_thing) do
+        cacheless_class.new.tap do |thing|
+          thing.title = "Merge branch 'branch-merged' into 'master'"
+          thing.project = project
+        end
+      end
 
       it 'renders and redacts an Array of objects' do
-        renderer.render([commit], :title)
+        renderer.render([cacheless_thing], :title)
 
-        expect(commit.redacted_title_html).to eq("Merge branch 'branch-merged' into 'master'")
+        expect(cacheless_thing.redacted_title_html).to eq("Merge branch 'branch-merged' into 'master'")
       end
 
       it 'calls Banzai::Redactor to perform redaction' do
         expect_any_instance_of(Banzai::Redactor).to receive(:redact).and_call_original
 
-        renderer.render([commit], :title)
+        renderer.render([cacheless_thing], :title)
       end
 
       it 'retrieves field content using Banzai::Renderer.cacheless_render_field' do
-        expect(Banzai::Renderer).to receive(:cacheless_render_field).with(commit, :title, {}).and_call_original
+        expect(Banzai::Renderer).to receive(:cacheless_render_field).with(cacheless_thing, :title, {}).and_call_original
 
-        renderer.render([commit], :title)
+        renderer.render([cacheless_thing], :title)
       end
     end
   end

spec/lib/banzai/renderer_spec.rb

@@ -11,16 +11,24 @@ describe Banzai::Renderer do
     object
   end
 
+  def fake_cacheless_object
+    object = double('cacheless object')
+
+    allow(object).to receive(:respond_to?).with(:cached_markdown_fields).and_return(false)
+
+    object
+  end
+
   describe '#render_field' do
     let(:renderer) { described_class }
 
     context 'without cache' do
-      let(:commit) { create(:project, :repository).commit }
+      let(:commit) { fake_cacheless_object }
 
       it 'returns cacheless render field' do
-        expect(renderer).to receive(:cacheless_render_field).with(commit, :title, {})
+        expect(renderer).to receive(:cacheless_render_field).with(commit, :field, {})
 
-        renderer.render_field(commit, :title)
+        renderer.render_field(commit, :field)
       end
     end
 

spec/lib/gitlab/markdown_cache/active_record/extension_spec.rb

+# frozen_string_literal: true
+require 'spec_helper'
+
+describe Gitlab::MarkdownCache::ActiveRecord::Extension do
+  let(:klass) do
+    Class.new(ActiveRecord::Base) do
+      self.table_name = 'issues'
+      include CacheMarkdownField
+      cache_markdown_field :title, whitelisted: true
+      cache_markdown_field :description, pipeline: :single_line
+
+      attr_accessor :author, :project
+    end
+  end
+
+  let(:cache_version) { Gitlab::MarkdownCache::CACHE_COMMONMARK_VERSION << 16 }
+  let(:thing) { klass.new(title: markdown, title_html: html, cached_markdown_version: cache_version) }
+
+  let(:markdown) { '`Foo`' }
+  let(:html) { '<p data-sourcepos="1:1-1:5" dir="auto"><code>Foo</code></p>' }
+
+  let(:updated_markdown) { '`Bar`' }
+  let(:updated_html) { '<p data-sourcepos="1:1-1:5" dir="auto"><code>Bar</code></p>' }
+
+  context 'an unchanged markdown field' do
+    let(:thing) { klass.new(title: markdown) }
+
+    before do
+      thing.title = thing.title
+      thing.save
+    end
+
+    it { expect(thing.title).to eq(markdown) }
+    it { expect(thing.title_html).to eq(html) }
+    it { expect(thing.title_html_changed?).not_to be_truthy }
+    it { expect(thing.cached_markdown_version).to eq(cache_version) }
+  end
+
+  context 'a changed markdown field' do
+    let(:thing) { klass.new(title: markdown, title_html: html, cached_markdown_version: cache_version) }
+
+    before do
+      thing.title = updated_markdown
+      thing.save
+    end
+
+    it { expect(thing.title_html).to eq(updated_html) }
+    it { expect(thing.cached_markdown_version).to eq(cache_version) }
+  end
+
+  context 'when a markdown field is set repeatedly to an empty string' do
+    it do
+      expect(thing).to receive(:refresh_markdown_cache).once
+      thing.title = ''
+      thing.save
+      thing.title = ''
+      thing.save
+    end
+  end
+
+  context 'when a markdown field is set repeatedly to a string which renders as empty html' do
+    it do
+      expect(thing).to receive(:refresh_markdown_cache).once
+      thing.title = '[//]: # (This is also a comment.)'
+      thing.save
+      thing.title = '[//]: # (This is also a comment.)'
+      thing.save
+    end
+  end
+
+  context 'a non-markdown field changed' do
+    let(:thing) { klass.new(title: markdown, title_html: html, cached_markdown_version: cache_version) }
+
+    before do
+      thing.state = 'closed'
+      thing.save
+    end
+
+    it { expect(thing.state).to eq('closed') }
+    it { expect(thing.title).to eq(markdown) }
+    it { expect(thing.title_html).to eq(html) }
+    it { expect(thing.cached_markdown_version).to eq(cache_version) }
+  end
+
+  context 'version is out of date' do
+    let(:thing) { klass.new(title: updated_markdown, title_html: html, cached_markdown_version: nil) }
+
+    before do
+      thing.save
+    end
+
+    it { expect(thing.title_html).to eq(updated_html) }
+    it { expect(thing.cached_markdown_version).to eq(cache_version) }
+  end
+
+  context 'when an invalidating field is changed' do
+    it 'invalidates the cache when project changes' do
+      thing.project = :new_project
+      allow(Banzai::Renderer).to receive(:cacheless_render_field).and_return(updated_html)
+
+      thing.save
+
+      expect(thing.title_html).to eq(updated_html)
+      expect(thing.description_html).to eq(updated_html)
+      expect(thing.cached_markdown_version).to eq(cache_version)
+    end
+
+    it 'invalidates the cache when author changes' do
+      thing.author = :new_author
+      allow(Banzai::Renderer).to receive(:cacheless_render_field).and_return(updated_html)
+
+      thing.save
+
+      expect(thing.title_html).to eq(updated_html)
+      expect(thing.description_html).to eq(updated_html)
+      expect(thing.cached_markdown_version).to eq(cache_version)
+    end
+  end
+
+  describe '.attributes' do
+    it 'excludes cache attributes that is blacklisted by default' do
+      expect(thing.attributes.keys.sort).not_to include(%w[description_html])
+    end
+  end
+
+  describe '#cached_html_up_to_date?' do
+    let(:thing) { klass.create(title: updated_markdown, title_html: html, cached_markdown_version: nil) }
+    subject { thing.cached_html_up_to_date?(:title) }
+
+    it 'returns false if markdown has been changed but html has not' do
+      thing.title = "changed!"
+
+      is_expected.to be_falsy
+    end
+
+    it 'returns true if markdown has not been changed but html has' do
+      thing.title_html = updated_html
+
+      is_expected.to be_truthy
+    end
+
+    it 'returns true if markdown and html have both been changed' do
+      thing.title = updated_markdown
+      thing.title_html = updated_html
+
+      is_expected.to be_truthy
+    end
+
+    it 'returns false if the markdown field is set but the html is not' do
+      thing.title_html = nil
+
+      is_expected.to be_falsy
+    end
+  end
+
+  describe '#refresh_markdown_cache!' do
+    before do
+      thing.title = updated_markdown
+    end
+
+    it 'skips saving if not persisted' do
+      expect(thing).to receive(:persisted?).and_return(false)
+      expect(thing).not_to receive(:update_columns)
+
+      thing.refresh_markdown_cache!
+    end
+
+    it 'saves the changes' do
+      expect(thing).to receive(:persisted?).and_return(true)
+
+      expect(thing).to receive(:update_columns)
+                         .with("title_html" => updated_html,
+                               "description_html" => "",
+                               "cached_markdown_version" => cache_version)
+
+      thing.refresh_markdown_cache!
+    end
+  end
+end

spec/lib/gitlab/markdown_cache/field_data_spec.rb

+# frozen_string_literal: true
+
+require 'fast_spec_helper'
+
+describe Gitlab::MarkdownCache::FieldData do
+  subject(:field_data) { described_class.new }
+
+  before do
+    field_data[:description] = { project: double('project in context') }
+  end
+
+  it 'translates a markdown field name into a html field name' do
+    expect(field_data.html_field(:description)).to eq("description_html")
+  end
+end

spec/lib/gitlab/markdown_cache/redis/extension_spec.rb

+# frozen_string_literal: true
+require 'spec_helper'
+
+describe Gitlab::MarkdownCache::Redis::Extension, :clean_gitlab_redis_cache do
+  let(:klass) do
+    Class.new do
+      include CacheMarkdownField
+
+      def initialize(title: nil, description: nil)
+        @title, @description = title, description
+      end
+
+      attr_reader :title, :description
+
+      cache_markdown_field :title, pipeline: :single_line
+      cache_markdown_field :description
+
+      def cache_key
+        "cache-key"
+      end
+    end
+  end
+
+  let(:cache_version) { Gitlab::MarkdownCache::CACHE_COMMONMARK_VERSION << 16 }
+  let(:thing) { klass.new(title: "`Hello`", description: "`World`") }
+  let(:expected_cache_key) { "markdown_cache:cache-key" }
+
+  it 'defines the html attributes' do
+    expect(thing).to respond_to(:title_html, :description_html, :cached_markdown_version)
+  end
+
+  it 'loads the markdown from the cache only once' do
+    expect(thing).to receive(:load_cached_markdown).once.and_call_original
+
+    thing.title_html
+    thing.description_html
+  end
+
+  it 'correctly loads the markdown if it was stored in redis' do
+    Gitlab::Redis::Cache.with do |r|
+      r.mapped_hmset(expected_cache_key,
+                     title_html: 'hello',
+                     description_html: 'world',
+                     cached_markdown_version: cache_version)
+    end
+
+    expect(thing.title_html).to eq('hello')
+    expect(thing.description_html).to eq('world')
+    expect(thing.cached_markdown_version).to eq(cache_version)
+  end
+
+  describe "#refresh_markdown_cache!" do
+    it "stores the value in redis" do
+      expected_results = { "title_html" => "`Hello`",
+                           "description_html" => "<p data-sourcepos=\"1:1-1:7\" dir=\"auto\"><code>World</code></p>",
+                           "cached_markdown_version" => cache_version.to_s }
+
+      thing.refresh_markdown_cache!
+
+      results = Gitlab::Redis::Cache.with do |r|
+        r.mapped_hmget(expected_cache_key,
+                       "title_html", "description_html", "cached_markdown_version")
+      end
+
+      expect(results).to eq(expected_results)
+    end
+
+    it "assigns the values" do
+      thing.refresh_markdown_cache!
+
+      expect(thing.title_html).to eq('`Hello`')
+      expect(thing.description_html).to eq("<p data-sourcepos=\"1:1-1:7\" dir=\"auto\"><code>World</code></p>")
+      expect(thing.cached_markdown_version).to eq(cache_version)
+    end
+  end
+end

spec/lib/gitlab/markdown_cache/redis/store_spec.rb

+# frozen_string_literal: true
+require 'spec_helper'
+
+describe Gitlab::MarkdownCache::Redis::Store, :clean_gitlab_redis_cache do
+  let(:storable_class) do
+    Class.new do
+      cattr_reader :cached_markdown_fields do
+        Gitlab::MarkdownCache::FieldData.new.tap do |field_data|
+          field_data[:field_1] = {}
+          field_data[:field_2] = {}
+        end
+      end
+
+      attr_accessor :field_1, :field_2, :field_1_html, :field_2_html, :cached_markdown_version
+
+      def cache_key
+        "cache-key"
+      end
+    end
+  end
+  let(:storable) { storable_class.new }
+  let(:cache_key) { "markdown_cache:#{storable.cache_key}" }
+
+  subject(:store) { described_class.new(storable) }
+
+  def read_values
+    Gitlab::Redis::Cache.with do |r|
+      r.mapped_hmget(cache_key,
+                     :field_1_html, :field_2_html, :cached_markdown_version)
+    end
+  end
+
+  def store_values(values)
+    Gitlab::Redis::Cache.with do |r|
+      r.mapped_hmset(cache_key,
+                     values)
+    end
+  end
+
+  describe '#save' do
+    it 'stores updates to html fields and version' do
+      values_to_store = { field_1_html: "hello", field_2_html: "world", cached_markdown_version: 1 }
+
+      store.save(values_to_store)
+
+      expect(read_values)
+        .to eq({ field_1_html: "hello", field_2_html: "world", cached_markdown_version: "1" })
+    end
+  end
+
+  describe '#read' do
+    it 'reads the html fields and version from redis if they were stored' do
+      stored_values = { field_1_html: "hello", field_2_html: "world", cached_markdown_version: 1 }
+
+      store_values(stored_values)
+
+      expect(store.read.symbolize_keys).to eq(stored_values)
+    end
+
+    it 'is mared loaded after reading' do
+      expect(store).not_to be_loaded
+
+      store.read
+
+      expect(store).to be_loaded
+    end
+  end
+end

spec/models/concerns/cache_markdown_field_spec.rb

@@ -2,178 +2,54 @@
 
 require 'spec_helper'
 
-describe CacheMarkdownField do
-  # The minimum necessary ActiveModel to test this concern
-  class ThingWithMarkdownFields
-    include ActiveModel::Model
-    include ActiveModel::Dirty
-
-    include ActiveModel::Serialization
-
-    class_attribute :attribute_names
-    self.attribute_names = []
-
-    def attributes
-      attribute_names.each_with_object({}) do |name, hsh|
-        hsh[name.to_s] = send(name)
-      end
-    end
-
-    extend ActiveModel::Callbacks
-    define_model_callbacks :create, :update
-
+describe CacheMarkdownField, :clean_gitlab_redis_cache do
+  let(:ar_class) do
+    Class.new(ActiveRecord::Base) do
+      self.table_name = 'issues'
       include CacheMarkdownField
-    cache_markdown_field :foo
-    cache_markdown_field :baz, pipeline: :single_line
-    cache_markdown_field :zoo, whitelisted: true
-
-    def self.add_attr(name)
-      self.attribute_names += [name]
-      define_attribute_methods(name)
-      attr_reader(name)
-      define_method("#{name}=") do |value|
-        write_attribute(name, value)
-      end
+      cache_markdown_field :title, pipeline: :single_line
+      cache_markdown_field :description
     end
-
-    add_attr :cached_markdown_version
-
-    [:foo, :foo_html, :bar, :baz, :baz_html, :zoo, :zoo_html].each do |name|
-      add_attr(name)
   end
 
-    def initialize(*)
-      super
+  let(:other_class) do
+    Class.new do
+      include CacheMarkdownField
 
-      # Pretend new is load
-      clear_changes_information
+      def initialize(args = {})
+        @title, @description, @cached_markdown_version = args[:title], args[:description], args[:cached_markdown_version]
+        @title_html, @description_html = args[:title_html], args[:description_html]
+        @author, @project = args[:author], args[:project]
       end
 
-    def read_attribute(name)
-      instance_variable_get("@#{name}")
-    end
+      attr_accessor :title, :description, :cached_markdown_version
 
-    def write_attribute(name, value)
-      send("#{name}_will_change!") unless value == read_attribute(name)
-      instance_variable_set("@#{name}", value)
-    end
+      cache_markdown_field :title, pipeline: :single_line
+      cache_markdown_field :description
 
-    def save
-      run_callbacks :update do
-        changes_applied
+      def cache_key
+        "cache-key"
       end
     end
-
-    def has_attribute?(attr_name)
-      attribute_names.include?(attr_name)
-    end
-  end
-
-  def thing_subclass(new_attr)
-    Class.new(ThingWithMarkdownFields) { add_attr(new_attr) }
   end
 
   let(:markdown) { '`Foo`' }
-  let(:html)     { '<p dir="auto"><code>Foo</code></p>' }
+  let(:html) { '<p data-sourcepos="1:1-1:5" dir="auto"><code>Foo</code></p>' }
 
   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: cache_version) }
-  let(:cache_version) { CacheMarkdownField::CACHE_COMMONMARK_VERSION << 16 }
-
-  before do
-    stub_commonmark_sourcepos_disabled
-  end
-
-  describe '.attributes' 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
-
-  context 'an unchanged markdown field' do
-    before do
-      thing.foo = thing.foo
-      thing.save
-    end
-
-    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(cache_version) }
-  end
-
-  context 'a changed markdown field' do
-    let(:thing) { ThingWithMarkdownFields.new(foo: markdown, foo_html: html, cached_markdown_version: cache_version - 1) }
-
-    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(cache_version) }
-  end
-
-  context 'when a markdown field is set repeatedly to an empty string' do
-    it do
-      expect(thing).to receive(:refresh_markdown_cache).once
-      thing.foo = ''
-      thing.save
-      thing.foo = ''
-      thing.save
-    end
-  end
-
-  context 'when a markdown field is set repeatedly to a string which renders as empty html' do
-    it do
-      expect(thing).to receive(:refresh_markdown_cache).once
-      thing.foo = '[//]: # (This is also a comment.)'
-      thing.save
-      thing.foo = '[//]: # (This is also a comment.)'
-      thing.save
-    end
-  end
-
-  context 'when a markdown field and html field are both changed' do
-    it do
-      expect(thing).not_to receive(:refresh_markdown_cache)
-      thing.foo = '_look over there!_'
-      thing.foo_html = '<em>look over there!</em>'
-      thing.save
-    end
-  end
-
-  context 'a non-markdown field changed' do
-    let(:thing) { ThingWithMarkdownFields.new(foo: markdown, foo_html: html, cached_markdown_version: cache_version - 1) }
+  let(:updated_html) { '<p data-sourcepos="1:1-1:5" dir="auto"><code>Bar</code></p>' }
 
-    before do
-      thing.bar = 'OK'
-      thing.save
-    end
+  let(:cache_version) { Gitlab::MarkdownCache::CACHE_COMMONMARK_VERSION << 16 }
 
-    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(cache_version) }
-  end
-
-  context 'version is out of date' do
-    let(:thing) { ThingWithMarkdownFields.new(foo: updated_markdown, foo_html: html, cached_markdown_version: nil) }
-
-    before do
-      thing.save
-    end
-
-    it { expect(thing.foo_html).to eq(updated_html) }
-    it { expect(thing.cached_markdown_version).to eq(cache_version) }
+  def thing_subclass(klass, extra_attribute)
+    Class.new(klass) { attr_accessor(extra_attribute) }
   end
 
+  shared_examples 'a class with cached markdown fields' do
     describe '#cached_html_up_to_date?' do
-    let(:thing) { ThingWithMarkdownFields.new(foo: markdown, foo_html: html, cached_markdown_version: cache_version) }
+      let(:thing) { klass.new(title: markdown, title_html: html, cached_markdown_version: cache_version) }
 
-    subject { thing.cached_html_up_to_date?(:foo) }
+      subject { thing.cached_html_up_to_date?(:title) }
 
       it 'returns false when the version is absent' do
         thing.cached_markdown_version = nil
@@ -181,14 +57,14 @@ describe CacheMarkdownField do
         is_expected.to be_falsy
       end
 
-    it 'returns false when the cached version is too old' do
-      thing.cached_markdown_version = cache_version - 1
+      it 'returns false when the version is too early' do
+        thing.cached_markdown_version -= 1
 
         is_expected.to be_falsy
       end
 
-    it 'returns false when the cached version is in future' do
-      thing.cached_markdown_version = cache_version + 1
+      it 'returns false when the version is too late' do
+        thing.cached_markdown_version += 1
 
         is_expected.to be_falsy
       end
@@ -212,34 +88,10 @@ describe CacheMarkdownField do
 
         is_expected.to be_truthy
       end
-
-    it 'returns false if markdown has been changed but html has not' do
-      thing.foo = updated_html
-
-      is_expected.to be_falsy
-    end
-
-    it 'returns true if markdown has not been changed but html has' do
-      thing.foo_html = updated_html
-
-      is_expected.to be_truthy
-    end
-
-    it 'returns true if markdown and html have both been changed' do
-      thing.foo = updated_markdown
-      thing.foo_html = updated_html
-
-      is_expected.to be_truthy
-    end
-
-    it 'returns false if the markdown field is set but the html is not' do
-      thing.foo_html = nil
-
-      is_expected.to be_falsy
-    end
     end
 
     describe '#latest_cached_markdown_version' do
+      let(:thing) { klass.new }
       subject { thing.latest_cached_markdown_version }
 
       it 'returns default version' do
@@ -249,20 +101,20 @@ describe CacheMarkdownField do
     end
 
     describe '#refresh_markdown_cache' do
+      let(:thing) { klass.new(description: markdown, description_html: html, cached_markdown_version: cache_version) }
+
       before do
-      thing.foo = updated_markdown
+        thing.description = updated_markdown
       end
 
       it 'fills all html fields' do
         thing.refresh_markdown_cache
 
-      expect(thing.foo_html).to eq(updated_html)
-      expect(thing.foo_html_changed?).to be_truthy
-      expect(thing.baz_html_changed?).to be_truthy
+        expect(thing.description_html).to eq(updated_html)
       end
 
       it 'does not save the result' do
-      expect(thing).not_to receive(:update_columns)
+        expect(thing).not_to receive(:save_markdown)
 
         thing.refresh_markdown_cache
       end
@@ -276,43 +128,30 @@ describe CacheMarkdownField do
     end
 
     describe '#refresh_markdown_cache!' do
-    let(:thing) { ThingWithMarkdownFields.new(foo: markdown, foo_html: html, cached_markdown_version: cache_version) }
+      let(:thing) { klass.new(description: markdown, description_html: html, cached_markdown_version: cache_version) }
 
       before do
-      thing.foo = updated_markdown
+        thing.description = updated_markdown
       end
 
       it 'fills all html fields' do
         thing.refresh_markdown_cache!
 
-      expect(thing.foo_html).to eq(updated_html)
-      expect(thing.foo_html_changed?).to be_truthy
-      expect(thing.baz_html_changed?).to be_truthy
+        expect(thing.description_html).to eq(updated_html)
       end
 
-    it 'skips saving if not persisted' do
-      expect(thing).to receive(:persisted?).and_return(false)
-      expect(thing).not_to receive(:update_columns)
-
-      thing.refresh_markdown_cache!
-    end
-
-    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" => "",
-          "zoo_html" => "",
-          "cached_markdown_version" => cache_version
-        )
+      it 'saves the changes' do
+        expect(thing)
+          .to receive(:save_markdown)
+          .with("description_html" => updated_html, "title_html" => "", "cached_markdown_version" => cache_version)
 
         thing.refresh_markdown_cache!
       end
     end
 
     describe '#banzai_render_context' do
-    subject(:context) { thing.banzai_render_context(:foo) }
+      let(:thing) { klass.new(title: markdown, title_html: html, cached_markdown_version: cache_version) }
+      subject(:context) { thing.banzai_render_context(:title) }
 
       it 'sets project to nil if the object lacks a project' do
         is_expected.to have_key(:project)
@@ -328,57 +167,48 @@ describe CacheMarkdownField do
       end
 
       it 'includes the pipeline' do
-      baz = thing.banzai_render_context(:baz)
+        title_context = thing.banzai_render_context(:title)
 
-      expect(baz[:pipeline]).to eq(:single_line)
+        expect(title_context[:pipeline]).to eq(:single_line)
       end
 
       it 'returns copies of the context template' do
-      template = thing.cached_markdown_fields[:baz]
-      copy = thing.banzai_render_context(:baz)
+        template = thing.cached_markdown_fields[:description]
+        copy = thing.banzai_render_context(:description)
 
         expect(copy).not_to be(template)
       end
 
       context 'with a project' do
-      let(:project) { create(:project, group: create(:group)) }
-      let(:thing) { thing_subclass(:project).new(foo: markdown, foo_html: html, project: project) }
+        let(:project) { build(:project, group: create(:group)) }
+        let(:thing) { thing_subclass(klass, :project).new(title: markdown, title_html: html, project: project) }
 
         it 'sets the project in the context' do
           is_expected.to have_key(:project)
           expect(context[:project]).to eq(project)
         end
-
-      it 'invalidates the cache when project changes' do
-        thing.project = :new_project
-        allow(Banzai::Renderer).to receive(:cacheless_render_field).and_return(updated_html)
-
-        thing.save
-
-        expect(thing.foo_html).to eq(updated_html)
-        expect(thing.baz_html).to eq(updated_html)
-        expect(thing.cached_markdown_version).to eq(cache_version)
-      end
       end
 
       context 'with an author' do
-      let(:thing) { thing_subclass(:author).new(foo: markdown, foo_html: html, author: :author_value) }
+        let(:thing) { thing_subclass(klass, :author).new(title: markdown, title_html: html, author: :author_value) }
 
         it 'sets the author in the context' do
           is_expected.to have_key(:author)
           expect(context[:author]).to eq(:author_value)
         end
+      end
+    end
+  end
 
-      it 'invalidates the cache when author changes' do
-        thing.author = :new_author
-        allow(Banzai::Renderer).to receive(:cacheless_render_field).and_return(updated_html)
-
-        thing.save
+  context 'for Active record classes' do
+    let(:klass) { ar_class }
 
-        expect(thing.foo_html).to eq(updated_html)
-        expect(thing.baz_html).to eq(updated_html)
-        expect(thing.cached_markdown_version).to eq(cache_version)
-      end
+    it_behaves_like 'a class with cached markdown fields'
   end
+
+  context 'for other classes' do
+    let(:klass) { other_class }
+
+    it_behaves_like 'a class with cached markdown fields'
   end
 end

spec/models/resource_label_event_spec.rb

@@ -82,13 +82,13 @@ RSpec.describe ResourceLabelEvent, type: :model do
     end
 
     it 'returns true if markdown is outdated' do
-      subject.attributes = { cached_markdown_version: ((CacheMarkdownField::CACHE_COMMONMARK_VERSION - 1) << 16) | 0 }
+      subject.attributes = { cached_markdown_version: ((Gitlab::MarkdownCache::CACHE_COMMONMARK_VERSION - 1) << 16) | 0 }
 
       expect(subject.outdated_markdown?).to be true
     end
 
     it 'returns false if label and reference are set' do
-      subject.attributes = { reference: 'whatever', cached_markdown_version: CacheMarkdownField::CACHE_COMMONMARK_VERSION << 16 }
+      subject.attributes = { reference: 'whatever', cached_markdown_version: Gitlab::MarkdownCache::CACHE_COMMONMARK_VERSION << 16 }
 
       expect(subject.outdated_markdown?).to be false
     end