Sync up Database and PreApprovalWebhook/PreApprovalWebhookReply Models #4838

pattisdr · 2024-04-27T00:59:57Z

Closes #PROD-2004

Description Of Changes

Autogenerating migrations reveals there are discrepancies between what is defined on the new PreApprovalWebhook and PreApprovalWebhookReply models and the tables in the fides application db. Let's get these in sync. Updating FK's, indices, etc. is trickier once we've started collecting data.

cd src/fides/api/alembic
alembic revision -m "pre_approval_webhook_cleanup" --autogenerate

Code Changes

Add missing preapprovalwebhook.key index in database to match model
Create unique constraint on preapprovalwebhook.name in database to match model
Index the preapprovalwebhookreply.privacy_request_id to match the model
Start indexing preapprovalwebhookreply.webhook_id on both the model and in the database
Fix/Add FK's on preapprovalwebhookreply privacy_request_id and webhook_id with on delete set null

Add further handling if resources get deleted:

Cascade Delete PreApprovalWebhook if the ConnectionConfig is deleted and adjust FK accordingly in migration
Set Webhook Id to Null on PreApprovalWebhook Reply if the Webhook itself is deleted to match what happens if Privacy Request is deleted. Alter same column to make it nullable if it does get deleted.
Update API endpoint that records if privacy request is eligible to be approved automatically to ignore webhook replies if the webhook is deleted

Steps to Confirm

Verify tables look as expected:

fides=# \d preapprovalwebhook;
                        Table "public.preapprovalwebhook"
        Column        |           Type           | Collation | Nullable | Default 
----------------------+--------------------------+-----------+----------+---------
 id                   | character varying(255)   |           | not null | 
 created_at           | timestamp with time zone |           |          | now()
 updated_at           | timestamp with time zone |           |          | now()
 name                 | character varying        |           | not null | 
 key                  | character varying        |           | not null | 
 connection_config_id | character varying        |           | not null | 
Indexes:
    "preapprovalwebhook_pkey" PRIMARY KEY, btree (id)
    "ix_preapprovalwebhook_id" btree (id)
    "ix_preapprovalwebhook_key" UNIQUE, btree (key)
    "preapprovalwebhook_name_key" UNIQUE CONSTRAINT, btree (name)
Foreign-key constraints:
    "preapprovalwebhook_connection_config_id_fkey" FOREIGN KEY (connection_config_id) REFERENCES connectionconfig(id) ON DELETE CASCADE
Referenced by:
    TABLE "preapprovalwebhookreply" CONSTRAINT "preapprovalwebhookreply_webhook_id_fkey" FOREIGN KEY (webhook_id) REFERENCES preapprovalwebhook(id) ON DELETE SET NULL

fides=# \d preapprovalwebhookreply;
                     Table "public.preapprovalwebhookreply"
       Column       |           Type           | Collation | Nullable | Default 
--------------------+--------------------------+-----------+----------+---------
 id                 | character varying(255)   |           | not null | 
 webhook_id         | character varying(255)   |           |          | 
 created_at         | timestamp with time zone |           |          | now()
 updated_at         | timestamp with time zone |           |          | now()
 privacy_request_id | character varying        |           |          | 
 is_eligible        | boolean                  |           | not null | 
Indexes:
    "preapprovalwebhookreply_pkey" PRIMARY KEY, btree (id)
    "ix_preapprovalwebhookreply_id" btree (id)
    "ix_preapprovalwebhookreply_privacy_request_id" btree (privacy_request_id)
    "ix_preapprovalwebhookreply_webhook_id" btree (webhook_id)
    "webhook_id_privacy_request_id" UNIQUE CONSTRAINT, btree (webhook_id, privacy_request_id)
Foreign-key constraints:
    "preapprovalwebhookreply_privacy_request_id_fkey" FOREIGN KEY (privacy_request_id) REFERENCES privacyrequest(id) ON DELETE SET NULL
    "preapprovalwebhookreply_webhook_id_fkey" FOREIGN KEY (webhook_id) REFERENCES preapprovalwebhook(id) ON DELETE SET NULL

Repeat testing steps in PROD-1870- Adds full support for pre-approval webhooks #4822 and PROD-1869 - Support config for pre-response webhooks #4795

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

