From 317eeadf21b4f953e732c3964cc0a2f71171c503 Mon Sep 17 00:00:00 2001 From: Russ Garrett Date: Sun, 15 Apr 2018 22:48:30 +0100 Subject: [PATCH 1/4] Correct escaping for HTML display of row links --- datasette/app.py | 5 +++-- datasette/utils.py | 19 ++++++++++++------- 2 files changed, 15 insertions(+), 9 deletions(-) diff --git a/datasette/app.py b/datasette/app.py index d17f2096..62411222 100644 --- a/datasette/app.py +++ b/datasette/app.py @@ -524,10 +524,11 @@ class RowTableShared(BaseView): cells.append({ 'column': 'Link', 'value': jinja2.Markup( - '{flat_pks}'.format( + '{flat_pks}'.format( database=database, table=urllib.parse.quote_plus(table), - flat_pks=path_from_row_pks(row, pks, not pks), + flat_pks=str(jinja2.escape(path_from_row_pks(row, pks, not pks, False))), + flat_pks_quoted=path_from_row_pks(row, pks, not pks) ) ), }) diff --git a/datasette/utils.py b/datasette/utils.py index 1f296a0b..7ba663f0 100644 --- a/datasette/utils.py +++ b/datasette/utils.py @@ -38,14 +38,19 @@ def urlsafe_components(token): ] -def path_from_row_pks(row, pks, use_rowid): +def path_from_row_pks(row, pks, use_rowid, quote=True): + """ Generate an optionally URL-quoted unique identifier + for a row from its primary keys.""" if use_rowid: - return urllib.parse.quote_plus(str(row['rowid'])) - bits = [] - for pk in pks: - bits.append( - urllib.parse.quote_plus(str(row[pk])) - ) + bits = [row['rowid']] + else: + bits = [row[pk] for pk in pks] + + if quote: + bits = [urllib.parse.quote_plus(str(bit)) for bit in bits] + else: + bits = [str(bit) for bit in bits] + return ','.join(bits) From 55b4697123f983cac5a80cd12bebdc98ef5004bd Mon Sep 17 00:00:00 2001 From: Russ Garrett Date: Sun, 15 Apr 2018 22:49:01 +0100 Subject: [PATCH 2/4] Don't duplicate simple primary keys in the link column When there's a simple (single-column) primary key, it looks weird to duplicate it in the link column. This change removes the second PK column and treats the link column as if it were the PK column from a header/sorting perspective. --- datasette/app.py | 35 +++++++++++++++++++++++------------ 1 file changed, 23 insertions(+), 12 deletions(-) diff --git a/datasette/app.py b/datasette/app.py index 62411222..4a161e97 100644 --- a/datasette/app.py +++ b/datasette/app.py @@ -532,8 +532,13 @@ class RowTableShared(BaseView): ) ), }) + for value, column_dict in zip(row, columns): column = column_dict['name'] + if link_column and len(pks) == 1 and column == pks[0]: + # If there's a simple primary key, don't repeat the value as it's + # already shown in the link column. + continue if (column, value) in labeled_fks: other_table, label = labeled_fks[(column, value)] display_value = jinja2.Markup( @@ -560,17 +565,17 @@ class RowTableShared(BaseView): url=jinja2.escape(value.strip()) ) ) + elif column in table_metadata.get('units', {}) and value != '': + # Interpret units using pint + value = value * ureg(table_metadata['units'][column]) + # Pint uses floating point which sometimes introduces errors in the compact + # representation, which we have to round off to avoid ugliness. In the vast + # majority of cases this rounding will be inconsequential. I hope. + value = round(value.to_compact(), 6) + display_value = jinja2.Markup('{:~P}'.format(value).replace(' ', ' ')) else: - if column in table_metadata.get('units', {}) and value != '': - # Interpret units using pint - value = value * ureg(table_metadata['units'][column]) - # Pint uses floating point which sometimes introduces errors in the compact - # representation, which we have to round off to avoid ugliness. In the vast - # majority of cases this rounding will be inconsequential. I hope. - value = round(value.to_compact(), 6) - display_value = jinja2.Markup('{:~P}'.format(value).replace(' ', ' ')) - else: - display_value = str(value) + display_value = str(value) + cells.append({ 'column': column, 'value': display_value, @@ -578,9 +583,15 @@ class RowTableShared(BaseView): cell_rows.append(cells) if link_column: + # Add the link column header. + # If it's a simple primary key, we have to remove and re-add that column name at + # the beginning of the header row. + if len(pks) == 1: + columns = [col for col in columns if col['name'] != pks[0]] + columns = [{ - 'name': 'Link', - 'sortable': False, + 'name': pks[0] if len(pks) == 1 else 'Link', + 'sortable': len(pks) == 1, }] + columns return columns, cell_rows From f4a1ab693f908008daa950221e595fcd871ae142 Mon Sep 17 00:00:00 2001 From: Russ Garrett Date: Mon, 16 Apr 2018 21:22:04 +0100 Subject: [PATCH 3/4] Additional test asserts --- tests/test_html.py | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/tests/test_html.py b/tests/test_html.py index 15ab8d41..239d5354 100644 --- a/tests/test_html.py +++ b/tests/test_html.py @@ -163,16 +163,18 @@ def test_sort_by_desc_redirects(app_client): ]) def test_css_classes_on_body(app_client, path, expected_classes): response = app_client.get(path, gather_request=False) + assert response.status == 200 classes = re.search(r'', response.text).group(1).split() assert classes == expected_classes def test_table_html_simple_primary_key(app_client): response = app_client.get('/test_tables/simple_primary_key', gather_request=False) + assert response.status == 200 table = Soup(response.body, 'html.parser').find('table') ths = table.findAll('th') - assert 'Link' == ths[0].string.strip() - for expected_col, th in zip(('id', 'content'), ths[1:]): + assert 'id' == ths[0].find('a').string.strip() + for expected_col, th in zip(('content',), ths[1:]): a = th.find('a') assert expected_col == a.string assert a['href'].endswith('/simple_primary_key?_sort={}'.format( @@ -182,15 +184,12 @@ def test_table_html_simple_primary_key(app_client): assert [ [ '1', - '1', 'hello' ], [ '2', - '2', 'world' ], [ '3', - '3', '' ] ] == [[str(td) for td in tr.select('td')] for tr in table.select('tbody tr')] @@ -198,6 +197,7 @@ def test_table_html_simple_primary_key(app_client): def test_row_html_simple_primary_key(app_client): response = app_client.get('/test_tables/simple_primary_key/1', gather_request=False) + assert response.status == 200 table = Soup(response.body, 'html.parser').find('table') assert [ 'id', 'content' @@ -218,6 +218,7 @@ def test_table_not_exists(app_client): def test_table_html_no_primary_key(app_client): response = app_client.get('/test_tables/no_primary_key', gather_request=False) + assert response.status == 200 table = Soup(response.body, 'html.parser').find('table') # We have disabled sorting for this table using metadata.json assert [ @@ -238,6 +239,7 @@ def test_table_html_no_primary_key(app_client): def test_row_html_no_primary_key(app_client): response = app_client.get('/test_tables/no_primary_key/1', gather_request=False) + assert response.status == 200 table = Soup(response.body, 'html.parser').find('table') assert [ 'rowid', 'content', 'a', 'b', 'c' @@ -256,6 +258,7 @@ def test_row_html_no_primary_key(app_client): def test_table_html_compound_primary_key(app_client): response = app_client.get('/test_tables/compound_primary_key', gather_request=False) + assert response.status == 200 table = Soup(response.body, 'html.parser').find('table') ths = table.findAll('th') assert 'Link' == ths[0].string.strip() @@ -278,11 +281,11 @@ def test_table_html_compound_primary_key(app_client): def test_table_html_foreign_key_links(app_client): response = app_client.get('/test_tables/foreign_key_references', gather_request=False) + assert response.status == 200 table = Soup(response.body, 'html.parser').find('table') expected = [ [ '1', - '1', 'hello\xa01', '1' ] @@ -292,6 +295,7 @@ def test_table_html_foreign_key_links(app_client): def test_row_html_compound_primary_key(app_client): response = app_client.get('/test_tables/compound_primary_key/a,b', gather_request=False) + assert response.status == 200 table = Soup(response.body, 'html.parser').find('table') assert [ 'pk1', 'pk2', 'content' @@ -308,6 +312,7 @@ def test_row_html_compound_primary_key(app_client): def test_view_html(app_client): response = app_client.get('/test_tables/simple_view', gather_request=False) + assert response.status == 200 table = Soup(response.body, 'html.parser').find('table') assert [ 'content', 'upper_content' @@ -329,6 +334,7 @@ def test_view_html(app_client): def test_index_metadata(app_client): response = app_client.get('/', gather_request=False) + assert response.status == 200 soup = Soup(response.body, 'html.parser') assert 'Datasette Title' == soup.find('h1').text assert 'Datasette Description' == inner_html( @@ -339,6 +345,7 @@ def test_index_metadata(app_client): def test_database_metadata(app_client): response = app_client.get('/test_tables', gather_request=False) + assert response.status == 200 soup = Soup(response.body, 'html.parser') # Page title should be the default assert 'test_tables' == soup.find('h1').text @@ -352,6 +359,7 @@ def test_database_metadata(app_client): def test_table_metadata(app_client): response = app_client.get('/test_tables/simple_primary_key', gather_request=False) + assert response.status == 200 soup = Soup(response.body, 'html.parser') # Page title should be custom and should be HTML escaped assert 'This <em>HTML</em> is escaped' == inner_html(soup.find('h1')) From 4acde4e187795214af6fc86f46af48982ec5de46 Mon Sep 17 00:00:00 2001 From: Russ Garrett Date: Tue, 17 Apr 2018 09:29:48 +0100 Subject: [PATCH 4/4] Add column name classes to s, make PK bold --- datasette/static/app.css | 3 + datasette/templates/_rows_and_columns.html | 2 +- tests/test_html.py | 70 +++++++++++----------- 3 files changed, 39 insertions(+), 36 deletions(-) diff --git a/datasette/static/app.css b/datasette/static/app.css index 21faa7a7..98c4a47c 100644 --- a/datasette/static/app.css +++ b/datasette/static/app.css @@ -17,6 +17,9 @@ td { padding: 4px; vertical-align: top; } +td.col-link { + font-weight: bold; +} td em { font-style: normal; font-size: 0.8em; diff --git a/datasette/templates/_rows_and_columns.html b/datasette/templates/_rows_and_columns.html index 7e11b2e8..8aa2e657 100644 --- a/datasette/templates/_rows_and_columns.html +++ b/datasette/templates/_rows_and_columns.html @@ -20,7 +20,7 @@ {% for row in display_rows %} {% for cell in row %} - {{ cell.value }} + {{ cell.value }} {% endfor %} {% endfor %} diff --git a/tests/test_html.py b/tests/test_html.py index 239d5354..e13c8b4f 100644 --- a/tests/test_html.py +++ b/tests/test_html.py @@ -183,14 +183,14 @@ def test_table_html_simple_primary_key(app_client): assert ['nofollow'] == a['rel'] assert [ [ - '1', - 'hello' + '1', + 'hello' ], [ - '2', - 'world' + '2', + 'world' ], [ - '3', - '' + '3', + '' ] ] == [[str(td) for td in tr.select('td')] for tr in table.select('tbody tr')] @@ -204,8 +204,8 @@ def test_row_html_simple_primary_key(app_client): ] == [th.string.strip() for th in table.select('thead th')] assert [ [ - '1', - 'hello' + '1', + 'hello' ] ] == [[str(td) for td in tr.select('td')] for tr in table.select('tbody tr')] @@ -226,12 +226,12 @@ def test_table_html_no_primary_key(app_client): ] == [th.string.strip() for th in table.select('thead th')[2:]] expected = [ [ - '{}'.format(i, i), - '{}'.format(i), - '{}'.format(i), - 'a{}'.format(i), - 'b{}'.format(i), - 'c{}'.format(i), + '{}'.format(i, i), + '{}'.format(i), + '{}'.format(i), + 'a{}'.format(i), + 'b{}'.format(i), + 'c{}'.format(i), ] for i in range(1, 51) ] assert expected == [[str(td) for td in tr.select('td')] for tr in table.select('tbody tr')] @@ -246,11 +246,11 @@ def test_row_html_no_primary_key(app_client): ] == [th.string.strip() for th in table.select('thead th')] expected = [ [ - '1', - '1', - 'a1', - 'b1', - 'c1', + '1', + '1', + 'a1', + 'b1', + 'c1', ] ] assert expected == [[str(td) for td in tr.select('td')] for tr in table.select('tbody tr')] @@ -270,10 +270,10 @@ def test_table_html_compound_primary_key(app_client): )) expected = [ [ - 'a,b', - 'a', - 'b', - 'c', + 'a,b', + 'a', + 'b', + 'c', ] ] assert expected == [[str(td) for td in tr.select('td')] for tr in table.select('tbody tr')] @@ -285,9 +285,9 @@ def test_table_html_foreign_key_links(app_client): table = Soup(response.body, 'html.parser').find('table') expected = [ [ - '1', - 'hello\xa01', - '1' + '1', + 'hello\xa01', + '1' ] ] assert expected == [[str(td) for td in tr.select('td')] for tr in table.select('tbody tr')] @@ -302,9 +302,9 @@ def test_row_html_compound_primary_key(app_client): ] == [th.string.strip() for th in table.select('thead th')] expected = [ [ - 'a', - 'b', - 'c', + 'a', + 'b', + 'c', ] ] assert expected == [[str(td) for td in tr.select('td')] for tr in table.select('tbody tr')] @@ -319,14 +319,14 @@ def test_view_html(app_client): ] == [th.string.strip() for th in table.select('thead th')] expected = [ [ - 'hello', - 'HELLO' + 'hello', + 'HELLO' ], [ - 'world', - 'WORLD' + 'world', + 'WORLD' ], [ - '', - '' + '', + '' ] ] assert expected == [[str(td) for td in tr.select('td')] for tr in table.select('tbody tr')]