diff --git a/app/serializers/diff_file_base_entity.rb b/app/serializers/diff_file_base_entity.rb new file mode 100644 index 00000000000..06a8db78476 --- /dev/null +++ b/app/serializers/diff_file_base_entity.rb @@ -0,0 +1,101 @@ +# 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 diff --git a/app/serializers/diff_file_entity.rb b/app/serializers/diff_file_entity.rb index e570039d47e..f0881829efd 100644 --- a/app/serializers/diff_file_entity.rb +++ b/app/serializers/diff_file_entity.rb @@ -1,64 +1,12 @@ # frozen_string_literal: true -class DiffFileEntity < Grape::Entity - include RequestAwareEntity +class DiffFileEntity < DiffFileBaseEntity include CommitsHelper - include DiffHelper - include SubmoduleHelper - include BlobHelper 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 :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 :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| merge_request = options[:merge_request] @@ -76,36 +24,6 @@ class DiffFileEntity < Grape::Entity ) 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| merge_request = options[:merge_request] @@ -146,18 +64,4 @@ class DiffFileEntity < Grape::Entity # Used for parallel diffs 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 diff --git a/app/serializers/discussion_diff_file_entity.rb b/app/serializers/discussion_diff_file_entity.rb new file mode 100644 index 00000000000..419e7edf94f --- /dev/null +++ b/app/serializers/discussion_diff_file_entity.rb @@ -0,0 +1,4 @@ +# frozen_string_literal: true + +class DiscussionDiffFileEntity < DiffFileBaseEntity +end diff --git a/app/serializers/discussion_entity.rb b/app/serializers/discussion_entity.rb index b6786a0d597..b2d9d52bd22 100644 --- a/app/serializers/discussion_entity.rb +++ b/app/serializers/discussion_entity.rb @@ -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) 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 @@ -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 :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 :commit_id diff --git a/changelogs/unreleased/osw-remove-unnused-data-from-diff-discussions.yml b/changelogs/unreleased/osw-remove-unnused-data-from-diff-discussions.yml new file mode 100644 index 00000000000..58d9a19d038 --- /dev/null +++ b/changelogs/unreleased/osw-remove-unnused-data-from-diff-discussions.yml @@ -0,0 +1,5 @@ +--- +title: Remove unused data from discussions endpoint +merge_request: 23570 +author: +type: performance diff --git a/spec/javascripts/diffs/mock_data/diff_discussions.js b/spec/javascripts/diffs/mock_data/diff_discussions.js index 5ffe5a366ba..44313caba29 100644 --- a/spec/javascripts/diffs/mock_data/diff_discussions.js +++ b/spec/javascripts/diffs/mock_data/diff_discussions.js @@ -489,8 +489,6 @@ export default { diff_discussion: true, truncated_diff_lines: '\n\n \n\n\n1\n\n - Bad dates\n\n\n\n\n \n\n\n2\n\n\n\n\n', - image_diff_html: - '
\n
\n
\n
\nCHANGELOG\n
\n

\n22.3 KB\n|\nW:\n\n|\nH:\n\n

\n
\n
\n
\nCHANGELOG\n
\n\n

\n22.3 KB\n|\nW:\n\n|\nH:\n\n

\n
\n
\n
\n
\n
\nCHANGELOG\n
\n
\n
\nCHANGELOG\n
\n\n
\n\n\n\n\n
\n
\n
\n
\n
\nCHANGELOG\n
\n
\nCHANGELOG\n
\n\n
\n
\n
\n
\n
\n
\n
\n
\n
\n
\n
\n\n
\n', }; export const imageDiffDiscussions = [ diff --git a/spec/serializers/diff_file_entity_spec.rb b/spec/serializers/diff_file_entity_spec.rb index 7497b8f27bd..073c13c2cbb 100644 --- a/spec/serializers/diff_file_entity_spec.rb +++ b/spec/serializers/diff_file_entity_spec.rb @@ -13,39 +13,6 @@ describe DiffFileEntity do 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 it_behaves_like 'diff file entity' end diff --git a/spec/serializers/discussion_diff_file_entity_spec.rb b/spec/serializers/discussion_diff_file_entity_spec.rb new file mode 100644 index 00000000000..101ac918a98 --- /dev/null +++ b/spec/serializers/discussion_diff_file_entity_spec.rb @@ -0,0 +1,39 @@ +# 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 diff --git a/spec/serializers/discussion_entity_spec.rb b/spec/serializers/discussion_entity_spec.rb index 0590304e832..138749b0fdf 100644 --- a/spec/serializers/discussion_entity_spec.rb +++ b/spec/serializers/discussion_entity_spec.rb @@ -74,13 +74,5 @@ describe DiscussionEntity do :active ) 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 diff --git a/spec/support/shared_examples/serializers/diff_file_entity_examples.rb b/spec/support/shared_examples/serializers/diff_file_entity_examples.rb new file mode 100644 index 00000000000..b8065886c42 --- /dev/null +++ b/spec/support/shared_examples/serializers/diff_file_entity_examples.rb @@ -0,0 +1,46 @@ +# 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