Merge pull request #3073 from qutebrowser/sql-errors

Handle some sqlite errors gracefully

5 files changed, 170 insertions, 47 deletions

diff --git a/qutebrowser/app.py b/qutebrowser/app.py
index 9b5de8e2e..a64d2236e 100644
--- a/qutebrowser/app.py
+++ b/qutebrowser/app.py
@@ -439,13 +439,19 @@ def _init_modules(args, crash_handler):
     readline_bridge = readline.ReadlineBridge()
     objreg.register('readline-bridge', readline_bridge)
 
-    log.init.debug("Initializing sql...")
     try:
+        log.init.debug("Initializing sql...")
         sql.init(os.path.join(standarddir.data(), 'history.sqlite'))
+
+        log.init.debug("Initializing web history...")
+        history.init(qApp)
     except sql.SqlError as e:
-        error.handle_fatal_exc(e, args, 'Error initializing SQL',
-                               pre_text='Error initializing SQL')
-        sys.exit(usertypes.Exit.err_init)
+        if e.environmental:
+            error.handle_fatal_exc(e, args, 'Error initializing SQL',
+                                   pre_text='Error initializing SQL')
+            sys.exit(usertypes.Exit.err_init)
+        else:
+            raise
 
     log.init.debug("Initializing completion...")
     completiondelegate.init()
@@ -453,9 +459,6 @@ def _init_modules(args, crash_handler):
     log.init.debug("Initializing command history...")
     cmdhistory.init()
 
-    log.init.debug("Initializing web history...")
-    history.init(qApp)
-
     log.init.debug("Initializing crashlog...")
     if not args.no_err_windows:
         crash_handler.handle_segfault()
diff --git a/qutebrowser/browser/history.py b/qutebrowser/browser/history.py
index 2c4c1f226..de9e9bb4f 100644
--- a/qutebrowser/browser/history.py
+++ b/qutebrowser/browser/history.py
@@ -190,15 +190,23 @@ class WebHistory(sql.SqlTable):
             return
 
         atime = int(atime) if (atime is not None) else int(time.time())
-        self.insert({'url': self._format_url(url),
-                     'title': title,
-                     'atime': atime,
-                     'redirect': redirect})
-        if not redirect:
-            self.completion.insert({'url': self._format_completion_url(url),
-                                    'title': title,
-                                    'last_atime': atime},
-                                   replace=True)
+
+        try:
+            self.insert({'url': self._format_url(url),
+                         'title': title,
+                         'atime': atime,
+                         'redirect': redirect})
+            if not redirect:
+                self.completion.insert({
+                    'url': self._format_completion_url(url),
+                    'title': title,
+                    'last_atime': atime
+                }, replace=True)
+        except sql.SqlError as e:
+            if e.environmental:
+                message.error("Failed to write history: {}".format(e.text()))
+            else:
+                raise
 
     def _parse_entry(self, line):
         """Parse a history line like '12345 http://example.com title'."""
diff --git a/qutebrowser/misc/sql.py b/qutebrowser/misc/sql.py
index 24e2035e6..5b0d3361d 100644
--- a/qutebrowser/misc/sql.py
+++ b/qutebrowser/misc/sql.py
@@ -29,9 +29,66 @@ from qutebrowser.utils import log, debug
 
 class SqlError(Exception):
 
