[ENG-807] refact:made parent json update only on tag update - #3728
[ENG-807] refact:made parent json update only on tag update#3728nandkishorr wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughChangesTagConfig parent-cache lifecycle
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR refactors how TagConfig caches and serves a tag’s parent JSON, shifting from time-based cache expiry to explicit cache refresh on tag configuration updates, and attempting to propagate updates to descendant tags.
Changes:
- Simplifies
get_parent_json()to return only the stored cached parent JSON. - Introduces
update_parent_json()and triggers it fromsave()to refresh cached parent JSON. - Adds descendant refresh logic (
update_child_cached_parent_json()) on updates.
| def update_parent_json(self): | ||
| with Lock(f"tag_config_parent_cache:{self.id}"): | ||
| if self.parent_id: | ||
| self.cached_parent_json = { | ||
| "id": str(self.parent.external_id), | ||
| "display": self.parent.display, | ||
| "description": self.parent.description, | ||
| "category": self.parent.category, | ||
| "parent": self.parent.cached_parent_json, | ||
| "level_cache": self.parent.level_cache, | ||
| } | ||
| super().save(update_fields=["cached_parent_json"]) |
| def update_child_cached_parent_json(self): | ||
| for child in TagConfig.objects.filter(parent=self).select_related("parent"): | ||
| child.update_parent_json() | ||
| child.update_child_cached_parent_json() |
| def get_parent_json(self): | ||
| if self.parent_id: | ||
| if self.cached_parent_json and timezone.now() < datetime.fromisoformat( | ||
| self.cached_parent_json["cache_expiry"] | ||
| ): | ||
| return self.cached_parent_json | ||
| self.parent.get_parent_json() | ||
| self.cached_parent_json = { | ||
| "id": str(self.parent.external_id), | ||
| "display": self.parent.display, | ||
| "description": self.parent.description, | ||
| "category": self.parent.category, | ||
| "parent": self.parent.cached_parent_json, | ||
| "level_cache": self.parent.level_cache, | ||
| "cache_expiry": str( | ||
| timezone.now() + timedelta(days=self.cache_expiry_days) | ||
| ), | ||
| } | ||
| self.save(update_fields=["cached_parent_json"]) | ||
| if self.parent_id and self.cached_parent_json: | ||
| return self.cached_parent_json | ||
| return {} |
| super().save(*args, **kwargs) | ||
| self.update_parent_json() | ||
| self.update_child_cached_parent_json() |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@care/emr/models/tag_config.py`:
- Around line 85-88: Update TagConfig parent assignment validation to reject
self-cycles and longer cycles before saving. Replace recursive
update_child_cached_parent_json traversal with an iterative descendant traversal
that tracks visited TagConfig primary keys, updates each child’s parent cache
once, and safely handles deep valid hierarchies.
- Around line 72-83: Update update_parent_json around the Lock acquisition and
save flow to handle ObjectLocked contention without allowing it to escape
save(). Add a bounded retry or guaranteed deferred retry that eventually
refreshes cached_parent_json, preserving the parent-derived fields and existing
super().save(update_fields=["cached_parent_json"]) behavior; do not silently
skip the refresh.
- Around line 90-98: Update TagConfig.save to wrap the model write and all cache
propagation calls (set_tag_config_cache, update_parent_json, and
update_child_cached_parent_json) in one transaction.atomic() unit, preserving
the existing create/update branches. Ensure ObjectLocked retry handling
surrounds the complete atomic unit so failures roll back the tag and every
descendant-cache change together.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: da5d9aa6-d312-4eb7-a1b6-2f1f705cda83
📒 Files selected for processing (1)
care/emr/models/tag_config.py
| def update_parent_json(self): | ||
| with Lock(f"tag_config_parent_cache:{self.id}"): | ||
| if self.parent_id: | ||
| self.cached_parent_json = { | ||
| "id": str(self.parent.external_id), | ||
| "display": self.parent.display, | ||
| "description": self.parent.description, | ||
| "category": self.parent.category, | ||
| "parent": self.parent.cached_parent_json, | ||
| "level_cache": self.parent.level_cache, | ||
| } | ||
| super().save(update_fields=["cached_parent_json"]) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Handle ObjectLocked before it escapes save().
Line 73 acquires a lock that raises ObjectLocked when another update holds the same key. This method does not handle that expected contention. Concurrent saves of one tag can fail after the model write starts.
Use a bounded retry or a deferred refresh with a guaranteed retry. Do not silently skip the refresh because get_parent_json() no longer has an expiry fallback.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@care/emr/models/tag_config.py` around lines 72 - 83, Update
update_parent_json around the Lock acquisition and save flow to handle
ObjectLocked contention without allowing it to escape save(). Add a bounded
retry or guaranteed deferred retry that eventually refreshes cached_parent_json,
preserving the parent-derived fields and existing
super().save(update_fields=["cached_parent_json"]) behavior; do not silently
skip the refresh.
| def update_child_cached_parent_json(self): | ||
| for child in TagConfig.objects.filter(parent=self).select_related("parent"): | ||
| child.update_parent_json() | ||
| child.update_child_cached_parent_json() |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Reject cyclic hierarchies and remove recursive traversal.
The self-referential parent relation can contain a self-cycle or a longer cycle. Lines 86-88 revisit the same TagConfig without detection. Each visit writes a deeper cached_parent_json before the process ends in RecursionError. A valid but deep hierarchy can also exceed the recursion limit.
Validate that parent assignments are acyclic. Traverse descendants iteratively and track visited primary keys.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@care/emr/models/tag_config.py` around lines 85 - 88, Update TagConfig parent
assignment validation to reject self-cycles and longer cycles before saving.
Replace recursive update_child_cached_parent_json traversal with an iterative
descendant traversal that tracks visited TagConfig primary keys, updates each
child’s parent cache once, and safely handles deep valid hierarchies.
| def save(self, *args, **kwargs): | ||
| if not self.id: | ||
| super().save(*args, **kwargs) | ||
| self.set_tag_config_cache() | ||
| self.update_parent_json() | ||
| else: | ||
| super().save(*args, **kwargs) | ||
| self.update_parent_json() | ||
| self.update_child_cached_parent_json() |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Commit the tag update and cache propagation as one unit.
Lines 92 and 96 persist the tag before lines 94 and 97-98 complete cache propagation. A later ObjectLocked or descendant update failure leaves the tag and only part of its descendant caches committed. The facility serializer returns this cached parent JSON directly, and the removed expiry check means stale responses can remain until another successful update.
Wrap the model write and full cache propagation in one transaction.atomic() unit. Retry lock contention around the complete unit.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@care/emr/models/tag_config.py` around lines 90 - 98, Update TagConfig.save to
wrap the model write and all cache propagation calls (set_tag_config_cache,
update_parent_json, and update_child_cached_parent_json) in one
transaction.atomic() unit, preserving the existing create/update branches.
Ensure ObjectLocked retry handling surrounds the complete atomic unit so
failures roll back the tag and every descendant-cache change together.
Proposed Changes
Associated Issue
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