From 0d0284b82f067bdd12c30641def912f2f3c43443 Mon Sep 17 00:00:00 2001 From: dhruv-goyal-10 Date: Fri, 23 Feb 2024 05:25:23 +0530 Subject: [PATCH 1/6] Fixed N+1 queries issue in /api/v1/bed/ --- care/facility/api/serializers/bed.py | 8 ++++++-- care/facility/api/viewsets/bed.py | 14 ++++++++++++-- 2 files changed, 18 insertions(+), 4 deletions(-) diff --git a/care/facility/api/serializers/bed.py b/care/facility/api/serializers/bed.py index 6daf57d609..6e32eae8ff 100644 --- a/care/facility/api/serializers/bed.py +++ b/care/facility/api/serializers/bed.py @@ -4,7 +4,6 @@ from django.utils import timezone from rest_framework.exceptions import ValidationError from rest_framework.serializers import ( - BooleanField, CharField, DateTimeField, IntegerField, @@ -40,7 +39,7 @@ class BedSerializer(ModelSerializer): bed_type = ChoiceField(choices=BedTypeChoices) location_object = AssetLocationSerializer(source="location", read_only=True) - is_occupied = BooleanField(default=False, read_only=True) + is_occupied = SerializerMethodField() location = UUIDField(write_only=True, required=True) facility = UUIDField(write_only=True, required=True) @@ -83,6 +82,11 @@ def validate(self, attrs): ) return super().validate(attrs) + def get_is_occupied(self, instance): + if hasattr(instance, "_is_occupied"): + return instance._is_occupied + return instance.is_occupied + class AssetBedSerializer(ModelSerializer): id = UUIDField(source="external_id", read_only=True) diff --git a/care/facility/api/viewsets/bed.py b/care/facility/api/viewsets/bed.py index 8bce2cb17b..aa589ac843 100644 --- a/care/facility/api/viewsets/bed.py +++ b/care/facility/api/viewsets/bed.py @@ -1,6 +1,6 @@ from django.core.exceptions import ValidationError as DjangoValidationError from django.db import IntegrityError, transaction -from django.db.models import OuterRef, Subquery +from django.db.models import Exists, OuterRef, Subquery from django_filters import rest_framework as filters from drf_spectacular.utils import extend_schema, extend_schema_view from rest_framework import filters as drf_filters @@ -50,7 +50,7 @@ class BedViewSet( ): queryset = ( Bed.objects.all() - .select_related("facility", "location") + .select_related("facility", "location", "location__facility") .order_by("-created_date") ) serializer_class = BedSerializer @@ -63,6 +63,16 @@ class BedViewSet( def get_queryset(self): user = self.request.user queryset = self.queryset + + if self.action == "list": + queryset = queryset.annotate( + _is_occupied=Exists( + ConsultationBed.objects.filter( + bed__id=OuterRef("id"), end_date__isnull=True + ) + ) + ) + if user.is_superuser: pass elif user.user_type >= User.TYPE_VALUE_MAP["StateLabAdmin"]: From 3a7e201e27a757e2b44a10fe4170e4bfa8b1f432 Mon Sep 17 00:00:00 2001 From: dhruv-goyal-10 Date: Mon, 26 Feb 2024 19:41:32 +0530 Subject: [PATCH 2/6] Feat: PR review changes for issue #1338 Removed is_occupied property from Bed model as it is not used anywhere Annotated is_occupied field in the queryset instead of _is_occupied --- care/facility/api/serializers/bed.py | 2 -- care/facility/api/viewsets/bed.py | 2 +- care/facility/models/bed.py | 4 ---- 3 files changed, 1 insertion(+), 7 deletions(-) diff --git a/care/facility/api/serializers/bed.py b/care/facility/api/serializers/bed.py index 6e32eae8ff..9f45991441 100644 --- a/care/facility/api/serializers/bed.py +++ b/care/facility/api/serializers/bed.py @@ -83,8 +83,6 @@ def validate(self, attrs): return super().validate(attrs) def get_is_occupied(self, instance): - if hasattr(instance, "_is_occupied"): - return instance._is_occupied return instance.is_occupied diff --git a/care/facility/api/viewsets/bed.py b/care/facility/api/viewsets/bed.py index aa589ac843..c2d5914286 100644 --- a/care/facility/api/viewsets/bed.py +++ b/care/facility/api/viewsets/bed.py @@ -66,7 +66,7 @@ def get_queryset(self): if self.action == "list": queryset = queryset.annotate( - _is_occupied=Exists( + is_occupied=Exists( ConsultationBed.objects.filter( bed__id=OuterRef("id"), end_date__isnull=True ) diff --git a/care/facility/models/bed.py b/care/facility/models/bed.py index d701dba41a..1f6ae30777 100644 --- a/care/facility/models/bed.py +++ b/care/facility/models/bed.py @@ -40,10 +40,6 @@ class Meta: ) ] - @property - def is_occupied(self) -> bool: - return ConsultationBed.objects.filter(bed=self, end_date__isnull=True).exists() - def __str__(self): return self.name From ca99c229e1ec790a7fb926bca3ecaa31a7cf2510 Mon Sep 17 00:00:00 2001 From: dhruv-goyal-10 Date: Mon, 26 Feb 2024 22:35:58 +0530 Subject: [PATCH 3/6] Feat: PR review changes Changed is_occupied from SerializerMethodField to Boolean Field --- care/facility/api/serializers/bed.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/care/facility/api/serializers/bed.py b/care/facility/api/serializers/bed.py index 9f45991441..6daf57d609 100644 --- a/care/facility/api/serializers/bed.py +++ b/care/facility/api/serializers/bed.py @@ -4,6 +4,7 @@ from django.utils import timezone from rest_framework.exceptions import ValidationError from rest_framework.serializers import ( + BooleanField, CharField, DateTimeField, IntegerField, @@ -39,7 +40,7 @@ class BedSerializer(ModelSerializer): bed_type = ChoiceField(choices=BedTypeChoices) location_object = AssetLocationSerializer(source="location", read_only=True) - is_occupied = SerializerMethodField() + is_occupied = BooleanField(default=False, read_only=True) location = UUIDField(write_only=True, required=True) facility = UUIDField(write_only=True, required=True) @@ -82,9 +83,6 @@ def validate(self, attrs): ) return super().validate(attrs) - def get_is_occupied(self, instance): - return instance.is_occupied - class AssetBedSerializer(ModelSerializer): id = UUIDField(source="external_id", read_only=True) From ce4a940e0048f8494dc8abe7994db2d138e766c7 Mon Sep 17 00:00:00 2001 From: dhruv-goyal-10 Date: Tue, 27 Feb 2024 01:05:21 +0530 Subject: [PATCH 4/6] Feat: PR Review Changes Annotated is_occupied field irrespective of action --- care/facility/api/viewsets/bed.py | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/care/facility/api/viewsets/bed.py b/care/facility/api/viewsets/bed.py index c2d5914286..cdda8af3c1 100644 --- a/care/facility/api/viewsets/bed.py +++ b/care/facility/api/viewsets/bed.py @@ -64,14 +64,13 @@ def get_queryset(self): user = self.request.user queryset = self.queryset - if self.action == "list": - queryset = queryset.annotate( - is_occupied=Exists( - ConsultationBed.objects.filter( - bed__id=OuterRef("id"), end_date__isnull=True - ) + queryset = queryset.annotate( + is_occupied=Exists( + ConsultationBed.objects.filter( + bed__id=OuterRef("id"), end_date__isnull=True ) ) + ) if user.is_superuser: pass From e9ea4b43eaf3e4ff7a1e123277e7ae2e0755b4a3 Mon Sep 17 00:00:00 2001 From: dhruv-goyal-10 Date: Thu, 29 Feb 2024 07:39:49 +0530 Subject: [PATCH 5/6] Added the test case for listing beds API --- care/facility/tests/test_bed_api.py | 37 +++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) create mode 100644 care/facility/tests/test_bed_api.py diff --git a/care/facility/tests/test_bed_api.py b/care/facility/tests/test_bed_api.py new file mode 100644 index 0000000000..c6bee6aa4e --- /dev/null +++ b/care/facility/tests/test_bed_api.py @@ -0,0 +1,37 @@ +from rest_framework import status +from rest_framework.test import APITestCase + +from care.facility.models import Bed +from care.utils.tests.test_utils import TestUtils + + +class BedViewSetTestCase(TestUtils, APITestCase): + @classmethod + def setUpTestData(cls) -> None: + cls.state = cls.create_state() + cls.district = cls.create_district(cls.state) + cls.local_body = cls.create_local_body(cls.district) + cls.super_user = cls.create_super_user("su", cls.district) + cls.facility = cls.create_facility(cls.super_user, cls.district, cls.local_body) + cls.asset_location = cls.create_asset_location(cls.facility) + cls.user = cls.create_user("staff", cls.district, home_facility=cls.facility) + cls.patient = cls.create_patient( + cls.district, cls.facility, local_body=cls.local_body + ) + + def setUp(self) -> None: + super().setUp() + self.bed1 = Bed.objects.create( + name="bed1", location=self.asset_location, facility=self.facility + ) + self.bed2 = Bed.objects.create( + name="bed2", location=self.asset_location, facility=self.facility + ) + self.bed3 = Bed.objects.create( + name="bed3", location=self.asset_location, facility=self.facility + ) + + def test_list_beds(self): + with self.assertNumQueries(5): + response = self.client.get("/api/v1/bed/") + self.assertEqual(response.status_code, status.HTTP_200_OK) From a8372ba630de3e415d21a2038953c508f45643a5 Mon Sep 17 00:00:00 2001 From: Aakash Singh Date: Thu, 29 Feb 2024 09:33:10 +0530 Subject: [PATCH 6/6] add comment explaining number of queries --- care/facility/tests/test_bed_api.py | 1 + 1 file changed, 1 insertion(+) diff --git a/care/facility/tests/test_bed_api.py b/care/facility/tests/test_bed_api.py index c6bee6aa4e..973db41a96 100644 --- a/care/facility/tests/test_bed_api.py +++ b/care/facility/tests/test_bed_api.py @@ -32,6 +32,7 @@ def setUp(self) -> None: ) def test_list_beds(self): + # includes 3 queries for auth and 1 for pagination count with self.assertNumQueries(5): response = self.client.get("/api/v1/bed/") self.assertEqual(response.status_code, status.HTTP_200_OK)