Add model cache and n+1 issues #3496
Conversation
📝 WalkthroughWalkthroughEager-loaded related objects in several viewsets, centralized audit-user serialization into a shared helper, replaced direct object serialization with cache-backed Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@care/emr/models/patient.py`:
- Around line 193-198: The code calls .to_json() on the result of
model_from_cache in the block that populates cls.configs using
PatientIdentifierConfig and PatientIdentifierListSpec, but model_from_cache
already returns a dict causing an AttributeError; remove the .to_json() call and
store the dict directly into cls.configs[config_id] (keep the lookup using
PatientIdentifierConfig.objects.only("id").get(external_id=config_id) and the
model_from_cache(PatientIdentifierListSpec, id=config.id) call).
In `@care/emr/resources/resource_request/spec.py`:
- Around line 170-173: In perform_extra_serialization(cls, mapping, obj) add
mapping["id"] = str(obj.external_id) (when external_id is present) so the method
sets the serialized id for comment objects; locate perform_extra_serialization
in the ResourceRequestComment spec (same file) and mirror the pattern used in
ResourceRequestListSpec and ResourceRequestRetrieveSpec by converting
obj.external_id to a string and assigning it to mapping["id"] alongside the
existing mapping["created_by"] assignment.
🧹 Nitpick comments (4)
care/emr/resources/device/history_spec.py (1)
43-47: Consider handling the case wheremodel_from_cachereturnsNone.The code handles the "no user ID" edge case nicely with an empty dict on line 46. However,
model_from_cachereturnsNonewhen the user ID exists but the user isn't found in the cache/database (which is the defaultquiet=Truebehavior). This would result inhistory["updated_by"]beingNoneinstead of{}, creating slightly inconsistent responses within the sameedit_historylist.♻️ Suggested fix for consistent fallback
user = history.get("updated_by") if user: - history["updated_by"] = model_from_cache(UserSpec, id=user) + history["updated_by"] = model_from_cache(UserSpec, id=user) or {} else: history["updated_by"] = {} # Edge Casecare/emr/resources/file_upload/spec.py (1)
115-116: Cache lookup for updated_by is fine, thoughupdated_byisn't declared as a field.This works because
mappingis a dict that gets passed tomodel_construct, but I noticeFileUploadRetrieveSpecdoesn't declare anupdated_byfield likeFileUploadListSpecdeclaresuploaded_by: dict. This means the field won't be part of the Pydantic schema. If that's intentional, no problem — just thought you might want to know.Consider adding the field declaration for consistency
class FileUploadRetrieveSpec(FileUploadListSpec): signed_url: str | None = None read_signed_url: str | None = None internal_name: str # Not sure if this needs to be returned + updated_by: dict | None = Nonecare/emr/resources/observation/spec.py (2)
121-123: Type annotations inconsistent with other files in this PR.Other files (e.g.,
thread_spec.py,notes_spec.py) changedcreated_byandupdated_byfromUserSpec = {}todict = {}. This file still usesUserSpec = {}. It would be wonderful if these were consistent across the codebase.♻️ Suggested consistency fix
class ObservationReadSpec(BaseObservationSpec): - created_by: UserSpec = {} - updated_by: UserSpec = {} + created_by: dict = {} + updated_by: dict = {} data_entered_by: UserSpec | None = NoneAnd while we're at it,
data_entered_byshould probably bedict | None = Noneas well.
133-139: Local import could be moved to the top.The
model_from_cacheimport on line 135 is done locally inside the method, butmodel_from_cacheis already imported in other files at the module level without issues. SinceUserSpecis already imported at the top of this file (line 19), there's no circular import concern here. Moving this import to line 7 alongsideEMRResourcewould be cleaner.That said, the cache-based lookup for
data_entered_byis a solid improvement for avoiding N+1 queries.♻️ Suggested import cleanup
-from care.emr.resources.base import EMRResource +from care.emr.resources.base import EMRResource, model_from_cacheThen remove the local import on line 135-136.
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/models/patient.py (1)
189-199: Return type annotation no longer matches the actual return value.
get_confignow returns a cached dict frommodel_from_cache, but the annotation still saysPatientIdentifierConfig. That mismatch will mislead callers and type checks. Please update the return type (and any caller assumptions) accordingly.🔧 Suggested change
- def get_config(cls, config_id) -> PatientIdentifierConfig: + def get_config(cls, config_id) -> dict[str, object]:
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #3496 +/- ##
===========================================
+ Coverage 74.43% 74.46% +0.02%
===========================================
Files 471 471
Lines 21732 21690 -42
Branches 2264 2238 -26
===========================================
- Hits 16176 16151 -25
+ Misses 5055 5049 -6
+ Partials 501 490 -11 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Proposed Changes
Added caching for -
Associated Issue
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.