From 1cc5161089e559c8b16049b20f7a5b3a43290c21 Mon Sep 17 00:00:00 2001 From: Russ Garrett Date: Sat, 14 Apr 2018 13:06:00 +0100 Subject: [PATCH 1/4] Fix sqlite error when loading rows with no incoming FKs This fixes `ERROR: conn=, sql = 'select ', params = {'id': '1'}` caused by an invalid query when loading incoming FKs. The error was ignored due to async but it still got printed to the console. --- datasette/app.py | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/datasette/app.py b/datasette/app.py index 6b143785..7d58319b 100644 --- a/datasette/app.py +++ b/datasette/app.py @@ -165,9 +165,9 @@ class BaseView(RenderMixin): else: rows = cursor.fetchall() truncated = False - except Exception: - print('ERROR: conn={}, sql = {}, params = {}'.format( - conn, repr(sql), params + except Exception as e: + print('ERROR: conn={}, sql = {}, params = {}: {}'.format( + conn, repr(sql), params, e )) raise if truncate: @@ -973,9 +973,12 @@ class RowView(RowTableShared): if len(pk_values) != 1: return [] table_info = self.ds.inspect()[name]['tables'].get(table) - if not table: + if not table_info: return [] foreign_keys = table_info['foreign_keys']['incoming'] + if len(foreign_keys) == 0: + return [] + sql = 'select ' + ', '.join([ '(select count(*) from {table} where "{column}"=:id)'.format( table=escape_sqlite(fk['other_table']), From d72201e883c0612d14dfb8ffdd61aa0fe223d94a Mon Sep 17 00:00:00 2001 From: Simon Willison Date: Sat, 14 Apr 2018 07:55:27 -0700 Subject: [PATCH 2/4] Added unit test for foreign key links in HTML Needed to add a further unit test for #207 --- tests/fixtures.py | 20 ++++++++++++- tests/test_api.py | 73 ++++++++++++++++++++++++++++++++++++---------- tests/test_html.py | 18 ++++++++++-- 3 files changed, 93 insertions(+), 18 deletions(-) diff --git a/tests/fixtures.py b/tests/fixtures.py index b44b2e1d..7796433c 100644 --- a/tests/fixtures.py +++ b/tests/fixtures.py @@ -87,10 +87,16 @@ METADATA = { TABLES = ''' CREATE TABLE simple_primary_key ( - pk varchar(30) primary key, + id varchar(30) primary key, content text ); +CREATE TABLE primary_key_multiple_columns ( + id varchar(30) primary key, + content text, + content2 text +); + CREATE TABLE compound_primary_key ( pk1 varchar(30), pk2 varchar(30), @@ -108,6 +114,14 @@ CREATE TABLE compound_three_primary_keys ( PRIMARY KEY (pk1, pk2, pk3) ); +CREATE TABLE foreign_key_references ( + pk varchar(30) primary key, + foreign_key_with_label varchar(30), + foreign_key_with_no_label varchar(30), + FOREIGN KEY (foreign_key_with_label) REFERENCES simple_primary_key(id), + FOREIGN KEY (foreign_key_with_no_label) REFERENCES primary_key_multiple_columns(id) +); + CREATE TABLE sortable ( pk1 varchar(30), pk2 varchar(30), @@ -166,6 +180,10 @@ INSERT INTO simple_primary_key VALUES (1, 'hello'); INSERT INTO simple_primary_key VALUES (2, 'world'); INSERT INTO simple_primary_key VALUES (3, ''); +INSERT INTO primary_key_multiple_columns VALUES (1, 'hey', 'world'); + +INSERT INTO foreign_key_references VALUES (1, 1, 1); + INSERT INTO complex_foreign_keys VALUES (1, 1, 2, 1); INSERT INTO [table/with/slashes.csv] VALUES (3, 'hey'); diff --git a/tests/test_api.py b/tests/test_api.py index 33938667..8b66e6b4 100644 --- a/tests/test_api.py +++ b/tests/test_api.py @@ -14,7 +14,7 @@ def test_homepage(app_client): assert response.json.keys() == {'test_tables': 0}.keys() d = response.json['test_tables'] assert d['name'] == 'test_tables' - assert d['tables_count'] == 10 + assert d['tables_count'] == 12 def test_database_page(app_client): @@ -76,6 +76,25 @@ def test_database_page(app_client): 'foreign_keys': {'incoming': [], 'outgoing': []}, 'label_column': None, 'primary_keys': ['pk1', 'pk2', 'pk3'], + }, { + 'columns': ['pk', 'foreign_key_with_label', 'foreign_key_with_no_label'], + 'name': 'foreign_key_references', + 'count': 1, + 'hidden': False, + 'foreign_keys': { + 'incoming': [], + 'outgoing': [{ + 'column': 'foreign_key_with_no_label', + 'other_column': 'id', + 'other_table': 'primary_key_multiple_columns' + }, { + 'column': 'foreign_key_with_label', + 'other_column': 'id', + 'other_table': 'simple_primary_key', + }], + }, + 'label_column': None, + 'primary_keys': ['pk'], }, { 'columns': ['content', 'a', 'b', 'c'], 'name': 'no_primary_key', @@ -84,6 +103,21 @@ def test_database_page(app_client): 'foreign_keys': {'incoming': [], 'outgoing': []}, 'label_column': None, 'primary_keys': [], + }, { + 'columns': ['id', 'content', 'content2'], + 'name': 'primary_key_multiple_columns', + 'count': 1, + 'foreign_keys': { + 'incoming': [{ + 'column': 'id', + 'other_column': 'foreign_key_with_no_label', + 'other_table': 'foreign_key_references' + }], + 'outgoing': [] + }, + 'hidden': False, + 'label_column': None, + 'primary_keys': ['id'] }, { 'columns': ['group', 'having', 'and'], 'name': 'select', @@ -93,12 +127,16 @@ def test_database_page(app_client): 'label_column': None, 'primary_keys': [], }, { - 'columns': ['pk', 'content'], + 'columns': ['id', 'content'], 'name': 'simple_primary_key', 'count': 3, 'hidden': False, 'foreign_keys': { 'incoming': [{ + 'column': 'id', + 'other_column': 'foreign_key_with_label', + 'other_table': 'foreign_key_references' + }, { 'column': 'id', 'other_column': 'f3', 'other_table': 'complex_foreign_keys' @@ -113,8 +151,8 @@ def test_database_page(app_client): }], 'outgoing': [], }, - 'label_column': None, - 'primary_keys': ['pk'], + 'label_column': 'content', + 'primary_keys': ['id'], }, { 'columns': [ 'pk1', 'pk2', 'content', 'sortable', 'sortable_with_nulls', @@ -194,16 +232,16 @@ def test_table_json(app_client): response = app_client.get('/test_tables/simple_primary_key.json?_shape=objects', gather_request=False) assert response.status == 200 data = response.json - assert data['query']['sql'] == 'select * from simple_primary_key order by pk limit 51' + assert data['query']['sql'] == 'select * from simple_primary_key order by id limit 51' assert data['query']['params'] == {} assert data['rows'] == [{ - 'pk': '1', + 'id': '1', 'content': 'hello', }, { - 'pk': '2', + 'id': '2', 'content': 'world', }, { - 'pk': '3', + 'id': '3', 'content': '', }] @@ -252,13 +290,13 @@ def test_table_shape_objects(app_client): gather_request=False ) assert [{ - 'pk': '1', + 'id': '1', 'content': 'hello', }, { - 'pk': '2', + 'id': '2', 'content': 'world', }, { - 'pk': '3', + 'id': '3', 'content': '', }] == response.json['rows'] @@ -270,15 +308,15 @@ def test_table_shape_object(app_client): ) assert { '1': { - 'pk': '1', + 'id': '1', 'content': 'hello', }, '2': { - 'pk': '2', + 'id': '2', 'content': 'world', }, '3': { - 'pk': '3', + 'id': '3', 'content': '', } } == response.json['rows'] @@ -512,13 +550,18 @@ def test_view(app_client): def test_row(app_client): response = app_client.get('/test_tables/simple_primary_key/1.json?_shape=objects', gather_request=False) assert response.status == 200 - assert [{'pk': '1', 'content': 'hello'}] == response.json['rows'] + assert [{'id': '1', 'content': 'hello'}] == response.json['rows'] def test_row_foreign_key_tables(app_client): response = app_client.get('/test_tables/simple_primary_key/1.json?_extras=foreign_key_tables', gather_request=False) assert response.status == 200 assert [{ + 'column': 'id', + 'count': 1, + 'other_column': 'foreign_key_with_label', + 'other_table': 'foreign_key_references' + }, { 'column': 'id', 'count': 1, 'other_column': 'f3', diff --git a/tests/test_html.py b/tests/test_html.py index b9ea246a..14b07fe2 100644 --- a/tests/test_html.py +++ b/tests/test_html.py @@ -172,7 +172,7 @@ def test_table_html_simple_primary_key(app_client): table = Soup(response.body, 'html.parser').find('table') ths = table.findAll('th') assert 'Link' == ths[0].string.strip() - for expected_col, th in zip(('pk', 'content'), ths[1:]): + for expected_col, th in zip(('id', 'content'), ths[1:]): a = th.find('a') assert expected_col == a.string assert a['href'].endswith('/simple_primary_key?_sort={}'.format( @@ -200,7 +200,7 @@ def test_row_html_simple_primary_key(app_client): response = app_client.get('/test_tables/simple_primary_key/1', gather_request=False) table = Soup(response.body, 'html.parser').find('table') assert [ - 'pk', 'content' + 'id', 'content' ] == [th.string.strip() for th in table.select('thead th')] assert [ [ @@ -276,6 +276,20 @@ def test_table_html_compound_primary_key(app_client): assert expected == [[str(td) for td in tr.select('td')] for tr in table.select('tbody tr')] +def test_table_html_foreign_key_links(app_client): + response = app_client.get('/test_tables/foreign_key_references', gather_request=False) + table = Soup(response.body, 'html.parser').find('table') + expected = [ + [ + '1', + '1', + 'hello\xa01', + '1' + ] + ] + assert expected == [[str(td) for td in tr.select('td')] for tr in table.select('tbody tr')] + + def test_row_html_compound_primary_key(app_client): response = app_client.get('/test_tables/compound_primary_key/a,b', gather_request=False) table = Soup(response.body, 'html.parser').find('table') From f2b940d6026677f6859d46a4f16fa402745d261d Mon Sep 17 00:00:00 2001 From: Russ Garrett Date: Sat, 14 Apr 2018 14:17:20 +0100 Subject: [PATCH 3/4] Link foreign keys which don't have labels This renders unlabeled FKs as simple links. I can't see why this would cause any major problems. Also includes bonus fixes for two minor issues: * In foreign key link hrefs the primary key was escaped using HTML escaping rather than URL escaping. This broke some non-integer PKs. * Print tracebacks to console when handling 500 errors. --- datasette/app.py | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/datasette/app.py b/datasette/app.py index 7d58319b..5259e253 100644 --- a/datasette/app.py +++ b/datasette/app.py @@ -17,6 +17,7 @@ import jinja2 import hashlib import time import pint +import traceback from .utils import ( Filters, CustomJSONEncoder, @@ -479,13 +480,15 @@ class RowTableShared(BaseView): pks = table_info.get('primary_keys') or [] # Prefetch foreign key resolutions for later expansion: - expanded = {} + fks = {} + labeled_fks = {} if table_info and expand_foreign_keys: foreign_keys = table_info['foreign_keys']['outgoing'] for fk in foreign_keys: label_column = tables.get(fk['other_table'], {}).get('label_column') if not label_column: - # We only link cells to other tables with label columns defined + # No label for this FK + fks[fk['column']] = fk['other_table'] continue ids_to_lookup = set([row[fk['column']] for row in rows]) sql = 'select "{other_column}", "{label_column}" from {other_table} where "{other_column}" in ({placeholders})'.format( @@ -501,7 +504,7 @@ class RowTableShared(BaseView): pass else: for id, value in results: - expanded[(fk['column'], id)] = (fk['other_table'], value) + labeled_fks[(fk['column'], id)] = (fk['other_table'], value) cell_rows = [] for row in rows: @@ -521,16 +524,24 @@ class RowTableShared(BaseView): }) for value, column_dict in zip(row, columns): column = column_dict['name'] - if (column, value) in expanded: - other_table, label = expanded[(column, value)] + if (column, value) in labeled_fks: + other_table, label = labeled_fks[(column, value)] display_value = jinja2.Markup( - '{label} {id}'.format( + '{label} {id}'.format( database=database, table=urllib.parse.quote_plus(other_table), + link_id=urllib.parse.quote_plus(str(value)), id=str(jinja2.escape(value)), label=str(jinja2.escape(label)), ) ) + elif column in fks: + display_value = jinja2.Markup( + '{id}'.format( + database=database, + table=urllib.parse.quote_plus(fks[column]), + link_id=urllib.parse.quote_plus(str(value)), + id=str(jinja2.escape(value)))) elif value is None: display_value = jinja2.Markup(' ') elif is_url(str(value).strip()): @@ -1244,6 +1255,7 @@ class Datasette: status = 500 info = {} message = str(exception) + traceback.print_exc() templates = ['500.html'] if status != 500: templates = ['{}.html'.format(status)] + templates From 6b15a53cd3cd40880a5e2d38827d5fac10e4bb5f Mon Sep 17 00:00:00 2001 From: Simon Willison Date: Sat, 14 Apr 2018 08:00:54 -0700 Subject: [PATCH 4/4] Unit test for unlabelled foreign keys from #207 --- tests/test_html.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_html.py b/tests/test_html.py index 14b07fe2..15ab8d41 100644 --- a/tests/test_html.py +++ b/tests/test_html.py @@ -284,7 +284,7 @@ def test_table_html_foreign_key_links(app_client): '1', '1', 'hello\xa01', - '1' + '1' ] ] assert expected == [[str(td) for td in tr.select('td')] for tr in table.select('tbody tr')]