Major refactoring on BlobsService interface

3 files changed, 53 insertions, 84 deletions

diff --git a/app/models/blobs_service.rb b/app/models/blobs_service.rb
index 94e4f9eb675..88d4ee900f1 100644
--- a/app/models/blobs_service.rb
+++ b/app/models/blobs_service.rb
@@ -2,87 +2,56 @@
 # It acts as a thin layer which caches lightweight information
 # based on the blob content (which is not cached).
 class BlobsService
-  def initialize(project, content_sha, path, highlighted:)
+  def initialize(project, content_sha, path)
     @project = project
     @content_sha = content_sha
     @path = path
-    @highlighted = highlighted
   end
 
-  def id
-    blob.id
-  end
-
-  def raw_size
-    blob.raw_size
-  end
-
-  def size
-    blob.size
-  end
-
-  def readable_text?
-    blob.readable_text?
-  end
+  def write_cache_if_empty
+    return unless content_sha
+    return if cache_exists?
 
-  def name
-    blob.name
-  end
-
-  def total_lines
-    blob.total_lines
-  end
-
-  def binary?
-    blob.binary?
-  end
-
-  def path
-    blob.path
-  end
-
-  def external_storage_error?
-    blob.external_storage_error?
-  end
-
-  def stored_externally?
-    blob.stored_externally?
-  end
-
-  def raw_binary?
-    blob.raw_binary?
-  end
-
-  def load_all_data!
-    blob.load_all_data!
-  end
+    blob_cache = { id: uncached_blob.id,
+                   raw_size: uncached_blob.raw_size,
+                   size: uncached_blob.size,
+                   readable_text: uncached_blob.readable_text?,
+                   name: uncached_blob.name,
+                   binary: uncached_blob.binary?,
+                   path: uncached_blob.path,
+                   external_storage_error: uncached_blob.external_storage_error?,
+                   stored_externally: uncached_blob.stored_externally?,
+                   total_lines: uncached_blob.total_lines }
 
-  def data
-    blob.data
+    cache.write(cache_key, blob_cache, expires_in: 1.week)
   end
 
   def clear_cache
     cache.delete(cache_key)
   end
 
-  private
-
   # We need blobs data (content) in order to highlight diffs (see
   # Gitlab::Diff:Highlight), and we don't cache this (Blob#data) on Redis,
   # mainly because it's a quite heavy information to cache for every blob.
   #
   # Therefore, in this scenario (no highlight yet) we use the uncached blob
   # version.
-  def blob
-    if @highlighted && cache_exists?
+  def blob(highlighted:)
+    return unless content_sha
+    return uncached_blob unless highlighted
+
+    if cache_exists?
       # TODO: This can be a CachedBlob
       Hashie::Mash.new(read_cache)
     else
-      write_cache
       uncached_blob
     end
   end
 
+  private
+
+  attr_reader :content_sha
+
   def uncached_blob
     @uncached_blob ||= Blob.lazy(@project, @content_sha, @path)&.itself
   end
@@ -99,21 +68,6 @@ class BlobsService
     cache.exist?(cache_key)
   end
 
-  def write_cache
-    blob_cache = { id: uncached_blob.id,
-                   raw_size: uncached_blob.raw_size,
-                   size: uncached_blob.size,
-                   readable_text: uncached_blob.readable_text?,
-                   name: uncached_blob.name,
-                   binary: uncached_blob.binary?,
-                   path: uncached_blob.path,
-                   external_storage_error: uncached_blob.external_storage_error?,
-                   stored_externally: uncached_blob.stored_externally?,
-                   total_lines: uncached_blob.total_lines }
-
-    cache.write(cache_key, blob_cache, expires_in: 1.week)
-  end
-
   def cache_key
     [@project.id, @content_sha, @path]
   end
diff --git a/lib/gitlab/diff/file.rb b/lib/gitlab/diff/file.rb
index 57608782331..39a64502f67 100644
--- a/lib/gitlab/diff/file.rb
+++ b/lib/gitlab/diff/file.rb
@@ -103,22 +103,24 @@ module Gitlab
         @old_content_sha = refs&.base_sha
       end
 
-      def new_blob
-        return unless new_content_sha
-
+      def new_blob_service
         BlobsService.new(repository.project,
                          new_content_sha,
-                         file_path,
-                         highlighted: highlighted?)
+                         file_path)
       end
 
-      def old_blob
-        return unless old_content_sha
-
+      def old_blob_service
         BlobsService.new(repository.project,
                          old_content_sha,
-                         file_path,
-                         highlighted: highlighted?)
+                         file_path)
+      end
+
+      def new_blob
+        new_blob_service.blob(highlighted: highlighted?)
+      end
+
+      def old_blob
+        old_blob_service.blob(highlighted: highlighted?)
       end
 
       def content_sha
diff --git a/lib/gitlab/diff/file_collection/merge_request_diff.rb b/lib/gitlab/diff/file_collection/merge_request_diff.rb
index 350ffa51290..ba3fd6b7403 100644
--- a/lib/gitlab/diff/file_collection/merge_request_diff.rb
+++ b/lib/gitlab/diff/file_collection/merge_request_diff.rb
@@ -19,7 +19,11 @@ module Gitlab
           #
           diff_files = super
 
-          diff_files.each { |diff_file| cache_highlight!(diff_file) }
+          diff_files.each do |diff_file|
+            cache_highlight!(diff_file)
+            cache_blobs!(diff_file)
+          end
+
           store_highlight_cache
 
           diff_files
@@ -35,8 +39,8 @@ module Gitlab
 
         def clear_blob_details_cache!
           diff_files.each do |file|
-            file.old_blob&.clear_cache
-            file.new_blob&.clear_cache
+            file.new_blob_service.clear_cache
+            file.old_blob_service.clear_cache
           end
         end
 
@@ -46,6 +50,11 @@ module Gitlab
 
         private
 
+        def cache_blobs!(diff_file)
+          diff_file.new_blob_service.write_cache_if_empty
+          diff_file.old_blob_service.write_cache_if_empty
+        end
+
         def highlight_diff_file_from_cache!(diff_file, cache_diff_lines)
           diff_file.highlighted_diff_lines = cache_diff_lines.map do |line|
             Gitlab::Diff::Line.init_from_hash(line)
@@ -66,7 +75,11 @@ module Gitlab
           if highlight_cache[item_key]
             highlight_diff_file_from_cache!(diff_file, highlight_cache[item_key])
           else
-            highlight_cache[item_key] = diff_file.highlighted_diff_lines.map(&:to_hash) if cacheable?(diff_file)
+            if cacheable?(diff_file)
+              highlight_cache[item_key] = diff_file.highlighted_diff_lines.map(&:to_hash)
+            else
+              binding.pry
+            end
           end
         end