Loopback-datasource-juggler: Operational hook 'after delete' should receive id(s)

[fabien]

One of the main use-cases for this hook - and it's predecessor even deleted - is handling 'cascading deletes' (in NoSQL). However, unlike the soon-to-be-deprecated event, the new operational hook has no exact way to know what has been deleted, except for the where condition, which is not really helpful.

[JonnyBGod]

In truth this hook should really receive a copy of the object that was deleted (instance).

One other common use case is to remove related data from other services like files for instance.
For most of these use cases a simple object id is not enough as it probably relies on other stored information such as a path and/or a related model.

Without this information I am unable to grasp the utility for this hook.

[fabien]

@JonnyBGod I'll try to fix this

[JonnyBGod]

Ok. let me know if you need help.

[bajtos]

Let me explain why we are not passing the list of deleted ids or instances in the "after delete" hook. AFAIK, database drivers don't report back the list of id (nor full row data) in the response of a DELETE command. We could implement this at LoopBack side, but that would cause performance degradation for users that don't need this functionality.

IMO, this issue is not a problem in the "after delete" hook per se, but instead a problem of the current design of "deleteAll".

BTW note that if you delete a single model only, the id of the model can be obtained from the where query - see https://github.com/strongloop/loopback/blob/84c65de38ac24e012e3eb8c3f7fa281d565101da/lib/persisted-model.js#L1126-L1139. I think it makes sense to expose this method as a public Model API.

Here is my proposal how to solve this problem:

• Add an option that will change the behaviour of DELETEs to return the list of deleted ids or instances. I think it makes most sense to read this option from Model settings.
• Modify the implementation of "deleteById" and "deleteAll" to honour this new option and include the list of ids or instances in the "after delete" hook.
• Unless there is a driver that provides this functionality OOTB, we will have to always implement it at LoopBack side. In that case we can include the list of ids or instances in the "before delete" hook too.

Example model configurations:

// car.json
{
  "name": "Car",
  "options": {
    // query the full instance data
    "queryOnDelete": "full"
  }
}

// bar.json
{
  "name": "Bar",
  "options": {
    // query the id column only
    "queryOnDelete": "id"
  }
}

[raymondfeng]

From the view of practicality, relational DBs or MongoDB only tell you how many records are deleted by a DELETE from SQL statement or db.collection.remove(). There is no efficient way to provide the list of ids of deleted records.

[fabien]

@bajtos having those options makes sense - but perhaps we should focus on the lower-level first? Because you can already do this from the combination of before delete with after delete and the state property (TBD).

[bajtos]

@fabien You can already do this from the combination of before delete with after delete and the state property (TBD).

I am curious on what solution you come up with, because the one described in https://github.com/strongloop/loopback-datasource-juggler/issues/481#issuecomment-77237758 is not correct as I explained in https://github.com/strongloop/loopback-datasource-juggler/issues/481#issuecomment-77314954.

BTW I opened a new issue to track the hook state - see https://github.com/strongloop/loopback-datasource-juggler/issues/484

[fabien]

@bajtos I must be missing something, because when I pass along the actual instances (or their ids) using hookState from a before delete to an after delete hook, how would I be executing the where query in the after delete hook? I would only operate on the ids/instances I gathered during before delete.

[bajtos]

