Commit 26b94bcc authored by Oswaldo Ferreira's avatar Oswaldo Ferreira Committed by Stan Hu

Remove unused data from discussions endpoint

We don't need a series of attributes to render diff files on
discussions.json request. Therefore this MR removes lots of unnecessary
attributes from the request, mainly the highlighted diff lines, which
are pretty expensive.
parent 9a78524f
# frozen_string_literal: true
class DiffFileBaseEntity < Grape::Entity
include RequestAwareEntity
include BlobHelper
include SubmoduleHelper
include DiffHelper
include TreeHelper
include ChecksCollaboration
include Gitlab::Utils::StrongMemoize
expose :content_sha
expose :submodule?, as: :submodule
expose :submodule_link do |diff_file|
memoized_submodule_links(diff_file).first
end
expose :submodule_tree_url do |diff_file|
memoized_submodule_links(diff_file).last
end
expose :edit_path, if: -> (_, options) { options[:merge_request] } do |diff_file|
merge_request = options[:merge_request]
options = merge_request.persisted? ? { from_merge_request_iid: merge_request.iid } : {}
next unless merge_request.source_project
project_edit_blob_path(merge_request.source_project,
tree_join(merge_request.source_branch, diff_file.new_path),
options)
end
expose :old_path_html do |diff_file|
old_path, _ = mark_inline_diffs(diff_file.old_path, diff_file.new_path)
old_path
end
expose :new_path_html do |diff_file|
_, new_path = mark_inline_diffs(diff_file.old_path, diff_file.new_path)
new_path
end
expose :formatted_external_url, if: -> (_, options) { options[:environment] } do |diff_file|
options[:environment].formatted_external_url
end
expose :external_url, if: -> (_, options) { options[:environment] } do |diff_file|
options[:environment].external_url_for(diff_file.new_path, diff_file.content_sha)
end
expose :blob, using: BlobEntity
expose :can_modify_blob do |diff_file|
merge_request = options[:merge_request]
next unless diff_file.blob
if merge_request&.source_project && current_user
can_modify_blob?(diff_file.blob, merge_request.source_project, merge_request.source_branch)
else
false
end
end
expose :file_hash do |diff_file|
Digest::SHA1.hexdigest(diff_file.file_path)
end
expose :file_path
expose :old_path
expose :new_path
expose :new_file?, as: :new_file
expose :collapsed?, as: :collapsed
expose :text?, as: :text
expose :diff_refs
expose :stored_externally?, as: :stored_externally
expose :external_storage
expose :renamed_file?, as: :renamed_file
expose :deleted_file?, as: :deleted_file
expose :mode_changed?, as: :mode_changed
expose :a_mode
expose :b_mode
private
def memoized_submodule_links(diff_file)
strong_memoize(:submodule_links) do
if diff_file.submodule?
submodule_links(diff_file.blob, diff_file.content_sha, diff_file.repository)
else
[]
end
end
end
def current_user
request.current_user
end
end
# frozen_string_literal: true # frozen_string_literal: true
class DiffFileEntity < Grape::Entity class DiffFileEntity < DiffFileBaseEntity
include RequestAwareEntity
include CommitsHelper include CommitsHelper
include DiffHelper
include SubmoduleHelper
include BlobHelper
include IconsHelper include IconsHelper
include TreeHelper
include ChecksCollaboration
include Gitlab::Utils::StrongMemoize
expose :submodule?, as: :submodule
expose :submodule_link do |diff_file|
memoized_submodule_links(diff_file).first
end
expose :submodule_tree_url do |diff_file|
memoized_submodule_links(diff_file).last
end
expose :blob, using: BlobEntity
expose :can_modify_blob do |diff_file|
merge_request = options[:merge_request]
next unless diff_file.blob
if merge_request&.source_project && current_user
can_modify_blob?(diff_file.blob, merge_request.source_project, merge_request.source_branch)
else
false
end
end
expose :file_hash do |diff_file|
Digest::SHA1.hexdigest(diff_file.file_path)
end
expose :file_path
expose :too_large?, as: :too_large expose :too_large?, as: :too_large
expose :collapsed?, as: :collapsed
expose :new_file?, as: :new_file
expose :deleted_file?, as: :deleted_file
expose :renamed_file?, as: :renamed_file
expose :mode_changed?, as: :mode_changed
expose :old_path
expose :new_path
expose :mode_changed?, as: :mode_changed
expose :a_mode
expose :b_mode
expose :text?, as: :text
expose :added_lines expose :added_lines
expose :removed_lines expose :removed_lines
expose :diff_refs
expose :content_sha
expose :stored_externally?, as: :stored_externally
expose :external_storage
expose :load_collapsed_diff_url, if: -> (diff_file, options) { diff_file.text? && options[:merge_request] } do |diff_file| expose :load_collapsed_diff_url, if: -> (diff_file, options) { diff_file.text? && options[:merge_request] } do |diff_file|
merge_request = options[:merge_request] merge_request = options[:merge_request]
...@@ -76,36 +24,6 @@ class DiffFileEntity < Grape::Entity ...@@ -76,36 +24,6 @@ class DiffFileEntity < Grape::Entity
) )
end end
expose :formatted_external_url, if: -> (_, options) { options[:environment] } do |diff_file|
options[:environment].formatted_external_url
end
expose :external_url, if: -> (_, options) { options[:environment] } do |diff_file|
options[:environment].external_url_for(diff_file.new_path, diff_file.content_sha)
end
expose :old_path_html do |diff_file|
old_path, _ = mark_inline_diffs(diff_file.old_path, diff_file.new_path)
old_path
end
expose :new_path_html do |diff_file|
_, new_path = mark_inline_diffs(diff_file.old_path, diff_file.new_path)
new_path
end
expose :edit_path, if: -> (_, options) { options[:merge_request] } do |diff_file|
merge_request = options[:merge_request]
options = merge_request.persisted? ? { from_merge_request_iid: merge_request.iid } : {}
next unless merge_request.source_project
project_edit_blob_path(merge_request.source_project,
tree_join(merge_request.source_branch, diff_file.new_path),
options)
end
expose :view_path, if: -> (_, options) { options[:merge_request] } do |diff_file| expose :view_path, if: -> (_, options) { options[:merge_request] } do |diff_file|
merge_request = options[:merge_request] merge_request = options[:merge_request]
...@@ -146,18 +64,4 @@ class DiffFileEntity < Grape::Entity ...@@ -146,18 +64,4 @@ class DiffFileEntity < Grape::Entity
# Used for parallel diffs # Used for parallel diffs
expose :parallel_diff_lines, using: DiffLineParallelEntity, if: -> (diff_file, _) { diff_file.text? } expose :parallel_diff_lines, using: DiffLineParallelEntity, if: -> (diff_file, _) { diff_file.text? }
def current_user
request.current_user
end
def memoized_submodule_links(diff_file)
strong_memoize(:submodule_links) do
if diff_file.submodule?
submodule_links(diff_file.blob, diff_file.content_sha, diff_file.repository)
else
[]
end
end
end
end end
# frozen_string_literal: true
class DiscussionDiffFileEntity < DiffFileBaseEntity
end
...@@ -36,7 +36,7 @@ class DiscussionEntity < Grape::Entity ...@@ -36,7 +36,7 @@ class DiscussionEntity < Grape::Entity
new_project_issue_path(discussion.project, merge_request_to_resolve_discussions_of: discussion.noteable.iid, discussion_to_resolve: discussion.id) new_project_issue_path(discussion.project, merge_request_to_resolve_discussions_of: discussion.noteable.iid, discussion_to_resolve: discussion.id)
end end
expose :diff_file, using: DiffFileEntity, if: -> (d, _) { d.diff_discussion? } expose :diff_file, using: DiscussionDiffFileEntity, if: -> (d, _) { d.diff_discussion? }
expose :diff_discussion?, as: :diff_discussion expose :diff_discussion?, as: :diff_discussion
...@@ -46,19 +46,6 @@ class DiscussionEntity < Grape::Entity ...@@ -46,19 +46,6 @@ class DiscussionEntity < Grape::Entity
expose :truncated_diff_lines, using: DiffLineEntity, if: -> (d, _) { d.diff_discussion? && d.on_text? && (d.expanded? || render_truncated_diff_lines?) } expose :truncated_diff_lines, using: DiffLineEntity, if: -> (d, _) { d.diff_discussion? && d.on_text? && (d.expanded? || render_truncated_diff_lines?) }
expose :image_diff_html, if: -> (d, _) { d.diff_discussion? && d.on_image? } do |discussion|
diff_file = discussion.diff_file
partial = diff_file.new_file? || diff_file.deleted_file? ? 'single_image_diff' : 'replaced_image_diff'
options[:context].render_to_string(
partial: "projects/diffs/#{partial}",
locals: { diff_file: diff_file,
position: discussion.position.to_json,
click_to_comment: false },
layout: false,
formats: [:html]
)
end
expose :for_commit?, as: :for_commit expose :for_commit?, as: :for_commit
expose :commit_id expose :commit_id
......
---
title: Remove unused data from discussions endpoint
merge_request: 23570
author:
type: performance
...@@ -489,8 +489,6 @@ export default { ...@@ -489,8 +489,6 @@ export default {
diff_discussion: true, diff_discussion: true,
truncated_diff_lines: truncated_diff_lines:
'<tr class="line_holder new" id="">\n<td class="diff-line-num new old_line" data-linenumber="1">\n \n</td>\n<td class="diff-line-num new new_line" data-linenumber="1">\n1\n</td>\n<td class="line_content new noteable_line"><span id="LC1" class="line" lang="plaintext"> - Bad dates</span>\n</td>\n</tr>\n<tr class="line_holder new" id="">\n<td class="diff-line-num new old_line" data-linenumber="1">\n \n</td>\n<td class="diff-line-num new new_line" data-linenumber="2">\n2\n</td>\n<td class="line_content new noteable_line"><span id="LC2" class="line" lang="plaintext"></span>\n</td>\n</tr>\n', '<tr class="line_holder new" id="">\n<td class="diff-line-num new old_line" data-linenumber="1">\n \n</td>\n<td class="diff-line-num new new_line" data-linenumber="1">\n1\n</td>\n<td class="line_content new noteable_line"><span id="LC1" class="line" lang="plaintext"> - Bad dates</span>\n</td>\n</tr>\n<tr class="line_holder new" id="">\n<td class="diff-line-num new old_line" data-linenumber="1">\n \n</td>\n<td class="diff-line-num new new_line" data-linenumber="2">\n2\n</td>\n<td class="line_content new noteable_line"><span id="LC2" class="line" lang="plaintext"></span>\n</td>\n</tr>\n',
image_diff_html:
'<div class="image js-replaced-image" data="">\n<div class="two-up view">\n<div class="wrap">\n<div class="frame deleted">\n<img alt="CHANGELOG" src="http://localhost:3000/gitlab-org/gitlab-test/raw/e63f41fe459e62e1228fcef60d7189127aeba95a/CHANGELOG" />\n</div>\n<p class="image-info hide">\n<span class="meta-filesize">22.3 KB</span>\n|\n<strong>W:</strong>\n<span class="meta-width"></span>\n|\n<strong>H:</strong>\n<span class="meta-height"></span>\n</p>\n</div>\n<div class="wrap">\n<div class="added frame js-image-frame" data-note-type="DiffNote" data-position="{&quot;base_sha&quot;:&quot;e63f41fe459e62e1228fcef60d7189127aeba95a&quot;,&quot;start_sha&quot;:&quot;d9eaefe5a676b820c57ff18cf5b68316025f7962&quot;,&quot;head_sha&quot;:&quot;c48ee0d1bf3b30453f5b32250ce03134beaa6d13&quot;,&quot;old_path&quot;:&quot;CHANGELOG&quot;,&quot;new_path&quot;:&quot;CHANGELOG&quot;,&quot;position_type&quot;:&quot;text&quot;,&quot;old_line&quot;:null,&quot;new_line&quot;:2}">\n<img alt="CHANGELOG" draggable="false" src="http://localhost:3000/gitlab-org/gitlab-test/raw/c48ee0d1bf3b30453f5b32250ce03134beaa6d13/CHANGELOG" />\n</div>\n\n<p class="image-info hide">\n<span class="meta-filesize">22.3 KB</span>\n|\n<strong>W:</strong>\n<span class="meta-width"></span>\n|\n<strong>H:</strong>\n<span class="meta-height"></span>\n</p>\n</div>\n</div>\n<div class="swipe view hide">\n<div class="swipe-frame">\n<div class="frame deleted">\n<img alt="CHANGELOG" src="http://localhost:3000/gitlab-org/gitlab-test/raw/e63f41fe459e62e1228fcef60d7189127aeba95a/CHANGELOG" />\n</div>\n<div class="swipe-wrap">\n<div class="added frame js-image-frame" data-note-type="DiffNote" data-position="{&quot;base_sha&quot;:&quot;e63f41fe459e62e1228fcef60d7189127aeba95a&quot;,&quot;start_sha&quot;:&quot;d9eaefe5a676b820c57ff18cf5b68316025f7962&quot;,&quot;head_sha&quot;:&quot;c48ee0d1bf3b30453f5b32250ce03134beaa6d13&quot;,&quot;old_path&quot;:&quot;CHANGELOG&quot;,&quot;new_path&quot;:&quot;CHANGELOG&quot;,&quot;position_type&quot;:&quot;text&quot;,&quot;old_line&quot;:null,&quot;new_line&quot;:2}">\n<img alt="CHANGELOG" draggable="false" src="http://localhost:3000/gitlab-org/gitlab-test/raw/c48ee0d1bf3b30453f5b32250ce03134beaa6d13/CHANGELOG" />\n</div>\n\n</div>\n<span class="swipe-bar">\n<span class="top-handle"></span>\n<span class="bottom-handle"></span>\n</span>\n</div>\n</div>\n<div class="onion-skin view hide">\n<div class="onion-skin-frame">\n<div class="frame deleted">\n<img alt="CHANGELOG" src="http://localhost:3000/gitlab-org/gitlab-test/raw/e63f41fe459e62e1228fcef60d7189127aeba95a/CHANGELOG" />\n</div>\n<div class="added frame js-image-frame" data-note-type="DiffNote" data-position="{&quot;base_sha&quot;:&quot;e63f41fe459e62e1228fcef60d7189127aeba95a&quot;,&quot;start_sha&quot;:&quot;d9eaefe5a676b820c57ff18cf5b68316025f7962&quot;,&quot;head_sha&quot;:&quot;c48ee0d1bf3b30453f5b32250ce03134beaa6d13&quot;,&quot;old_path&quot;:&quot;CHANGELOG&quot;,&quot;new_path&quot;:&quot;CHANGELOG&quot;,&quot;position_type&quot;:&quot;text&quot;,&quot;old_line&quot;:null,&quot;new_line&quot;:2}">\n<img alt="CHANGELOG" draggable="false" src="http://localhost:3000/gitlab-org/gitlab-test/raw/c48ee0d1bf3b30453f5b32250ce03134beaa6d13/CHANGELOG" />\n</div>\n\n<div class="controls">\n<div class="transparent"></div>\n<div class="drag-track">\n<div class="dragger" style="left: 0px;"></div>\n</div>\n<div class="opaque"></div>\n</div>\n</div>\n</div>\n</div>\n<div class="view-modes hide">\n<ul class="view-modes-menu">\n<li class="two-up" data-mode="two-up">2-up</li>\n<li class="swipe" data-mode="swipe">Swipe</li>\n<li class="onion-skin" data-mode="onion-skin">Onion skin</li>\n</ul>\n</div>\n',
}; };
export const imageDiffDiscussions = [ export const imageDiffDiscussions = [
......
...@@ -13,39 +13,6 @@ describe DiffFileEntity do ...@@ -13,39 +13,6 @@ describe DiffFileEntity do
subject { entity.as_json } subject { entity.as_json }
shared_examples 'diff file entity' do
it 'exposes correct attributes' do
expect(subject).to include(
:submodule, :submodule_link, :submodule_tree_url, :file_path,
:deleted_file, :old_path, :new_path, :mode_changed,
:a_mode, :b_mode, :text, :old_path_html,
:new_path_html, :highlighted_diff_lines, :parallel_diff_lines,
:blob, :file_hash, :added_lines, :removed_lines, :diff_refs, :content_sha,
:stored_externally, :external_storage, :too_large, :collapsed, :new_file,
:context_lines_path
)
end
it 'includes viewer' do
expect(subject[:viewer].with_indifferent_access)
.to match_schema('entities/diff_viewer')
end
# Converted diff files from GitHub import does not contain blob file
# and content sha.
context 'when diff file does not have a blob and content sha' do
it 'exposes some attributes as nil' do
allow(diff_file).to receive(:content_sha).and_return(nil)
allow(diff_file).to receive(:blob).and_return(nil)
expect(subject[:context_lines_path]).to be_nil
expect(subject[:view_path]).to be_nil
expect(subject[:highlighted_diff_lines]).to be_nil
expect(subject[:can_modify_blob]).to be_nil
end
end
end
context 'when there is no merge request' do context 'when there is no merge request' do
it_behaves_like 'diff file entity' it_behaves_like 'diff file entity'
end end
......
# frozen_string_literal: true
require 'spec_helper'
describe DiscussionDiffFileEntity do
include RepoHelpers
let(:project) { create(:project, :repository) }
let(:repository) { project.repository }
let(:commit) { project.commit(sample_commit.id) }
let(:diff_refs) { commit.diff_refs }
let(:diff) { commit.raw_diffs.first }
let(:diff_file) { Gitlab::Diff::File.new(diff, diff_refs: diff_refs, repository: repository) }
let(:entity) { described_class.new(diff_file, request: {}) }
subject { entity.as_json }
context 'when there is no merge request' do
it_behaves_like 'diff file discussion entity'
end
context 'when there is a merge request' do
let(:user) { create(:user) }
let(:request) { EntityRequest.new(project: project, current_user: user) }
let(:merge_request) { create(:merge_request, source_project: project, target_project: project) }
let(:entity) { described_class.new(diff_file, request: request, merge_request: merge_request) }
it_behaves_like 'diff file discussion entity'
it 'exposes additional attributes' do
expect(subject).to include(:edit_path)
end
it 'exposes no diff lines' do
expect(subject).not_to include(:highlighted_diff_lines,
:parallel_diff_lines)
end
end
end
...@@ -74,13 +74,5 @@ describe DiscussionEntity do ...@@ -74,13 +74,5 @@ describe DiscussionEntity do
:active :active
) )
end end
context 'when diff file is a image' do
it 'exposes image attributes' do
allow(discussion).to receive(:on_image?).and_return(true)
expect(subject.keys).to include(:image_diff_html)
end
end
end end
end end
# frozen_string_literal: true
shared_examples 'diff file base entity' do
it 'exposes essential attributes' do
expect(subject).to include(:content_sha, :submodule, :submodule_link,
:submodule_tree_url, :old_path_html,
:new_path_html, :blob, :can_modify_blob,
:file_hash, :file_path, :old_path, :new_path,
:collapsed, :text, :diff_refs, :stored_externally,
:external_storage, :renamed_file, :deleted_file,
:mode_changed, :a_mode, :b_mode, :new_file)
end
# Converted diff files from GitHub import does not contain blob file
# and content sha.
context 'when diff file does not have a blob and content sha' do
it 'exposes some attributes as nil' do
allow(diff_file).to receive(:content_sha).and_return(nil)
allow(diff_file).to receive(:blob).and_return(nil)
expect(subject[:context_lines_path]).to be_nil
expect(subject[:view_path]).to be_nil
expect(subject[:highlighted_diff_lines]).to be_nil
expect(subject[:can_modify_blob]).to be_nil
end
end
end
shared_examples 'diff file entity' do
it_behaves_like 'diff file base entity'
it 'exposes correct attributes' do
expect(subject).to include(:too_large, :added_lines, :removed_lines,
:context_lines_path, :highlighted_diff_lines,
:parallel_diff_lines)
end
it 'includes viewer' do
expect(subject[:viewer].with_indifferent_access)
.to match_schema('entities/diff_viewer')
end
end
shared_examples 'diff file discussion entity' do
it_behaves_like 'diff file base entity'
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