-
Notifications
You must be signed in to change notification settings - Fork 86
Eng 1740 fallback to database identities if identities have expired from redis #6896
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Eng 1740 fallback to database identities if identities have expired from redis #6896
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
…identities-have-expired-from-redis
…identities-have-expired-from-redis
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6896 +/- ##
=======================================
Coverage 87.29% 87.29%
=======================================
Files 523 523
Lines 34172 34188 +16
Branches 3932 3934 +2
=======================================
+ Hits 29829 29845 +16
Misses 3487 3487
Partials 856 856 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…identities-have-expired-from-redis
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Greptile Overview
Greptile Summary
Implements fallback logic to retrieve identity and custom privacy request fields from the database when Redis cache expires, preventing privacy requests from failing when cached data is no longer available.
Key changes:
- Added
verify_cache_for_identity_data()method to check if identity data exists in cache without triggering fallback - Enhanced
get_cached_identity_data()to automatically fetch from DB and re-cache when cache miss occurs - Enhanced
get_cached_custom_privacy_request_fields()with similar fallback behavior - Updated restart endpoint to use verification method instead of retrieval method for cache checking
- Added comprehensive test coverage for fallback scenarios
Test improvements:
- Removed manual cache deletion in test fixtures, following best practices (f4371da8)
- Added tests for both successful fallback and edge cases (no persisted identity)
Confidence Score: 5/5
- This PR is safe to merge with minimal risk
- The implementation is straightforward and defensive - adding fallback logic that prevents failures when cache expires. The changes are well-tested with multiple test cases covering edge cases. No breaking changes or risky refactoring. The endpoint change correctly delegates to a new verification method that doesn't trigger side effects.
- No files require special attention
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| src/fides/api/models/privacy_request/privacy_request.py | 4/5 | Added fallback logic to retrieve identity and custom fields from DB when Redis cache expires, plus helper method to verify cache presence |
| src/fides/api/api/v1/endpoints/privacy_request_endpoints.py | 5/5 | Updated restart endpoint to use new verify_cache_for_identity_data() method instead of get_cached_identity_data() for cache verification |
| tests/ops/models/privacy_request/test_privacy_request.py | 5/5 | Added comprehensive tests for cache fallback behavior for identity and custom privacy request fields |
4 files reviewed, no comments
| prefix = f"id-{self.id}-identity-*" | ||
| cache: FidesopsRedis = get_cache() | ||
| keys = cache.keys(prefix) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe we can move these 3 lines to a private method that we can reuse in this verify_cache_for_identity_data function and also in get_cached_identity_data ?
| @pytest.fixture(scope="function") | ||
| def third_privacy_request_awaiting_erasure_email_send( | ||
| db: Session, erasure_policy: Policy | ||
| ) -> PrivacyRequest: | ||
| """Add a third erasure privacy request w/ no identity in this state for these tests""" | ||
| privacy_request = _create_privacy_request_for_policy( | ||
| db, | ||
| erasure_policy, | ||
| ) | ||
| privacy_request.status = PrivacyRequestStatus.awaiting_email_send | ||
| privacy_request.save(db) | ||
| yield privacy_request | ||
| privacy_request.delete(db) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the reason for deleting this fixture ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is un-used with the changes. The _create_privacy_request_for_policy persists and identity and in the tests we were deleting it. That doesn't really work for the tests anymore since we just go get it from the db now.
…identities-have-expired-from-redis
…identities-have-expired-from-redis
Ticket ENG-1740
Description Of Changes
🎯 Whenever we try to read identities or custom privacy request fields from Redis, fallback to getting the data from the database if the values have already expired.
Refer to the updates to get_cached_identity_data and get_cached_custom_privacy_request_fields in this closed PR Privacy request cache refresh by galvana · Pull Request #4610 · ethyca/fides
Code Changes
Steps to Confirm
PATCH /api/v1/configwith:redis-cli -a your_passwordDEL id-<<privacy_request_id here>>-identity-emailKEYS *to verify that key is gonePre-Merge Checklist
CHANGELOG.mdupdatedmaindowngrade()migration is correct and works