From 8ff45331df32e35c214662056f8b5d72e789b8eb Mon Sep 17 00:00:00 2001 From: Ryan Roden-Corrent Date: Tue, 4 Apr 2017 08:27:42 -0400 Subject: [PATCH] Clean up sql implementation. Now that sql is only used for history (not quickmarks/bookmarks) a number of functions are no longer needed. In addition, primary key support was removed as we actually need to support multiple entries for the same url with different access times. The completion model will have to handle this by selecting something like (url, title, max(atime)). This also fixes up a number of tests that were broken with the last few sql-related commits. --- qutebrowser/app.py | 2 +- qutebrowser/browser/history.py | 4 +- qutebrowser/misc/sql.py | 45 ++++-------------- qutebrowser/utils/version.py | 3 +- tests/end2end/features/test_history_bdd.py | 38 +++++++-------- tests/unit/browser/test_qutescheme.py | 3 +- tests/unit/browser/webkit/test_history.py | 19 +------- tests/unit/completion/test_models.py | 3 +- tests/unit/completion/test_sqlcategory.py | 10 ++-- tests/unit/misc/test_sql.py | 55 ++++------------------ 10 files changed, 49 insertions(+), 133 deletions(-) diff --git a/qutebrowser/app.py b/qutebrowser/app.py index e2e2d5107..ccb474e69 100644 --- a/qutebrowser/app.py +++ b/qutebrowser/app.py @@ -481,7 +481,7 @@ def _import_history(): In older versions of qutebrowser, history was stored in a text format. This converts that file into the new sqlite format and removes it. - """ + """ path = os.path.join(standarddir.data(), 'history') if not os.path.isfile(path): return diff --git a/qutebrowser/browser/history.py b/qutebrowser/browser/history.py index 8d10cdbbe..29e23750b 100644 --- a/qutebrowser/browser/history.py +++ b/qutebrowser/browser/history.py @@ -97,7 +97,7 @@ class WebHistory(sql.SqlTable): def __init__(self, parent=None): super().__init__("History", ['url', 'title', 'atime', 'redirect'], - primary_key='url', parent=parent) + parent=parent) def __repr__(self): return utils.get_repr(self, length=len(self)) @@ -105,7 +105,7 @@ class WebHistory(sql.SqlTable): def _add_entry(self, entry): """Add an entry to the in-memory database.""" self.insert([entry.url_str(), entry.title, entry.atime, - entry.redirect], replace=True) + entry.redirect]) def get_recent(self): """Get the most recent history entries.""" diff --git a/qutebrowser/misc/sql.py b/qutebrowser/misc/sql.py index d1d478302..707e0c4ce 100644 --- a/qutebrowser/misc/sql.py +++ b/qutebrowser/misc/sql.py @@ -101,6 +101,7 @@ def run_batch(querystr, values): return query + class SqlTable(QObject): """Interface to a sql table. @@ -108,7 +109,6 @@ class SqlTable(QObject): Attributes: Entry: The class wrapping row data from this table. _name: Name of the SQL table this wraps. - _primary_key: The primary key of the table. Signals: changed: Emitted when the table is modified. @@ -116,7 +116,7 @@ class SqlTable(QObject): changed = pyqtSignal() - def __init__(self, name, fields, primary_key, parent=None): + def __init__(self, name, fields, parent=None): """Create a new table in the sql database. Raises SqlException if the table already exists. @@ -124,11 +124,9 @@ class SqlTable(QObject): Args: name: Name of the table. fields: A list of field names. - primary_key: Name of the field to serve as the primary key. """ super().__init__(parent) self._name = name - self._primary_key = primary_key run_query("CREATE TABLE IF NOT EXISTS {} ({})" .format(name, ','.join(fields))) # pylint: disable=invalid-name @@ -141,74 +139,47 @@ class SqlTable(QObject): rec = result.record() yield self.Entry(*[rec.value(i) for i in range(rec.count())]) - def __contains__(self, key): - """Return whether the table contains the matching item. - - Args: - key: Primary key value to search for. - """ - query = run_query("SELECT * FROM {} where {} = ?".format( - self._name, self._primary_key), [key]) - return query.next() - def __len__(self): """Return the count of rows in the table.""" result = run_query("SELECT count(*) FROM {}".format(self._name)) result.next() return result.value(0) - def __getitem__(self, key): - """Retrieve the row matching the given key. - - Args: - key: Primary key value to fetch. - """ - result = run_query("SELECT * FROM {} where {} = ?".format( - self._name, self._primary_key), [key]) - result.next() - rec = result.record() - return self.Entry(*[rec.value(i) for i in range(rec.count())]) - - def delete(self, value, field=None): + def delete(self, value, field): """Remove all rows for which `field` equals `value`. Args: value: Key value to delete. - field: Field to use as the key, defaults to the primary key. + field: Field to use as the key. Return: The number of rows deleted. """ - field = field or self._primary_key query = run_query("DELETE FROM {} where {} = ?".format( self._name, field), [value]) if not query.numRowsAffected(): raise KeyError('No row with {} = "{}"'.format(field, value)) self.changed.emit() - def insert(self, values, replace=False): + def insert(self, values): """Append a row to the table. Args: values: A list of values to insert. - replace: If true, allow inserting over an existing primary key. """ - cmd = "REPLACE" if replace else "INSERT" paramstr = ','.join(['?'] * len(values)) - run_query("{} INTO {} values({})".format(cmd, self._name, paramstr), + run_query("INSERT INTO {} values({})".format(self._name, paramstr), values) self.changed.emit() - def insert_batch(self, rows, replace=False): + def insert_batch(self, rows): """Performantly append multiple rows to the table. Args: rows: A list of lists, where each sub-list is a row. - replace: If true, allow inserting over an existing primary key. """ - cmd = "REPLACE" if replace else "INSERT" paramstr = ','.join(['?'] * len(rows[0])) - run_batch("{} INTO {} values({})".format(cmd, self._name, paramstr), + run_batch("INSERT INTO {} values({})".format(self._name, paramstr), rows) self.changed.emit() diff --git a/qutebrowser/utils/version.py b/qutebrowser/utils/version.py index ac74f997a..faa890152 100644 --- a/qutebrowser/utils/version.py +++ b/qutebrowser/utils/version.py @@ -327,7 +327,8 @@ def version(): lines += ['pdf.js: {}'.format(_pdfjs_version())] - sql.init() + # we can use an in-memory database as we just want to query the version + sql.init('') lines += ['sqlite: {}'.format(sql.version())] sql.close() diff --git a/tests/end2end/features/test_history_bdd.py b/tests/end2end/features/test_history_bdd.py index 1fee533eb..25995e9d0 100644 --- a/tests/end2end/features/test_history_bdd.py +++ b/tests/end2end/features/test_history_bdd.py @@ -20,31 +20,29 @@ import os.path import pytest_bdd as bdd + +from PyQt5.QtSql import QSqlDatabase, QSqlQuery + bdd.scenarios('history.feature') @bdd.then(bdd.parsers.parse("the history file should contain:\n{expected}")) def check_history(quteproc, httpbin, expected): - history_file = os.path.join(quteproc.basedir, 'data', 'history') - quteproc.send_cmd(':save history') - quteproc.wait_for(message=':save saved history') - - expected = expected.replace('(port)', str(httpbin.port)).splitlines() - - with open(history_file, 'r', encoding='utf-8') as f: - lines = [] - for line in f: - if not line.strip(): - continue - print('history line: ' + line) - atime, line = line.split(' ', maxsplit=1) - line = line.rstrip() - if '-' in atime: - flags = atime.split('-')[1] - line = '{} {}'.format(flags, line) - lines.append(line) - - assert lines == expected + path = os.path.join(quteproc.basedir, 'data', 'history.sqlite') + db = QSqlDatabase.addDatabase('QSQLITE') + db.setDatabaseName(path) + assert db.open(), 'Failed to open history database' + query = db.exec_('select * from History') + actual = [] + while query.next(): + rec = query.record() + url = rec.value(0) + title = rec.value(1) + redirect = rec.value(3) + actual.append('{} {} {}'.format('r' * redirect, url, title).strip()) + db = None + QSqlDatabase.removeDatabase(QSqlDatabase.database().connectionName()) + assert actual == expected.replace('(port)', str(httpbin.port)).splitlines() @bdd.then("the history file should be empty") diff --git a/tests/unit/browser/test_qutescheme.py b/tests/unit/browser/test_qutescheme.py index d5e60efab..86a322322 100644 --- a/tests/unit/browser/test_qutescheme.py +++ b/tests/unit/browser/test_qutescheme.py @@ -98,7 +98,7 @@ class TestHistoryHandler: @pytest.fixture def fake_web_history(self, fake_save_manager, tmpdir, init_sql): """Create a fake web-history and register it into objreg.""" - web_history = history.WebHistory(tmpdir.dirname, 'fake-history') + web_history = history.WebHistory() objreg.register('web-history', web_history) yield web_history objreg.delete('web-history') @@ -108,7 +108,6 @@ class TestHistoryHandler: """Create fake history.""" for item in entries: fake_web_history._add_entry(item) - fake_web_history.save() @pytest.mark.parametrize("start_time_offset, expected_item_count", [ (0, 4), diff --git a/tests/unit/browser/webkit/test_history.py b/tests/unit/browser/webkit/test_history.py index e86950a82..50c632f7d 100644 --- a/tests/unit/browser/webkit/test_history.py +++ b/tests/unit/browser/webkit/test_history.py @@ -65,14 +65,6 @@ def test_len(hist): assert len(hist) == 1 -def test_updated_entries(tmpdir, hist): - hist.add_url(QUrl('http://example.com/'), atime=67890) - assert list(hist) == [('http://example.com/', '', 67890, False)] - - hist.add_url(QUrl('http://example.com/'), atime=99999) - assert list(hist) == [('http://example.com/', '', 99999, False)] - - def test_get_recent(hist): hist.add_url(QUrl('http://www.qutebrowser.org/'), atime=67890) hist.add_url(QUrl('http://example.com/'), atime=12345) @@ -117,7 +109,7 @@ def test_clear_force(qtbot, tmpdir, hist): def test_add_item(qtbot, hist, item): (url, atime, title, redirect) = item hist.add_url(QUrl(url), atime=atime, title=title, redirect=redirect) - assert hist[url] == (url, title, atime, redirect) + assert list(hist) == [(url, title, atime, redirect)] def test_add_item_invalid(qtbot, hist, caplog): @@ -139,15 +131,6 @@ def test_add_from_tab(hist, level, url, req_url, expected, mock_time, caplog): assert set(list(hist)) == set(expected) -def test_add_item_redirect_update(qtbot, tmpdir, hist): - """A redirect update added should override a non-redirect one.""" - url = 'http://www.example.com/' - hist.add_url(QUrl(url), atime=5555) - hist.add_url(QUrl(url), redirect=True, atime=67890) - - assert hist[url] == (url, '', 67890, True) - - @pytest.fixture def hist_interface(): # pylint: disable=invalid-name diff --git a/tests/unit/completion/test_models.py b/tests/unit/completion/test_models.py index 9bc6fb289..e071f1584 100644 --- a/tests/unit/completion/test_models.py +++ b/tests/unit/completion/test_models.py @@ -157,8 +157,7 @@ def bookmarks(bookmark_manager_stub): @pytest.fixture def web_history(stubs, init_sql): """Pre-populate the web-history database.""" - table = sql.SqlTable("History", ['url', 'title', 'atime', 'redirect'], - primary_key='url') + table = sql.SqlTable("History", ['url', 'title', 'atime', 'redirect']) table.insert(['http://some-redirect.example.com', 'redirect', datetime(2016, 9, 5).timestamp(), True]) table.insert(['http://qutebrowser.org', 'qutebrowser', diff --git a/tests/unit/completion/test_sqlcategory.py b/tests/unit/completion/test_sqlcategory.py index 1d25c5954..03288f016 100644 --- a/tests/unit/completion/test_sqlcategory.py +++ b/tests/unit/completion/test_sqlcategory.py @@ -71,7 +71,7 @@ def _validate(cat, expected): [('B', 'C', 2), ('C', 'A', 1), ('A', 'F', 0)]), ]) def test_sorting(sort_by, sort_order, data, expected): - table = sql.SqlTable('Foo', ['a', 'b', 'c'], primary_key='a') + table = sql.SqlTable('Foo', ['a', 'b', 'c']) for row in data: table.insert(row) cat = sqlcategory.SqlCategory('Foo', sort_by=sort_by, @@ -126,7 +126,7 @@ def test_sorting(sort_by, sort_order, data, expected): ]) def test_set_pattern(pattern, filter_cols, before, after): """Validate the filtering and sorting results of set_pattern.""" - table = sql.SqlTable('Foo', ['a', 'b', 'c'], primary_key='a') + table = sql.SqlTable('Foo', ['a', 'b', 'c']) for row in before: table.insert(row) cat = sqlcategory.SqlCategory('Foo') @@ -135,14 +135,14 @@ def test_set_pattern(pattern, filter_cols, before, after): def test_select(): - table = sql.SqlTable('Foo', ['a', 'b', 'c'], primary_key='a') + table = sql.SqlTable('Foo', ['a', 'b', 'c']) table.insert(['foo', 'bar', 'baz']) cat = sqlcategory.SqlCategory('Foo', select='b, c, a') _validate(cat, [('bar', 'baz', 'foo')]) def test_where(): - table = sql.SqlTable('Foo', ['a', 'b', 'c'], primary_key='a') + table = sql.SqlTable('Foo', ['a', 'b', 'c']) table.insert(['foo', 'bar', False]) table.insert(['baz', 'biz', True]) cat = sqlcategory.SqlCategory('Foo', where='not c') @@ -150,7 +150,7 @@ def test_where(): def test_entry(): - table = sql.SqlTable('Foo', ['a', 'b', 'c'], primary_key='a') + table = sql.SqlTable('Foo', ['a', 'b', 'c']) assert hasattr(table.Entry, 'a') assert hasattr(table.Entry, 'b') assert hasattr(table.Entry, 'c') diff --git a/tests/unit/misc/test_sql.py b/tests/unit/misc/test_sql.py index 4221bb022..387b5ce46 100644 --- a/tests/unit/misc/test_sql.py +++ b/tests/unit/misc/test_sql.py @@ -27,24 +27,21 @@ pytestmark = pytest.mark.usefixtures('init_sql') def test_init(): - sql.SqlTable('Foo', ['name', 'val', 'lucky'], primary_key='name') + sql.SqlTable('Foo', ['name', 'val', 'lucky']) # should not error if table already exists - sql.SqlTable('Foo', ['name', 'val', 'lucky'], primary_key='name') + sql.SqlTable('Foo', ['name', 'val', 'lucky']) def test_insert(qtbot): - table = sql.SqlTable('Foo', ['name', 'val', 'lucky'], primary_key='name') + table = sql.SqlTable('Foo', ['name', 'val', 'lucky']) with qtbot.waitSignal(table.changed): table.insert(['one', 1, False]) with qtbot.waitSignal(table.changed): table.insert(['wan', 1, False]) - with pytest.raises(sql.SqlException): - # duplicate primary key - table.insert(['one', 1, False]) def test_iter(): - table = sql.SqlTable('Foo', ['name', 'val', 'lucky'], primary_key='name') + table = sql.SqlTable('Foo', ['name', 'val', 'lucky']) table.insert(['one', 1, False]) table.insert(['nine', 9, False]) table.insert(['thirteen', 13, True]) @@ -59,29 +56,21 @@ def test_iter(): ([[2, 5], [1, 6], [3, 4]], 'b', 'desc', 2, [(1, 6), (2, 5)]) ]) def test_select(rows, sort_by, sort_order, limit, result): - table = sql.SqlTable('Foo', ['a', 'b'], primary_key='a') + table = sql.SqlTable('Foo', ['a', 'b']) for row in rows: table.insert(row) assert list(table.select(sort_by, sort_order, limit)) == result -def test_replace(qtbot): - table = sql.SqlTable('Foo', ['name', 'val', 'lucky'], primary_key='name') - table.insert(['one', 1, False]) - with qtbot.waitSignal(table.changed): - table.insert(['one', 1, True], replace=True) - assert list(table) == [('one', 1, True)] - - def test_delete(qtbot): - table = sql.SqlTable('Foo', ['name', 'val', 'lucky'], primary_key='name') + table = sql.SqlTable('Foo', ['name', 'val', 'lucky']) table.insert(['one', 1, False]) table.insert(['nine', 9, False]) table.insert(['thirteen', 13, True]) with pytest.raises(KeyError): - table.delete('nope') + table.delete('nope', 'name') with qtbot.waitSignal(table.changed): - table.delete('thirteen') + table.delete('thirteen', 'name') assert list(table) == [('one', 1, False), ('nine', 9, False)] with qtbot.waitSignal(table.changed): table.delete(False, field='lucky') @@ -89,7 +78,7 @@ def test_delete(qtbot): def test_len(): - table = sql.SqlTable('Foo', ['name', 'val', 'lucky'], primary_key='name') + table = sql.SqlTable('Foo', ['name', 'val', 'lucky']) assert len(table) == 0 table.insert(['one', 1, False]) assert len(table) == 1 @@ -99,32 +88,8 @@ def test_len(): assert len(table) == 3 -def test_contains(): - table = sql.SqlTable('Foo', ['name', 'val', 'lucky'], primary_key='name') - table.insert(['one', 1, False]) - table.insert(['nine', 9, False]) - table.insert(['thirteen', 13, True]) - assert 'oone' not in table - assert 'ninee' not in table - assert 1 not in table - assert '*' not in table - assert 'one' in table - assert 'nine' in table - assert 'thirteen' in table - - -def test_getitem(): - table = sql.SqlTable('Foo', ['name', 'val', 'lucky'], primary_key='name') - table.insert(['one', 1, False]) - table.insert(['nine', 9, False]) - table.insert(['thirteen', 13, True]) - assert table['one'] == ('one', 1, False) - assert table['nine'] == ('nine', 9, False) - assert table['thirteen'] == ('thirteen', 13, True) - - def test_delete_all(qtbot): - table = sql.SqlTable('Foo', ['name', 'val', 'lucky'], primary_key='name') + table = sql.SqlTable('Foo', ['name', 'val', 'lucky']) table.insert(['one', 1, False]) table.insert(['nine', 9, False]) table.insert(['thirteen', 13, True])