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

[mrkmg]

Needs way more testing before I would have any confidence of this being used.

TODO

• Implement restoreById
• Add more tests
• Test on more than just mariadb

[mrkmg]

For Issue #534

[mrkmg]

Questions:

• Should we also restrict the findById method to not include soft-deleted entities? A developer could still pass in a FindOneOptions with withDeleted: true.
• Do we need to do anything with the QueryBuilder?. I would assume that we do not.
• What kind of tests should I implement?

[pleerock]

Do we have a better alternative names for this decorator? This looks correct but a bit long

[NoNameProvided]

How about a simple DeletedAt?

[pleerock]

sounds better

[mrkmg]

Should we then also change the CreateDate and UpdateDate decorators as well? I tried to conform to the existing decorators.

[pleerock]

ah you are right, its not consistent. For some reason I thought they are called CreatedAt and UpdatedAt haha. No, we can't rename them since we also need a Column postfix as used in all column decorators. @NoNameProvided any other suggestions for a more consistent decorator name?

[pleerock]

Hi and thank you for your contribution. Can you please make a PR against next branch. I want to land it in 0.2.0. Plus there are lot of changes in that branch, you'll need to change your approach.

Implement restoreById

don't think we need such method.

Should we also restrict the findById method to not include soft-deleted entities? A developer could still pass in a FindOneOptions with withDeleted: true.

I think yes.

withDeleted: true.

but we need call it better

Do we need to do anything with the QueryBuilder?. I would assume that we do not.

we do need. All find operations just delegate everything to query builder

What kind of tests should I implement?

test every scenario you imagine :)

[mrkmg]

Do we need to do anything with the QueryBuilder?. I would assume that we do not

we do need. All find operations just delegate everything to query builder

What I mean by that question is should we automatically exclude soft-deleted entities when a "user" (developer) is manually creating a query with the createQueryBuilder method.

Right now, I am only adding in the soft-delete checks on the convenience methods like find, findOne, etc. I think the repercussions of messing with a custom built query outweigh the advantages.

Plus there are lot of changes in that branch, you'll need to change your approach.

Could you clarify? What do you mean change my approach? I tried to touch as few places as possible, while maintaining the same style of the existing code.

[mrkmg]

withDeleted: true.

but we need call it better

Any ideas on the same we could use? Maybe includeDeleted?

[mrkmg]

Refactoring this now for the next branch

[pleerock]

Could you clarify? What do you mean change my approach? I tried to touch as few places as possible, while maintaining the same style of the existing code.

i mean there are lot of changes in next branch and you'll need to refactor your code a bit

What I mean by that question is should we automatically exclude soft-deleted entities when a "user" (developer) is manually creating a query with the createQueryBuilder method.

TBH Im not yet sure if this should be a querybuilder behaviour as well. If we include this in query builder it will look like a magic - user tries to build a custom query and query builder magically adds some conditions... Thats why for example eager relations are implemented only in find methods. From other side, if user needs a bit more complex behaviour like finding with >= or LIKE operator he forced to use query builder and this new functionality would not work for him - it brings of kinda sometimes it working (find) and sometimes not working (qb) feelings. Im not really sure what way we should go.

[pleerock]

also requesting @AlexMesser @daniel-lang thoughts

[mrkmg]

@pleerock The more I think about it, the more I think that it should not magically inject anything into query builder. If this is noted in the doc's, then I think from a project perspective we have nothing to worry about. To me, if typeorm does not automatically add your joins, then it should not automatically add in the soft-delete restrictions.

To deal with this, maybe with could add a "magic" method to query builder which adds the soft-delete restriction.

Entity.createQueryBuilder("e").where(someCondition).withoutDeleted().getOne();

Then, I could also use that method in the backend of typeorm for all the EntityManager functions.

The question that then comes up, is what about deleted related entities. Let's say I have a "Post" and "PostTag" which is soft-deletable. What happens when I use queryBuilder to select a Post and join PostTag. Would the withoutDeleted method propagate to the joined entities.

Thoughts?

[NoNameProvided]

I would argue this. not creating joins is because of performance reasons, while not loading deleted entities is the main reason for using soft delete and I would like to see it in the query builder as well. Ofc we should have a .withDeleted() call to return the deleted values if needed.

[mrkmg]

I am going to create a PR against next, should I close this PR?

[mrkmg]

Closing this, new PR based on next

[pleerock]

@mrkmg I also strive more to this solution. Also we'll need to add method called "loadEagerRelations" or something like that for eager relations.

I would argue this. not creating joins is because of performance reasons, while not loading deleted entities is the main reason for using soft delete and I would like to see it in the query builder as well. Ofc we should have a .withDeleted() call to return the deleted values if needed.

the main problem is that add extra query which may be unexpected for users since they are kinda build their own query from scratch using query builder. Really hard to decide.

[NoNameProvided]

the main problem is that add extra query

Doesn't it append to the current query only if the decorator is present?

[pleerock]

yes, of course

[NoNameProvided]

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.

[mrkmg]

Please continue conversation on the new PR #1192