From 7b76c8d644624f9a8a05e2e7a39af30c0c0c4ae0 Mon Sep 17 00:00:00 2001 From: Ahmad Sherif Date: Wed, 10 Jul 2019 19:34:05 +0200 Subject: [PATCH] Return an ETag headers for the archive endpoint We use the relative path of the archive to check for archive staleness. --- .../projects/repositories_controller.rb | 60 ++++++++++++++++--- app/models/repository.rb | 3 + .../add-caching-to-archive-endpoint.yml | 5 ++ .../projects/repositories_controller_spec.rb | 47 +++++++++++++++ 4 files changed, 108 insertions(+), 7 deletions(-) create mode 100644 changelogs/unreleased/add-caching-to-archive-endpoint.yml diff --git a/app/controllers/projects/repositories_controller.rb b/app/controllers/projects/repositories_controller.rb index 3b4215b766e..a51759641e4 100644 --- a/app/controllers/projects/repositories_controller.rb +++ b/app/controllers/projects/repositories_controller.rb @@ -6,6 +6,7 @@ class Projects::RepositoriesController < Projects::ApplicationController # Authorize before_action :require_non_empty_project, except: :create before_action :assign_archive_vars, only: :archive + before_action :assign_append_sha, only: :archive before_action :authorize_download_code! before_action :authorize_admin_project!, only: :create @@ -16,19 +17,64 @@ class Projects::RepositoriesController < Projects::ApplicationController end def archive - append_sha = params[:append_sha] + set_cache_headers + return if archive_not_modified? - if @ref - shortname = "#{@project.path}-#{@ref.tr('/', '-')}" - append_sha = false if @filename == shortname - end - - send_git_archive @repository, ref: @ref, path: params[:path], format: params[:format], append_sha: append_sha + send_git_archive @repository, **repo_params rescue => ex logger.error("#{self.class.name}: #{ex}") git_not_found! end + private + + def repo_params + @repo_params ||= { ref: @ref, path: params[:path], format: params[:format], append_sha: @append_sha } + end + + def set_cache_headers + expires_in cache_max_age(archive_metadata['CommitId']), public: project.public? + fresh_when(etag: archive_metadata['ArchivePath']) + end + + def archive_not_modified? + # Check response freshness (Last-Modified and ETag) + # against request If-Modified-Since and If-None-Match conditions. + request.fresh?(response) + end + + def archive_metadata + @archive_metadata ||= @repository.archive_metadata( + @ref, + '', # Where archives are stored isn't really important for ETag purposes + repo_params[:format], + path: repo_params[:path], + append_sha: @append_sha + ) + end + + def cache_max_age(commit_id) + if @ref == commit_id + # This is a link to an archive by a commit SHA. That means that the archive + # is immutable. The only reason to invalidate the cache is if the commit + # was deleted or if the user lost access to the repository. + Repository::ARCHIVE_CACHE_TIME_IMMUTABLE + else + # A branch or tag points at this archive. That means that the expected archive + # content may change over time. + Repository::ARCHIVE_CACHE_TIME + end + end + + def assign_append_sha + @append_sha = params[:append_sha] + + if @ref + shortname = "#{@project.path}-#{@ref.tr('/', '-')}" + @append_sha = false if @filename == shortname + end + end + def assign_archive_vars if params[:id] @ref, @filename = extract_ref(params[:id]) diff --git a/app/models/repository.rb b/app/models/repository.rb index a25d5abfa64..79dd0bab4ec 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -7,6 +7,9 @@ class Repository REF_KEEP_AROUND = 'keep-around'.freeze REF_ENVIRONMENTS = 'environments'.freeze + ARCHIVE_CACHE_TIME = 60 # Cache archives referred to by a (mutable) ref for 1 minute + ARCHIVE_CACHE_TIME_IMMUTABLE = 3600 # Cache archives referred to by an immutable reference for 1 hour + RESERVED_REFS_NAMES = %W[ heads tags diff --git a/changelogs/unreleased/add-caching-to-archive-endpoint.yml b/changelogs/unreleased/add-caching-to-archive-endpoint.yml new file mode 100644 index 00000000000..770ec16e804 --- /dev/null +++ b/changelogs/unreleased/add-caching-to-archive-endpoint.yml @@ -0,0 +1,5 @@ +--- +title: Return an ETag header for the archive endpoint +merge_request: 30581 +author: +type: added diff --git a/spec/controllers/projects/repositories_controller_spec.rb b/spec/controllers/projects/repositories_controller_spec.rb index 8fca9e680dd..fcab4d73dca 100644 --- a/spec/controllers/projects/repositories_controller_spec.rb +++ b/spec/controllers/projects/repositories_controller_spec.rb @@ -77,6 +77,53 @@ describe Projects::RepositoriesController do expect(response).to have_gitlab_http_status(404) end end + + describe 'caching' do + it 'sets appropriate caching headers' do + get_archive + + expect(response).to have_gitlab_http_status(200) + expect(response.header['ETag']).to be_present + expect(response.header['Cache-Control']).to include('max-age=60, private') + end + + context 'when project is public' do + let(:project) { create(:project, :repository, :public) } + + it 'sets appropriate caching headers' do + get_archive + + expect(response).to have_gitlab_http_status(200) + expect(response.header['ETag']).to be_present + expect(response.header['Cache-Control']).to include('max-age=60, public') + end + end + + context 'when ref is a commit SHA' do + it 'max-age is set to 3600 in Cache-Control header' do + get_archive('ddd0f15ae83993f5cb66a927a28673882e99100b') + + expect(response).to have_gitlab_http_status(200) + expect(response.header['Cache-Control']).to include('max-age=3600') + end + end + + context 'when If-None-Modified header is set' do + it 'returns a 304 status' do + # Get the archive cached first + get_archive + + request.headers['If-None-Match'] = response.headers['ETag'] + get_archive + + expect(response).to have_gitlab_http_status(304) + end + end + + def get_archive(id = 'feature') + get :archive, params: { namespace_id: project.namespace, project_id: project, id: id }, format: 'zip' + end + end end end end