[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 4 commits into
Conversation
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthrough
ChangesTagConfig parent-cache lifecycle
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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.
| 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 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.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3728 +/- ##
========================================
Coverage 79.45% 79.46%
========================================
Files 480 480
Lines 23213 23218 +5
Branches 2420 2421 +1
========================================
+ Hits 18444 18450 +6
+ Misses 4165 4164 -1
Partials 604 604 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@coderabbitai please re-review |
|
|
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
Summary by CodeRabbit