-    """Raised on an error interacting with the SQL database."""
+    """Raised on an error interacting with the SQL database.
 
-    pass
+    Attributes:
+        environmental: Whether the error is likely caused by the environment
+                       and not a qutebrowser bug.
+    """
+
+    def __init__(self, msg, environmental=False):
+        super().__init__(msg)
+        self.environmental = environmental
+
+    def text(self):
+        """Get a short text to display."""
+        return str(self)
+
+
+class SqliteError(SqlError):
+
+    """A SQL error with a QSqlError available.
+
+    Attributes:
+        error: The QSqlError object.
+    """
+
+    def __init__(self, msg, error):
+        super().__init__(msg)
+        self.error = error
+
+        log.sql.debug("SQL error:")
+        log.sql.debug("type: {}".format(
+            debug.qenum_key(QSqlError, error.type())))
+        log.sql.debug("database text: {}".format(error.databaseText()))
+        log.sql.debug("driver text: {}".format(error.driverText()))
+        log.sql.debug("error code: {}".format(error.nativeErrorCode()))
+
+        # https://sqlite.org/rescode.html
+        environmental_errors = [
+            # SQLITE_LOCKED,
+            # https://github.com/qutebrowser/qutebrowser/issues/2930
+            '9',
+            # SQLITE_FULL,
+            # https://github.com/qutebrowser/qutebrowser/issues/3004
+            '13',
+        ]
+        self.environmental = error.nativeErrorCode() in environmental_errors
+
+    def text(self):
+        return self.error.databaseText()
+
+    @classmethod
+    def from_query(cls, what, query, error):
+        """Construct an error from a failed query.
+
+        Arguments:
+            what: What we were doing when the error happened.
+            query: The query which was executed.
+            error: The QSqlError object.
+        """
+        msg = 'Failed to {} query "{}": "{}"'.format(what, query, error.text())
+        return cls(msg, error)
 
 
 def init(db_path):
@@ -39,13 +96,13 @@ def init(db_path):
     database = QSqlDatabase.addDatabase('QSQLITE')
     if not database.isValid():
         raise SqlError('Failed to add database. '
-            'Are sqlite and Qt sqlite support installed?')
+                       'Are sqlite and Qt sqlite support installed?',
+                       environmental=True)
     database.setDatabaseName(db_path)
     if not database.open():
         error = database.lastError()
-        _log_error(error)
-        raise SqlError("Failed to open sqlite database at {}: {}"
-                       .format(db_path, error.text()))
+        raise SqliteError("Failed to open sqlite database at {}: {}"
+                          .format(db_path, error.text()), error)
 
 
 def close():
@@ -66,28 +123,6 @@ def version():
         return 'UNAVAILABLE ({})'.format(e)
 
 
-def _log_error(error):
-    """Log informations about a SQL error to the debug log."""
-    log.sql.debug("SQL error:")
-    log.sql.debug("type: {}".format(debug.qenum_key(QSqlError, error.type())))
-    log.sql.debug("database text: {}".format(error.databaseText()))
-    log.sql.debug("driver text: {}".format(error.driverText()))
-    log.sql.debug("error code: {}".format(error.nativeErrorCode()))
-
-
-def _handle_query_error(what, query, error):
-    """Handle a sqlite error.
-
-    Arguments:
-        what: What we were doing when the error happened.
-        query: The query which was executed.
-        error: The QSqlError object.
-    """
-    _log_error(error)
-    msg = 'Failed to {} query "{}": "{}"'.format(what, query, error.text())
-    raise SqlError(msg)
-
-
 class Query(QSqlQuery):
 
     """A prepared SQL Query."""
@@ -103,7 +138,7 @@ class Query(QSqlQuery):
         super().__init__(QSqlDatabase.database())
         log.sql.debug('Preparing SQL query: "{}"'.format(querystr))
         if not self.prepare(querystr):
-            _handle_query_error('prepare', querystr, self.lastError())
+            raise SqliteError.from_query('prepare', querystr, self.lastError())
         self.setForwardOnly(forward_only)
 
     def __iter__(self):
@@ -124,7 +159,8 @@ class Query(QSqlQuery):
             self.bindValue(':{}'.format(key), val)
         log.sql.debug('query bindings: {}'.format(self.boundValues()))
         if not self.exec_():
-            _handle_query_error('exec', self.lastQuery(), self.lastError())
+            raise SqliteError.from_query('exec', self.lastQuery(),
+                                         self.lastError())
         return self
 
     def value(self):
@@ -250,7 +286,7 @@ class SqlTable(QObject):
         db = QSqlDatabase.database()
         db.transaction()
         if not q.execBatch():
-            _handle_query_error('exec', q.lastQuery(), q.lastError())
+            raise SqliteError.from_query('exec', q.lastQuery(), q.lastError())
         db.commit()
         self.changed.emit()
 
