Merge branch '18337-cache-html-in-database' into 'master'

Cache rendered Markdown fields in the database

## What does this MR do?

Introduces cache fields for Markdown-containing fields in the database, and populates them.

## Why was this MR needed?

Rendering Markdown into HTML is performance-intensive. A Redis cache already exists, but this approach is expected to be more performant and reduce unnecessary cache invalidations.

## What are the relevant issue numbers?

Closes #18337

See merge request !6095

CHANGELOG

@@ -15,6 +15,7 @@ v 8.13.0 (unreleased)
   - Keep refs for each deployment
   - Log LDAP lookup errors and don't swallow unrelated exceptions. !6103 (Markus Koller)
   - Add more tests for calendar contribution (ClemMakesApps)
+  - Cache rendered markdown in the database, rather than Redis
   - Avoid database queries on Banzai::ReferenceParser::BaseParser for nodes without references
   - Simplify Mentionable concern instance methods
   - Fix permission for setting an issue's due date

Gemfile

@@ -110,6 +110,7 @@ gem 'creole',             '~> 0.5.0'
 gem 'wikicloth',          '0.8.1'
 gem 'asciidoctor',        '~> 1.5.2'
 gem 'rouge',              '~> 2.0'
+gem 'truncato',           '~> 0.7.8'
 # See https://groups.google.com/forum/#!topic/ruby-security-ann/aSbgDiwb24s
 # and https://groups.google.com/forum/#!topic/ruby-security-ann/Dy7YiKb_pMM

Gemfile.lock

@@ -745,6 +745,9 @@ GEM
     tilt (2.0.5)
     timecop (0.8.1)
     timfel-krb5-auth (0.8.3)
+    truncato (0.7.8)
+      htmlentities (~> 4.3.1)
+      nokogiri (~> 1.6.1)
     turbolinks (2.5.3)
       coffee-rails
     tzinfo (1.2.2)
@@ -971,6 +974,7 @@ DEPENDENCIES
   test_after_commit (~> 0.4.2)
   thin (~> 1.7.0)
   timecop (~> 0.8.0)
+  truncato (~> 0.7.8)
   turbolinks (~> 2.5.0)
   u2f (~> 0.2.1)
   uglifier (~> 2.7.2)

app/controllers/admin/broadcast_messages_controller.rb

@@ -37,7 +37,7 @@ class Admin::BroadcastMessagesController < Admin::ApplicationController
   end
   def preview
-    @message = broadcast_message_params[:message]
+    @broadcast_message = BroadcastMessage.new(broadcast_message_params)
   end
   protected

app/helpers/appearances_helper.rb

@@ -16,7 +16,7 @@ module AppearancesHelper
   end
   def brand_text
-    markdown(brand_item.description)
+    markdown_field(brand_item, :description)
   end
   def brand_item

app/helpers/application_settings_helper.rb

@@ -11,18 +11,6 @@ module ApplicationSettingsHelper
     current_application_settings.signin_enabled?
   end
-  def extra_sign_in_text
-    current_application_settings.sign_in_text
-  end
-
-  def after_sign_up_text
-    current_application_settings.after_sign_up_text
-  end
-
-  def shared_runners_text
-    current_application_settings.shared_runners_text
-  end
-
   def user_oauth_applications?
     current_application_settings.user_oauth_applications
   end

app/helpers/broadcast_messages_helper.rb

@@ -3,7 +3,7 @@ module BroadcastMessagesHelper
     return unless message.present?
     content_tag :div, class: 'broadcast-message', style: broadcast_message_style(message) do
-      icon('bullhorn') << ' ' << render_broadcast_message(message.message)
+      icon('bullhorn') << ' ' << render_broadcast_message(message)
     end
   end
@@ -32,7 +32,7 @@ module BroadcastMessagesHelper
     end
   end
-  def render_broadcast_message(message)
-    Banzai.render(message, pipeline: :broadcast_message).html_safe
+  def render_broadcast_message(broadcast_message)
+    Banzai.render_field(broadcast_message, :message).html_safe
   end
 end

app/helpers/gitlab_markdown_helper.rb

@@ -13,14 +13,12 @@ module GitlabMarkdownHelper
   def link_to_gfm(body, url, html_options = {})
     return "" if body.blank?
-    escaped_body = if body.start_with?('<img')
-                     body
-                   else
-                     escape_once(body)
-                   end
-
-    user = current_user if defined?(current_user)
-    gfm_body = Banzai.render(escaped_body, project: @project, current_user: user, pipeline: :single_line)
+    context = {
+      project: @project,
+      current_user: (current_user if defined?(current_user)),
+      pipeline: :single_line,
+    }
+    gfm_body = Banzai.render(body, context)
     fragment = Nokogiri::HTML::DocumentFragment.parse(gfm_body)
     if fragment.children.size == 1 && fragment.children[0].name == 'a'
@@ -51,17 +49,15 @@ module GitlabMarkdownHelper
     context[:project] ||= @project
     html = Banzai.render(text, context)