…alWebhookReply models: - Add missing preapprovalwebhook.key index in database to match model - Create unique constraint on preapprovalwebhook.name in database to match model - Index the preapprovalwebhookreply.privacy_request_id.privacy_request_id to match the model - Fix/Add FK's on preapprovalwebhookreply privacy_request_id and webhook_id Add further handling if resources get deleted: - Cascade Delete PreApprovalWebhook if the ConnectionConfig is deleted - Set Webhook Id to Null on PreApprovalWebhook Reply if the Webhook itself is deleted to match what happens if Privacy Request is deleted. Make that nullable too.

vercel · 2024-04-27T01:00:02Z

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		Apr 27, 2024 1:08am

…in the callback.

cypress · 2024-04-27T01:11:52Z

Passing run #7495 ↗︎

0	4	0	0	0
⚠️ You've recorded test results over your free plan limit. Upgrade your plan to view test results.

Details:

Merge `3e6db52` into `bcebc35`...
Project: fides	Commit: `2e4fa9743b ℹ️`
Status: Passed	Duration: 00:34 💡
Started: Apr 27, 2024 1:20 AM	Ended: Apr 27, 2024 1:20 AM

Review all test suite changes for PR #4838 ↗︎

src/fides/api/alembic/migrations/versions/9e83545ed9b6_pre_approval_webhook_cleanup.py

src/fides/api/api/v1/endpoints/privacy_request_endpoints.py

pattisdr · 2024-04-27T01:15:24Z

src/fides/api/api/v1/endpoints/privacy_request_endpoints.py

-    # Check if all replies are true
-    replies_for_privacy_request = PreApprovalWebhookReply.filter(
-        db=db,
-        conditions=(PreApprovalWebhookReply.privacy_request_id == privacy_request_id),
-    ).all()
+
+    # Check if all replies are true.  Reply ignored if its webhook has since been deleted.
+    replies_for_privacy_request = (
+        db.query(PreApprovalWebhookReply)
+        .filter(
+            PreApprovalWebhookReply.privacy_request_id == privacy_request_id,
+            PreApprovalWebhookReply.webhook_id.isnot(None),
+        )
+        .all()
+    )
+


It seems easy to remove a preapproval webhook with the PUT endpoint that removes webhooks that aren't in the request. So if this does happen, replies for a deleted webhook are ignored.

Agree, and this additional check to ignore deleted webhooks is a good one 💯

src/fides/api/models/pre_approval_webhook.py

pattisdr · 2024-04-27T01:16:35Z

src/fides/api/models/pre_approval_webhook.py

    webhook_id = Column(
-        String, ForeignKey(PreApprovalWebhook.id_field_path), nullable=False
+        String,
+        ForeignKey(PreApprovalWebhook.id_field_path, ondelete="SET NULL"), index=True
    )


