Find, Destroy or Update methods ignore undefined type attributes

[Josebaseba]

Waterline version: 0.13.0-rc11
Node version: 4.5
NPM version: 2.15.9
Operating system: MacOs

---

Hi guys, I found an issue that I'm not sure if it's a bug or something that you have considered. If you do something like this:

User.update({id: undefined}, {name: 'joe'}).exec()

This is gonna update ALL the users in the database, because the mongo query is gonna be something like this {}. The same thing happens with find/destroy methods.

So imagine that in your code you don't realize data X value is undefined and you lauch a destroy against it, you'll end up dropping all the collection with no clue about what just happened.

Thanks for your time.

[sailsbot]

@Josebaseba Thanks for posting, we'll take a look as soon as possible.

---

For help with questions about Sails, click here. If you’re interested in hiring @sailsbot and her minions in Austin, click here.

[luislobo]

I found the same issue before. Good thing it was not in prod.

[Xandrak]

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

[mikermcneil]

This is intentional, but it's not too late to change the behavior for v1, so let's definitely discuss.

tldr; I wrote a big long thing, so if you don't have time to read it all, please just skip down to "Questions" below :)

Why it works this way now

The "newRecord" and "valuesToSet" side of things have always done this (stripped keys with undefined values). I realize this isn't what we're talking about here-- just making sure that's clear from the get-go so we're on the same page (and for anyone stumbling on this issue down the road.)

For criteria (and specifically for the where clause), the reason to do it this way is for use cases where you want to apply an optional constraint.

That might be for convenience within your code-- e.g. letting us do this:

// Look up all facilities if the logged-in user is an admin,
// otherwise look up just the facilities she owns.
// (Also, if the name search or country filters were specified, apply them)
var facilities = await Facility.find({
  owner: loggedInUser.isAdmin ? undefined : this.req.session.userId,
  name: inputs.name ? { contains: inputs.name } : undefined,
  country: inputs.country ? { contains: inputs.country } : undefined,
  isSoftDeleted: false
});

Instead of this (having to build the where clause separately), which is considerably more verbose:

// Look up all facilities if the logged-in user is an admin,
// otherwise look up just the facilities she owns.
// (Also, if the name search or country filters were specified, apply them)
var constraintsByAttrName = {
  isSoftDeleted: false
};

if (!loggedInUser.isAdmin) {
  constraintsByAttrName.owner = loggedInUser.id;
}

if (inputs.name) {
  constraintsByAttrName.name = inputs.name;
}

if (inputs.country) {
  constraintsByAttrName.country = inputs.country;
}

var facilities = await Facility.find(constraintsByAttrName);

We could try refactoring it a bit, which makes the code more concise -- but doing so makes it considerably more opaque, since you still have to address the non-fungibility of undefined vs. omission:

// Look up all facilities if the logged-in user is an admin,
// otherwise look up just the facilities she owns.
// (Also, if the name search or country filters were specified, apply them)
var constraintsByAttrName = {
  owner: loggedInUser.isAdmin ? undefined : this.req.session.userId,
  name: inputs.name ? { contains: inputs.name } : undefined,
  country: inputs.country ? { contains: inputs.country } : undefined,
  isSoftDeleted: false
};

_.each(_.keys(constraintsByAttrName), (attrName)=>{
  if (constraintsByAttrName[attrName] === undefined) {
    delete constraintsByAttrName[attrName];
  }
});

var facilities = await Facility.find(whereClause);

You probably also noticed in the example above that part of the reason it got way worse was because our use case involved an optional "AND" filter as a parameter. That's where this makes the most difference, it seems. Here's another example of where relying on undefined-stripping saves code complexity / development time / frustration:

{
  description: 'Archive posts more than 45 days old (within the additional geographical boundaries, if specified.)',
  inputs: {
    minLat: { type: 'number' },
    maxLat: { type: 'number' },
    minLong: { type: 'number' },
    maxLong: { type: 'number' },
  },
  fn: async function(inputs, exits){
    await Post.update({
      createdAt: { '<': Date.now()-(1000*60*60*24*45) },
      minLat: inputs.minLat,
      maxLat: inputs.maxLat,
      minLong: inputs.minLong,
      maxLong: inputs.maxLong,
    }).set({ isArchived: true });

    return exits.success();
  }
}

