Merge branch 'kerrizor/check-for-max-patch-size-changes-preventing-caching' into 'master'

Check for default_max_patch_size changes

See merge request gitlab-org/gitlab!43778

lib/gitlab/diff/highlight_cache.rb

@@ -20,10 +20,23 @@ module Gitlab
       # - 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.safe_init_from_hash(line)
-          end
+        content = read_file(diff_file)
+
+        return [] unless content
+
+        if content.empty? && recache_due_to_size?(diff_file)
+          # If the file is missing from the cache and there's reason to believe
+          #   it is uncached due to a size issue around changing the values for
+          #   max patch size, manually populate the hash and then set the value.
+          #
+          new_cache_content = {}
+          new_cache_content[diff_file.file_path] = diff_file.highlighted_diff_lines.map(&:to_hash)
+
+          write_to_redis_hash(new_cache_content)
+
+          set_highlighted_diff_lines(diff_file, read_file(diff_file))
+        else
+          set_highlighted_diff_lines(diff_file, content)
         end
       end
 
@@ -58,6 +71,28 @@ module Gitlab
 
       private
 
+      def set_highlighted_diff_lines(diff_file, content)
+        diff_file.highlighted_diff_lines = content.map do |line|
+          Gitlab::Diff::Line.safe_init_from_hash(line)
+        end
+      end
+
+      def recache_due_to_size?(diff_file)
+        diff_file_class = diff_file.diff.class
+
+        current_patch_safe_limit_bytes = diff_file_class.patch_safe_limit_bytes
+        default_patch_safe_limit_bytes = diff_file_class.patch_safe_limit_bytes(diff_file_class::DEFAULT_MAX_PATCH_BYTES)
+
+        # If the diff is >= than the default limit, but less than the current
+        #   limit, it is likely uncached due to having hit the default limit,
+        #   making it eligible for recalculating.
+        #
+        diff_file.diff.diff_bytesize.between?(
+          default_patch_safe_limit_bytes,
+          current_patch_safe_limit_bytes
+        )
+      end
+
       def cacheable_files
         strong_memoize(:cacheable_files) do
           diff_files.select { |file| cacheable?(file) && read_file(file).nil? }

lib/gitlab/git/diff.rb

@@ -120,8 +120,8 @@ module Gitlab
         # default.
         #
         # Patches surpassing this limit should still be persisted in the database.
-        def patch_safe_limit_bytes
-          patch_hard_limit_bytes / 10
+        def patch_safe_limit_bytes(limit = patch_hard_limit_bytes)
+          limit / 10
         end
 
         # Returns the limit for a single diff file (patch).
@@ -174,9 +174,13 @@ module Gitlab
         @line_count ||= Util.count_lines(@diff)
       end
 
+      def diff_bytesize
+        @diff_bytesize ||= @diff.bytesize
+      end
+
       def too_large?
         if @too_large.nil?
-          @too_large = @diff.bytesize >= self.class.patch_hard_limit_bytes
+          @too_large = diff_bytesize >= self.class.patch_hard_limit_bytes
         else
           @too_large
         end
@@ -194,7 +198,7 @@ module Gitlab
       def collapsed?
         return @collapsed if defined?(@collapsed)
 
-        @collapsed = !expanded && @diff.bytesize >= self.class.patch_safe_limit_bytes
+        @collapsed = !expanded && diff_bytesize >= self.class.patch_safe_limit_bytes
       end
 
       def collapse!

spec/lib/gitlab/diff/highlight_cache_spec.rb

@@ -44,6 +44,7 @@ RSpec.describe Gitlab::Diff::HighlightCache, :clean_gitlab_redis_cache do
   describe '#decorate' do
     # 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
@@ -73,6 +74,37 @@ RSpec.describe Gitlab::Diff::HighlightCache, :clean_gitlab_redis_cache do
 
       expect(rich_texts).to all(be_html_safe)
     end
+
+    context "when diff_file is uncached due to default_max_patch_bytes change" do
+      before do
+        expect(cache).to receive(:read_file).at_least(:once).and_return([])
+
+        # Stub out the application's default and current patch size limits. We
+        #   want them to be different, and the diff file to be sized between
+        #   the 2 values.
+        #
+        diff_file_size_kb = (diff_file.diff.diff.bytesize * 10)
+
+        stub_const("#{diff_file.diff.class}::DEFAULT_MAX_PATCH_BYTES", diff_file_size_kb - 1 )
+        expect(diff_file.diff.class).to receive(:patch_safe_limit_bytes).and_return(diff_file_size_kb + 1)
+        expect(diff_file.diff.class)
+          .to receive(:patch_safe_limit_bytes)
+          .with(diff_file.diff.class::DEFAULT_MAX_PATCH_BYTES)
+          .and_call_original
+      end
+
+      it "manually writes highlighted lines to the cache" do
+        expect(cache).to receive(:write_to_redis_hash).and_call_original
+
+        cache.decorate(diff_file)
+      end
+
+      it "assigns highlighted diff lines to the DiffFile" do
+        expect(diff_file.highlighted_diff_lines.size).to be > 5
+
+        cache.decorate(diff_file)
+      end
+    end
   end
 
   shared_examples 'caches missing entries' do

spec/lib/gitlab/git/diff_spec.rb

@@ -284,13 +284,21 @@ EOT
   end
 
   describe '#line_count' do
-    it 'returns the correct number of lines' do
-      diff = described_class.new(gitaly_diff)
+    let(:diff) { described_class.new(gitaly_diff) }
 
+    it 'returns the correct number of lines' do
       expect(diff.line_count).to eq(7)
     end
   end
 
+  describe "#diff_bytesize" do
+    let(:diff) { described_class.new(gitaly_diff) }
+
+    it "returns the size of the diff in bytes" do
+      expect(diff.diff_bytesize).to eq(diff.diff.bytesize)
+    end
+  end
+
   describe '#too_large?' do
     it 'returns true for a diff that is too large' do
       diff = described_class.new(diff: 'a' * 204800)