From fc90d9e5896cdcccedb697fd4536f126d10f3f8e Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Thu, 3 Mar 2016 17:59:47 +0100 Subject: Tell clients/proxies to cache raw blob requests --- app/controllers/projects/avatars_controller.rb | 2 ++ app/controllers/projects/raw_controller.rb | 2 ++ app/helpers/blob_helper.rb | 25 +++++++++++++++++++++++++ app/models/project.rb | 7 ++++--- app/views/projects/blob/_image.html.haml | 2 +- 5 files changed, 34 insertions(+), 4 deletions(-) diff --git a/app/controllers/projects/avatars_controller.rb b/app/controllers/projects/avatars_controller.rb index b64dbbd89ce..6de7888888f 100644 --- a/app/controllers/projects/avatars_controller.rb +++ b/app/controllers/projects/avatars_controller.rb @@ -7,6 +7,8 @@ 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! 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 d9723acb1d9..b6ff08262d7 100644 --- a/app/controllers/projects/raw_controller.rb +++ b/app/controllers/projects/raw_controller.rb @@ -12,6 +12,8 @@ class Projects::RawController < Projects::ApplicationController if @blob headers['X-Content-Type-Options'] = 'nosniff' + check_etag! + set_cache_headers if @blob.lfs_pointer? send_lfs_object diff --git a/app/helpers/blob_helper.rb b/app/helpers/blob_helper.rb index 7f63a2e2cb4..adb56e49c62 100644 --- a/app/helpers/blob_helper.rb +++ b/app/helpers/blob_helper.rb @@ -152,4 +152,29 @@ module BlobHelper 'application/octet-stream' end end + + def set_cache_headers + if @project.visibility_level == Project::PUBLIC + cache_control = 'public, ' + else + cache_control = 'private, ' + end + + 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 + 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 + end + + headers['Cache-Control'] = cache_control + headers['ETag'] = @blob.id + end + + def check_etag! + stale?(etag: @blob.id) + end end diff --git a/app/models/project.rb b/app/models/project.rb index 148eab692ff..aba48f87be3 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -56,6 +56,7 @@ class Project < ActiveRecord::Base extend Gitlab::ConfigHelper UNKNOWN_IMPORT_URL = 'http://unknown.git' + AVATAR_BRANCH = 'master' default_value_for :archived, false default_value_for :visibility_level, gitlab_config_features.visibility_level @@ -544,9 +545,9 @@ class Project < ActiveRecord::Base end def avatar_in_git - @avatar_file ||= 'logo.png' if repository.blob_at_branch('master', 'logo.png') - @avatar_file ||= 'logo.jpg' if repository.blob_at_branch('master', 'logo.jpg') - @avatar_file ||= 'logo.gif' if repository.blob_at_branch('master', 'logo.gif') + @avatar_file ||= 'logo.png' if repository.blob_at_branch(AVATAR_BRANCH, 'logo.png') + @avatar_file ||= 'logo.jpg' if repository.blob_at_branch(AVATAR_BRANCH, 'logo.jpg') + @avatar_file ||= 'logo.gif' if repository.blob_at_branch(AVATAR_BRANCH, 'logo.gif') @avatar_file end diff --git a/app/views/projects/blob/_image.html.haml b/app/views/projects/blob/_image.html.haml index 3c11b97921f..18caddabd39 100644 --- a/app/views/projects/blob/_image.html.haml +++ b/app/views/projects/blob/_image.html.haml @@ -6,4 +6,4 @@ - blob = sanitize_svg(blob) %img{src: "data:#{blob.mime_type};base64,#{Base64.encode64(blob.data)}"} - else - %img{src: namespace_project_raw_path(@project.namespace, @project, @id)} + %img{src: namespace_project_raw_path(@project.namespace, @project, tree_join(@commit.id, blob.path))} -- cgit v1.2.1 From a215e2ee8ddaefbfef16669ad0bd8ccd6853e163 Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Mon, 7 Mar 2016 14:11:38 +0100 Subject: Revert changes in the Project model --- app/models/project.rb | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/app/models/project.rb b/app/models/project.rb index aba48f87be3..148eab692ff 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -56,7 +56,6 @@ class Project < ActiveRecord::Base extend Gitlab::ConfigHelper UNKNOWN_IMPORT_URL = 'http://unknown.git' - AVATAR_BRANCH = 'master' default_value_for :archived, false default_value_for :visibility_level, gitlab_config_features.visibility_level @@ -545,9 +544,9 @@ class Project < ActiveRecord::Base end def avatar_in_git - @avatar_file ||= 'logo.png' if repository.blob_at_branch(AVATAR_BRANCH, 'logo.png') - @avatar_file ||= 'logo.jpg' if repository.blob_at_branch(AVATAR_BRANCH, 'logo.jpg') - @avatar_file ||= 'logo.gif' if repository.blob_at_branch(AVATAR_BRANCH, 'logo.gif') + @avatar_file ||= 'logo.png' if repository.blob_at_branch('master', 'logo.png') + @avatar_file ||= 'logo.jpg' if repository.blob_at_branch('master', 'logo.jpg') + @avatar_file ||= 'logo.gif' if repository.blob_at_branch('master', 'logo.gif') @avatar_file end -- cgit v1.2.1 From 41bc9c463c187396e47b4a942965de6ecddca5a1 Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Mon, 7 Mar 2016 14:27:53 +0100 Subject: Refactor caching code --- app/controllers/projects/avatars_controller.rb | 5 +++-- app/controllers/projects/raw_controller.rb | 4 ++-- app/helpers/blob_helper.rb | 17 ++++++++++------- app/models/blob.rb | 3 +++ 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 -- cgit v1.2.1 From a284d307838adf44080c43e4f553fe60b3495ffe Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Mon, 7 Mar 2016 16:49:46 +0100 Subject: Use Rails etag/cache_control helpers --- app/helpers/blob_helper.rb | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-) diff --git a/app/helpers/blob_helper.rb b/app/helpers/blob_helper.rb index e5c0ed4b7bd..0f77b3b299a 100644 --- a/app/helpers/blob_helper.rb +++ b/app/helpers/blob_helper.rb @@ -157,27 +157,20 @@ module BlobHelper 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 - cache_control = 'private, ' - end + response.cache_control[:public] = @project.public? if @ref && @commit && @ref == @commit.id # This is a link to a commit by its commit SHA. That means that the blob # 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 + response.cache_control[: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. - max_age = Blob::CACHE_TIME + response.cache_control[:max_age] = Blob::CACHE_TIME end - cache_control << "max-age=#{max_age}" - headers['Cache-Control'] = cache_control - headers['ETag'] = @blob.id - + response.etag = @blob.id !stale end end -- cgit v1.2.1