Banzai - avoid redis if attr is in DB cache

When cache_collection_render runs we end up reading and writing
things to redis even if we already have the rendered field cached
in the DB. This commit avoids using redis at all whenever we have
the field already rendered in the DB cache.

app/models/concerns/cache_markdown_field.rb

@@ -87,6 +87,16 @@ module CacheMarkdownField
     __send__(cached_markdown_fields.html_field(markdown_field)) # rubocop:disable GitlabSecurity/PublicSend
   end
 
+  # Updates the markdown cache if necessary, then returns the field
+  # Unlike `cached_html_for` it returns `nil` if the field does not exist
+  def updated_cached_html_for(markdown_field)
+    return unless cached_markdown_fields.markdown_fields.include?(markdown_field)
+
+    refresh_markdown_cache if attribute_invalidated?(cached_markdown_fields.html_field(markdown_field))
+
+    cached_html_for(markdown_field)
+  end
+
   def latest_cached_markdown_version
     @latest_cached_markdown_version ||= (Gitlab::MarkdownCache::CACHE_COMMONMARK_VERSION << 16) | local_version
   end
@@ -139,8 +149,9 @@ 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
-        invalidations = changed_markdown_fields & [markdown_field.to_s, *INVALIDATED_BY]
-        invalidations.delete(markdown_field.to_s) if changed_markdown_fields.include?("#{markdown_field}_html")
+        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.empty? || !cached_html_up_to_date?(markdown_field)
       end
     end

app/models/concerns/mentionable.rb

@@ -63,6 +63,9 @@ module Mentionable
         skip_project_check: skip_project_check?
       ).merge(mentionable_params)
 
+      cached_html = self.try(:updated_cached_html_for, attr.to_sym)
+      options[:rendered] = cached_html if cached_html
+
       extractor.analyze(text, options)
     end
 

lib/banzai/renderer.rb

@@ -55,11 +55,16 @@ module Banzai
     # Perform multiple render from an Array of Markdown String into an
     # Array of HTML-safe String of HTML.
     #
-    # As the rendered Markdown String can be already cached read all the data
-    # from the cache using Rails.cache.read_multi operation. If the Markdown String
-    # is not in the cache or it's not cacheable (no cache_key entry is provided in
-    # the context) the Markdown String is rendered and stored in the cache so the
-    # next render call gets the rendered HTML-safe String from the cache.
+    # The redis cache is completely obviated if we receive a `:rendered` key in the
+    # context, as it is assumed the item has been pre-rendered somewhere else and there
+    # is no need to cache it.
+    #
+    # If no `:rendered` key is present in the context, as the rendered Markdown String
+    # can be already cached, read all the data from the cache using
+    # Rails.cache.read_multi operation. If the Markdown String is not in the cache
+    # or it's not cacheable (no cache_key entry is provided in the context) the
+    # Markdown String is rendered and stored in the cache so the next render call
+    # gets the rendered HTML-safe String from the cache.
     #
     # For further explanation see #render method comments.
     #
@@ -76,19 +81,34 @@ module Banzai
     #    => [{ text: '### Hello',
     #          context: { cache_key: [note, :note] } }]
     def self.cache_collection_render(texts_and_contexts)
-      items_collection = texts_and_contexts.each_with_index do |item, index|
+      items_collection = texts_and_contexts.each do |item|
         context = item[:context]
-        cache_key = full_cache_multi_key(context.delete(:cache_key), context[:pipeline])
 
+        if context.key?(:rendered)
+          item[:rendered] = context.delete(:rendered)
+        else
+          # If the attribute didn't come in pre-rendered, let's prepare it for caching it in redis
+          cache_key = full_cache_multi_key(context.delete(:cache_key), context[:pipeline])
           item[:cache_key] = cache_key if cache_key
         end
+      end
 
-      cacheable_items, non_cacheable_items = items_collection.partition { |item| item.key?(:cache_key) }
+      cacheable_items, non_cacheable_items = items_collection.group_by do |item|
+        if item.key?(:rendered)
+          # We're not really doing anything here as these don't need any processing, but leaving it just in case
+          # as they could have a cache_key and we don't want them to be re-rendered
+          :rendered
+        elsif item.key?(:cache_key)
+          :cacheable
+        else
+          :non_cacheable
+        end
+      end.values_at(:cacheable, :non_cacheable)
 
       items_in_cache = []
       items_not_in_cache = []
 
-      unless cacheable_items.empty?
+      if cacheable_items.present?
         items_in_cache = Rails.cache.read_multi(*cacheable_items.map { |item| item[:cache_key] })
         items_not_in_cache = cacheable_items.reject do |item|
           item[:rendered] = items_in_cache[item[:cache_key]]
