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

feat: add cascading deletion info #145

Merged
merged 3 commits into from
Dec 29, 2021
Merged

Conversation

wschurman
Copy link
Member

Why

The use case for this is as follows:

  • We have a trigger that we want to execute to prevent deletion of an entity in certain cases but not others.
  • In a normal deletion case, we want to prevent deletion if it is the last remaining entity of its type.
  • In a cascade deletion case, we want to allow deletion even if it is the last remaining entity of its type so that it doesn't become orphaned.

How

This feature lets us solve this by inspecting the value of the cascade to see what case it's being deleted in. If it is being deleted in the normal case (no cascading info or different type of cascade) then we block the deletion in a beforeDelete trigger. If it is being deleted in a cascade deletion case we allow the deletion.

Test Plan

Run tests.

@codecov
Copy link

codecov bot commented Dec 23, 2021

Codecov Report

Merging #145 (02266a7) into master (efa131e) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 02266a7 differs from pull request most recent head 6287ee3. Consider uploading reports for the commit 6287ee3 to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##           master     #145   +/-   ##
=======================================
  Coverage   95.90%   95.91%           
=======================================
  Files          73       74    +1     
  Lines        1857     1860    +3     
  Branches      205      203    -2     
=======================================
+ Hits         1781     1784    +3     
  Misses         75       75           
  Partials        1        1           
Flag Coverage Δ
integration 95.91% <100.00%> (+<0.01%) ⬆️
unittest 95.91% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...s/entity/src/EntityMutationTriggerConfiguration.ts 100.00% <ø> (ø)
packages/entity/src/EntityMutationInfo.ts 100.00% <100.00%> (ø)
packages/entity/src/EntityMutator.ts 98.19% <100.00%> (-0.02%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update efa131e...6287ee3. Read the comment docs.

Copy link
Member

@ide ide left a comment

Choose a reason for hiding this comment

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

Code looks OK.

One possible gotcha that comes to mind is that we often express ON DELETE CASCADE in Postgres and use CASCADE_DELETE_INVALIDATE_CACHE in Entities, the logic being: let Postgres's schema be the source of truth of how to handle foreign-key deletions, and invalidate the entity cache to let the next read operation fill it with the correct value.

CASCADE_DELETE_INVALIDATE_CACHE doesn't actually issue any DELETE queries. It might not delete any entities at all (except for the first one being deleted that started the cascade).

So, if triggers want to behave differently based on whether a row is actually being deleted, the entity's inbound FK edges needs to be annotated with CASCADE_DELETE or the programmer needs to be sure that Postgres has an ON DELETE CASCADE constraint.

(The motivation for this PR sounds like it is to distinguish between cascading and non-cascading operations, which shouldn't be affected by the behavior described above, which is just something else to watch out for.)

packages/entity/src/__tests__/EntityEdges-test.ts Outdated Show resolved Hide resolved
packages/entity/src/__tests__/EntityEdges-test.ts Outdated Show resolved Hide resolved
@wschurman
Copy link
Member Author

Code looks OK.

One possible gotcha that comes to mind is that we often express ON DELETE CASCADE in Postgres and use CASCADE_DELETE_INVALIDATE_CACHE in Entities, the logic being: let Postgres's schema be the source of truth of how to handle foreign-key deletions, and invalidate the entity cache to let the next read operation fill it with the correct value.

CASCADE_DELETE_INVALIDATE_CACHE doesn't actually issue any DELETE queries. It might not delete any entities at all (except for the first one being deleted that started the cascade).

So, if triggers want to behave differently based on whether a row is actually being deleted, the entity's inbound FK edges needs to be annotated with CASCADE_DELETE or the programmer needs to be sure that Postgres has an ON DELETE CASCADE constraint.

(The motivation for this PR sounds like it is to distinguish between cascading and non-cascading operations, which shouldn't be affected by the behavior described above, which is just something else to watch out for.)

Yeah, I think for now we assume that CASCADE_DELETE_INVALIDATE_CACHE will always have a corresponding ON DELETE CASCADE constraint. I think in an ideal world we wouldn't even have CASCADE_DELETE_INVALIDATE_CACHE and this wouldn't be an issue.

As for this PR, we should continue with this assumption and assume that all cascades detected by entity will actually match those done in the database.

Co-authored-by: James Ide <ide@users.noreply.github.com>
@wschurman wschurman merged commit 3727191 into master Dec 29, 2021
@wschurman wschurman deleted the @wschurman/mutation-info branch December 29, 2021 18:39
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