Commit 5b69a221 authored by Oswaldo Ferreira's avatar Oswaldo Ferreira

Introduce a diffs_metadata endpoint

It creates a separate endpoint to return just
metadata of the diffs, but not the actual
serialized diff lines, which are _very_
expensive to serialize.
parent 65381105
...@@ -7,7 +7,7 @@ class Projects::MergeRequests::DiffsController < Projects::MergeRequests::Applic ...@@ -7,7 +7,7 @@ class Projects::MergeRequests::DiffsController < Projects::MergeRequests::Applic
before_action :apply_diff_view_cookie! before_action :apply_diff_view_cookie!
before_action :commit, except: :diffs_batch before_action :commit, except: :diffs_batch
before_action :define_diff_vars, except: :diffs_batch before_action :define_diff_vars, except: :diffs_batch
before_action :define_diff_comment_vars, except: :diffs_batch before_action :define_diff_comment_vars, except: [:diffs_batch, :diffs_metadata]
def show def show
render_diffs render_diffs
...@@ -37,6 +37,11 @@ class Projects::MergeRequests::DiffsController < Projects::MergeRequests::Applic ...@@ -37,6 +37,11 @@ class Projects::MergeRequests::DiffsController < Projects::MergeRequests::Applic
render json: PaginatedDiffSerializer.new(current_user: current_user).represent(diffs, options) render json: PaginatedDiffSerializer.new(current_user: current_user).represent(diffs, options)
end end
def diffs_metadata
render json: DiffsMetadataSerializer.new(project: @merge_request.project)
.represent(@diffs, additional_attributes)
end
private private
def preloadable_mr_relations def preloadable_mr_relations
......
...@@ -203,8 +203,8 @@ module DiffHelper ...@@ -203,8 +203,8 @@ module DiffHelper
link_to "#{hide_whitespace? ? 'Show' : 'Hide'} whitespace changes", url, class: options[:class] link_to "#{hide_whitespace? ? 'Show' : 'Hide'} whitespace changes", url, class: options[:class]
end end
def render_overflow_warning?(diff_files) def render_overflow_warning?(diffs_collection)
diffs = @merge_request_diff.presence || diff_files diffs = @merge_request_diff.presence || diffs_collection.diff_files
diffs.overflow? diffs.overflow?
end end
......
# frozen_string_literal: true
class DiffFileMetadataEntity < Grape::Entity
expose :added_lines
expose :removed_lines
expose :new_path
expose :old_path
expose :new_file?, as: :new_file
expose :deleted_file?, as: :deleted_file
end
...@@ -53,7 +53,7 @@ class DiffsEntity < Grape::Entity ...@@ -53,7 +53,7 @@ class DiffsEntity < Grape::Entity
# rubocop: enable CodeReuse/ActiveRecord # rubocop: enable CodeReuse/ActiveRecord
expose :render_overflow_warning do |diffs| expose :render_overflow_warning do |diffs|
render_overflow_warning?(diffs.diff_files) render_overflow_warning?(diffs)
end end
expose :email_patch_path, if: -> (*) { merge_request } do |diffs| expose :email_patch_path, if: -> (*) { merge_request } do |diffs|
......
# frozen_string_literal: true
class DiffsMetadataEntity < DiffsEntity
unexpose :diff_files
expose :diff_files, using: DiffFileMetadataEntity
end
# frozen_string_literal: true
class DiffsMetadataSerializer < BaseSerializer
entity DiffsMetadataEntity
end
...@@ -21,7 +21,7 @@ ...@@ -21,7 +21,7 @@
= parallel_diff_btn = parallel_diff_btn
= render 'projects/diffs/stats', diff_files: diff_files = render 'projects/diffs/stats', diff_files: diff_files
- if render_overflow_warning?(diff_files) - if render_overflow_warning?(diffs)
= render 'projects/diffs/warning', diff_files: diffs = render 'projects/diffs/warning', diff_files: diffs
.files{ data: { can_create_note: can_create_note } } .files{ data: { can_create_note: can_create_note } }
......
---
title: Introduce a lightweight diffs_metadata endpoint
merge_request: 18104
author:
type: added
...@@ -282,6 +282,7 @@ constraints(::Constraints::ProjectUrlConstrainer.new) do ...@@ -282,6 +282,7 @@ constraints(::Constraints::ProjectUrlConstrainer.new) do
get :pipelines get :pipelines
get :diffs, to: 'merge_requests/diffs#show' get :diffs, to: 'merge_requests/diffs#show'
get :diffs_batch, to: 'merge_requests/diffs#diffs_batch' get :diffs_batch, to: 'merge_requests/diffs#diffs_batch'
get :diffs_metadata, to: 'merge_requests/diffs#diffs_metadata'
get :widget, to: 'merge_requests/content#widget' get :widget, to: 'merge_requests/content#widget'
get :cached_widget, to: 'merge_requests/content#cached_widget' get :cached_widget, to: 'merge_requests/content#cached_widget'
end end
......
...@@ -100,6 +100,136 @@ describe Projects::MergeRequests::DiffsController do ...@@ -100,6 +100,136 @@ describe Projects::MergeRequests::DiffsController do
it_behaves_like 'persisted preferred diff view cookie' it_behaves_like 'persisted preferred diff view cookie'
end end
describe 'GET diffs_metadata' do
def go(extra_params = {})
params = {
namespace_id: project.namespace.to_param,
project_id: project,
id: merge_request.iid,
format: 'json'
}
get :diffs_metadata, params: params.merge(extra_params)
end
context 'when not authorized' do
let(:another_user) { create(:user) }
before do
sign_in(another_user)
end
it 'returns 404 when not a member' do
go
expect(response).to have_gitlab_http_status(404)
end
it 'returns 404 when visibility level is not enough' do
project.add_guest(another_user)
go
expect(response).to have_gitlab_http_status(404)
end
end
context 'when diffable does not exists' do
it 'returns 404' do
go(diff_id: 9999)
expect(response).to have_gitlab_http_status(404)
end
end
context 'with valid diff_id' do
it 'returns success' do
go(diff_id: merge_request.merge_request_diff.id)
expect(response).to have_gitlab_http_status(200)
end
it 'serializes diffs metadata with expected arguments' do
expected_options = {
environment: nil,
merge_request: merge_request,
merge_request_diff: merge_request.merge_request_diff,
merge_request_diffs: merge_request.merge_request_diffs,
start_version: nil,
start_sha: nil,
commit: nil,
latest_diff: true
}
expect_next_instance_of(DiffsMetadataSerializer) do |instance|
expect(instance).to receive(:represent)
.with(an_instance_of(Gitlab::Diff::FileCollection::MergeRequestDiff), expected_options)
.and_call_original
end
go(diff_id: merge_request.merge_request_diff.id)
end
end
context 'with MR regular diff params' do
it 'returns success' do
go
expect(response).to have_gitlab_http_status(200)
end
it 'serializes diffs metadata with expected arguments' do
expected_options = {
environment: nil,
merge_request: merge_request,
merge_request_diff: merge_request.merge_request_diff,
merge_request_diffs: merge_request.merge_request_diffs,
start_version: nil,
start_sha: nil,
commit: nil,
latest_diff: true
}
expect_next_instance_of(DiffsMetadataSerializer) do |instance|
expect(instance).to receive(:represent)
.with(an_instance_of(Gitlab::Diff::FileCollection::MergeRequestDiff), expected_options)
.and_call_original
end
go
end
end
context 'with commit param' do
it 'returns success' do
go(commit_id: merge_request.diff_head_sha)
expect(response).to have_gitlab_http_status(200)
end
it 'serializes diffs metadata with expected arguments' do
expected_options = {
environment: nil,
merge_request: merge_request,
merge_request_diff: nil,
merge_request_diffs: merge_request.merge_request_diffs,
start_version: nil,
start_sha: nil,
commit: merge_request.diff_head_commit,
latest_diff: nil
}
expect_next_instance_of(DiffsMetadataSerializer) do |instance|
expect(instance).to receive(:represent)
.with(an_instance_of(Gitlab::Diff::FileCollection::Commit), expected_options)
.and_call_original
end
go(commit_id: merge_request.diff_head_sha)
end
end
end
describe 'GET diff_for_path' do describe 'GET diff_for_path' do
def diff_for_path(extra_params = {}) def diff_for_path(extra_params = {})
params = { params = {
......
# frozen_string_literal: true
require 'spec_helper'
describe DiffsMetadataEntity do
let(:user) { create(:user) }
let(:project) { create(:project, :repository) }
let(:request) { EntityRequest.new(project: project, current_user: user) }
let(:merge_request) { create(:merge_request_with_diffs, target_project: project, source_project: project) }
let(:merge_request_diffs) { merge_request.merge_request_diffs }
let(:merge_request_diff) { merge_request_diffs.last }
let(:entity) do
described_class.new(merge_request_diff.diffs,
request: request,
merge_request: merge_request,
merge_request_diffs: merge_request_diffs)
end
context 'as json' do
subject { entity.as_json }
it 'contain only required attributes' do
expect(subject.keys).to contain_exactly(
# Inherited attributes
:real_size, :size, :branch_name,
:target_branch_name, :commit, :merge_request_diff,
:start_version, :latest_diff, :latest_version_path,
:added_lines, :removed_lines, :render_overflow_warning,
:email_patch_path, :plain_diff_path,
:merge_request_diffs,
# Attributes
:diff_files
)
end
describe 'diff_files' do
it 'returns diff files metadata' do
payload =
DiffFileMetadataEntity.represent(merge_request_diff.diffs.diff_files).as_json
expect(subject[:diff_files]).to eq(payload)
end
end
end
end
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