Migrate HighlightCache -> DeprecatedHighlightCache

Originally I had hoped to feature-flag the changes to support redis hash
caching, as they seemed small, but of course those changes expanded and
expanded until it became clear it really needed to be extracted into a
new class with a parallel API to keep it clean. h/t to @oswaldo for
pointing out when we'd crossed that threshold!

lib/gitlab/diff/deprecated_highlight_cache.rb

+# frozen_string_literal: true
+#
+module Gitlab
+  module Diff
+    class DeprecatedHighlightCache
+      delegate :diffable, to: :@diff_collection
+      delegate :diff_options, to: :@diff_collection
+
+      def initialize(diff_collection, backend: Rails.cache)
+        @backend = backend
+        @diff_collection = diff_collection
+      end
+
+      # - Reads from cache
+      # - Assigns DiffFile#highlighted_diff_lines for cached files
+      def decorate(diff_file)
+        if content = read_file(diff_file)
+          diff_file.highlighted_diff_lines = content.map do |line|
+            Gitlab::Diff::Line.init_from_hash(line)
+          end
+        end
+      end
+
+      # It populates a Hash in order to submit a single write to the memory
+      # cache. This avoids excessive IO generated by N+1's (1 writing for
+      # each highlighted line or file).
+      def write_if_empty
+        return if cached_content.present?
+
+        @diff_collection.diff_files.each do |diff_file|
+          next unless cacheable?(diff_file)
+
+          diff_file_id = diff_file.file_identifier
+
+          cached_content[diff_file_id] = diff_file.highlighted_diff_lines.map(&:to_hash)
+        end
+
+        cache.write(key, cached_content, expires_in: 1.week)
+      end
+
+      def clear
+        cache.delete(key)
+      end
+
+      def key
+        [diffable, 'highlighted-diff-files', Gitlab::Diff::Line::SERIALIZE_KEYS, diff_options]
+      end
+
+      private
+
+      def read_file(diff_file)
+        cached_content[diff_file.file_identifier]
+      end
+
+      def cache
+        @backend
+      end
+
+      def cached_content
+        @cached_content ||= cache.read(key) || {}
+      end
+
+      def cacheable?(diff_file)
+        diffable.present? && diff_file.text? && diff_file.diffable?
+      end
+    end
+  end
+end

lib/gitlab/diff/file_collection/merge_request_diff.rb

@@ -33,7 +33,8 @@ module Gitlab
         private
 
         def cache
-          @cache ||= Gitlab::Diff::HighlightCache.new(self)
+          # if Feature.enabled?(:redis_diff_caching)
+          @cache ||= Gitlab::Diff::DeprecatedHighlightCache.new(self)
         end
       end
     end

lib/gitlab/diff/highlight_cache.rb

@@ -11,7 +11,7 @@ module Gitlab
       def initialize(diff_collection, backend: Rails.cache)
         @backend = backend
         @diff_collection = diff_collection
-        @redis_key = "highlighted-diff-files:#{diffable.cache_key}" if Feature.enabled?(:redis_diff_caching)
+        @redis_key = "highlighted-diff-files:#{diffable.cache_key}"
       end
 
       # - Reads from cache
@@ -38,11 +38,7 @@ module Gitlab
           cached_content[diff_file_id] = diff_file.highlighted_diff_lines.map(&:to_hash)
         end
 
-        if Feature.enabled?(:redis_diff_caching)
-          write_to_redis_hash(cached_content)
-        else
-          cache.write(key, cached_content, expires_in: 1.week)
-        end
+        write_to_redis_hash(cached_content)
       end
 
       # Given a hash of:
@@ -85,11 +81,9 @@ module Gitlab
       end
 
       def clear
-        cache.delete(key)
-      end
-
-      def key
-        [diffable, 'highlighted-diff-files', Gitlab::Diff::Line::SERIALIZE_KEYS, diff_options]
+        Redis::Cache.with do |redis|
+          redis.del(@redis_key)
+        end
       end
 
       private
@@ -98,20 +92,12 @@ module Gitlab
         cached_content[diff_file.file_identifier]
       end
 
-      def cache
-        @backend
-      end
-
       def cached_content
         @cached_content ||= populate_cached_content
       end
 
       def populate_cached_content
-        if Feature.enabled?(:redis_diff_caching)
-          read_entire_redis_hash.transform_values! { |v| v.present? ? JSON.parse(v, symbolize_names: true) : nil }
-        else
-          cache.read(key) || {}
-        end
+        read_entire_redis_hash.transform_values! { |v| v.present? ? JSON.parse(v, symbolize_names: true) : nil }
       end
 
       def cacheable?(diff_file)

