From 9f28bbe43dc277a3963a12aaae37b5ee3c277207 Mon Sep 17 00:00:00 2001 From: Simon Willison Date: Fri, 13 Apr 2018 11:17:22 -0700 Subject: [PATCH] Better mechanism for handling errors; 404s for missing table/database New error mechanism closes #193 404s for missing tables/databesse closes #184 Makes pull request #202 unnecessary. --- datasette/app.py | 78 ++++++++++++++++++++++++++-------- datasette/templates/500.html | 12 ++++++ datasette/templates/base.html | 22 +++++----- datasette/templates/error.html | 14 ------ tests/test_api.py | 11 +++++ tests/test_html.py | 6 +++ 6 files changed, 101 insertions(+), 42 deletions(-) create mode 100644 datasette/templates/500.html delete mode 100644 datasette/templates/error.html diff --git a/datasette/app.py b/datasette/app.py index 50794954..807918bc 100644 --- a/datasette/app.py +++ b/datasette/app.py @@ -47,7 +47,11 @@ connections = threading.local() class DatasetteError(Exception): - pass + def __init__(self, message, title=None, error_dict=None, status=500, template=None): + self.message = message + self.title = title + self.error_dict = error_dict or {} + self.status = status class RenderMixin(HTTPMethodView): @@ -200,14 +204,11 @@ class BaseView(RenderMixin): else: data, extra_template_data, templates = response_or_template_contexts except (sqlite3.OperationalError, InvalidSql, DatasetteError) as e: - data = { - 'ok': False, - 'error': str(e), - 'database': name, - 'database_hash': hash, - } - status_code = 400 - templates = ['error.html'] + raise DatasetteError(str(e), title='Invalid SQL', status=400) + except (sqlite3.OperationalError) as e: + raise DatasetteError(str(e)) + except DatasetteError: + raise end = time.time() data['query_ms'] = (end - start) * 1000 for key in ('source', 'source_url', 'license', 'license_url'): @@ -554,19 +555,28 @@ class TableView(RowTableShared): canned_query = self.ds.get_canned_query(name, table) if canned_query is not None: return await self.custom_sql(request, name, hash, canned_query['sql'], editable=False, canned_query=table) - is_view = bool(list(await self.execute(name, "SELECT count(*) from sqlite_master WHERE type = 'view' and name=:n", { - 'n': table, - }))[0][0]) + is_view = bool(list(await self.execute( + name, + "SELECT count(*) from sqlite_master WHERE type = 'view' and name=:n", + {'n': table} + ))[0][0]) view_definition = None table_definition = None if is_view: - view_definition = list(await self.execute(name, 'select sql from sqlite_master where name = :n and type="view"', { - 'n': table, - }))[0][0] + view_definition = list(await self.execute( + name, + 'select sql from sqlite_master where name = :n and type="view"', + {'n': table} + ))[0][0] else: - table_definition = list(await self.execute(name, 'select sql from sqlite_master where name = :n and type="table"', { - 'n': table, - }))[0][0] + table_definition_rows = list(await self.execute( + name, + 'select sql from sqlite_master where name = :n and type="table"', + {'n': table} + )) + if not table_definition_rows: + raise NotFound('Table not found: {}'.format(table)) + table_definition = table_definition_rows[0][0] info = self.ds.inspect() table_info = info[name]['tables'].get(table) or {} pks = table_info.get('primary_keys') or [] @@ -1199,4 +1209,36 @@ class Datasette: RowView.as_view(self), '///' ) + + @app.exception(Exception) + def on_exception(request, exception): + title = None + if isinstance(exception, NotFound): + status = 404 + info = {} + message = exception.args[0] + elif isinstance(exception, DatasetteError): + status = exception.status + info = exception.error_dict + message = exception.message + title = exception.title + else: + status = 500 + info = {} + message = str(exception) + templates = ['500.html'] + if status != 500: + templates = ['{}.html'.format(status)] + templates + info.update({ + 'ok': False, + 'error': message, + 'status': status, + 'title': title, + }) + if (request.path.split('?')[0].endswith('.json')): + return response.json(info, status=status) + else: + template = self.jinja_env.select_template(templates) + return response.html(template.render(info), status=status) + return app diff --git a/datasette/templates/500.html b/datasette/templates/500.html new file mode 100644 index 00000000..809b2a71 --- /dev/null +++ b/datasette/templates/500.html @@ -0,0 +1,12 @@ +{% extends "base.html" %} + +{% block title %}{% if title %}{{ title }}{% else %}Error {{ status }}{% endif %}{% endblock %} + +{% block content %} +
home
+ +

