Skip to content
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

Gracefully handle Foreign Key Constraint Errors #4406

Merged
merged 8 commits into from
Nov 22, 2023

Conversation

ThomasLaPiana
Copy link
Contributor

@ThomasLaPiana ThomasLaPiana commented Nov 8, 2023

Closes https://ethyca.atlassian.net/browse/PROD-1108?focusedCommentId=37809

Description Of Changes

When someone deletes a dataset, it throws an error due to foreign key constraints if there is a referenced connection

The solution is to update our delete logic to more intelligently catch and surface errors relate to foreign key constraints instead of blanket labeling everything as the same error.

Code Changes

  • catch and surface IntegrityErrors when deleting objects

Steps to Confirm

  • the initial Steps to Reproduce no longer work in the Jira ticket

Pre-Merge Checklist

  • All CI Pipelines Succeeded
  • Issue Requirements are Met
  • Update CHANGELOG.md

The new error message:

image

@ThomasLaPiana ThomasLaPiana self-assigned this Nov 8, 2023
Copy link

vercel bot commented Nov 8, 2023

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 Nov 22, 2023 5:38pm

Copy link

cypress bot commented Nov 8, 2023

Passing run #5345 ↗︎

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

Details:

Merge 010e615 into 1459323...
Project: fides Commit: d8f4649f47 ℹ️
Status: Passed Duration: 00:32 💡
Started: Nov 22, 2023 5:48 PM Ended: Nov 22, 2023 5:49 PM

Review all test suite changes for PR #4406 ↗︎

Copy link

codecov bot commented Nov 9, 2023

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (ec7c90d) 87.08% compared to head (010e615) 87.10%.
Report is 28 commits behind head on main.

Files Patch % Lines
src/fides/api/db/crud.py 70.00% 1 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4406      +/-   ##
==========================================
+ Coverage   87.08%   87.10%   +0.01%     
==========================================
  Files         329      329              
  Lines       20309    20386      +77     
  Branches     2610     2627      +17     
==========================================
+ Hits        17687    17758      +71     
- Misses       2160     2163       +3     
- Partials      462      465       +3     

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

@ThomasLaPiana ThomasLaPiana marked this pull request as ready for review November 9, 2023 12:11
Copy link
Contributor

@adamsachs adamsachs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice @ThomasLaPiana , this makes sense and looks good to me! i was able to verify with some basic local testing with fides_env (see screenshot below).

only thing is whether we'd want a bit of automated test coverage, if that's not too hard to get in place.

image

src/fides/api/db/crud.py Outdated Show resolved Hide resolved
src/fides/api/db/crud.py Show resolved Hide resolved
@adamsachs adamsachs merged commit 15b79ee into main Nov 22, 2023
41 of 42 checks passed
@adamsachs adamsachs deleted the ThomasLaPiana-notify-foreign-key-constraints branch November 22, 2023 18:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants