Commit 93e36d56 authored by Rémy Coutable's avatar Rémy Coutable

Merge branch 'fix/import-fork' into 'master'

Fix issues importing forked projects

Closes #26184 and #29380

See merge request !9102
parents 82836af4 22d7ae80
...@@ -177,6 +177,16 @@ class MergeRequestDiff < ActiveRecord::Base ...@@ -177,6 +177,16 @@ class MergeRequestDiff < ActiveRecord::Base
st_commits.count st_commits.count
end end
def utf8_st_diffs
return [] if st_diffs.blank?
st_diffs.map do |diff|
diff.each do |k, v|
diff[k] = encode_utf8(v) if v.respond_to?(:encoding)
end
end
end
private private
# Old GitLab implementations may have generated diffs as ["--broken-diff"]. # Old GitLab implementations may have generated diffs as ["--broken-diff"].
...@@ -270,14 +280,6 @@ class MergeRequestDiff < ActiveRecord::Base ...@@ -270,14 +280,6 @@ class MergeRequestDiff < ActiveRecord::Base
project.merge_base_commit(head_commit_sha, start_commit_sha).try(:sha) project.merge_base_commit(head_commit_sha, start_commit_sha).try(:sha)
end end
def utf8_st_diffs
st_diffs.map do |diff|
diff.each do |k, v|
diff[k] = encode_utf8(v) if v.respond_to?(:encoding)
end
end
end
# #
# #save or #update_attributes providing changes on serialized attributes do a lot of # #save or #update_attributes providing changes on serialized attributes do a lot of
# serialization and deserialization calls resulting in bad performance. # serialization and deserialization calls resulting in bad performance.
......
---
title: Fix Import/Export MR diffs not showing and missing forked MRs
merge_request:
author:
module Gitlab
module ImportExport
class HashUtil
def self.deep_symbolize_array!(array)
return if array.blank?
array.map! do |hash|
hash.deep_symbolize_keys!
yield(hash) if block_given?
hash
end
end
def self.deep_symbolize_array_with_date!(array)
self.deep_symbolize_array!(array) do |hash|
hash.select { |k, _v| k.to_s.end_with?('_date') }.each do |key, value|
hash[key] = Time.zone.parse(value)
end
end
end
end
end
end
...@@ -89,3 +89,5 @@ methods: ...@@ -89,3 +89,5 @@ methods:
- :type - :type
merge_request_diff: merge_request_diff:
- :utf8_st_diffs - :utf8_st_diffs
merge_requests:
- :diff_head_sha
...@@ -9,7 +9,7 @@ module Gitlab ...@@ -9,7 +9,7 @@ module Gitlab
end end
def execute def execute
if import_file && check_version! && [project_tree, avatar_restorer, repo_restorer, wiki_restorer, uploads_restorer].all?(&:restore) if import_file && check_version! && [repo_restorer, wiki_restorer, project_tree, avatar_restorer, uploads_restorer].all?(&:restore)
project_tree.restored_project project_tree.restored_project
else else
raise Projects::ImportService::Error.new(@shared.errors.join(', ')) raise Projects::ImportService::Error.new(@shared.errors.join(', '))
......
module Gitlab
module ImportExport
class MergeRequestParser
FORKED_PROJECT_ID = -1
def initialize(project, diff_head_sha, merge_request, relation_hash)
@project = project
@diff_head_sha = diff_head_sha
@merge_request = merge_request
@relation_hash = relation_hash
end
def parse!
if fork_merge_request? && @diff_head_sha
@merge_request.source_project_id = @relation_hash['project_id']
fetch_ref unless branch_exists?(@merge_request.source_branch)
create_target_branch unless branch_exists?(@merge_request.target_branch)
end
@merge_request
end
def create_target_branch
@project.repository.create_branch(@merge_request.target_branch, @merge_request.target_branch_sha)
end
def fetch_ref
@project.repository.fetch_ref(@project.repository.path, @diff_head_sha, @merge_request.source_branch)
end
def branch_exists?(branch_name)
@project.repository.branch_exists?(branch_name)
end
def fork_merge_request?
@relation_hash['source_project_id'] == FORKED_PROJECT_ID
end
end
end
end
...@@ -119,7 +119,7 @@ module Gitlab ...@@ -119,7 +119,7 @@ module Gitlab
relation_hash: parsed_relation_hash(relation_hash), relation_hash: parsed_relation_hash(relation_hash),
members_mapper: members_mapper, members_mapper: members_mapper,
user: @user, user: @user,
project_id: restored_project.id) project: restored_project)
end.compact end.compact
relation_hash_list.is_a?(Array) ? relation_array : relation_array.first relation_hash_list.is_a?(Array) ? relation_array : relation_array.first
......
...@@ -29,11 +29,12 @@ module Gitlab ...@@ -29,11 +29,12 @@ module Gitlab
new(*args).create new(*args).create
end end
def initialize(relation_sym:, relation_hash:, members_mapper:, user:, project_id:) def initialize(relation_sym:, relation_hash:, members_mapper:, user:, project:)
@relation_name = OVERRIDES[relation_sym] || relation_sym @relation_name = OVERRIDES[relation_sym] || relation_sym
@relation_hash = relation_hash.except('noteable_id').merge('project_id' => project_id) @relation_hash = relation_hash.except('noteable_id').merge('project_id' => project.id)
@members_mapper = members_mapper @members_mapper = members_mapper
@user = user @user = user
@project = project
@imported_object_retries = 0 @imported_object_retries = 0
end end
...@@ -66,7 +67,7 @@ module Gitlab ...@@ -66,7 +67,7 @@ module Gitlab
remove_encrypted_attributes! remove_encrypted_attributes!
@relation_hash['data'].deep_symbolize_keys! if @relation_name == :events && @relation_hash['data'] @relation_hash['data'].deep_symbolize_keys! if @relation_name == :events && @relation_hash['data']
set_st_diffs if @relation_name == :merge_request_diff set_st_diff_commits if @relation_name == :merge_request_diff
end end
def update_user_references def update_user_references
...@@ -105,6 +106,8 @@ module Gitlab ...@@ -105,6 +106,8 @@ module Gitlab
imported_object do |object| imported_object do |object|
object.commit_id = nil object.commit_id = nil
end end
elsif @relation_name == :merge_requests
MergeRequestParser.new(@project, @relation_hash.delete('diff_head_sha'), imported_object, @relation_hash).parse!
else else
imported_object imported_object
end end
...@@ -115,7 +118,7 @@ module Gitlab ...@@ -115,7 +118,7 @@ module Gitlab
# If source and target are the same, populate them with the new project ID. # If source and target are the same, populate them with the new project ID.
if @relation_hash['source_project_id'] if @relation_hash['source_project_id']
@relation_hash['source_project_id'] = same_source_and_target? ? project_id : -1 @relation_hash['source_project_id'] = same_source_and_target? ? project_id : MergeRequestParser::FORKED_PROJECT_ID
end end
# project_id may not be part of the export, but we always need to populate it if required. # project_id may not be part of the export, but we always need to populate it if required.
...@@ -166,6 +169,7 @@ module Gitlab ...@@ -166,6 +169,7 @@ module Gitlab
def imported_object def imported_object
yield(existing_or_new_object) if block_given? yield(existing_or_new_object) if block_given?
existing_or_new_object.importing = true if existing_or_new_object.respond_to?(:importing) existing_or_new_object.importing = true if existing_or_new_object.respond_to?(:importing)
existing_or_new_object existing_or_new_object
rescue ActiveRecord::RecordNotUnique rescue ActiveRecord::RecordNotUnique
# as the operation is not atomic, retry in the unlikely scenario an INSERT is # as the operation is not atomic, retry in the unlikely scenario an INSERT is
...@@ -188,8 +192,11 @@ module Gitlab ...@@ -188,8 +192,11 @@ module Gitlab
relation_class: relation_class) relation_class: relation_class)
end end
def set_st_diffs def set_st_diff_commits
@relation_hash['st_diffs'] = @relation_hash.delete('utf8_st_diffs') @relation_hash['st_diffs'] = @relation_hash.delete('utf8_st_diffs')
HashUtil.deep_symbolize_array!(@relation_hash['st_diffs'])
HashUtil.deep_symbolize_array_with_date!(@relation_hash['st_commits'])
end end
def existing_or_new_object def existing_or_new_object
......
require 'spec_helper'
describe 'forked project import', services: true do
let(:user) { create(:user) }
let!(:project_with_repo) { create(:project, :test_repo, name: 'test-repo-restorer', path: 'test-repo-restorer') }
let!(:project) { create(:empty_project) }
let(:export_path) { "#{Dir.tmpdir}/project_tree_saver_spec" }
let(:shared) { Gitlab::ImportExport::Shared.new(relative_path: project.path_with_namespace) }
let(:forked_from_project) { create(:project) }
let(:fork_link) { create(:forked_project_link, forked_from_project: project_with_repo) }
let(:repo_saver) { Gitlab::ImportExport::RepoSaver.new(project: project_with_repo, shared: shared) }
let(:bundle_path) { File.join(shared.export_path, Gitlab::ImportExport.project_bundle_filename) }
let(:repo_restorer) do
Gitlab::ImportExport::RepoRestorer.new(path_to_bundle: bundle_path, shared: shared, project: project)
end
let!(:merge_request) do
create(:merge_request, source_project: fork_link.forked_to_project, target_project: project_with_repo)
end
let(:saver) do
Gitlab::ImportExport::ProjectTreeSaver.new(project: project_with_repo, current_user: user, shared: shared)
end
let(:restorer) do
Gitlab::ImportExport::ProjectTreeRestorer.new(user: user, shared: shared, project: project)
end
before do
allow_any_instance_of(Gitlab::ImportExport).to receive(:storage_path).and_return(export_path)
saver.save
repo_saver.save
repo_restorer.restore
restorer.restore
end
after do
FileUtils.rm_rf(export_path)
FileUtils.rm_rf(project_with_repo.repository.path_to_repo)
FileUtils.rm_rf(project.repository.path_to_repo)
end
it 'can access the MR' do
expect(project.merge_requests.first.ensure_ref_fetched.first).to include('refs/merge-requests/1/head')
end
end
require 'spec_helper'
describe Gitlab::ImportExport::HashUtil, lib: true do
let(:stringified_array) { [{ 'test' => 1 }] }
let(:stringified_array_with_date) { [{ 'test_date' => '2016-04-06 06:17:44 +0200' }] }
describe '.deep_symbolize_array!' do
it 'symbolizes keys' do
expect { described_class.deep_symbolize_array!(stringified_array) }.to change {
stringified_array.first.keys.first
}.from('test').to(:test)
end
end
describe '.deep_symbolize_array_with_date!' do
it 'symbolizes keys' do
expect { described_class.deep_symbolize_array_with_date!(stringified_array_with_date) }.to change {
stringified_array_with_date.first.keys.first
}.from('test_date').to(:test_date)
end
it 'transforms date strings into Time objects' do
expect { described_class.deep_symbolize_array_with_date!(stringified_array_with_date) }.to change {
stringified_array_with_date.first.values.first.class
}.from(String).to(ActiveSupport::TimeWithZone)
end
end
end
require 'spec_helper'
describe Gitlab::ImportExport::MergeRequestParser do
let(:user) { create(:user) }
let!(:project) { create(:project, :test_repo, name: 'test-repo-restorer', path: 'test-repo-restorer') }
let(:forked_from_project) { create(:project) }
let(:fork_link) { create(:forked_project_link, forked_from_project: project) }
let!(:merge_request) do
create(:merge_request, source_project: fork_link.forked_to_project, target_project: project)
end
let(:parsed_merge_request) do
described_class.new(project,
merge_request.diff_head_sha,
merge_request,
merge_request.as_json).parse!
end
after do
FileUtils.rm_rf(project.repository.path_to_repo)
end
it 'has a source branch' do
expect(project.repository.branch_exists?(parsed_merge_request.source_branch)).to be true
end
it 'has a target branch' do
expect(project.repository.branch_exists?(parsed_merge_request.target_branch)).to be true
end
end
...@@ -82,6 +82,12 @@ describe Gitlab::ImportExport::ProjectTreeRestorer, services: true do ...@@ -82,6 +82,12 @@ describe Gitlab::ImportExport::ProjectTreeRestorer, services: true do
expect(MergeRequestDiff.where.not(st_diffs: nil).count).to eq(9) expect(MergeRequestDiff.where.not(st_diffs: nil).count).to eq(9)
end end
it 'has the correct time for merge request st_commits' do
st_commits = MergeRequestDiff.where.not(st_commits: nil).first.st_commits
expect(st_commits.first[:committed_date]).to be_kind_of(Time)
end
it 'has labels associated to label links, associated to issues' do it 'has labels associated to label links, associated to issues' do
expect(Label.first.label_links.first.target).not_to be_nil expect(Label.first.label_links.first.target).not_to be_nil
end end
......
...@@ -79,6 +79,10 @@ describe Gitlab::ImportExport::ProjectTreeSaver, services: true do ...@@ -79,6 +79,10 @@ describe Gitlab::ImportExport::ProjectTreeSaver, services: true do
expect(saved_project_json['merge_requests'].first['merge_request_diff']).not_to be_empty expect(saved_project_json['merge_requests'].first['merge_request_diff']).not_to be_empty
end end
it 'has merge requests diff st_diffs' do
expect(saved_project_json['merge_requests'].first['merge_request_diff']['utf8_st_diffs']).not_to be_nil
end
it 'has merge requests comments' do it 'has merge requests comments' do
expect(saved_project_json['merge_requests'].first['notes']).not_to be_empty expect(saved_project_json['merge_requests'].first['notes']).not_to be_empty
end end
......
...@@ -9,7 +9,7 @@ describe Gitlab::ImportExport::RelationFactory, lib: true do ...@@ -9,7 +9,7 @@ describe Gitlab::ImportExport::RelationFactory, lib: true do
relation_hash: relation_hash, relation_hash: relation_hash,
members_mapper: members_mapper, members_mapper: members_mapper,
user: user, user: user,
project_id: project.id) project: project)
end end
context 'hook object' do context 'hook object' do
......
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