-
Notifications
You must be signed in to change notification settings - Fork 74
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
Add FastAPI Redis Caching #4195
Closed
Closed
Changes from all commits
Commits
Show all changes
20 commits
Select commit
Hold shift + click to select a range
1849fd3
feat: get logins working with drill
ThomasLaPiana 1d60191
feat: add more endpoints
ThomasLaPiana 5229381
feat: separate each endpoint into its own task
ThomasLaPiana 220f060
feat: optimize the Drill test
ThomasLaPiana eb0d7e7
feat: make the privacy-experience endpoint async
ThomasLaPiana 800a944
feat: sprinkle some `async sleep` magic into the privacy-experience e…
ThomasLaPiana abe119c
feat: remove an unused requirement
ThomasLaPiana 2ae8afb
fix: static checks
ThomasLaPiana 1f1f12b
Add FastAPI Redis Caching
ThomasLaPiana 6b33377
Merge branch 'add-more-drill-endpoints' into ThomasLaPiana-add-fastap…
ThomasLaPiana ffe5a7e
Merge branch 'main' into ThomasLaPiana-add-fastapi-redis-caching
ThomasLaPiana 1347246
fix: remove outdated redis import
ThomasLaPiana 161e271
feat: add caching to the privacy-experience endpoint
ThomasLaPiana 38f32bc
feat: rough POC of custom caching for the privacy experience endpoint
ThomasLaPiana 4d50fd9
feat: additional cleanup
ThomasLaPiana fdf1d95
Revert "feat: additional cleanup"
ThomasLaPiana cdb6e49
feat: cleanup, passing more tests
ThomasLaPiana be70d0c
feat: add headers to the privacy-experience response object indicatin…
ThomasLaPiana 2b64527
checkin: add headers and cache tests, but not passing
ThomasLaPiana a8a45a2
fix: more tests and almost all static checks
ThomasLaPiana File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Just want to comment early here. I see you're caching the output of the entire endpoint: now the performance gains make more sense. This is a neat strategy, I'm just more concerned about accuracy now. I had been assuming we'd instead cache some time-consuming individual pieces that go into the experience like the output of
get_tcf_contents
.For example,
fides_user_device_id
is a unique user identifier. If present, we supplement the experience with previously-saved preferences for that user. So if the user saves new preferences, this experience would still return their outdated preferences.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.
That makes sense! Without knowledge of what is actually going on here I can't make those fine-grained caching decisions.
So we can either handle this on the front-end using the header that resets the cache, or we can use a more fine-grained caching method in which I'd need some more guidance on what is safe to cache.
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.
real quick, something like the output of
get_tcf_contents
which is the bulk of TCF experiences and PrivacyExperience.get_related_privacy_notices which is for non-TCF experiences but perhaps the latter refactored into a separate method that isn't taking fides_user_device_id in, that is added separatelyThere 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.
Just a note here, I'm moving a great deal of privacy preferences / privacy experiences endpoints, supporting methods, TCF experiences, etc. to Fidesplus -
https://ethyca.atlassian.net/browse/PROD-1258
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.
thank you for the heads up! In that case I'll move this work over there. I haven't been able to circle back to this yet but Adam already figured it out for some other endpoints we have :)