As you can imagine, the above code would be a lot longer if we couldn't rely on the stripping-out of undefined keys in criteria. (This is definitely less of a big deal now that we can take advantage of async/await -- but it's still a material advantage that helps keeps code more concise.)

Of course, we have to contrast that with the downsides.

Thoughts

All that said, I've had similar experiences to what @Josebaseba @Xandrak and @luislobo are describing, and I think some kind of configurable warning, or failing with an error (e.g. assuming this was disable-able using .meta()) would be worth exploring. I'm just wary about doing any of that lightly, because the last thing we want is to have to change it back (it's a pretty insidious breaking change as is, and it'd be really frustrating to have it flip-flop more than once).

We'd also need to think through how any change here would affect recursive parsing-- or possibly just try adding the warning and then try a ton of different things (currently we're relying on this rhs-stripping logic in the generic where clause parse logic, which takes care of a few other edge cases involving "fracturing" the where clause, if memory serves.)

One last thing on that: There are a few other cases where the where clause parsing in FS2Q does special stuff: nin/in/and/or arrays are automatically cleansed of undefined values, and are automatically collapsed. For the recursion to work as implemented, it currently relies on undefined constraints to be understood as no-ops, which it then puzzles over when cleansing the array for the parent predicate term (or/and). So any change here would need to be carefully examined to make sure that all still makes sense.

See https://github.com/balderdashy/waterline/blob/5a74dfb8cb8b3926aef1966db89e72c82a7d8779/lib/waterline/utils/query/private/normalize-where-clause.js#L177-L229 for more on that.

Here's a more concrete example:

// If labels array turns out to be empty, it's treated as a "void" term,
// and thus (since it's at the top level, i.e. an "and") no issues are matched.
await Issue.find({
  primaryLabel: { in: _.without(inputs.labels, 'bug') }
});

// Similar to above, but in this case, it's more clear how its treatment depends on the context:
// 
// In this case, if the array passed to `in` turns up empty, it's still understood as void,
// which then collapses that disjunct in the "or" predicate into `false`.
// Since `false` is the same thing as the void term, it matches _nothing_.
// 
// But this time, since we're in an "or" context instead an "and", we don't make the
// entire `where` clause void.  Instead, this acts like `||false`, which means it's effectively
// disregarded and pruned out of the "or" predicate, leaving only the second disjunct.
//
// So at the end of the day, if the array provided for `in` turns up empty, we end up
// fetching _all_ issues that were created by user #13.
await Issue.find({
  or: [
    { primaryLabel: { in: _.without(inputs.labels, 'bug') } },
    { creator: 13 }
  ]
});

// Here's another one.  In this case, you only fetch issues that WEREN'T
// created by members of the specified organization.  But if the only users
// that exist are members of the specified organization (or perhaps if there're
// no users at all), then this ends up as an empty array.  Since that's inside of
// a "nin", rather than an "in", it is understood as "universal" rather than "void".
//
// In other words, since there are no users in the specified org, excluding those
// users is basically the same as matching any user-- any "creator". 
// Furthermore, in this case, since we're effectively inside an "and"
// (b/c we're at the top level of the "where" clause), the universal term is
// understood like `&&true` -- and effectively ignored.
//
// So at the end of the day, all issues that have `isDeleted: false` are matched
// and returned.
var orgMembers = await User.find({ memberOf: inputs.organizationId });
await Issue.find({
  isDeleted: false,
  creator: { nin: _.pluck(orgMembers, 'id') }
});

