Provide feedback (result count) for updateAll and destroyAll

[fabien]

This currently targets the Memory connector and fixes #294 - it appears that the MongoDB connector already does the right thing. We'd have to check the other connectors (running test/manipulation.test.js will fail if unsupported).

This is the first step towards more meaningful feedback from the (HTTP) API as well (needs loopback update, for remoting return values).

[fabien]

@raymondfeng shouldn't removeById return an error if nothing was removed? I think it should - the current API doesn't return anything (HTTP 204) - I'd expect HTTP 404 if nothing was deleted. However, there might be security implications to this.

[raymondfeng]

I think we can return the number of records deleted for removeById too.

[fabien]

I don't think a HTTP 200 with { count: 1 }, makes sense in this case, it is a singular resource.

I'd prefer HTTP 404 if the item doesn't exist in the first place, and a 500-type error if it failed to delete. And of course a 403 one if ACL's forbid.

[raymondfeng]

DELETE /api/orders/1 usually produces 204 or 404 depending on if the order 1 is found. I think we can return the count in Node.js callback but translate it to 204 or 404 for REST.

[fabien]

204 or 404 for REST is OK, however, why not return a (pure) boolean instead of 0 or 1 in Node.js?

[raymondfeng]

true or false is fine with me.

[fabien]

@raymondfeng updated - enabling this in the REST API is probably a breaking change though.

[raymondfeng]

We still have to fix connectors to return the count too.

[fabien]

@raymondfeng from what I've seen loopback-connector-mongodb already does. Not sure about the SQL ones, but they (Postgres) receive a result that is passed to the callback.

[raymondfeng]

Most SQL based connectors already set a property to track the number of changed/affected rows. But we need to make the property name consistent. For example,

• MySQL: affectedRows & changedRows, see https://github.com/felixge/node-mysql/#getting-the-number-of-changed-rows
• PostgreSQL: rowCount, see https://github.com/brianc/node-postgres/wiki/Query#result-object

[raymondfeng]

@fabien I assume we need to fix all connectors before we can land this patch. What do you think?

[fabien]

@raymondfeng yes - wish I had the time to work on that right now, though!

[bajtos]

• Memory connector returns updated records count #502 added feedback on the number of records modified by updateAll.
• deleteAll returns number of deleted records #511 added feedback on the number of records deleted by deleteAll.

The only remaining piece in this patch is reporting whether "deleteById" deleted the record or whether the record was already deleted.

Considering that this PR has been stale for more than 4 months, I am closing it.

@fabien please send a new pull request when you get time to implement feedback in "deleteById".

[fabien]

@bajtos @raymondfeng how should we handle the return value for prototype.destroy? see https://github.com/strongloop/loopback-datasource-juggler/blob/master/lib/dao.js#L1763

We should do one of the following:

cb(err, count > 0)

OR:

cb(err ? err : (count === 0 ? new Error('Not deleted ...') : undefined))

[raymondfeng]

Reporting as an error seems to be more appropriate to me.

[fabien]

@raymondfeng want me to create a PR?

[bajtos]

Reporting as an error is IMO a breaking change. When I want to delete a record, then the fact that it was already deleted is usually not important.

[bajtos]

Don't forget to update destroyById while you are changing prototype.delete, so that the API is consistent.

[fabien]

@bajtos so you're not interested in knowing if it succeeded the first time? ...

[fabien]

If it's a breaking change (because the error will propagate to the REST API), I'm find with cb(err, count > 0) to get a boolean result (count as a number doesn't make any sense IMO).

[raymondfeng]

Now propotype.delete takes an options object. Can we do the following?

1. By default, it calls back with cb(null, count > 0);
2. We allow an option property such as reportFailureAsError to report an error.

[fabien]

@raymondfeng what will be the error message?

[raymondfeng]

Something like 'No instance found for id idValue'

[bajtos]

Can you enlighten me and describe a situation when you want the delete operation to fail when the record is already not there?

Hiding this behaviour behind a flag is a reasonable compromise allowing users to choose the flavour they prefer. Although it won't be available via REST API (at least not now), so perhaps a per-model option (setting) or a global loopback option would make more sense?

[fabien]

Well, for the same reasons you'd like to know how many items you deleted doing deleteAll - it confirms that something was there and now isn't anymore. Perhaps someone else deleted it in the meantime, while you were under the impression it was still there when you decided to delete it. It sounds like a contrived example, but in general you'd like to know if the action you performed was effective (even if the end result is the same, it is gone either way).

We can always add such a setting or global option later. If it weren't for @raymondfeng to bring the Error option up, I would have omitted error reporting for this first pass anyway.