Merge branch...

Merge branch '225322-improve-performance-of-merge-request-changes-api-under-load-into-s3-tier' into 'master'

Add FF to avoid directly accessing diffs via gitaly

See merge request gitlab-org/gitlab!46190

config/feature_flags/development/mrc_api_use_raw_diffs_from_gitaly.yml

+---
+name: mrc_api_use_raw_diffs_from_gitaly
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/46190
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/225322
+type: development
+group: group::source code
+default_enabled: false

lib/api/entities/merge_request_changes.rb

@@ -4,7 +4,27 @@ module API
   module Entities
     class MergeRequestChanges < MergeRequest
       expose :diffs, as: :changes, using: Entities::Diff do |compare, _|
-        compare.raw_diffs(limits: false).to_a
+        Array(diff_collection(compare))
+      end
+
+      expose :overflow?, as: :overflow
+
+      private
+
+      def overflow?
+        expose_raw_diffs? ? false : diff_collection(object).overflow?
+      end
+
+      def diff_collection(compare)
+        @diffs ||= if expose_raw_diffs?
+                     compare.raw_diffs(limits: false)
+                   else
+                     compare.diffs.diffs
+                   end
+      end
+
+      def expose_raw_diffs?
+        options[:access_raw_diffs] || ::Feature.enabled?(:mrc_api_use_raw_diffs_from_gitaly, options[:project])
       end
     end
   end

lib/api/merge_requests.rb

@@ -352,7 +352,11 @@ module API
       get ':id/merge_requests/:merge_request_iid/changes' do
         merge_request = find_merge_request_with_access(params[:merge_request_iid])
 
-        present merge_request, with: Entities::MergeRequestChanges, current_user: current_user, project: user_project
+        present merge_request,
+          with: Entities::MergeRequestChanges,
+          current_user: current_user,
+          project: user_project,
+          access_raw_diffs: params.fetch(:access_raw_diffs, false)
       end
 
       desc 'Get the merge request pipelines' do

spec/lib/api/entities/merge_request_changes_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe ::API::Entities::MergeRequestChanges do
+  let_it_be(:user) { create(:user) }
+  let_it_be(:merge_request) { create(:merge_request) }
+  let(:entity) { described_class.new(merge_request, current_user: user) }
+
+  subject(:basic_entity) { entity.as_json }
+
+  it "exposes basic entity fields" do
+    is_expected.to include(:changes, :overflow)
+  end
+
+  context "when #expose_raw_diffs? returns false" do
+    before do
+      expect(entity).to receive(:expose_raw_diffs?).twice.and_return(false)
+      expect_any_instance_of(Gitlab::Git::DiffCollection).to receive(:overflow?)
+    end
+
+    it "does not access merge_request.raw_diffs" do
+      expect(merge_request).not_to receive(:raw_diffs)
+
+      basic_entity
+    end
+  end
+
+  context "when #expose_raw_diffs? returns true" do
+    before do
+      expect(entity).to receive(:expose_raw_diffs?).twice.and_return(true)
+      expect_any_instance_of(Gitlab::Git::DiffCollection).not_to receive(:overflow?)
+    end
+
+    it "does not access merge_request.raw_diffs" do
+      expect(merge_request).to receive(:raw_diffs)
+
+      basic_entity
+    end
+  end
+
+  describe ":overflow field" do
+    context "when :access_raw_diffs is true" do
+      let_it_be(:entity_with_raw_diffs) do
+        described_class.new(merge_request, current_user: user, access_raw_diffs: true).as_json
+      end
+
+      before do
+        expect_any_instance_of(Gitlab::Git::DiffCollection).not_to receive(:overflow?)
+      end
+
+      it "reports false" do
+        expect(entity_with_raw_diffs[:overflow]).to be_falsy
+      end
+    end
+  end
+end

spec/requests/api/merge_requests_spec.rb

@@ -1312,13 +1312,44 @@ RSpec.describe API::MergeRequests do
   end
 
   describe 'GET /projects/:id/merge_requests/:merge_request_iid/changes' do
-    let_it_be(:merge_request) { create(:merge_request, :simple, author: user, assignees: [user], source_project: project, target_project: project, source_branch: 'markdown', title: "Test", created_at: base_time) }
+    let_it_be(:merge_request) do
+      create(
+        :merge_request,
+        :simple,
+        author: user,
+        assignees: [user],
+        source_project: project,
+        target_project: project,
+        source_branch: 'markdown',
+        title: "Test",
+        created_at: base_time
+      )
+    end
 
-    it 'returns the change information of the merge_request' do
-      get api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/changes", user)
+    shared_examples 'find an existing merge request' do
+      it 'returns the change information of the merge_request' do
+        get api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/changes", user)
 
-      expect(response).to have_gitlab_http_status(:ok)
-      expect(json_response['changes'].size).to eq(merge_request.diffs.size)
+        expect(response).to have_gitlab_http_status(:ok)
+        expect(json_response['changes'].size).to eq(merge_request.diffs.size)
+        expect(json_response['overflow']).to be_falsy
+      end
+    end
+
+    shared_examples 'accesses diffs via raw_diffs' do
+      let(:params) { {} }
+
+      it 'as expected' do
+        expect_any_instance_of(MergeRequest) do |merge_request|
+          expect(merge_request).to receive(:raw_diffs).and_call_original
+        end
+
+        expect_any_instance_of(MergeRequest) do |merge_request|
+          expect(merge_request).not_to receive(:diffs)
+        end
+
+        get(api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/changes", user), params: params)
+      end
     end
 
     it 'returns a 404 when merge_request_iid not found' do
@@ -1331,6 +1362,53 @@ RSpec.describe API::MergeRequests do
 
       expect(response).to have_gitlab_http_status(:not_found)
     end
+
+    it_behaves_like 'find an existing merge request'
+    it_behaves_like 'accesses diffs via raw_diffs'
+
+    it 'returns the overflow status as false' do
+      get api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/changes", user)
+
+      expect(response).to have_gitlab_http_status(:ok)
+      expect(json_response['overflow']).to be_falsy
+    end
+
+    context 'when using DB-backed diffs via feature flag' do
+      before do
+        stub_feature_flags(mrc_api_use_raw_diffs_from_gitaly: false)
+      end
+
+      it_behaves_like 'find an existing merge request'
+
+      it 'accesses diffs via DB-backed diffs.diffs' do
+        expect_any_instance_of(MergeRequest) do |merge_request|
+          expect(merge_request).to receive(:diffs).and_call_original
+        end
+
+        get api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/changes", user)
+      end
+
+      context 'when the diff_collection has overflowed its size limits' do
+        before do
+          expect_next_instance_of(Gitlab::Git::DiffCollection) do |diff_collection|
+            expect(diff_collection).to receive(:overflow?).and_return(true)
+          end
+        end
+
+        it 'returns the overflow status as true' do
+          get api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/changes", user)
+
+          expect(response).to have_gitlab_http_status(:ok)
+          expect(json_response['overflow']).to be_truthy
+        end
+      end
+
+      context 'when access_raw_diffs is passed as an option' do
+        it_behaves_like 'accesses diffs via raw_diffs' do
+          let(:params) { { access_raw_diffs: true } }
+        end
+      end
+    end
   end
 
   describe 'GET /projects/:id/merge_requests/:merge_request_iid/pipelines' do