diff --git a/CHANGELOG.md b/CHANGELOG.md index 47f1eaca3..b5d74f4d9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -98,7 +98,6 @@ and this project adheres to on its own - 🔧(dev) generate the JWT signing key of the collaboration server when bootstrapping the dev stack, alongside the backend one -- ✨(collaboration) add a get-ydoc endpoint on yhub - ✨(backend) serve `documents/{id}/formatted-content/` from yhub - ✨(backend) duplicate a document through the collaboration server - ✨(backend) call YHubService to seed initial document content @@ -139,6 +138,11 @@ and this project adheres to identity of the user the sandbox is created for. A collaboration server that cannot be reached skips the sandbox, as a missing template already did, and never fails the signup +- ♻️(backend) read the content of a document from the built-in `ydoc` endpoint + of the collaboration server: `YHubService` asks for JSON, which yhub speaks + since 0.5.0, so the custom `get-ydoc` endpoint it used to need is gone. + `create-ydoc` stays, no built-in offers what it does — a strict create, and + content credited to the user rather than to the backend - 🔥(backend) remove the unused `CollaborationService` - 💥(backend) remove the `documents/{id}/content/` endpoint - 💥(backend) remove the `documents/{id}/can-edit/` endpoint diff --git a/src/backend/core/services/yhub_services.py b/src/backend/core/services/yhub_services.py index fed8c3ce5..c67758f10 100644 --- a/src/backend/core/services/yhub_services.py +++ b/src/backend/core/services/yhub_services.py @@ -13,6 +13,12 @@ update), `rollback`, `prune`, `changeset` and `activity`, all at `v1`. yhub also accepts a `branch` query parameter, but our auth plugin only ever grants access to the `main` branch, so this service never sends it. +Since yhub 0.5.0 those endpoints answer JSON to a request asking for it, with +the binary fields base64 encoded, so this service sends `Accept: +application/json` and reads them without a lib0 decoder. Their errors come back +the same way, as a JSON `{"error": ...}` this service reports along with the +status. + A few routes are about the server itself rather than about a document, and carry no room: `/{prefix}/jwks/{version}` publishes the public keys validating the tokens yhub signs to call us back. @@ -21,6 +27,7 @@ This service only owns the transport for now, the endpoints are added as we need them. """ +import base64 import logging from django.conf import settings @@ -72,6 +79,11 @@ class YHubService: # Version of the endpoints we call, the one all the built-ins are at. api_version = "v1" + # A Yjs update carrying no content encodes to 2 bytes, and yhub reads + # anything up to 3 as an empty document. `ydoc` answers the encoding of an + # empty document for a room it holds nothing for, never an empty body. + empty_update_max_bytes = 3 + def __init__(self, user=None): """Bind the service to the user a call is made on behalf of, if any.""" self.user = user @@ -173,7 +185,7 @@ class YHubService: def request(self, method, url, data=None, headers=None): """ - Send an authenticated request to the yhub API. + Send an authenticated request to the yhub API, asking it for JSON. Return the raw response, it is up to the caller to decode its body: the endpoints do not all answer with the same payload. @@ -185,7 +197,8 @@ class YHubService: data=data, headers={ "Authorization": self.auth_header, - "Content-Type": "application/octet-stream", + # what makes yhub answer JSON rather than its lib0 encoding + "Accept": "application/json", **(headers or {}), }, timeout=self.timeout, @@ -203,44 +216,73 @@ class YHubService: response.status_code, response.text[:200] if response.text else "empty", ) + detail = self.json_body(response).get("error") raise APIError( - f"The yhub API answered {response.status_code} on {url}", + f"The yhub API answered {response.status_code} on {url}" + + (f": {detail}" if detail else ""), status_code=response.status_code, ) return response + @staticmethod + def json_body(response): + """ + Return the JSON body of a response, an empty dict when it has none. + + yhub reports its errors as `{"error": ...}` and its endpoints answer + JSON, but a failure can also come from something else on the way (a + proxy, a gateway): what it says is a bonus, never something to fail on. + """ + try: + body = response.json() + except ValueError: + return {} + + return body if isinstance(body, dict) else {} + def get_ydoc(self, document): """ Return the current Yjs state of a document, None when it has none. The raw update is what `create_ydoc` takes, so the state of a document - can be copied into another one. The built-in `ydoc` endpoint is not - used, it answers the lib0 encoding of an envelope rather than the - update itself. + can be copied into another one. The built-in `ydoc` endpoint answers + `{"doc": ...}`, the update base64 encoded, and the encoding of an empty + document for a room it holds no content for. """ - response = self.request("get", self.build_url("get-ydoc", document)) + response = self.request("get", self.build_url("ydoc", document)) - return response.content or None + try: + update = base64.b64decode(self.json_body(response)["doc"]) + except (KeyError, TypeError, ValueError) as err: + raise APIError( + f"The yhub API answered no readable document on {response.url}" + ) from err + + return update if len(update) > self.empty_update_max_bytes else None def create_ydoc(self, document, update): """ Seed the initial Yjs state of a document. The body is the raw binary update, what pycrdt's `get_update()` - returns, and not the lib0 encoding the built-in `ydoc` endpoint speaks. - The content is attributed to the user the service is bound to, yhub - only takes our word for it because the token grants admin. + returns. This is not the built-in `ydoc` endpoint, which would take the + same update base64 encoded but knows nothing of the two things this one + is for: it is a strict create, and it attributes the content to the + user the service is bound to rather than to the backend calling it. - It is a strict create: yhub answers 409 when the document already has - content, 413 over 10MB and 400 on an update it cannot apply, all - reported as an `APIError` carrying the status. + yhub answers 409 when the document already has content, 413 over 10MB + and 400 on an update it cannot apply, all reported as an `APIError` + carrying the status. """ return self.request( "post", self.build_url("create-ydoc", document), data=update, - headers=self.build_user_header(self.user_id), + headers={ + "Content-Type": "application/octet-stream", + **self.build_user_header(self.user_id), + }, ) def reset_connections(self, document, user_id=None): diff --git a/src/backend/core/tests/test_services_yhub_services.py b/src/backend/core/tests/test_services_yhub_services.py index 08d08bf57..ffb90cc41 100644 --- a/src/backend/core/tests/test_services_yhub_services.py +++ b/src/backend/core/tests/test_services_yhub_services.py @@ -1,5 +1,6 @@ """Test yhub services.""" +from base64 import b64encode from unittest.mock import patch from uuid import uuid4 @@ -127,7 +128,8 @@ def test_request(mock_request): assert kwargs["data"] == b"body" assert kwargs["timeout"] == 30 assert kwargs["headers"]["Authorization"].startswith("Bearer ") - assert kwargs["headers"]["Content-Type"] == "application/octet-stream" + # asked of every endpoint: yhub answers its lib0 encoding otherwise + assert kwargs["headers"]["Accept"] == "application/json" @patch("requests.request") @@ -236,23 +238,67 @@ def test_reset_connections_of_a_single_user(mock_request): def test_get_ydoc(mock_request): """Should return the raw update the collaboration server holds.""" mock_request.return_value.ok = True - mock_request.return_value.content = b"\x01\x02raw yjs update" + mock_request.return_value.json.return_value = { + "doc": b64encode(b"\x01\x02raw yjs update").decode() + } update = YHubService().get_ydoc(DOCUMENT) assert update == b"\x01\x02raw yjs update" - args, _kwargs = mock_request.call_args + args, kwargs = mock_request.call_args + # the built-in endpoint, which answers the update base64 encoded in json assert args == ( "get", - f"http://yhub:3002/collaboration/get-ydoc/v1/docs/{DOCUMENT.id!s}", + f"http://yhub:3002/collaboration/ydoc/v1/docs/{DOCUMENT.id!s}", ) + assert kwargs["headers"]["Accept"] == "application/json" @patch("requests.request") def test_get_ydoc_without_content(mock_request): """A document the collaboration server holds no content for should return None.""" mock_request.return_value.ok = True - # yhub answers 204 No Content, hence an empty body - mock_request.return_value.content = b"" + # a room with no content answers the encoding of an empty document + mock_request.return_value.json.return_value = { + "doc": b64encode(b"\x00\x00").decode() + } assert YHubService().get_ydoc(DOCUMENT) is None + + +@patch("requests.request") +def test_get_ydoc_unreadable_answer(mock_request): + """An answer we cannot read a document out of should raise, never look empty.""" + mock_request.return_value.ok = True + mock_request.return_value.json.return_value = {"unexpected": "payload"} + + with pytest.raises(APIError): + YHubService().get_ydoc(DOCUMENT) + + +@patch("requests.request") +def test_request_error_reports_what_yhub_said(mock_request): + """The message yhub puts in its json error should travel with the status.""" + mock_request.return_value.ok = False + mock_request.return_value.status_code = 409 + mock_request.return_value.text = '{"error": "Document already exists"}' + mock_request.return_value.json.return_value = {"error": "Document already exists"} + + with pytest.raises(APIError, match="Document already exists") as excinfo: + YHubService().create_ydoc(DOCUMENT, b"\x01\x02update") + + assert excinfo.value.status_code == 409 + + +@patch("requests.request") +def test_request_error_without_json_body(mock_request): + """An error from something else on the way should be reported all the same.""" + mock_request.return_value.ok = False + mock_request.return_value.status_code = 502 + mock_request.return_value.text = "Bad Gateway" + mock_request.return_value.json.side_effect = ValueError("not json") + + with pytest.raises(APIError, match="answered 502") as excinfo: + YHubService().get_ydoc(DOCUMENT) + + assert excinfo.value.status_code == 502