.json errors should be returned as JSON

[simonw]

Eg the error in this issue:

• view-instance should not be checked for /-/actor.json #1945

[simonw]

Most .json errors DO return as JSON, thanks to this:

datasette/datasette/handle_exception.py

Lines 19 to 24 in c094dde

	@hookimpl(trylast=True)
	def handle_exception(datasette, request, exception):
	async def inner():
	if datasette.pdb:
	pdb.post_mortem(exception.__traceback__)

datasette/datasette/handle_exception.py

Lines 57 to 58 in c094dde

	if request.path.split("?")[0].endswith(".json"):
	return Response.json(info, status=status, headers=headers)

But that code triggers when the URL ends with .json - and none of the JSON write API endpoints (things like /db/-/create and /db/table/-/insert) follow that convention.

[simonw]

I need a way for those JSON endpoints to communicate back to the handle_exception handler that they are returning JSON, so it knows to behave differently.

Since it gets the request object, one way could be to have view code set request.should_return_json = True so that the handler knows to do something different.

It's a bit of a cludge though!

[simonw]

I'm going to prototype that up to see what it looks like.

[simonw]

This raises a more complicated issue

At some point I'm likely to want to add an HTML interface for creating tables and inserting and updating rows.

The obvious URLs for that are the same as for the JSON API: /db/table/-/insert and suchlike.

Those endpoints are currently POST only - and can return JSON all the time.

If they start accepting form POSTs too they'll need to be able to accept form-encoded data and return HTML instead. That's OK - they can detect incoming JSON thanks to the content-type header an the fact that the request body starts with { - but the should_return_json fix described above could intefere with how errors are returned if I'm not careful.

I think it can still work though: I'll only set should_return_json = True if the endpoint gets a POST with a body starting {, or a content-type JSON header.

[simonw]

Sniffing for a { is a little bit tricky though, as the post body is lazily loaded on request here:

datasette/datasette/utils/asgi.py

Lines 127 to 135 in 9ad76d2

	async def post_body(self):
	body = b""
	more_body = True
	while more_body:
	message = await self.receive()
	assert message["type"] == "http.request", message
	body += message.get("body", b"")
	more_body = message.get("more_body", False)
	return body

[simonw]

Easiest fix would be to look for accept: application/json and/or content-type: application/json headers.

Not bullet-proof, so people might occasionally make JSON requests and get back an HTML error - but the documentation can tell people that they need to send those headers if they want to reliably get back JSON error messages.

I'm happy with this as a solution.

[simonw]

Looks like the code I've written for permission checking on TableCreateView and friends doesn't use the regular raise Forbidden or raise DatasetteError mechanisms - it does its own thing here:

datasette/datasette/views/database.py

Lines 580 to 584 in 9ad76d2

	# Must have create-table permission
	if not await self.ds.permission_allowed(
	request.actor, "create-table", resource=database_name
	):
	return _error(["Permission denied"], 403)

Which uses this:

datasette/datasette/views/base.py

Lines 547 to 548 in 9ad76d2

	def _error(messages, status=400):
	return Response.json({"ok": False, "errors": messages}, status=status)

Having two different patterns to return errors is bad, I should fix that.

[simonw]

Also weird: errors returned by that mechanism look like this:

{
  "ok": false,
  "errors": ["list of error messages"]
}

While errors returned by the rest of Datasette look like this: https://latest.datasette.io/fixtures/no_table.json

{
  "ok": false,
  "error": "Table not found: no_table",
  "status": 404,
  "title": null
}

Related:

• Figure out design for JSON errors (consider RFC 7807) #1875

[simonw]

I fixed this issue to help research this further:

• Demo has gone stale datasette-ripgrep#26

Now this search works:

https://ripgrep.datasette.io/-/ripgrep?pattern=return+_error&literal=on&glob=datasette%2F**

I wish I had this feature!

• Copy results as Markdown feature datasette-ripgrep#24

Looks like I have both _error() and _errors() functions in there!

[simonw]

I got this far:

diff --git a/datasette/handle_exception.py b/datasette/handle_exception.py
index 8b7e83e3..31d41e00 100644
--- a/datasette/handle_exception.py
+++ b/datasette/handle_exception.py
@@ -54,7 +54,17 @@ def handle_exception(datasette, request, exception):
         headers = {}
         if datasette.cors:
             add_cors_headers(headers)
-        if request.path.split("?")[0].endswith(".json"):
+        # Return JSON error under certain conditions
+        should_return_json = (
+            # URL ends in .json
+            request.path.split("?")[0].endswith(".json")
+            or
+            # Hints from incoming request headers
+            request.headers.get("content-type") == "application/json"
+            or "application/json" in request.headers.get("accept", "")
+        )
+        breakpoint()
+        if should_return_json:
             return Response.json(info, status=status, headers=headers)
         else:
             template = datasette.jinja_env.select_template(templates)
diff --git a/tests/test_api_write.py b/tests/test_api_write.py
index f27d143f..982543a6 100644
--- a/tests/test_api_write.py
+++ b/tests/test_api_write.py
@@ -1140,6 +1140,38 @@ async def test_create_table_permissions(
         assert data["errors"] == expected_errors
 
 
+@pytest.mark.asyncio
+@pytest.mark.parametrize(
+    "headers,expect_json",
+    (
+        ({}, False),
+        ({"Accept": "text/html"}, True),
+        ({"Accept": "application/json"}, True),
+        ({"Content-Type": "application/json"}, True),
+        ({"Accept": "application/json, text/plain, */*"}, True),
+        ({"Content-Type": "application/json"}, True),
+        ({"accept": "application/json, text/plain, */*"}, True),
+        ({"content-type": "application/json"}, True),
+    ),
+)
+async def test_permission_errors_html_and_json(ds_write, headers, expect_json):
+    request_headers = {"Authorization": "Bearer bad_token"}
+    request_headers.update(headers)
+    response = await ds_write.client.post(
+        "/data/-/create",
+        json={},
+        headers=request_headers,
+    )
+    assert response.status_code == 403
+    if expect_json:
+        data = response.json()
+        assert data["ok"] is False
+        assert data["errors"] == ["Permission denied"]
+    else:
+        assert response.headers["Content-Type"] == "text/html; charset=utf-8"
+        assert "Permission denied" in response.text
+
+
 @pytest.mark.asyncio
 @pytest.mark.parametrize(
     "input,expected_rows_after",

Then decided I would punt this until the next milestone.