Soft delete recursive cascade #8436
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 10 commits
December 6, 2021 11:58
|
I'm really against cascades further complication, but let's merge it for now, later on we need either simplify the codebase, or find a maintainer who is going to deal with constant cascade issues. |
pleerock
added a commit
that referenced
this pull request
Feb 17, 2022
|
I had to revert these changes because of failing CI. I missed few points in this PR, I'll left comments. |
pleerock
reviewed
Feb 17, 2022
| .softDelete() | ||
| .from(relation.inverseEntityMetadata.target) | ||
| // We get back list of the affected rows primary keys for call again | ||
| .returning([primaryPropertyName]) |
There was a problem hiding this comment.
.returning isn't supported by all databases, for example sqlite.
pleerock
reviewed
Feb 17, 2022
| // Move throw all the relation of the subject | ||
| for (const relation of subject.metadata.relations) { | ||
| // Call recursive function that get the parents primary keys that in used on the inverse side in all one to many relations | ||
| await this.executeSoftRemoveRecursive(relation, [Reflect.get(subject.identifier, subject.metadata.primaryColumns[0].propertyName)]); |
There was a problem hiding this comment.
not really clear why Reflect.get is used here. Decorators isn't the only source of entity definitions and some people define entities using entity schemas.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The soft-delete would update recursively the related entities without loading them
related to - #8416
Description of change
The Problem
Today, the cascade soft delete update the related entities just if we would load all the entities that we want to soft-remove
This behavior is bad in term of performance and in addition, don't work recursively
Because there is no feature like that today, a lot of developers write soft delete flow for every entity that includes a lot of code because they need to kill the children as well, and that causes a lot of bugs
The Solution
Change the soft-remove flow to soft-remove all related entities recursively and automatically
I prefer to soft remove without loading all the entities and create subject to each of them because it's more efficient
Other option
Add a flag to the soft-remove function that declares that the operation should do the new logic instead of the old one
Pull-Request Checklist
masterbranchnpm run lintpasses with this changenpm run testpasses with this changeFixes #0000