Commit 957bc994 authored by Oswaldo Ferreira's avatar Oswaldo Ferreira
Browse files

Prepare suggestion implementation for multi-line

Adds the groundwork needed in order to persist multi-line suggestions,
while providing the parsing strategy which will be reused for the
**Preview** as well.
parent 5b9c8064dc5e
......@@ -77,8 +77,8 @@ def created_at_diff?(diff_refs)
def supports_suggestion?
return false unless noteable.supports_suggestion? && on_text?
# We don't want to trigger side-effects of `diff_file` call.
return false unless file = fetch_diff_file
return false unless line = file.line_for_position(self.original_position)
return false unless file = latest_diff_file
return false unless line = file.line_for_position(self.position)
line&.suggestible?
end
......
......@@ -66,6 +66,7 @@ def merge_request_diff
has_many :cached_closes_issues, through: :merge_requests_closing_issues, source: :issue
has_many :merge_request_pipelines, foreign_key: 'merge_request_id', class_name: 'Ci::Pipeline'
has_many :suggestions, through: :notes
has_many :merge_request_assignees
# Will be deprecated at https://gitlab.com/gitlab-org/gitlab-ce/issues/59457
......
# frozen_string_literal: true
class Suggestion < ApplicationRecord
include Suggestible
belongs_to :note, inverse_of: :suggestions
validates :note, presence: true
validates :commit_id, presence: true, if: :applied?
delegate :original_position, :position, :noteable, to: :note
delegate :position, :noteable, to: :note
scope :active, -> { where(outdated: false) }
def diff_file
note.latest_diff_file
end
def project
noteable.source_project
......@@ -19,37 +27,37 @@ def file_path
position.file_path
end
# For now, suggestions only serve as a way to send patches that
# will change a single line (being able to apply multiple in the same place),
# which explains `from_line` and `to_line` being the same line.
# We'll iterate on that in https://gitlab.com/gitlab-org/gitlab-ce/issues/53310
# when allowing multi-line suggestions.
def from_line
position.new_line
end
alias_method :to_line, :from_line
def from_original_line
original_position.new_line
end
alias_method :to_original_line, :from_original_line
# `from_line_index` and `to_line_index` represents diff/blob line numbers in
# index-like way (N-1).
def from_line_index
from_line - 1
end
alias_method :to_line_index, :from_line_index
def appliable?
return false unless note.supports_suggestion?
def to_line_index
to_line - 1
end
def appliable?(cached: true)
!applied? &&
noteable.opened? &&
!outdated?(cached: cached) &&
note.supports_suggestion? &&
different_content? &&
note.active?
end
# Overwrites outdated column
def outdated?(cached: true)
return super() if cached
return true unless diff_file
from_content != fetch_from_content
end
def target_line
position.new_line
end
private
def different_content?
......
# frozen_string_literal: true
module Suggestible
extend ActiveSupport::Concern
# This translates into limiting suggestion changes to `suggestion:-100+100`.
MAX_LINES_CONTEXT = 100.freeze
def fetch_from_content
diff_file.new_blob_lines_between(from_line, to_line).join
end
def from_line
real_above = [lines_above, MAX_LINES_CONTEXT].min
[target_line - real_above, 1].max
end
def to_line
real_below = [lines_below, MAX_LINES_CONTEXT].min
target_line + real_below
end
def diff_file
raise NotImplementedError
end
def target_line
raise NotImplementedError
end
end
......@@ -20,6 +20,7 @@ def refresh_merge_requests!
close_upon_missing_source_branch_ref
post_merge_manually_merged
reload_merge_requests
outdate_suggestions
reset_merge_when_pipeline_succeeds
mark_pending_todos_done
cache_merge_requests_closing_issues
......@@ -125,6 +126,14 @@ def branch_and_project_match?(merge_request)
merge_request.source_branch == @push.branch_name
end
def outdate_suggestions
outdate_service = Suggestions::OutdateService.new
merge_requests_for_source_branch.each do |merge_request|
outdate_service.execute(merge_request)
end
end
def reset_merge_when_pipeline_succeeds
merge_requests_for_source_branch.each(&:reset_merge_when_pipeline_succeeds)
end
......
......@@ -7,7 +7,7 @@ def initialize(current_user)
end
def execute(suggestion)
unless suggestion.appliable?
unless suggestion.appliable?(cached: false)
return error('Suggestion is not appliable')
end
......@@ -15,7 +15,7 @@ def execute(suggestion)
return error('The file has been changed')
end
diff_file = suggestion.note.latest_diff_file
diff_file = suggestion.diff_file
unless diff_file
return error('The file was not found')
......
......@@ -9,52 +9,24 @@ def initialize(note)
def execute
return unless @note.supports_suggestion?
diff_file = @note.latest_diff_file
return unless diff_file
suggestions = Banzai::SuggestionsParser.parse(@note.note)
# For single line suggestion we're only looking forward to
# change the line receiving the comment. Though, in
# https://gitlab.com/gitlab-org/gitlab-ce/issues/53310
# we'll introduce a ```suggestion:L<x>-<y>, so this will
# slightly change.
comment_line = @note.position.new_line
suggestions = Gitlab::Diff::SuggestionsParser.parse(@note.note,
project: @note.project,
position: @note.position)
rows =
suggestions.map.with_index do |suggestion, index|
from_content = changing_lines(diff_file, comment_line, comment_line)
creation_params =
suggestion.to_hash.slice(:from_content,
:to_content,
:lines_above,
:lines_below)
# The parsed suggestion doesn't have information about the correct
# ending characters (we may have a line break, or not), so we take
# this information from the last line being changed (last
# characters).
endline_chars = line_break_chars(from_content.lines.last)
to_content = "#{suggestion}#{endline_chars}"
{
note_id: @note.id,
from_content: from_content,
to_content: to_content,
relative_order: index
}
creation_params.merge!(note_id: @note.id, relative_order: index)
end
rows.in_groups_of(100, false) do |rows|
Gitlab::Database.bulk_insert('suggestions', rows)
end
end
private
def changing_lines(diff_file, from_line, to_line)
diff_file.new_blob_lines_between(from_line, to_line).join
end
def line_break_chars(line)
match = /\r\n|\r|\n/.match(line)
match[0] if match
end
end
end
# frozen_string_literal: true
module Suggestions
class OutdateService
def execute(merge_request)
# rubocop: disable CodeReuse/ActiveRecord
suggestions = merge_request.suggestions.active.includes(:note)
suggestions.find_in_batches(batch_size: 100) do |group|
outdatable_suggestion_ids = group.select do |suggestion|
suggestion.outdated?(cached: false)
end.map(&:id)
Suggestion.where(id: outdatable_suggestion_ids).update_all(outdated: true)
end
# rubocop: enable CodeReuse/ActiveRecord
end
end
end
---
title: Implements the creation strategy for multi-line suggestions
merge_request: 26057
author:
type: changed
......@@ -8,9 +8,9 @@ class AddMultiLineAttributesToSuggestion < ActiveRecord::Migration[5.0]
disable_ddl_transaction!
def up
add_column_with_default :suggestions, :outdated, :boolean, default: false, allow_null: false
add_column_with_default :suggestions, :lines_above, :integer, default: 0, allow_null: false
add_column_with_default :suggestions, :lines_below, :integer, default: 0, allow_null: false
add_column_with_default :suggestions, :outdated, :boolean, default: false, allow_null: false
end
def down
......
......@@ -2039,9 +2039,9 @@
t.string "commit_id"
t.text "from_content", null: false
t.text "to_content", null: false
t.boolean "outdated", default: false, null: false
t.integer "lines_above", default: 0, null: false
t.integer "lines_below", default: 0, null: false
t.boolean "outdated", default: false, null: false
t.index ["note_id", "relative_order"], name: "index_suggestions_on_note_id_and_relative_order", unique: true, using: :btree
end
......
......@@ -24,8 +24,6 @@ Example response:
```json
{
"id": 36,
"from_original_line": 10,
"to_original_line": 10,
"from_line": 10,
"to_line": 10,
"appliable": false,
......
......@@ -1558,8 +1558,6 @@ class ResourceLabelEvent < Grape::Entity
class Suggestion < Grape::Entity
expose :id
expose :from_original_line
expose :to_original_line
expose :from_line
expose :to_line
expose :appliable?, as: :appliable
......
# frozen_string_literal: true
# TODO: Delete when https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/26107
# exchange this parser by `Gitlab::Diff::SuggestionsParser`.
module Banzai
module SuggestionsParser
# Returns the content of each suggestion code block.
......
# frozen_string_literal: true
module Gitlab
module Diff
class Suggestion
include Suggestible
include Gitlab::Utils::StrongMemoize
attr_reader :diff_file, :lines_above, :lines_below,
:target_line
def initialize(text, line:, above:, below:, diff_file:)
@text = text
@target_line = line
@lines_above = above.to_i
@lines_below = below.to_i
@diff_file = diff_file
end
def to_hash
{
from_content: from_content,
to_content: to_content,
lines_above: @lines_above,
lines_below: @lines_below
}
end
def from_content
strong_memoize(:from_content) do
fetch_from_content
end
end
def to_content
# The parsed suggestion doesn't have information about the correct
# ending characters (we may have a line break, or not), so we take
# this information from the last line being changed (last
# characters).
endline_chars = line_break_chars(from_content.lines.last)
"#{@text}#{endline_chars}"
end
private
def line_break_chars(line)
match = /\r\n|\r|\n/.match(line)
match[0] if match
end
end
end
end
......@@ -5,6 +5,47 @@ module Diff
class SuggestionsParser
# Matches for instance "-1", "+1" or "-1+2".
SUGGESTION_CONTEXT = /^(\-(?<above>\d+))?(\+(?<below>\d+))?$/.freeze
class << self
# Returns an array of Gitlab::Diff::Suggestion which represents each
# suggestion in the given text.
#
def parse(text, position:, project:)
return [] unless position.complete?
html = Banzai.render(text, project: nil, no_original_data: true)
doc = Nokogiri::HTML(html)
suggestion_nodes = doc.search('pre.suggestion')
return [] if suggestion_nodes.empty?
diff_file = position.diff_file(project.repository)
suggestion_nodes.map do |node|
lang_param = node['data-lang-params']
lines_above, lines_below = nil
if lang_param && suggestion_params = fetch_suggestion_params(lang_param)
lines_above, lines_below =
suggestion_params[:above],
suggestion_params[:below]
end
Gitlab::Diff::Suggestion.new(node.text,
line: position.new_line,
above: lines_above.to_i,
below: lines_below.to_i,
diff_file: diff_file)
end
end
private
def fetch_suggestion_params(lang_param)
lang_param.match(SUGGESTION_CONTEXT)
end
end
end
end
end
......@@ -33,6 +33,7 @@ project_tree:
- :user
- merge_requests:
- :metrics
- :suggestions
- notes:
- :author
- events:
......
......@@ -16,5 +16,11 @@
applied true
commit_id { RepoHelpers.sample_commit.id }
end
trait :content_from_repo do
after(:build) do |suggestion, evaluator|
suggestion.from_content = suggestion.fetch_from_content
end
end
end
end
# frozen_string_literal: true
require 'spec_helper'
describe Gitlab::Diff::Suggestion do
shared_examples 'correct suggestion raw content' do
it 'returns correct raw data' do
expect(suggestion.to_hash).to include(from_content: expected_lines.join,
to_content: "#{text}\n",
lines_above: above,
lines_below: below)
end
end
let(:merge_request) { create(:merge_request) }
let(:project) { merge_request.project }
let(:position) do
Gitlab::Diff::Position.new(old_path: "files/ruby/popen.rb",
new_path: "files/ruby/popen.rb",
old_line: nil,
new_line: 9,
diff_refs: merge_request.diff_refs)
end
let(:diff_file) do
position.diff_file(project.repository)
end
let(:text) { "# parsed suggestion content\n# with comments" }
def blob_lines_data(from_line, to_line)
diff_file.new_blob_lines_between(from_line, to_line)
end
def blob_data
blob = diff_file.new_blob
blob.load_all_data!
blob.data
end
let(:suggestion) do
described_class.new(text, line: line, above: above, below: below, diff_file: diff_file)
end
describe '#to_hash' do
context 'when changing content surpasses the top limit' do
let(:line) { 4 }
let(:above) { 5 }
let(:below) { 2 }
let(:expected_above) { line - 1 }
let(:expected_below) { below }
let(:expected_lines) { blob_lines_data(line - expected_above, line + expected_below) }
it_behaves_like 'correct suggestion raw content'
end
context 'when changing content surpasses the amount of lines in the blob (bottom)' do
let(:line) { 5 }
let(:above) { 1 }
let(:below) { blob_data.lines.size + 10 }
let(:expected_below) { below }
let(:expected_above) { above }
let(:expected_lines) { blob_lines_data(line - expected_above, line + expected_below) }
it_behaves_like 'correct suggestion raw content'
end
context 'when lines are within blob lines boundary' do
let(:line) { 5 }
let(:above) { 2 }
let(:below) { 3 }
let(:expected_below) { below }
let(:expected_above) { above }
let(:expected_lines) { blob_lines_data(line - expected_above, line + expected_below) }
it_behaves_like 'correct suggestion raw content'
end
context 'when no extra lines (single-line suggestion)' do
let(:line) { 5 }
let(:above) { 0 }
let(:below) { 0 }
let(:expected_below) { below }
let(:expected_above) { above }
let(:expected_lines) { blob_lines_data(line - expected_above, line + expected_below) }
it_behaves_like 'correct suggestion raw content'
end
end
end
# frozen_string_literal: true
require 'spec_helper'
describe Gitlab::Diff::SuggestionsParser do
describe '.parse' do
let(:merge_request) { create(:merge_request) }
let(:project) { merge_request.project }
let(:position) do
Gitlab::Diff::Position.new(old_path: "files/ruby/popen.rb",
new_path: "files/ruby/popen.rb",
old_line: nil,
new_line: 9,
diff_refs: merge_request.diff_refs)
end
let(:diff_file) do
position.diff_file(project.repository)
end
subject do
described_class.parse(markdown, project: merge_request.project,
position: position)
end
def blob_lines_data(from_line, to_line)
diff_file.new_blob_lines_between(from_line, to_line).join
end
context 'single-line suggestions' do
let(:markdown) do
<<-MARKDOWN.strip_heredoc
```suggestion
foo
bar
```
```
nothing
```
```suggestion
xpto
baz
```
```thing
this is not a suggestion, it's a thing
```
MARKDOWN
end
it 'returns a list of Gitlab::Diff::Suggestion' do
expect(subject).to all(be_a(Gitlab::Diff::Suggestion))
expect(subject.size).to eq(2)
end
it 'parsed suggestion has correct data' do
from_line, to_line = position.new_line, position.new_line
expect(subject.first.to_hash).to include(from_content: blob_lines_data(from_line, to_line),
to_content: " foo\n bar\n",
lines_above: 0,
lines_below: 0)
expect(subject.second.to_hash).to include(from_content: blob_lines_data(from_line, to_line),
to_content: " xpto\n baz\n",
lines_above: 0,
lines_below: 0)
end
end
end
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