Use a dict instead of named params for insert.

This allows replace to be a named parameter and allows consolidating
some duplicate code between various insert methods.

This also fixes some tests that broke because batch insert was broken.

qutebrowser/browser/history.py

@@ -150,11 +150,15 @@ class WebHistory(sql.SqlTable):
 
         atime = int(atime) if (atime is not None) else int(time.time())
         url_str = url.toString(QUrl.FullyEncoded | QUrl.RemovePassword)
-        self.insert(url=url_str, title=title, atime=atime, redirect=redirect)
+        self.insert({'url': url_str,
+                     'title': title,
+                     'atime': atime,
+                     'redirect': redirect})
         if not redirect:
-            self.completion.insert_or_replace(url=url_str,
+            self.completion.insert({'url': url_str,
-                                              title=title,
+                                    'title': title,
-                                              last_atime=atime)
+                                    'last_atime': atime},
+                                    replace=True)
 
     def _parse_entry(self, line):
         """Parse a history line like '12345 http://example.com title'."""
@@ -183,10 +187,7 @@ class WebHistory(sql.SqlTable):
             raise ValueError("Invalid flags {!r}".format(flags))
 
         redirect = 'r' in flags
-        row = (url, title, float(atime), redirect)
+        return (url, title, int(atime), redirect)
-        completion_row = None if redirect else (url, title, float(atime))
-
-        return (row, completion_row)
 
     def import_txt(self):
         """Import a history text file into sqlite if it exists.
@@ -218,22 +219,27 @@ class WebHistory(sql.SqlTable):
     def _read(self, path):
         """Import a text file into the sql database."""
         with open(path, 'r', encoding='utf-8') as f:
-            rows = []
+            data = {'url': [], 'title': [], 'atime': [], 'redirect': []}
-            completion_rows = []
+            completion_data = {'url': [], 'title': [], 'last_atime': []}
             for (i, line) in enumerate(f):
                 line = line.strip()
                 if not line:
                     continue
                 try:
-                    row, completion_row = self._parse_entry(line.strip())
+                    url, title, atime, redirect = self._parse_entry(line)
-                    rows.append(row)
+                    data['url'].append(url)
-                    if completion_row is not None:
+                    data['title'].append(title)
-                        completion_rows.append(completion_row)
+                    data['atime'].append(atime)
+                    data['redirect'].append(redirect)
+                    if not redirect:
+                        completion_data['url'].append(url)
+                        completion_data['title'].append(title)
+                        completion_data['last_atime'].append(atime)
                 except ValueError as ex:
                     raise ValueError('Failed to parse line #{} of {}: "{}"'
                                      .format(i, path, ex))
-        self.insert_batch(rows)
+        self.insert_batch(data)
-        self.completion.insert_batch(completion_rows, replace=True)
+        self.completion.insert_batch(completion_data, replace=True)
 
     @cmdutils.register(instance='web-history', debug=True)
     def debug_dump_history(self, dest):

qutebrowser/misc/sql.py

@@ -184,44 +184,32 @@ class SqlTable(QObject):
             raise KeyError('No row with {} = "{}"'.format(field, value))
         self.changed.emit()
 
-    def insert(self, **values):
+    def _insert_query(self, values, replace):
+        params = ','.join(':{}'.format(key) for key in values)
+        verb = "REPLACE" if replace else "INSERT"
+        return Query("{} INTO {} values({})".format(verb, self._name, params))
+
+    def insert(self, values, replace=False):
         """Append a row to the table.
 
         Args:
