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

investigate soft delete feature #1241

Closed
clarkbw opened this issue Mar 24, 2015 · 14 comments
Closed

investigate soft delete feature #1241

clarkbw opened this issue Mar 24, 2015 · 14 comments
Assignees
Labels

Comments

@clarkbw
Copy link

clarkbw commented Mar 24, 2015

This is a fork of the issue strongloop/loopback-component-push#57 and the related pull request strongloop/loopback-component-push#72 where a quick patch was created to investigate implementing a soft delete for the strongloop/loopback-component-push module.

@raymondfeng mentioned that this would make sense to generalize and bring up to the loopback level so I'm opening this issue for that.

I'd appreciate any comments from @karlmikko who mentioned here #222 (comment) implementing a soft delete via a boot script.

I'm assuming we want a field, perhaps configurable, which looks something like _is_deleted that defaults to false and is changed to true where we intend to delete. When this soft delete option is turned on for a Model the find and upsert methods likely want to filter for instances where the _is_deleted property is true.

Here's an example model definition with a possible option:

{
  "name": "Customer",
  "description": "A Customer model representing our customers.",
  "base": "User",
  "softDelete" : true
  }
}

And this could also allow implementers to specify the attribute name so they can match this up with data they might already have.

{
  "name": "Customer",
  "description": "A Customer model representing our customers.",
  "base": "User",
  "softDelete" : {
    "attribute": "isDeleted"
  }
  }
}

From a quick search I see a couple options:

  • Use the PersistedModel setup method to wrap the destroyAll functions changing from a destroyAll to updateAll call passing { _is_deleted : true }
  • Add a default scope to the model so our find methods all work as expected.
  • Instead of default scope we could use the PersistedModel setup to wrap the find / upsert methods to filter for the soft delete attribute

Below is just for clarity, we would set the default scope programmatically, but this is what the model definition would look like with a soft delete default scope.

{
  "name": "Customer",
  "description": "A Customer model representing our customers.",
  "base": "User",
  "scope": {
    "where": {
      "_is_deleted": false
    }
  }
}

I also looked at doing this from the connector level but I don't think connectors should be aware of options in the models, they are better kept simple to just implement the required CRUD methods.

@raymondfeng
Copy link
Member

+1 to explore the idea to use softDelete setting.

@seriousben
Copy link
Contributor

We have easily implemented softDelete in our application in:

  • Adding a softDelete method to models
  • Disabling the remote del /:id method
  • Adding a remote del /:id method calling softDelete
  • Ensuring remotely fetched objects never include deleted objects (after hooks on find*)
  • adding a default query criteria on 'access' when isDeleted is not in the query

Things to think about:

  • a generic softDelete should allow setting deleteBy, deletedCause maybe using a hook

@fabien
Copy link
Contributor

fabien commented Mar 28, 2015

@raymondfeng @seriousben this is a snapshot of the code I have been using - it's generalized as a Mixin, and uses the new access operation hook:

https://gist.github.com/fabien/1585f7be8a1c05e5ec21

The code has been updated recently, but the gist does not reflect this yet. If there's enough interest I'll see if I can release it as a proper npm package.

Suggestions for getting rid of the ugly hack are welcome ;)

@seriousben
Copy link
Contributor

Really cool stuff in there, thanks @fabien. I really think having this implemented as a mixin is the way to go and as soon I have some time will give you better feedback by implementing that way as well.

@raymondfeng How awesome would it be to enable mixins from the json model defintion file? And how awesome would it be if those mixins could be provided by third party npm modules a bit like the middlewares can be.

{
  "name": "Customer",
  "description": "A Customer model representing our customers.",
  "mixins": {
    "timestamps": {
      "updatedPropertyName": "updatedAt",
      "createdPropertyName": "createdAt"
    },
    "softDelete": {
      // options example
      "deletedPropertyName": "isDeleted",
      "deletedTimestampPropertyName": "deletedAt",
      "deleteByPropertyName": "deletedBy"
    }
  }
}

@fabien
Copy link
Contributor

fabien commented Mar 28, 2015

@seriousben thanks for the heads-up. AFAIK this is already working (or perhaps this is still pending with this: strongloop/loopback-boot#111 ?) - I'm not really sure, because I'm waiting forever to have this patch merged ... I have been using the patched code since last year though, which enabled me to extract all kinds of 'traits' as mixins.

Confirmed: this functionality has been merged in a long time ago, and it works: https://github.com/strongloop/loopback-datasource-juggler/blob/093a3070522f93015b3af888921a26a713754375/lib/model-builder.js#L484

('mixins' were originally called 'plugins', but this is the relevant commit: loopbackio/loopback-datasource-juggler@1a4e886)

@clarkbw
Copy link
Author

clarkbw commented Mar 29, 2015

enable mixins from the json model defintion file

This is exactly how my example mixin works, see: https://github.com/clarkbw/loopback-ds-timestamp-mixin

@karlmikko
Copy link

I agree with keeping this as a mixin as it will make adding more datasources in the future easier. Would be interesting to consider how an immutable data store like http://www.datomic.com/ with a builtin notion of time and history would be implemented. As timestamps would need to be shimmed if you wanted a similar data structure.

@clarkbw
Copy link
Author

clarkbw commented Apr 15, 2015

@fabien is there anything I can do to help with the code for this?

@fabien
Copy link
Contributor

fabien commented Apr 15, 2015

@clarkbw I'm currently flooded with (client) work, I'm afraid. Apart from giving the pending PR strongloop/loopback-boot#111 a shot, you might be able to create the mixin as an npm module, based on the code I'm using.

I can post the most recent version as a gist - if you don't mind, it will be as-is; it will need some tweaks because of the internal dependencies I'm using. But in this particular case, I don't think I'm using anything special, except for lodash, async and such. Let me know!

@eugenehp
Copy link

@altsang how far have you moved with implementation of this feature?

@altsang
Copy link
Contributor

altsang commented Jun 15, 2015

i'm supportive of this feature but our maintainers are inundated with the work they have supporting other customer related features

@gausie
Copy link
Contributor

gausie commented Nov 19, 2015

I've been working on this as a mixin today. Not finished yet, but take a look!

https://github.com/gausie/loopback-softdelete-mixin

@jdhrivas
Copy link
Contributor

Any updates on this feature?

@clarkbw
Copy link
Author

clarkbw commented Jul 11, 2016

I've been working on this as a mixin today. Not finished yet, but take a look!

https://github.com/gausie/loopback-softdelete-mixin

I'm not longer actively working on this feature, I'd recommend taking a look at the mixin from @gausie

@clarkbw clarkbw closed this as completed Feb 16, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

10 participants