findOne() returns non-empty result if a value of filtering criteria is undefined

[voitau]

Basic cases:

1. Car.findOne(function(err, car) {...}) - no filtering applied
2. Car.findOne({ where: { number: "" }}, function(err, car) {...}) - filtering by empty value;

Question:

• Car.findOne({ where: { number: undefined }}, function(err, car) {...}) - filtering is not applied, same as case 1. The question is whether it should it behave like case 2 instead.

Could you please clarify whether this is an expected behavior.

Some more details:

• undefined properties seem to be removed from query here: https://github.com/strongloop/loopback-datasource-juggler/blob/master/lib/dao.js#L512
• sample code with tests: https://gist.github.com/voitau/f03ea7564ee77ac5b6a5

[bajtos]

This looks like a bug to me. @raymondfeng could you please confirm?

@voitau Would you mind to submit a pull request? The implementation of findOne is in loopback-datasource-juggler: lib/dao.js

[raymondfeng]

As you have noticed, we remove undefined value from the where clause. So {where: {name: undefined}} is the same as {where: {}} and they match any records.

For case 2 {where: {number: ''}}, what is the type of number property? What DB do you use?

[voitau]

@raymondfeng, DB is a memory db, Car.number property is of string type:

{
  "name": "Car",
  "plural": "Cars",
  "base": "PersistedModel",
  "properties": {
    "number": {
      "type": "string",
      "required": true
    }
  }
}

I shared the complete sample project here: https://github.com/voitau/loopback-example-findbyundefined
Which actually shows that REST API behaves exactly the way one might expect to:

• filter by value, if value is specified,
• don't filter, if no filtering criteria is specified,
• filter by empty value, if empty value is specified.

So, here is my understanding of difference:

• {where: {name: "value"}} filter the records, which have name set to "value".
• {where: {}} - no filtering rules applied,
• {where: {name: undefined}}, {where: {name: ""}} filter the records, which do not have this property set.

I can certainly submit a pull request for this method. I also think it may be related to find() method, which I can check as well and the behavior probably has to be consistent.

The only ramification of this change I see is that some existing applications using this functionality can be broken due to the reason I'm facing myself during development: let's assume there is a service method, which accepts the query. The query for some reason was not populated on a higher level. As a result while the query is incorrect, there is still a record in the response.

@bajtos, @raymondfeng, please let me know if this does make sense, so that I can start working on a fix for this.

[bajtos]

I don't have a strong opinion here. Since where: { name: '' } (and possibly where: { name: null }) can be used to express the condition "name does not have any value", we are free to choose how where: {name: undefined} should be interpreted.

I am slightly inclined to keep it as it is for the sake of backwards compatibility. Perhaps add a warning to notify the developer that the query may not be expressing what was their intent.

@voitau How did you run into this issue? What kind of a high-level problem are we trying to solve here?

[raymondfeng]

I would prefer the following:

