Skip to content
Snippets Groups Projects
Commit 22fc95b2 authored by Kerri Miller's avatar Kerri Miller Committed by Mayra Cabrera
Browse files

Move raw_diffs access behind a feature flag

raw_diffs goes to gitaly, and can cause poor performance. This flag is
the first step in experimenting with accessing the diffs stored in the
DB.
parent 60e145b1
No related branches found
No related tags found
No related merge requests found
---
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
......@@ -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
......
......@@ -352,7 +352,11 @@ def authorize_push_to_merge_request!(merge_request)
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
......
# 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
......@@ -1312,13 +1312,44 @@
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 @@
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
......
0% Loading or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment