Commit 70af1e2e authored by Sean McGivern's avatar Sean McGivern

Fix 500 error when trying to resolve non-ASCII conflicts in editor

When we added caching, this meant that calling `can_be_resolved_in_ui?` didn't
always call `lines`, which meant that we didn't get the benefit of the
side-effect from that, where it forced the conflict data itself to UTF-8.

To fix that, make this explicit by separating the `raw_content` (any encoding)
from the `content` (which is either UTF-8, or an exception is raised).
parent b06a44c4
---
title: Fix 500 error when trying to resolve non-ASCII conflicts in the editor
merge_request: 17962
author:
type: fixed
...@@ -40,7 +40,10 @@ module Gitlab ...@@ -40,7 +40,10 @@ module Gitlab
# when there are no conflict files. # when there are no conflict files.
files.each(&:lines) files.each(&:lines)
files.any? files.any?
rescue Gitlab::Git::CommandError, Gitlab::Git::Conflict::Parser::UnresolvableError, Gitlab::Git::Conflict::Resolver::ConflictSideMissing rescue Gitlab::Git::CommandError,
Gitlab::Git::Conflict::Parser::UnresolvableError,
Gitlab::Git::Conflict::Resolver::ConflictSideMissing,
Gitlab::Git::Conflict::File::UnsupportedEncoding
false false
end end
cache_method :can_be_resolved_in_ui? cache_method :can_be_resolved_in_ui?
......
...@@ -2,17 +2,19 @@ module Gitlab ...@@ -2,17 +2,19 @@ module Gitlab
module Git module Git
module Conflict module Conflict
class File class File
UnsupportedEncoding = Class.new(StandardError)
attr_reader :their_path, :our_path, :our_mode, :repository, :commit_oid attr_reader :their_path, :our_path, :our_mode, :repository, :commit_oid
attr_accessor :content attr_accessor :raw_content
def initialize(repository, commit_oid, conflict, content) def initialize(repository, commit_oid, conflict, raw_content)
@repository = repository @repository = repository
@commit_oid = commit_oid @commit_oid = commit_oid
@their_path = conflict[:theirs][:path] @their_path = conflict[:theirs][:path]
@our_path = conflict[:ours][:path] @our_path = conflict[:ours][:path]
@our_mode = conflict[:ours][:mode] @our_mode = conflict[:ours][:mode]
@content = content @raw_content = raw_content
end end
def lines def lines
...@@ -29,6 +31,14 @@ module Gitlab ...@@ -29,6 +31,14 @@ module Gitlab
end end
end end
def content
@content ||= @raw_content.dup.force_encoding('UTF-8')
raise UnsupportedEncoding unless @content.valid_encoding?
@content
end
def type def type
lines unless @type lines unless @type
......
...@@ -4,7 +4,6 @@ module Gitlab ...@@ -4,7 +4,6 @@ module Gitlab
class Parser class Parser
UnresolvableError = Class.new(StandardError) UnresolvableError = Class.new(StandardError)
UnmergeableFile = Class.new(UnresolvableError) UnmergeableFile = Class.new(UnresolvableError)
UnsupportedEncoding = Class.new(UnresolvableError)
# Recoverable errors - the conflict can be resolved in an editor, but not with # Recoverable errors - the conflict can be resolved in an editor, but not with
# sections. # sections.
...@@ -75,10 +74,6 @@ module Gitlab ...@@ -75,10 +74,6 @@ module Gitlab
def validate_text!(text) def validate_text!(text)
raise UnmergeableFile if text.blank? # Typically a binary file raise UnmergeableFile if text.blank? # Typically a binary file
raise UnmergeableFile if text.length > 200.kilobytes raise UnmergeableFile if text.length > 200.kilobytes
text.force_encoding('UTF-8')
raise UnsupportedEncoding unless text.valid_encoding?
end end
def validate_delimiter!(condition) def validate_delimiter!(condition)
......
...@@ -17,7 +17,7 @@ module Gitlab ...@@ -17,7 +17,7 @@ module Gitlab
current_file = file_from_gitaly_header(gitaly_file.header) current_file = file_from_gitaly_header(gitaly_file.header)
else else
current_file.content << gitaly_file.content current_file.raw_content << gitaly_file.content
end end
end end
end end
......
# coding: utf-8
require 'spec_helper'
describe Gitlab::Git::Conflict::File do
let(:conflict) { { theirs: { path: 'foo', mode: 33188 }, ours: { path: 'foo', mode: 33188 } } }
let(:invalid_content) { described_class.new(nil, nil, conflict, "a\xC4\xFC".force_encoding(Encoding::ASCII_8BIT)) }
let(:valid_content) { described_class.new(nil, nil, conflict, "Espa\xC3\xB1a".force_encoding(Encoding::ASCII_8BIT)) }
describe '#lines' do
context 'when the content contains non-UTF-8 characters' do
it 'raises UnsupportedEncoding' do
expect { invalid_content.lines }
.to raise_error(described_class::UnsupportedEncoding)
end
end
context 'when the content can be converted to UTF-8' do
it 'sets lines to the lines' do
expect(valid_content.lines).to eq([{
full_line: 'España',
type: nil,
line_obj_index: 0,
line_old: 1,
line_new: 1
}])
end
it 'sets the type to text' do
expect(valid_content.type).to eq('text')
end
end
end
describe '#content' do
context 'when the content contains non-UTF-8 characters' do
it 'raises UnsupportedEncoding' do
expect { invalid_content.content }
.to raise_error(described_class::UnsupportedEncoding)
end
end
context 'when the content can be converted to UTF-8' do
it 'returns a valid UTF-8 string' do
expect(valid_content.content).to eq('España')
expect(valid_content.content).to be_valid_encoding
expect(valid_content.content.encoding).to eq(Encoding::UTF_8)
end
end
end
end
...@@ -212,13 +212,6 @@ CONFLICT ...@@ -212,13 +212,6 @@ CONFLICT
.not_to raise_error .not_to raise_error
end end
end end
context 'when the file contains non-UTF-8 characters' do
it 'raises UnsupportedEncoding' do
expect { parse_text("a\xC4\xFC".force_encoding(Encoding::ASCII_8BIT)) }
.to raise_error(Gitlab::Git::Conflict::Parser::UnsupportedEncoding)
end
end
end end
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