// Finally, this example demonstrates how all the pieces fit together,
// as well as how the stripping of `undefined` values works within
// "and"/"or" predicates, and within "in"/"nin" constraints.
//
// This example matches all issues whose primary label is one of the specified
// labels (except "bug"), and also matches any other issues which were
// created by the logged-in user.  If there is no logged-in user, then the `undefined`
// is ignored.  (In an "or" predicate, an `undefined` disjunct operates like `||false`.
// In an "and" predicate, an `undefined` conjunct operates like `&&true`.  i.e. in
// either case, it is ignored.)
await Issue.find({
  or: [
    { primaryLabel: { in: _.without(inputs.labels, 'bug') } },
    this.req.session.userId ? { creator: this.req.session.userId } : undefined
  ]
});

Questions

@luislobo @Xandrak @Josebaseba @sgress454:

1. Did the explanation above make sense, or did I go too far into the weeds?
2. Has this ever been a problem for you when updating/destroying by something other than primary key value? Or has the behavior only felt unexpected when attempting to update/destroy a particular record by id?
3. Has this behavior ever felt unexpected when doing a .find(), .findOne(), .count(), .avg(), or .sum()? Or is it just with update and destroy?

[Josebaseba]

Hi @mikermcneil, thanks for the answer it's good to know that this is an intentional feature and not a bug.

First, answering your question:

1. It makes sense
2. For us it's a problem (updating/deleting), we're afraid of upgrading to sails1 and finding that we've updated all the users' table because of a {email: undefined} for example
3. That's a good question, I think that I'll go with the same behavior in all of them.

So, I'll add an option in the .meta()some thing like: removeUndefined: true (default false). Why? Because you're right deleting the undefined values can make the code easier to read/understand but facing an accidental drop of an entire table is very dangerous.

[luislobo]

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.

[sgress454]

@mikermcneil @Josebaseba @luislobo The explanation makes sense to me, but I don't count :P.

As far as impact, I've only experienced the problem when running an .update() on something other than a primary key. When it really bites you is when you do something like .update({ name: someObj.name }) while assuming that someObj.name will always be defined. It's usually due to a logical bug in the app, but it's hard to figure that out and in the meantime, you might have blown your database.

After mulling this over for quite some time, I don't think that the answer is to strip out undefined values or to throw warnings / errors. I think what we should do is transform undefined values into the base value for that attribute (or null if allowNull is set for the attribute). That is, after all, what being "undefined" means in Waterline 0.13.x.

When I see this example:

var facilities = await Facility.find({
  owner: loggedInUser.isAdmin ? undefined : this.req.session.userId,
});

I don't intuitively read it as, "If the logged in user is an admin, then find all facilities". I read it as, "If the logged in user is an admin, then find all facilities that don't have an owner". Casting undefined values to the proper base value would support that. It would mean an extra step if you did intend to find all facilities when the user is an admin, but I'd argue that it's worth it for the clarity provided in all cases.