1. {where: {name: "value"}} filter the records, which have name set to "value".
2. {where: {}} - no filtering rules applied,
3. {where: {name: undefined}} - no filtering rules applied (please note JSON doesn't have a undefined)
4. {where: {name: ""}} filter the records, which have name set to ""
5. {where: {name: null}} filter the records, which have name set to null. (IS NULL in SQL)

The current implementation probably treats 4 & 5 the same as 3.

[voitau]

@bajtos The high-level problem is the implementation of the method, which does some custom logic based on presence of a certain model instance, where presence is defined by a search by specified fields from the query model instance, passed as an argument in the body. What is being done:

• custom method with signature:

SomeModel.confirm = function(someModelQuery, next) {...}

and remote method signature including:

 accepts: { arg: 'data', type: 'SomeModel', http: { source: 'body' } }

where SomeModel is the Model with some required and optional fields

• in the custom method there is a findOne method invocation which schematically looks like this:

SomeModel.findOne({ where: {field1: someModelQuery.field1, field2: someModelQuery.field2} }, cb)

If REST API client will pass the empty query to confirm method, or the query, which contains only one of the two required fileds, they will get some "broader" results anyway, because undefined criterias will be removed. This is how findOne REST API method will behave. But this is not what expected from the method I'm trying to implement. That's why instead of passing query from custom REST API method to findOne as is, where clause was composed to get the values and search by them. So, the solution was to add custom validation of the someModelQuery. However, my original expectation was to get this filtering in findOne.

[voitau]

@raymondfeng here is how mongodb shell behaves in all these cases (maybe somehow this will be useful):

> db.cars.find({})
{ "_id" : ObjectId("5444349871af283b92c440cc"), "number" : "1234" }
{ "_id" : ObjectId("54447d9a71af283b92c440cd"), "number" : null }
{ "_id" : ObjectId("54447e7571af283b92c440ce"), "number" : "" }
> db.cars.findOne({})
{ "_id" : ObjectId("5444349871af283b92c440cc"), "number" : "1234" }
> db.cars.findOne({"number":"1234"})
{ "_id" : ObjectId("5444349871af283b92c440cc"), "number" : "1234" }
> db.cars.findOne({"number":null})
{ "_id" : ObjectId("54447d9a71af283b92c440cd"), "number" : null }
> db.cars.findOne({"number":""})
{ "_id" : ObjectId("54447e7571af283b92c440ce"), "number" : "" }
> db.cars.findOne({"number":undefined})
2014-10-19T20:25:02.851-0700 error: {
    "$err" : "Can't canonicalize query: BadValue cannot compare to undefined",
    "code" : 17287
} at src/mongo/shell/query.js:131

[johnsoftek]

IMO undefined should not be allowed as a search parameter value. It has no meaning with respect to the column values in the data base. Therefore the API must implement a rule to handle undefined search values. It would be simpler to disallow undefined search values.

[esco]

👍

Silently stripping out undefined values can have catastrophic outcomes for MongoDB queries.

var userId; // undefined due to application logic error

User.update({ where: { id: userId } }, { email: "[email protected]" })

The result of the above code is that all users in the database will have their email updated. Although this also requires an application error, this is not an expected outcome for the developer in any circumstance.

[jrschumacher]

To add to this a worse outcome which we have experienced was a rouge client bug removed all documents from our collection: User.destroyAll({ id: undefined });

cc @raymondfeng

[jrschumacher]

I would prefer the ability to set a configuration to:

1. Throw an error if undefined is found in a query throw new Error("Undefined is an unacceptable query value");
2. Convert undefined values to null
3. Work as it does now

[raymondfeng]

@jrschumacher +1. Do you want to submit a patch?

[jrschumacher]

@raymondfeng if you can point me to which method is stripping the undefined values I will have a patch you you by Wednesday! Spent 4 hours tracing through code and not a hair close.

[kblcuk]

@jrschumacher it's in datasource-juggler

Keep in mind that it is used internally as well, and sometimes should strip undefined values.

[jrschumacher]

@kblcuk thanks! I'll keep that in mind.

[esco]

@jrschumacher great idea

[jrschumacher]

@raymondfeng can you look at loopbackio/loopback-datasource-juggler#725 and give me some feedback.

[jrschumacher]

@voitau @esco a patch to this has been released in [email protected] you can use it by adding normalizeUndefinedInQuery setting to your datasource config or model config (see tests for more detail). Options are:

• "nullify" which converts undefined to null
• "throw" which throws an error Unexpected```undefined```in query
• "ignore" which strips the undefined key (default function)

@bajtos this issue can probably be closed now?

[esco]

Excellent:exclamation: :+1: thanks guys

[voitau]

Thank you!

[bilal68]

can we make findOne should return null if no condition is matched? cuz now it returnz empty object to me

[bajtos]

@bilal68 please open a new issue to discuss the problem you are experiencing, and include a small application we can use to reproduce it - see https://loopback.io/doc/en/contrib/Reporting-issues.html#loopback-4x-bugs