[WIP] Initial Implementation of Soft Delete #1192

mrkmg · 2017-11-16T19:53:18Z

Addresses #534
See #1165 for previous conversation.

mrkmg · 2017-11-16T20:03:39Z

Open Topics:

Should QueryBuilder automatically add the exclusion for deleted item? What about using querybuilder to delete a record? Should that soft-delete? If so, how would we then actually delete an entity?
Better name for FindOneOptions.withDeleted.
Potentially a better name for the SoftDeleteDateColumn decorator.

@pleerock @NoNameProvided Since you two have been the most active in this I will defer to your opinions on these topics.

Some other notes:

Once we decide of the QueryBuilder debate, I will refactor entity manager to just let the QueryBuilder to handle the determination, and remove applySoftDeletedExclusion from that.

mrkmg · 2017-11-17T19:40:42Z

Darn it, it was working... Now I merged upstream/next and it's all broken again. Back to grind

mrkmg · 2017-11-17T19:54:03Z

New slew of updates.

I went ahead and assumed we would want to exclude soft-deleted entities in the QueryBuilder. I added the withDeleted() method to the QueryBuilder as well.

Let me know your thoughts.

mrkmg · 2017-11-17T20:35:56Z

Some things that need to be addressed:

Related Entities
- I need to make sure that related entities are excluded when soft-deleted
- Need to makes sure that instead of deleting soft-deletable related entities, that we actually soft-delete
SQL.js
- Does not appear to be soft-deleting entities, just ignores setting the soft-deleted column

mrkmg · 2017-11-20T20:24:44Z

Just merged next. Still working on this.

pleerock · 2017-11-21T15:49:48Z

Then I don't understand why it's a hard decision to make it the default? It won't affect anyone until they start to use the soft delete.

@NoNameProvided okay then what about eager relations? Shall eager relations always be loaded by query builder?

NoNameProvided · 2017-11-21T17:40:27Z

okay then what about eager relations? Shall eager relations always be loaded by query builder?

I think not

pleerock · 2017-11-21T17:48:35Z

but why not? If user marked eager: true on relations, shouldn't logic be same as in deleted rows case?

NoNameProvided · 2017-11-21T21:07:58Z

Because there won't be a way to get single entity, or you want to add a withoutEagerRelations() flag? Also, loading multiple entities by default can cause perf bottlenecks.

mrkmg · 2017-11-22T03:45:15Z

I think the contention here is consistency vs utility.

From a utility standpoint, most developers will assume that a deleted entity will not show up in anything unless you tell explicitly ask for deleted entities. This will be the use case "most" of the time, especially considering there is not way to do anything except the simplest of conditions aside from QueryBuilder.

From a consistency standpoint, if relations are not going to be magically added in order to prevent the the QueryBuilder altering the developers SQL query, then it should not alter the SQL to put in the soft-delete conditions. To be honest, if I manually build a query, I want it execute as I wrote it.

I think this brings up a better topic, and maybe something for a 0.3 consideration. Since the 0.1 version, we are now forced to use QueryBuilder more often. Given I have to use it to get entities, I want it to load in my eager relations be default. But I also occasionally have a very complex query that I want to optimize and to have more "raw" results without all the magic involved. There is currently no middle ground to this. You have to use the same QueryBuilder, and explicitly state each relation you want.

I have two ideas on this subject.

Provide a flag to either load or not load eager relations. (like include or not include deleted entities). This would allow you to have the best of both worlds. You would just have to decide whether QueryBuilder is magic by default, or "raw" by default, then add in flags for the opposite.
Split QueryBuilder into two different classes. One for a more "raw" SQL, which does not return Entities, but just "Raw Results", and one that does more magic for you and returns actual entities. We could keep QueryBuilder which returns the entities, and then add in a RawQueryBuilder which only returns "raw" results. The QueryBuilder could would perform the magic items like relations and soft-deletes, while the RawQueryBuilder would perform the exact query you built.

Thoughts?

NoNameProvided · 2017-11-22T13:11:13Z