-            values: A list of values to insert.
+            values: A dict with a value to insert for each field name.
             replace: If set, replace existing values.
         """
-        paramstr = ','.join(':{}'.format(key) for key in values)
+        q = self._insert_query(values, replace)
-        q = Query("INSERT INTO {} values({})".format(self._name, paramstr))
         q.run(**values)
         self.changed.emit()
 
-    def insert_or_replace(self, **values):
+    def insert_batch(self, values, replace=False):
-        """Append a row to the table.
-
-        Args:
-            values: A list of values to insert.
-            replace: If set, replace existing values.
-        """
-        paramstr = ','.join(':{}'.format(key) for key in values)
-        q = Query("REPLACE INTO {} values({})".format(self._name, paramstr))
-        q.run(**values)
-        self.changed.emit()
-
-    def insert_batch(self, rows, replace=False):
         """Performantly append multiple rows to the table.
 
         Args:
             rows: A list of lists, where each sub-list is a row.
-            replace: If set, replace existing values.
+            values: A dict with a list of values to insert for each field name.
         """
-        paramstr = ','.join(['?'] * len(rows[0]))
+        q = self._insert_query(values, replace)
-        q = Query("INSERT {} INTO {} values({})".format(
+        for key, val in values.items():
-            'OR REPLACE' if replace else '', self._name, paramstr))
+            q.bindValue(':{}'.format(key), val)
-
-        transposed = [list(row) for row in zip(*rows)]
-        for val in transposed:
-            q.addBindValue(val)
 
         db = QSqlDatabase.database()
         db.transaction()

tests/unit/browser/test_qutescheme.py

@@ -133,14 +133,13 @@ class TestHistoryHandler:
             assert item['time'] > end_time * 1000
 
     def test_qute_history_benchmark(self, fake_web_history, benchmark, now):
-        entries = []
+        r = range(100000)
-        for t in range(100000):  # one history per second
+        entries = {
-            entry = fake_web_history.Entry(
+            'atime': [int(now - t) for t in r],
-                atime=str(now - t),
+            'url': ['www.x.com/{}'.format(t) for t in r],
-                url=QUrl('www.x.com/{}'.format(t)),
+            'title': ['x at {}'.format(t) for t in r],
-                title='x at {}'.format(t),
+            'redirect': [False for _ in r],
-                redirect=False)
+        }
-            entries.append(entry)
 
         fake_web_history.insert_batch(entries)
         url = QUrl("qute://history/data?start_time={}".format(now))

tests/unit/completion/test_models.py

@@ -161,15 +161,15 @@ def web_history_stub(stubs, init_sql):
 @pytest.fixture
 def web_history(web_history_stub, init_sql):
     """Pre-populate the web-history database."""
-    web_history_stub.insert(url='http://qutebrowser.org',
+    web_history_stub.insert({'url': 'http://qutebrowser.org',
-                            title='qutebrowser',
+                            'title': 'qutebrowser',
-                            last_atime=datetime(2015, 9, 5).timestamp())
+                            'last_atime': datetime(2015, 9, 5).timestamp()})
-    web_history_stub.insert(url='https://python.org',
+    web_history_stub.insert({'url': 'https://python.org',
-                            title='Welcome to Python.org',
+                            'title': 'Welcome to Python.org',
-                            last_atime=datetime(2016, 3, 8).timestamp())
+                            'last_atime': datetime(2016, 3, 8).timestamp()})
-    web_history_stub.insert(url='https://github.com',
+    web_history_stub.insert({'url': 'https://github.com',
-                            title='https://github.com',
+                            'title': 'https://github.com',
-                            last_atime=datetime(2016, 5, 1).timestamp())
+                            'last_atime': datetime(2016, 5, 1).timestamp()})
     return web_history_stub
 
 
@@ -331,7 +331,7 @@ def test_url_completion_pattern(config_stub, web_history_stub,
                                 url, title, pattern, rowcount):
     """Test that url completion filters by url and title."""
     config_stub.data['completion'] = {'timestamp-format': '%Y-%m-%d'}
-    web_history_stub.insert(url=url, title=title, last_atime=0)
+    web_history_stub.insert({'url': url, 'title': title, 'last_atime': 0})
     model = urlmodel.url()
     model.set_pattern(pattern)
     # 2, 0 is History
@@ -576,21 +576,22 @@ def test_url_completion_benchmark(benchmark, config_stub,
     config_stub.data['completion'] = {'timestamp-format': '%Y-%m-%d',
                                       'web-history-max-items': 1000}
 
-    entries = [web_history_stub.Entry(
+    r = range(100000)
-        last_atime=i,
+    entries = {
-        url='http://example.com/{}'.format(i),
+        'last_atime': list(r),
-        title='title{}'.format(i))
+        'url': ['http://example.com/{}'.format(i) for i in r],
-        for i in range(100000)]
+        'title': ['title{}'.format(i) for i in r]
+    }
 
     web_history_stub.insert_batch(entries)
 
-    quickmark_manager_stub.marks = collections.OrderedDict(
+    quickmark_manager_stub.marks = collections.OrderedDict([
-        (e.title, e.url)
+        ('title{}'.format(i), 'example.com/{}'.format(i))
-        for e in entries[0:1000])
+        for i in range(1000)])
 
-    bookmark_manager_stub.marks = collections.OrderedDict(
+    bookmark_manager_stub.marks = collections.OrderedDict([
-        (e.url, e.title)
+        ('example.com/{}'.format(i), 'title{}'.format(i))
-        for e in entries[0:1000])
+        for i in range(1000)])
 
     def bench():
         model = urlmodel.url()

tests/unit/misc/test_sql.py

@@ -35,26 +35,67 @@ def test_init():
 def test_insert(qtbot):
     table = sql.SqlTable('Foo', ['name', 'val', 'lucky'])
     with qtbot.waitSignal(table.changed):
-        table.insert(name='one', val=1, lucky=False)
+        table.insert({'name': 'one', 'val': 1, 'lucky': False})
     with qtbot.waitSignal(table.changed):
-        table.insert(name='wan', val=1, lucky=False)
+        table.insert({'name': 'wan', 'val': 1, 'lucky': False})
 
 
-def test_insert_or_replace(qtbot):
+def test_insert_replace(qtbot):
     table = sql.SqlTable('Foo', ['name', 'val', 'lucky'],
                          constraints={'name': 'PRIMARY KEY'})
     with qtbot.waitSignal(table.changed):
-        table.insert_or_replace(name='one', val=1, lucky=False)
+        table.insert({'name': 'one', 'val': 1, 'lucky': False}, replace=True)
     with qtbot.waitSignal(table.changed):
-        table.insert_or_replace(name='one', val=11, lucky=True)
+        table.insert({'name': 'one', 'val': 11, 'lucky': True}, replace=True)
     assert list(table) == [('one', 11, True)]
 
+    with pytest.raises(sql.SqlException):
+        table.insert({'name': 'one', 'val': 11, 'lucky': True}, replace=False)
+
+
+def test_insert_batch(qtbot):
+    table = sql.SqlTable('Foo', ['name', 'val', 'lucky'])
+
+    with qtbot.waitSignal(table.changed):
+        table.insert_batch({'name': ['one', 'nine', 'thirteen'],
+                            'val': [1, 9, 13],
+                            'lucky': [False, False, True]})
+
+    assert list(table) == [('one', 1, False),
+                           ('nine', 9, False),
+                           ('thirteen', 13, True)]
+
+
+def test_insert_batch_replace(qtbot):
+    table = sql.SqlTable('Foo', ['name', 'val', 'lucky'],
+                         constraints={'name': 'PRIMARY KEY'})
+
+    with qtbot.waitSignal(table.changed):
+        table.insert_batch({'name': ['one', 'nine', 'thirteen'],
+                            'val': [1, 9, 13],
+                            'lucky': [False, False, True]})
+
+    with qtbot.waitSignal(table.changed):
+        table.insert_batch({'name': ['one', 'nine'],
+                            'val': [11, 19],
+                            'lucky': [True, True]},
+                            replace=True)
+
+    assert list(table) == [('thirteen', 13, True),
+                           ('one', 11, True),
+                           ('nine', 19, True)]
+
+    with pytest.raises(sql.SqlException):
+        table.insert_batch({'name': ['one', 'nine'],
+                            'val': [11, 19],
+                            'lucky': [True, True]})
+
 
 def test_iter():
     table = sql.SqlTable('Foo', ['name', 'val', 'lucky'])
-    table.insert(name='one', val=1, lucky=False)
+    table.insert({'name': 'one', 'val': 1, 'lucky': False})
-    table.insert(name='nine', val=9, lucky=False)
+    table.insert({'name': 'nine', 'val': 9, 'lucky': False})
-    table.insert(name='thirteen', val=13, lucky=True)
+    table.insert({'name': 'thirteen', 'val': 13, 'lucky': True})
     assert list(table) == [('one', 1, False),
                            ('nine', 9, False),
                            ('thirteen', 13, True)]
@@ -73,15 +114,15 @@ def test_iter():
 def test_select(rows, sort_by, sort_order, limit, result):
     table = sql.SqlTable('Foo', ['a', 'b'])
     for row in rows:
-        table.insert(**row)
+        table.insert(row)
     assert list(table.select(sort_by, sort_order, limit)) == result
 
 
 def test_delete(qtbot):
     table = sql.SqlTable('Foo', ['name', 'val', 'lucky'])
-    table.insert(name='one', val=1, lucky=False)
+    table.insert({'name': 'one', 'val': 1, 'lucky': False})
-    table.insert(name='nine', val=9, lucky=False)
+    table.insert({'name': 'nine', 'val': 9, 'lucky': False})
-    table.insert(name='thirteen', val=13, lucky=True)
+    table.insert({'name': 'thirteen', 'val': 13, 'lucky': True})
     with pytest.raises(KeyError):
         table.delete('nope', 'name')
     with qtbot.waitSignal(table.changed):
@@ -95,19 +136,19 @@ def test_delete(qtbot):
 def test_len():
     table = sql.SqlTable('Foo', ['name', 'val', 'lucky'])
     assert len(table) == 0
-    table.insert(name='one', val=1, lucky=False)
+    table.insert({'name': 'one', 'val': 1, 'lucky': False})
     assert len(table) == 1
-    table.insert(name='nine', val=9, lucky=False)
+    table.insert({'name': 'nine', 'val': 9, 'lucky': False})
     assert len(table) == 2
-    table.insert(name='thirteen', val=13, lucky=True)
+    table.insert({'name': 'thirteen', 'val': 13, 'lucky': True})
     assert len(table) == 3
 
 
 def test_contains():
     table = sql.SqlTable('Foo', ['name', 'val', 'lucky'])
-    table.insert(name='one', val=1, lucky=False)
+    table.insert({'name': 'one', 'val': 1, 'lucky': False})
-    table.insert(name='nine', val=9, lucky=False)
+    table.insert({'name': 'nine', 'val': 9, 'lucky': False})
-    table.insert(name='thirteen', val=13, lucky=True)
+    table.insert({'name': 'thirteen', 'val': 13, 'lucky': True})
 
     name_query = table.contains_query('name')
     val_query = table.contains_query('val')
@@ -126,9 +167,9 @@ def test_contains():
 
 def test_delete_all(qtbot):
     table = sql.SqlTable('Foo', ['name', 'val', 'lucky'])
-    table.insert(name='one', val=1, lucky=False)
+    table.insert({'name': 'one', 'val': 1, 'lucky': False})
-    table.insert(name='nine', val=9, lucky=False)
+    table.insert({'name': 'nine', 'val': 9, 'lucky': False})
-    table.insert(name='thirteen', val=13, lucky=True)
+    table.insert({'name': 'thirteen', 'val': 13, 'lucky': True})
     with qtbot.waitSignal(table.changed):
         table.delete_all()
     assert list(table) == []