@@ -96,7 +116,7 @@ module Banzai
         end
       end
 
-      (items_not_in_cache + non_cacheable_items).each do |item|
+      (items_not_in_cache + Array.wrap(non_cacheable_items)).each do |item|
         item[:rendered] = render(item[:text], item[:context])
         Rails.cache.write(item[:cache_key], item[:rendered]) if item[:cache_key]
       end

lib/gitlab/markdown_cache/active_record/extension.rb

@@ -26,10 +26,6 @@ module Gitlab
           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

lib/gitlab/markdown_cache/redis/extension.rb

@@ -36,8 +36,8 @@ module Gitlab
           false
         end
 
-        def changed_markdown_fields
-          []
+        def changed_attributes
+          {}
         end
 
         def cached_markdown

spec/lib/banzai/renderer_spec.rb

@@ -19,6 +19,24 @@ describe Banzai::Renderer do
     object
   end
 
+  describe '#cache_collection_render' do
+    let(:merge_request) { fake_object(fresh: true) }
+    let(:context) { { cache_key: [merge_request, 'field'], rendered: merge_request.field_html } }
+
+    context 'when an item has a rendered field' do
+      before do
+        allow(merge_request).to receive(:field).and_return('This is the field')
+        allow(merge_request).to receive(:field_html).and_return('This is the field')
+      end
+
+      it 'does not touch redis if the field is in the cache' do
+        expect(Rails).not_to receive(:cache)
+
+        described_class.cache_collection_render([{ text: merge_request.field, context: context }])
+      end
+    end
+  end
+
   describe '#render_field' do
     let(:renderer) { described_class }
 

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

@@ -9,12 +9,13 @@ describe Gitlab::MarkdownCache::ActiveRecord::Extension do
       cache_markdown_field :title, whitelisted: true
       cache_markdown_field :description, pipeline: :single_line
 
-      attr_accessor :author, :project
+      attribute :author
+      attribute :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(:thing) { klass.create(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>' }
@@ -37,7 +38,7 @@ describe Gitlab::MarkdownCache::ActiveRecord::Extension do
   end
 
   context 'a changed markdown field' do
-    let(:thing) { klass.new(title: markdown, title_html: html, cached_markdown_version: cache_version) }
+    let(:thing) { klass.create(title: markdown, title_html: html, cached_markdown_version: cache_version) }
 
     before do
       thing.title = updated_markdown

spec/models/commit_range_spec.rb

@@ -139,8 +139,8 @@ describe CommitRange do
   end
 
   describe '#has_been_reverted?' do
-    let(:issue) { create(:issue) }
-    let(:user) { issue.author }
+    let(:user) { create(:user) }
+    let(:issue) { create(:issue, author: user, project: project) }
 
     it 'returns true if the commit has been reverted' do
       create(:note_on_issue,
@@ -149,9 +149,11 @@ describe CommitRange do
              note: commit1.revert_description(user),
              project: issue.project)
 
-      expect_any_instance_of(Commit).to receive(:reverts_commit?)
+      expect_next_instance_of(Commit) do |commit|
+        expect(commit).to receive(:reverts_commit?)
           .with(commit1, user)
           .and_return(true)
+      end
 
       expect(commit1.has_been_reverted?(user, issue.notes_with_associations)).to eq(true)
     end

spec/models/concerns/cache_markdown_field_spec.rb

@@ -198,6 +198,36 @@ describe CacheMarkdownField, :clean_gitlab_redis_cache do
         end
       end
     end
+
+    describe '#updated_cached_html_for' do
+      let(:thing) { klass.new(description: markdown, description_html: html, cached_markdown_version: cache_version) }
+
+      context 'when the markdown cache is outdated' do
+        before do
+          thing.cached_markdown_version += 1
+        end
+
+        it 'calls #refresh_markdown_cache' do
+          expect(thing).to receive(:refresh_markdown_cache)
+
+          expect(thing.updated_cached_html_for(:description)).to eq(html)
+        end
+      end
+
+      context 'when the markdown field does not exist' do
+        it 'returns nil' do
+          expect(thing.updated_cached_html_for(:something)).to eq(nil)
+        end
+      end
+
+      context 'when the markdown cache is up to date' do
+        it 'does not call #refresh_markdown_cache' do
+          expect(thing).not_to receive(:refresh_markdown_cache)
+
+          expect(thing.updated_cached_html_for(:description)).to eq(html)
+        end
+      end
+    end
   end
 
   context 'for Active record classes' do

spec/models/note_spec.rb

@@ -177,6 +177,7 @@ describe Note do
             pipeline: :note,
             cache_key: [note1, "note"],
             project: note1.project,
+            rendered: note1.note_html,
             author: note1.author
           }
         }]).and_call_original