-    context.merge!(
-      current_user:   (current_user if defined?(current_user)),
-
-      # RelativeLinkFilter
-      requested_path: @path,
-      project_wiki:   @project_wiki,
-      ref:            @ref
-    )
-    Banzai.post_process(html, context)
+    banzai_postprocess(html, context)
+  end
+  def markdown_field(object, field)
+    object = object.for_display if object.respond_to?(:for_display)
+    return "" unless object.present?
+    html = Banzai.render_field(object, field)
+    banzai_postprocess(html, object.banzai_render_context(field))
   end
   def asciidoc(text)
@@ -196,4 +192,18 @@ module GitlabMarkdownHelper
       icon(options[:icon])
     end
   end
+
+  # Calls Banzai.post_process with some common context options
+  def banzai_postprocess(html, context)
+    context.merge!(
+      current_user:   (current_user if defined?(current_user)),
+
+      # RelativeLinkFilter
+      requested_path: @path,
+      project_wiki:   @project_wiki,
+      ref:            @ref
+    )
+
+    Banzai.post_process(html, context)
+  end
 end

app/helpers/search_helper.rb

@@ -153,8 +153,18 @@ module SearchHelper
     search_path(options)
   end
-  # Sanitize html generated after parsing markdown from issue description or comment
-  def search_md_sanitize(html)
+  # Sanitize a HTML field for search display. Most tags are stripped out and the
+  # maximum length is set to 200 characters.
+  def search_md_sanitize(object, field)
+    html = markdown_field(object, field)
+    html = Truncato.truncate(
+      html,
+      count_tags: false,
+      count_tail: false,
+      max_length: 200
+    )
+
+    # Truncato's filtered_tags and filtered_attributes are not quite the same
     sanitize(html, tags: %w(a p ol ul li pre code))
   end
 end

app/models/abuse_report.rb

 class AbuseReport < ActiveRecord::Base
+  include CacheMarkdownField
+
+  cache_markdown_field :message, pipeline: :single_line
+
   belongs_to :reporter, class_name: 'User'
   belongs_to :user
@@ -7,6 +11,9 @@ class AbuseReport < ActiveRecord::Base
   validates :message, presence: true
   validates :user_id, uniqueness: { message: 'has already been reported' }
+  # For CacheMarkdownField
+  alias_method :author, :reporter
+
   def remove_user(deleted_by:)
     user.block
     DeleteUserWorker.perform_async(deleted_by.id, user.id, delete_solo_owned_groups: true)

app/models/appearance.rb

 class Appearance < ActiveRecord::Base
+  include CacheMarkdownField
+
+  cache_markdown_field :description
+
   validates :title,       presence: true
   validates :description, presence: true
   validates :logo,        file_size: { maximum: 1.megabyte }

app/models/application_setting.rb

 class ApplicationSetting < ActiveRecord::Base
+  include CacheMarkdownField
   include TokenAuthenticatable
+
   add_authentication_token_field :runners_registration_token
   add_authentication_token_field :health_check_access_token
@@ -17,6 +19,11 @@ class ApplicationSetting < ActiveRecord::Base
   serialize :domain_whitelist, Array
   serialize :domain_blacklist, Array
+  cache_markdown_field :sign_in_text
+  cache_markdown_field :help_page_text
+  cache_markdown_field :shared_runners_text, pipeline: :plain_markdown
+  cache_markdown_field :after_sign_up_text
+
   attr_accessor :domain_whitelist_raw, :domain_blacklist_raw
   validates :session_expire_delay,

app/models/broadcast_message.rb

 class BroadcastMessage < ActiveRecord::Base
+  include CacheMarkdownField
   include Sortable
+  cache_markdown_field :message, pipeline: :broadcast_message
+
   validates :message,   presence: true
   validates :starts_at, presence: true
   validates :ends_at,   presence: true

app/models/concerns/cache_markdown_field.rb

