diff --git a/CHANGELOG.md b/CHANGELOG.md index 10a9ceb8f..48ade5220 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,7 @@ and this project adheres to ### Changed - ⬆️(backend) upgrade celery to version 5.6.3 +- ⚡️(backend) stop using LEFT(value, LENGTH(path)) in sql queries ### Fixed diff --git a/src/backend/core/api/viewsets.py b/src/backend/core/api/viewsets.py index 7f55a8aa2..c059eaff1 100644 --- a/src/backend/core/api/viewsets.py +++ b/src/backend/core/api/viewsets.py @@ -23,7 +23,7 @@ from django.core.validators import URLValidator from django.db import DatabaseError, connection, transaction from django.db import models as db from django.db.models.expressions import RawSQL -from django.db.models.functions import Greatest, Left, Length +from django.db.models.functions import Greatest from django.http import Http404, StreamingHttpResponse from django.urls import reverse from django.utils import timezone @@ -1752,7 +1752,7 @@ class DocumentViewSet( # document. Filter to get the minimum access date for the logged-in user access_queryset = models.DocumentAccess.objects.filter( db.Q(user=user) | db.Q(team__in=user.teams), - document__path=Left(db.Value(document.path), Length("document__path")), + document__path__in=document.get_self_and_ancestors_paths(), ).aggregate(min_date=db.Min("created_at")) # Handle the case where the user has no accesses @@ -1792,7 +1792,7 @@ class DocumentViewSet( access.created_at for access in models.DocumentAccess.objects.filter( db.Q(user=user) | db.Q(team__in=user.teams), - document__path=Left(db.Value(document.path), Length("document__path")), + document__path__in=document.get_self_and_ancestors_paths(), ) ) diff --git a/src/backend/core/models.py b/src/backend/core/models.py index 96ba11598..04bcd239b 100644 --- a/src/backend/core/models.py +++ b/src/backend/core/models.py @@ -1158,6 +1158,20 @@ class Document(MP_Node, BaseModel): Bucket=default_storage.bucket_name, Key=self.file_key, VersionId=version_id ) + def get_self_and_ancestors_paths(self): + """ + Return the paths of the document and of all its ancestors, computed from + the materialized path without querying the database. + + Filtering on `path__in` with this list hits the unique index on `path`, + whereas comparing `path` with `LEFT(value, LENGTH(path))` forces a + sequential scan of the whole table. + """ + return [ + self.path[:pos] + for pos in range(self.steplen, len(self.path) + 1, self.steplen) + ] + def get_nb_accesses_cache_key(self): """Generate a unique cache key for each document.""" return f"document_{self.id!s}_nb_accesses" @@ -1175,9 +1189,7 @@ class Document(MP_Node, BaseModel): nb_accesses = ( DocumentAccess.objects.filter(document=self).count(), DocumentAccess.objects.filter( - document__path=Left( - models.Value(self.path), Length("document__path") - ), + document__path__in=self.get_self_and_ancestors_paths(), document__ancestors_deleted_at__isnull=True, ).count(), ) @@ -1217,7 +1229,7 @@ class Document(MP_Node, BaseModel): except AttributeError: roles = DocumentAccess.objects.filter( models.Q(user=user) | models.Q(team__in=user.teams), - document__path=Left(models.Value(self.path), Length("document__path")), + document__path__in=self.get_self_and_ancestors_paths(), ).values_list("role", flat=True) return RoleChoices.max(*roles) diff --git a/src/backend/core/tests/test_models_documents.py b/src/backend/core/tests/test_models_documents.py index bc63b122a..b6bd19768 100644 --- a/src/backend/core/tests/test_models_documents.py +++ b/src/backend/core/tests/test_models_documents.py @@ -1729,3 +1729,59 @@ def test_models_documents_compute_ancestors_links_paths_mapping_structure( {"link_reach": sibling.link_reach, "link_role": sibling.link_role}, ], } + + +def test_models_documents_get_self_and_ancestors_paths_root(): + """A root document should only return its own path.""" + document = factories.DocumentFactory() + + assert len(document.path) == models.Document.steplen + assert document.get_self_and_ancestors_paths() == [document.path] + + +def test_models_documents_get_self_and_ancestors_paths_tree( + django_assert_num_queries, +): + """ + The method should return the paths of the document and all its ancestors, + ordered from the root down to the document itself, without hitting the database. + """ + root = factories.DocumentFactory() + factories.DocumentFactory(parent=root) # sibling branch, should be ignored + parent = factories.DocumentFactory(parent=root) + document = factories.DocumentFactory(parent=parent) + child = factories.DocumentFactory(parent=document) + + with django_assert_num_queries(0): + paths = child.get_self_and_ancestors_paths() + + assert paths == [root.path, parent.path, document.path, child.path] + + # Should match what treebeard computes with a database query + ancestors_paths = list( + child.get_ancestors().order_by("path").values_list("path", flat=True) + ) + assert paths == ancestors_paths + [child.path] + + # Filtering on these paths should return exactly the ancestors and the document + assert set( + models.Document.objects.filter(path__in=paths).values_list("id", flat=True) + ) == {root.id, parent.id, document.id, child.id} + + +def test_models_documents_get_self_and_ancestors_paths_from_path_only(): + """ + The method should only rely on the materialized path and the step length, + so it can be used on an unsaved instance. + """ + steplen = models.Document.steplen + document = models.Document(path="0000001" + "000000A" + "00000Zz") + + assert document.get_self_and_ancestors_paths() == [ + "0000001", + "0000001000000A", + "0000001000000A00000Zz", + ] + assert all( + len(path) % steplen == 0 for path in document.get_self_and_ancestors_paths() + )