To be honest, if I manually build a query, I want it execute as I wrote it.

Right now you must have to use the query builder for almost everything that is more complex than a where condition. We also tend to use it over the find, etc methods because it's consistent. And now we have a custom soft delete implementation and we need to add a where condition for every query we build. It's repetitive and not good. What is the point of building soft delete if you still have to add a custom condition to it. The point is here that TypeORM takes care of that and you doesn't have to.

So I would say rather include eager relations then. Yet I still think the two is not the same, loading more data is a performance problem, why not loading the deleted entries is not and excepted by default is you enabled soft delete.

mrkmg · 2017-11-22T16:34:31Z

This is a tough point. Here is what I see it coming down to:

Maintain consistency in the API.
Provide usefulness.

To maintain consistency, QueryBuilder should not alter the query at all. To provide the most usefulness of the SoftDelete functionality, it should prevent deleted entities from being returned.

Again, I am going to go back to the fact that you can not perform anything other than super simple conditions unless you use QueryBuilder. Looking at #1101, it appears that at least some queries could be refactored into using the EntityManager's find function, putting more into the API consistency case.

Overall though, I still do feel that if we are going to return an entity, it should contain all the magic. As a developer, I want my eager relations to load, my deleted entities to be excluded, etc. If I have a query that is going to be perfomance restricted, I still want the ability to say "run this query as is". I wouldn't expect that to return entities though, I would expect a more "raw" result.

I am going to go back to my point above of there is no middle ground, and maybe a separation of responsibility is in order. If QueryBuilder is not going to perform any magic, then it should not return entities at all. If it is going to return entities, it should pull the relations and exclude the soft-deleted entities. Basically, make QueryBuilder either always work on entities, or never work on entities.

I know this is a paradigm shift, but as a developer using TypeORM, I want consistency over everything else. If something returns an entity|entities, it should have my relations, etc. If it does not return an entity, it should return exactly what I asked it for. Having this difference makes compartmentalizing my code way more difficult because I have to know if my Entity came from a find method, or a QueryBuilder method. Currently, if I have a "processor" function, which loops through relations of an entity I have to first check if those are present. I would have expected that because they are "eager", that they would have been present. When I first upgraded to 0.1 I had to add in many more conditionals in my code.

Sorry for the wall of text here, I just feel this is a bigger topic than the simple question that is immediately in front of us.

pleerock · 2017-12-01T10:18:09Z

Having all those issues now Im not really sure if we need this functionality... Maybe its better to left this implementation on users, so they'll implement it on their own. At the end not everybody need this functionality.

NoNameProvided · 2017-12-01T10:27:36Z

I don't agree, soft delete is a pretty basic concept, and should exist at the ORM level because it's hard to do it on the user level. You need to create a wrapper for every function in the ORM or adding it to every call manually. Neither of these is a scalable solution.

mrkmg · 2017-12-01T13:23:20Z

I agree with @NoNameProvided, most ORM and ActiveRecord Implementations I have used implement some sort of soft-delete functionality. I have a user-space implementation in an app I am working on, and it's a very much not a good solution.

I think we can come to some sort of agreement here.

pleerock · 2017-12-13T13:30:11Z

src/entity-manager/EntityManager.ts

