Merge branch 'no-findcommit-for-raw-endpoints' into 'master'

Don't call FindCommit for raw blobs

See merge request gitlab-org/gitlab!48917

app/controllers/projects/raw_controller.rb

@@ -10,29 +10,31 @@ class Projects::RawController < Projects::ApplicationController
 
   prepend_before_action(only: [:show]) { authenticate_sessionless_user!(:blob) }
 
+  before_action :set_ref_and_path
   before_action :require_non_empty_project
   before_action :authorize_download_code!
   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?
 
   feature_category :source_code_management
 
   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?)
   end
 
   private
 
-  def show_rate_limit
+  def set_ref_and_path
     # This bypasses assign_ref_vars to avoid a Gitaly FindCommit lookup.
-    # When rate limiting, we really don't care if a different commit is
-    # being requested.
-    _ref, path = extract_ref(get_id)
+    # We don't need to find the commit to either rate limit or send the
+    # blob.
+    @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)
 
       render plain: _('You cannot access the raw file. Please wait a minute.'), status: :too_many_requests

app/models/repository.rb

@@ -513,6 +513,9 @@ class Repository
     # Don't attempt to return a special result if there is no blob at all
     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
     return blob unless head_commit&.sha == sha
 

changelogs/unreleased/no-findcommit-for-raw-endpoints.yml

+---
+title: Remove unnecessary Gitaly calls from raw endpoint
+merge_request: 48917
+author:
+type: performance

spec/controllers/projects/raw_controller_spec.rb

@@ -5,11 +5,11 @@ require 'spec_helper'
 RSpec.describe Projects::RawController do
   include RepoHelpers
 
-  let(:project) { create(:project, :public, :repository) }
+  let_it_be(:project) { create(:project, :public, :repository) }
   let(:inline) { nil }
 
   describe 'GET #show' do
-    subject do
+    def get_show
       get(:show,
           params: {
             namespace_id: project.namespace,
@@ -19,6 +19,18 @@ RSpec.describe Projects::RawController do
           })
     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
       let(:filepath) { 'master/README.md' }
 
@@ -33,6 +45,7 @@ RSpec.describe Projects::RawController do
 
       it_behaves_like 'project cache control headers'
       it_behaves_like 'content disposition headers'
+      include_examples 'single Gitaly request'
     end
 
     context 'image header' do
@@ -48,6 +61,7 @@ RSpec.describe Projects::RawController do
 
       it_behaves_like 'project cache control headers'
       it_behaves_like 'content disposition headers'
+      include_examples 'single Gitaly request'
     end
 
     context 'with LFS files' do
@@ -56,6 +70,7 @@ RSpec.describe Projects::RawController do
 
       it_behaves_like 'a controller that can serve LFS files'
       it_behaves_like 'project cache control headers'
+      include_examples 'single Gitaly request'
     end
 
     context 'when the endpoint receives requests above the limit', :clean_gitlab_redis_cache do