Phill has proposed merging lp:~phill-ridout/openlp/bug1073931 into lp:openlp.
Requested reviews: OpenLP Core (openlp-core) For more details, see: https://code.launchpad.net/~phill-ridout/openlp/bug1073931/+merge/248803 Fixes bug1073931 "Corrupted databases stop OpenLP from starting" Checks if the database session is available before trying to use it. Use a sha256 hash to verify downloaded files. See also: https://code.launchpad.net/~phill-ridout/openlp/sha256 Tests failed on the crosswalk tests. I have included my locally run test output. Add this to your merge proposal: -------------------------------- lp:~phill-ridout/openlp/bug1073931 (revision 2495) [SUCCESS] http://ci.openlp.org/job/Branch-01-Pull/934/ [SUCCESS] http://ci.openlp.org/job/Branch-02-Functional-Tests/860/ [FAILURE] http://ci.openlp.org/job/Branch-03-Interface-Tests/805/ Stopping after failure Process finished with exit code 0 E Error Traceback (most recent call last): File "/usr/lib/python3.4/unittest/case.py", line 58, in testPartExecutor yield File "/usr/lib/python3.4/unittest/case.py", line 577, in run testMethod() File "/home/phill/Projects/openlp/bug1073931/tests/interfaces/openlp_plugins/bibles/test_lib_http.py", line 102, in crosswalk_extract_books_test books = handler.get_books_from_http('niv') File "/home/phill/Projects/openlp/bug1073931/openlp/plugins/bibles/lib/http.py", line 413, in get_books_from_http content = content.find('ul', {'class': 'parent'}) AttributeError: 'NoneType' object has no attribute 'find' -------------------- >> begin captured logging << -------------------- openlp.core.common.registry: INFO: Registry Initialising openlp.plugins.bibles.lib.http: DEBUG: CWExtract.init("None") openlp.plugins.bibles.lib.http: DEBUG: CWExtract.get_books_from_http("niv") openlp.core.utils.__init__: DEBUG: Downloading URL = http://www.biblestudytools.com/niv/ openlp.core.utils.__init__: DEBUG: Downloaded URL = http://www.biblestudytools.com/niv/ openlp.core.utils.__init__: DEBUG: <http.client.HTTPResponse object at 0x7f66e37052b0> --------------------- >> end captured logging << --------------------- E Error Traceback (most recent call last): File "/usr/lib/python3.4/unittest/case.py", line 58, in testPartExecutor yield File "/usr/lib/python3.4/unittest/case.py", line 577, in run testMethod() File "/home/phill/Projects/openlp/bug1073931/tests/interfaces/openlp_plugins/bibles/test_lib_http.py", line 115, in crosswalk_extract_verse_test results = handler.get_bible_chapter('niv', 'john', 3) File "/home/phill/Projects/openlp/bug1073931/openlp/plugins/bibles/lib/http.py", line 371, in get_bible_chapter send_error_message('parse') File "/home/phill/Projects/openlp/bug1073931/openlp/plugins/bibles/lib/http.py", line 649, in send_error_message translate('BiblesPlugin.HTTPBible', 'There was a problem extracting your verse selection. If this error ' File "/home/phill/Projects/openlp/bug1073931/openlp/core/lib/ui.py", line 114, in critical_error_message_box return Registry().get('main_window').error_message(title if title else UiStrings().Error, message) AttributeError: 'NoneType' object has no attribute 'error_message' -------------------- >> begin captured logging << -------------------- openlp.core.common.registry: INFO: Registry Initialising openlp.plugins.bibles.lib.http: DEBUG: CWExtract.init("None") openlp.plugins.bibles.lib.http: DEBUG: CWExtract.get_bible_chapter("niv", "john", "3") openlp.core.utils.__init__: DEBUG: Downloading URL = http://www.biblestudytools.com/niv/john/3.html openlp.core.utils.__init__: DEBUG: Downloaded URL = http://www.biblestudytools.com/niv/john/3.html openlp.core.utils.__init__: DEBUG: <http.client.HTTPResponse object at 0x7f66e33ecd68> openlp.plugins.bibles.lib.http: ERROR: No verses found in the CrossWalk response. --------------------- >> end captured logging << --------------------- ====================================================================== ERROR: Test Crosswalk retrieval of book list for NIV bible ---------------------------------------------------------------------- Traceback (most recent call last): File "/home/phill/Projects/openlp/bug1073931/tests/interfaces/openlp_plugins/bibles/test_lib_http.py", line 102, in crosswalk_extract_books_test books = handler.get_books_from_http('niv') File "/home/phill/Projects/openlp/bug1073931/openlp/plugins/bibles/lib/http.py", line 413, in get_books_from_http content = content.find('ul', {'class': 'parent'}) nose.proxy.AttributeError: 'NoneType' object has no attribute 'find' -------------------- >> begin captured logging << -------------------- openlp.core.common.registry: INFO: Registry Initialising openlp.plugins.bibles.lib.http: DEBUG: CWExtract.init("None") openlp.plugins.bibles.lib.http: DEBUG: CWExtract.get_books_from_http("niv") openlp.core.utils.__init__: DEBUG: Downloading URL = http://www.biblestudytools.com/niv/ openlp.core.utils.__init__: DEBUG: Downloaded URL = http://www.biblestudytools.com/niv/ openlp.core.utils.__init__: DEBUG: <http.client.HTTPResponse object at 0x7f66e37052b0> --------------------- >> end captured logging << --------------------- ====================================================================== ERROR: Test Crosswalk retrieval of verse list for NIV bible John 3 ---------------------------------------------------------------------- Traceback (most recent call last): File "/home/phill/Projects/openlp/bug1073931/tests/interfaces/openlp_plugins/bibles/test_lib_http.py", line 115, in crosswalk_extract_verse_test results = handler.get_bible_chapter('niv', 'john', 3) File "/home/phill/Projects/openlp/bug1073931/openlp/plugins/bibles/lib/http.py", line 371, in get_bible_chapter send_error_message('parse') File "/home/phill/Projects/openlp/bug1073931/openlp/plugins/bibles/lib/http.py", line 649, in send_error_message translate('BiblesPlugin.HTTPBible', 'There was a problem extracting your verse selection. If this error ' File "/home/phill/Projects/openlp/bug1073931/openlp/core/lib/ui.py", line 114, in critical_error_message_box return Registry().get('main_window').error_message(title if title else UiStrings().Error, message) nose.proxy.AttributeError: 'NoneType' object has no attribute 'error_message' -------------------- >> begin captured logging << -------------------- openlp.core.common.registry: INFO: Registry Initialising openlp.plugins.bibles.lib.http: DEBUG: CWExtract.init("None") openlp.plugins.bibles.lib.http: DEBUG: CWExtract.get_bible_chapter("niv", "john", "3") openlp.core.utils.__init__: DEBUG: Downloading URL = http://www.biblestudytools.com/niv/john/3.html openlp.core.utils.__init__: DEBUG: Downloaded URL = http://www.biblestudytools.com/niv/john/3.html openlp.core.utils.__init__: DEBUG: <http.client.HTTPResponse object at 0x7f66e33ecd68> openlp.plugins.bibles.lib.http: ERROR: No verses found in the CrossWalk response. --------------------- >> end captured logging << --------------------- ---------------------------------------------------------------------- Ran 576 tests in 26.153s FAILED (SKIP=4, errors=2) Process finished with exit code 1 -- Your team OpenLP Core is requested to review the proposed merge of lp:~phill-ridout/openlp/bug1073931 into lp:openlp.
=== modified file 'openlp/core/lib/db.py' --- openlp/core/lib/db.py 2015-01-19 08:34:29 +0000 +++ openlp/core/lib/db.py 2015-02-05 17:35:44 +0000 @@ -60,6 +60,35 @@ return session, metadata +def get_db_path(plugin_name, db_file_name=None): + """ + Create a path to a database from the plugin name and database name + + :param plugin_name: Name of plugin + :param db_file_name: File name of database + :return: The path to the database as type str + """ + if db_file_name is None: + return 'sqlite:///%s/%s.sqlite' % (AppLocation.get_section_data_path(plugin_name), plugin_name) + else: + return 'sqlite:///%s/%s' % (AppLocation.get_section_data_path(plugin_name), db_file_name) + + +def handle_db_error(plugin_name, db_file_name): + """ + Log and report to the user that a database cannot be loaded + + :param plugin_name: Name of plugin + :param db_file_name: File name of database + :return: None + """ + db_path = get_db_path(plugin_name, db_file_name) + log.exception('Error loading database: %s', db_path) + critical_error_message_box(translate('OpenLP.Manager', 'Database Error'), + translate('OpenLP.Manager', 'OpenLP cannot load your database.\n\nDatabase: %s') + % db_path) + + def init_url(plugin_name, db_file_name=None): """ Return the database URL. @@ -69,13 +98,9 @@ """ settings = Settings() settings.beginGroup(plugin_name) - db_url = '' db_type = settings.value('db type') if db_type == 'sqlite': - if db_file_name is None: - db_url = 'sqlite:///%s/%s.sqlite' % (AppLocation.get_section_data_path(plugin_name), plugin_name) - else: - db_url = 'sqlite:///%s/%s' % (AppLocation.get_section_data_path(plugin_name), db_file_name) + db_url = get_db_path(plugin_name, db_file_name) else: db_url = '%s://%s:%s@%s/%s' % (db_type, urlquote(settings.value('db username')), urlquote(settings.value('db password')), @@ -212,7 +237,7 @@ try: db_ver, up_ver = upgrade_db(self.db_url, upgrade_mod) except (SQLAlchemyError, DBAPIError): - log.exception('Error loading database: %s', self.db_url) + handle_db_error(plugin_name, db_file_name) return if db_ver > up_ver: critical_error_message_box( @@ -225,10 +250,7 @@ try: self.session = init_schema(self.db_url) except (SQLAlchemyError, DBAPIError): - log.exception('Error loading database: %s', self.db_url) - critical_error_message_box(translate('OpenLP.Manager', 'Database Error'), - translate('OpenLP.Manager', 'OpenLP cannot load your database.\n\nDatabase: %s') - % self.db_url) + handle_db_error(plugin_name, db_file_name) def save_object(self, object_instance, commit=True): """ @@ -362,6 +384,8 @@ :param object_class: The type of objects to return. :param filter_clause: The filter governing selection of objects to return. Defaults to None. """ + if not self.session: + return query = self.session.query(object_class) if filter_clause is not None: query = query.filter(filter_clause) === modified file 'openlp/core/ui/firsttimeform.py' --- openlp/core/ui/firsttimeform.py 2015-01-20 21:38:34 +0000 +++ openlp/core/ui/firsttimeform.py 2015-02-05 17:35:44 +0000 @@ -22,6 +22,7 @@ """ This module contains the first time wizard. """ +import hashlib import logging import os import time @@ -47,10 +48,10 @@ """ This thread downloads a theme's screenshot """ - screenshot_downloaded = QtCore.pyqtSignal(str, str) + screenshot_downloaded = QtCore.pyqtSignal(str, str, str) finished = QtCore.pyqtSignal() - def __init__(self, themes_url, title, filename, screenshot): + def __init__(self, themes_url, title, filename, sha256, screenshot): """ Set up the worker object """ @@ -58,6 +59,7 @@ self.themes_url = themes_url self.title = title self.filename = filename + self.sha256 = sha256 self.screenshot = screenshot super(ThemeScreenshotWorker, self).__init__() @@ -71,7 +73,7 @@ urllib.request.urlretrieve('%s%s' % (self.themes_url, self.screenshot), os.path.join(gettempdir(), 'openlp', self.screenshot)) # Signal that the screenshot has been downloaded - self.screenshot_downloaded.emit(self.title, self.filename) + self.screenshot_downloaded.emit(self.title, self.filename, self.sha256) except: log.exception('Unable to download screenshot') finally: @@ -221,8 +223,9 @@ self.application.process_events() title = self.config.get('songs_%s' % song, 'title') filename = self.config.get('songs_%s' % song, 'filename') + sha256 = self.config.get('songs_%s' % song, 'sha256', fallback='') item = QtGui.QListWidgetItem(title, self.songs_list_widget) - item.setData(QtCore.Qt.UserRole, filename) + item.setData(QtCore.Qt.UserRole, (filename, sha256)) item.setCheckState(QtCore.Qt.Unchecked) item.setFlags(item.flags() | QtCore.Qt.ItemIsUserCheckable) bible_languages = self.config.get('bibles', 'languages') @@ -237,8 +240,9 @@ self.application.process_events() title = self.config.get('bible_%s' % bible, 'title') filename = self.config.get('bible_%s' % bible, 'filename') + sha256 = self.config.get('bible_%s' % bible, 'sha256', fallback='') item = QtGui.QTreeWidgetItem(lang_item, [title]) - item.setData(0, QtCore.Qt.UserRole, filename) + item.setData(0, QtCore.Qt.UserRole, (filename, sha256)) item.setCheckState(0, QtCore.Qt.Unchecked) item.setFlags(item.flags() | QtCore.Qt.ItemIsUserCheckable) self.bibles_tree_widget.expandAll() @@ -249,8 +253,9 @@ self.application.process_events() title = self.config.get('theme_%s' % theme, 'title') filename = self.config.get('theme_%s' % theme, 'filename') + sha256 = self.config.get('theme_%s' % theme, 'sha256', fallback='') screenshot = self.config.get('theme_%s' % theme, 'screenshot') - worker = ThemeScreenshotWorker(self.themes_url, title, filename, screenshot) + worker = ThemeScreenshotWorker(self.themes_url, title, filename, sha256, screenshot) self.theme_screenshot_workers.append(worker) worker.screenshot_downloaded.connect(self.on_screenshot_downloaded) thread = QtCore.QThread(self) @@ -350,7 +355,7 @@ time.sleep(0.1) self.application.set_normal_cursor() - def on_screenshot_downloaded(self, title, filename): + def on_screenshot_downloaded(self, title, filename, sha256): """ Add an item to the list when a theme has been downloaded @@ -358,7 +363,7 @@ :param filename: The filename of the theme """ item = QtGui.QListWidgetItem(title, self.themes_list_widget) - item.setData(QtCore.Qt.UserRole, filename) + item.setData(QtCore.Qt.UserRole, (filename, sha256)) item.setCheckState(QtCore.Qt.Unchecked) item.setFlags(item.flags() | QtCore.Qt.ItemIsUserCheckable) @@ -372,7 +377,7 @@ Settings().setValue('core/has run wizard', True) self.close() - def url_get_file(self, url, f_path): + def url_get_file(self, url, f_path, sha256=None): """" Download a file given a URL. The file is retrieved in chunks, giving the ability to cancel the download at any point. Returns False on download error. @@ -387,16 +392,24 @@ try: url_file = urllib.request.urlopen(url, timeout=CONNECTION_TIMEOUT) filename = open(f_path, "wb") + if sha256: + hasher = hashlib.sha256() # Download until finished or canceled. while not self.was_cancelled: data = url_file.read(block_size) if not data: break filename.write(data) + if sha256: + hasher.update(data) block_count += 1 self._download_progress(block_count, block_size) filename.close() - except ConnectionError: + if sha256 and hasher.hexdigest() != sha256: + log.error('sha256 sums did not match for file: {}'.format(f_path)) + os.remove(f_path) + return False + except urllib.error.URLError: trace_error_handler(log) filename.close() os.remove(f_path) @@ -436,7 +449,7 @@ site = urllib.request.urlopen(url, timeout=CONNECTION_TIMEOUT) meta = site.info() return int(meta.get("Content-Length")) - except ConnectionException: + except urllib.error.URLError: if retries > CONNECTION_RETRIES: raise else: @@ -478,7 +491,7 @@ self.application.process_events() item = self.songs_list_widget.item(i) if item.checkState() == QtCore.Qt.Checked: - filename = item.data(QtCore.Qt.UserRole) + filename, _ = item.data(QtCore.Qt.UserRole) size = self._get_file_size('%s%s' % (self.songs_url, filename)) self.max_progress += size # Loop through the Bibles list and increase for each selected item @@ -487,7 +500,7 @@ self.application.process_events() item = iterator.value() if item.parent() and item.checkState(0) == QtCore.Qt.Checked: - filename = item.data(0, QtCore.Qt.UserRole) + filename, _ = item.data(0, QtCore.Qt.UserRole) size = self._get_file_size('%s%s' % (self.bibles_url, filename)) self.max_progress += size iterator += 1 @@ -496,10 +509,10 @@ self.application.process_events() item = self.themes_list_widget.item(i) if item.checkState() == QtCore.Qt.Checked: - filename = item.data(QtCore.Qt.UserRole) + filename, _ = item.data(QtCore.Qt.UserRole) size = self._get_file_size('%s%s' % (self.themes_url, filename)) self.max_progress += size - except ConnectionError: + except urllib.error.URLError: trace_error_handler(log) critical_error_message_box(translate('OpenLP.FirstTimeWizard', 'Download Error'), translate('OpenLP.FirstTimeWizard', 'There was a connection problem during ' @@ -595,31 +608,33 @@ for i in range(self.songs_list_widget.count()): item = self.songs_list_widget.item(i) if item.checkState() == QtCore.Qt.Checked: - filename = item.data(QtCore.Qt.UserRole) + filename, sha256 = item.data(QtCore.Qt.UserRole) self._increment_progress_bar(self.downloading % filename, 0) self.previous_size = 0 destination = os.path.join(songs_destination, str(filename)) - if not self.url_get_file('%s%s' % (self.songs_url, filename), destination): + if not self.url_get_file('%s%s' % (self.songs_url, filename), destination, sha256): return False # Download Bibles bibles_iterator = QtGui.QTreeWidgetItemIterator(self.bibles_tree_widget) while bibles_iterator.value(): item = bibles_iterator.value() if item.parent() and item.checkState(0) == QtCore.Qt.Checked: - bible = item.data(0, QtCore.Qt.UserRole) + bible, sha256 = item.data(0, QtCore.Qt.UserRole) self._increment_progress_bar(self.downloading % bible, 0) self.previous_size = 0 - if not self.url_get_file('%s%s' % (self.bibles_url, bible), os.path.join(bibles_destination, bible)): + if not self.url_get_file('%s%s' % (self.bibles_url, bible), os.path.join(bibles_destination, bible), + sha256): return False bibles_iterator += 1 # Download themes for i in range(self.themes_list_widget.count()): item = self.themes_list_widget.item(i) if item.checkState() == QtCore.Qt.Checked: - theme = item.data(QtCore.Qt.UserRole) + theme, sha256 = item.data(QtCore.Qt.UserRole) self._increment_progress_bar(self.downloading % theme, 0) self.previous_size = 0 - if not self.url_get_file('%s%s' % (self.themes_url, theme), os.path.join(themes_destination, theme)): + if not self.url_get_file('%s%s' % (self.themes_url, theme), os.path.join(themes_destination, theme), + sha256): return False return True === modified file 'openlp/plugins/bibles/lib/db.py' --- openlp/plugins/bibles/lib/db.py 2015-01-18 13:39:21 +0000 +++ openlp/plugins/bibles/lib/db.py 2015-02-05 17:35:44 +0000 @@ -131,6 +131,7 @@ log.info('BibleDB loaded') QtCore.QObject.__init__(self) self.bible_plugin = parent + self.session = None if 'path' not in kwargs: raise KeyError('Missing keyword argument "path".') if 'name' not in kwargs and 'file' not in kwargs: @@ -144,8 +145,9 @@ if 'file' in kwargs: self.file = kwargs['file'] Manager.__init__(self, 'bibles', init_schema, self.file, upgrade) - if 'file' in kwargs: - self.get_name() + if self.session: + if 'file' in kwargs: + self.get_name() if 'path' in kwargs: self.path = kwargs['path'] self.wizard = None === modified file 'openlp/plugins/bibles/lib/manager.py' --- openlp/plugins/bibles/lib/manager.py 2015-01-18 13:39:21 +0000 +++ openlp/plugins/bibles/lib/manager.py 2015-02-05 17:35:44 +0000 @@ -121,6 +121,8 @@ self.old_bible_databases = [] for filename in files: bible = BibleDB(self.parent, path=self.path, file=filename) + if not bible.session: + continue name = bible.get_name() # Remove corrupted files. if name is None: === modified file 'tests/functional/test_init.py' --- tests/functional/test_init.py 2015-01-26 20:42:19 +0000 +++ tests/functional/test_init.py 2015-02-05 17:35:44 +0000 @@ -125,7 +125,7 @@ # WHEN: Calling parse_options results = parse_options(options) - # THEN: A tuple should be returned with the parsed options and left over args + # THEN: A tuple should be returned with the parsed options and left over options self.assertEqual(results, (Values({'no_error_form': True, 'dev_version': True, 'portable': True, 'style': 'style', 'loglevel': 'debug'}), ['extra', 'qt', 'args'])) @@ -136,10 +136,51 @@ # GIVEN: A list of valid options options = ['openlp.py', '-e', '-l', 'debug', '-pd', '-s', 'style', 'extra', 'qt', 'args'] - # WHEN: Passing in the options through sys.argv and calling parse_args with None + # WHEN: Passing in the options through sys.argv and calling parse_options with None with patch.object(sys, 'argv', options): results = parse_options(None) - # THEN: parse_args should return a tuple of valid options and of left over options that OpenLP does not use - self.assertEqual(results, (Values({'no_error_form': True, 'dev_version': True, 'portable': True, - 'style': 'style', 'loglevel': 'debug'}), ['extra', 'qt', 'args'])) + # THEN: parse_options should return a tuple of valid options and of left over options that OpenLP does not use + self.assertEqual(results, (Values({'no_error_form': True, 'dev_version': True, 'portable': True, + 'style': 'style', 'loglevel': 'debug'}), ['extra', 'qt', 'args'])) + + def test_parse_options_valid_long_options(self): + """ + Test that parse_options parses valid long options correctly + """ + # GIVEN: A list of valid options + options = ['--no-error-form', 'extra', '--log-level', 'debug', 'qt', '--portable', '--dev-version', 'args', + '--style=style'] + + # WHEN: Calling parse_options + results = parse_options(options) + + # THEN: parse_options should return a tuple of valid options and of left over options that OpenLP does not use + self.assertEqual(results, (Values({'no_error_form': True, 'dev_version': True, 'portable': True, + 'style': 'style', 'loglevel': 'debug'}), ['extra', 'qt', 'args'])) + + def test_parse_options_help_option(self): + """ + Test that parse_options raises an SystemExit exception when called with invalid options + """ + # GIVEN: The --help option + options = ['--help'] + + # WHEN: Calling parse_options + # THEN: parse_options should raise an SystemExit exception with exception code 0 + with self.assertRaises(SystemExit) as raised_exception: + parse_options(options) + self.assertEqual(raised_exception.exception.code, 0) + + def test_parse_options_invalid_option(self): + """ + Test that parse_options raises an SystemExit exception when called with invalid options + """ + # GIVEN: A list including invalid options + options = ['-t'] + + # WHEN: Calling parse_options + # THEN: parse_options should raise an SystemExit exception with exception code 2 + with self.assertRaises(SystemExit) as raised_exception: + parse_options(options) + self.assertEqual(raised_exception.exception.code, 2)
_______________________________________________ Mailing list: https://launchpad.net/~openlp-core Post to : openlp-core@lists.launchpad.net Unsubscribe : https://launchpad.net/~openlp-core More help : https://help.launchpad.net/ListHelp