[REST] DELETE with invalid ID is successful

[virkt25]

Description / Steps to reproduce / Feature proposal

From #1206

• In examples/todo, using the API Explorer make a request to the DELETE endpoint. Use id 2 / something that doesn't exist.
• You get a success message ... but it should've been a Bad Request since the id doesn't exist.

Current Behavior

• Get a success message when deleting a resource that doesn't exist.

Expected Behavior

• Return a non-success message when attempting to delete a resource that doesn't exist

See Reporting Issues for more tips on writing good issues

[marioestradarosa]

Actually, if you call the DELETE operation with no WHERE clause, it will remove ALL records. I think the juggler Delete All is not checking the empty where clause either. For security this should be disable. What we do for now is that we override these delete methods and place our own validations.

[bajtos]

Is this perhaps related to PersistedModel.destroyAll has very dangerous behaviour if the filter is not passed in correctly strongloop/loopback#3094?

[marioestradarosa]

It is not related, The DEL operation on endpoint /mymodel/{id} (deleteById) where ID doesn't exist returns an http 200 status with empty response body instead of 404 probably. If {id} exists it returns 200 status with true in the response body.

[marioestradarosa]

However, on DeleteAll() I believe which is the main problem because users can delete all their records in one operation, we should read the record first, even if that means a double operation on DeleteAll.

if (!where) { throw an exception }

if (find) { //find with where clause included, we can place a count instead (less expensive)
Delete()
}
else { throw an exception }

This way if the user sends an invalid where clause we don't issue the delete operation at all. If users really need to delete All records they should pass where 1=1 (which is always true) in the where clause.

[bajtos]

It is not related,

@marioestradarosa In that case, let's discuss deleteAll elsewhere please, to keep this issue focused on deleting a record by id that does not exist. I think strongloop/loopback#3094 is the best place for such discussion, we have already covered many aspects there.

[bajtos]

Return a non-success message when attempting to delete a resource that doesn't exist

Let's discuss this, I can see good reasons why it may be actually a good thing to return 200 even when the requested id does not exist.

Here are two use cases I have in mind:

• Multiple clients are trying to delete the same record. This creates a race condition - the first HTTP request to arrive is going to delete the record (and return 200), the second HTTP request will become a no-op.

• The client is on a slow or unreliable network connection (think of 3G mobile access from a rural area, or any connection from a subway/underground where only the stations are covered by mobile signal). Shortly after the first delete request is sent, the connection is dropped or perhaps the user don't want to wait anymore and cancels the request. As far as the client (the end user) is concerned, the record was not deleted. However, the server did received the request and deleted the data. Later, the user decides to retry the operation and sends another delete request with the same id. Because the record has been already deleted by the first request (unbeknowst to the user), the second request becomes a no-op.

I am arguing that we should return 200 OK in both cases. The intention was fulfilled - the record-to-be-deleted no longer exists.

Now if we return 404, then clients have to implemented additional error handling to distinguish between 404 (record no longer exists) and other kinds of errors. This is poor user/developer experience to me.

Having wrote all of this, it looks like most of the internet is of a different opinion and thinks that DELETE request with an unknown ID should return 404 indeed.

• https://www.restapitutorial.com/lessons/idempotency.html
• https://stackoverflow.com/questions/4088350/is-rest-delete-really-idempotent
• https://www.quora.com/What-is-the-appropriate-HTTP-response-code-to-a-successful-DELETE-request
• https://stackoverflow.com/questions/6439416/deleting-a-resource-using-http-delete

In that light, I am fine to proceed and change our DELETE operation to return 404 when the id provided by the client does not exist.

[marioestradarosa]

Great!. And yes, the delete operation on a database is idempotent even when the response code should be different (the first one would receive 200 and subsequent ones will receive 404 in a race condition).

For simplicity the client application would receive 200 or 404 and act upon it accordingly to its needs. For example, the user could receive "Record successfully deleted" or "The record was not found" and that should be enough information for him/her.

[virkt25]

I agree that sending 200 OK for non-existing records isn't particularly wrong because that is the correct state on the server ... I'm ok with either or but personally prefer returning a 404 so the client can decide to handle the case however they want vs. not having the option. But not a strong opinion.

[bajtos]

Cross-posting our discussion from elsewhere:

In LoopBack 3.x, we have a model-level setting strictDelete that enables the desired behaviour (returning 404 for requests attempting to delete a model instance that does not exist). This option was introduced by loopbackio/loopback-datasource-juggler#679.

I think at the time we were implementing strictDelete, we did not want to introduce a breaking change, that's why strictDelete is disabled in LB3.x by default. Now that we are working on LB4, we are pretty much free to change the default behavior.

I have already changed LB models to be strict: true by default. We can do the same for strictDelete and enable it by default for all LB4 models (entities). Here is the place: https://github.com/strongloop/loopback-next/blob/ec37f2fe90f9cda7802d6adcd9632ce8d1516c59/packages/repository/src/repositories/legacy-juggler-bridge.ts#L131

[hacksparrow]

Fixed by #1621.