HJ-319 Add DBCache model and table #5613
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
fides
|
||||||||||||||||||||||||||||||
| Project |
fides
|
| Branch Review |
refs/pull/5613/merge
|
| Run status |
|
| Run duration | 00m 50s |
| Commit |
|
| Committer | erosselli |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
0
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
4
|
Upgrade your plan to view test results. | |
| View all changes introduced in this branch ↗︎ | |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5613 +/- ##
==========================================
+ Coverage 87.15% 87.17% +0.01%
==========================================
Files 387 388 +1
Lines 23900 23931 +31
Branches 2585 2586 +1
==========================================
+ Hits 20830 20861 +31
Misses 2512 2512
Partials 558 558 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
this looks solid to me! just a nit on the sqlalchemy representation of the composite index 👍
edit: just a note to keep an eye on the downrev in your migration, i know there's at least one other in-flight PR that touches migrations
src/fides/api/models/db_cache.py
Outdated
| namespace = Column( | ||
| String, nullable=False, index=True | ||
| ) # Add a namespace since the same cache key could technically be used for different contexts | ||
| cache_key = Column(String, nullable=False, index=True) |
There was a problem hiding this comment.
hmm - i'm not sure how much it matters, but i think for a composite index (which this is, right?) it's generally specified a bit differently?
and we have some examples in our code base, e.g. here
There was a problem hiding this comment.
ah I forgot to specify the composite index on the model definition, I just added it in the migration 🤦♀️ I'll add it.
I did add index=True just to namespace thinking of queries like "Let's remove all entries for a namespace" , that don't have a specific cache_key but maybe the composite index still works for those kinds of queries? I'm a bit rusty on the specifics of composite indexes 😅
There was a problem hiding this comment.
heh, i'm not really an expert either. from my general understanding, i don't think the composite index would work well specifically for that use case - but i haven't validated that assumption at all.
i was aware of your intention with the two indexes there, and i didn't really question it - so i'd say it's fine to keep that in place! 👍
| String, nullable=False, index=True | ||
| ) # Add a namespace since the same cache key could technically be used for different contexts | ||
| cache_key = Column(String, nullable=False, index=True) | ||
| cache_value = Column(BYTEA, nullable=False) |
There was a problem hiding this comment.
ok, i hope this works well and i wasn't silly to suggest a byte column :)
fides
|
||||||||||||||||||||||||||||||
| Project |
fides
|
| Branch Review |
main
|
| Run status |
|
| Run duration | 00m 47s |
| Commit |
|
| Committer | erosselli |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
|
Failures
|
0
|
|
Flaky
|
0
|
|
Pending
|
0
|
|
Skipped
|
0
|
|
Passing
|
4
|
Upgrade your plan to view test results. | |
| View all changes introduced in this branch ↗︎ | |
Implements part of HJ-319
Description Of Changes
Adds a
DBCachemodel and table to be used for persistent caching. The table has thenamespaceandcache_keycolumns that uniquely determine a row, and the columncache_valueis used for storing an arbitrary value. For now we'll only use this for the list privacy experiences endpoint, but in the future it should be easy to add other things as well.Code Changes
DBCachemodel and its corresponding migrationSteps to Confirm
Pre-Merge Checklist
CHANGELOG.mdupdatedmaindowngrade()migration is correct and works