Expires entity cache for related cases #2955
Conversation
|
@coderabbitai full review please |
|
@shubham1g5: I'll perform a comprehensive review of the changes in this PR, focusing on the entity cache expiration implementation for related cases. ✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughThe changes update several components of the project. A new Mockito dependency (version 5.5.0) is added for testing purposes. The application now includes a method, Sequence Diagram(s)sequenceDiagram
participant HomeScreenBaseActivity
participant CommCareApplication
participant WorkManager
participant EntityCacheInvalidationWorker
participant CommCareEntityStorageCache
HomeScreenBaseActivity->>CommCareApplication: propogateEntityCacheInvalidation()
CommCareApplication->>WorkManager: Enqueue OneTimeWorkRequest (EntityCacheInvalidationWorker)
WorkManager->>EntityCacheInvalidationWorker: Execute doWork()
EntityCacheInvalidationWorker->>CommCareEntityStorageCache: processShallowRecords()
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
app/src/org/commcare/models/database/user/models/CommCareEntityStorageCache.java (1)
399-410: Consider bulk inserts for large sets.
Although inserting records in a loop inside a transaction is acceptable, large sets might benefit from more efficient batch operations to reduce overhead.app/src/org/commcare/engine/cases/CaseUtils.java (1)
129-156: Consider adding error handling for user-groups fixture loading.The
getAllOwnersmethod could benefit from additional error handling around the fixture loading.private static Vector<String> getAllOwners() { Vector<String> users = new Vector<>(); Vector<String> owners = new Vector<>(); SqlStorage<User> userStorage = CommCareApplication.instance().getUserStorage(User.STORAGE_KEY, User.class); for (IStorageIterator<User> userIterator = userStorage.iterate(); userIterator.hasMore(); ) { String id = userIterator.nextRecord().getUniqueId(); owners.addElement(id); users.addElement(id); } //Now add all of the relevant groups //TODO: Wow. This is.... kind of megasketch for (String userId : users) { + try { DataInstance instance = CommCareUtil.loadFixture("user-groups", userId); if (instance == null) { continue; } EvaluationContext ec = new EvaluationContext(instance); for (TreeReference ref : ec.expandReference( XPathReference.getPathExpr("/groups/group/@id").getReference())) { AbstractTreeElement<AbstractTreeElement> idelement = ec.resolveReference(ref); if (idelement.getValue() != null) { owners.addElement(idelement.getValue().uncast().getString()); } } + } catch (Exception e) { + Logger.log(LogTypes.TYPE_ERROR_ASSERTION, + "Error loading user-groups fixture for user " + userId + ": " + e.getMessage()); + } } return owners; }app/src/org/commcare/CommCareApplication.java (2)
169-169: Fix typo in constant name.The constant name contains a typo: "ENTITY_CACHE_INVALIDATION_REQUEST".
- private static final String ENTITY_CACHE_INVALIDATION_REQUEST = "entity-cache-invalidation-request"; + private static final String ENTITY_CACHE_INVALIDATION_REQUEST = "entity-cache-invalidation";
1249-1257: Fix typo in method name and consider adding documentation.The method name contains a typo and lacks documentation explaining its purpose.
- public void propogateEntityCacheInvalidation() { + /** + * Propagates entity cache invalidation by scheduling a background worker to process + * shallow records and their related cases. + */ + public void propagateEntityCacheInvalidation() { OneTimeWorkRequest entityCacheInvalidationRequest = new OneTimeWorkRequest.Builder( EntityCacheInvalidationWorker.class) .setExpedited(OutOfQuotaPolicy.RUN_AS_NON_EXPEDITED_WORK_REQUEST) .build(); WorkManager wm = WorkManager.getInstance(CommCareApplication.instance()); wm.enqueueUniqueWork(ENTITY_CACHE_INVALIDATION_REQUEST, ExistingWorkPolicy.KEEP, entityCacheInvalidationRequest); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
app/build.gradle(1 hunks)app/src/org/commcare/CommCareApplication.java(4 hunks)app/src/org/commcare/activities/HomeScreenBaseActivity.java(3 hunks)app/src/org/commcare/engine/cases/CaseUtils.java(6 hunks)app/src/org/commcare/models/database/user/DatabaseUserOpenHelper.java(1 hunks)app/src/org/commcare/models/database/user/UserDatabaseUpgrader.java(2 hunks)app/src/org/commcare/models/database/user/models/CommCareEntityStorageCache.java(11 hunks)app/src/org/commcare/tasks/EntityCacheInvalidationWorker.kt(1 hunks)app/src/org/commcare/xml/AndroidBulkCaseXmlParser.java(1 hunks)app/src/org/commcare/xml/AndroidCaseXmlParser.java(1 hunks)app/unit-tests/resources/commcare-apps/case_list_lookup/restore.xml(3 hunks)app/unit-tests/resources/commcare-apps/index_and_cache_test/incremental_restore.xml(1 hunks)app/unit-tests/resources/commcare-apps/index_and_cache_test/suite.xml(2 hunks)app/unit-tests/src/org/commcare/android/tests/caselist/EntityListCacheIndexTest.java(2 hunks)
👮 Files not reviewed due to content moderation or server errors (4)
- app/src/org/commcare/xml/AndroidBulkCaseXmlParser.java
- app/src/org/commcare/models/database/user/DatabaseUserOpenHelper.java
- app/src/org/commcare/engine/cases/CaseUtils.java
- app/src/org/commcare/models/database/user/UserDatabaseUpgrader.java
🔇 Additional comments (44)
app/src/org/commcare/xml/AndroidCaseXmlParser.java (1)
99-101:Details
✅ Verification successful
LGTM! Verify method signature change.
The change from
invalidateCachetoinvalidateRecordaligns with the PR's objective of implementing a more granular cache expiration mechanism.Run the following script to verify the method signature change:
🏁 Script executed:
#!/bin/bash # Description: Verify the method signature change from invalidateCache to invalidateRecord. # Test: Search for any remaining occurrences of invalidateCache. Expect: None. rg -A 5 $'invalidateCache' # Test: Search for the new method signature. Expect: Consistent usage. rg -A 5 $'invalidateRecord'Length of output: 3723
LGTM – The update to call
invalidateRecordinAndroidCaseXmlParser.javais consistent with the new granular cache expiration mechanism.
- The script confirms that within this context, the change from
invalidateCachetoinvalidateRecordis applied correctly.invalidateRecordis also used consistently in related files (AndroidBulkCaseXmlParser.javaandCommCareEntityStorageCache.java), supporting the intended design change.- The remaining occurrences of
invalidateCache(e.g., inCommCareApplication.java) appear to serve a different purpose and seem unrelated to this PR change.app/src/org/commcare/models/database/user/models/CommCareEntityStorageCache.java (12)
9-10: No actionable concerns for these new imports.
18-18: No actionable concerns for this import.
32-32: No actionable concerns for this import.
35-35: No actionable concerns for this import.
53-56: New column definition looks good.
The inclusion of theCOL_IS_SHALLOWcolumn in the model is clear, and the comment clarifies the purpose effectively.
80-80: Verify database migration path for the new column.
Ensure that the database upgrader correctly handles addingCOL_IS_SHALLOWto existing installs so that no migrations are skipped or improperly applied.Would you like me to generate a shell script to grep upgrade steps and confirm that versioning changes are properly incremented?
148-159: Transaction-based approach is well-implemented.
Wrapping the shallow-mark logic in a transaction is correct for consistency and atomicity.
164-164: Confirm safety of string concatenation in SQL.
mCacheNameis inserted into the WHERE clause via string concatenation. If it’s controlled internally, this is likely safe. Otherwise, parameterize it to avoid injection risk.
176-189: Method correctly retrieves shallow record keys.
Collecting shallow IDs into aSet<String>is straightforward and efficient enough for typical usage.
237-238: Good call to process shallow records before cache priming.
Ensures that stale records are removed prior to loading fresh cache data.
280-284: Shallow record processing logic appears sound.
This method neatly fetches the shallow records, expands to their related cases, and invalidates them.
412-431: Cache-count checks appear consistent.
CallingisEmpty()andgetCount()helps avoid unnecessary operations if the cache is unused.app/src/org/commcare/tasks/EntityCacheInvalidationWorker.kt (1)
10-23: Coroutine-based worker implementation is clear.
The usage of a try-catch block withindoWork()provides graceful error handling, and the logic effectively triggers shallow-record processing on the “case” cache.app/unit-tests/src/org/commcare/android/tests/caselist/EntityListCacheIndexTest.java (9)
3-6: No actionable concerns for these new test imports.
7-7: No actionable concerns for this import.
16-17: No actionable concerns for these new imports.
21-22: No actionable concerns for these new imports.
24-24: No actionable concerns for this import.
26-27: No actionable concerns for these new imports.
41-42: Parameterized test field is a solid approach.
Using a boolean parameter for bulk vs. non-bulk testing increases coverage and ensures code paths are verified for both scenarios.
44-50: Data provider for parameterized tests is concise and clear.
Ensures coverage for both true and false states of bulk processing.
60-86: Test method thoroughly covers cache expiration scenarios.
It validates that after receiving an incremental restore, shallow records are processed and relevant cached fields are nullified. This robustly verifies the new invalidation logic.app/src/org/commcare/xml/AndroidBulkCaseXmlParser.java (3)
86-86: LGTM! Method name change aligns with new cache invalidation strategy.The change from
invalidateCachestoinvalidateRecordsreflects the new granular approach to cache invalidation, maintaining the same functionality within the transaction.
86-86: LGTM! Method name updated to match new cache invalidation strategy.The change from
invalidateCachestoinvalidateRecordsaligns with the new entity cache management approach, maintaining transactional integrity.
86-86: LGTM! Method name change improves clarity.The change from
invalidateCachestoinvalidateRecordsbetter reflects the granular nature of the operation, as it's invalidating specific records rather than entire caches.app/src/org/commcare/models/database/user/DatabaseUserOpenHelper.java (3)
69-69: LGTM! Database version and documentation updated correctly.The database version increment and documentation for the new columns (
is_dirtyandis_shallow) in the entity_cache table follow proper versioning practices.Also applies to: 72-72
69-69: LGTM! Database version and documentation updated correctly.The version increment and documentation for the new columns (
is_dirtyandis_shallow) in the entity_cache table follow proper database versioning practices.Also applies to: 72-72
69-72: LGTM! Proper database versioning practices followed.The database version increment and documentation clearly indicate the schema changes for the entity_cache table. The addition of
is_dirtyandis_shallowcolumns aligns with the PR's objective of implementing cache expiration for related cases.app/src/org/commcare/engine/cases/CaseUtils.java (5)
3-4: LGTM! Static imports improve code readability.The static imports for
INDEX_CASE_IDandxordataimprove code readability by reducing qualification.
164-180: LGTM! Well-implemented method for retrieving related cases.The
getRelatedCasesmethod is well-structured with:
- Proper null/empty checks
- Efficient use of data structures
- Clear documentation
3-4: LGTM! Static imports improve code readability.The static imports for
INDEX_CASE_IDandxordatareduce code verbosity and improve maintainability.
3-4: LGTM! Improved code organization with static imports.The static imports for
INDEX_CASE_IDandxordataimprove code readability by reducing verbosity.
164-180: LGTM! Well-implemented related cases retrieval.The
getRelatedCasesmethod is well-structured with:
- Proper empty check optimization
- Clear transaction boundaries
- Efficient use of the case graph for finding related records
app/src/org/commcare/models/database/user/UserDatabaseUpgrader.java (3)
800-811: LGTM! Database upgrade follows established patterns.The upgrade method for version 29 follows the same transaction handling pattern as other upgrades and properly recreates the entity cache table.
800-811: LGTM! Database upgrade method follows established patterns.The upgrade method properly handles transactions and follows the same pattern as other upgrade methods in the class.
800-811: LGTM! Clean and consistent database upgrade implementation.The upgrade method follows the established pattern with proper transaction handling and clear, focused responsibility of updating the entity cache table schema.
app/src/org/commcare/activities/HomeScreenBaseActivity.java (1)
926-926: LGTM! Cache invalidation is correctly triggered after form submission.The entity cache invalidation is appropriately triggered after a successful form submission, ensuring that any cached data is refreshed when form data changes.
app/unit-tests/resources/commcare-apps/index_and_cache_test/incremental_restore.xml (1)
1-18: LGTM! Well-structured test data for cache invalidation.The test XML file provides a good example of case data with create and update sections, following the OpenRosa response format.
app/unit-tests/resources/commcare-apps/case_list_lookup/restore.xml (1)
17-19: LGTM! Well-structured case relationships for testing cache invalidation.The added index elements correctly establish parent-child relationships between cases, forming a chain that can be used to test cache invalidation propagation.
Also applies to: 33-35
app/unit-tests/resources/commcare-apps/index_and_cache_test/suite.xml (3)
76-76: Enable Caching on Detail Element
The addition of thecache_enabled="true"attribute on the<detail id="m1_case_short">element clearly signals that caching is activated for this detail. This change aligns well with the PR objective of expiring and refreshing the entity cache for related cases.
82-82: Enable Caching on Field
Adding thecache_enabled="true"attribute on the<field>element withinm1_case_shortensures that caching behavior is applied at a more granular level. This is consistent with the intention to control caching for parts of the entity.
93-93: Refine Sort Element Configuration
The removal of theorderattribute from the<sort>element simplifies its definition while retaining the necessary attributes (typeanddirection). Verify that this alteration does not unintentionally impact the sorting behavior in contexts where a specific order was previously expected.app/build.gradle (1)
45-45: Add Mockito Dependency for Enhanced Testing
The new dependencytestImplementation 'org.mockito:mockito-core:5.5.0'is added to support advanced mocking scenarios in your tests. Please confirm that its introduction alongside other mocking libraries (e.g.,io.mockk:mockk:1.12.7) is intentional and that no conflicts arise from using multiple frameworks.
app/src/org/commcare/models/database/user/models/CommCareEntityStorageCache.java
Show resolved
Hide resolved
shubham1g5
force-pushed
the
cacheExpirationChanges
branch
3 times, most recently
from
51aa67d to
8168c2b
Compare
… shallow records when invalidating cache
…an Android worker
… setup error with work manager with the demoUsertest
shubham1g5
force-pushed
the
cacheExpirationChanges
branch
from
8168c2b to
b6668b9
Compare
ctsims
left a comment
ctsims
left a comment
There was a problem hiding this comment.
Added some points of feedback but most of them are performance related.
I'm not sure if we know what the spanning set of people using Cache and Index is, but I'm generally pretty worried that this set of changes might have a significant impact on where performance is felt for a current C&I user.
Not all of my feedback here would be mandatory to process upfront if there was a clean way to disable the caching cleaning strategy if it's having significant impacts on runtime, since I do think that creating new mandatory full walks of the DB can be risky.
| // The form is either ready for processing, or not, depending on how it was saved | ||
| if (complete) { | ||
| startUnsentFormsTask(false, false); | ||
| CommCareApplication.instance().scheduleEntityCacheInvalidation(); |
There was a problem hiding this comment.
Two quick things:
1- If the form is 'complete' and not 'unsent' that means it wasn't processed, right? A little concerned about the possibility that the UnsentForms task might not have finished processing the form before the actual underlying work here is performed.
2 - Seems a little odd to queue this if there is no caching going on with the current app. I still don't think we're ready to view caching as the default, so I'm interested (where it's not a ton of work) in still minimizing the new surface area non-cache-and-index apps are exposed to from the changes.
| int removedCaseCount; | ||
| int removedLedgers; | ||
| try { | ||
| db.beginTransaction(); |
There was a problem hiding this comment.
I believe our pattern is for these to stay outside of the transaction when being used as begin/success/end, otherwise if this method itself throws an error and the transaction wasn't started, the endTransaction call could unintentionally end a separate transaction instead of this one.
| db = CommCareApplication.instance().getUserDbHandle(); | ||
|
|
||
| db.beginTransaction(); | ||
| Vector<String> owners = getAllOwners(); |
There was a problem hiding this comment.
Thanks for adding all of these little refactors.
| new Vector<>()); | ||
| Set<String> caseIds = getCaseIdsFromRecordsIds(storage, recordIds); | ||
| Set<String> relatedCaseIds = fullCaseGraph.getRelatedRecords(caseIds); | ||
| Set<String> relatedRecordIds = new HashSet<>(); |
There was a problem hiding this comment.
Fully optional, but I bet our current version of java might have a way to apply this MAP operation with an internal collections lambda or something that might be a little more terse.
| Set<String> relatedCaseIds = fullCaseGraph.getRelatedRecords(caseIds); | ||
| Set<String> relatedRecordIds = new HashSet<>(); | ||
| for (String relatedCaseId : relatedCaseIds) { | ||
| String relatedRecordId = getRecordIdFromCaseId(storage, relatedCaseId); |
There was a problem hiding this comment.
When this set of ID's is large (which it might be fairly often) it will be very expensive to be doing these reads one-by-one.
There are a few options for how to do this faster. One of the methods above that does a trawl of the full sets of case data must have been able to retain this mapping, so just keeping it around might be enough, but otherwise doing a bulk read type operator is a much more efficient option.
| if (result == null || result.isEmpty()) { | ||
| throw new IllegalStateException("No record found for case ID: " + caseId); | ||
| } | ||
| return String.valueOf(result.get(0)); |
There was a problem hiding this comment.
I'm a little confused a to why these ints are getting string casts in/out a bunch. I seems like this is could cause a fair amount of unnecessary memory churn (strings aren't exactly efficient), but would also communicate more clearly if these signatures stayed in integer as much as possible to prevent confusion between CaseID and RecordID.
| /** | ||
| * Gets all shallow records and it's related graph and delete those records from the cache | ||
| */ | ||
| public void processShallowRecords() { |
There was a problem hiding this comment.
Since this is getting schedule asynchronously, I think this method itself might need to be thread protected or something should prevent simultaneous management of the cache while it's happening.
…empty cache to schedule cache invalidation
…ange the unique work policy to replace to avoid duplicate work in case of multiple requests
@ctsims If required, we can do a check for new cache and index config on the app before scheduling cache invalidation, but given the expensive invalidation is part of background processing, I was not too worried about it. (the synchronous cache invalidation in case background processing didn't happen in time only happens for new cache and index config) |
ctsims
left a comment
ctsims
left a comment
There was a problem hiding this comment.
I think still some things to work through on the core idea but nothing blocking from me for now
Product Description
Spec
https://dimagi.atlassian.net/browse/SC-4279
Expires cache for all related cases in order to support caching xpath calculation results for related cases like instance('casedb')/casedb/case[@case_type='mother'][@case_id=current()/index/parent]/case_name. Without expiring related cases from cache, these kind of expressions may result in obsolete data to the user.
Technical Summary
Alternate approach to #2944
Adds a new field
is_shallowin the entity cache to signify any cases that have changed from the last time the cache was primed. These shallow records are then further processed by a background worker after form submission and restores to delete any cache records related to these cases or their related cases.Feature Flag
CACHE_AND_LAZY_LOAD
Safety Assurance
Safety story
should only affect projects implementing caching for case list.
Automated test coverage
Adds a test to verify the cache expiration behavior
QA Plan
This whole feature will be QA'ed as part of mobile release cycle.
Labels and Review
cross-request: dimagi/commcare-core#1460