sequelize
instance.destroy(...) does not return any information if rows has been deleted or not
using Sequelize 3.3.2
var user = User.build({ id: 1 }, { isNewRecord: false })
user.destroy()
.then(function(){
console.log(arguments)
})
Code above always prints { '0': [] } no matter if row has been deleted or not. I am not sure if this is intentional, but it would useful to know that... Also returning option could be very useful (postgres) as I've mentioned in #4122
In fact instance.save() also does not provide informations if record has been actually updated. Also I was wondering why Model.destroy (bulk destroy) returns (rows) from promise while Model.update returns ([rows]) (in array) from promise. To sum up:
var User = sequelize.define('User');
var user = User.build({ id: 1 }, { isNewRecord: false })
// bulk update
User.update({}, { where: { id: 1 } })
.then(function(){
console.log(arguments) // { '0': [ 0 ] }
})
// bulk update with returning option
User.update({}, { where: { id: 1, returning: true } })
.then(function(){
console.log(arguments) // { '0': [ 0, [] ] }
})
// bulk destroy
User.destroy({ where: { id: 1 } })
.then(function(){
console.log(arguments) // { '0': 0 } - why not same as in bulkUpdate?
})
// update
user.save()
.then(function(){
console.log(arguments) // { '0': USER_INSTANCE } - returned instance has only "id" and "updateAt" dataValues - there is no any information if any row has been updated
})
// update with returning option
user.save({ returning: true })
.then(function(){
console.log(arguments) // { '0': USER_INSTANCE } - returned instance has "id", "updateAt" and other row values it has been actually updated
})
// destroy
user.destroy()
.then(function(){
console.log(arguments) // { '0': [] } - no informations if rows has been deleted or not...
})
In each case, your .then( function() {} ) declaration doesn't provide a parameter in the signature, but you're referring to arguments as if it were provided in the signature. Your notes indicate that you're getting something though. Am I missing something?
save currently returns the saved instance, changing that signature would be a BC break that only benefits postgres. Not sure how a parameter indicating whether a successfull update happened would look. (parallel updates could also result in a race since they would set a flag on the same object).
destroy does not support returning atm, hence it does not have the same signature as update, there's an issue open for destroy with returning, you can voice your support there.
@mickhansen maybe it would be possible to make instance.save() to return (instance, saved) where saved would be a boolean indicating if row has been updated or not? For example:
instance.save()
.then(function(instance, saved){
// saved = true | false
})
However I am not sure if it is possible here to call callback with two arguments instead of one being an array ([instance, saved])...
@rainabba I don't understand your question - the problem is that instance.save/destroy don't provide any informations if row has been actually updated/deleted or not.
@alekbarszczewski We could, but that would break backwards compat - and only be usefully for postgres, but an annoyance for everyone else - having to use spread instead of then for save()
@janmeier @mickhansen So maybe yet flag in model instance despite race probability? If someone would update instance just once, then it would still be useful.
The use case for me is that for example I want to implement following api:
PUT /users/:id
which I want to update user with specified ID. So I want to:
var user = User.build({ id: id }, { isNewRecord: false })
user.update({ /* update data */ })
.then(function(user){
// now I want to know if user has been updated or not to serve 404 error in case of failure
})
I could achieve this with bulk update but then "beforeBulkUpdate" hook would be called and since I am just updating single row then it is reasonable to get "beforeUpdate" called. I know that I could use individualHooks option but again this is not what I want (I don't want to fetch row before updating it...).
Same applies to instance.destroy().
@alekbarszczewski Admittedly, I'm still getting my footing with promises so maybe I'm missing something, but sticking to basic JS, wouldn't you need more like below to even have an arguments object to look at? It strikes me as something very basic which I'm guessing you wouldn't miss, but I don't see how it works otherwise.
Note the arguments parameter in the function signature which you don't have
user.destroy()
.then(function( arguments ){
console.log(arguments) // { '0': [] } - no informations if rows has been deleted or not...
})
@mickhansen You're saying that (as @alekbarszczewski suggested), for update/destory, returning an array of instances which failed or an array of failure objects having a reference to the instance and a failureReason would break compatibility for something else or was that in reference to another issue?
@rainabba arguments is a javascript reserved keyword and it "contains" array (well not exactly array, let's call it object) of all arguments passed to the function.
@alekbarszczewski Been working with html/css/js since the days of AOL and have been building web apps for more than a decade and I still learn something new every day :) Thanks.
That will be quite helpful in handling optional arguments.
@janmeier And what about new option that could be passed to instance.update({},{ /* here */ }) that would cause to return no instance if row has not been updated, or even throw (reject) with some error?
For example:
user = User.build({ id: 1 }, { isNewRecord: false })
user.update({ name: 'John' }, { checkExistance: true })
.then(function(instance){
// instance = null if row has not been updated
})
user.update({ name: 'John' }, { /* without option */ })
.then(function(instance){
// instance is always returned (current behaviour)
})
user.destroy({ returning: true, checkExistance: true })
.then(function(instance){
// instance = null if row has not been deleted
})
That would work without breaking BC - what do you think?
@alekbarszczewski It's not perfect, but I think that would be the best solution for now.
I'm also thinking we might want to deprecate always returning the instance from update and destroy - because your way, only returning if there was actually a match makes more sense :). What do you think @mickhansen?
We could add a deprecation notice that the instance will only be returned if there was a match from 4.0 onwards? It would then be disabled if passing checkExistence = true.
Not sure about the option name though ;). Also, it might be a bit cumbersome to pass it every time. I'm thinking more it should be a general behaviour you can opt into, which then becomes the default in 4.0.
This is also similar to https://github.com/sequelize/sequelize/issues/272 - perhaps we might even want a flag to throw an error if trying to delete / update something that doesn't exist
@janmeier Maybe the best solution is to leave it as is now and just make it consistent in 4.x (breaking BC). You are right that without breaking BC it will be messy with all those options and return types.
As for throwing error, now I am not sure if it's good idea.
Anyway if afterUpdate/afterDestroy hook will also provide information if record has been updated/destroyed then user will be able to implement such global hook and throw error on his own if he wants to.
For 4.x my proposition is as follows:
instance.update({},{})
.then(function(rows){ }) // rows = number of affected rows, 0 or 1 in this case
instance.update({},{ returning: true })
.then(function(instance){ }) // instance = null if row has not been updated
instance.destroy({},{})
.then(function(rows){ }) // rows = number of affected rows, 0 or 1 in this case
instance.destroy({},{ returning: true })
.then(function(instance){ }) // instance = null if row has not been deleted
Model.update({},{})
.then(function([rows]){ }) // (same as now)
Model.update({},{ returning: true })
.then(function([rows, [instances]]){ }) // (same as now)
Model.destroy({},{})
.then(function([rows]){ }) // make it consistent with Model.update
Model.destroy({},{ returning: true })
.then(function([rows, [instances]]){ }) // make it consistent with Model.update
I'm pretty sure A LOT of code depends on being able to chain stuff that needs instance together with save().
The change would make this impossible:
Model.findOne().call('update', values).call('addAssociation', association);
Yeah I don't think returning affected rows is a good idea - but if we return null when rows == 0 for update that would explicitly show the error - whereas right now, you might call instance.addAssociation(); on an instance that doesn't exists any more
Hmm, the problem is even deeper I guess. When you call ANY instance method that affects databse (instance.save, instance.update, instance.destroy) you NEVER know if that rows still exists in db or not... Instance is just server side description of row that is not actually synced with database - so in @mickhansen example above theoretically even if update failed, it does not mean that row does not exists when it comes to instance.addAssociation().
Just wondering how to make instance.update and instance.destroy "consistent".
So maybe:
instance.update({})
.then(function(instance){}) // automatically return null if not updated
instance.update({}, { returning: true })
.then(function(instance){}) // automatically return null if not updated + if succeeded instance will contain all instance values
instance.destroy()
.then(function(destroyed){}) // destroyed = true or false
instance.destroy({ returning: true })
.then(function(instance){}) // instance = null if row has not been deleted, otherwise it will contain deleted row
instance.destroy has the same issue as instance.update, we need to allow chaining. (Less so for instance.destroy i suppose, but you could need the id afterwards etc).
Well for now instance.destroy() does not return anything, so making it return anything we want won't break BC for sure. But you are right, it will be more consistent if destroy will beahve same as update:
instance.destroy()
.then(function(instance){}) // instance = null if not deleted
instance.destroy({ returning: true })
.then(function(instance){}) // instance = null if row has not been deleted, otherwise it will contain deleted row
@mickhansen Well it always returns ([]) (empty array) - I don't know why always empty array is returned... Theoretically some1 could depend on it but it would be silly :)
In docs it also states that instance.destroy does not return enything (http://docs.sequelizejs.com/en/latest/api/instance/#destroyoptions-promiseundefined)
Then I think we can safely change destroy to return null / instance depending on whether it was deleted or not :)
I've started working on this in PR #4299. No tests yet. So far I've managed to make it work like this:
instance.update({}, options)
// beforeUpdate: (instance, options)
// afterUpdate: (instance, options) - instance is null if not updated, instance contains fetched data if options.returning
.then(function(instance){}) // - instance is null if not updated, instance contains fetched data if options.returning
instance.destroy(options)
// beforeDestroy: (instance, options)
// afterDestroy: (instance, options) - instance is null if not deleted, instance contains fetched (deleted) data if options.returning
.then(function(instance){}) // - instance is null if not deleted, instance contains fetched (deleted) data if options.returning
Model.update({}, options)
// beforeBulkUpdate (options)
// afterBulkUpdate (options)
.then(function([rows, instances]){}) // instances not present if result array if !options.returning
Model.destroy({}, options)
// beforeBulkDestroy (options)
// afterBulkDestroy (options)
.then(function([rows, instances]){}) // instances not present if result array if !options.returning
One thing I can't figure out is how to deal with bulkUpdate and bulkDestroy and hooks. It is possible to pass individualHooks option to both methods and both beforeUpdate/Destroy and afterUpdate/Destroy hooks will be executed for each instance. However, since there is returning option, it would be useful to able to perform bulkUpdate/bulkDestroy WITHOUT individual before hooks, but WITHIN individual after hooks. What do you think guys?
Also I was wondering if it wouldn't be useful to pass results of bulkUpdate and bulkCreate ([rows, instances]) to afterBulkUpdate and afterBulkDelete hooks:
afterBulkUpdate: function([rows, instances], options){ ... }
Wrapping around functionality with services can help fading this issue. Just an idea / temp solution:
deleteOne: ( req, res ) => {
// Temp store for record
let record = null;
let Options = {
where : {
id: parseInt( req.params.id || 0)
},
};
return Model.findOne ( Options )
.then ( r => { record = r; } )
.then ( () => Model.destroy ( Options ) )
.then ( d => {
if ( d ) {
return [ d, [ record ] ];
} else {
return [ 0 ];
}
} );
}
@mickhansen @janmeier
instance.destroy() still returns empty array in postgresql. Any updates on this and why is it closed?
I am using v4.37.5 and Model.destroy() still returns an empty array. However the document says:
Return:
Promise<Integer> The number of destroyed rows
Not sure if I missed anything?
I noticed this problem when investigating another issue, I was already aware of this but didn't know there was an issue for it!
By the way: I noticed that the value returned by destroy depends on the dialect. I would like to consider this more of a docs issue than a bug. Any objections? I think normalizing the behavior across all dialects, although ideal, would require extra queries... Perhaps a compromise can be achieved... I am having some ideas... Stay tuned!
This doesn't warrant opening a separate issue. But I have a question. The docs state destroy returns a Promise. That's fine, but what does the Promise contain? I wish that was added into the docs.