@@ -189,6 +190,7 @@ describe Note do
             pipeline: :note,
             cache_key: [note2, "note"],
             project: note2.project,
+            rendered: note2.note_html,
             author: note2.author
           }
         }]).and_call_original

spec/services/notification_service_spec.rb

@@ -215,13 +215,14 @@ describe NotificationService, :mailer do
       let(:project) { create(:project, :private) }
       let(:issue) { create(:issue, project: project, assignees: [assignee]) }
       let(:mentioned_issue) { create(:issue, assignees: issue.assignees) }
-      let(:note) { create(:note_on_issue, noteable: issue, project_id: issue.project_id, note: '@mention referenced, @unsubscribed_mentioned and @outsider also') }
+      let(:author) { create(:user) }
+      let(:note) { create(:note_on_issue, author: author, noteable: issue, project_id: issue.project_id, note: '@mention referenced, @unsubscribed_mentioned and @outsider also') }
 
       before do
-        build_team(note.project)
+        build_team(project)
         project.add_maintainer(issue.author)
         project.add_maintainer(assignee)
-        project.add_maintainer(note.author)
+        project.add_maintainer(author)
 
         @u_custom_off = create_user_with_notification(:custom, 'custom_off')
         project.add_guest(@u_custom_off)
@@ -240,7 +241,8 @@ describe NotificationService, :mailer do
 
       describe '#new_note' do
         it do
-          add_users_with_subscription(note.project, issue)
+          add_users(project)
+          add_user_subscriptions(issue)
           reset_delivered_emails!
 
           expect(SentNotification).to receive(:record).with(issue, any_args).exactly(10).times
@@ -268,7 +270,8 @@ describe NotificationService, :mailer do
         end
 
         it "emails the note author if they've opted into notifications about their activity" do
-          add_users_with_subscription(note.project, issue)
+          add_users(project)
+          add_user_subscriptions(issue)
           reset_delivered_emails!
 
           note.author.notified_of_own_activity = true
@@ -415,13 +418,15 @@ describe NotificationService, :mailer do
       let(:project) { create(:project, :public) }
       let(:issue) { create(:issue, project: project, assignees: [assignee]) }
       let(:mentioned_issue) { create(:issue, assignees: issue.assignees) }
-      let(:note) { create(:note_on_issue, noteable: issue, project_id: issue.project_id, note: '@all mentioned') }
+      let(:author) { create(:user) }
+      let(:note) { create(:note_on_issue, author: author, noteable: issue, project_id: issue.project_id, note: '@all mentioned') }
 
       before do
-        build_team(note.project)
-        build_group(note.project)
-        note.project.add_maintainer(note.author)
-        add_users_with_subscription(note.project, issue)
+        build_team(project)
+        build_group(project)
+        add_users(project)
+        add_user_subscriptions(issue)
+        project.add_maintainer(author)
         reset_delivered_emails!
       end
 
@@ -473,17 +478,18 @@ describe NotificationService, :mailer do
     context 'project snippet note' do
       let!(:project) { create(:project, :public) }
       let(:snippet) { create(:project_snippet, project: project, author: create(:user)) }
-      let(:note) { create(:note_on_project_snippet, noteable: snippet, project_id: project.id, note: '@all mentioned') }
+      let(:author) { create(:user) }
+      let(:note) { create(:note_on_project_snippet, author: author, noteable: snippet, project_id: project.id, note: '@all mentioned') }
 
       before do
         build_team(project)
         build_group(project)
+        project.add_maintainer(author)
 
         # make sure these users can read the project snippet!
         project.add_guest(@u_guest_watcher)
         project.add_guest(@u_guest_custom)
         add_member_for_parent_group(@pg_watcher, project)
-        note.project.add_maintainer(note.author)
         reset_delivered_emails!
       end
 
@@ -708,10 +714,11 @@ describe NotificationService, :mailer do
     let(:issue) { create :issue, project: project, assignees: [assignee], description: 'cc @participant @unsubscribed_mentioned' }
 
     before do
-      build_team(issue.project)
-      build_group(issue.project)
+      build_team(project)
+      build_group(project)
 
-      add_users_with_subscription(issue.project, issue)
+      add_users(project)
+      add_user_subscriptions(issue)
       reset_delivered_emails!
       update_custom_notification(:new_issue, @u_guest_custom, resource: project)
       update_custom_notification(:new_issue, @u_custom_global)
