diff --git a/CHANGELOG.md b/CHANGELOG.md index 7f09d9ca3..9287352e1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -119,9 +119,8 @@ and this project adheres to endpoints since the history panel started reading the collaboration server's `activity` and `changeset` routes instead. Gone with them: `Document.get_versions_slice`, `Document.delete_version`, the `version_id` - argument of - `get_content_response`, the `DOCUMENT_VERSIONS_PAGE_SIZE` setting, and the - `versions_retrieve` and `versions_destroy` abilities + argument of `get_content_response`, the `DOCUMENT_VERSIONS_PAGE_SIZE` + setting, and the `versions_retrieve` and `versions_destroy` abilities The `versions_list` ability stays and keeps its meaning — may this user see this document's history — now as a pure gate rather than the permission for @@ -129,6 +128,21 @@ and this project adheres to collaboration server reads it to decide whether to ask this backend when the caller's access began, which is where the history it serves starts +- 🔥(backend) remove `Document.content`. The collaboration server has owned the + content of the documents since the migration, and the version endpoints + removed above were the last thing in Django that read the legacy `{id}/file` + object; nothing had written it for as long. Gone with the property: its + setter, `Document.save_content`, `Document.get_content_response` and the + `save()` override that existed only to write the content — a document is + saved by `Model.save` alone now, and creating one no longer costs a `HEAD` + and a `PUT` against the object storage + + The objects are untouched and must stay: the collaboration server seeds a + room from `{id}/file` on first access (`SOFT_MIGRATION`) and replays its + versions to rebuild the history. `Document.file_key` stays with them, and + `clean_document` goes on purging that object so that a document it reset + cannot be seeded back from what it left behind + ### Fixed - 🐛(frontend) stop reconnecting to the collaboration server when it has refused diff --git a/UPGRADE.md b/UPGRADE.md index 2a48a7237..018fcec78 100644 --- a/UPGRADE.md +++ b/UPGRADE.md @@ -85,8 +85,9 @@ upgrade, in the order they are done, and end with the API changes. 2. Then backfill the corpus, which the lazy seeding never finishes on its own — a document nobody opens stays in S3 forever. `python manage.py migrate_documents` hands every document to the collaboration server, which - replays its **full** S3 version history rather than its last snapshot, so - `/documents/{id}/versions/` and the history the editor shows agree. It is + replays its **full** S3 version history rather than its last snapshot, + so the history the editor shows reaches back to the document's first save + instead of beginning the day it reached the collaboration server. It is bounded (`--concurrency`, `--rate`, `--limit`, `--created-before`), resumable and safe to re-run: what became of every document is recorded in the new `impress_document_migration` table, a document the server refused @@ -98,9 +99,18 @@ upgrade, in the order they are done, and end with the API changes. migrated — but an unmigrated document then opens as an *empty* room over content that is alive in S3. - Keep the media bucket, its objects and its versioning either way: - `/documents/{id}/versions/` still serves the version history from there, and - the full migration replays it. + The media bucket itself is not going anywhere: the attachments of the + documents live in it, under `{document-id}/attachments/`, and the backend + goes on uploading them, signing urls for them and scanning them through the + S3 API. What this upgrade adds to it is temporary, and has to be kept either + way: the legacy `{document-id}/file` objects **and their versions**. Django + has let go of that key — it neither reads nor writes it anymore, and the + endpoints that used to serve its versions are removed below — but the two + steps above are what read it now, the lazy seed taking its newest version and + the backfill replaying every one of them. They are the whole record of what + the documents were before the collaboration server, so leave the bucket + versioned and those objects in place until the backfill has covered the + corpus; there is no way back from deleting them. - ⚠️ **The websocket url changed**, and so does what `/collaboration/` is routed to. The room is appended by the client, and the last segment of the @@ -242,6 +252,22 @@ upgrade, in the order they are done, and end with the API changes. it. `/api/v1.0/documents/{document_id}/formatted-content/` is not affected. The `CONTENT_METADATA_CACHE_TIMEOUT` setting only tuned the cache of the removed `GET` and is no longer read, you can drop it from your configuration. +- The endpoints `GET /api/v1.0/documents/{document_id}/versions/` and `GET, + DELETE /api/v1.0/documents/{document_id}/versions/{version_id}/` are removed. + They listed and served the S3 object versions of `{document_id}/file`, the + key the content of a document used to be stored under; nothing has written it + since the content moved to the collaboration server, so what they listed was + frozen at each document's migration date. The editor reads the history from + the collaboration server instead. If you integrate with Docs, stop calling + them: the `versions_retrieve` and `versions_destroy` abilities disappear from + the document payload along with them, and the `DOCUMENT_VERSIONS_PAGE_SIZE` + setting is no longer read, you can drop it from your configuration. + + The `versions_list` ability stays, and keeps its meaning — may this user see + this document's history. It is a gate now rather than the permission for an + endpoint of ours: the frontend hides the history menu without it, and the + collaboration server reads it to decide whether to ask this backend when the + caller's access began, which is where the history it serves starts. - The JWKS of the resource server moved from `/api/{version}/jwks` to `/external_api/{version}/jwks`, alongside the rest of the resource server endpoints. `/api/{version}/jwks` now publishes the public key validating the diff --git a/src/backend/core/factories.py b/src/backend/core/factories.py index d918c073f..7d3d49054 100644 --- a/src/backend/core/factories.py +++ b/src/backend/core/factories.py @@ -32,8 +32,8 @@ YDOC_HELLO_WORLD_BASE64 = ( # The same document as the raw Yjs update the collaboration server serves, which # is what a test faking `YHubService.get_ydoc` answers with (see the -# `yhub_content` fixture). The base64 above is the legacy object storage format, -# only the tests still about that storage have a use for it. +# `yhub_content` fixture). The base64 above is what a test expecting the text of +# that document reads it from. YDOC_HELLO_WORLD_UPDATE = base64.b64decode(YDOC_HELLO_WORLD_BASE64) @@ -93,8 +93,7 @@ class DocumentFactory(factory.django.DjangoModelFactory): excerpt = factory.Sequence(lambda n: f"excerpt{n}") # No content: the collaboration server holds it, and a document built here # is one it knows nothing of. A test needing a document with content fakes - # what the collaboration server serves for it (`YHubService.get_ydoc`), and - # only the ones about the legacy object storage itself pass `content=`. + # what the collaboration server serves for it (`YHubService.get_ydoc`). creator = factory.SubFactory(UserFactory) deleted_at = None link_reach = factory.fuzzy.FuzzyChoice( diff --git a/src/backend/core/models.py b/src/backend/core/models.py index 483eabc7d..3e9c06f4e 100644 --- a/src/backend/core/models.py +++ b/src/backend/core/models.py @@ -4,7 +4,6 @@ Declare and configure the models for the impress core application # pylint: disable=too-many-lines -import hashlib import smtplib import uuid from datetime import timedelta @@ -17,8 +16,6 @@ from django.contrib.postgres.fields import ArrayField from django.contrib.postgres.indexes import GinIndex from django.contrib.sites.models import Site from django.core.cache import cache -from django.core.files.base import ContentFile -from django.core.files.storage import default_storage from django.core.mail import send_mail from django.db import models, transaction from django.db.models import Count @@ -29,7 +26,6 @@ from django.utils.functional import cached_property from django.utils.translation import get_language, override from django.utils.translation import gettext_lazy as _ -from botocore.exceptions import ClientError from rest_framework.exceptions import ValidationError from timezone_field import TimeZoneField from treebeard.mp_tree import MP_Node, MP_NodeManager, MP_NodeQuerySet @@ -955,7 +951,7 @@ class DocumentManager(MP_NodeManager.from_queryset(DocumentQuerySet)): # pylint: disable=too-many-public-methods class Document(MP_Node, BaseModel): - """Pad document carrying the content.""" + """Pad document, the content of which lives in the collaboration server.""" title = models.CharField(_("title"), max_length=255, null=True, blank=True) excerpt = models.TextField(_("excerpt"), max_length=300, null=True, blank=True) @@ -993,8 +989,6 @@ class Document(MP_Node, BaseModel): null=True, ) - _content = None - # Tree structure alphabet = "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz" steplen = 7 # nb siblings max: 3,521,614,606,208 @@ -1033,39 +1027,6 @@ class Document(MP_Node, BaseModel): self._ancestors_link_definition = None self._computed_link_definition = None - def save(self, *args, **kwargs): - """Write content to object storage only if _content has changed.""" - super().save(*args, **kwargs) - if self._content: - self.save_content(self._content) - - def save_content(self, content): - """Save content to object storage.""" - - file_key = self.file_key - bytes_content = content.encode("utf-8") - - # Attempt to directly check if the object exists using the storage client. - try: - response = default_storage.connection.meta.client.head_object( - Bucket=default_storage.bucket_name, Key=file_key - ) - except ClientError as excpt: - # If the error is a 404, the object doesn't exist, so we should create it. - if excpt.response["Error"]["Code"] == "404": - has_changed = True - else: - raise - else: - # Compare the existing ETag with the MD5 hash of the new content. - has_changed = ( - response["ETag"].strip('"') != hashlib.md5(bytes_content).hexdigest() # noqa: S324 - ) - - if has_changed: - content_file = ContentFile(bytes_content) - default_storage.save(file_key, content_file) - def is_leaf(self): """ :returns: True if the node is has no children @@ -1083,35 +1044,17 @@ class Document(MP_Node, BaseModel): @property def file_key(self): - """Key of the object storage file to which the document content is stored""" + """ + Key of the legacy object storage file that used to hold the content. + + The collaboration server owns the content now, and Django neither reads + nor writes this object any more. The key outlives it: the collaboration + server seeds a room from that object on first access and replays its + versions to rebuild the history, and `clean_document` purges it so that + a document it reset cannot be seeded back from what it left behind. + """ return f"{self.key_base}/file" - @property - def content(self): - """Return the json content from object storage if available""" - if self._content is None and self.id: - try: - response = self.get_content_response() - except FileNotFoundError, ClientError: - pass - else: - self._content = response["Body"].read().decode("utf-8") - return self._content - - @content.setter - def content(self, content): - """Cache the content, don't write to object storage yet""" - if not isinstance(content, str): - raise ValueError("content should be a string.") - - self._content = content - - def get_content_response(self): - """Get the content of the document from object storage""" - return default_storage.connection.meta.client.get_object( - Bucket=default_storage.bucket_name, Key=self.file_key - ) - def get_nb_accesses_cache_key(self): """Generate a unique cache key for each document.""" return f"document_{self.id!s}_nb_accesses" diff --git a/src/backend/core/tests/documents/test_api_documents_children_create.py b/src/backend/core/tests/documents/test_api_documents_children_create.py index 679edca8f..8cb92dd70 100644 --- a/src/backend/core/tests/documents/test_api_documents_children_create.py +++ b/src/backend/core/tests/documents/test_api_documents_children_create.py @@ -354,7 +354,6 @@ def test_api_documents_children_create_with_docx_file_success( children = Document.objects.get(pk=response.json()["id"]) assert children.title == "My Important Document.docx" # the content is saved by the collaboration server, not by Django - assert children.content is None mock_yhub.assert_called_once_with(user=user) mock_yhub.return_value.create_ydoc.assert_called_once_with(children, converted_yjs) diff --git a/src/backend/core/tests/documents/test_api_documents_content_updated.py b/src/backend/core/tests/documents/test_api_documents_content_updated.py index fa7b5ac69..dd093d3de 100644 --- a/src/backend/core/tests/documents/test_api_documents_content_updated.py +++ b/src/backend/core/tests/documents/test_api_documents_content_updated.py @@ -222,8 +222,7 @@ def test_api_documents_content_updated_rolled_key(yhub_jwks): def test_api_documents_content_updated(): """The collaboration server should be able to refresh the date of a document.""" with freeze_time("2026-08-01 12:00:00"): - # no content: writing one to S3 under a frozen clock breaks its signature - document = factories.DocumentFactory(title="my document", content="") + document = factories.DocumentFactory(title="my document") with freeze_time("2026-08-06 12:00:00"): response = APIClient().post( diff --git a/src/backend/core/tests/documents/test_api_documents_create_for_owner.py b/src/backend/core/tests/documents/test_api_documents_create_for_owner.py index c27d57b16..c9c45a938 100644 --- a/src/backend/core/tests/documents/test_api_documents_create_for_owner.py +++ b/src/backend/core/tests/documents/test_api_documents_create_for_owner.py @@ -219,7 +219,6 @@ def test_api_documents_create_for_owner_existing(mock_convert_md, mock_yhub): assert document.title == "My Document" # the content is saved by the collaboration server, not by Django - assert document.content is None mock_yhub.return_value.create_ydoc.assert_called_once_with( document, CONVERTED_CONTENT ) @@ -289,7 +288,6 @@ def test_api_documents_create_for_owner_new_user(mock_convert_md, mock_yhub): assert document.title == "My Document" # the content is saved by the collaboration server, not by Django - assert document.content is None mock_yhub.return_value.create_ydoc.assert_called_once_with( document, CONVERTED_CONTENT ) @@ -401,7 +399,6 @@ def test_api_documents_create_for_owner_existing_user_email_no_sub_with_fallback assert document.title == "My Document" # the content is saved by the collaboration server, not by Django - assert document.content is None mock_yhub.return_value.create_ydoc.assert_called_once_with( document, CONVERTED_CONTENT ) @@ -503,7 +500,6 @@ def test_api_documents_create_for_owner_new_user_no_sub_no_fallback_allow_duplic assert document.title == "My Document" # the content is saved by the collaboration server, not by Django - assert document.content is None mock_yhub.return_value.create_ydoc.assert_called_once_with( document, CONVERTED_CONTENT ) diff --git a/src/backend/core/tests/documents/test_api_documents_create_with_file.py b/src/backend/core/tests/documents/test_api_documents_create_with_file.py index 89066c965..4bf07a940 100644 --- a/src/backend/core/tests/documents/test_api_documents_create_with_file.py +++ b/src/backend/core/tests/documents/test_api_documents_create_with_file.py @@ -76,10 +76,9 @@ def test_api_documents_create_with_docx_file_success(mock_convert, mock_yhub, se assert response.status_code == 201 document = Document.objects.get() assert document.title == "My Important Document.docx" - # the content is saved by the collaboration server, not by Django - assert document.content is None assert document.accesses.filter(role="owner", user=user).exists() + # the content is saved by the collaboration server, not by Django mock_yhub.assert_called_once_with(user=user) mock_yhub.return_value.create_ydoc.assert_called_once_with(document, converted_yjs) @@ -176,10 +175,9 @@ def test_api_documents_create_with_markdown_file_success( assert response.status_code == 201 document = Document.objects.get() assert document.title == "readme.md" - # the content is saved by the collaboration server, not by Django - assert document.content is None assert document.accesses.filter(role="owner", user=user).exists() + # the content is saved by the collaboration server, not by Django mock_yhub.return_value.create_ydoc.assert_called_once_with(document, converted_yjs) # Verify the converter was called correctly @@ -383,7 +381,6 @@ def test_api_documents_create_without_file_still_works(): assert response.status_code == 201 document = Document.objects.get() assert document.title == "Regular document without file" - assert document.content is None assert document.accesses.filter(role="owner", user=user).exists() mock_capture.assert_called_once_with( @@ -465,7 +462,6 @@ def test_api_documents_create_with_file_preserves_content_format( # The update is sent untouched, it is not base64 encoded on the way mock_yhub.return_value.create_ydoc.assert_called_once_with(document, converted_yjs) - assert document.content is None # The successful conversion should be tracked in PostHog mock_capture.assert_any_call( diff --git a/src/backend/core/tests/documents/test_api_documents_duplicate.py b/src/backend/core/tests/documents/test_api_documents_duplicate.py index 7ce23dfda..0dadc4f82 100644 --- a/src/backend/core/tests/documents/test_api_documents_duplicate.py +++ b/src/backend/core/tests/documents/test_api_documents_duplicate.py @@ -145,7 +145,6 @@ def test_api_documents_duplicate_success(index, mock_yhub): duplicated_document = models.Document.objects.get(id=response.json()["id"]) assert duplicated_document.title == "Copy of document with an image" # the content is copied through the collaboration server - assert duplicated_document.content is None mock_yhub.return_value.create_ydoc.assert_called_once_with( duplicated_document, update ) @@ -248,7 +247,6 @@ def test_api_documents_duplicate_with_accesses_admin(role, mock_yhub): duplicated_document = models.Document.objects.get(id=response.json()["id"]) assert duplicated_document.title == "Copy of document with accesses" # the content is copied through the collaboration server - assert duplicated_document.content is None mock_yhub.return_value.create_ydoc.assert_called_once_with( duplicated_document, YDOC_HELLO_WORLD_UPDATE ) @@ -307,7 +305,6 @@ def test_api_documents_duplicate_with_accesses_non_admin(role, mock_yhub): duplicated_document = models.Document.objects.get(id=response.json()["id"]) assert duplicated_document.title == "Copy of document with accesses" # the content is copied through the collaboration server - assert duplicated_document.content is None mock_yhub.return_value.create_ydoc.assert_called_once_with( duplicated_document, YDOC_HELLO_WORLD_UPDATE ) @@ -359,7 +356,6 @@ def test_api_documents_duplicate_non_root_document(role, mock_yhub): duplicated_document = models.Document.objects.get(id=response.json()["id"]) assert duplicated_document.title == "Copy of document with accesses" # the content is copied through the collaboration server - assert duplicated_document.content is None mock_yhub.return_value.create_ydoc.assert_called_once_with( duplicated_document, YDOC_HELLO_WORLD_UPDATE ) @@ -636,8 +632,6 @@ def test_api_documents_duplicate_with_descendants_and_attachments(mock_yhub): assert dup_child.attachments == [image_key_child] # the content of the whole subtree is copied through the collaboration server - assert duplicated_root.content is None - assert dup_child.content is None assert mock_yhub.return_value.create_ydoc.call_args_list == [ mock.call(duplicated_root, root_update), mock.call(dup_child, child_update), diff --git a/src/backend/core/tests/external_api/test_external_api_documents.py b/src/backend/core/tests/external_api/test_external_api_documents.py index 183b092ed..ca154870c 100644 --- a/src/backend/core/tests/external_api/test_external_api_documents.py +++ b/src/backend/core/tests/external_api/test_external_api_documents.py @@ -302,10 +302,9 @@ def test_external_api_documents_create_with_markdown_file_success( document = models.Document.objects.get(id=data["id"]) assert document.title == "readme.md" - # the content is saved by the collaboration server, not by Django - assert document.content is None assert document.accesses.filter(role="owner", user=user_specific_sub).exists() + # the content is saved by the collaboration server, not by Django mock_yhub.assert_called_once_with(user=user_specific_sub) mock_yhub.return_value.create_ydoc.assert_called_once_with(document, converted_yjs) diff --git a/src/backend/core/tests/test_models_documents.py b/src/backend/core/tests/test_models_documents.py index 14f6d920d..1f5fd327f 100644 --- a/src/backend/core/tests/test_models_documents.py +++ b/src/backend/core/tests/test_models_documents.py @@ -12,7 +12,6 @@ from django.contrib.auth.models import AnonymousUser from django.core import mail from django.core.cache import cache from django.core.exceptions import ValidationError -from django.core.files.storage import default_storage from django.test.utils import override_settings from django.utils import timezone @@ -908,34 +907,6 @@ def test_models_document_get_abilities_ai_access_public(is_authenticated, reach) assert abilities["ai_translate"] == is_authenticated -def test_models_documents_version_duplicate(): - """A new version should be created in object storage only if the content has changed.""" - document = factories.DocumentFactory(content=factories.YDOC_HELLO_WORLD_BASE64) - - file_key = str(document.pk) - response = default_storage.connection.meta.client.list_object_versions( - Bucket=default_storage.bucket_name, Prefix=file_key - ) - assert len(response["Versions"]) == 1 - - # Save again with the same content - document.save() - - response = default_storage.connection.meta.client.list_object_versions( - Bucket=default_storage.bucket_name, Prefix=file_key - ) - assert len(response["Versions"]) == 1 - - # Save modified content - document.content = "new content" - document.save() - - response = default_storage.connection.meta.client.list_object_versions( - Bucket=default_storage.bucket_name, Prefix=file_key - ) - assert len(response["Versions"]) == 2 - - def test_models_documents__email_invitation__success(): """ The email invitation is sent successfully. diff --git a/src/backend/demo/tests/test_commands_create_demo.py b/src/backend/demo/tests/test_commands_create_demo.py index 8c2e58566..cdb2acd9c 100644 --- a/src/backend/demo/tests/test_commands_create_demo.py +++ b/src/backend/demo/tests/test_commands_create_demo.py @@ -44,14 +44,12 @@ def test_commands_create_demo(collaboration_server): assert models.Document.objects.count() >= 10 assert models.DocumentAccess.objects.count() > 10 - # every document was seeded with its content in the collaboration server, - # and nothing was written to the object storage + # every document was seeded with its content in the collaboration server assert collaboration_server.call_count == 10 seeded = {call.args[0].id for call in collaboration_server.call_args_list} assert seeded == set(models.Document.objects.values_list("id", flat=True)) for call in collaboration_server.call_args_list: document, update = call.args - assert document.content is None # the structure BlockNote stores, so the editor opens a real document xml = yjs_to_xml(update)