Banzai - avoid redis if attr is in DB cachebanzai-avoid-redis-if-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.

7 files changed, 145 insertions, 29 deletions

diff --git a/spec/lib/banzai/renderer_spec.rb b/spec/lib/banzai/renderer_spec.rb
index aa828e2f0e9..a099f7482c1 100644
--- a/spec/lib/banzai/renderer_spec.rb
+++ b/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 }
 
diff --git a/spec/lib/gitlab/markdown_cache/active_record/extension_spec.rb b/spec/lib/gitlab/markdown_cache/active_record/extension_spec.rb
index 18052b1991c..c5fc74afea5 100644
--- a/spec/lib/gitlab/markdown_cache/active_record/extension_spec.rb
+++ b/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
diff --git a/spec/models/commit_range_spec.rb b/spec/models/commit_range_spec.rb
index b96ca89c893..4a524b585e1 100644
--- a/spec/models/commit_range_spec.rb
+++ b/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
diff --git a/spec/models/concerns/cache_markdown_field_spec.rb b/spec/models/concerns/cache_markdown_field_spec.rb
index 0e5fb2b5153..9a12c3d6965 100644
--- a/spec/models/concerns/cache_markdown_field_spec.rb
+++ b/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
diff --git a/spec/models/note_spec.rb b/spec/models/note_spec.rb
index 7a1ab20186a..03003e3dd7d 100644
--- a/spec/models/note_spec.rb
+++ b/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
diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb
index f25e2fe5e2b..1d7bf91fda1 100644
--- a/spec/services/notification_service_spec.rb
+++ b/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)
diff --git a/spec/support/shared_examples/mentionable_shared_examples.rb b/spec/support/shared_examples/mentionable_shared_examples.rb
index 1226841f24c..fea52c2eeb2 100644
--- a/spec/support/shared_examples/mentionable_shared_examples.rb
+++ b/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!