Fix Bitbucket import

In ebf16ada85
we introduced a SHA validator, to ensure that the data provided in
merge request diffs, was legit. Nevertheless, the validator
assumed that the SHA should be 40 chars long.

When we import a project from BitBucket, the retrieved SHA is
shorter (12 chars long). Therefore, this validator prevented to
create a valid MergeRequestDiff for ever MergeRequest (triggering
an exception).
This commit is contained in:
Francisco Javier López 2019-03-14 10:05:17 +00:00 committed by Douwe Maan
parent d0053d5171
commit 150f7c1e9c
4 changed files with 31 additions and 5 deletions

View file

@ -2,7 +2,7 @@
class ShaValidator < ActiveModel::EachValidator class ShaValidator < ActiveModel::EachValidator
def validate_each(record, attribute, value) def validate_each(record, attribute, value)
return if value.blank? || value.match(/\A\h{40}\z/) return if value.blank? || Commit.valid_hash?(value)
record.errors.add(attribute, 'is not a valid SHA') record.errors.add(attribute, 'is not a valid SHA')
end end

View file

@ -0,0 +1,5 @@
---
title: Fix bug in BitBucket imports with SHA shorter than 40 chars
merge_request: 26050
author:
type: fixed

View file

@ -95,6 +95,9 @@ describe Gitlab::BitbucketImport::Importer do
subject { described_class.new(project) } subject { described_class.new(project) }
describe '#import_pull_requests' do describe '#import_pull_requests' do
let(:source_branch_sha) { sample.commits.last }
let(:target_branch_sha) { sample.commits.first }
before do before do
allow(subject).to receive(:import_wiki) allow(subject).to receive(:import_wiki)
allow(subject).to receive(:import_issues) allow(subject).to receive(:import_issues)
@ -102,9 +105,9 @@ describe Gitlab::BitbucketImport::Importer do
pull_request = instance_double( pull_request = instance_double(
Bitbucket::Representation::PullRequest, Bitbucket::Representation::PullRequest,
iid: 10, iid: 10,
source_branch_sha: sample.commits.last, source_branch_sha: source_branch_sha,
source_branch_name: Gitlab::Git::BRANCH_REF_PREFIX + sample.source_branch, source_branch_name: Gitlab::Git::BRANCH_REF_PREFIX + sample.source_branch,
target_branch_sha: sample.commits.first, target_branch_sha: target_branch_sha,
target_branch_name: Gitlab::Git::BRANCH_REF_PREFIX + sample.target_branch, target_branch_name: Gitlab::Git::BRANCH_REF_PREFIX + sample.target_branch,
title: 'This is a title', title: 'This is a title',
description: 'This is a test pull request', description: 'This is a test pull request',
@ -162,6 +165,19 @@ describe Gitlab::BitbucketImport::Importer do
expect(reply_note).to be_a(DiffNote) expect(reply_note).to be_a(DiffNote)
expect(reply_note.note).to eq(@reply.note) expect(reply_note.note).to eq(@reply.note)
end end
context "when branches' sha is not found in the repository" do
let(:source_branch_sha) { 'a' * Commit::MIN_SHA_LENGTH }
let(:target_branch_sha) { 'b' * Commit::MIN_SHA_LENGTH }
it 'uses the pull request sha references' do
expect { subject.execute }.to change { MergeRequest.count }.by(1)
merge_request_diff = MergeRequest.first.merge_request_diff
expect(merge_request_diff.head_commit_sha).to eq source_branch_sha
expect(merge_request_diff.start_commit_sha).to eq target_branch_sha
end
end
end end
context 'issues statuses' do context 'issues statuses' do

View file

@ -4,7 +4,7 @@ require 'spec_helper'
describe ShaValidator do describe ShaValidator do
let(:validator) { described_class.new(attributes: [:base_commit_sha]) } let(:validator) { described_class.new(attributes: [:base_commit_sha]) }
let(:merge_diff) { build(:merge_request_diff) } let!(:merge_diff) { build(:merge_request_diff) }
subject { validator.validate_each(merge_diff, :base_commit_sha, value) } subject { validator.validate_each(merge_diff, :base_commit_sha, value) }
@ -12,6 +12,8 @@ describe ShaValidator do
let(:value) { nil } let(:value) { nil }
it 'does not add any error if value is empty' do it 'does not add any error if value is empty' do
expect(Commit).not_to receive(:valid_hash?)
subject subject
expect(merge_diff.errors).to be_empty expect(merge_diff.errors).to be_empty
@ -21,7 +23,9 @@ describe ShaValidator do
context 'with valid sha' do context 'with valid sha' do
let(:value) { Digest::SHA1.hexdigest(SecureRandom.hex) } let(:value) { Digest::SHA1.hexdigest(SecureRandom.hex) }
it 'does not add any error if value is empty' do it 'does not add any error' do
expect(Commit).to receive(:valid_hash?).and_call_original
subject subject
expect(merge_diff.errors).to be_empty expect(merge_diff.errors).to be_empty
@ -32,6 +36,7 @@ describe ShaValidator do
let(:value) { 'foo' } let(:value) { 'foo' }
it 'adds error to the record' do it 'adds error to the record' do
expect(Commit).to receive(:valid_hash?).and_call_original
expect(merge_diff.errors).to be_empty expect(merge_diff.errors).to be_empty
subject subject