Refactor caching code

4 files changed, 18 insertions, 11 deletions

diff --git a/app/controllers/projects/avatars_controller.rb b/app/controllers/projects/avatars_controller.rb
index 6de7888888f..a6bebc46b06 100644
--- a/app/controllers/projects/avatars_controller.rb
+++ b/app/controllers/projects/avatars_controller.rb
@@ -7,8 +7,9 @@ class Projects::AvatarsController < Projects::ApplicationController
     @blob = @repository.blob_at_branch('master', @project.avatar_in_git)
     if @blob
       headers['X-Content-Type-Options'] = 'nosniff'
-      set_cache_headers
-      check_etag!
+
+      return if cached_blob?
+
       headers.store(*Gitlab::Workhorse.send_git_blob(@repository, @blob))
       headers['Content-Disposition'] = 'inline'
       headers['Content-Type'] = safe_content_type(@blob)
diff --git a/app/controllers/projects/raw_controller.rb b/app/controllers/projects/raw_controller.rb
index b6ff08262d7..10de0e60530 100644
--- a/app/controllers/projects/raw_controller.rb
+++ b/app/controllers/projects/raw_controller.rb
@@ -12,8 +12,8 @@ class Projects::RawController < Projects::ApplicationController
 
     if @blob
       headers['X-Content-Type-Options'] = 'nosniff'
-      check_etag!
-      set_cache_headers
+
+      return if cached_blob?
 
       if @blob.lfs_pointer?
         send_lfs_object
diff --git a/app/helpers/blob_helper.rb b/app/helpers/blob_helper.rb
index adb56e49c62..e5c0ed4b7bd 100644
--- a/app/helpers/blob_helper.rb
+++ b/app/helpers/blob_helper.rb
@@ -153,7 +153,10 @@ module BlobHelper
     end
   end
 
-  def set_cache_headers
+  def cached_blob?
+    stale = stale?(etag: @blob.id) # The #stale? method sets cache headers.
+
+    # Because we are opionated we set the cache headers ourselves.
     if @project.visibility_level == Project::PUBLIC
       cache_control = 'public, '
     else
@@ -162,19 +165,19 @@ module BlobHelper
 
     if @ref && @commit && @ref == @commit.id
       # This is a link to a commit by its commit SHA. That means that the blob
-      # is immutable.
-      cache_control << 'max-age=600' # 10 minutes
+      # is immutable. The only reason to invalidate the cache is if the commit
+      # was deleted or if the user lost access to the repository.
+      max_age = Blob::CACHE_TIME_IMMUTABLE
     else
       # A branch or tag points at this blob. That means that the expected blob
       # value may change over time.
-      cache_control << 'max-age=60' # 1 minute
+      max_age = Blob::CACHE_TIME
     end
 
+    cache_control << "max-age=#{max_age}"
     headers['Cache-Control'] = cache_control
     headers['ETag'] = @blob.id
-  end
 
-  def check_etag!
-    stale?(etag: @blob.id)
+    !stale
   end
 end
diff --git a/app/models/blob.rb b/app/models/blob.rb
index 8ee9f3006b2..72e6c5fa3fd 100644
--- a/app/models/blob.rb
+++ b/app/models/blob.rb
@@ -1,5 +1,8 @@
 # Blob is a Rails-specific wrapper around Gitlab::Git::Blob objects
 class Blob < SimpleDelegator
+  CACHE_TIME = 60 # Cache raw blobs referred to by a (mutable) ref for 1 minute
+  CACHE_TIME_IMMUTABLE = 3600 # Cache blobs referred to by an immutable reference for 1 hour
+
   # Wrap a Gitlab::Git::Blob object, or return nil when given nil
   #
   # This method prevents the decorated object from evaluating to "truthy" when