From 1603b15cfd3981f67ec1275ce101a7f32b434ffe Mon Sep 17 00:00:00 2001 From: Florian Bruhin Date: Tue, 3 Oct 2017 07:49:27 +0200 Subject: [PATCH 1/4] Default to NOT NULL for table constraints Ideally, we'd update all existing tables to add the new constraints, but sqlite doesn't offer an easy way to do so: https://www.sqlite.org/lang_altertable.html Since that migration really isn't worth the effort, we only set the constraint for new tables... --- qutebrowser/misc/sql.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/qutebrowser/misc/sql.py b/qutebrowser/misc/sql.py index 375d0464d..b4829bd50 100644 --- a/qutebrowser/misc/sql.py +++ b/qutebrowser/misc/sql.py @@ -161,7 +161,8 @@ class SqlTable(QObject): self._name = name constraints = constraints or {} - column_defs = ['{} {}'.format(field, constraints.get(field, '')) + default = 'NOT NULL' + column_defs = ['{} {}'.format(field, constraints.get(field, default)) for field in fields] q = Query("CREATE TABLE IF NOT EXISTS {name} ({column_defs})" .format(name=name, column_defs=', '.join(column_defs))) From 31f49afdb2ee06b4c3d610c73ed89a3bc8ed4b64 Mon Sep 17 00:00:00 2001 From: Florian Bruhin Date: Tue, 3 Oct 2017 07:50:21 +0200 Subject: [PATCH 2/4] Fix incorrect docstring --- qutebrowser/misc/sql.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/qutebrowser/misc/sql.py b/qutebrowser/misc/sql.py index b4829bd50..bfb1d1e8b 100644 --- a/qutebrowser/misc/sql.py +++ b/qutebrowser/misc/sql.py @@ -150,7 +150,7 @@ class SqlTable(QObject): def __init__(self, name, fields, constraints=None, parent=None): """Create a new table in the sql database. - Raises SqlError if the table already exists. + Does nothing if the table already exists. Args: name: Name of the table. From 3772084cbf0c6f2319630b80070c728588ef5d96 Mon Sep 17 00:00:00 2001 From: Florian Bruhin Date: Tue, 3 Oct 2017 10:26:55 +0200 Subject: [PATCH 3/4] Adjust test_histcategory for NOT NULL constraints --- tests/unit/completion/test_histcategory.py | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/tests/unit/completion/test_histcategory.py b/tests/unit/completion/test_histcategory.py index 8ae3bb1f4..c093513cb 100644 --- a/tests/unit/completion/test_histcategory.py +++ b/tests/unit/completion/test_histcategory.py @@ -140,20 +140,24 @@ def test_sorting(max_items, before, after, model_validator, hist, config_stub): def test_remove_rows(hist, model_validator): - hist.insert({'url': 'foo', 'title': 'Foo'}) - hist.insert({'url': 'bar', 'title': 'Bar'}) + hist.insert({'url': 'foo', 'title': 'Foo', 'last_atime': 0}) + hist.insert({'url': 'bar', 'title': 'Bar', 'last_atime': 0}) cat = histcategory.HistoryCategory() model_validator.set_model(cat) cat.set_pattern('') hist.delete('url', 'foo') cat.removeRows(0, 1) - model_validator.validate([('bar', 'Bar', '')]) + model_validator.validate([('bar', 'Bar', '1970-01-01')]) def test_remove_rows_fetch(hist): """removeRows should fetch enough data to make the current index valid.""" # we cannot use model_validator as it will fetch everything up front - hist.insert_batch({'url': [str(i) for i in range(300)]}) + hist.insert_batch({ + 'url': [str(i) for i in range(300)], + 'title': [str(i) for i in range(300)], + 'last_atime': [0] * 300, + }) cat = histcategory.HistoryCategory() cat.set_pattern('') From 7f28097f558ccca779d402850ed9f7c8358bca15 Mon Sep 17 00:00:00 2001 From: Florian Bruhin Date: Tue, 3 Oct 2017 22:17:29 +0200 Subject: [PATCH 4/4] Be explicit about constraints instead --- qutebrowser/browser/history.py | 9 ++++++++- qutebrowser/misc/sql.py | 3 +-- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/qutebrowser/browser/history.py b/qutebrowser/browser/history.py index ca62d9e6c..2c4c1f226 100644 --- a/qutebrowser/browser/history.py +++ b/qutebrowser/browser/history.py @@ -40,7 +40,10 @@ class CompletionHistory(sql.SqlTable): def __init__(self, parent=None): super().__init__("CompletionHistory", ['url', 'title', 'last_atime'], - constraints={'url': 'PRIMARY KEY'}, parent=parent) + constraints={'url': 'PRIMARY KEY', + 'title': 'NOT NULL', + 'last_atime': 'NOT NULL'}, + parent=parent) self.create_index('CompletionHistoryAtimeIndex', 'last_atime') @@ -50,6 +53,10 @@ class WebHistory(sql.SqlTable): def __init__(self, parent=None): super().__init__("History", ['url', 'title', 'atime', 'redirect'], + constraints={'url': 'NOT NULL', + 'title': 'NOT NULL', + 'atime': 'NOT NULL', + 'redirect': 'NOT NULL'}, parent=parent) self.completion = CompletionHistory(parent=self) if sql.Query('pragma user_version').run().value() < _USER_VERSION: diff --git a/qutebrowser/misc/sql.py b/qutebrowser/misc/sql.py index bfb1d1e8b..24e2035e6 100644 --- a/qutebrowser/misc/sql.py +++ b/qutebrowser/misc/sql.py @@ -161,8 +161,7 @@ class SqlTable(QObject): self._name = name constraints = constraints or {} - default = 'NOT NULL' - column_defs = ['{} {}'.format(field, constraints.get(field, default)) + column_defs = ['{} {}'.format(field, constraints.get(field, '')) for field in fields] q = Query("CREATE TABLE IF NOT EXISTS {name} ({column_defs})" .format(name=name, column_defs=', '.join(column_defs)))