-                .execute();
-        }
+    } else {
+     return this.createQueryBuilder()


something wrong here

will be fixed in next update

pleerock · 2017-12-13T13:32:40Z

src/persistence/EntityPersistExecutor.ts

                    if (entityTarget === Object)
                        throw new CannotDetermineEntityError(this.mode);

+                    if (this.mode === "restore" && metadata.softDeletedDateColumn) {
+                        entity[metadata.softDeletedDateColumn.propertyName] = null;


Please use metadata.softDeletedDateColumn.setEntityValue(entity, null) instead. This is important if your column is in embedded.

will be fixed in next update

pleerock · 2017-12-13T13:32:46Z

src/persistence/EntityPersistExecutor.ts

+
+                    if (this.mode === "remove" && metadata.softDeletedDateColumn) {
+                        this.mode = "save";
+                        entity[metadata.softDeletedDateColumn.propertyName] = new Date();


Please use metadata.softDeletedDateColumn.setEntityValue(entity, new Date()) instead. This is important if your column is in embedded.

will be fixed in next update

pleerock · 2017-12-13T13:38:53Z

src/persistence/SubjectExecutor.ts

@@ -241,7 +240,8 @@ export class SubjectExecutor {
                const updateQueryBuilder = this.queryRunner
                    .manager
                    .createQueryBuilder()
-                    .update(subject.metadata.target)
+                    .withDeleted(true)
+                .update(subject.metadata.target)


bad formatting

fixed in next update

pleerock · 2017-12-13T13:39:06Z

src/query-builder/QueryBuilder.ts

@@ -457,6 +457,14 @@ export abstract class QueryBuilder<Entity> {
        return this;
    }

+    /**
+     * Set with deleted mode


Please provide a better comment

fixed in next update

pleerock · 2017-12-13T13:41:00Z

src/decorator/columns/SoftDeleteDateColumn.ts

+ * This column will store an soft delete date of the deleted object.
+ * This date is set when you delete the object, and set to null when you restore it
+ */
+export function SoftDeleteDateColumn(options?: ColumnOptions): Function {


What if people want to use a flag instead of date? Something like isRemoved: boolean. I think we need to support that as well.

If we'll do so we can simply call it SoftDeleteColumn

I have always seen it as a date. I suppose we could support both options, but I think that complicates things.

TBH, I would not use it if it was only a flag. In every single project I have, I need to know when something was deleted.

I had several projects where we used just a flag. And when things are getting really complex and we need a deletion date we store in a separate table where all activity on the given object is stored.

Would be great if you can implement this functionality. If we'll do do it later, but we definitely need to rename decorator right now.

I would argue that having it as a date has a more general scope of usage than it being only a flag:

it gives information on when the entry was deleted (useful in debugging)

a nice side effect is that you can expire an entry by setting delete time in the future

Unfortunately I don't have knowledge if having a boolean value over date is better performance wise so I can't argue on that point.

As a last point, library (in general terms) should provide feature that covers as much use cases as possible for the feature in question.

pleerock · 2017-12-13T13:42:58Z

src/entity-manager/EntityManager.ts

+    }
+
+    /**
+     * Restores a given entity.


Provide a better comment, mention that it restores entities with soft delete column

Also what happens when user sends entity without soft delete column here?

pleerock · 2017-12-13T14:02:07Z

Also please update a changelog and add documentation regarding to this feature.

mrkmg · 2017-12-14T17:46:00Z

So, I basically re-implemented from next again, because it was easier than rebasing.

Also added some docs, and made the changes from the comments you put in.

pabl-o-ce · 2018-02-01T03:53:03Z

@mrkmg I want to help you... maybe can we make a list of thing we would need to implement for start working on it?

this will help so other people can contribute in your work.. and give some type of status about this feature

b3ross · 2018-02-12T18:43:31Z

Just checking in on the status of this issue - definitely would love to see this in place.

br0wn · 2018-02-13T18:31:46Z

A suggestion: when in doubt see how others implemented it. I personally like the way how Doctrine Extensions implemented it. From it's docs:

Works with DQL DELETE queries (using a Query Hint).
All SELECT queries will be filtered, not matter from where they are executed (Repositories, DQL SELECT queries, etc).
Can be nested with other behaviors
Support for 'timeAware' option: When creating an entity set a date of deletion in the future and never worry about cleaning up at expiration time.

mrgusev · 2018-02-22T10:53:48Z

@pleerock @mrkmg are there any unresolved design concerns left? Or any help or feedback required from other contributors?

mrkmg · 2018-02-22T13:40:24Z

There was a disagreement on whether the column that denotes a record was deleted should be a boolean or a date, or allow for both.

I am totally willing to re-factor my this branch against HEAD again, but I do not want to spend all that time for it to sit here with no progress.

In my projects, I am currently using a wrapper to add this functionality into all my Entities.

b3ross · 2018-02-23T03:35:15Z

@mrkmg @pleerock just throwing it out there - why can't we add a boolean delete flag later as a follow-on PR? Soft-delete support would be super useful to get in place, and it seems to me (an outsider here though), that we can easily add boolean functionality later on.

A date-based delete flag is better than no delete flag.

Since we don't have scope support (see #1601 - I'm still a little surprised that was closed) there's no way to easily add support soft-delete functionality other than explicitly modifying each query (eww).

mrgusev · 2018-02-23T09:54:32Z

@mrkmg could you please share a link to that wrapper?
Regarding the field type, I think most of people here state that it should be date by default. And boolean is rather optional. Also for better consistency I think it would be better to have it same as updated and created column types. I have never seen soft delete column as boolean in my practice too.

pleerock · 2018-02-26T06:00:52Z

Im going to left it on 0.3.0 milestone since currently we are quite busy with 0.2.0 release and want to finish all tasks of 0.2.0 scope. I'll return back to this conversation and start thinking what is the best way to implement this feature once 0.2.0 is released. (0.3.0 contains multiple issues about query builder refactoring). Maybe we'll use filters as @b3ross suggests, not yet sure, will see.

b3ross · 2018-02-27T03:56:07Z

Thanks @pleerock

Destreyf · 2018-04-06T22:31:07Z

I just wanted to give my feedback on this, i am in need of this functionality and am currently working on my own approach to it just extending The repository and modifying all calls, however when it comes to the query builder, i think it could come from a chained approach, i'd imagine adding a withDeleted and/or a withoutDeleted method to the chain, allowing the commands following after that to know if they should exclude/include deleted items.

I also think it could be configured in a way that you could set a default in the ORM config of "include deleted by default" or some other type of flag as a default control behavior.

Destreyf · 2018-04-06T22:43:36Z

As a follow up for some more clarification on why you'd include deleted by default, maybe the part of the UI you're designing has the ability to show "deleted" records, and other parts would want to exclude them, this makes it so the client could use a "Soft Delete" column, while also having logic that could omit them with a withoutDeleted call.

consense · 2018-04-06T23:12:43Z

Deleted means we'll deleted.
For what you describe I'd say something like 'hidden' or similar would be more appropriate which is managed from the application layer and not from typeorm

Destreyf · 2018-04-08T06:16:32Z

@consense i disagree on the hidden field, i also disagree with the idea of moving this to the application layer exclusively, a soft delete is characterized as:

https://www.pcmag.com/encyclopedia/term/57355/soft-delete

To mark a record in a database for deletion or to temporarily prevent it from being selected. In order to actually delete the record, a "hard" delete or "permanent" delete function must be performed.

https://www.codeproject.com/Articles/46889/Soft-Delete-or-Logical-Delete-in-ORM

"Soft Delete" is a way of removing business entities, which implies that we mark an entity as deleted instead of physically removing it from the database. This approach is often used in Line of Business applications because of its several advantages:

It allows us to keep history for different auditing scenarios. For example, somebody removed one document in the past from our workflow system, we surely want to be able to audit removing log and data removed document contained.

It allows to implement Recycle Bin approach in an easy way. We'd like to be able to recycle any document removed in the past.

The whole point of using soft deletes is not to "Delete" data permanently (hard delete), its to "soft" delete the data, making it possible to restore the data until the application/developer decides to "purge" the data (hard delete) based on their own business use case. To give you an example, i work with data that has a 7 year retention policy, but records DO need to be "deleted" however this data has to be recoverable for up to 7 years.

The ORM layer could very easily handle this, and as long as the application/developer is aware they need to indicate with/without deleted records when using the query builder.

Some additional references when it comes to how other ORM layers are handling this (integrated & some extensions):
Laravel Eloquent - https://laravel.com/docs/5.6/eloquent#soft-deleting
Rails ActiveRecord - https://github.com/jhawthorn/discard
Sequelize http://docs.sequelizejs.com/manual/tutorial/models-definition.html#configuration
Bookshelf Plugin: https://github.com/lanetix/node-bookshelf-soft-delete

I would like to note that there's a few options for Rails, some of them implement soft delete by overriding default behavior (Paranoid is one of these) and then there's Discard, which adds additional separate methods.

My previous message was not specifically in pushing this as a direct feature of typeorm, but there was some questions on how the query builder could/should handle this: #1192 (comment) and i think that having a config option that sets the default inclusion state makes the most sense, and then having a withDeleted(true/false) or a withDeleted/withoutDeleted method for the chain would be the cleanest way to handle it, leaving the default state of queries for the developer to choose.

anodynos · 2018-05-01T17:30:03Z

Hi Umed @pleerock and thanks fot this framework :-)
Any progress on the Soft Delete feature?

barcoo-hb · 2018-05-17T20:53:25Z

@Destreyf
please excuse my massive delay in responding - must have missed the notification of your comment ;-(

My comment from April was not intended to question the usefulness of having a soft-delete functionality in the ORM layer in general - quite the contrary as I am looking forward to this feature as well and the sources you quote also mirror my understanding.

Still the semantics of "delete" in the English language indicate a permanent action. I would put up the hypothesis that having a get() following a delete() return the "deleted" record would confuse most users initially using the ORM and would be perceived as a bug.

Your use case involving retention policies is understandable, but solving this via an ORM soft-delete feature seems to me a bit like "archiving" documents which arent used on a daily basis anymore (but need to be retrievable for years ahead) in the trash bin. Sure - they could be recovered, but maybe adding an archive room in the basement (which would be an application level managed "archive" flag on the model from a technical POV) would be more appropriate. The trash bin (or soft-delete flag - be it a bool or deleted_at timestamp) would serve more a purpose of giving the application a safety net in case of an "ah-shit-shoudlnt-have-deleted-this-please-undo" moment.

Your secondary suggestion to make the default behavior configurable is of course possible, but if you have an application where "delete" doesnt mean "delete" but "archive/hide" in 90% of the cases and you set "with-delete" as default for get/fetch calls, you would effectively rob yourself off the safety-net a soft-delete feature usually provides as you couldnt undo accidentally deleting an "archived" record anymore.

Anyway - no offense intended - just wanted to clarify my earlier thoughts :-)

dmitrirussu · 2018-08-28T12:25:52Z

Hi @pleerock, @AlexMesser, I would like to know if you still think to merge that pull request to 0.3.0 release and when it should appear? You told that after release 0.2.0 you will come back to that issue.... now we have release 0.2.6 still no activity here and any reaction for that pull request.... The community of TypeOrm would like to have this feature....

mrkmg · 2018-08-28T15:25:37Z

I am still willing to re-implement this for the next release, but I want to make sure that @pleerock agrees with the API before I do the work. I have implemented it twice now, and both times it stalled.

Once there is a defined API of how it will work, I will do the work to implement it.

I will leave the API up to the community to decide on. You can see my thoughts above.

vkosachev · 2018-08-29T07:38:33Z

Hey @dmitrirussu @pleerock @AlexMesser I would like to contribute to this feature as well.
Can we open up the discussion again and see what blockers we currently have? Thanks

oeddyo · 2018-09-15T05:30:21Z

would love to see this coming

RDeluxe · 2019-01-21T18:48:25Z

Hello there,

Is this still on the table ? Seems pretty well advanced to me

pleerock · 2019-01-22T07:13:57Z

In the future versions of typeorm I plan to refactor current QueryBuilder implementation where I'll reconsider this functionality. So, no plans to make it in typeorm for now.

kvandake · 2019-07-22T07:39:02Z

Any progress on the feature?

jpollard-cs · 2019-08-21T19:01:50Z

welp looks like we have an answer on this: #534 😭

goldcaddy77 · 2019-08-21T19:08:14Z

Bummer... I'm using soft deletes in Warthog, which is built on TypeORM exclusively and have been hoping to see them land in this PR. Anybody know of any TypeORM magic I could expose to my BaseService here to get cascading soft-deletes?

pleerock · 2019-12-21T08:02:36Z

Let's continue discussion in #5034 and try to merge that PR.

mrkmg mentioned this pull request Nov 21, 2017

[WIP] Initial implementation of soft-deletes for Issue #534 #1165

Closed

3 tasks

pleerock reviewed Dec 13, 2017

View reviewed changes

Implemented Soft Deletes against next

a436b5a

mrkmg force-pushed the next-implement-softdelete branch from a6ce4fa to a436b5a Compare December 14, 2017 16:47

mrkmg added 2 commits December 14, 2017 12:28

Worked on Documentation

c33fd59

Added restore to base entity.

b3b8984

Added error for attempting to restore on a non soft deletable entity

99f3d41

pleerock added this to the 0.3.0 milestone Mar 27, 2018

michaelbromley mentioned this pull request Jan 22, 2019

"Delete" mutations vendure-ecommerce/vendure#21

Closed

jonathanrdelgado mentioned this pull request May 30, 2019

Feature: Soft Delete #534

Closed

iWinston mentioned this pull request Oct 22, 2019

Amazing iWinston/typeorm-plus#4

Closed

pleerock closed this Dec 21, 2019

[WIP] Initial Implementation of Soft Delete #1192

[WIP] Initial Implementation of Soft Delete #1192

Conversation

mrkmg commented Nov 16, 2017

mrkmg commented Nov 16, 2017 • edited

mrkmg commented Nov 17, 2017

mrkmg commented Nov 17, 2017

mrkmg commented Nov 17, 2017

mrkmg commented Nov 20, 2017

pleerock commented Nov 21, 2017

NoNameProvided commented Nov 21, 2017

pleerock commented Nov 21, 2017

NoNameProvided commented Nov 21, 2017 • edited

mrkmg commented Nov 22, 2017 • edited

NoNameProvided commented Nov 22, 2017

mrkmg commented Nov 22, 2017 • edited

pleerock commented Dec 1, 2017

NoNameProvided commented Dec 1, 2017

mrkmg commented Dec 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pleerock Dec 13, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pleerock Dec 13, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pleerock Dec 13, 2017 • edited

Choose a reason for hiding this comment

pleerock Dec 13, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pleerock commented Dec 13, 2017

mrkmg commented Dec 14, 2017

pabl-o-ce commented Feb 1, 2018

b3ross commented Feb 12, 2018

br0wn commented Feb 13, 2018

mrgusev commented Feb 22, 2018

mrkmg commented Feb 22, 2018

b3ross commented Feb 23, 2018

mrgusev commented Feb 23, 2018

pleerock commented Feb 26, 2018

b3ross commented Feb 27, 2018

Destreyf commented Apr 6, 2018

Destreyf commented Apr 6, 2018

consense commented Apr 6, 2018

Destreyf commented Apr 8, 2018

anodynos commented May 1, 2018

barcoo-hb commented May 17, 2018

dmitrirussu commented Aug 28, 2018

mrkmg commented Aug 28, 2018

vkosachev commented Aug 29, 2018

oeddyo commented Sep 15, 2018

RDeluxe commented Jan 21, 2019

pleerock commented Jan 22, 2019

kvandake commented Jul 22, 2019

jpollard-cs commented Aug 21, 2019

goldcaddy77 commented Aug 21, 2019 • edited

pleerock commented Dec 21, 2019

mrkmg commented Nov 16, 2017 •

edited

NoNameProvided commented Nov 21, 2017 •

edited

mrkmg commented Nov 22, 2017 •

edited

mrkmg commented Nov 22, 2017 •

edited

pleerock Dec 13, 2017 •

edited

pleerock Dec 13, 2017 •

edited

pleerock Dec 13, 2017 •

edited

pleerock Dec 13, 2017 •

edited

goldcaddy77 commented Aug 21, 2019 •

edited