adding model caching and resolving n+1 issues#3484
Conversation
# Conflicts: # care/emr/api/viewsets/encounter.py # care/emr/resources/facility/spec.py # care/emr/resources/organization/spec.py
# Conflicts: # care/emr/resources/medication/dispense/dispense_order.py
📝 WalkthroughWalkthroughEager-load related objects with select_related in several viewsets; centralize audit-user serialization into a shared helper across many resource specs; replace direct User object serialization with cache-backed lookups via model_from_cache; and consolidate location/organization serialization to use ORM queries. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #3484 +/- ##
===========================================
+ Coverage 74.49% 74.54% +0.05%
===========================================
Files 471 471
Lines 21679 21628 -51
Branches 2257 2226 -31
===========================================
- Hits 16149 16122 -27
+ Misses 5029 5017 -12
+ Partials 501 489 -12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
# Conflicts: # care/emr/api/viewsets/questionnaire.py
…olve-n+1-issues # Conflicts: # care/emr/resources/patient/spec.py
praffq
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@care/emr/api/viewsets/encounter.py`:
- Around line 270-276: The list comprehension that builds data from
EncounterOrganization rows may include None when
model_from_cache(FacilityOrganizationReadSpec, id=org_id) misses the cache;
update the logic that creates data (using encounter_organizations and
model_from_cache) to filter out None results (or skip those org_ids) so the
returned list contains only valid FacilityOrganizationReadSpec objects; locate
the comprehension that assigns data and add a guard (e.g., only include entries
where model_from_cache returned a non-None value) to avoid nulls in the
response.
In `@care/emr/resources/consent/spec.py`:
- Around line 119-124: The code mutates obj.verification_details in-place
causing cached model data to be corrupted; instead, make a shallow or deep copy
of the list/dicts before modifying them (e.g., new_list = [dict(v) for v in
obj.verification_details] or use copy.deepcopy), then replace
mapping["verification_details"] with that modified copy; keep the use of
model_from_cache(UserSpec, external_id=...) but apply it to the copied dicts so
obj.verification_details remains untouched.
In `@care/emr/resources/device/history_spec.py`:
- Around line 43-47: The current logic sets history["updated_by"] to
model_from_cache(UserSpec, id=user) when user is truthy, which can yield None
for deleted/missing users, but sets {} when user is falsy, causing inconsistent
API responses; change the assignment so that the result of
model_from_cache(UserSpec, id=user) is normalized to {} when it is falsy (e.g.,
history["updated_by"] = model_from_cache(UserSpec, id=user) or {}), ensuring
history["updated_by"] is always an object; update the block that populates
history in the function handling edit_history to use this normalized value.
In `@care/emr/resources/healthcare_service/spec.py`:
- Around line 71-81: The current serialization loop reintroduces an N+1 by
querying FacilityLocation objects without preloading related objects used by
FacilityLocationListSpec (notably current_encounter); change the queryset in the
serialization block that iterates over
FacilityLocation.objects.filter(id__in=obj.locations) to preload the related
fields (e.g., use select_related('current_encounter') or appropriate
prefetch_related for any FK/M2M accessed by FacilityLocationListSpec) so
FacilityLocationListSpec.serialize(...) does not trigger extra DB queries.
In `@care/emr/resources/scheduling/resource/spec.py`:
- Around line 15-16: The current code does a fresh DB lookup via
FacilityLocation.objects.get(id=obj.location_id) causing N+1 queries; change it
to use the already-loaded relation accessor obj.location (and fall back safely
if missing) and pass that instance into
FacilityLocationListSpec.serialize(...).to_json() so
select_related("resource__location") will be honored; update the code paths that
use this logic (the serializer function/method containing
FacilityLocation.objects.get and the call to FacilityLocationListSpec.serialize)
to rely on obj.location instead of querying by id.
🧹 Nitpick comments (8)
care/emr/resources/medication/dispense/dispense_order.py (1)
68-80: Duplicated logic between parent and child class.The location fetching and serialization logic on lines 78-79 is identical to lines 63-64 in
MedicationDispenseOrderReadSpec. SinceMedicationDispenseOrderRetrieveSpecinherits fromMedicationDispenseOrderReadSpec, consider callingsuper().perform_extra_serialization(cls, mapping, obj)and then overriding only what differs (the patient serialization and audit users).This would reduce duplication and ensure any fix to the N+1 issue only needs to be applied once.
♻️ Suggested refactor
class MedicationDispenseOrderRetrieveSpec(MedicationDispenseOrderReadSpec): patient: dict = {} created_by: dict = {} updated_by: dict = {} `@classmethod` def perform_extra_serialization(cls, mapping, obj): + super().perform_extra_serialization(mapping, obj) cls.serialize_audit_users(mapping, obj) - mapping["id"] = obj.external_id - location = FacilityLocation.objects.get(id=obj.location_id) - mapping["location"] = FacilityLocationListSpec.serialize(location).to_json() mapping["patient"] = PatientRetrieveSpec.serialize(obj.patient).to_json()care/emr/resources/tag/config_spec.py (1)
177-194: Consider moving import to module level.The
model_from_cacheimport on line 179 is inside the method, whileEMRResourcefrom the same module is already imported at the top (line 14). Unless there's a circular import concern, moving this to the module-level imports would be more consistent with the rest of the codebase.That said, the actual logic change using
obj.facility_organization_idandobj.organization_idwithmodel_from_cacheis correct and resolves the N+1 nicely.♻️ Optional: Move import to module level
-from care.emr.resources.base import EMRResource, cacheable, model_string +from care.emr.resources.base import EMRResource, cacheable, model_from_cache, model_stringThen remove line 179.
care/emr/resources/service_request/spec.py (1)
169-172: Addselect_relatedto avoid N+1 queries during locations serialization.The
FacilityLocation.objects.filter(id__in=obj.locations)query doesn't prefetch related fields. SinceFacilityLocationListSpec.serialize()accesses bothobj.current_encounterandobj.parent(viaget_parent_json()), you'll trigger separate queries for each of the N locations.♻️ Suggested optimization
mapping["locations"] = [ FacilityLocationListSpec.serialize(location).to_json() - for location in FacilityLocation.objects.filter(id__in=obj.locations) + for location in FacilityLocation.objects.filter(id__in=obj.locations).select_related("current_encounter", "parent") ]care/emr/resources/file_upload/spec.py (1)
115-116: Consider usingserialize_audit_usersfor consistency.Other specs in this PR (e.g.,
ObservationReadSpec,ResourceRequestRetrieveSpec) delegate audit-field serialization tocls.serialize_audit_users(mapping, obj). This spec handlesupdated_bymanually while the parent handlesuploaded_by(which maps tocreated_by).If the intent is to only show
updated_byin the retrieve spec anduploaded_byin the list spec, the current approach is fine. Just noting the slight deviation from the pattern elsewhere.care/emr/api/viewsets/location.py (1)
180-186: Consider filtering out None values from the results.
model_from_cachereturnsNonewhen a model isn't found (withquiet=Trueby default). If an organization is deleted between fetching IDs and the cache lookup, the response could includeNoneentries in the results array.♻️ Suggested improvement
data = [ model_from_cache(FacilityOrganizationReadSpec, id=org_id) for org_id in location_organizations + if model_from_cache(FacilityOrganizationReadSpec, id=org_id) is not None ]Or more efficiently:
- data = [ - model_from_cache(FacilityOrganizationReadSpec, id=org_id) - for org_id in location_organizations - ] + data = [ + org_data + for org_id in location_organizations + if (org_data := model_from_cache(FacilityOrganizationReadSpec, id=org_id)) is not None + ]care/emr/api/viewsets/patient.py (1)
284-288: Same None-filtering consideration as other list comprehensions.If a user is deleted between the
PatientUserquery and the cache lookup,Nonecould appear in results. This is probably an edge case, but worth noting for consistency with how you handle it elsewhere.care/emr/resources/inventory/supply_request/request_order.py (1)
110-121: Use the ForeignKey relations directly to avoid unnecessary queries.
FacilityLocation.objects.get(...)makes extra trips to the database whenobj.originandobj.destinationare already available. While you're at it, might as well grab them straight from the model instance.♻️ Suggested tweak
- if obj.origin_id: - origin = FacilityLocation.objects.get(id=obj.origin_id) - mapping["origin"] = FacilityLocationListSpec.serialize(origin).to_json() - if obj.destination_id: - destination = FacilityLocation.objects.get(id=obj.destination_id) - mapping["destination"] = FacilityLocationListSpec.serialize( - destination - ).to_json() + if obj.origin_id: + mapping["origin"] = FacilityLocationListSpec.serialize(obj.origin).to_json() + if obj.destination_id: + mapping["destination"] = FacilityLocationListSpec.serialize( + obj.destination + ).to_json()care/emr/resources/organization/spec.py (1)
6-67: Avoid accidentally caching user-scoped retrieve specs.
@cacheableis inherited, soOrganizationRetrieveSpecbecomes cacheable too. If anyone ever usesmodel_from_cachewith it, permissions won’t be user-specific. It’s safer to explicitly disable caching on the retrieve spec.♻️ Suggested safeguard
class OrganizationRetrieveSpec(OrganizationReadSpec): + __cacheable__ = False permissions: list[str] = []
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
care/emr/resources/user/spec.py (1)
203-205: Bug: Loop variable shadowed by outer parameterobj.Line 204 still uses
objwhen it should use the loop variable from iterating overuser_facilities. This serializes the sameUserobject repeatedly instead of each facility.🐛 Proposed fix
mapping["facilities"] = [ - FacilityBareMinimumSpec.serialize(obj).to_json() for obj in user_facilities + FacilityBareMinimumSpec.serialize(facility).to_json() for facility in user_facilities ]
🧹 Nitpick comments (1)
care/emr/resources/facility/spec.py (1)
103-110: Addupdated_byfield declaration for consistency with other ReadSpec implementations.The
serialize_audit_usersmethod inbase.pyhandles bothcreated_by_idandupdated_by_id, and if the Facility model has anupdated_by_id, it will be included in the serialized output. However, FacilityReadSpec only declarescreated_by: dict = {}while nearly all other ReadSpec classes that callserialize_audit_usersdeclare both fields. Addingupdated_by: dict = {}aligns the implementation with the established pattern throughout the codebase.Suggested field addition
geo_organization: dict = {} created_by: dict = {} + updated_by: dict = {} invoice_number_expression: str | None = None
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@care/emr/resources/medication/dispense/dispense_order.py`:
- Around line 78-79: MedicationDispenseOrderRetrieveSpec duplicates per-object
DB lookups for location (FacilityLocation.objects.get(id=obj.location_id)),
causing an N+1; update the code to reuse the parent's serialization or read
obj.location directly and remove the separate query, and ensure the viewset that
provides these objects uses select_related("location") so obj.location is
pre-fetched; adjust MedicationDispenseOrderRetrieveSpec to call
FacilityLocationListSpec.serialize(obj.location).to_json() (or delegate to
MedicationDispenseOrderReadSpec’s serialization) instead of performing a new
FacilityLocation.objects.get.
- Around line 63-64: Add select_related('location', 'patient') to
DispenseOrderViewSet.get_queryset() so related objects are fetched in the same
query, then stop doing per-object lookups: replace the
FacilityLocation.objects.get(id=obj.location_id) usage and any similar manual
queries (lines using mapping["location"]) with direct access to obj.location and
serialize via FacilityLocationListSpec.serialize(obj.location).to_json(); other
code already accesses obj.patient directly so mirror that pattern. Ensure no
additional DB fetches remain for location in dispense_order.py.
🧹 Nitpick comments (1)
care/emr/resources/service_request/spec.py (1)
169-172: Consider addingselect_relatedto avoid N+1 oncurrent_encounter.The refactor consolidates the locations query nicely, but
FacilityLocationListSpec.perform_extra_serializationaccessesobj.current_encounter(seecare/emr/resources/location/spec.pylines 140-143). Without eager loading, this could trigger an additional query per location.♻️ Suggested improvement
mapping["locations"] = [ FacilityLocationListSpec.serialize(location).to_json() - for location in FacilityLocation.objects.filter(id__in=obj.locations) + for location in FacilityLocation.objects.filter(id__in=obj.locations).select_related("current_encounter") ]Based on learnings, optimize query performance using Django ORM's
select_relatedfor related object fetching.
|
resolved in #3496 |
1 participant
Proposed Changes
Added caching for -
Associated Issue
Architecture changes
Merge Checklist
/docsOnly PR's with test cases included and passing lint and test pipelines will be reviewed
@ohcnetwork/care-backend-maintainers @ohcnetwork/care-backend-admins
Summary by CodeRabbit
Bug Fixes
Performance Improvements
✏️ Tip: You can customize this high-level summary in your review settings.