Merge branch 'issue_48084' into 'master'

Use a serializer to render diff lines

Closes #48084

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

app/controllers/projects/blob_controller.rb

@@ -127,7 +127,7 @@ class Projects::BlobController < Projects::ApplicationController
 
     add_match_line
 
-    render json: @lines
+    render json: DiffLineSerializer.new.represent(@lines)
   end
 
   def add_match_line

app/serializers/diff_file_entity.rb

@@ -135,12 +135,12 @@ class DiffFileEntity < Grape::Entity
   end
 
   # Used for inline diffs
-  expose :highlighted_diff_lines, if: -> (diff_file, _) { diff_file.text? } do |diff_file|
+  expose :highlighted_diff_lines, using: DiffLineEntity, if: -> (diff_file, _) { diff_file.text? } do |diff_file|
     diff_file.diff_lines_for_serializer
   end
 
   # Used for parallel diffs
-  expose :parallel_diff_lines, if: -> (diff_file, _) { diff_file.text? }
+  expose :parallel_diff_lines, using: DiffLineParallelEntity, if: -> (diff_file, _) { diff_file.text? }
 
   def current_user
     request.current_user

app/serializers/diff_line_entity.rb

+# frozen_string_literal: true
+
+class DiffLineEntity < Grape::Entity
+  expose :line_code
+  expose :type
+  expose :old_line
+  expose :new_line
+  expose :text
+  expose :meta_positions, as: :meta_data
+
+  expose :rich_text do |line|
+    line.rich_text || CGI.escapeHTML(line.text)
+  end
+end

app/serializers/diff_line_parallel_entity.rb

+# frozen_string_literal: true
+
+class DiffLineParallelEntity < Grape::Entity
+  expose :left, using: DiffLineEntity
+  expose :right, using: DiffLineEntity
+end

app/serializers/diff_line_serializer.rb

+# frozen_string_literal: true
+
+class DiffLineSerializer < BaseSerializer
+  entity DiffLineEntity
+end

app/serializers/discussion_entity.rb

@@ -43,7 +43,7 @@ class DiscussionEntity < Grape::Entity
     project_merge_request_discussion_path(discussion.project, discussion.noteable, discussion)
   end
 
-  expose :truncated_diff_lines, 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

lib/gitlab/diff/line.rb

@@ -79,16 +79,10 @@ module Gitlab
         }
       end
 
+      # We have to keep this here since it is still used for conflict resolution
+      # Conflict::File#as_json renders json diff lines in sections
       def as_json(opts = nil)
-        {
-          line_code: line_code,
-          type: type,
-          old_line: old_line,
-          new_line: new_line,
-          text: text,
-          rich_text: rich_text || CGI.escapeHTML(text),
-          meta_data: meta_positions
-        }
+        DiffLineSerializer.new.represent(self)
       end
 
       private

spec/fixtures/api/schemas/entities/diff_line.json

+{
+  "type": "object",
+  "required": ["type"],
+  "properties": {
+    "line_code": { "type": ["string", "null"] },
+    "type": { "type": ["string", "null"] },
+    "old_line": { "type": ["integer", "null"] },
+    "new_line": { "type": ["integer", "null"] },
+    "text": { "type": ["string"] },
+    "rich_text": { "type": ["string"] },
+    "meta_data": { "type": ["object", "null"] }
+  },
+  "additionalProperties": false
+}

spec/fixtures/api/schemas/entities/diff_line_parallel.json

+{
+  "required" : [
+    "left",
+    "right"
+  ],
+  "properties" : {
+    "left": { "$ref": "diff_line.json" },
+    "right": { "$ref": "diff_line.json" }
+  },
+  "additionalProperties": false
+}

spec/serializers/diff_file_entity_spec.rb

@@ -67,4 +67,21 @@ describe DiffFileEntity do
       end
     end
   end
+
+  context '#parallel_diff_lines' do
+    it 'exposes parallel diff lines correctly' do
+      response = subject
+
+      lines = response[:parallel_diff_lines]
+
+      # make sure at least one line is present for each side
+      expect(lines.map { |line| line[:right] }.compact).to be_present
+      expect(lines.map { |line| line[:left] }.compact).to be_present
+      # make sure all lines are in correct format
+      lines.each do |parallel_line|
+        expect(parallel_line[:left].as_json).to match_schema('entities/diff_line') if parallel_line[:left]
+        expect(parallel_line[:right].as_json).to match_schema('entities/diff_line') if parallel_line[:right]
+      end
+    end
+  end
 end

spec/serializers/diff_line_serializer_spec.rb

+require 'spec_helper'
+
+describe DiffLineSerializer do
+  let(:line) { Gitlab::Diff::Line.new('hello world', 'new', 1, nil, 1) }
+  let(:serializer) { described_class.new.represent(line) }
+
+  describe '#to_json' do
+    subject { serializer.to_json }
+
+    it 'matches the schema' do
+      expect(subject).to match_schema('entities/diff_line')
+    end
+
+    context 'when lines are parallel' do
+      let(:right_line) { Gitlab::Diff::Line.new('right line', 'new', 1, nil, 1) }
+      let(:left_line) { Gitlab::Diff::Line.new('left line', 'match', 1, nil, 1) }
+      let(:parallel_line) { [{ right: right_line, left: left_line }] }
+      let(:serializer) { described_class.new.represent(parallel_line, {}, DiffLineParallelEntity) }
+
+      it 'matches the schema' do
+        expect(subject).to match_schema('entities/diff_line_parallel')
+      end
+    end
+  end
+end