add `System.vendor_deleted_date` db model field; switch DB engines to use custom JSON serialization to handle non-standard data types #4818

adamsachs · 2024-04-22T14:41:52Z

Closes PROD-1979

Description Of Changes

Adds in DB model support for a new vendor_deleted_date field on the System model, added in corresponding fideslang model as a potential means of supporting the GVL vendor deleted date field.

As a side effect of this change, a new date field is now needs to be stored in a JSONB field (in the SystemHistory table), we needed to ensure that those fields were being properly encoded (serialized) as JSON to store in the DB. Instead of special-casing this particular field, we decided to update our SQLAlchemy DB engines more broadly to use json (de)serializer functions that leverage our CustomJSONEncoder that handles datetimes and a few other data types that cannot be serialized to JSON via json.dumps().

Corresponding upstream PRs (should be merged first):

add a System.vendor_deleted_date property fideslang#10
https://github.com/ethyca/fides-services/pull/134 (not strictly required to be merged first)

Corresponding downstream PR:

https://github.com/ethyca/fidesplus/pull/1392

Code Changes

update DB model
add column with DB migration
update common DB engines to use CustomJSONEncoder for JSON (de)serialization
update (remove) previous manual uses of CustomJSONEncoder for DB fields to leverage the now generic/automatic handling via the DB engine

Still TODO:

populate the property on non-bulk compass vendor add?

Steps to Confirm

tested full stack in corresponding plus PR: https://github.com/ethyca/fidesplus/pull/1392

Pre-Merge Checklist

All CI Pipelines Succeeded
Documentation:
- documentation complete, PR opened in fidesdocs
- documentation issue created in fidesdocs
Issue Requirements are Met
Relevant Follow-Up Issues Created
Update CHANGELOG.md
For API changes, the Postman collection has been updated

vercel · 2024-04-22T14:41:58Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment

Name	Status	Preview	Comments	Updated (UTC)
fides-plus-nightly	⬜️ Ignored (Inspect)	Visit Preview		May 8, 2024 2:17am

cypress · 2024-05-01T13:27:34Z

Passing run #7629 ↗︎

0	4	0	0	0

Details:

Merge `aaea874` into `0e27b21`...
Project: fides	Commit: `57c7d641c1 ℹ️`
Status: Passed	Duration: 00:35 💡
Started: May 8, 2024 2:29 AM	Ended: May 8, 2024 2:29 AM

Review all test suite changes for PR #4818 ↗︎

This reverts commit 9247189.

…ization edge cases

…level uses

adamsachs · 2024-05-02T13:45:50Z

ok @pattisdr i think the update to use the custom encoder at the DB engine level is looking good, per our discussion yesterday! (tests seem to be passing).

would like to get your eyes/thoughts on this before moving on further though!

pattisdr · 2024-05-02T14:34:19Z

Looking @adamsachs!

pattisdr · 2024-05-02T14:35:51Z

lots of failing DSR 3.0 tests 😬

adamsachs · 2024-05-02T14:51:13Z

lots of failing DSR 3.0 tests 😬

