WIP: Soft deletes

[radmen]

In my recent project, I had to add support for soft deletes in models.
I thought that it could be a good starting point for #257.

I decided to create a PR to start a discussion whether this approach is correct. If yes - I can write missing parts.

For now, there's nothing much - just a trait I wrote in my project.

---

Changed behavior:

• delete() by default sets deleted_at timestamp (trashes model); it accepts object with force option, when set model will be removed
• isTrashed attribute returns always false - it's a hack, should be removed
• add global scope which excludes models with deleted_at property set

Added behavior:

• restore() unsets deleted_at which marks model as non-trashed
• isTrashed property
• wasTrashed - returns TRUE if deleted_at changed to null

Things to consider:

• deleting model triggers delete hooks; maybe there should be trash hook?
  • if we decide to add trash hook wasTrashed property could be removed
• I'm not sure how isDeleted property should behave. I've hardcoded false because it allowed me to work with the models internal state. Otherwise, I should change few things in Lucid internals, can't remember what it was exactly...

TODO:

• add test suite
• add withTrashed query scope
• allow to define deleted column name (static get deletedAtColumn())
• softDeletes() helper method for schema migration

[RomainLanz]

Hey @radmen! 👋

Thanks for helping us build this feature.

One thing you need to consider to be careful about the orWhere() function since it can remove the condition on deleted_at and add trashed model.
This is an issue that exists for a long time in Laravel codebase and we don't want it in Adonis. 😃

[radmen]

@RomainLanz thanks for pointing that. Indeed it can be a problem. Do know any solution for this problem?

I see some possible ones:

1. add scope using setImmediate(); delay adding scope until very end and try to group the rest of queries (deleted_at IS NULL AND (/* here rest of queries */)) - this may not work as intended
2. deeply modify structure of query builder - imo not very universal solution unless it will be made as a generic one :)
3. do not care, as they don't care in Laravel (:shrug:)

Me, personally, would stick to option three for the time being. It's not something which is required for the first iteration (it's just a matter of putting additional note somewhere in the docs).

[thetutlage]

@radmen Just because Laravel doesn't do it, I will not ignore the bug (or tradeoff).

I would like to explore the possibilities and see if it can be managed by default and if not, we should have some strong reasoning behind it.

If you can make it work (without setImmediate), then I can look into the implementation and make it part of the core.

Maybe we need some way to modify the internal representation of the knex where clauses.

You can explore the possibilities and I will jump on it later (on something else right now)

[radmen]

Just because Laravel doesn't do it, I will not ignore the bug (or tradeoff).

All I'm saying is that it shouldn't be necessary implemented in v1.

If you can make it work (with setImmediate), then I can look into the implementation and make it part of the core.

I'm afraid that this could lead to some strange behaviors.

You can explore the possibilities and I will jump on it later (on something else right now)

Will take a look at QueryBuilder. Was thinking about introducing something like whereEnsure() which has the same API as where() but queries pushed that way will be joined in a bit different way than normally (smth like WHERE ( /* whereEnsure queries */) AND (/* other queries */)

[thetutlage]

I'm afraid that this could lead to some strange behaviors.

Ohh typo there. I wanted to say without setImmediate

[radmen]

I've taken a look at knex queryBuilder. Looks like without writing a wrapper for it it won't be possible to add something like whereEnforce().

Basically, such wrapper should:

• catch every call to *Where* methods of query builder (eg, orWhere, whereBetween etc) and add them to stack
• append to separate stack queries from whereEnsure()
• when calling toSQL() apply queries from both stacks in right order.

It can be done, however, it will be another layer of complexity added to the library.

What do you think about it?

[RomainLanz]

Sorry for the delay, I like the idea!

[radmen]

@RomainLanz thanks for info. I'll check whats possible.

Yet, before I start. Is the general shape of the idea ok (see: my first comment)?
Could you help me with "things to consider"? I'm not sure what should be done with them.

[ghost]

equivelent to return affected?

[webdevian]

It converts it to a boolean

[ghost]

nice trick, thanks (y)

[jessicamrbr]

query.whereNull(`${Model.table}.deleted_at`)

[radmen]

@jessicamrbr thanks, good catch!

[radmen]

@RomainLanz it's been a month since my last comment. I didn't receive any response. Would it be possible for you to look at it? Thank you :)

[RomainLanz]

