From 2c915d53f490546d3d7075995249efc66c5969d6 Mon Sep 17 00:00:00 2001 From: Samuel Paccoud - DINUM Date: Sat, 9 Nov 2024 10:27:21 +0100 Subject: [PATCH] =?UTF-8?q?=E2=9A=A1=EF=B8=8F(backend)=20optimize=20number?= =?UTF-8?q?=20of=20queries=20on=20document=20list=20view?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit I realized most of the database queries made when getting a document list view were to include nested accesses. This detailed information about accesses in only necessary for the document detail view. I introduced a specific serializer for the document list view with less fields. For a list of 20 documents with 5 accesses, we go down from 3x5x20= 300 queries to just 3 queries. --- CHANGELOG.md | 1 + src/backend/core/api/serializers.py | 41 ++- src/backend/core/api/viewsets.py | 18 +- .../documents/test_api_documents_list.py | 38 ++- .../documents/test_api_documents_retrieve.py | 257 ++---------------- .../documents/test_api_documents_update.py | 4 +- 6 files changed, 97 insertions(+), 262 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fcf5cd1e..94902f5a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -39,6 +39,7 @@ and this project adheres to ## Changed +- ⚡️(backend) optimize number of queries on document list view #411 - 🚸(backend) improve users similarity search and sort results #391 - ♻️(frontend) simplify stores #402 - ✨(frontend) update $css Box props type to add styled components RuleSet #423 diff --git a/src/backend/core/api/serializers.py b/src/backend/core/api/serializers.py index 2947e3b8..46016350 100644 --- a/src/backend/core/api/serializers.py +++ b/src/backend/core/api/serializers.py @@ -137,32 +137,55 @@ class BaseResourceSerializer(serializers.ModelSerializer): return {} -class DocumentSerializer(BaseResourceSerializer): - """Serialize documents.""" +class ListDocumentSerializer(BaseResourceSerializer): + """Serialize documents with limited fields for display in lists.""" - content = serializers.CharField(required=False) - accesses = DocumentAccessSerializer(many=True, read_only=True) class Meta: model = models.Document fields = [ "id", - "content", - "title", - "accesses", "abilities", + "content", + "created_at", "link_role", "link_reach", - "created_at", + "title", "updated_at", ] read_only_fields = [ "id", - "accesses", "abilities", + "created_at", "link_role", "link_reach", + "updated_at", + ] + + +class DocumentSerializer(ListDocumentSerializer): + """Serialize documents with all fields for display in detail views.""" + + content = serializers.CharField(required=False) + + class Meta: + model = models.Document + fields = [ + "id", + "abilities", + "content", "created_at", + "link_role", + "link_reach", + "title", + "updated_at", + ] + read_only_fields = [ + "id", + "abilities", + "created_at", + "link_role", + "link_reach", "updated_at", ] diff --git a/src/backend/core/api/viewsets.py b/src/backend/core/api/viewsets.py index 91e5745f..29bead7e 100644 --- a/src/backend/core/api/viewsets.py +++ b/src/backend/core/api/viewsets.py @@ -346,15 +346,23 @@ class DocumentViewSet( ): """Document ViewSet""" + access_model_class = models.DocumentAccess + metadata_class = DocumentMetadata + ordering = ["-updated_at"] permission_classes = [ permissions.AccessPermission, ] - serializer_class = serializers.DocumentSerializer - access_model_class = models.DocumentAccess - resource_field_name = "document" queryset = models.Document.objects.all() - ordering = ["-updated_at"] - metadata_class = DocumentMetadata + resource_field_name = "document" + serializer_class = serializers.DocumentSerializer + + def get_serializer_class(self): + """ + Use ListDocumentSerializer for list actions, otherwise use DocumentSerializer. + """ + if self.action == "list": + return serializers.ListDocumentSerializer + return self.serializer_class def list(self, request, *args, **kwargs): """Restrict resources returned by the list endpoint""" diff --git a/src/backend/core/tests/documents/test_api_documents_list.py b/src/backend/core/tests/documents/test_api_documents_list.py index 5045da22..2c30b77c 100644 --- a/src/backend/core/tests/documents/test_api_documents_list.py +++ b/src/backend/core/tests/documents/test_api_documents_list.py @@ -32,7 +32,7 @@ def test_api_documents_list_anonymous(reach, role): assert len(results) == 0 -def test_api_documents_list_authenticated_direct(): +def test_api_documents_list_authenticated_direct(django_assert_num_queries): """ Authenticated users should be able to list documents they are a direct owner/administrator/member of or documents that have a link reach other @@ -55,9 +55,10 @@ def test_api_documents_list_authenticated_direct(): expected_ids = {str(document.id) for document in documents} - response = client.get( - "/api/v1.0/documents/", - ) + with django_assert_num_queries(3): + response = client.get( + "/api/v1.0/documents/", + ) assert response.status_code == 200 results = response.json()["results"] @@ -66,7 +67,9 @@ def test_api_documents_list_authenticated_direct(): assert expected_ids == results_id -def test_api_documents_list_authenticated_via_team(mock_user_teams): +def test_api_documents_list_authenticated_via_team( + django_assert_num_queries, mock_user_teams +): """ Authenticated users should be able to list documents they are a owner/administrator/member of via a team. @@ -89,7 +92,8 @@ def test_api_documents_list_authenticated_via_team(mock_user_teams): expected_ids = {str(document.id) for document in documents_team1 + documents_team2} - response = client.get("/api/v1.0/documents/") + with django_assert_num_queries(3): + response = client.get("/api/v1.0/documents/") assert response.status_code == 200 results = response.json()["results"] @@ -98,7 +102,9 @@ def test_api_documents_list_authenticated_via_team(mock_user_teams): assert expected_ids == results_id -def test_api_documents_list_authenticated_link_reach_restricted(): +def test_api_documents_list_authenticated_link_reach_restricted( + django_assert_num_queries, +): """ An authenticated user who has link traces to a document that is restricted should not see it on the list view @@ -115,9 +121,10 @@ def test_api_documents_list_authenticated_link_reach_restricted(): other_document = factories.DocumentFactory(link_reach="public") models.LinkTrace.objects.create(document=other_document, user=user) - response = client.get( - "/api/v1.0/documents/", - ) + with django_assert_num_queries(3): + response = client.get( + "/api/v1.0/documents/", + ) assert response.status_code == 200 results = response.json()["results"] @@ -127,7 +134,9 @@ def test_api_documents_list_authenticated_link_reach_restricted(): assert results[0]["id"] == str(other_document.id) -def test_api_documents_list_authenticated_link_reach_public_or_authenticated(): +def test_api_documents_list_authenticated_link_reach_public_or_authenticated( + django_assert_num_queries, +): """ An authenticated user who has link traces to a document with public or authenticated link reach should see it on the list view. @@ -144,9 +153,10 @@ def test_api_documents_list_authenticated_link_reach_public_or_authenticated(): ] expected_ids = {str(document.id) for document in documents} - response = client.get( - "/api/v1.0/documents/", - ) + with django_assert_num_queries(3): + response = client.get( + "/api/v1.0/documents/", + ) assert response.status_code == 200 results = response.json()["results"] diff --git a/src/backend/core/tests/documents/test_api_documents_retrieve.py b/src/backend/core/tests/documents/test_api_documents_retrieve.py index c7a76032..ad46611d 100644 --- a/src/backend/core/tests/documents/test_api_documents_retrieve.py +++ b/src/backend/core/tests/documents/test_api_documents_retrieve.py @@ -36,12 +36,12 @@ def test_api_documents_retrieve_anonymous_public(): "versions_list": False, "versions_retrieve": False, }, - "accesses": [], + "content": document.content, + "created_at": document.created_at.isoformat().replace("+00:00", "Z"), + "is_user_favorite": False, "link_reach": "public", "link_role": document.link_role, "title": document.title, - "content": document.content, - "created_at": document.created_at.isoformat().replace("+00:00", "Z"), "updated_at": document.updated_at.isoformat().replace("+00:00", "Z"), } @@ -94,7 +94,7 @@ def test_api_documents_retrieve_authenticated_unrelated_public_or_authenticated( "versions_list": False, "versions_retrieve": False, }, - "accesses": [], + "is_user_favorite": False, "link_reach": reach, "link_role": document.link_role, "title": document.title, @@ -168,35 +168,15 @@ def test_api_documents_retrieve_authenticated_related_direct(): client.force_login(user) document = factories.DocumentFactory() - access1 = factories.UserDocumentAccessFactory(document=document, user=user) + factories.UserDocumentAccessFactory(document=document, user=user) access2 = factories.UserDocumentAccessFactory(document=document) - access1_user = serializers.UserSerializer(instance=user).data - access2_user = serializers.UserSerializer(instance=access2.user).data + serializers.UserSerializer(instance=user) + serializers.UserSerializer(instance=access2.user) response = client.get( f"/api/v1.0/documents/{document.id!s}/", ) assert response.status_code == 200 - content = response.json() - assert sorted(content.pop("accesses"), key=lambda x: x["id"]) == sorted( - [ - { - "id": str(access1.id), - "user": access1_user, - "team": "", - "role": access1.role, - "abilities": access1.get_abilities(user), - }, - { - "id": str(access2.id), - "user": access2_user, - "team": "", - "role": access2.role, - "abilities": access2.get_abilities(user), - }, - ], - key=lambda x: x["id"], - ) assert response.json() == { "id": str(document.id), "title": document.title, @@ -257,7 +237,7 @@ def test_api_documents_retrieve_authenticated_related_team_members( ): """ Authenticated users should be allowed to retrieve a document to which they - are related via a team whatever the role and see all its accesses. + are related via a team whatever the role. """ mock_user_teams.return_value = teams @@ -268,73 +248,23 @@ def test_api_documents_retrieve_authenticated_related_team_members( document = factories.DocumentFactory(link_reach="restricted") - access_reader = factories.TeamDocumentAccessFactory( + factories.TeamDocumentAccessFactory( document=document, team="readers", role="reader" ) - access_editor = factories.TeamDocumentAccessFactory( + factories.TeamDocumentAccessFactory( document=document, team="editors", role="editor" ) - access_administrator = factories.TeamDocumentAccessFactory( + factories.TeamDocumentAccessFactory( document=document, team="administrators", role="administrator" ) - access_owner = factories.TeamDocumentAccessFactory( - document=document, team="owners", role="owner" - ) - other_access = factories.TeamDocumentAccessFactory(document=document) + factories.TeamDocumentAccessFactory(document=document, team="owners", role="owner") + factories.TeamDocumentAccessFactory(document=document) factories.TeamDocumentAccessFactory() response = client.get(f"/api/v1.0/documents/{document.id!s}/") # pylint: disable=R0801 assert response.status_code == 200 - content = response.json() - expected_abilities = { - "destroy": False, - "retrieve": True, - "set_role_to": [], - "update": False, - "partial_update": False, - } - assert sorted(content.pop("accesses"), key=lambda x: x["id"]) == sorted( - [ - { - "id": str(access_reader.id), - "user": None, - "team": "readers", - "role": access_reader.role, - "abilities": expected_abilities, - }, - { - "id": str(access_editor.id), - "user": None, - "team": "editors", - "role": access_editor.role, - "abilities": expected_abilities, - }, - { - "id": str(access_administrator.id), - "user": None, - "team": "administrators", - "role": access_administrator.role, - "abilities": expected_abilities, - }, - { - "id": str(access_owner.id), - "user": None, - "team": "owners", - "role": access_owner.role, - "abilities": expected_abilities, - }, - { - "id": str(other_access.id), - "user": None, - "team": other_access.team, - "role": other_access.role, - "abilities": expected_abilities, - }, - ], - key=lambda x: x["id"], - ) assert response.json() == { "id": str(document.id), "title": document.title, @@ -360,7 +290,7 @@ def test_api_documents_retrieve_authenticated_related_team_administrators( ): """ Authenticated users should be allowed to retrieve a document to which they - are related via a team whatever the role and see all its accesses. + are related via a team whatever the role. """ mock_user_teams.return_value = teams @@ -371,90 +301,23 @@ def test_api_documents_retrieve_authenticated_related_team_administrators( document = factories.DocumentFactory(link_reach="restricted") - access_reader = factories.TeamDocumentAccessFactory( + factories.TeamDocumentAccessFactory( document=document, team="readers", role="reader" ) - access_editor = factories.TeamDocumentAccessFactory( + factories.TeamDocumentAccessFactory( document=document, team="editors", role="editor" ) - access_administrator = factories.TeamDocumentAccessFactory( + factories.TeamDocumentAccessFactory( document=document, team="administrators", role="administrator" ) - access_owner = factories.TeamDocumentAccessFactory( - document=document, team="owners", role="owner" - ) - other_access = factories.TeamDocumentAccessFactory(document=document) + factories.TeamDocumentAccessFactory(document=document, team="owners", role="owner") + factories.TeamDocumentAccessFactory(document=document) factories.TeamDocumentAccessFactory() response = client.get(f"/api/v1.0/documents/{document.id!s}/") # pylint: disable=R0801 assert response.status_code == 200 - content = response.json() - assert sorted(content.pop("accesses"), key=lambda x: x["id"]) == sorted( - [ - { - "id": str(access_reader.id), - "user": None, - "team": "readers", - "role": "reader", - "abilities": { - "destroy": True, - "retrieve": True, - "set_role_to": ["administrator", "editor"], - "update": True, - "partial_update": True, - }, - }, - { - "id": str(access_editor.id), - "user": None, - "team": "editors", - "role": "editor", - "abilities": { - "destroy": True, - "retrieve": True, - "set_role_to": ["administrator", "reader"], - "update": True, - "partial_update": True, - }, - }, - { - "id": str(access_administrator.id), - "user": None, - "team": "administrators", - "role": "administrator", - "abilities": { - "destroy": True, - "retrieve": True, - "set_role_to": ["editor", "reader"], - "update": True, - "partial_update": True, - }, - }, - { - "id": str(access_owner.id), - "user": None, - "team": "owners", - "role": "owner", - "abilities": { - "destroy": False, - "retrieve": True, - "set_role_to": [], - "update": False, - "partial_update": False, - }, - }, - { - "id": str(other_access.id), - "user": None, - "team": other_access.team, - "role": other_access.role, - "abilities": other_access.get_abilities(user), - }, - ], - key=lambda x: x["id"], - ) assert response.json() == { "id": str(document.id), "title": document.title, @@ -481,7 +344,7 @@ def test_api_documents_retrieve_authenticated_related_team_owners( ): """ Authenticated users should be allowed to retrieve a restricted document to which - they are related via a team whatever the role and see all its accesses. + they are related via a team whatever the role. """ mock_user_teams.return_value = teams @@ -492,93 +355,23 @@ def test_api_documents_retrieve_authenticated_related_team_owners( document = factories.DocumentFactory(link_reach="restricted") - access_reader = factories.TeamDocumentAccessFactory( + factories.TeamDocumentAccessFactory( document=document, team="readers", role="reader" ) - access_editor = factories.TeamDocumentAccessFactory( + factories.TeamDocumentAccessFactory( document=document, team="editors", role="editor" ) - access_administrator = factories.TeamDocumentAccessFactory( + factories.TeamDocumentAccessFactory( document=document, team="administrators", role="administrator" ) - access_owner = factories.TeamDocumentAccessFactory( - document=document, team="owners", role="owner" - ) - other_access = factories.TeamDocumentAccessFactory(document=document) + factories.TeamDocumentAccessFactory(document=document, team="owners", role="owner") + factories.TeamDocumentAccessFactory(document=document) factories.TeamDocumentAccessFactory() response = client.get(f"/api/v1.0/documents/{document.id!s}/") # pylint: disable=R0801 assert response.status_code == 200 - content = response.json() - assert sorted(content.pop("accesses"), key=lambda x: x["id"]) == sorted( - [ - { - "id": str(access_reader.id), - "user": None, - "team": "readers", - "role": "reader", - "abilities": { - "destroy": True, - "retrieve": True, - "set_role_to": ["owner", "administrator", "editor"], - "update": True, - "partial_update": True, - }, - }, - { - "id": str(access_editor.id), - "user": None, - "team": "editors", - "role": "editor", - "abilities": { - "destroy": True, - "retrieve": True, - "set_role_to": ["owner", "administrator", "reader"], - "update": True, - "partial_update": True, - }, - }, - { - "id": str(access_administrator.id), - "user": None, - "team": "administrators", - "role": "administrator", - "abilities": { - "destroy": True, - "retrieve": True, - "set_role_to": ["owner", "editor", "reader"], - "update": True, - "partial_update": True, - }, - }, - { - "id": str(access_owner.id), - "user": None, - "team": "owners", - "role": "owner", - "abilities": { - # editable only if there is another owner role than the user's team... - "destroy": other_access.role == "owner", - "retrieve": True, - "set_role_to": ["administrator", "editor", "reader"] - if other_access.role == "owner" - else [], - "update": other_access.role == "owner", - "partial_update": other_access.role == "owner", - }, - }, - { - "id": str(other_access.id), - "user": None, - "team": other_access.team, - "role": other_access.role, - "abilities": other_access.get_abilities(user), - }, - ], - key=lambda x: x["id"], - ) assert response.json() == { "id": str(document.id), "title": document.title, diff --git a/src/backend/core/tests/documents/test_api_documents_update.py b/src/backend/core/tests/documents/test_api_documents_update.py index d0c1ae32..e333e49d 100644 --- a/src/backend/core/tests/documents/test_api_documents_update.py +++ b/src/backend/core/tests/documents/test_api_documents_update.py @@ -216,7 +216,7 @@ def test_api_documents_update_authenticated_editor_administrator_or_owner( document = models.Document.objects.get(pk=document.pk) document_values = serializers.DocumentSerializer(instance=document).data for key, value in document_values.items(): - if key in ["id", "accesses", "created_at", "link_reach", "link_role"]: + if key in ["id", "created_at", "link_reach", "link_role"]: assert value == old_document_values[key] elif key == "updated_at": assert value > old_document_values[key] @@ -255,7 +255,7 @@ def test_api_documents_update_authenticated_owners(via, mock_user_teams): document = models.Document.objects.get(pk=document.pk) document_values = serializers.DocumentSerializer(instance=document).data for key, value in document_values.items(): - if key in ["id", "accesses", "created_at", "link_reach", "link_role"]: + if key in ["id", "created_at", "link_reach", "link_role"]: assert value == old_document_values[key] elif key == "updated_at": assert value > old_document_values[key]