mirror of
https://github.com/simonw/datasette.git
synced 2026-07-09 17:14:35 +02:00
Unify JSON error responses into one canonical shape
All JSON error responses now use a single format built by the new
datasette.utils.error_body() helper:
{"ok": false, "error": "...", "errors": ["..."], "status": 400}
- error is all messages joined with '; ', errors is the full list,
status always matches the HTTP status code
- The exception handler no longer emits the legacy title key in JSON
(it is still available to the HTML error template)
- The permission debug endpoints (/-/allowed, /-/rules, /-/check,
POST /-/permissions) no longer return bare {"error": ...} objects
- JSON renderer SQL errors keep their rows/truncated context keys but
now include the canonical keys as well
- _shape=object misuse (queries or tables without primary keys) now
returns HTTP 400 instead of 200 with an error body
- Method-not-allowed 405 responses use the canonical shape
Adds tests/test_error_shape.py covering all four previous shape
producers, updates affected tests, and documents the format in a new
'Error responses' section of docs/json_api.rst.
Implements section 1 of stable-api-recommendations.md.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GrHZSypDfMnym1tM5XJAFZ
This commit is contained in:
parent
8985ecf438
commit
0679e04bd3
15 changed files with 429 additions and 169 deletions
|
|
@ -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 = (
|
||||
"<p>SQL query took too long. The time limit is controlled by the\n"
|
||||
'<a href="https://docs.datasette.io/en/stable/settings.html#sql-time-limit-ms">sql_time_limit_ms</a>\n'
|
||||
"configuration option.</p>\n"
|
||||
'<textarea style="width: 90%">select sleep(0.5)</textarea>\n'
|
||||
"<script>\n"
|
||||
'let ta = document.querySelector("textarea");\n'
|
||||
'ta.style.height = ta.scrollHeight + "px";\n'
|
||||
"</script>"
|
||||
)
|
||||
assert response.json == {
|
||||
"ok": False,
|
||||
"error": (
|
||||
"<p>SQL query took too long. The time limit is controlled by the\n"
|
||||
'<a href="https://docs.datasette.io/en/stable/settings.html#sql-time-limit-ms">sql_time_limit_ms</a>\n'
|
||||
"configuration option.</p>\n"
|
||||
'<textarea style="width: 90%">select sleep(0.5)</textarea>\n'
|
||||
"<script>\n"
|
||||
'let ta = document.querySelector("textarea");\n'
|
||||
'ta.style.height = ta.scrollHeight + "px";\n'
|
||||
"</script>"
|
||||
),
|
||||
"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("<p>SQL query took too long.")
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
185
tests/test_error_shape.py
Normal file
185
tests/test_error_shape.py
Normal file
|
|
@ -0,0 +1,185 @@
|
|||
"""
|
||||
Tests for the canonical JSON error shape.
|
||||
|
||||
Every JSON error response from Datasette should use one shape:
|
||||
|
||||
{
|
||||
"ok": false,
|
||||
"error": "<all messages joined with '; '>",
|
||||
"errors": ["<message>", ...],
|
||||
"status": <int matching the HTTP status code>
|
||||
}
|
||||
|
||||
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)
|
||||
|
|
@ -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():
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue