ENG-3288 Add onupdate to PrivacyPreferences.updated_at column#7851
ENG-3288 Add onupdate to PrivacyPreferences.updated_at column#7851
Conversation
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7851 +/- ##
=======================================
Coverage 85.06% 85.06%
=======================================
Files 627 627
Lines 40765 40767 +2
Branches 4740 4740
=======================================
+ Hits 34678 34680 +2
Misses 5018 5018
Partials 1069 1069 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
The fix is correct and minimal. Adding onupdate=func.now() to PrivacyPreferences.updated_at is consistent with the pattern used across the codebase (including FidesBase.updated_at, privacy_preference.py, and many others), and no database migration is needed since this is a SQLAlchemy ORM-level hook, not a schema change.
Two observations worth noting:
-
Raw SQL paths bypass
onupdate—onupdate=func.now()only triggers via the ORM's unit-of-work.consent_encryption_migration.pyissues rawUPDATESQL against this table, so those updates won't stampupdated_at. This is a pre-existing limitation, not a regression, but it's a gotcha if anyone relies on this column being current after running the encryption migration CLI command. -
NULL on INSERT is intentional but undocumented — unlike
FidesBase, which setsserver_default=func.now()onupdated_at, this column staysNULLuntil the first ORM-level update. That's a defensible design choice (you can tell "never updated" from "updated at T"), but a short comment on the column definition would help future readers avoid confusion.
No blocking issues. The core fix is sound.
| DateTime(timezone=True), | ||
| nullable=True, | ||
| onupdate=func.now(), | ||
| ) |
There was a problem hiding this comment.
The onupdate hook fires only through SQLAlchemy's ORM unit-of-work pattern. It will not fire for bulk ORM updates (session.query(...).update(...)) or raw SQL. Looking at consent_encryption_migration.py, updates to this table are executed via raw SQL (UPDATE_QUERY = text(...)), which means updated_at will remain stale after an encryption migration run.
This isn't a regression introduced here — it's a pre-existing limitation — but it's worth being aware of. If accurate updated_at timestamps are important for those code paths too, those updates would need to explicitly set updated_at = now() in the raw SQL, or be routed through the ORM.
| DateTime(timezone=True), | ||
| nullable=True, | ||
| onupdate=func.now(), | ||
| ) |
There was a problem hiding this comment.
Nit / intentional difference to flag: the base class FidesBase.updated_at uses both server_default=func.now() and onupdate=func.now(), which means that column is populated on both INSERT and UPDATE. This model keeps nullable=True with no server_default, so updated_at starts as NULL and only gets set on the first ORM UPDATE. That appears intentional (you can distinguish "never updated" from "updated at time T"), but worth a brief comment in the code to make the design choice explicit for future readers.
adamsachs
left a comment
There was a problem hiding this comment.
generally looks good!
only comment is similar to what claude called out - is the deviation from the base class (no server default, nullable=true) intentional? if not, then we should just rely on the base class definition. if so (which i imagine it is), then i do think a quick comment could be helpful explaining what deviates and how. but that's a nit.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Ticket ENG-3288
Description Of Changes
The
updated_atcolumn on thePrivacyPreferencesmodel was overridden from theBaseclass withoutonupdate=func.now(), so it remained NULL even when records were updated (e.g. whenis_latestis flipped toFalse). This adds theonupdatetrigger soupdated_atis automatically set on any ORM-level UPDATE.No migration needed —
onupdateis a SQLAlchemy ORM-level behavior that addsSET updated_at = now()to UPDATE statements. It does not change the database schema.Code Changes
onupdate=func.now()to theupdated_atcolumn onPrivacyPreferencesfrom sqlalchemy.sql import funcimportSteps to Confirm
POST /api/v3/privacy-preferencesGET /api/v3/privacy-preferenceswithinclude_historical=trueupdated_atsetupdated_atas nullPre-Merge Checklist
CHANGELOG.mdupdated