diff --git a/tests/unit/browser/test_history.py b/tests/unit/browser/test_history.py
index 1454e259b..1b706331a 100644
--- a/tests/unit/browser/test_history.py
+++ b/tests/unit/browser/test_history.py
@@ -27,6 +27,7 @@ from PyQt5.QtCore import QUrl
 from qutebrowser.browser import history
 from qutebrowser.utils import objreg, urlutils, usertypes
 from qutebrowser.commands import cmdexc
+from qutebrowser.misc import sql
 
 
 @pytest.fixture(autouse=True)
@@ -178,6 +179,28 @@ def test_add_url_invalid(qtbot, hist, caplog):
     assert not list(hist.completion)
 
 
+@pytest.mark.parametrize('environmental', [True, False])
+@pytest.mark.parametrize('completion', [True, False])
+def test_add_url_error(monkeypatch, hist, message_mock, caplog,
+                       environmental, completion):
+    def raise_error(url, replace=False):
+        raise sql.SqlError("Error message", environmental=environmental)
+
+    if completion:
+        monkeypatch.setattr(hist.completion, 'insert', raise_error)
+    else:
+        monkeypatch.setattr(hist, 'insert', raise_error)
+
+    if environmental:
+        with caplog.at_level(logging.ERROR):
+            hist.add_url(QUrl('https://www.example.org/'))
+        msg = message_mock.getmsg(usertypes.MessageLevel.error)
+        assert msg.text == "Failed to write history: Error message"
+    else:
+        with pytest.raises(sql.SqlError):
+            hist.add_url(QUrl('https://www.example.org/'))
+
+
 @pytest.mark.parametrize('level, url, req_url, expected', [
     (logging.DEBUG, 'a.com', 'a.com', [('a.com', 'title', 12345, False)]),
     (logging.DEBUG, 'a.com', 'b.com', [('a.com', 'title', 12345, False),
diff --git a/tests/unit/misc/test_sql.py b/tests/unit/misc/test_sql.py
index 953c8c498..fdd5c89c8 100644
--- a/tests/unit/misc/test_sql.py
+++ b/tests/unit/misc/test_sql.py
@@ -22,10 +22,63 @@
 import pytest
 from qutebrowser.misc import sql
 
+from PyQt5.QtSql import QSqlError
+
 
 pytestmark = pytest.mark.usefixtures('init_sql')
 
 
+def test_sqlerror():
+    text = "Hello World"
+    err = sql.SqlError(text, environmental=True)
+    assert str(err) == text
+    assert err.text() == text
+    assert err.environmental
+
+
+class TestSqliteError:
+
+    @pytest.mark.parametrize('error_code, environmental', [
+        ('9', True),  # SQLITE_LOCKED
+        ('19', False),  # SQLITE_CONSTRAINT
+    ])
+    def test_environmental(self, error_code, environmental):
+        sql_err = QSqlError("driver text", "db text", QSqlError.UnknownError,
+                            error_code)
+        err = sql.SqliteError("Message", sql_err)
+        assert err.environmental == environmental
+
+    def test_logging(self, caplog):
+        sql_err = QSqlError("driver text", "db text", QSqlError.UnknownError,
+                            '23')
+        sql.SqliteError("Message", sql_err)
+        lines = [r.message for r in caplog.records]
+        expected = ['SQL error:',
+                    'type: UnknownError',
+                    'database text: db text',
+                    'driver text: driver text',
+                    'error code: 23']
+
+        assert lines == expected
+
+    def test_from_query(self):
+        sql_err = QSqlError("driver text", "db text")
+        err = sql.SqliteError.from_query(
+            what='test', query='SELECT * from foo;', error=sql_err)
+        expected = ('Failed to test query "SELECT * from foo;": '
+                    '"db text driver text"')
+        assert str(err) == expected
+
+    def test_subclass(self):
+        with pytest.raises(sql.SqlError):
+            raise sql.SqliteError("text", QSqlError())
+
+    def test_text(self):
+        sql_err = QSqlError("driver text", "db text")
+        err = sql.SqliteError("Message", sql_err)
+        assert err.text() == "db text"
+
+
 def test_init():
     sql.SqlTable('Foo', ['name', 'val', 'lucky'])
     # should not error if table already exists