Commit 73cc6715 authored by Phil Hughes's avatar Phil Hughes Committed by Tiger Watson

Fixes draft note race condition with whitespace changes

Fixes a race condition when draft notes exist on a line in a whitespace
diff but would end up getting an incorrect `line_code` set.
This is fixed by saving the `line_code` property into the database
to better match what the notes table does.

Changelog: fixed

Closes https://gitlab.com/gitlab-org/gitlab/-/issues/340815
parent d3de75ba
......@@ -92,7 +92,8 @@ class Projects::MergeRequests::DraftsController < Projects::MergeRequests::Appli
:commit_id,
:note,
:position,
:resolve_discussion
:resolve_discussion,
:line_code
).tap do |h|
# Old FE version will still be sending `draft_note[commit_id]` as 'undefined'.
# That can result to having a note linked to a commit with 'undefined' ID
......
......@@ -25,6 +25,7 @@ class DraftNote < ApplicationRecord
validates :merge_request_id, presence: true
validates :author_id, presence: true, uniqueness: { scope: [:merge_request_id, :discussion_id] }, if: :discussion_id?
validates :discussion_id, allow_nil: true, format: { with: /\A\h{40}\z/ }
validates :line_code, length: { maximum: 255 }, allow_nil: true
scope :authored_by, ->(u) { where(author_id: u.id) }
......@@ -89,7 +90,11 @@ class DraftNote < ApplicationRecord
end
def line_code
@line_code ||= diff_file&.line_code_for_position(original_position)
super.presence || find_line_code
end
def find_line_code
write_attribute(:line_code, diff_file&.line_code_for_position(original_position))
end
def publish_params
......
# frozen_string_literal: true
class AddLineCodeToDraftNotes < Gitlab::Database::Migration[1.0]
# rubocop:disable Migration/AddLimitToTextColumns
# limit is added in db/migrate/20211124095704_add_draft_notes_line_code_text_limit.rb
def change
add_column :draft_notes, :line_code, :text
end
# rubocop:enable Migration/AddLimitToTextColumns
end
# frozen_string_literal: true
class AddDraftNotesLineCodeTextLimit < Gitlab::Database::Migration[1.0]
disable_ddl_transaction!
def up
add_text_limit :draft_notes, :line_code, 255
end
def down
remove_text_limit :draft_notes, :line_code
end
end
674a44e70291d6ed04318a5f6b639d216f2c26c43d15cb00e59b06cc6f6cc401
\ No newline at end of file
1f5ed9e7af3f56d0e11d1a2bb78a7430ce05af49c8102d1c75c8ff84ae4e1c6d
\ No newline at end of file
......@@ -13687,7 +13687,9 @@ CREATE TABLE draft_notes (
"position" text,
original_position text,
change_position text,
commit_id bytea
commit_id bytea,
line_code text,
CONSTRAINT check_c497a94a0e CHECK ((char_length(line_code) <= 255))
);
CREATE SEQUENCE draft_notes_id_seq
......@@ -20,6 +20,28 @@ RSpec.describe DraftNote do
it { is_expected.to delegate_method(:file_identifier_hash).to(:diff_file).allow_nil }
end
describe '#line_code' do
describe 'stored line_code' do
let(:draft_note) { build(:draft_note, merge_request: merge_request, line_code: '1234567890') }
it 'returns stored line_code' do
expect(draft_note.line_code).to eq('1234567890')
end
end
describe 'none stored line_code' do
let(:draft_note) { build(:draft_note, merge_request: merge_request) }
before do
allow(draft_note).to receive(:find_line_code).and_return('none stored line_code')
end
it 'returns found line_code' do
expect(draft_note.line_code).to eq('none stored line_code')
end
end
end
describe '#diff_file' do
let(:draft_note) { build(:draft_note, merge_request: merge_request) }
......
......@@ -92,7 +92,7 @@ RSpec.describe DraftNotes::CreateService do
expect(merge_request).to receive_message_chain(:diffs, :clear_cache)
create_draft(note: 'This is a test')
create_draft(note: 'This is a test', line_code: '123')
end
end
......@@ -104,7 +104,7 @@ RSpec.describe DraftNotes::CreateService do
expect(merge_request).not_to receive(:diffs)
create_draft(note: 'This is a test')
create_draft(note: 'This is a test', line_code: '123')
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