@fabien yes, you will operate on the instances gathered during before delete, but it may be a different list from what was actually deleted, depending on the outcome of the race (cross-posting from #481):

Three requests running at the same time:
  Client1 sends R1 "delete({ color: 'red' })
  Client2 sends R2 updateAttributes({ color: 'red' }) for id=2
  Client3 sends R3 updateAttributes({ color: 'black' } ) for id=1

A possible outcome leading to a bug:

1. (R1) "before delete" hook lists and saves the instances
  SELECT * FROM Cars WHERE color = 'red' 
 and receives Car1, Car3

2. (R2) another client updates the data in parallel, the request runs to completion
   UPDATE Cars SET color = "red" WHERE id = 2;

3. (R3) yet another client updates the data in parallel, the request runs to completion
  UPDATE Cars SET color = "black" WHERE id = 1

4. (R1) Finally the "delete" command for the first request is executed by the connector
  DELETE FROM Cars WHERE color = "red"
 and deletes Car2, Car3

 5.(R1)  "after hook" receives Car1 and Car3 in hook state
   but Car2 and Car3 were deleted from the db instead

[bajtos]

So if you want to implement this properly, you need to make the following changes:

1. (R1) "before delete" hook lists and saves the instances
  SELECT * FROM Cars WHERE color = 'red' 
 and receives Car1, Car3
 NEW: and sets ctx.where = { id: { inq: [1, 3] } }

 (...)
4. (R1) Finally the "delete" command for the first request is executed by the connector
  DELETE FROM Cars WHERE id IN (1,3)
 and deletes Car1, Car3

 5.(R1)  "after hook" receives Car1 and Car3 in hook state
   and that's correct because Car1 and Car3 were deleted from the db

[fabien]

The real issue is that the race condition means that during 4. other items will be deleted than you'd expect at 1. - it doesn't mean that the after hook doesn't apply to the actual intended data when you first invoked it on R1, at 1.

In other words, the hook intent and execution is correct - it's the delete operation itself that might be prone to race conditions - that's food for another discussion I think. Sounds pretty serious, actually.

[fabien]

Thinking about this further, it means that there's always a race condition lurking in such a setup, and because of this you'd almost always want to change it like you explained.

[raymondfeng]

The racing condition could be potentially mitigated using transactions with appropriate isolation level.

[fabien]

@raymondfeng yes, this is something to work on later. For now, can we move forward with #485 and/or #486 ? Thanks.

[bajtos]

488 added ctx.instance to both "before delete" and "after delete" hooks, this property is set by methods deleting a single model instance.

Note that ctx.instance is not set by bulk delete, you need to apply the approach in https://github.com/strongloop/loopback-datasource-juggler/issues/474#issuecomment-77338379 if you need that too.

Because bulk delete is not exposed via REST API by default IIRC, the following implementation may work well in many cases:

Model.observe("before delete", function(ctx, next) {
  if (!ctx.instance)
    return next(new Error("Bulk delete is not supported."));

  // perform any pre-delete steps
  next();
});

I am closing this issue as resolved, please open a new issue if you have a use-case that cannot be solved even in the new version.

[FreakTheMighty]

I'm unclear on what the outcome of this ticket was, or perhaps I'm reporting a regression. In the an after delete hook ctx.instance is undefined. According to these docs, it shouldn't be. I'm deleting the instance through the REST api.

[alexthewilde]

@FreakTheMighty the docs are misleading. I ended up querying the model instance manually using the ctx.where.id as explained here.

[superkhau]

@crandmck ^

[crandmck]

@alexthewilde I'd like to fix the docs. Can you tell me what's wrong or misleading in https://docs.strongloop.com/display/public/LB/Operation+hooks#Operationhooks-Accessingtheaffectedinstance?

[alexthewilde]

@crandmck sure. The table indicates that ctx.instance is available in the before delete hook as well as in after delete, yet this doesn't seem to be the case actually. It could be documented instead that it's possible to retrieve the instance using the ctx.where.id parameter.

[crandmck]

Thanks! I updated the table in https://docs.strongloop.com/display/public/LB/Operation+hooks#Operationhooks-Accessingtheaffectedinstance. Please LMK if that's sufficient.

[alemhnan]

It appears that the documentation is still misleading. Here still says this:

However, when deleting a single model instance the hook receives ctx.instance that contains the instance being deleted.

That is in fact not correct, should be something like: However, when deleting a single model instance the hook receives ctx.where that contains the id of the instance being deleted

The table fixed by @crandmck is instead correct.
Could be possible to fix both the after delete and before delete paragraphs?

[crandmck]

Good catch @alemhnan. I made the suggested change in https://docs.strongloop.com/display/LB/Operation+hooks#Operationhooks-beforedelete.

[gdeckert]

"after delete" documentation still incorrectly states that ctx.instance is set, as pointed out by @alexmhan.

https://docs.strongloop.com/display/public/LB/Operation+hooks#Operationhooks-afterdelete

Also @bajtos comment from 3/11/2016 suggests ctx.instance _should_ be defined in this case.

[bajtos]

"after delete" documentation still incorrectly states that ctx.instance is set, as pointed out by @alexmhan.

https://docs.strongloop.com/display/public/LB/Operation+hooks#Operationhooks-afterdelete

Also @bajtos comment from 3/11/2016 suggests ctx.instance _should_ be defined in this case.

@crandmck PTAL :point_up:

[crandmck]

Changed in http://loopback.io/doc/en/lb2/Operation-hooks.html#after-delete and
http://loopback.io/doc/en/lb3/Operation-hooks-version-3.0.html#after-delete