From 2213d7a3afde177fbdcbc897bb9b6df9e3bc709b Mon Sep 17 00:00:00 2001 From: charles Date: Tue, 28 Apr 2026 15:26:44 +0200 Subject: [PATCH] =?UTF-8?q?=E2=9C=A8(backend)=20update=20document.updated?= =?UTF-8?q?=5Fat=20on=20restore?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If a file is restored, its deleted_at field is deleted, and its `updated_at` and `ancestors_deleted_at` fields are not updated to the current date. this prevents using the crash safe mode. Signed-off-by: charles --- src/backend/core/models.py | 21 +++-- .../documents/test_api_documents_delete.py | 9 ++- .../documents/test_api_documents_restore.py | 6 ++ .../core/tests/test_models_documents.py | 81 ++++++++++++------- 4 files changed, 72 insertions(+), 45 deletions(-) diff --git a/src/backend/core/models.py b/src/backend/core/models.py index 382482887..0d8c23038 100644 --- a/src/backend/core/models.py +++ b/src/backend/core/models.py @@ -875,19 +875,13 @@ class DocumentQuerySet(MP_NodeQuerySet): conditions = models.Q() if lower_time_bound and upper_time_bound: conditions = models.Q( - updated_at__gte=lower_time_bound, updated_at__lte=upper_time_bound - ) | models.Q( - ancestors_deleted_at__gte=lower_time_bound, - ancestors_deleted_at__lte=upper_time_bound, + updated_at__gte=lower_time_bound, + updated_at__lte=upper_time_bound, ) elif lower_time_bound: - conditions = models.Q(updated_at__gte=lower_time_bound) | models.Q( - ancestors_deleted_at__gte=lower_time_bound - ) + conditions = models.Q(updated_at__gte=lower_time_bound) elif upper_time_bound: - conditions = models.Q(updated_at__lte=upper_time_bound) | models.Q( - ancestors_deleted_at__lte=upper_time_bound - ) + conditions = models.Q(updated_at__lte=upper_time_bound) return self.filter(conditions) @@ -1491,13 +1485,16 @@ class Document(MP_Node, BaseModel): .first() ) self.ancestors_deleted_at = ancestors_deleted_at - self.save(update_fields=["deleted_at", "ancestors_deleted_at"]) + self.save(update_fields=["deleted_at", "ancestors_deleted_at", "updated_at"]) self.invalidate_nb_accesses_cache() self.get_descendants().exclude( models.Q(deleted_at__isnull=False) | models.Q(ancestors_deleted_at__lt=current_deleted_at) - ).update(ancestors_deleted_at=self.ancestors_deleted_at) + ).update( + ancestors_deleted_at=self.ancestors_deleted_at, + updated_at=self.updated_at, + ) if self.depth > 1: self._meta.model.objects.filter(pk=self.get_parent().pk).update( diff --git a/src/backend/core/tests/documents/test_api_documents_delete.py b/src/backend/core/tests/documents/test_api_documents_delete.py index 776bbe1fd..c107d2826 100644 --- a/src/backend/core/tests/documents/test_api_documents_delete.py +++ b/src/backend/core/tests/documents/test_api_documents_delete.py @@ -96,8 +96,9 @@ def test_api_documents_delete_authenticated_owner_of_ancestor(depth): ) assert models.Document.objects.count() == depth + document_to_delete = documents[-1] response = client.delete( - f"/api/v1.0/documents/{documents[-1].id}/", + f"/api/v1.0/documents/{document_to_delete.id}/", ) assert response.status_code == 204 @@ -105,7 +106,11 @@ def test_api_documents_delete_authenticated_owner_of_ancestor(depth): # Make sure it is only a soft delete assert models.Document.objects.count() == depth assert models.Document.objects.filter(deleted_at__isnull=True).count() == depth - 1 - assert models.Document.objects.filter(deleted_at__isnull=False).count() == 1 + deleted_documents = models.Document.objects.filter(deleted_at__isnull=False) + assert len(deleted_documents) == 1 + deleted_document = deleted_documents[0] + # updated_at is updated by the soft delete + assert deleted_document.updated_at > document_to_delete.updated_at @pytest.mark.parametrize("via", VIA) diff --git a/src/backend/core/tests/documents/test_api_documents_restore.py b/src/backend/core/tests/documents/test_api_documents_restore.py index 5ae64aec1..56aec6b70 100644 --- a/src/backend/core/tests/documents/test_api_documents_restore.py +++ b/src/backend/core/tests/documents/test_api_documents_restore.py @@ -91,11 +91,15 @@ def test_api_documents_restore_authenticated_owner_ancestor_deleted(): factories.UserDocumentAccessFactory(document=document, user=user, role="owner") document.soft_delete() + document.refresh_from_db() document_deleted_at = document.deleted_at + document_updated_at = document.updated_at + assert document_deleted_at is not None grand_parent.soft_delete() grand_parent_deleted_at = grand_parent.deleted_at + assert grand_parent_deleted_at is not None response = client.post(f"/api/v1.0/documents/{document.id!s}/restore/") @@ -105,6 +109,8 @@ def test_api_documents_restore_authenticated_owner_ancestor_deleted(): document.refresh_from_db() assert document.deleted_at is None + # document is updated by restore + assert document.updated_at > document_updated_at # document is still marked as deleted assert document.ancestors_deleted_at == grand_parent_deleted_at assert grand_parent_deleted_at > document_deleted_at diff --git a/src/backend/core/tests/test_models_documents.py b/src/backend/core/tests/test_models_documents.py index 1aa1c4afb..64a80689c 100644 --- a/src/backend/core/tests/test_models_documents.py +++ b/src/backend/core/tests/test_models_documents.py @@ -90,7 +90,8 @@ def test_models_documents_tree_alphabet(): @pytest.mark.parametrize("depth", range(5)) def test_models_documents_soft_delete(depth): - """Trying to delete a document that is already deleted or is a descendant of + """ + Trying to delete a document that is already deleted or is a descendant of a deleted document should raise an error. """ documents = [] @@ -102,6 +103,8 @@ def test_models_documents_soft_delete(depth): ) assert models.Document.objects.count() == depth + 1 + document_pk_to_updated_at = {d.pk: d.updated_at for d in documents} + # Delete any one of the documents... deleted_document = random.choice(documents) deleted_document.soft_delete() @@ -109,19 +112,26 @@ def test_models_documents_soft_delete(depth): with pytest.raises(RuntimeError): documents[-1].soft_delete() + deleted_document.refresh_from_db() assert deleted_document.deleted_at is not None assert deleted_document.ancestors_deleted_at == deleted_document.deleted_at + # updated_at is updated on the deleted document + assert deleted_document.updated_at > document_pk_to_updated_at[deleted_document.pk] descendants = deleted_document.get_descendants() for child in descendants: assert child.deleted_at is None assert child.ancestors_deleted_at is not None assert child.ancestors_deleted_at == deleted_document.deleted_at + # updated_at is updated on descendants + assert child.updated_at > document_pk_to_updated_at[child.pk] ancestors = deleted_document.get_ancestors() for parent in ancestors: assert parent.deleted_at is None assert parent.ancestors_deleted_at is None + # updated_at is not affected on parents + assert parent.updated_at == document_pk_to_updated_at[parent.pk] assert len(ancestors) + len(descendants) == depth @@ -1422,16 +1432,20 @@ def test_models_documents_restore_tempering_with_instance(): def test_models_documents_restore(django_assert_num_queries): """The restore method should restore a soft-deleted document.""" document = factories.DocumentFactory() + document.soft_delete() document.refresh_from_db() assert document.deleted_at is not None assert document.ancestors_deleted_at == document.deleted_at + original_updated_after_delete = document.updated_at with django_assert_num_queries(10): document.restore() document.refresh_from_db() assert document.deleted_at is None - assert document.ancestors_deleted_at == document.deleted_at + assert document.ancestors_deleted_at is None + # updated_at is updated by restore + assert original_updated_after_delete < document.updated_at def test_models_documents_restore_complex(django_assert_num_queries): @@ -1448,6 +1462,7 @@ def test_models_documents_restore_complex(django_assert_num_queries): document.refresh_from_db() child1.refresh_from_db() child2.refresh_from_db() + assert document.deleted_at is not None assert document.ancestors_deleted_at == document.deleted_at assert child1.ancestors_deleted_at == document.deleted_at @@ -1457,13 +1472,18 @@ def test_models_documents_restore_complex(django_assert_num_queries): grand_parent.soft_delete() grand_parent.refresh_from_db() parent.refresh_from_db() + document.refresh_from_db() + child1.refresh_from_db() + child2.refresh_from_db() + grand_parent_updated_at = grand_parent.updated_at + document_updated_at = document.updated_at + child1_updated_at = child2.updated_at + child2_updated_at = child2.updated_at + assert grand_parent.deleted_at is not None assert grand_parent.ancestors_deleted_at == grand_parent.deleted_at assert parent.ancestors_deleted_at == grand_parent.deleted_at # item, child1 and child2 should not be affected - document.refresh_from_db() - child1.refresh_from_db() - child2.refresh_from_db() assert document.deleted_at is not None assert document.ancestors_deleted_at == document.deleted_at assert child1.ancestors_deleted_at == document.deleted_at @@ -1472,15 +1492,23 @@ def test_models_documents_restore_complex(django_assert_num_queries): # Restore the item with django_assert_num_queries(14): document.restore() + document.refresh_from_db() child1.refresh_from_db() child2.refresh_from_db() grand_parent.refresh_from_db() + assert document.deleted_at is None assert document.ancestors_deleted_at == grand_parent.deleted_at # child 1 and child 2 should now have the same ancestors_deleted_at as the grand parent assert child1.ancestors_deleted_at == grand_parent.deleted_at assert child2.ancestors_deleted_at == grand_parent.deleted_at + # updated_at is updated for document and children after restore + assert document.updated_at > document_updated_at + assert child1.updated_at > child1_updated_at + assert child2.updated_at > child2_updated_at + # grand_parent updated_at is not affected + assert grand_parent.updated_at == grand_parent_updated_at def test_models_documents_restore_complex_bis(django_assert_num_queries): @@ -1488,31 +1516,37 @@ def test_models_documents_restore_complex_bis(django_assert_num_queries): grand_parent = factories.DocumentFactory() parent = factories.DocumentFactory(parent=grand_parent) document = factories.DocumentFactory(parent=parent) - child1 = factories.DocumentFactory(parent=document) child2 = factories.DocumentFactory(parent=document) # Soft delete first the document document.soft_delete() + document.refresh_from_db() child1.refresh_from_db() child2.refresh_from_db() + assert document.deleted_at is not None assert document.ancestors_deleted_at == document.deleted_at assert child1.ancestors_deleted_at == document.deleted_at assert child2.ancestors_deleted_at == document.deleted_at - # Soft delete the grand parent + # Soft delete the grand_parent grand_parent.soft_delete() + grand_parent.refresh_from_db() parent.refresh_from_db() + document.refresh_from_db() + child1.refresh_from_db() + child2.refresh_from_db() + original_parent_updated_at = parent.updated_at + original_child1_updated_at = child1.updated_at + original_child2_updated_at = child2.updated_at + assert grand_parent.deleted_at is not None assert grand_parent.ancestors_deleted_at == grand_parent.deleted_at assert parent.ancestors_deleted_at == grand_parent.deleted_at # item, child1 and child2 should not be affected - document.refresh_from_db() - child1.refresh_from_db() - child2.refresh_from_db() assert document.deleted_at is not None assert document.ancestors_deleted_at == document.deleted_at assert child1.ancestors_deleted_at == document.deleted_at @@ -1528,14 +1562,20 @@ def test_models_documents_restore_complex_bis(django_assert_num_queries): document.refresh_from_db() child1.refresh_from_db() child2.refresh_from_db() + assert grand_parent.deleted_at is None assert grand_parent.ancestors_deleted_at is None assert parent.deleted_at is None assert parent.ancestors_deleted_at is None + # parent should have updated_at updated (descendant of restored grand_parent) + assert parent.updated_at > original_parent_updated_at assert document.deleted_at is not None assert document.ancestors_deleted_at == document.deleted_at + # children are not restored and then there updated_at should not be affected assert child1.ancestors_deleted_at == document.deleted_at + assert child1.updated_at == original_child1_updated_at assert child2.ancestors_deleted_at == document.deleted_at + assert child2.updated_at == original_child2_updated_at @pytest.mark.parametrize( @@ -1773,24 +1813,3 @@ def test_models_documents_manager_time_filter_both_bounds(): assert document_in_window in queryset assert other_document_in_window in queryset assert document_too_late not in queryset - - -def test_utils_build_indexable_documents_queryset_ancestors_deleted_at(): - """Test filtering includes documents with ancestors_deleted_at in range.""" - lower_time_bound = datetime(2024, 2, 1, tzinfo=base_timezone.utc) - - document_ancestor_deleted_in_range = DocumentFactory( - updated_at=lower_time_bound - timedelta(days=30), - ancestors_deleted_at=lower_time_bound + timedelta(days=15), - ) - document_updated_before_range = DocumentFactory( - updated_at=lower_time_bound - timedelta(days=30) - ) - - queryset = models.Document.objects.filter_updated_at( - lower_time_bound=lower_time_bound - ) - - assert queryset.count() == 1 - assert document_ancestor_deleted_in_range in queryset - assert document_updated_before_range not in queryset