@@ -1281,13 +1288,16 @@ describe NotificationService, :mailer do
     let(:project) { create(:project, :public, :repository, namespace: group) }
     let(:another_project) { create(:project, :public, namespace: group) }
     let(:assignee) { create(:user) }
-    let(:merge_request) { create :merge_request, source_project: project, assignees: [assignee], description: 'cc @participant' }
+    let(:assignees) { Array.wrap(assignee) }
+    let(:author) { create(:user) }
+    let(:merge_request) { create :merge_request, author: author, source_project: project, assignees: assignees, description: 'cc @participant' }
 
     before do
-      project.add_maintainer(merge_request.author)
-      merge_request.assignees.each { |assignee| project.add_maintainer(assignee) }
-      build_team(merge_request.target_project)
-      add_users_with_subscription(merge_request.target_project, merge_request)
+      project.add_maintainer(author)
+      assignees.each { |assignee| project.add_maintainer(assignee) }
+      build_team(project)
+      add_users(project)
+      add_user_subscriptions(merge_request)
       update_custom_notification(:new_merge_request, @u_guest_custom, resource: project)
       update_custom_notification(:new_merge_request, @u_custom_global)
       reset_delivered_emails!
@@ -2417,7 +2427,7 @@ describe NotificationService, :mailer do
     should_not_email(user, recipients: email_recipients)
   end
 
-  def add_users_with_subscription(project, issuable)
+  def add_users(project)
     @subscriber = create :user
     @unsubscriber = create :user
     @unsubscribed_mentioned = create :user, username: 'unsubscribed_mentioned'
@@ -2429,7 +2439,9 @@ describe NotificationService, :mailer do
     project.add_maintainer(@unsubscriber)
     project.add_maintainer(@watcher_and_subscriber)
     project.add_maintainer(@unsubscribed_mentioned)
+  end
 
+  def add_user_subscriptions(issuable)
     issuable.subscriptions.create(user: @unsubscribed_mentioned, project: project, subscribed: false)
     issuable.subscriptions.create(user: @subscriber, project: project, subscribed: true)
     issuable.subscriptions.create(user: @subscribed_participant, project: project, subscribed: true)

spec/support/shared_examples/mentionable_shared_examples.rb

@@ -76,6 +76,30 @@ shared_examples 'a mentionable' do
     expect(refs).to include(ext_commit)
   end
 
+  context 'when there are cached markdown fields' do
+    before do
+      if subject.is_a?(CacheMarkdownField)
+        subject.refresh_markdown_cache
+      end
+    end
+
+    it 'sends in cached markdown fields when appropriate' do
+      if subject.is_a?(CacheMarkdownField)
+        expect_next_instance_of(Gitlab::ReferenceExtractor) do |ext|
+          attrs = subject.class.mentionable_attrs.collect(&:first) & subject.cached_markdown_fields.markdown_fields
+          attrs.each do |field|
+            expect(ext).to receive(:analyze).with(subject.send(field), hash_including(rendered: anything))
+          end
+        end
+
+        expect(subject).not_to receive(:refresh_markdown_cache)
+        expect(subject).to receive(:cached_markdown_fields).at_least(:once).and_call_original
+
+        subject.all_references(author)
+      end
+    end
+  end
+
   it 'creates cross-reference notes' do
     mentioned_objects = [mentioned_issue, mentioned_mr, mentioned_commit,
                          ext_issue, ext_mr, ext_commit]
@@ -98,6 +122,33 @@ shared_examples 'an editable mentionable' do
     [create(:issue, project: project), create(:issue, project: ext_proj)]
   end
 
+  context 'when there are cached markdown fields' do
+    before do
+      if subject.is_a?(CacheMarkdownField)
+        subject.refresh_markdown_cache
+      end
+    end
+
+    it 'refreshes markdown cache if necessary' do
+      subject.save!
+
+      set_mentionable_text.call('This is a text')
+
+      if subject.is_a?(CacheMarkdownField)
+        expect_next_instance_of(Gitlab::ReferenceExtractor) do |ext|
+          subject.cached_markdown_fields.markdown_fields.each do |field|
+            expect(ext).to receive(:analyze).with(subject.send(field), hash_including(rendered: anything))
+          end
+        end
+
+        expect(subject).to receive(:refresh_markdown_cache)
+        expect(subject).to receive(:cached_markdown_fields).at_least(:once).and_call_original
+
+        subject.all_references(author)
+      end
+    end
+  end
+
   it 'creates new cross-reference notes when the mentionable text is edited' do
     subject.save
     subject.create_cross_references!