+# This module takes care of updating cache columns for Markdown-containing
+# fields. Use like this in the body of your class:
+#
+#     include CacheMarkdownField
+#     cache_markdown_field :foo
+#     cache_markdown_field :bar
+#     cache_markdown_field :baz, pipeline: :single_line
+#
+# Corresponding foo_html, bar_html and baz_html fields should exist.
+module CacheMarkdownField
+  # Knows about the relationship between markdown and html field names, and
+  # stores the rendering contexts for the latter
+  class FieldData
+    extend Forwardable
+
+    def initialize
+      @data = {}
+    end
+
+    def_delegators :@data, :[], :[]=
+    def_delegator :@data, :keys, :markdown_fields
+
+    def html_field(markdown_field)
+      "#{markdown_field}_html"
+    end
+
+    def html_fields
+      markdown_fields.map {|field| html_field(field) }
+    end
+  end
+
+  # Dynamic registries don't really work in Rails as it's not guaranteed that
+  # every class will be loaded, so hardcode the list.
+  CACHING_CLASSES = %w[
+    AbuseReport
+    Appearance
+    ApplicationSetting
+    BroadcastMessage
+    Issue
+    Label
+    MergeRequest
+    Milestone
+    Namespace
+    Note
+    Project
+    Release
+    Snippet
+  ]
+
+  def self.caching_classes
+    CACHING_CLASSES.map(&:constantize)
+  end
+
+  extend ActiveSupport::Concern
+
+  included do
+    cattr_reader :cached_markdown_fields do
+      FieldData.new
+    end
+
+    # Returns the default Banzai render context for the cached markdown field.
+    def banzai_render_context(field)
+      raise ArgumentError.new("Unknown field: #{field.inspect}") unless
+        cached_markdown_fields.markdown_fields.include?(field)
+
+      # Always include a project key, or Banzai complains
+      project = self.project if self.respond_to?(:project)
+      context = cached_markdown_fields[field].merge(project: project)
+
+      # Banzai is less strict about authors, so don't always have an author key
+      context[:author] = self.author if self.respond_to?(:author)
+
+      context
+    end
+
+    # Allow callers to look up the cache field name, rather than hardcoding it
+    def markdown_cache_field_for(field)
+      raise ArgumentError.new("Unknown field: #{field}") unless
+        cached_markdown_fields.markdown_fields.include?(field)
+
+      cached_markdown_fields.html_field(field)
+    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
+
+      cached_markdown_fields.html_fields.each do |field|
+        attrs.delete(field)
+      end
+
+      attrs
+    end
+  end
+
+  class_methods do
+    private
+
+    # Specify that a field is markdown. Its rendered output will be cached in
+    # a corresponding _html field. Any custom rendering options may be provided
+    # as a context.
+    def cache_markdown_field(markdown_field, context = {})
+      raise "Add #{self} to CacheMarkdownField::CACHING_CLASSES" unless
+        CacheMarkdownField::CACHING_CLASSES.include?(self.to_s)
+
+      cached_markdown_fields[markdown_field] = context
+
+      html_field = cached_markdown_fields.html_field(markdown_field)
+      cache_method = "#{markdown_field}_cache_refresh".to_sym
+      invalidation_method = "#{html_field}_invalidated?".to_sym
+
+      define_method(cache_method) do
+        html = Banzai::Renderer.cacheless_render_field(self, markdown_field)
+        __send__("#{html_field}=", html)
+        true
+      end
+
+      # 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, "author", "project"]
+        !invalidations.empty?
+      end
+
+      before_save cache_method, if: invalidation_method
+    end
+  end
+end

app/models/concerns/issuable.rb

@@ -6,6 +6,7 @@
 #
 module Issuable
   extend ActiveSupport::Concern
+  include CacheMarkdownField
   include Participable
   include Mentionable
   include Subscribable
@@ -13,6 +14,9 @@ module Issuable
   include Awardable
   included do
+    cache_markdown_field :title, pipeline: :single_line
+    cache_markdown_field :description
+
     belongs_to :author, class_name: "User"
     belongs_to :assignee, class_name: "User"
     belongs_to :updated_by, class_name: "User"

app/models/global_label.rb

@@ -4,6 +4,10 @@ class GlobalLabel
   delegate :color, :description, to: :@first_label
+  def for_display
+    @first_label
+  end
+
   def self.build_collection(labels)
     labels = labels.group_by(&:title)

app/models/global_milestone.rb

@@ -4,6 +4,10 @@ class GlobalMilestone
   attr_accessor :title, :milestones
   alias_attribute :name, :title
+  def for_display
+    @first_milestone
+  end
+
   def self.build_collection(milestones)
     milestones = milestones.group_by(&:title)
@@ -17,6 +21,7 @@ class GlobalMilestone
     @title = title
     @name = title
     @milestones = milestones
+    @first_milestone = milestones.find {|m| m.description.present? } || milestones.first
   end
   def safe_title

app/models/label.rb

 class Label < ActiveRecord::Base
+  include CacheMarkdownField
   include Referable
   include Subscribable
@@ -8,6 +9,8 @@ class Label < ActiveRecord::Base
   None = LabelStruct.new('No Label', 'No Label')
   Any = LabelStruct.new('Any Label', '')
+  cache_markdown_field :description, pipeline: :single_line
+
   DEFAULT_COLOR = '#428BCA'
   default_value_for :color, DEFAULT_COLOR

app/models/milestone.rb

@@ -6,12 +6,16 @@ class Milestone < ActiveRecord::Base
   Any = MilestoneStruct.new('Any Milestone', '', -1)
   Upcoming = MilestoneStruct.new('Upcoming', '#upcoming', -2)
+  include CacheMarkdownField
   include InternalId
   include Sortable
   include Referable
   include StripAttribute
   include Milestoneish
+  cache_markdown_field :title, pipeline: :single_line
+  cache_markdown_field :description
+
   belongs_to :project
   has_many :issues
   has_many :labels, -> { distinct.reorder('labels.title') },  through: :issues

app/models/namespace.rb

 class Namespace < ActiveRecord::Base
   acts_as_paranoid
+  include CacheMarkdownField
   include Sortable
   include Gitlab::ShellAdapter
+  cache_markdown_field :description, pipeline: :description
+
   has_many :projects, dependent: :destroy
   belongs_to :owner, class_name: "User"