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

cascade deletes from system -> connectionconfig -> datasetconfig #3771

Conversation

adamsachs
Copy link
Contributor

@adamsachs adamsachs commented Jul 10, 2023

Closes #3769

Description Of Changes

Updates the ORM backlink relationships between System --> ConnectionConfig and ConnectionConfig --> DatasetConfig to cascade deletes. This effectively ensures, via the ORM layer, that our DELETE /system API will also delete these related integration (i.e. connection) artifacts in the DB, as is expected.

Code Changes

  • specify cascade="all, delete", argument for relationships between System --> ConnectionConfig and between ConnectionConfig --> DatasetConfig

Steps to Confirm

  • confirm reported bug is fixed:
    • create a system (e.g. type "Domo")
    • create a bogus integration config on that system
    • run a privacy request, see it error out on the Domo integration step (expected)
    • delete the system
    • try running another privacy request. it shouldn't error out, at least not from the Domo integration.
  • confirm no other regressions related to system/connection configs
    • try a bunch of CRUD operations on systems thru the UI -- some with no integrations, some with "linked" existing standalone integration, some with "new" integrations
    • try creating a "legacy" standalone connection by flipping the feature flag. ensure CRUD operations on this connection still work as expected

Pre-Merge Checklist

@cypress
Copy link

cypress bot commented Jul 10, 2023

Passing run #3122 ↗︎

0 4 0 0 Flakiness 0

Details:

Merge d9740ff into 956745d...
Project: fides Commit: 0b2e94c62a ℹ️
Status: Passed Duration: 00:45 💡
Started: Jul 11, 2023 2:06 PM Ended: Jul 11, 2023 2:07 PM

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

@adamsachs adamsachs force-pushed the asachs/3769-removing-a-failing-system-and-integration-will-still-cause-a-request-to-fail branch 2 times, most recently from 5347540 to 5aea35f Compare July 11, 2023 02:13
@adamsachs
Copy link
Contributor Author

@galvana the test is just getting hung up here on CI, not sure why - when i execute the test manually, locally, it runs fine. i can dig into it more tomorrow AM my time, but i wanted to flag the PR for you in case you've got any time to review before i pick it back up tomorrow.

from what i can tell in manual testing, this approach works well and i haven't noticed any regressions yet. but the update could have a broader impact and could potentially have unintended side effects. let me know your thoughts if you've got a chance!

@adamsachs adamsachs force-pushed the asachs/3769-removing-a-failing-system-and-integration-will-still-cause-a-request-to-fail branch from 5aea35f to cd55a70 Compare July 11, 2023 13:35
@adamsachs adamsachs force-pushed the asachs/3769-removing-a-failing-system-and-integration-will-still-cause-a-request-to-fail branch from af53cb5 to d9740ff Compare July 11, 2023 13:51
@adamsachs
Copy link
Contributor Author

@galvana the test is just getting hung up here on CI, not sure why - when i execute the test manually, locally, it runs fine. i can dig into it more tomorrow AM my time, but i wanted to flag the PR for you in case you've got any time to review before i pick it back up tomorrow.

from what i can tell in manual testing, this approach works well and i haven't noticed any regressions yet. but the update could have a broader impact and could potentially have unintended side effects. let me know your thoughts if you've got a chance!

pinned down the test issue to be due to the async nature of the endpoint being called -- simplest fix was to move the test to be alongside other "ctl" tests hitting the DELETE /system endpoint, that used similar async testing constructs. running the ctl-unit test suite locally worked well (whereas before running the ops-unit suite was showing similar issues to what was seen in CI). so hopefully 🤞 we're good now!

also completed some regression testing on my fides_env test env, mainly re: UI-based CRUD operations on systems and connections. things looked good there.

i think this is ready for review now (assuming CI passes, which it should now!)

@adamsachs adamsachs marked this pull request as ready for review July 11, 2023 13:53
@adamsachs adamsachs self-assigned this Jul 11, 2023
@codecov
Copy link

codecov bot commented Jul 11, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (956745d) 87.12% compared to head (d9740ff) 87.12%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3771   +/-   ##
=======================================
  Coverage   87.12%   87.12%           
=======================================
  Files         311      311           
  Lines       19072    19072           
  Branches     2462     2462           
=======================================
  Hits        16617    16617           
  Misses       2027     2027           
  Partials      428      428           
Impacted Files Coverage Δ
src/fides/api/models/connectionconfig.py 97.00% <ø> (ø)
src/fides/api/models/sql_models.py 97.81% <ø> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Contributor

@TheAndrewJackson TheAndrewJackson left a comment

Choose a reason for hiding this comment

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

💥

…tegration-will-still-cause-a-request-to-fail
@adamsachs adamsachs merged commit 53ce232 into main Jul 11, 2023
@adamsachs adamsachs deleted the asachs/3769-removing-a-failing-system-and-integration-will-still-cause-a-request-to-fail branch July 11, 2023 16:43
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.

removing a failing system and integration will still cause a request to fail
3 participants