diff --git a/app/controllers/projects/raw_controller.rb b/app/controllers/projects/raw_controller.rb index 924de0ee7ea5e1875e8566dc3befb9f43460131f..895a9a00624d18bea98a09c01ecdd6d6f3da2ae8 100644 --- a/app/controllers/projects/raw_controller.rb +++ b/app/controllers/projects/raw_controller.rb @@ -10,7 +10,7 @@ class Projects::RawController < Projects::ApplicationController prepend_before_action(only: [:show]) { authenticate_sessionless_user!(:blob) } - before_action :set_ref_and_path + before_action :assign_ref_vars before_action :require_non_empty_project before_action :authorize_read_code! before_action :check_show_rate_limit!, only: [:show], unless: :external_storage_request? diff --git a/app/controllers/projects/repositories_controller.rb b/app/controllers/projects/repositories_controller.rb index 33ce37ef4fbe90ddeeb8102d6c9730805344163b..a9ff5d8a3bfae41ea0888239b40600acb78ef9ab 100644 --- a/app/controllers/projects/repositories_controller.rb +++ b/app/controllers/projects/repositories_controller.rb @@ -47,8 +47,18 @@ def repo_params end def set_cache_headers - expires_in cache_max_age(archive_metadata['CommitId']), public: Guest.can?(:download_code, project) - fresh_when(etag: archive_metadata['ArchivePath']) + commit_id = archive_metadata['CommitId'] + + if Feature.enabled?(:improve_blobs_cache_headers) + expires_in(cache_max_age(commit_id), + public: Guest.can?(:download_code, project), must_revalidate: true, stale_if_error: 5.minutes, + stale_while_revalidate: 1.minute, 's-maxage': 1.minute) + + fresh_when(strong_etag: [commit_id, archive_metadata['ArchivePath']]) + else + expires_in cache_max_age(commit_id), public: Guest.can?(:download_code, project) + fresh_when(etag: archive_metadata['ArchivePath']) + end end def archive_not_modified? diff --git a/spec/controllers/projects/raw_controller_spec.rb b/spec/controllers/projects/raw_controller_spec.rb index 1c9aafacbd9ab59bc82d9cc0764b691628fe531a..a21727e56919c43af26f955656b6c10b0f864991 100644 --- a/spec/controllers/projects/raw_controller_spec.rb +++ b/spec/controllers/projects/raw_controller_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Projects::RawController do +RSpec.describe Projects::RawController, feature_category: :source_code_management do include RepoHelpers let_it_be(:project) { create(:project, :public, :repository) } @@ -23,13 +23,13 @@ def get_show subject { get_show } - shared_examples 'single Gitaly request' do - it 'makes a single Gitaly request', :request_store, :clean_gitlab_redis_cache do + shared_examples 'limited number of Gitaly request' do + it 'makes a limited number of Gitaly request', :request_store, :clean_gitlab_redis_cache do # Warm up to populate repository cache get_show RequestStore.clear! - expect { get_show }.to change { Gitlab::GitalyClient.get_request_count }.by(1) + expect { get_show }.to change { Gitlab::GitalyClient.get_request_count }.by(2) end end @@ -57,7 +57,7 @@ def get_show it_behaves_like 'project cache control headers' it_behaves_like 'content disposition headers' - include_examples 'single Gitaly request' + include_examples 'limited number of Gitaly request' end context 'image header' do @@ -73,7 +73,7 @@ def get_show it_behaves_like 'project cache control headers' it_behaves_like 'content disposition headers' - include_examples 'single Gitaly request' + include_examples 'limited number of Gitaly request' end context 'with LFS files' do @@ -82,7 +82,7 @@ def get_show it_behaves_like 'a controller that can serve LFS files' it_behaves_like 'project cache control headers' - include_examples 'single Gitaly request' + include_examples 'limited number of Gitaly request' end context 'when the endpoint receives requests above the limit' do @@ -239,8 +239,10 @@ def get_show end describe 'caching' do + let(:ref) { project.default_branch } + def request_file - get(:show, params: { namespace_id: project.namespace, project_id: project, id: 'master/README.md' }) + get(:show, params: { namespace_id: project.namespace, project_id: project, id: "#{ref}/README.md" }) end it 'sets appropriate caching headers' do @@ -254,6 +256,21 @@ def request_file ) end + context 'when a blob access by permalink' do + let(:ref) { project.commit.id } + + it 'sets appropriate caching headers with longer max-age' do + sign_in create(:user) + request_file + + expect(response.headers['ETag']).to eq("\"bdd5aa537c1e1f6d1b66de4bac8a6132\"") + expect(response.cache_control[:no_store]).to be_nil + expect(response.header['Cache-Control']).to eq( + 'max-age=3600, public, must-revalidate, stale-while-revalidate=60, stale-if-error=300, s-maxage=60' + ) + end + end + context 'when a public project has private repo' do let(:project) { create(:project, :public, :repository, :repository_private) } let(:user) { create(:user, maintainer_projects: [project]) } diff --git a/spec/controllers/projects/repositories_controller_spec.rb b/spec/controllers/projects/repositories_controller_spec.rb index 928428b5caf3f2696c831bd07ab4ee255a47a33e..bc2a3abc2d675730d009a33e22bb2fce044ba1df 100644 --- a/spec/controllers/projects/repositories_controller_spec.rb +++ b/spec/controllers/projects/repositories_controller_spec.rb @@ -2,7 +2,7 @@ require "spec_helper" -RSpec.describe Projects::RepositoriesController do +RSpec.describe Projects::RepositoriesController, feature_category: :source_code_management do let_it_be(:project) { create(:project, :repository) } describe 'POST create' do @@ -143,7 +143,9 @@ expect(response).to have_gitlab_http_status(:ok) expect(response.header['ETag']).to be_present - expect(response.header['Cache-Control']).to include('max-age=60, public') + expect(response.header['Cache-Control']).to eq( + 'max-age=60, public, must-revalidate, stale-while-revalidate=60, stale-if-error=300, s-maxage=60' + ) end context 'and repo is private' do @@ -154,7 +156,9 @@ expect(response).to have_gitlab_http_status(:ok) expect(response.header['ETag']).to be_present - expect(response.header['Cache-Control']).to include('max-age=60, private') + expect(response.header['Cache-Control']).to eq( + 'max-age=60, private, must-revalidate, stale-while-revalidate=60, stale-if-error=300, s-maxage=60' + ) end end end @@ -164,7 +168,48 @@ get_archive('ddd0f15ae83993f5cb66a927a28673882e99100b') expect(response).to have_gitlab_http_status(:ok) - expect(response.header['Cache-Control']).to include('max-age=3600') + expect(response.header['Cache-Control']).to eq( + 'max-age=3600, private, must-revalidate, stale-while-revalidate=60, stale-if-error=300, s-maxage=60' + ) + end + end + + context 'when improve_blobs_cache_headers is disabled' do + before do + stub_feature_flags(improve_blobs_cache_headers: false) + 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(:ok) + expect(response.header['ETag']).to be_present + expect(response.header['Cache-Control']).to eq('max-age=60, public') + end + + context 'and repo is private' do + let(:project) { create(:project, :repository, :public, :repository_private) } + + it 'sets appropriate caching headers' do + get_archive + + expect(response).to have_gitlab_http_status(:ok) + expect(response.header['ETag']).to be_present + expect(response.header['Cache-Control']).to eq('max-age=60, private') + end + 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(:ok) + expect(response.header['Cache-Control']).to eq('max-age=3600, private') + end end end