From 65d40ace236c71447712cd7baf876ea4e57e32f4 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Dec 14 2016 09:48:35 +0000 Subject: [PATCH 1/10] Let guess_encoding try all the possible encodings before making its choice This way, if by any chance the first one isn't the right one, we can still try the others. If all luck ran out, we can then properly bail and leave it up to the caller to handle the situation. --- diff --git a/pagure/lib/encoding_utils.py b/pagure/lib/encoding_utils.py index 4a9136a..267ef35 100644 --- a/pagure/lib/encoding_utils.py +++ b/pagure/lib/encoding_utils.py @@ -17,6 +17,8 @@ import logging from chardet import universaldetector +from pagure.exceptions import PagureException + _log = logging.getLogger(__name__) @@ -55,7 +57,14 @@ def guess_encoding(data): encodings, key=lambda guess: guess.confidence, reverse=True) _log.debug('Possible encodings: ' + str(sorted_encodings)) - return sorted_encodings[0].encoding + for encoding in sorted_encodings: + _log.debug('Trying encoding: %s', str(encoding)) + try: + data.decode(encoding.encoding) + return encoding.encoding + except UnicodeDecodeError: + pass + raise PagureException('No encoding could be guessed for this file') def detect_encodings(data): diff --git a/pagure/ui/issues.py b/pagure/ui/issues.py index b4a3c7f..43b5014 100644 --- a/pagure/ui/issues.py +++ b/pagure/ui/issues.py @@ -1175,8 +1175,12 @@ def view_issue_raw_file( headers['Content-Disposition'] = 'attachment' if mimetype.startswith('text/') and not encoding: - encoding = pagure.lib.encoding_utils.guess_encoding( - ktc.to_bytes(data)) + try: + encoding = pagure.lib.encoding_utils.guess_encoding( + ktc.to_bytes(data)) + except pagure.exceptions.PagureException: + # We cannot decode the file, so bail but warn the admins + LOG.exception('File could not be decoded') if encoding: mimetype += '; charset={encoding}'.format(encoding=encoding) diff --git a/pagure/ui/repo.py b/pagure/ui/repo.py index 096aa97..e36f5a6 100644 --- a/pagure/ui/repo.py +++ b/pagure/ui/repo.py @@ -533,7 +533,13 @@ def view_file(repo, identifier, filename, username=None, namespace=None): content, safe = pagure.doc_utils.convert_readme(content.data, ext) output_type = 'markup' elif not is_binary_string(content.data): - file_content = encoding_utils.decode(ktc.to_bytes(content.data)) + try: + file_content = encoding_utils.decode(ktc.to_bytes(content.data)) + except pagure.exceptions.PagureException: + # We cannot decode the file, so let's pretend it's a binary + # file and let the user download it instead of displaying + # it. + output_type = 'binary' try: lexer = guess_lexer_for_filename( filename, @@ -670,7 +676,11 @@ def view_raw_file( headers['Content-Disposition'] = 'attachment' if mimetype.startswith('text/') and not encoding: - encoding = encoding_utils.guess_encoding(ktc.to_bytes(data)) + try: + encoding = encoding_utils.guess_encoding(ktc.to_bytes(data)) + except pagure.exceptions.PagureException: + # We cannot decode the file, so bail but warn the admins + LOG.exception('File could not be decoded') if encoding: mimetype += '; charset={encoding}'.format(encoding=encoding) @@ -708,7 +718,13 @@ def view_blame_file(repo, filename, username=None, namespace=None): if is_binary_string(content.data): flask.abort(400, 'Binary files cannot be blamed') - content = encoding_utils.decode(content.data) + try: + content = encoding_utils.decode(content.data) + except pagure.exceptions.PagureException: + # We cannot decode the file, so bail but warn the admins + LOG.exception('File could not be decoded') + flask.abort(400, 'File could not be decoded') + blame = repo_obj.blame(filename) return flask.render_template( From 52bc4579b30152b7baff1eb3238ee13163639951 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Dec 14 2016 09:48:35 +0000 Subject: [PATCH 2/10] Make it simpler to run the tests for pagure.lib.encoding_utils --- diff --git a/tests/test_pagure_lib_encoding_utils.py b/tests/test_pagure_lib_encoding_utils.py index 8c63d4a..b99e0b1 100644 --- a/tests/test_pagure_lib_encoding_utils.py +++ b/tests/test_pagure_lib_encoding_utils.py @@ -4,7 +4,12 @@ Tests for :module:`pagure.lib.encoding_utils`. """ import chardet +import os import unittest +import sys + +sys.path.insert(0, os.path.join(os.path.dirname( + os.path.abspath(__file__)), '..')) from pagure.lib import encoding_utils @@ -30,6 +35,7 @@ class TestGuessEncoding(unittest.TestCase): self.assertEqual(chardet_result['encoding'], 'ISO-8859-2') def test_guess_encoding_no_data(self): + """ Test encoding_utils.guess_encoding() with an emtpy string """ result = encoding_utils.guess_encoding(u''.encode('utf-8')) self.assertEqual(result, 'ascii') @@ -37,9 +43,10 @@ class TestGuessEncoding(unittest.TestCase): class TestDecode(unittest.TestCase): def test_decode(self): + """ Test encoding_utils.decode() """ data = u'Šabata' self.assertEqual(data, encoding_utils.decode(data.encode('utf-8'))) if __name__ == '__main__': - unittest.main() + unittest.main(verbosity=2) From 2eba2775be448c083025f3e27d31e7cecfc73f28 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Dec 14 2016 09:48:35 +0000 Subject: [PATCH 3/10] Create a guess_encodings() method returning all the possible encodings And let guess_encoding() rely on this method. --- diff --git a/pagure/lib/encoding_utils.py b/pagure/lib/encoding_utils.py index 267ef35..f2a5666 100644 --- a/pagure/lib/encoding_utils.py +++ b/pagure/lib/encoding_utils.py @@ -25,9 +25,10 @@ _log = logging.getLogger(__name__) Guess = namedtuple('Guess', ['encoding', 'confidence']) -def guess_encoding(data): + +def guess_encodings(data): """ - Attempt to guess the text encoding used for the given data. + List all the possible encoding found for the given data. This uses chardet to guess the encoding, but biases the results towards UTF-8. There are cases where chardet cannot know the encoding and @@ -57,7 +58,31 @@ def guess_encoding(data): encodings, key=lambda guess: guess.confidence, reverse=True) _log.debug('Possible encodings: ' + str(sorted_encodings)) - for encoding in sorted_encodings: + return sorted_encodings + + +def guess_encoding(data): + """ + Attempt to guess the text encoding used for the given data. + + This uses chardet to guess the encoding, but biases the results towards + UTF-8. There are cases where chardet cannot know the encoding and + therefore is occasionally wrong. In those cases it was decided that it + would be better to err on the side of UTF-8 rather than ISO-8859-*. + However, it is important to be aware that this also guesses and _will_ + misclassify ISO-8859-* encoded text as UTF-8 in some cases. + + The discussion that lead to this decision can be found at + https://pagure.io/pagure/issue/891. + + :param data: An array of bytes to treat as text data + :type data: bytes + :raises PagureException: if no encoding was found that the data could + be decoded into + """ + encodings = guess_encodings(data) + + for encoding in encodings: _log.debug('Trying encoding: %s', str(encoding)) try: data.decode(encoding.encoding) From 29a8fc4653372f3166afb4f64a7f4f5fd8c2fd24 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Dec 14 2016 09:48:35 +0000 Subject: [PATCH 4/10] Place the methods in the order in which they are used for easier reading --- diff --git a/pagure/lib/encoding_utils.py b/pagure/lib/encoding_utils.py index f2a5666..0372889 100644 --- a/pagure/lib/encoding_utils.py +++ b/pagure/lib/encoding_utils.py @@ -25,6 +25,38 @@ _log = logging.getLogger(__name__) Guess = namedtuple('Guess', ['encoding', 'confidence']) +def detect_encodings(data): + """ + Analyze the provided data for possible character encodings. + + This simply wraps chardet and extracts all the potential encodings it + considered before deciding on a particular result. + + :param data: An array of bytes to treat as text data + :type data: bytes + + :return: A dictionary mapping possible encodings to confidence levels + :rtype: dict + """ + if not data: + # It's an empty string so we can safely say it's ascii + return {'ascii': 1.0} + + # We can't use ``chardet.detect`` because we want to dig in the internals + # of the detector to bias the utf-8 result. + detector = universaldetector.UniversalDetector() + detector.reset() + detector.feed(data) + result = detector.close() + if not result: + return {'utf-8': 1.0} + encodings = {result['encoding']: result['confidence']} + for prober in detector._mCharSetProbers: + if prober: + encodings[prober.get_charset_name()] = prober.get_confidence() + + return encodings + def guess_encodings(data): """ @@ -92,39 +124,6 @@ def guess_encoding(data): raise PagureException('No encoding could be guessed for this file') -def detect_encodings(data): - """ - Analyze the provided data for possible character encodings. - - This simply wraps chardet and extracts all the potential encodings it - considered before deciding on a particular result. - - :param data: An array of bytes to treat as text data - :type data: bytes - - :return: A dictionary mapping possible encodings to confidence levels - :rtype: dict - """ - if not data: - # It's an empty string so we can safely say it's ascii - return {'ascii': 1.0} - - # We can't use ``chardet.detect`` because we want to dig in the internals - # of the detector to bias the utf-8 result. - detector = universaldetector.UniversalDetector() - detector.reset() - detector.feed(data) - result = detector.close() - if not result: - return {'utf-8': 1.0} - encodings = {result['encoding']: result['confidence']} - for prober in detector._mCharSetProbers: - if prober: - encodings[prober.get_charset_name()] = prober.get_confidence() - - return encodings - - def decode(data): """ Guesses the encoding using ``guess_encoding`` and decodes the data. From 29890e9b6f31287f7342d4d905194f89c2abbc82 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Dec 14 2016 09:48:35 +0000 Subject: [PATCH 5/10] Add unit-tests for pagure.lib.encoding_utils.guess_encodings() --- diff --git a/tests/test_pagure_lib_encoding_utils.py b/tests/test_pagure_lib_encoding_utils.py index b99e0b1..c06b584 100644 --- a/tests/test_pagure_lib_encoding_utils.py +++ b/tests/test_pagure_lib_encoding_utils.py @@ -40,6 +40,25 @@ class TestGuessEncoding(unittest.TestCase): self.assertEqual(result, 'ascii') +class TestGuessEncodings(unittest.TestCase): + + def test_guess_encodings(self): + """ Test the encoding_utils.guess_encodings() method. """ + data = u'Šabata'.encode('utf-8') + result = encoding_utils.guess_encodings(data) + chardet_result = chardet.detect(data) + self.assertEqual( + [encoding.encoding for encoding in result], + ['utf-8', 'ISO-8859-2', 'windows-1252']) + self.assertEqual(chardet_result['encoding'], 'ISO-8859-2') + + def test_guess_encodings_no_data(self): + """ Test encoding_utils.guess_encodings() with an emtpy string """ + result = encoding_utils.guess_encodings(u''.encode('utf-8')) + self.assertEqual( + [encoding.encoding for encoding in result], + ['ascii']) + class TestDecode(unittest.TestCase): def test_decode(self): From f072477a6201564b3aa677f8b20e21922fb89e54 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Dec 14 2016 09:48:35 +0000 Subject: [PATCH 6/10] Adjust docstrings in pagure.lib.encoding_utils --- diff --git a/pagure/lib/encoding_utils.py b/pagure/lib/encoding_utils.py index 0372889..3bf418c 100644 --- a/pagure/lib/encoding_utils.py +++ b/pagure/lib/encoding_utils.py @@ -34,9 +34,9 @@ def detect_encodings(data): :param data: An array of bytes to treat as text data :type data: bytes - :return: A dictionary mapping possible encodings to confidence levels :rtype: dict + """ if not data: # It's an empty string so we can safely say it's ascii @@ -74,6 +74,9 @@ def guess_encodings(data): :param data: An array of bytes to treat as text data :type data: bytes + :return: A dictionary mapping possible encodings to confidence levels + :rtype: dict + """ encodings = detect_encodings(data) @@ -109,8 +112,11 @@ def guess_encoding(data): :param data: An array of bytes to treat as text data :type data: bytes + :return: A string of the best encoding found + :rtype: str :raises PagureException: if no encoding was found that the data could be decoded into + """ encodings = guess_encodings(data) From 195dd848cabfe9924289b8755540f58afb82383b Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Dec 14 2016 09:48:35 +0000 Subject: [PATCH 7/10] Decode the content of the README files when converting them --- diff --git a/pagure/doc_utils.py b/pagure/doc_utils.py index 6ea748f..95f47c5 100644 --- a/pagure/doc_utils.py +++ b/pagure/doc_utils.py @@ -12,11 +12,13 @@ import docutils import docutils.core import docutils.examples +import kitchen.text.converters as ktc import markupsafe import markdown import textwrap import pagure.lib +import pagure.lib.encoding_utils def modify_rst(rst, view_file_url=None): @@ -95,17 +97,17 @@ def convert_readme(content, ext, view_file_url=None): ''' Convert the provided content according to the extension of the file provided. ''' - output = content + output = pagure.lib.encoding_utils.decode(ktc.to_bytes(content)) safe = False if ext and ext in ['.rst']: safe = True - output = convert_doc(content.decode('utf-8'), view_file_url) + output = convert_doc(output, view_file_url) elif ext and ext in ['.mk', '.md', '.markdown']: - output = pagure.lib.text2markdown(content.decode('utf-8')) + output = pagure.lib.text2markdown(output) safe = True elif not ext or (ext and ext in ['.text', '.txt']): safe = True - output = '
%s
' % content + output = '
%s
' % output return output, safe From 826adb9d14a123a3da2c0836205b80821815cdae Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Dec 14 2016 09:48:35 +0000 Subject: [PATCH 8/10] Fix typo in the docstring pointed out by @jcline :) --- diff --git a/tests/test_pagure_lib_encoding_utils.py b/tests/test_pagure_lib_encoding_utils.py index c06b584..67fdb68 100644 --- a/tests/test_pagure_lib_encoding_utils.py +++ b/tests/test_pagure_lib_encoding_utils.py @@ -35,7 +35,7 @@ class TestGuessEncoding(unittest.TestCase): self.assertEqual(chardet_result['encoding'], 'ISO-8859-2') def test_guess_encoding_no_data(self): - """ Test encoding_utils.guess_encoding() with an emtpy string """ + """ Test encoding_utils.guess_encoding() with an empty string """ result = encoding_utils.guess_encoding(u''.encode('utf-8')) self.assertEqual(result, 'ascii') From 7a4e68a1de083a0bccd4a04853104c9829cf1ffb Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Dec 14 2016 09:48:35 +0000 Subject: [PATCH 9/10] Drop redundant call to str() --- diff --git a/pagure/lib/encoding_utils.py b/pagure/lib/encoding_utils.py index 3bf418c..550de58 100644 --- a/pagure/lib/encoding_utils.py +++ b/pagure/lib/encoding_utils.py @@ -121,7 +121,7 @@ def guess_encoding(data): encodings = guess_encodings(data) for encoding in encodings: - _log.debug('Trying encoding: %s', str(encoding)) + _log.debug('Trying encoding: %s', encoding) try: data.decode(encoding.encoding) return encoding.encoding From 826f227175c61bacf5d39579d81db82954455ca9 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Dec 14 2016 09:48:35 +0000 Subject: [PATCH 10/10] Return a 500 error instead of a 400 when the server fails to decode the file --- diff --git a/pagure/ui/repo.py b/pagure/ui/repo.py index e36f5a6..bd94814 100644 --- a/pagure/ui/repo.py +++ b/pagure/ui/repo.py @@ -723,7 +723,7 @@ def view_blame_file(repo, filename, username=None, namespace=None): except pagure.exceptions.PagureException: # We cannot decode the file, so bail but warn the admins LOG.exception('File could not be decoded') - flask.abort(400, 'File could not be decoded') + flask.abort(500, 'File could not be decoded') blame = repo_obj.blame(filename)