{% if title %}{{ title }}{% else %}Error {{ status }}{% endif %}

+ +
{{ error }}
+ +{% endblock %} diff --git a/datasette/templates/base.html b/datasette/templates/base.html index 6da5ee46..382c8e92 100644 --- a/datasette/templates/base.html +++ b/datasette/templates/base.html @@ -20,17 +20,19 @@
Powered by Datasette {% if query_ms %}· Query took {{ query_ms|round(3) }}ms{% endif %} - {% if metadata.license or metadata.license_url %}· Data license: - {% if metadata.license_url %} - {{ metadata.license or metadata.license_url }} - {% else %} - {{ metadata.license }} + {% if metadata %} + {% if metadata.license or metadata.license_url %}· Data license: + {% if metadata.license_url %} + {{ metadata.license or metadata.license_url }} + {% else %} + {{ metadata.license }} + {% endif %} + {% endif %} + {% if metadata.source or metadata.source_url %}· + Data source: {% if metadata.source_url %} + + {% endif %}{{ metadata.source or metadata.source_url }}{% if metadata.source_url %}{% endif %} {% endif %} - {% endif %} - {% if metadata.source or metadata.source_url %}· - Data source: {% if metadata.source_url %} - - {% endif %}{{ metadata.source or metadata.source_url }}{% if metadata.source_url %}{% endif %} {% endif %}
{% if select_templates %}{% endif %} diff --git a/datasette/templates/error.html b/datasette/templates/error.html deleted file mode 100644 index 8d9bb462..00000000 --- a/datasette/templates/error.html +++ /dev/null @@ -1,14 +0,0 @@ -{% extends "base.html" %} - -{% block title %}{{ database }}{% endblock %} - -{% block content %} -
home / {{ database }}
- -

{{ database }}

- -{% if error %} -
{{ error }}
-{% endif %} - -{% endblock %} diff --git a/tests/test_api.py b/tests/test_api.py index df3b3dd9..33938667 100644 --- a/tests/test_api.py +++ b/tests/test_api.py @@ -208,6 +208,17 @@ def test_table_json(app_client): }] +def test_table_not_exists_json(app_client): + assert { + 'ok': False, + 'error': 'Table not found: blah', + 'status': 404, + 'title': None, + } == app_client.get( + '/test_tables/blah.json', gather_request=False + ).json + + def test_jsono_redirects_to_shape_objects(app_client): response_1 = app_client.get( '/test_tables/simple_primary_key.jsono', diff --git a/tests/test_html.py b/tests/test_html.py index 31928a34..b9ea246a 100644 --- a/tests/test_html.py +++ b/tests/test_html.py @@ -210,6 +210,12 @@ def test_row_html_simple_primary_key(app_client): ] == [[str(td) for td in tr.select('td')] for tr in table.select('tbody tr')] +def test_table_not_exists(app_client): + assert 'Table not found: blah' in app_client.get( + '/test_tables/blah', gather_request=False + ).body.decode('utf8') + + def test_table_html_no_primary_key(app_client): response = app_client.get('/test_tables/no_primary_key', gather_request=False) table = Soup(response.body, 'html.parser').find('table')