trailsjs / trailpack-sequelize

:package: Sequelize.js Trailpack http://sequelizejs.com
MIT License
5 stars 9 forks source link

Support options.returning on Update #32

Closed JAertgeerts closed 7 years ago

JAertgeerts commented 7 years ago

http://docs.sequelizejs.com/en/latest/api/model/#updatevalues-options-promisearrayaffectedcount-affectedrows

Is it possible to allow the Postgres specific option for Sequelize:

Update multiple instances that match the where options. The promise returns an array with one or two elements. The first element is always the number of affected rows, while the second element is the actual affected rows (only supported in postgres with options.returning true.)

jaumard commented 7 years ago

Yeah but only for Postgres... if you want to make a PR for this please do, there no compatibility problem, I think if it's true but not on postgres it's just ignored right ? I didn't do it because I wanted to keep same for every database to be consistent, but for postgres user it can be very useful to have the object in result

JAertgeerts commented 7 years ago

It'll just ignore, correct. Sequelize checks every dialect and only postgres has the option embedded. I'll open a PR.

JAertgeerts commented 7 years ago

A solution could be to change the return object as follows:

query = Model.update(values, _.extend(criteria, options)).then(result => {
        if(_.isBoolean(options.returning) && options.returning == true) {
          return result[1][0]
        }
        else {
          return result[0]
        }
      })

But it looks dirty. Any feedback?

jaumard commented 7 years ago

Not a solution for me :) options.returning is manage directly by sequelize, why do you want to do this ?

JAertgeerts commented 7 years ago

Ok with the returning property, all not necessary. However:

    else {
      criteria = {
        where: {
          [Model.primaryKeyAttribute]: criteria
        }
      }

      query = Model.update(values, _.extend(criteria, options)).then(results => results[0])

results[0] - prevents me to get results[1] which is the row that has been changed.

jaumard commented 7 years ago

You're right then we should just return results for everyone... it's a common sequelize result don't know why I put [0], maybe just to avoid people from doing it but if they're used to sequelize it's not a problem

JAertgeerts commented 7 years ago

That would work. Then options.returning will work as expected.