For anyone still following this, I’ve forked sails-hook-orm at https://www.npmjs.com/package/sails-hook-safe-orm / https://github.com/alxndrsn/sails-hook-orm so that any undefined values passed to update() or updateOne() WHERE clauses should be rejected with an Error.

I intend to keep that lib and the underlying fork of waterline up-to-date as required. If you want specific vesions released then let me know.

@mikermcneil I’d be interested in hearing your thoughts on the approach in that fork (visible at https://github.com/balderdashy/waterline/compare/master...alxndrsn:master); if these are of interest then I’d be happy to fix them up for a PR into the official waterline repo.

I was curious to see what the best practices around this issue are in other Node ORM’s. I made a simple test of 6 ORM packages, including Waterline, and included the results below.

Waterline:

var users = await User.update({ 'id': undefined }, { lastName: 'Spaghetti' }).fetch();

update `user` set `lastName` = 'Spaghetti' limit 9007199254740991

Loopback:

User.updateAll({id: undefined}, {name: 'Fred'}, function(err, info) {});

UPDATE `User` SET `name`='Fred'

Same behavior as Waterline, intentionally changes {id: undefined} to {}.

Configuration option “normalizeUndefinedInQuery” available to modify this behavior. https://github.com/strongloop/loopback/issues/637#issuecomment-146574151

Sequelize:

await User.update({ favorite_color: 'red' }, { where: { username: undefined } });

UPDATE `users` SET `favorite_color`='red',`updatedAt`='2018-06-02 13:22:51' WHERE `id` = NULL

Bookshelf (Knex Query Builder):

await User.where({ 'id': undefined }).save(
        { name: 'Fred' },
        { method: 'update', patch: true }
);

    Error: Undefined binding(s) detected when compiling UPDATE query: update `bookshelf_users` set `name` = ? where `id` = ?
            at QueryCompiler_MySQL.toSQL (/home/src/orm_test/books/node_modules/knex/lib/query/compiler.js:131:13)

TypeORM:

await getConnection()
        .createQueryBuilder()
        .update(User)
        .set({ firstName: "Mickie", lastName: "Mouse" })
        .where("id = :id", { id: undefined })
        .execute();

UPDATE `user` SET `firstName` = 'Mickie', `lastName` = 'Mouse' WHERE `id` = NULL

Node-ORM2 (No longer actively maintained):

Person.find({ id: undefined }, function (err, people) {});

SELECT `name`, `surname`, `age`, `male`, `id` FROM `person` WHERE `id` IS NULL

@luislobo Throwing errors when undefined values are detected is certainly the safest way to go. If we were out of beta I’d be more hesitant since it’d be a significant breaking change, but in this case, I’m on board.

So it sounds like the current proposal is:

1. The addition of a meta option removeUndefined, which if true strips undefined values from criteria objects (as in the current behavior). Defaults to false.
2. If removeUndefined is false and an undefined value is detected in a criteria object, an Error is thrown.

Examples:

// This would throw an error.
User.find({ name: undefined }).exec(...)

// This would retrieve all User records.
User.find({ name: undefined }).meta({ removeUndefined: true }).exec(...)

I agree with @Josebaseba, it would be great to be able to opt out of the “auto Remove Undefined From Where Clauses” feature. Although I see it’s convenience, it’s consequences can be, as demonstrated, complicated. Writing a little bit more code to handle specific criteria cases is not that bad for us. For example, we do have a “if this kind of user use this criteria and if not, use this other”, and we are fine with that.

@luislobo - They will all be located in balderdashy/sails - yes. 👍

Hi @Josebaseba, @luislobo, @sreed101 and @davidreghay, We are currently consolidating all Sails issues into one repo to help keep a better eye on things. We defiantly dont want to loss this well loved/documented issue! Cheers all!

I like having updateOne and deleteOne methods.

But I think the problem lies with acting on documents/records that don’t match with your “expected” criteria.

The same issues causing a condition to match a different set of documents/records could happen to one single document/record

await Person.update({
  SSN: personSSN
})
.set({
   someField: 'value'
});

Imagine having one record. SSN: ‘123456’, and the calling service/application tries to update SSN:‘847327’. For some bug/issue, the API receives personSSN == undefined, it should not update the SSN correspoding to 123456, because it doesn’t match, but in this particular case it will. I know it’s an edge case, but that can happen, right?

Thanks @luislobo I’m hoping against hope for it to be accepted 😆

SailsBot, don’t close it yet 😃 #1545 seems to fix it!

Hey @davidreghay it looks good to me. Thanks!

Hi @Josebaseba, @luislobo, @Xandrak, @mikermcneil, @sgress454

I’ve opened https://github.com/balderdashy/waterline/pull/1545 which is an attempt at implementing the solution as outlined by @mikermcneil

Please have a look and let me know what you think!

I like @Josebaseba suggestion, but, in addition to that, I would add an exception whenever a “value” in a where clause is undefined, in the case removeUndefined: false. If you are “wanting” undefined NOT to be removed, you are worried about getting undefined values from your inputs, so you should be warned/stopped in those cases. Of course… one should validate it’s inputs… but sometimes having a large codebase that currently behaves in a different way, it’s easier to search for all update and delete statements and add a meta option, for the sake of the upgrade to Sails v1.

We just updated an entire database collection on accident IN prod due to this.