spec/lib/gitlab/diff/deprecated_highlight_cache_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::Diff::DeprecatedHighlightCache do
+  let(:merge_request) { create(:merge_request_with_diffs) }
+
+  subject(:cache) { described_class.new(merge_request.diffs, backend: backend) }
+
+  describe '#decorate' do
+    let(:backend) { double('backend').as_null_object }
+
+    # Manually creates a Diff::File object to avoid triggering the cache on
+    # the FileCollection::MergeRequestDiff
+    let(:diff_file) do
+      diffs = merge_request.diffs
+      raw_diff = diffs.diffable.raw_diffs(diffs.diff_options.merge(paths: ['CHANGELOG'])).first
+      Gitlab::Diff::File.new(raw_diff,
+                             repository: diffs.project.repository,
+                             diff_refs: diffs.diff_refs,
+                             fallback_diff_refs: diffs.fallback_diff_refs)
+    end
+
+    it 'does not calculate highlighting when reading from cache' do
+      cache.write_if_empty
+      cache.decorate(diff_file)
+
+      expect_any_instance_of(Gitlab::Diff::Highlight).not_to receive(:highlight)
+
+      diff_file.highlighted_diff_lines
+    end
+
+    it 'assigns highlighted diff lines to the DiffFile' do
+      cache.write_if_empty
+      cache.decorate(diff_file)
+
+      expect(diff_file.highlighted_diff_lines.size).to be > 5
+    end
+
+    it 'submits a single reading from the cache' do
+      cache.decorate(diff_file)
+      cache.decorate(diff_file)
+
+      expect(backend).to have_received(:read).with(cache.key).once
+    end
+  end
+
+  describe '#write_if_empty' do
+    let(:backend) { double('backend', read: {}).as_null_object }
+
+    it 'submits a single writing to the cache' do
+      cache.write_if_empty
+      cache.write_if_empty
+
+      expect(backend).to have_received(:write).with(cache.key,
+                                                    hash_including('CHANGELOG-false-false-false'),
+                                                    expires_in: 1.week).once
+    end
+  end
+
+  describe '#clear' do
+    let(:backend) { double('backend').as_null_object }
+
+    it 'clears cache' do
+      cache.clear
+
+      expect(backend).to have_received(:delete).with(cache.key)
+    end
+  end
+end

spec/lib/gitlab/diff/highlight_cache_spec.rb

@@ -37,6 +37,8 @@ describe Gitlab::Diff::HighlightCache, :clean_gitlab_redis_cache do
         new_pos: 20 }] }
   end
 
+  let(:cache_key) { cache.instance_variable_get(:@redis_key) }
+
   subject(:cache) { described_class.new(merge_request.diffs, backend: backend) }
 
   describe '#decorate' do
@@ -68,56 +70,22 @@ describe Gitlab::Diff::HighlightCache, :clean_gitlab_redis_cache do
 
       expect(diff_file.highlighted_diff_lines.size).to be > 5
     end
-
-    context 'when :redis_diff_caching is not enabled' do
-      before do
-        expect(Feature).to receive(:enabled?).with(:redis_diff_caching).and_return(false)
-      end
-
-      it 'submits a single reading from the cache' do
-        expect(Feature).to receive(:enabled?).with(:redis_diff_caching).at_least(:once).and_return(false)
-
-        2.times { cache.decorate(diff_file) }
-
-        expect(backend).to have_received(:read).with(cache.key).once
-      end
-    end
   end
 
   describe '#write_if_empty' do
     let(:backend) { double('backend', read: {}).as_null_object }
 
     context 'when :redis_diff_caching is enabled' do
-      before do
-        expect(Feature).to receive(:enabled?).with(:redis_diff_caching).and_return(true)
-      end
-
       it 'submits a single write action to the redis cache when invoked multiple times' do
         expect(cache).to receive(:write_to_redis_hash).once
 
         2.times { cache.write_if_empty }
       end
     end
-
-    context 'when :redis_diff_caching is not enabled' do
-      before do
-        expect(Feature).to receive(:enabled?).with(:redis_diff_caching).at_least(:once).and_return(false)
-      end
-
-      it 'submits a single writing to the cache' do
-        2.times { cache.write_if_empty }
-
-        expect(backend)
-          .to have_received(:write)
-          .with(cache.key, hash_including('CHANGELOG-false-false-false'), expires_in: 1.week)
-          .once
-      end
-    end
   end
 
   describe '#write_to_redis_hash' do
     let(:backend) { Rails.cache }
-    let(:cache_key) { cache.diffable.cache_key }
 
     it 'creates or updates a Redis hash' do
       expect { cache.write_to_redis_hash(diff_hash) }
@@ -127,7 +95,6 @@ describe Gitlab::Diff::HighlightCache, :clean_gitlab_redis_cache do
 
   describe '#read_entire_redis_hash' do
     let(:backend) { Rails.cache }
-    let(:cache_key) { cache.diffable.cache_key }
 
     before do
       cache.write_to_redis_hash(diff_hash)
@@ -142,7 +109,6 @@ describe Gitlab::Diff::HighlightCache, :clean_gitlab_redis_cache do
 
   describe '#read_single_entry_from_redis_hash' do
     let(:backend) { Rails.cache }
-    let(:cache_key) { cache.diffable.cache_key }
 
     before do
       cache.write_to_redis_hash(diff_hash)
@@ -161,9 +127,9 @@ describe Gitlab::Diff::HighlightCache, :clean_gitlab_redis_cache do
     let(:backend) { double('backend').as_null_object }
 
     it 'clears cache' do
-      cache.clear
+      expect_any_instance_of(Redis).to receive(:del).with(cache_key)
 
-      expect(backend).to have_received(:delete).with(cache.key)
+      cache.clear
     end
   end
 end

spec/services/notes/create_service_spec.rb

@@ -88,7 +88,7 @@ describe Notes::CreateService do
       end
 
       it 'clears noteable diff cache when it was unfolded for the note position' do
-        expect_any_instance_of(Gitlab::Diff::HighlightCache).to receive(:clear)
+        expect_any_instance_of(Gitlab::Diff::DeprecatedHighlightCache).to receive(:clear)
 
         described_class.new(project_with_repo, user, new_opts).execute
       end