Automatic merge of gitlab-org/gitlab-ce master

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])

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

changelogs/unreleased/add-caching-to-archive-endpoint.yml

+---
+title: Return an ETag header for the archive endpoint
+merge_request: 30581
+author:
+type: added

lib/gitlab/database/subquery.rb

@@ -6,11 +6,7 @@ module Gitlab
       class << self
         def self_join(relation)
           t = relation.arel_table
-          # Work around a bug in Rails 5, where LIMIT causes trouble
-          # See https://gitlab.com/gitlab-org/gitlab-ce/issues/51729
-          r = relation.limit(nil).arel
-          r.take(relation.limit_value) if relation.limit_value
-          t2 = r.as('t2')
+          t2 = relation.arel.as('t2')
 
           relation.unscoped.joins(t.join(t2).on(t[:id].eq(t2[:id])).join_sources.first)
         end

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