Merge branch 'banzai-avoid-redis-if-db-cache' into 'master'

Banzai - avoid redis if attr is in DB cache

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

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])
 
-        item[:cache_key] = cache_key if cache_key
+        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?)
-        .with(commit1, user)
-        .and_return(true)
+      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!