Merge branch 'osw-remove-unnused-data-from-diff-discussions' into 'master'

Remove un-used data from discussions endpoint

Closes #54288

See merge request gitlab-org/gitlab-ce!23570

10 files changed, 197 insertions, 154 deletions

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:
     '<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 = [
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