whoops, the tests i ran locally were far too narrow :( i'll take a closer look, sorry for asking prematurely!

codecov · 2024-05-02T19:59:55Z

Codecov Report

Attention: Patch coverage is 98.46154% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 86.79%. Comparing base (1e180ec) to head (eeda81e).

Files	Patch %	Lines
src/fides/api/task/execute_request_tasks.py	50.00%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #4818   +/-   ##
=======================================
  Coverage   86.78%   86.79%           
=======================================
  Files         346      347    +1     
  Lines       20919    20932   +13     
  Branches     2734     2734           
=======================================
+ Hits        18154    18167   +13     
  Misses       2289     2289           
  Partials      476      476

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…FE updates

pattisdr

good work @adamsachs 👍 thanks for going the extra mile and getting this custom json serialization and deserialization working more broadly. I'd add this more recent change to the PR description too. I also forgot we were overriding JSON de/serialization in our JSONTypeOverride. nice tracking this down.

src/fides/api/alembic/migrations/versions/93af4df40cc0_adds_webhook_id_column_to_auditlog.py

requirements.txt

src/fides/api/db/base_class.py

src/fides/api/db/session.py

src/fides/api/models/privacy_request.py

… testign

adamsachs

some self-review on my minor FE updates that i'm very unsure about!

.gitignore

clients/admin-ui/cypress/fixtures/dictionary-entries.json

clients/admin-ui/cypress/fixtures/systems/dictionary-system.json

clients/admin-ui/src/features/system/SystemInformationForm.tsx

clients/admin-ui/src/types/api/README.md

clients/admin-ui/src/types/dictionary-api/index.ts

clients/admin-ui/src/types/dictionary-api/models/DataUseDeclaration.ts

clients/turbo.json

NevilleS

The Admin UI types/code changes are pretty solid. I didn't UAT the UI though so you should just grab a quick screenshot and get some eyes from @Kelsey-Ethyca or otherwise to see, since that System form is already pretty monstrous

NevilleS · 2024-05-03T15:54:48Z

clients/admin-ui/src/features/system/SystemInformationForm.tsx

+                  name="vendor_deleted_date"
+                  id="vendor_deleted_date"
+                  // disable this field for editing:
+                  // deleted date is populated by the GVL and should not be editable by users


Suggested change

// deleted date is populated by the GVL and should not be editable by users

// deleted date is populated by Compass and should not be editable by users

super nitpicky but this isn't GVL specific

hmm, but isn't it GVL specific? that date is coming from the GVL, through compass...

i don't mind making the change here if you think it's more appropriate but my word choice here was somewhat deliberate!

clients/admin-ui/src/features/system/SystemInformationForm.tsx

clients/admin-ui/src/types/api/README.md

NevilleS · 2024-05-03T17:50:55Z

clients/admin-ui/src/types/api/models/BasicSystemResponse.ts

+  /**
+   * The deleted date of the vendor that's associated with this system.
+   */
+  vendor_deleted_date?: string;


side q: the vendor_deleted_date will always be in the response, right? It'll just be null by default?

(this aspect of the autogenerated types bugs me)

ah yeah that's correct. idk the best way to handle that in typescript, let me know if this is something you think is necessary to adjust

clients/admin-ui/src/types/dictionary-api/index.ts

clients/admin-ui/src/types/dictionary-api/models/DataUseDeclaration.ts

clients/turbo.json

adamsachs · 2024-05-03T20:04:13Z

The Admin UI types/code changes are pretty solid. I didn't UAT the UI though so you should just grab a quick screenshot and get some eyes from @Kelsey-Ethyca or otherwise to see, since that System form is already pretty monstrous

for UAT purposes, here are some screenshots of the new field on my local:

when creating a system, field is populated from compass:

when creating a system, field is not populated from compass (vendor doesn't have a deleted_date):

editing a saved system with the field populated (field is not editable):

editing a saved system with the field not populated (field is still not editable):

adamsachs · 2024-05-08T02:42:30Z

merging, discussed with @Kelsey-Ethyca that she will UAT once it's on main and this is a relatively low-risk update in any case 👍

adamsachs force-pushed the asachs/PROD-1979-fides-updates branch from fb0d8b5 to 710063a Compare May 1, 2024 13:00

adamsachs added 7 commits May 1, 2024 14:39

add System.vendor_deleted_date db model field

5d26496

bump downrev

da3d561

add new field to db_dataset.yml

ae989af

attempt to handle datetimes in sqlalchemy json serialization

634f6dd

Revert "attempt to handle datetimes in sqlalchemy json serialization"

7294607

This reverts commit 9247189.

use custom json encoder on SystemHistory.save to better handle serial…

33a4121

…ization edge cases

deserialize back to dict for jsonb storage

ac6474e

adamsachs force-pushed the asachs/PROD-1979-fides-updates branch from 091c69d to ac6474e Compare May 1, 2024 19:16

adamsachs added 4 commits May 1, 2024 15:29

add comment

63e5573

try using custom json encoder at db engine-level, swap out any field-…

e3fcf04

…level uses

cleanup

57f68d5

Merge branch 'main' into asachs/PROD-1979-fides-updates

6c94d09

adamsachs added 4 commits May 2, 2024 13:57

add custom json encoder to our json type override

8c36437

adjust test assertions

d66a26b

adjust assertion

e7fb628

fix syntax error

87217f4

support vendor_deleted_date in single system creation UX; associated …

595b8ce

…FE updates

vercel bot had a problem deploying to Preview May 2, 2024 20:48 Failure

adamsachs added 2 commits May 3, 2024 08:29

fix linter issue, boolean attribute defaults to true

647b6c7

udpate gitignore to clean up local git state

5709432

pattisdr approved these changes May 3, 2024

View reviewed changes

adamsachs changed the title ~~add System.vendor_deleted_date db model field~~ add System.vendor_deleted_date db model field; switch DB engines to use custom JSON serialization to handle non-standard data types May 3, 2024

adamsachs added 5 commits May 3, 2024 09:44

update cypress test fixtures to include vendor_deleted_date field for…

ae3246c

… testign

rename decode methods

c0405e8

update changelog

e2921ba

Merge branch 'main' into asachs/PROD-1979-fides-updates

9b7f5e0

re-add newline eof for gitignore

3049ebd

adamsachs commented May 3, 2024

View reviewed changes

NevilleS approved these changes May 3, 2024

View reviewed changes

add tooltip for vendor deleted date field

87d039f

vercel bot deployed to Preview May 3, 2024 19:25 View deployment

bump fideslang to official 3.0.1

8227808

adamsachs marked this pull request as ready for review May 6, 2024 18:59

adamsachs requested a review from a team as a code owner May 6, 2024 18:59

adamsachs added 2 commits May 7, 2024 22:14

Merge branch 'main' into asachs/PROD-1979-fides-updates

4e8634c

bump downrev

aaea874

adamsachs merged commit ea5351a into main May 8, 2024
46 checks passed

adamsachs deleted the asachs/PROD-1979-fides-updates branch May 8, 2024 02:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add `System.vendor_deleted_date` db model field; switch DB engines to use custom JSON serialization to handle non-standard data types #4818

add `System.vendor_deleted_date` db model field; switch DB engines to use custom JSON serialization to handle non-standard data types #4818

adamsachs commented Apr 22, 2024 •

edited

Loading

vercel bot commented Apr 22, 2024 •

edited

Loading

cypress bot commented May 1, 2024 •

edited

Loading

adamsachs commented May 2, 2024

pattisdr commented May 2, 2024

pattisdr commented May 2, 2024

adamsachs commented May 2, 2024

codecov bot commented May 2, 2024 •

edited

Loading

pattisdr left a comment

adamsachs left a comment

NevilleS left a comment

NevilleS May 3, 2024

adamsachs May 3, 2024

NevilleS May 3, 2024

adamsachs May 3, 2024

adamsachs commented May 3, 2024

adamsachs commented May 8, 2024

	// deleted date is populated by the GVL and should not be editable by users
	// deleted date is populated by Compass and should not be editable by users

add System.vendor_deleted_date db model field; switch DB engines to use custom JSON serialization to handle non-standard data types #4818

add System.vendor_deleted_date db model field; switch DB engines to use custom JSON serialization to handle non-standard data types #4818

Conversation

adamsachs commented Apr 22, 2024 • edited Loading

Description Of Changes

Code Changes

Steps to Confirm

Pre-Merge Checklist

vercel bot commented Apr 22, 2024 • edited Loading

cypress bot commented May 1, 2024 • edited Loading

Passing run #7629 ↗︎

Review all test suite changes for PR #4818 ↗︎

adamsachs commented May 2, 2024

pattisdr commented May 2, 2024

pattisdr commented May 2, 2024

adamsachs commented May 2, 2024

codecov bot commented May 2, 2024 • edited Loading

Codecov Report

pattisdr left a comment

Choose a reason for hiding this comment

adamsachs left a comment

Choose a reason for hiding this comment

NevilleS left a comment

Choose a reason for hiding this comment

NevilleS May 3, 2024

Choose a reason for hiding this comment

adamsachs May 3, 2024

Choose a reason for hiding this comment

NevilleS May 3, 2024

Choose a reason for hiding this comment

adamsachs May 3, 2024

Choose a reason for hiding this comment

adamsachs commented May 3, 2024

adamsachs commented May 8, 2024

add `System.vendor_deleted_date` db model field; switch DB engines to use custom JSON serialization to handle non-standard data types #4818

add `System.vendor_deleted_date` db model field; switch DB engines to use custom JSON serialization to handle non-standard data types #4818

adamsachs commented Apr 22, 2024 •

edited

Loading

vercel bot commented Apr 22, 2024 •

edited

Loading

cypress bot commented May 1, 2024 •

edited

Loading

codecov bot commented May 2, 2024 •

edited

Loading