Commit ed09090b authored by Heinrich Lee Yu's avatar Heinrich Lee Yu

Enable caching of git blobs even when signed in

We should skip the `no-store` cache control header that we append by
default in these controller actions where we want to cache. We already
do this for some controllers like uploads but this was missing for a
very long time now.
parent fe62abdc
...@@ -3,6 +3,8 @@ ...@@ -3,6 +3,8 @@
class Projects::AvatarsController < Projects::ApplicationController class Projects::AvatarsController < Projects::ApplicationController
include SendsBlob include SendsBlob
skip_before_action :default_cache_headers, only: :show
before_action :authorize_admin_project!, only: [:destroy] before_action :authorize_admin_project!, only: [:destroy]
feature_category :projects feature_category :projects
......
...@@ -6,6 +6,8 @@ class Projects::RawController < Projects::ApplicationController ...@@ -6,6 +6,8 @@ class Projects::RawController < Projects::ApplicationController
include SendsBlob include SendsBlob
include StaticObjectExternalStorage include StaticObjectExternalStorage
skip_before_action :default_cache_headers, only: :show
prepend_before_action(only: [:show]) { authenticate_sessionless_user!(:blob) } prepend_before_action(only: [:show]) { authenticate_sessionless_user!(:blob) }
before_action :require_non_empty_project before_action :require_non_empty_project
......
...@@ -8,6 +8,8 @@ class Projects::RepositoriesController < Projects::ApplicationController ...@@ -8,6 +8,8 @@ class Projects::RepositoriesController < Projects::ApplicationController
prepend_before_action(only: [:archive]) { authenticate_sessionless_user!(:archive) } prepend_before_action(only: [:archive]) { authenticate_sessionless_user!(:archive) }
skip_before_action :default_cache_headers, only: :archive
# Authorize # Authorize
before_action :require_non_empty_project, except: :create before_action :require_non_empty_project, except: :create
before_action :archive_rate_limit!, only: :archive before_action :archive_rate_limit!, only: :archive
......
---
title: Enable HTTP caching of repository raw, archive, and avatar endpoints
merge_request: 47430
author:
type: performance
...@@ -3,14 +3,14 @@ ...@@ -3,14 +3,14 @@
require 'spec_helper' require 'spec_helper'
RSpec.describe Projects::AvatarsController do RSpec.describe Projects::AvatarsController do
let_it_be(:project) { create(:project, :repository) } describe 'GET #show' do
let_it_be(:project) { create(:project, :public, :repository) }
before do before do
controller.instance_variable_set(:@project, project) controller.instance_variable_set(:@project, project)
end end
describe 'GET #show' do subject { get :show, params: { namespace_id: project.namespace, project_id: project.path } }
subject { get :show, params: { namespace_id: project.namespace, project_id: project } }
context 'when repository has no avatar' do context 'when repository has no avatar' do
it 'shows 404' do it 'shows 404' do
...@@ -37,6 +37,15 @@ RSpec.describe Projects::AvatarsController do ...@@ -37,6 +37,15 @@ RSpec.describe Projects::AvatarsController do
expect(response.header[Gitlab::Workhorse::DETECT_HEADER]).to eq 'true' expect(response.header[Gitlab::Workhorse::DETECT_HEADER]).to eq 'true'
end end
it 'sets appropriate caching headers' do
sign_in(project.owner)
subject
expect(response.cache_control[:public]).to eq(true)
expect(response.cache_control[:max_age]).to eq(60)
expect(response.cache_control[:no_store]).to be_nil
end
it_behaves_like 'project cache control headers' it_behaves_like 'project cache control headers'
end end
...@@ -51,9 +60,16 @@ RSpec.describe Projects::AvatarsController do ...@@ -51,9 +60,16 @@ RSpec.describe Projects::AvatarsController do
end end
describe 'DELETE #destroy' do describe 'DELETE #destroy' do
let(:project) { create(:project, :repository, avatar: fixture_file_upload("spec/fixtures/dk.png", "image/png")) }
before do
sign_in(project.owner)
end
it 'removes avatar from DB by calling destroy' do it 'removes avatar from DB by calling destroy' do
delete :destroy, params: { namespace_id: project.namespace.id, project_id: project.id } delete :destroy, params: { namespace_id: project.namespace.path, project_id: project.path }
expect(response).to redirect_to(edit_project_path(project, anchor: 'js-general-project-settings'))
expect(project.avatar.present?).to be_falsey expect(project.avatar.present?).to be_falsey
expect(project).to be_valid expect(project).to be_valid
end end
......
...@@ -226,6 +226,15 @@ RSpec.describe Projects::RawController do ...@@ -226,6 +226,15 @@ RSpec.describe Projects::RawController do
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: 'master/README.md' })
end end
it 'sets appropriate caching headers' do
sign_in create(:user)
request_file
expect(response.cache_control[:public]).to eq(true)
expect(response.cache_control[:max_age]).to eq(60)
expect(response.cache_control[:no_store]).to be_nil
end
context 'when If-None-Match header is set' do context 'when If-None-Match header is set' do
it 'returns a 304 status' do it 'returns a 304 status' do
request_file request_file
......
...@@ -122,7 +122,9 @@ RSpec.describe Projects::RepositoriesController do ...@@ -122,7 +122,9 @@ RSpec.describe Projects::RepositoriesController do
expect(response).to have_gitlab_http_status(:ok) expect(response).to have_gitlab_http_status(:ok)
expect(response.header['ETag']).to be_present expect(response.header['ETag']).to be_present
expect(response.header['Cache-Control']).to include('max-age=60, private') expect(response.cache_control[:public]).to eq(false)
expect(response.cache_control[:max_age]).to eq(60)
expect(response.cache_control[:no_store]).to be_nil
end end
context 'when project is public' do context 'when project is public' do
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment