From 5bc54ca425866c37f6cdf6448d8ae2dcbabea3bf Mon Sep 17 00:00:00 2001 From: Felipe Artur Date: Thu, 16 Aug 2018 15:53:33 -0300 Subject: [PATCH] Use a serializer to render diff lines --- app/controllers/projects/blob_controller.rb | 2 +- app/serializers/diff_file_entity.rb | 4 +-- app/serializers/diff_line_entity.rb | 14 +++++++++++ app/serializers/diff_line_parallel_entity.rb | 6 +++++ app/serializers/diff_line_serializer.rb | 5 ++++ app/serializers/discussion_entity.rb | 2 +- lib/gitlab/diff/line.rb | 12 +++------ .../api/schemas/entities/diff_line.json | 14 +++++++++++ .../schemas/entities/diff_line_parallel.json | 11 ++++++++ spec/serializers/diff_file_entity_spec.rb | 17 +++++++++++++ spec/serializers/diff_line_serializer_spec.rb | 25 +++++++++++++++++++ 11 files changed, 99 insertions(+), 13 deletions(-) create mode 100644 app/serializers/diff_line_entity.rb create mode 100644 app/serializers/diff_line_parallel_entity.rb create mode 100644 app/serializers/diff_line_serializer.rb create mode 100644 spec/fixtures/api/schemas/entities/diff_line.json create mode 100644 spec/fixtures/api/schemas/entities/diff_line_parallel.json create mode 100644 spec/serializers/diff_line_serializer_spec.rb diff --git a/app/controllers/projects/blob_controller.rb b/app/controllers/projects/blob_controller.rb index ebc61264b39..56dafa31332 100644 --- a/app/controllers/projects/blob_controller.rb +++ b/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 diff --git a/app/serializers/diff_file_entity.rb b/app/serializers/diff_file_entity.rb index d49d4895d89..cbe6f200b86 100644 --- a/app/serializers/diff_file_entity.rb +++ b/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 diff --git a/app/serializers/diff_line_entity.rb b/app/serializers/diff_line_entity.rb new file mode 100644 index 00000000000..2119a1017d3 --- /dev/null +++ b/app/serializers/diff_line_entity.rb @@ -0,0 +1,14 @@ +# 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 diff --git a/app/serializers/diff_line_parallel_entity.rb b/app/serializers/diff_line_parallel_entity.rb new file mode 100644 index 00000000000..0438a67d51b --- /dev/null +++ b/app/serializers/diff_line_parallel_entity.rb @@ -0,0 +1,6 @@ +# frozen_string_literal: true + +class DiffLineParallelEntity < Grape::Entity + expose :left, using: DiffLineEntity + expose :right, using: DiffLineEntity +end diff --git a/app/serializers/diff_line_serializer.rb b/app/serializers/diff_line_serializer.rb new file mode 100644 index 00000000000..7f1f2d9aa7c --- /dev/null +++ b/app/serializers/diff_line_serializer.rb @@ -0,0 +1,5 @@ +# frozen_string_literal: true + +class DiffLineSerializer < BaseSerializer + entity DiffLineEntity +end diff --git a/app/serializers/discussion_entity.rb b/app/serializers/discussion_entity.rb index b8321037fa5..ed09db0f3f4 100644 --- a/app/serializers/discussion_entity.rb +++ b/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 diff --git a/lib/gitlab/diff/line.rb b/lib/gitlab/diff/line.rb index 1ab6df0b6ae..5b67cd46c48 100644 --- a/lib/gitlab/diff/line.rb +++ b/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 diff --git a/spec/fixtures/api/schemas/entities/diff_line.json b/spec/fixtures/api/schemas/entities/diff_line.json new file mode 100644 index 00000000000..66e8b443e1b --- /dev/null +++ b/spec/fixtures/api/schemas/entities/diff_line.json @@ -0,0 +1,14 @@ +{ + "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 +} diff --git a/spec/fixtures/api/schemas/entities/diff_line_parallel.json b/spec/fixtures/api/schemas/entities/diff_line_parallel.json new file mode 100644 index 00000000000..f924eb0c601 --- /dev/null +++ b/spec/fixtures/api/schemas/entities/diff_line_parallel.json @@ -0,0 +1,11 @@ +{ + "required" : [ + "left", + "right" + ], + "properties" : { + "left": { "$ref": "diff_line.json" }, + "right": { "$ref": "diff_line.json" } + }, + "additionalProperties": false +} diff --git a/spec/serializers/diff_file_entity_spec.rb b/spec/serializers/diff_file_entity_spec.rb index 00b2146dc86..3d90ce44dfb 100644 --- a/spec/serializers/diff_file_entity_spec.rb +++ b/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 diff --git a/spec/serializers/diff_line_serializer_spec.rb b/spec/serializers/diff_line_serializer_spec.rb new file mode 100644 index 00000000000..6dd8abd0579 --- /dev/null +++ b/spec/serializers/diff_line_serializer_spec.rb @@ -0,0 +1,25 @@ +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