New: adding an index on this column (similar to the one on priavcy_request_id because it is commonly queried. Further, I'm setting this field to null in the event the webhook itself is deleted.

thanks, this makes sense. For future reference, are there any disadvantages to setting an index on a column at all?

yes good question, the main disadvantage is that it slows down writes, you definitely don't want to/need to index everything. This seemed like one of the fields that would be queried frequently and this will be a large table.

src/fides/api/models/pre_approval_webhook.py

codecov · 2024-04-27T01:46:11Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.87%. Comparing base (bcebc35) to head (3e6db52).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #4838   +/-   ##
=======================================
  Coverage   86.87%   86.87%           
=======================================
  Files         345      345           
  Lines       20855    20856    +1     
  Branches     2727     2727           
=======================================
+ Hits        18118    18119    +1     
  Misses       2262     2262           
  Partials      475      475

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

eastandwestwind

Dawn, your assumptions about the foreign key delete behavior make sense to me. Thanks for adding tests and additionally updating the eligible endpoint to account for previously deleted webhooks.

I really appreciate you tackling this one! 🌟

src/fides/api/alembic/migrations/versions/9e83545ed9b6_pre_approval_webhook_cleanup.py

eastandwestwind · 2024-04-29T08:54:36Z

src/fides/api/api/v1/endpoints/privacy_request_endpoints.py

-    # Check if all replies are true
-    replies_for_privacy_request = PreApprovalWebhookReply.filter(
-        db=db,
-        conditions=(PreApprovalWebhookReply.privacy_request_id == privacy_request_id),
-    ).all()
+
+    # Check if all replies are true.  Reply ignored if its webhook has since been deleted.
+    replies_for_privacy_request = (
+        db.query(PreApprovalWebhookReply)
+        .filter(
+            PreApprovalWebhookReply.privacy_request_id == privacy_request_id,
+            PreApprovalWebhookReply.webhook_id.isnot(None),
+        )
+        .all()
+    )
+


Agree, and this additional check to ignore deleted webhooks is a good one 💯

eastandwestwind · 2024-04-29T08:56:23Z

src/fides/api/models/pre_approval_webhook.py

    webhook_id = Column(
-        String, ForeignKey(PreApprovalWebhook.id_field_path), nullable=False
+        String,
+        ForeignKey(PreApprovalWebhook.id_field_path, ondelete="SET NULL"), index=True
    )


thanks, this makes sense. For future reference, are there any disadvantages to setting an index on a column at all?

src/fides/api/models/pre_approval_webhook.py

tests/ops/models/test_pre_approval_webhook.py

#4838)

pattisdr added 2 commits April 26, 2024 20:07

Index PreApprovalWebhookReply.webhook_id since it is used frequently …

3a69c42

…in the callback.

Update changelog.

3e6db52

pattisdr commented Apr 27, 2024

View reviewed changes

pattisdr requested a review from eastandwestwind April 27, 2024 01:17

eastandwestwind approved these changes Apr 29, 2024

View reviewed changes

pattisdr merged commit 5b4983d into main Apr 29, 2024
41 of 42 checks passed

pattisdr deleted the PROD-2004-preapproval-webhook-cleanup branch April 29, 2024 14:14

pattisdr added a commit that referenced this pull request Apr 29, 2024

Sync up Database and PreApprovalWebhook/PreApprovalWebhookReply Models (

368f84b

#4838)

cypress bot mentioned this pull request Apr 29, 2024

Release 2.35.0 #4832

Closed

31 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sync up Database and PreApprovalWebhook/PreApprovalWebhookReply Models #4838

Sync up Database and PreApprovalWebhook/PreApprovalWebhookReply Models #4838

pattisdr commented Apr 27, 2024 •

edited

Loading

vercel bot commented Apr 27, 2024 •

edited

Loading

cypress bot commented Apr 27, 2024 •

edited

Loading

pattisdr Apr 27, 2024

eastandwestwind Apr 29, 2024

pattisdr Apr 27, 2024

eastandwestwind Apr 29, 2024

pattisdr Apr 29, 2024 •

edited

Loading

codecov bot commented Apr 27, 2024 •

edited

Loading

eastandwestwind left a comment

eastandwestwind Apr 29, 2024

eastandwestwind Apr 29, 2024

Sync up Database and PreApprovalWebhook/PreApprovalWebhookReply Models #4838

Sync up Database and PreApprovalWebhook/PreApprovalWebhookReply Models #4838

Conversation

pattisdr commented Apr 27, 2024 • edited Loading

Description Of Changes

Code Changes

Steps to Confirm

Pre-Merge Checklist

vercel bot commented Apr 27, 2024 • edited Loading

cypress bot commented Apr 27, 2024 • edited Loading

Passing run #7495 ↗︎

Review all test suite changes for PR #4838 ↗︎

pattisdr Apr 27, 2024

Choose a reason for hiding this comment

eastandwestwind Apr 29, 2024

Choose a reason for hiding this comment

pattisdr Apr 27, 2024

Choose a reason for hiding this comment

eastandwestwind Apr 29, 2024

Choose a reason for hiding this comment

pattisdr Apr 29, 2024 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Apr 27, 2024 • edited Loading

Codecov Report

eastandwestwind left a comment

Choose a reason for hiding this comment

eastandwestwind Apr 29, 2024

Choose a reason for hiding this comment

eastandwestwind Apr 29, 2024

Choose a reason for hiding this comment

pattisdr commented Apr 27, 2024 •

edited

Loading

vercel bot commented Apr 27, 2024 •

edited

Loading

cypress bot commented Apr 27, 2024 •

edited

Loading

pattisdr Apr 29, 2024 •

edited

Loading

codecov bot commented Apr 27, 2024 •

edited

Loading