Commit 7a0aee12 authored by Sean McGivern's avatar Sean McGivern

Don't call FindCommit for raw blobs

`Repository#blob_at` can accept a reference or a SHA, so we don't need
to find the commit for the reference, we can just pass it straight
through.

We also make sure that `Repository#blob_at` doesn't try to do anything
fancy with a README unless the file could be a README.
parent 1d3fb7bf
...@@ -10,29 +10,31 @@ class Projects::RawController < Projects::ApplicationController ...@@ -10,29 +10,31 @@ class Projects::RawController < Projects::ApplicationController
prepend_before_action(only: [:show]) { authenticate_sessionless_user!(:blob) } prepend_before_action(only: [:show]) { authenticate_sessionless_user!(:blob) }
before_action :set_ref_and_path
before_action :require_non_empty_project before_action :require_non_empty_project
before_action :authorize_download_code! before_action :authorize_download_code!
before_action :show_rate_limit, only: [:show], unless: :external_storage_request? before_action :show_rate_limit, only: [:show], unless: :external_storage_request?
before_action :assign_ref_vars
before_action :redirect_to_external_storage, only: :show, if: :static_objects_external_storage_enabled? before_action :redirect_to_external_storage, only: :show, if: :static_objects_external_storage_enabled?
feature_category :source_code_management feature_category :source_code_management
def show def show
@blob = @repository.blob_at(@commit.id, @path) @blob = @repository.blob_at(@ref, @path)
send_blob(@repository, @blob, inline: (params[:inline] != 'false'), allow_caching: @project.public?) send_blob(@repository, @blob, inline: (params[:inline] != 'false'), allow_caching: @project.public?)
end end
private private
def show_rate_limit def set_ref_and_path
# This bypasses assign_ref_vars to avoid a Gitaly FindCommit lookup. # This bypasses assign_ref_vars to avoid a Gitaly FindCommit lookup.
# When rate limiting, we really don't care if a different commit is # We don't need to find the commit to either rate limit or send the
# being requested. # blob.
_ref, path = extract_ref(get_id) @ref, @path = extract_ref(get_id)
end
if rate_limiter.throttled?(:show_raw_controller, scope: [@project, path], threshold: raw_blob_request_limit) def show_rate_limit
if rate_limiter.throttled?(:show_raw_controller, scope: [@project, @path], threshold: raw_blob_request_limit)
rate_limiter.log_request(request, :raw_blob_request_limit, current_user) rate_limiter.log_request(request, :raw_blob_request_limit, current_user)
render plain: _('You cannot access the raw file. Please wait a minute.'), status: :too_many_requests render plain: _('You cannot access the raw file. Please wait a minute.'), status: :too_many_requests
......
...@@ -513,6 +513,9 @@ class Repository ...@@ -513,6 +513,9 @@ class Repository
# Don't attempt to return a special result if there is no blob at all # Don't attempt to return a special result if there is no blob at all
return unless blob return unless blob
# Don't attempt to return a special result if this can't be a README
return blob unless Gitlab::FileDetector.type_of(blob.name) == :readme
# Don't attempt to return a special result unless we're looking at HEAD # Don't attempt to return a special result unless we're looking at HEAD
return blob unless head_commit&.sha == sha return blob unless head_commit&.sha == sha
......
---
title: Remove unnecessary Gitaly calls from raw endpoint
merge_request:
author:
type: performance
...@@ -5,11 +5,11 @@ require 'spec_helper' ...@@ -5,11 +5,11 @@ require 'spec_helper'
RSpec.describe Projects::RawController do RSpec.describe Projects::RawController do
include RepoHelpers include RepoHelpers
let(:project) { create(:project, :public, :repository) } let_it_be(:project) { create(:project, :public, :repository) }
let(:inline) { nil } let(:inline) { nil }
describe 'GET #show' do describe 'GET #show' do
subject do def get_show
get(:show, get(:show,
params: { params: {
namespace_id: project.namespace, namespace_id: project.namespace,
...@@ -19,6 +19,18 @@ RSpec.describe Projects::RawController do ...@@ -19,6 +19,18 @@ RSpec.describe Projects::RawController do
}) })
end end
subject { get_show }
shared_examples 'single Gitaly request' do
it 'makes a single 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)
end
end
context 'regular filename' do context 'regular filename' do
let(:filepath) { 'master/README.md' } let(:filepath) { 'master/README.md' }
...@@ -33,6 +45,7 @@ RSpec.describe Projects::RawController do ...@@ -33,6 +45,7 @@ RSpec.describe Projects::RawController do
it_behaves_like 'project cache control headers' it_behaves_like 'project cache control headers'
it_behaves_like 'content disposition headers' it_behaves_like 'content disposition headers'
include_examples 'single Gitaly request'
end end
context 'image header' do context 'image header' do
...@@ -48,6 +61,7 @@ RSpec.describe Projects::RawController do ...@@ -48,6 +61,7 @@ RSpec.describe Projects::RawController do
it_behaves_like 'project cache control headers' it_behaves_like 'project cache control headers'
it_behaves_like 'content disposition headers' it_behaves_like 'content disposition headers'
include_examples 'single Gitaly request'
end end
context 'with LFS files' do context 'with LFS files' do
...@@ -56,6 +70,7 @@ RSpec.describe Projects::RawController do ...@@ -56,6 +70,7 @@ RSpec.describe Projects::RawController do
it_behaves_like 'a controller that can serve LFS files' it_behaves_like 'a controller that can serve LFS files'
it_behaves_like 'project cache control headers' it_behaves_like 'project cache control headers'
include_examples 'single Gitaly request'
end end
context 'when the endpoint receives requests above the limit', :clean_gitlab_redis_cache do context 'when the endpoint receives requests above the limit', :clean_gitlab_redis_cache 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