Hey @radmen,

I'm sorry I was completely busy with other stuff and forget to get back to you.
I'll take a look at those later today 👍

[RomainLanz]

deleting model triggers delete hooks; maybe there should be trash hook?
if we decide to add trash hook wasTrashed property could be removed

I like the idea of having a trash hook, beforeTrashing & afterTrashing.

I'm not sure how isDeleted property should behave. I've hardcoded false because it allowed me to work with the models internal state. Otherwise, I should change few things in Lucid internals, can't remember what it was exactly...

I believe we can deprecate isDeleted and use isTrashed instead.

[radmen]

I like the idea of having a trash hook, beforeTrashing & afterTrashing.

that makes sense. When I was implementing soft delete trait hooks did not allow me to do this (they're quite strict and it's not possible to extend them in a project). In this case I can simply update the implementation of Hooks module.

I believe we can deprecate isDeleted and use isTrashed instead.

isDeleted() is used only in one place:

https://github.com/adonisjs/adonis-lucid/blob/1cacc582aac66ca3c98a7a17c1cc8ef6e77e58b4/src/Lucid/Model/proxyHandler.js#L26-L29

If we deprecate it this code will be useless and should be removed. I think it has it's purpose so I'd say that we should keep isDeleted but maybe modify it to look something like this:

get isDeleted() {
  return this.$frozen && !this.isTrashed
}

What do you think about it?

[RomainLanz]

I would like to have the feedback of @thetutlage

[LordZombi]

any progress with softDeletes() ? how can I help you ? thanks

[radmen]

@LordZombi you can look at the questions I left. Maybe you can help me and answer to them? :)

[hadihallak]

@radmen i would suggest that you publish is as a standalone package so that people could use and contribute to it and then later at some point it gets merged into lucid. this way people will have something to work with rather than nothing.

[radmen]

@hadihallak I could do it this way, yet the implementation is a bit hacky. It can blow some things and I would not like to be responsible for that :)

If you want to I can put it as is, without tests, nothing. The risk is on you folks.

If I get the answers to the things I can't figure out I will try to finish the implementation.

[hadihallak]

@radmen You could make it clear in the repo that it's not production ready yet... Lets say a "Developer Preview".

The reason i think we should make it available as is, is so that people could try it, open issues, make pull requests and start some discussion around it and then when it's ready i'm sure Virk wouldn't mind merging it since it's on the Adonis V5 roadmap anyway.

what i'm saying is that having a half baked solution that the community can fix and enhance is better than not having one at all.

[radmen]

@hadihallak I've thought for a while on what you suggested. I think it's a good idea to publish what is and let people test it. Maybe this way we can find out what's important in this functionality and actually reimplement it back to Adonis.

I will prepare this package (currently adding test suite) and publish it in upcoming days.

[hadihallak]

@radmen Awesome! cant wait to try it 👍

[radmen]

@hadihallak @LordZombi I've published package based on this PR. Feel free to use and leave some feedback. Thanks! https://www.npmjs.com/package/@radmen/adonis-lucid-soft-deletes

[RomainLanz]

Hey @radmen!

Have you received any feedback about your package?

[radmen]

@RomainLanz unfortunately not much. Two bug reports which were fixed and that was all.

According to NPM stats package has some downloads, yet that's all I can tell.

[ChristopherDosin]

Soft deleting would be a cool feature, any news about the process to put it to the core? :)

[MZanggl]

To give some feedback I still encountered some cases where it didn't filter out the deleted records

Specifically with

• whereNotExists
• withCount

---

I wonder if something like this could also be prevented

const User = use('App/Models/User')
Table.query().join('users', 'user_id', 'users.id')
      .whereNull('deleted_at')

by doing

const User = use('App/Models/User')
Table.query().join(User)

[radmen]

To give some feedback I still encountered some cases where it didn't filter out the deleted records
Specifically with
whereNotExists
withCount

@MZanggl could you give examples when this was failing? Maybe you could leave separate issues in the package repository?

[enbermudas]

Any news in here? I would love to use this feature!

[radmen]

@enbermudas from my side there's no news regarding this issue.

I would love to use this feature!

You can! :) I've extracted a package with the suggested implementation: https://github.com/radmen/adonis-lucid-soft-deletes

[radmen]

FYI: this MR is outdated. The package I've created received few updates, which are not included here.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.