diff --git a/datasette/handle_exception.py b/datasette/handle_exception.py index 2b311644..fc290dbc 100644 --- a/datasette/handle_exception.py +++ b/datasette/handle_exception.py @@ -1,5 +1,5 @@ from datasette import hookimpl, Response -from .utils import add_cors_headers +from .utils import add_cors_headers, error_body from .utils.asgi import ( Base400, ) @@ -45,6 +45,13 @@ def handle_exception(datasette, request, exception): message = str(exception) traceback.print_exc() templates = [f"{status}.html", "error.html"] + headers = {} + if datasette.cors: + add_cors_headers(headers) + if request.path.split("?")[0].endswith(".json"): + body = dict(info) + body.update(error_body(message, status)) + return Response.json(body, status=status, headers=headers) info.update( { "ok": False, @@ -53,24 +60,18 @@ def handle_exception(datasette, request, exception): "title": title, } ) - headers = {} - if datasette.cors: - add_cors_headers(headers) - if request.path.split("?")[0].endswith(".json"): - return Response.json(info, status=status, headers=headers) - else: - environment = datasette.get_jinja_environment(request) - template = environment.select_template(templates) - return Response.html( - await template.render_async( - dict( - info, - urls=datasette.urls, - menu_links=lambda: [], - ) - ), - status=status, - headers=headers, - ) + environment = datasette.get_jinja_environment(request) + template = environment.select_template(templates) + return Response.html( + await template.render_async( + dict( + info, + urls=datasette.urls, + menu_links=lambda: [], + ) + ), + status=status, + headers=headers, + ) return inner diff --git a/datasette/renderer.py b/datasette/renderer.py index f40e3dbb..7c94f6ee 100644 --- a/datasette/renderer.py +++ b/datasette/renderer.py @@ -1,6 +1,7 @@ import json from datasette.extras import extra_names_from_request from datasette.utils import ( + error_body, value_as_boolean, remove_infinites, CustomJSONEncoder, @@ -52,8 +53,7 @@ def json_renderer(request, args, data, error, truncated=None): if error: shape = "objects" status_code = 400 - data["error"] = error - data["ok"] = False + data.update(error_body(error, status_code)) if truncated is not None: data["truncated"] = truncated @@ -87,7 +87,8 @@ def json_renderer(request, args, data, error, truncated=None): object_rows[pk_string] = row data = object_rows if shape_error: - data = {"ok": False, "error": shape_error} + status_code = 400 + data = error_body(shape_error, status_code) elif shape == "array": data = data["rows"] @@ -100,12 +101,7 @@ def json_renderer(request, args, data, error, truncated=None): data["rows"] = [list(row.values()) for row in data["rows"]] else: status_code = 400 - data = { - "ok": False, - "error": f"Invalid _shape: {shape}", - "status": 400, - "title": None, - } + data = error_body(f"Invalid _shape: {shape}", status_code) # Don't include "columns" in output # https://github.com/simonw/datasette/issues/2136 diff --git a/datasette/utils/__init__.py b/datasette/utils/__init__.py index 1caff3a8..17c8702f 100644 --- a/datasette/utils/__init__.py +++ b/datasette/utils/__init__.py @@ -1294,6 +1294,27 @@ async def derive_named_parameters(db: "Database", sql: str) -> List[str]: return named_parameters(sql) +def error_body(messages, status): + """ + The canonical JSON error body used by every Datasette JSON error response: + + {"ok": False, "error": "...", "errors": ["...", ...], "status": 400} + + "error" is all of the messages joined with "; ", "errors" is the full + list, "status" matches the HTTP status code. Callers may add extra + context keys to the returned dictionary but must not remove these four. + """ + if isinstance(messages, str): + messages = [messages] + messages = [str(message) for message in messages] + return { + "ok": False, + "error": "; ".join(messages), + "errors": messages, + "status": status, + } + + def add_cors_headers(headers): headers["Access-Control-Allow-Origin"] = "*" headers["Access-Control-Allow-Headers"] = "Authorization, Content-Type" diff --git a/datasette/views/base.py b/datasette/views/base.py index 30026f4b..a12ae050 100644 --- a/datasette/views/base.py +++ b/datasette/views/base.py @@ -5,6 +5,7 @@ import sys from datasette.utils.asgi import Request from datasette.utils import ( add_cors_headers, + error_body, EscapeHtmlWriter, InvalidSql, LimitedWriter, @@ -49,9 +50,7 @@ class View: request.path.endswith(".json") or request.headers.get("content-type") == "application/json" ): - response = Response.json( - {"ok": False, "error": "Method not allowed"}, status=405 - ) + response = Response.json(error_body("Method not allowed", 405), status=405) else: response = Response.text("Method not allowed", status=405) return response @@ -90,9 +89,7 @@ class BaseView: request.path.endswith(".json") or request.headers.get("content-type") == "application/json" ): - response = Response.json( - {"ok": False, "error": "Method not allowed"}, status=405 - ) + response = Response.json(error_body("Method not allowed", 405), status=405) else: response = Response.text("Method not allowed", status=405) return response @@ -181,7 +178,7 @@ class BaseView: def _error(messages, status=400): - return Response.json({"ok": False, "errors": messages}, status=status) + return Response.json(error_body(messages, status), status=status) async def stream_csv(datasette, fetch_data, request, database): diff --git a/datasette/views/special.py b/datasette/views/special.py index 3245bc13..602ec5ea 100644 --- a/datasette/views/special.py +++ b/datasette/views/special.py @@ -9,6 +9,7 @@ from datasette.utils import ( actor_matches_allow, add_cors_headers, await_me_maybe, + error_body, tilde_encode, tilde_decode, ) @@ -348,26 +349,29 @@ class AllowedResourcesView(BaseView): async def _allowed_payload(self, request, has_debug_permission): action = request.args.get("action") if not action: - return {"error": "action parameter is required"}, 400 + return error_body("action parameter is required", 400), 400 if action not in self.ds.actions: - return {"error": f"Unknown action: {action}"}, 404 + return error_body(f"Unknown action: {action}", 404), 404 actor = request.actor if isinstance(request.actor, dict) else None actor_id = actor.get("id") if actor else None parent_filter = request.args.get("parent") child_filter = request.args.get("child") if child_filter and not parent_filter: - return {"error": "parent must be provided when child is specified"}, 400 + return ( + error_body("parent must be provided when child is specified", 400), + 400, + ) try: page = int(request.args.get("page", "1")) page_size = int(request.args.get("page_size", "50")) except ValueError: - return {"error": "page and page_size must be integers"}, 400 + return error_body("page and page_size must be integers", 400), 400 if page < 1: - return {"error": "page must be >= 1"}, 400 + return error_body("page must be >= 1", 400), 400 if page_size < 1: - return {"error": "page_size must be >= 1"}, 400 + return error_body("page_size must be >= 1", 400), 400 max_page_size = 200 if page_size > max_page_size: page_size = max_page_size @@ -485,9 +489,13 @@ class PermissionRulesView(BaseView): # JSON API - action parameter is required action = request.args.get("action") if not action: - return Response.json({"error": "action parameter is required"}, status=400) + return Response.json( + error_body("action parameter is required", 400), status=400 + ) if action not in self.ds.actions: - return Response.json({"error": f"Unknown action: {action}"}, status=404) + return Response.json( + error_body(f"Unknown action: {action}", 404), status=404 + ) actor = request.actor if isinstance(request.actor, dict) else None @@ -496,12 +504,12 @@ class PermissionRulesView(BaseView): page_size = int(request.args.get("page_size", "50")) except ValueError: return Response.json( - {"error": "page and page_size must be integers"}, status=400 + error_body("page and page_size must be integers", 400), status=400 ) if page < 1: - return Response.json({"error": "page must be >= 1"}, status=400) + return Response.json(error_body("page must be >= 1", 400), status=400) if page_size < 1: - return Response.json({"error": "page_size must be >= 1"}, status=400) + return Response.json(error_body("page_size must be >= 1", 400), status=400) max_page_size = 200 if page_size > max_page_size: page_size = max_page_size @@ -587,15 +595,15 @@ class PermissionRulesView(BaseView): async def _check_permission_for_actor(ds, action, parent, child, actor): """Shared logic for checking permissions. Returns a dict with check results.""" if action not in ds.actions: - return {"error": f"Unknown action: {action}"}, 404 + return error_body(f"Unknown action: {action}", 404), 404 if child and not parent: - return {"error": "parent is required when child is provided"}, 400 + return error_body("parent is required when child is provided", 400), 400 # Use the action's properties to create the appropriate resource object action_obj = ds.actions.get(action) if not action_obj: - return {"error": f"Unknown action: {action}"}, 400 + return error_body(f"Unknown action: {action}", 400), 400 # Global actions (no resource_class) don't have a resource if action_obj.resource_class is None: @@ -610,7 +618,7 @@ async def _check_permission_for_actor(ds, action, parent, child, actor): resource_obj = action_obj.resource_class(parent) else: # This shouldn't happen given validation in Action.__post_init__ - return {"error": f"Invalid action configuration: {action}"}, 500 + return error_body(f"Invalid action configuration: {action}", 500), 500 allowed = await ds.allowed(action=action, resource=resource_obj, actor=actor) @@ -651,7 +659,9 @@ class PermissionCheckView(BaseView): # JSON API - action parameter is required action = request.args.get("action") if not action: - return Response.json({"error": "action parameter is required"}, status=400) + return Response.json( + error_body("action parameter is required", 400), status=400 + ) parent = request.args.get("parent") child = request.args.get("child") @@ -1229,7 +1239,7 @@ class SchemaBaseView(BaseView): if self.ds.cors: add_cors_headers(headers) return Response.json( - {"ok": False, "error": error_message}, status=status, headers=headers + error_body(error_message, status), status=status, headers=headers ) else: return Response.text(error_message, status=status) diff --git a/docs/json_api.rst b/docs/json_api.rst index eca22fdc..3df66a8e 100644 --- a/docs/json_api.rst +++ b/docs/json_api.rst @@ -50,6 +50,37 @@ The ``"truncated"`` key lets you know if the query was truncated. This can happe For table pages, an additional key ``"next"`` may be present. This indicates that the next page in the pagination set can be retrieved using ``?_next=VALUE``. +.. _json_api_errors: + +Error responses +--------------- + +Every JSON error response from Datasette uses the same format: + +.. code-block:: json + + { + "ok": false, + "error": "Table not found", + "errors": [ + "Table not found" + ], + "status": 404 + } + +- ``"ok"`` is always ``false`` for an error. +- ``"errors"`` is a list of one or more error message strings. Endpoints that + validate multiple things at once - such as the :ref:`insert API ` - + may return several messages here. +- ``"error"`` is all of those messages joined with ``"; "``, for + convenience when displaying a single string. +- ``"status"`` matches the HTTP status code of the response. + +Some endpoints add extra context keys. For example, a SQL error from a +:ref:`custom query ` also includes the empty +``"rows"`` and ``"truncated"`` keys of the response it was unable to +produce. + .. _json_api_custom_sql: Executing custom SQL @@ -1625,15 +1656,17 @@ the execute-write returning row limit, which defaults to 10: ] } -Errors use the standard Datasette error format: +Errors use the :ref:`standard Datasette error format `: .. code-block:: json { "ok": false, + "error": "Permission denied: need execute-write-sql", "errors": [ "Permission denied: need execute-write-sql" - ] + ], + "status": 403 } .. _TableInsertView: @@ -1727,9 +1760,11 @@ If any of your rows have a primary key that is already in use, you will get an e { "ok": false, + "error": "UNIQUE constraint failed: new_table.id", "errors": [ "UNIQUE constraint failed: new_table.id" - ] + ], + "status": 400 } Pass ``"ignore": true`` to ignore these errors and insert the other rows: @@ -1859,9 +1894,11 @@ When using upsert you must provide the primary key column (or columns if the tab { "ok": false, + "error": "Row 0 is missing primary key column(s): \"id\"", "errors": [ "Row 0 is missing primary key column(s): \"id\"" - ] + ], + "status": 400 } If your table does not have an explicit primary key you should pass the SQLite ``rowid`` key instead. @@ -1921,7 +1958,7 @@ The returned JSON will look like this: } } -Any errors will return ``{"errors": ["... descriptive message ..."], "ok": false}``, and a ``400`` status code for a bad input or a ``403`` status code for an authentication or permission error. +Any errors will use the :ref:`standard error format `, with a ``400`` status code for a bad input or a ``403`` status code for an authentication or permission error. Pass ``"alter: true`` to automatically add any missing columns to the table. This requires the :ref:`actions_alter_table` permission. @@ -1942,7 +1979,7 @@ To delete a row, make a ``POST`` to ``////-/delete``. If successful, this will return a ``200`` status code and a ``{"ok": true}`` response body. -Any errors will return ``{"errors": ["... descriptive message ..."], "ok": false}``, and a ``400`` status code for a bad input or a ``403`` status code for an authentication or permission error. +Any errors will use the :ref:`standard error format `, with a ``400`` status code for a bad input or a ``403`` status code for an authentication or permission error. .. _TableCreateView: @@ -2122,9 +2159,11 @@ If you pass a row to the create endpoint with a primary key that already exists { "ok": false, + "error": "UNIQUE constraint failed: creatures.id", "errors": [ "UNIQUE constraint failed: creatures.id" - ] + ], + "status": 400 } You can avoid this error by passing the same ``"ignore": true`` or ``"replace": true`` options to the create endpoint as you can to the :ref:`insert endpoint `. @@ -2360,7 +2399,7 @@ A successful response returns the new schema and the previous schema. If the req "operations_applied": 11 } -Any errors will return ``{"errors": ["... descriptive message ..."], "ok": false}``, and a ``400`` status code for a bad input or a ``403`` status code for an authentication or permission error. +Any errors will use the :ref:`standard error format `, with a ``400`` status code for a bad input or a ``403`` status code for an authentication or permission error. .. _TableSetColumnTypeView: @@ -2424,7 +2463,7 @@ To clear an existing column type assignment, set ``column_type`` to ``null``: This API stores the assignment in Datasette's internal database, so it can be used with immutable databases as well as mutable ones. -Any errors will return ``{"errors": ["... descriptive message ..."], "ok": false}``, and a ``400`` status code for a bad input or a ``403`` status code for an authentication or permission error. +Any errors will use the :ref:`standard error format `, with a ``400`` status code for a bad input or a ``403`` status code for an authentication or permission error. .. _TableDropView: @@ -2461,4 +2500,4 @@ If you pass the following POST body: Then the table will be dropped and a status ``200`` response of ``{"ok": true}`` will be returned. -Any errors will return ``{"errors": ["... descriptive message ..."], "ok": false}``, and a ``400`` status code for a bad input or a ``403`` status code for an authentication or permission error. +Any errors will use the :ref:`standard error format `, with a ``400`` status code for a bad input or a ``403`` status code for an authentication or permission error. diff --git a/existing-api.md b/existing-api.md index b3eda9d4..1933d287 100644 --- a/existing-api.md +++ b/existing-api.md @@ -44,46 +44,51 @@ directory: every claim below is based on the route table in `datasette/app.py` - Success content type: `application/json; charset=utf-8` (`_shape=array&_nl=on` responses use `text/plain`). -### Error shapes (there are several) +### Error shape (canonical) -The codebase produces **four distinct JSON error shapes**, depending on which -layer generates the error: +Every JSON error response uses one canonical shape, built by `error_body()` +(utils/__init__.py): -1. **Exception handler** (handle_exception.py:21-59) — used when a view raises - `NotFound`, `Forbidden` (JSON paths only — see below), `DatasetteError`, - `BadRequest` etc. and the request path ends in `.json`: +```json +{ + "ok": false, + "error": "all messages joined with '; '", + "errors": ["message", "..."], + "status": 404 +} +``` + +- `errors` is a list of one or more message strings (multi-message + validation errors, e.g. per-row insert errors, list them all). +- `error` is the messages joined with `"; "`. +- `status` always matches the HTTP status code. + +The shape is produced by four code paths, all delegating to `error_body()`: + +1. **Exception handler** (handle_exception.py) — `NotFound`, + `DatasetteError`, `BadRequest` etc. on `.json` paths. `DatasetteError` + `error_dict` context keys are merged in; the legacy `title` key is no + longer emitted in JSON (it survives in the HTML error template context). +2. **The `_error()` helper** (views/base.py:183-184) — the write API, + stored-query API, execute-write and permission-denied paths. +3. **JSON renderer errors** (renderer.py) — SQL errors on table/query + endpoints return HTTP 400 with the canonical keys **plus** the context + keys of the response it could not produce: ```json - {"ok": false, "error": "message", "status": 404, "title": null} + {"ok": false, "error": "no such table: x", "errors": ["no such table: x"], + "status": 400, "rows": [], "truncated": false} ``` -2. **The `_error()` helper** (views/base.py:183-184) — used by the write API, - stored-query API, execute-write and several permission-denied paths: + Invalid `_shape=` values and `_shape=object` misuse (on queries or + pk-less tables) also return canonical 400 errors. +4. **Permission debug endpoints** (`/-/allowed`, `/-/rules`, `/-/check`, + POST `/-/permissions`) — canonical shape (previously bare + `{"error": ...}` objects). - ```json - {"ok": false, "errors": ["message", "..."]} - ``` - - Note: plural `errors`, a list, and no `status`/`title` keys. - -3. **JSON renderer errors** (renderer.py:52-56) — SQL errors on table/query - endpoints return HTTP 400 with the error embedded in the data envelope: - - ```json - {"ok": false, "error": "no such table: x", "rows": [], "truncated": false} - ``` - - An invalid `_shape=` value produces `{"ok": false, "error": "Invalid _shape: x", - "status": 400, "title": null}` (renderer.py:101-108). - -4. **Ad-hoc `{"error": ...}` objects** — the permission debug endpoints - (`/-/allowed`, `/-/rules`, `/-/check`, POST `/-/permissions`) return e.g. - `{"error": "Unknown action: x"}` with no `ok` key (views/special.py). - -Method-not-allowed responses return HTTP 405 -`{"ok": false, "error": "Method not allowed"}` when the path ends in `.json` -or the request content type is `application/json`; plain text otherwise -(views/base.py:53, 88-98). +Method-not-allowed responses return HTTP 405 with the canonical shape when +the path ends in `.json` or the request content type is `application/json`; +plain text otherwise (views/base.py). **`Forbidden` is special:** when a view raises `Forbidden` (e.g. via `ensure_permission`), the default `forbidden()` plugin hook renders an **HTML @@ -144,11 +149,10 @@ build JSON directly): - `array` — response body is a bare JSON array of row objects - `arrayfirst` — bare JSON array of the first column's values - `object` — table views only: an object keyed by primary-key string. - On queries: `{"ok": false, "error": "_shape=object is only available on - tables"}` (with HTTP status 200); on tables without primary keys a similar - error. - - anything else — HTTP 400 `{"ok": false, "error": "Invalid _shape: x", - "status": 400, "title": null}` + On queries or tables without primary keys: a canonical 400 error + (`_shape=object is only available on tables` / + `_shape=object not available for tables with no primary keys`). + - anything else — canonical HTTP 400 error `Invalid _shape: x` - **`_nl=on`** — with `_shape=array` only: newline-delimited JSON, `text/plain`. - **`_json=COLUMN`** (repeatable) — parse that column's string values with `json.loads` so they nest as JSON; parse failures leave the value unchanged. @@ -157,7 +161,7 @@ build JSON directly): - `columns` is stripped from dict-shaped output unless `?_extra=columns` was requested (renderer.py:110-113). - If a SQL error occurred, `_shape` is ignored, HTTP status is 400 and the - envelope carries `"ok": false, "error": ...` (renderer.py:52-56). + envelope carries the canonical error keys alongside `rows`/`truncated`. ### The `?_extra=` system @@ -340,8 +344,8 @@ GET renders a confirmation page (or redirects if anonymous); POST deletes the - **POST** — form-encoded `actor` (JSON string), `permission`, optional `resource_1`, `resource_2`; returns **JSON** `{"action", "allowed", "resource": {"parent", "child", "path"}}` plus - `actor_id` when present. Errors: unknown action → 404 `{"error": ...}`; - child without parent → 400 `{"error": ...}`. + `actor_id` when present. Errors: unknown action → 404; child without + parent → 400 (both canonical error shape). ### GET /-/allowed(.json) @@ -351,7 +355,7 @@ path always renders the HTML form; `.json` returns JSON. - **Permission:** none — reports the **current actor's own** allowed resources. Items gain a `reason` field if the actor also holds `permissions-debug`. -- **Parameters:** `action` (required; missing → 400 `{"error": ...}`, unknown +- **Parameters:** `action` (required; missing → 400 canonical error, unknown → 404), `parent`, `child` (requires `parent`), `page` (default 1), `page_size` (default 50, silently capped at 200). - **Response:** `{"action", "actor_id", "page", "page_size", "total", @@ -497,7 +501,7 @@ queries section). GET → 405. Body is parsed as JSON regardless of content type; invalid JSON → 400 `{"ok": false, "errors": ["Invalid JSON: ..."]}`. -- **Permissions** (all denials → 403 `{"ok": false, "errors": [...]}`, +- **Permissions** (all denials → 403 canonical error JSON, all checked at the **database** level): - `create-table` — always required (`["Permission denied"]`) - `insert-row` — if `rows`/`row` provided (`need insert-row`) @@ -812,8 +816,8 @@ only — views get 400 `"Autocomplete is only available for tables"`. ## The write API -All write endpoints return errors via `_error()` -(`{"ok": false, "errors": [...]}`) and check permissions with +All write endpoints return errors via `_error()` (the canonical error +shape) and check permissions with `datasette.allowed()` directly, so their 403s are JSON (unlike the `Forbidden`-raising read endpoints). Routes: app.py:2719-2762. diff --git a/stable-api-recommendations.md b/stable-api-recommendations.md index e630ae4b..9eac543f 100644 --- a/stable-api-recommendations.md +++ b/stable-api-recommendations.md @@ -18,7 +18,17 @@ Findings are grouped by theme. Each carries a priority: --- -## 1. Error responses: four shapes is three too many (P1) +## 1. Error responses: four shapes is three too many (P1) — ✅ IMPLEMENTED + +> **Status:** implemented. All four shapes now delegate to a shared +> `error_body()` helper (`datasette/utils/__init__.py`) producing +> `{"ok": false, "error": "", "errors": [...], "status": }`. +> The `title` key is no longer emitted in JSON; the bare `{"error": ...}` +> debug-endpoint shape is gone; `_shape=object` misuse now returns HTTP 400 +> (part of §1b). Covered by `tests/test_error_shape.py` and documented in +> the "Error responses" section of `docs/json_api.rst`. Still open from +> this section's sub-items: §1a (`Forbidden` → HTML), the write +> canned-query 200 (§1b), and the §1c status outliers. The API currently produces four distinct JSON error shapes depending on which internal layer generates the error: @@ -348,7 +358,7 @@ Two details make tiering urgent rather than optional: ## 10. Summary of P1 items (the pre-1.0 checklist) -1. One canonical JSON error shape; retire the other three (§1). +1. ~~One canonical JSON error shape; retire the other three (§1).~~ ✅ Done. 2. `Forbidden` → JSON 403 for JSON requests (§1a). 3. No `ok: false` with HTTP 200 (§1b: `_shape=object`, write canned-query SQL errors). diff --git a/tests/test_api.py b/tests/test_api.py index f57d0206..e5ed1d23 100644 --- a/tests/test_api.py +++ b/tests/test_api.py @@ -323,20 +323,21 @@ def test_sql_time_limit(app_client_shorter_time_limit): "/fixtures/-/query.json?sql=select+sleep(0.5)", ) assert 400 == response.status + expected_message = ( + "

SQL query took too long. The time limit is controlled by the\n" + 'sql_time_limit_ms\n' + "configuration option.

\n" + '\n' + "" + ) assert response.json == { "ok": False, - "error": ( - "

SQL query took too long. The time limit is controlled by the\n" - 'sql_time_limit_ms\n' - "configuration option.

\n" - '\n' - "" - ), + "error": expected_message, + "errors": [expected_message], "status": 400, - "title": "SQL Interrupted", } @@ -350,7 +351,7 @@ async def test_custom_sql_time_limit(ds_client): "/fixtures/-/query.json?sql=select+sleep(0.01)&_timelimit=5", ) assert response.status_code == 400 - assert response.json()["title"] == "SQL Interrupted" + assert response.json()["error"].startswith("

SQL query took too long.") @pytest.mark.asyncio diff --git a/tests/test_api_write.py b/tests/test_api_write.py index 76797742..17542d4b 100644 --- a/tests/test_api_write.py +++ b/tests/test_api_write.py @@ -1,6 +1,6 @@ from datasette.app import Datasette from datasette.events import RenameTableEvent -from datasette.utils import escape_sqlite, sqlite3 +from datasette.utils import error_body, escape_sqlite, sqlite3 from .utils import last_event import pytest import time @@ -788,7 +788,12 @@ async def test_update_row_invalid_key(ds_write): headers=_headers(token), ) assert response.status_code == 400 - assert response.json() == {"ok": False, "errors": ["Invalid keys: bad_key"]} + assert response.json() == { + "ok": False, + "error": "Invalid keys: bad_key", + "errors": ["Invalid keys: bad_key"], + "status": 400, + } @pytest.mark.asyncio @@ -1103,10 +1108,9 @@ async def test_alter_table_foreign_key_requires_fk_table_for_fk_column(ds_write) headers=_headers(write_token(ds_write, permissions=["at"])), ) assert response.status_code == 400 - assert response.json() == { - "ok": False, - "errors": ["operations.0.add_foreign_key.args: fk_column requires fk_table"], - } + assert response.json() == error_body( + ["operations.0.add_foreign_key.args: fk_column requires fk_table"], 400 + ) @pytest.mark.asyncio @@ -1130,10 +1134,9 @@ async def test_alter_table_foreign_key_without_fk_column_requires_single_pk(ds_w headers=_headers(token), ) assert response.status_code == 400 - assert response.json() == { - "ok": False, - "errors": ["Could not detect single primary key for table 'accounts'"], - } + assert response.json() == error_body( + ["Could not detect single primary key for table 'accounts'"], 400 + ) @pytest.mark.asyncio @@ -1199,10 +1202,7 @@ async def test_foreign_key_suggestions_permission_denied(ds_write): headers=_headers(token), ) assert response.status_code == 403 - assert response.json() == { - "ok": False, - "errors": ["Permission denied: need alter-table"], - } + assert response.json() == error_body(["Permission denied: need alter-table"], 403) @pytest.mark.asyncio @@ -1313,10 +1313,7 @@ async def test_foreign_key_targets_permission_denied(ds_write): headers=_headers(token), ) assert response.status_code == 403 - assert response.json() == { - "ok": False, - "errors": ["Permission denied: need create-table"], - } + assert response.json() == error_body(["Permission denied: need create-table"], 403) @pytest.mark.asyncio @@ -1339,10 +1336,7 @@ async def test_alter_table_permission_denied(ds_write): headers=_headers(token), ) assert response.status_code == 403 - assert response.json() == { - "ok": False, - "errors": ["Permission denied: need alter-table"], - } + assert response.json() == error_body(["Permission denied: need alter-table"], 403) @pytest.mark.asyncio @@ -2021,6 +2015,9 @@ async def test_create_table( ) assert response.status_code == expected_status data = response.json() + if expected_response.get("ok") is False: + # Error expectations list their messages; derive the canonical envelope + expected_response = error_body(expected_response["errors"], expected_status) assert data == expected_response # Should have tracked the expected events events = ds_write._tracked_events @@ -2218,13 +2215,12 @@ async def test_create_table_column_validation(ds_write, column, expected_error): ) if expected_error: assert response.status_code == 400 - assert response.json() == {"ok": False, "errors": [expected_error]} + assert response.json() == error_body([expected_error], 400) else: assert response.status_code == 400 - assert response.json() == { - "ok": False, - "errors": ["Could not detect single primary key for table 'owners'"], - } + assert response.json() == error_body( + ["Could not detect single primary key for table 'owners'"], 400 + ) @pytest.mark.asyncio @@ -2262,10 +2258,9 @@ async def test_create_table_foreign_key_without_fk_column_requires_single_pk(ds_ headers=_headers(token), ) assert response.status_code == 400 - assert response.json() == { - "ok": False, - "errors": ["Could not detect single primary key for table 'accounts'"], - } + assert response.json() == error_body( + ["Could not detect single primary key for table 'accounts'"], 400 + ) @pytest.mark.asyncio @@ -2415,10 +2410,9 @@ async def test_create_table_error_if_pk_changed(ds_write): headers=_headers(token), ) assert second_response.status_code == 400 - assert second_response.json() == { - "ok": False, - "errors": ["pk cannot be changed for existing table"], - } + assert second_response.json() == error_body( + ["pk cannot be changed for existing table"], 400 + ) @pytest.mark.asyncio @@ -2442,10 +2436,9 @@ async def test_create_table_error_rows_twice_with_duplicates(ds_write): headers=_headers(token), ) assert second_response.status_code == 400 - assert second_response.json() == { - "ok": False, - "errors": ["UNIQUE constraint failed: test_create_twice.id"], - } + assert second_response.json() == error_body( + ["UNIQUE constraint failed: test_create_twice.id"], 400 + ) @pytest.mark.asyncio @@ -2468,6 +2461,8 @@ async def test_method_not_allowed(ds_write, path): assert response.json() == { "ok": False, "error": "Method not allowed", + "errors": ["Method not allowed"], + "status": 405, } @@ -2535,10 +2530,9 @@ async def test_create_using_alter_against_existing_table( ) if not has_alter_permission: assert response2.status_code == 403 - assert response2.json() == { - "ok": False, - "errors": ["Permission denied: need alter-table"], - } + assert response2.json() == error_body( + ["Permission denied: need alter-table"], 403 + ) else: assert response2.status_code == 201 diff --git a/tests/test_base_view.py b/tests/test_base_view.py index 2cd4d601..c1b0cf20 100644 --- a/tests/test_base_view.py +++ b/tests/test_base_view.py @@ -53,6 +53,8 @@ async def test_get_view(): assert json.loads(post_json_response.body) == { "ok": False, "error": "Method not allowed", + "errors": ["Method not allowed"], + "status": 405, } assert post_json_response.status == 405 diff --git a/tests/test_column_types.py b/tests/test_column_types.py index 45a9e7d1..4e553771 100644 --- a/tests/test_column_types.py +++ b/tests/test_column_types.py @@ -9,7 +9,7 @@ from datasette.column_types import ( ) from datasette.hookspecs import hookimpl from datasette.plugins import pm -from datasette.utils import sqlite3 +from datasette.utils import error_body, sqlite3 from datasette.utils import StartupError import markupsafe import pytest @@ -426,7 +426,7 @@ async def test_set_column_type_api_errors( kwargs["json"] = body response = await ds_ct.client.post("/data/posts/-/set-column-type", **kwargs) assert response.status_code == expected_status - assert response.json() == {"ok": False, "errors": expected_errors} + assert response.json() == error_body(expected_errors, expected_status) @pytest.mark.asyncio diff --git a/tests/test_error_shape.py b/tests/test_error_shape.py new file mode 100644 index 00000000..6747e47a --- /dev/null +++ b/tests/test_error_shape.py @@ -0,0 +1,185 @@ +""" +Tests for the canonical JSON error shape. + +Every JSON error response from Datasette should use one shape: + + { + "ok": false, + "error": "", + "errors": ["", ...], + "status": + } + +Additional context keys (for example "rows" and "truncated" on SQL errors) +are permitted, but "ok", "error", "errors" and "status" must always be +present and the legacy "title" key must not be. + +https://github.com/simonw/datasette/issues - 1.0 API consistency +""" + +import pytest +from datasette.app import Datasette +from datasette.utils import sqlite3 + + +def assert_canonical_error(response, expected_status): + assert response.status_code == expected_status + data = response.json() + assert data["ok"] is False + assert isinstance(data["error"], str) + assert data["error"] + assert isinstance(data["errors"], list) + assert data["errors"] + assert all(isinstance(message, str) for message in data["errors"]) + assert data["error"] == "; ".join(data["errors"]) + assert data["status"] == expected_status + assert "title" not in data + return data + + +@pytest.fixture +def ds_error_shape(tmp_path_factory): + db_directory = tmp_path_factory.mktemp("dbs") + db_path = str(db_directory / "data.db") + conn = sqlite3.connect(db_path) + conn.execute("vacuum") + conn.execute("create table docs (id integer primary key, title text)") + conn.close() + ds = Datasette([db_path]) + ds.root_enabled = True + yield ds + ds.close() + + +# Shape 1: the exception handler (handle_exception.py) + + +@pytest.mark.asyncio +async def test_not_found_error_shape(ds_client): + response = await ds_client.get("/fixtures/no_such_table.json") + assert_canonical_error(response, 404) + + +@pytest.mark.asyncio +async def test_datasette_error_with_title_omits_title_key(ds_client): + # DatasetteError(title="Invalid SQL") previously leaked a "title" key + response = await ds_client.get( + "/fixtures/-/query.json?sql=update+facetable+set+state+=+1" + ) + data = assert_canonical_error(response, 400) + assert data["errors"] == ["Statement must be a SELECT"] + + +# Shape 2: the _error() helper (views/base.py) - write API and friends + + +@pytest.mark.asyncio +async def test_write_api_validation_error_shape(ds_error_shape): + token = "dstok_{}".format( + ds_error_shape.sign( + {"a": "root", "token": "dstok", "t": 0}, + namespace="token", + ) + ) + response = await ds_error_shape.client.post( + "/data/docs/-/insert", + json={"rows": [{"nope": 1}, {"also_nope": 2}]}, + headers={ + "Authorization": "Bearer {}".format(token), + "Content-Type": "application/json", + }, + ) + data = assert_canonical_error(response, 400) + # Multiple messages: errors keeps them all, error joins them + assert len(data["errors"]) == 2 + assert data["errors"][0].startswith("Row 0") + assert data["errors"][1].startswith("Row 1") + + +@pytest.mark.asyncio +async def test_write_api_permission_denied_shape(ds_error_shape): + response = await ds_error_shape.client.post( + "/data/docs/-/insert", + json={"rows": [{"title": "hello"}]}, + headers={"Content-Type": "application/json"}, + ) + assert_canonical_error(response, 403) + + +# Shape 3: the JSON renderer (renderer.py) + + +@pytest.mark.asyncio +async def test_sql_error_shape_keeps_context_keys(ds_client): + response = await ds_client.get( + "/fixtures/-/query.json?sql=select+*+from+no_such_table" + ) + data = assert_canonical_error(response, 400) + # Renderer errors keep their context keys + assert data["rows"] == [] + assert "truncated" in data + + +@pytest.mark.asyncio +async def test_invalid_shape_error_shape(ds_client): + response = await ds_client.get("/fixtures/-/query.json?sql=select+1&_shape=bananas") + data = assert_canonical_error(response, 400) + assert data["errors"] == ["Invalid _shape: bananas"] + + +@pytest.mark.asyncio +async def test_shape_object_on_query_is_a_400_error(ds_client): + # Previously returned HTTP 200 with an ok: false body + response = await ds_client.get("/fixtures/-/query.json?sql=select+1&_shape=object") + data = assert_canonical_error(response, 400) + assert data["errors"] == ["_shape=object is only available on tables"] + + +# Shape 4: bare {"error": ...} from the permission debug endpoints + + +@pytest.mark.asyncio +async def test_allowed_missing_action_error_shape(ds_client): + response = await ds_client.get("/-/allowed.json") + data = assert_canonical_error(response, 400) + assert data["errors"] == ["action parameter is required"] + + +@pytest.mark.asyncio +async def test_allowed_unknown_action_error_shape(ds_client): + response = await ds_client.get("/-/allowed.json?action=no_such_action") + assert_canonical_error(response, 404) + + +@pytest.mark.asyncio +async def test_check_unknown_action_error_shape(ds_error_shape): + response = await ds_error_shape.client.get( + "/-/check.json?action=no_such_action", + actor={"id": "root"}, + ) + assert_canonical_error(response, 404) + + +@pytest.mark.asyncio +async def test_rules_missing_action_error_shape(ds_error_shape): + response = await ds_error_shape.client.get( + "/-/rules.json", + actor={"id": "root"}, + ) + data = assert_canonical_error(response, 400) + assert data["errors"] == ["action parameter is required"] + + +# Other stragglers + + +@pytest.mark.asyncio +async def test_method_not_allowed_error_shape(ds_client): + response = await ds_client.post("/fixtures.json") + assert_canonical_error(response, 405) + + +@pytest.mark.asyncio +async def test_schema_unknown_database_error_shape(ds_client): + response = await ds_client.get("/no_such_db/-/schema.json") + assert_canonical_error(response, 404) diff --git a/tests/test_table_api.py b/tests/test_table_api.py index 272e39e3..c8ba31b7 100644 --- a/tests/test_table_api.py +++ b/tests/test_table_api.py @@ -31,8 +31,8 @@ async def test_table_not_exists_json(ds_client): assert (await ds_client.get("/fixtures/blah.json")).json() == { "ok": False, "error": "Table not found", + "errors": ["Table not found"], "status": 404, - "title": None, } @@ -242,8 +242,8 @@ async def test_table_shape_invalid(ds_client): assert response.json() == { "ok": False, "error": "Invalid _shape: invalid", + "errors": ["Invalid _shape: invalid"], "status": 400, - "title": None, } @@ -635,8 +635,8 @@ async def test_searchable_invalid_column(ds_client): assert response.json() == { "ok": False, "error": "Cannot search by that column", + "errors": ["Cannot search by that column"], "status": 400, - "title": None, } @@ -775,7 +775,7 @@ async def test_table_filter_extra_where_invalid(ds_client): "/fixtures/facetable.json?_where=_neighborhood=Dogpatch'" ) assert response.status_code == 400 - assert "Invalid SQL" == response.json()["title"] + assert "unrecognized token" in response.json()["error"] def test_table_filter_extra_where_disabled_if_no_sql_allowed(): diff --git a/tests/test_table_html.py b/tests/test_table_html.py index 3af2bb08..46d43c6c 100644 --- a/tests/test_table_html.py +++ b/tests/test_table_html.py @@ -1979,8 +1979,8 @@ async def test_sort_errors(ds_client, json, params, error): assert response.json() == { "ok": False, "error": error, + "errors": [error], "status": 400, - "title": None, } else: assert error in response.text