I took some time to look into how various other software handles this case, and it's all over the map. The Mongo client app returns an error if you run a query with an undefined value, but the Node driver seems to translate it into an {$exists: false} clause, so that only records that don't have that field are returned (you could think of undefined as Mongo's "base value" for every field). The Node Postgres driver allows you to bind undefined to a parameterized query, e.g.

pool.query('select * from test where age = $1', [undefined], console.log)

and you'll just always get zero results (even if there are records where age is null). Moving on to the ORM world, if you try the Postgres example with Knex, it throws an error about "Undefined binding(s) detected when compiling SELECT query".

So, good arguments all around. Casting to the base value is the most intuitive for me and seems like the most consistent thing with our scheme in general, and has the benefit of not throwing errors which might not be seen until production. And in debugging, in my recent experience the question of "why didn't any rows update?" is a lot easier to figure out then, "why did ALL of my rows update"?

[luislobo]

@sgress454 That is valid, unless, you do have values that are NULL, and are affected by your update, but you didn't intend to update those. In the case of an update or delete, I still prefer to fail because the only way to recover from that is a database restore, if you have such thing ;).

I don't think that doing any smart thing/asumption about an undefined is desirable in our case.

Although we do Pull Requests review and even sometimes Pair review, not all developers are senior or "awake" 100% of the time... Errors happen, validations are skipped, data is deleted. Massive Devops suicide happen.

[luislobo]

Also, databases might even handle NULL differently, depending on it's configuration (ansinull, in some of them) so, it's not that easy neither to assume something like that.

Some references
https://stackoverflow.com/a/1843460/358634

http://sqlanywhere-forum.sap.com/questions/9285/is-null-vs-isnull-vs-null-correct-usage

http://dcx.sybase.com/index.html#1201/en/dbreference/nulls.html

https://www.postgresql.org/docs/8.3/static/functions-comparison.html

[sgress454]

@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(...)

[Xandrak]

@mikermcneil @sgress454 Thanks for taking the time to address this! Let me clarify that it was actually my code error that brought this to light--unfortunately for us, it was in production. So I am not blaming Sails or anything. This could have been avoided with a subtle switch from using a .then() to an .each() after a .find(). I should have been expecting the possibility of multiple values to be returned, but for whatever reason it slipped my mind. Please excuse the following, hastily written example 😉 :

...
// code before fix
model.find(findThis)
.then((foundModel) => {
  model.update({ id: foundModel.id }, updateModelValues)
})
...
// code after fix
model.find(findThis)
.each((foundModel) => {
  model.update({ id: foundModel.id }, updateModelValues)
})
...

We are now wrapping every update/destroy in our API within a check to make sure the criteria passed in is never undefined. We realize this is not exactly efficient or elegant, but it is the current solution we have.

At any rate, we did not, however, expect the default behavior for an undefined criteria value to update the entire collection. We did expect an error to be thrown due to thinking that we do not ever send in undefined values on purpose. This may be specific only for our team. Personally, I would still be wary of such methods that affect entire collections at once. I realize this is not the case for everyone--just my two cents.

@mikermcneil To answer your previous questions:

1. Yes, it makes sense.
2. In our case it only happened by id, but we have yet to test anything else.
3. Currently, just on .update() 😄 , but I think I would echo @Josebaseba's #3 answer as well.

@Josebaseba @luislobo Thanks for your input and bringing this issue to light.

[luislobo]

@sgress454 I agree with your last comment.

[Josebaseba]

@mikermcneil @sgress454 guys, this is a big deal, right now my production DB got all the collection updated at the same time, even if I went query by query making sure that everything was under control and right now I'm screwed... Is there anything that I can do to turn off this "feature"?

[mikermcneil]

@Josebaseba sorry to hear the upgrade is causing you trouble. Thanks for following up.

We're relying on undefined-stripping in "where" clauses a lot, and in new code, it definitely saves time without introducing problems (it reduces the complexity of writing code to do optional filters quite a lot). Thus I'm still not convinced that we should change this default behavior.

That said, I understand you might need an immediate change to help through the upgrade. I like @sgress454's proposal, with three tweaks:
• the flag needs to be an option that you enable to change the default behavior. The default behavior will continue to be what it is today: that undefined conjuncts are stripped from the 'where' clause in any kind of Waterline query.
• the option should be explicit in its name to clarify that it only applies to the 'where' clause:failOnUndefinedConjunctsInWhereClause, which defaults to false. Setting it to true causes an error instead of the default (current) behavior
• instead of a meta key (no one would ever want to type that all the time anyway), we should do this only as a model setting (no way to override on a query-by-query basis). That way, we minimize complexity while making it useful as a one line of code paste-in (in your config/models.js file)

@Josebaseba if you've got a more immediate-term need and can't wait for the new model setting (it'll need to be added, tested, and documented), I think the best bet is to modify the code in normalize-where-clause I highlighted above and use that.

Note about implementation: Since this isn't a meta key, implementing it is easier. We already have access to the orm in that file linked previously, so we need only to look up the WLModel, check if it has the model setting as true, and if so, skip the undefined stripping. (No need to pass in more arguments or anything)

[mikermcneil]

(BTW since that's such a mouthful, let's also, in parallel, down the road, see about coming up with a definition of what we want "strict mode" to look like in the future. It might make sense to lump a few similar features with the same motivations into that bucket)