Commit cd546a78 authored by James Lopez's avatar James Lopez

Merge branch 'fix/import-export-encoding' into 'master'

Fix MR diff encoding issues exporting projects

Fixes https://gitlab.com/gitlab-org/gitlab-ce/issues/19338

See merge request !5039
parents 78cd5b8d bf1ea8c6
...@@ -125,6 +125,7 @@ v 8.10.0 (unreleased) ...@@ -125,6 +125,7 @@ v 8.10.0 (unreleased)
- Allow empty repositories on project import/export - Allow empty repositories on project import/export
- Render only commit message title in builds (Katarzyna Kobierska Ula Budziszewska) - Render only commit message title in builds (Katarzyna Kobierska Ula Budziszewska)
- Allow bulk (un)subscription from issues in issue index - Allow bulk (un)subscription from issues in issue index
- Fix MR diff encoding issues exporting GitLab projects
v 8.9.6 v 8.9.6
- Fix importing of events under notes for GitLab projects. !5154 - Fix importing of events under notes for GitLab projects. !5154
......
class MergeRequestDiff < ActiveRecord::Base class MergeRequestDiff < ActiveRecord::Base
include Sortable include Sortable
include Importable include Importable
include EncodingHelper
# Prevent store of diff if commits amount more then 500 # Prevent store of diff if commits amount more then 500
COMMITS_SAFE_SIZE = 100 COMMITS_SAFE_SIZE = 100
...@@ -211,6 +212,14 @@ class MergeRequestDiff < ActiveRecord::Base ...@@ -211,6 +212,14 @@ class MergeRequestDiff < ActiveRecord::Base
branch_base_commit.try(:sha) branch_base_commit.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.
......
...@@ -2,7 +2,7 @@ module Gitlab ...@@ -2,7 +2,7 @@ module Gitlab
module ImportExport module ImportExport
extend self extend self
VERSION = '0.1.1' VERSION = '0.1.2'
FILENAME_LIMIT = 50 FILENAME_LIMIT = 50
def export_path(relative_path:) def export_path(relative_path:)
......
...@@ -53,7 +53,11 @@ included_attributes: ...@@ -53,7 +53,11 @@ included_attributes:
excluded_attributes: excluded_attributes:
snippets: snippets:
- :expired_at - :expired_at
merge_request_diff:
- :st_diffs
methods: methods:
statuses: statuses:
- :type - :type
\ No newline at end of file merge_request_diff:
- :utf8_st_diffs
\ No newline at end of file
...@@ -33,6 +33,7 @@ module Gitlab ...@@ -33,6 +33,7 @@ module Gitlab
update_project_references update_project_references
reset_ci_tokens if @relation_name == 'Ci::Trigger' reset_ci_tokens if @relation_name == 'Ci::Trigger'
@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
generate_imported_object generate_imported_object
end end
...@@ -129,6 +130,10 @@ module Gitlab ...@@ -129,6 +130,10 @@ module Gitlab
def parsed_relation_hash def parsed_relation_hash
@relation_hash.reject { |k, _v| !relation_class.attribute_method?(k) } @relation_hash.reject { |k, _v| !relation_class.attribute_method?(k) }
end end
def set_st_diffs
@relation_hash['st_diffs'] = @relation_hash.delete('utf8_st_diffs')
end
end end
end end
end end
...@@ -2765,7 +2765,7 @@ ...@@ -2765,7 +2765,7 @@
"committer_email": "dmitriy.zaporozhets@gmail.com" "committer_email": "dmitriy.zaporozhets@gmail.com"
} }
], ],
"st_diffs": [ "utf8_st_diffs": [
{ {
"diff": "Binary files a/.DS_Store and /dev/null differ\n", "diff": "Binary files a/.DS_Store and /dev/null differ\n",
"new_path": ".DS_Store", "new_path": ".DS_Store",
...@@ -3138,7 +3138,7 @@ ...@@ -3138,7 +3138,7 @@
"committer_email": "dmitriy.zaporozhets@gmail.com" "committer_email": "dmitriy.zaporozhets@gmail.com"
} }
], ],
"st_diffs": [ "utf8_st_diffs": [
{ {
"diff": "--- /dev/null\n+++ b/files/ruby/feature.rb\n@@ -0,0 +1,5 @@\n+class Feature\n+ def foo\n+ puts 'bar'\n+ end\n+end\n", "diff": "--- /dev/null\n+++ b/files/ruby/feature.rb\n@@ -0,0 +1,5 @@\n+class Feature\n+ def foo\n+ puts 'bar'\n+ end\n+end\n",
"new_path": "files/ruby/feature.rb", "new_path": "files/ruby/feature.rb",
...@@ -3423,7 +3423,7 @@ ...@@ -3423,7 +3423,7 @@
"committer_email": "james@jameslopez.es" "committer_email": "james@jameslopez.es"
} }
], ],
"st_diffs": [ "utf8_st_diffs": [
{ {
"diff": "--- /dev/null\n+++ b/test\n", "diff": "--- /dev/null\n+++ b/test\n",
"new_path": "test", "new_path": "test",
...@@ -3960,7 +3960,7 @@ ...@@ -3960,7 +3960,7 @@
"committer_email": "dmitriy.zaporozhets@gmail.com" "committer_email": "dmitriy.zaporozhets@gmail.com"
} }
], ],
"st_diffs": [ "utf8_st_diffs": [
{ {
"diff": "Binary files a/.DS_Store and /dev/null differ\n", "diff": "Binary files a/.DS_Store and /dev/null differ\n",
"new_path": ".DS_Store", "new_path": ".DS_Store",
...@@ -4597,7 +4597,7 @@ ...@@ -4597,7 +4597,7 @@
"committer_email": "marmis85@gmail.com" "committer_email": "marmis85@gmail.com"
} }
], ],
"st_diffs": [ "utf8_st_diffs": [
{ {
"diff": "--- a/CHANGELOG\n+++ b/CHANGELOG\n@@ -1,4 +1,6 @@\n-v 6.7.0\n+v6.8.0\n+\n+v6.7.0\n - Add support for Gemnasium as a Project Service (Olivier Gonzalez)\n - Add edit file button to MergeRequest diff\n - Public groups (Jason Hollingsworth)\n", "diff": "--- a/CHANGELOG\n+++ b/CHANGELOG\n@@ -1,4 +1,6 @@\n-v 6.7.0\n+v6.8.0\n+\n+v6.7.0\n - Add support for Gemnasium as a Project Service (Olivier Gonzalez)\n - Add edit file button to MergeRequest diff\n - Public groups (Jason Hollingsworth)\n",
"new_path": "CHANGELOG", "new_path": "CHANGELOG",
...@@ -5108,7 +5108,7 @@ ...@@ -5108,7 +5108,7 @@
"committer_email": "stanhu@packetzoom.com" "committer_email": "stanhu@packetzoom.com"
} }
], ],
"st_diffs": [ "utf8_st_diffs": [
{ {
"diff": "--- a/CHANGELOG\n+++ b/CHANGELOG\n@@ -1,4 +1,6 @@\n-v 6.7.0\n+v6.8.0\n+\n+v6.7.0\n - Add support for Gemnasium as a Project Service (Olivier Gonzalez)\n - Add edit file button to MergeRequest diff\n - Public groups (Jason Hollingsworth)\n", "diff": "--- a/CHANGELOG\n+++ b/CHANGELOG\n@@ -1,4 +1,6 @@\n-v 6.7.0\n+v6.8.0\n+\n+v6.7.0\n - Add support for Gemnasium as a Project Service (Olivier Gonzalez)\n - Add edit file button to MergeRequest diff\n - Public groups (Jason Hollingsworth)\n",
"new_path": "CHANGELOG", "new_path": "CHANGELOG",
...@@ -5434,7 +5434,7 @@ ...@@ -5434,7 +5434,7 @@
"id": 11, "id": 11,
"state": "empty", "state": "empty",
"st_commits": null, "st_commits": null,
"st_diffs": [ "utf8_st_diffs": [
], ],
"merge_request_id": 11, "merge_request_id": 11,
...@@ -5961,7 +5961,7 @@ ...@@ -5961,7 +5961,7 @@
"committer_email": "dmitriy.zaporozhets@gmail.com" "committer_email": "dmitriy.zaporozhets@gmail.com"
} }
], ],
"st_diffs": [ "utf8_st_diffs": [
{ {
"diff": "Binary files a/.DS_Store and /dev/null differ\n", "diff": "Binary files a/.DS_Store and /dev/null differ\n",
"new_path": ".DS_Store", "new_path": ".DS_Store",
...@@ -6400,7 +6400,7 @@ ...@@ -6400,7 +6400,7 @@
"committer_email": "james@jameslopez.es" "committer_email": "james@jameslopez.es"
} }
], ],
"st_diffs": [ "utf8_st_diffs": [
{ {
"diff": "--- /dev/null\n+++ b/test\n", "diff": "--- /dev/null\n+++ b/test\n",
"new_path": "test", "new_path": "test",
......
...@@ -2,6 +2,7 @@ require 'spec_helper' ...@@ -2,6 +2,7 @@ require 'spec_helper'
describe Gitlab::ImportExport::ProjectTreeRestorer, services: true do describe Gitlab::ImportExport::ProjectTreeRestorer, services: true do
describe 'restore project tree' do describe 'restore project tree' do
let(:user) { create(:user) } let(:user) { create(:user) }
let(:namespace) { create(:namespace, owner: user) } let(:namespace) { create(:namespace, owner: user) }
let(:shared) { Gitlab::ImportExport::Shared.new(relative_path: "", project_path: 'path') } let(:shared) { Gitlab::ImportExport::Shared.new(relative_path: "", project_path: 'path') }
...@@ -53,6 +54,12 @@ describe Gitlab::ImportExport::ProjectTreeRestorer, services: true do ...@@ -53,6 +54,12 @@ describe Gitlab::ImportExport::ProjectTreeRestorer, services: true do
expect(event.note.noteable.project).not_to be_nil expect(event.note.noteable.project).not_to be_nil
end end
end end
it 'has the correct data for merge request st_diffs' do
# makes sure we are renaming the custom method +utf8_st_diffs+ into +st_diffs+
expect { restored_project_json }.to change(MergeRequestDiff.where.not(st_diffs: nil), :count).by(9)
end
end end
end end
end end
...@@ -102,12 +102,17 @@ describe Gitlab::ImportExport::ProjectTreeSaver, services: true do ...@@ -102,12 +102,17 @@ describe Gitlab::ImportExport::ProjectTreeSaver, services: true do
it 'has ci pipeline notes' do it 'has ci pipeline notes' do
expect(saved_project_json['pipelines'].first['notes']).not_to be_empty expect(saved_project_json['pipelines'].first['notes']).not_to be_empty
end end
it 'does not complain about non UTF-8 characters in MR diffs' do
ActiveRecord::Base.connection.execute("UPDATE merge_request_diffs SET st_diffs = '---\n- :diff: !binary |-\n LS0tIC9kZXYvbnVsbAorKysgYi9pbWFnZXMvbnVjb3IucGRmCkBAIC0wLDAg\n KzEsMTY3OSBAQAorJVBERi0xLjUNJeLjz9MNCisxIDAgb2JqDTw8L01ldGFk\n YXR'")
expect(project_tree_saver.save).to be true
end
end end
end end
def setup_project def setup_project
issue = create(:issue, assignee: user) issue = create(:issue, assignee: user)
merge_request = create(:merge_request)
label = create(:label) label = create(:label)
snippet = create(:project_snippet) snippet = create(:project_snippet)
release = create(:release) release = create(:release)
...@@ -115,12 +120,12 @@ describe Gitlab::ImportExport::ProjectTreeSaver, services: true do ...@@ -115,12 +120,12 @@ describe Gitlab::ImportExport::ProjectTreeSaver, services: true do
project = create(:project, project = create(:project,
:public, :public,
issues: [issue], issues: [issue],
merge_requests: [merge_request],
labels: [label], labels: [label],
snippets: [snippet], snippets: [snippet],
releases: [release] releases: [release]
) )
merge_request = create(:merge_request, source_project: project)
commit_status = create(:commit_status, project: project) commit_status = create(:commit_status, project: project)
ci_pipeline = create(:ci_pipeline, ci_pipeline = create(:ci_pipeline,
......
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