From fbc3ef83ba32c60f8c376d1d4b575f03eea9ea6c Mon Sep 17 00:00:00 2001 From: Manuel Raynaud Date: Wed, 9 Sep 2026 14:24:42 +0200 Subject: [PATCH] =?UTF-8?q?=E2=9A=A1=EF=B8=8F(backend)=20stop=20using=20LE?= =?UTF-8?q?FT(value,=20LENGTH(path))=20in=20sql=20queries?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Comparing path with LEFT(value, LENGTH(path)) makes a sequential scan on all the Document table, the more this table grow, the more the query using it will be slow. We dediced instead to lookup on the path extracting all ancestors path for a given document and then make a path IN statement to use the index existing on the path column. --- CHANGELOG.md | 1 + src/backend/core/api/viewsets.py | 6 +- src/backend/core/models.py | 20 +++++-- .../core/tests/test_models_documents.py | 56 +++++++++++++++++++ 4 files changed, 76 insertions(+), 7 deletions(-) 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() + )