From 6dba781aa687824c17a4af2c15bcd40a448f5813 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Feb 16 2017 09:23:52 +0000 Subject: [PATCH 1/4] Let the doc server return something even if we do not know its encoding Sometime when trying to retrieve the encoding of some content, we just fail at guessing it. So far this led pagure to return a 500 error without any content. With this commit, we know catch the exception, consider the content as not safe but we will return it. This means that we will at least return something to the user. This fixes an error reported by pagure by email where it could not render https://docs.pagure.org/packaging-guidelines/ --- diff --git a/pagure/docs_server.py b/pagure/docs_server.py index 3947a63..66aad6d 100644 --- a/pagure/docs_server.py +++ b/pagure/docs_server.py @@ -111,7 +111,12 @@ def __get_tree_and_content(repo_obj, commit, path): ext = os.path.splitext(blob_or_tree.name)[1] blob_obj = repo_obj[blob_or_tree.oid] if not is_binary_string(blob_obj.data): - content, safe = pagure.doc_utils.convert_readme(blob_obj.data, ext) + try: + content, safe = pagure.doc_utils.convert_readme( + blob_obj.data, ext) + except pagure.exceptions.PagureException: + safe = False + content = blob_obj.data else: safe = True content = blob_obj.data From f88a8600c38e726648aa9fed275b12e5947bd7f7 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Feb 16 2017 09:23:52 +0000 Subject: [PATCH 2/4] Enhance the unit-tests on the doc server We're now testing how the server behaves when faced to some exceptions --- diff --git a/pagure/docs_server.py b/pagure/docs_server.py index 66aad6d..42ee168 100644 --- a/pagure/docs_server.py +++ b/pagure/docs_server.py @@ -176,6 +176,9 @@ def view_docs(repo, username=None, namespace=None, filename=None): filename += '/' except pagure.exceptions.FileNotFoundException as err: flask.flash(err.message, 'error') + except Exception as err: + LOG.exception(err) + flask.abort(500, 'Unkown error encountered and reported') mimetype = None if not filename: diff --git a/tests/test_pagure_flask_docs.py b/tests/test_pagure_flask_docs.py index 4756a5b..297f566 100644 --- a/tests/test_pagure_flask_docs.py +++ b/tests/test_pagure_flask_docs.py @@ -1,7 +1,7 @@ # -*- coding: utf-8 -*- """ - (c) 2015 - Copyright Red Hat Inc + (c) 2015-2017 - Copyright Red Hat Inc Authors: Pierre-Yves Chibon @@ -17,6 +17,7 @@ import shutil import sys import os +import mock import pygit2 from mock import patch @@ -58,6 +59,53 @@ class PagureFlaskDocstests(tests.Modeltests): self.path, 'docs') self.app = pagure.docs_server.APP.test_client() + def _set_up_doc(self): + # forked doc repo + docrepo = os.path.join(self.path, 'docs', 'test', 'test.git') + repo = pygit2.init_repository(docrepo) + + # Create files in that git repo + with open(os.path.join(docrepo, 'sources'), 'w') as stream: + stream.write('foo\n bar') + repo.index.add('sources') + repo.index.write() + + folderpart = os.path.join(docrepo, 'folder1', 'folder2') + os.makedirs(folderpart) + with open(os.path.join(folderpart, 'test_file'), 'w') as stream: + stream.write('row1\nrow2\nrow3') + repo.index.add(os.path.join('folder1', 'folder2', 'test_file')) + repo.index.write() + + # Commits the files added + tree = repo.index.write_tree() + author = pygit2.Signature( + 'Alice Author', 'alice@authors.tld') + committer = pygit2.Signature( + 'Cecil Committer', 'cecil@committers.tld') + repo.create_commit( + 'refs/heads/master', # the name of the reference to update + author, + committer, + 'Add test files and folder', + # binary string representing the tree object ID + tree, + # list of binary strings representing parents of the new commit + [] + ) + + # Push the changes to the bare repo + remote = repo.create_remote( + 'origin', os.path.join(self.path, 'docs', 'test.git')) + + PagureRepo.push(remote, 'refs/heads/master:refs/heads/master') + + # Turn on the docs project since it's off by default + repo = pagure.lib.get_project(self.session, 'test') + repo.settings = {'project_documentation': True} + self.session.add(repo) + self.session.commit() + def test_view_docs_no_project(self): """ Test the view_docs endpoint with no project. """ @@ -113,51 +161,7 @@ class PagureFlaskDocstests(tests.Modeltests): output = self.app.get('/test/docs') self.assertEqual(output.status_code, 404) - # forked doc repo - docrepo = os.path.join(self.path, 'docs', 'test', 'test.git') - repo = pygit2.init_repository(docrepo) - - # Create files in that git repo - with open(os.path.join(docrepo, 'sources'), 'w') as stream: - stream.write('foo\n bar') - repo.index.add('sources') - repo.index.write() - - folderpart = os.path.join(docrepo, 'folder1', 'folder2') - os.makedirs(folderpart) - with open(os.path.join(folderpart, 'test_file'), 'w') as stream: - stream.write('row1\nrow2\nrow3') - repo.index.add(os.path.join('folder1', 'folder2', 'test_file')) - repo.index.write() - - # Commits the files added - tree = repo.index.write_tree() - author = pygit2.Signature( - 'Alice Author', 'alice@authors.tld') - committer = pygit2.Signature( - 'Cecil Committer', 'cecil@committers.tld') - repo.create_commit( - 'refs/heads/master', # the name of the reference to update - author, - committer, - 'Add test files and folder', - # binary string representing the tree object ID - tree, - # list of binary strings representing parents of the new commit - [] - ) - - # Push the changes to the bare repo - remote = repo.create_remote( - 'origin', os.path.join(self.path, 'docs', 'test.git')) - - PagureRepo.push(remote, 'refs/heads/master:refs/heads/master') - - # Turn on the docs project since it's off by default - repo = pagure.lib.get_project(self.session, 'test') - repo.settings = {'project_documentation': True} - self.session.add(repo) - self.session.commit() + self._set_up_doc() # Now check the UI @@ -190,6 +194,50 @@ class PagureFlaskDocstests(tests.Modeltests): output = self.app.get('/test/folder1/foo/folder2') self.assertEqual(output.status_code, 404) + @mock.patch( + 'pagure.lib.encoding_utils.decode', + mock.MagicMock(side_effect=pagure.exceptions.PagureException)) + def test_view_docs_encoding_error(self): + """ Test viewing a file of which we cannot find the encoding. """ + tests.create_projects(self.session) + repo = pygit2.init_repository( + os.path.join(self.path, 'docs', 'test.git'), bare=True) + + output = self.app.get('/test/docs') + self.assertEqual(output.status_code, 404) + + self._set_up_doc() + + output = self.app.get('/test/sources') + self.assertEqual(output.status_code, 200) + self.assertEqual('foo\n bar', output.data) + + output = self.app.get('/test/folder1') + self.assertEqual(output.status_code, 200) + self.assertTrue( + '
    • ' + in output.data) + + @mock.patch( + 'pagure.lib.encoding_utils.decode', + mock.MagicMock(side_effect=IOError)) + def test_view_docs_unknown_error(self): + """ Test viewing a file of which we cannot find the encoding. """ + tests.create_projects(self.session) + repo = pygit2.init_repository( + os.path.join(self.path, 'docs', 'test.git'), bare=True) + + output = self.app.get('/test/docs') + self.assertEqual(output.status_code, 404) + + self._set_up_doc() + + output = self.app.get('/test/sources') + self.assertEqual(output.status_code, 500) + + output = self.app.get('/test/folder1') + self.assertEqual(output.status_code, 500) + if __name__ == '__main__': SUITE = unittest.TestLoader().loadTestsFromTestCase(PagureFlaskDocstests) From ada323f4c2a2f8198f4fe74f8e6ff5ade2dfbf9c Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Feb 16 2017 09:23:52 +0000 Subject: [PATCH 3/4] Raise a PagureEncodingException when none of the encoding guessed worked In pagure.doc_utils.convert_readme() we try guessing the encoding of the file, then we iterate over all these encoding and try to decode the file in it. If all the encoding fails, we raise an exception, it used to be a generic PagureException, it will now be a PagureEncodingException inheriting from both PagureExpception and ValueError --- diff --git a/pagure/docs_server.py b/pagure/docs_server.py index 42ee168..bc17900 100644 --- a/pagure/docs_server.py +++ b/pagure/docs_server.py @@ -114,7 +114,7 @@ def __get_tree_and_content(repo_obj, commit, path): try: content, safe = pagure.doc_utils.convert_readme( blob_obj.data, ext) - except pagure.exceptions.PagureException: + except pagure.exceptions.PagureEncodingException: safe = False content = blob_obj.data else: diff --git a/pagure/exceptions.py b/pagure/exceptions.py index 6892623..d4e7a1a 100644 --- a/pagure/exceptions.py +++ b/pagure/exceptions.py @@ -87,3 +87,10 @@ class NoCorrespondingPR(PagureException): class InvalidObjectException(PagureException): ''' Exception raised when a given object is not what was expected. ''' pass + + +class PagureEncodingException(PagureException, ValueError): + ''' Exception raised none of the encoding guessed could be applied to + the content examined + ''' + pass diff --git a/pagure/lib/encoding_utils.py b/pagure/lib/encoding_utils.py index 550de58..70569d6 100644 --- a/pagure/lib/encoding_utils.py +++ b/pagure/lib/encoding_utils.py @@ -127,7 +127,7 @@ def guess_encoding(data): return encoding.encoding except UnicodeDecodeError: pass - raise PagureException('No encoding could be guessed for this file') + raise PagureEncodingException('No encoding could be guessed for this file') def decode(data): From 0183b4bc05d21c5ec7e56ade0fbc86dd56ea7da6 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Feb 16 2017 09:23:52 +0000 Subject: [PATCH 4/4] Use the proper PagureEncodingException exception in the tests --- diff --git a/tests/test_pagure_flask_docs.py b/tests/test_pagure_flask_docs.py index 297f566..d5e1731 100644 --- a/tests/test_pagure_flask_docs.py +++ b/tests/test_pagure_flask_docs.py @@ -196,7 +196,7 @@ class PagureFlaskDocstests(tests.Modeltests): @mock.patch( 'pagure.lib.encoding_utils.decode', - mock.MagicMock(side_effect=pagure.exceptions.PagureException)) + mock.MagicMock(side_effect=pagure.exceptions.PagureEncodingException)) def test_view_docs_encoding_error(self): """ Test viewing a file of which we cannot find the encoding. """ tests.create_projects(self.session)