Automattic / mongoose

MongoDB object modeling designed to work in an asynchronous environment.
https://mongoosejs.com
MIT License
26.95k stars 3.84k forks source link

Unpredictable behaviour of mongoose.Types.ObjectId() with falsify values. #8489

Open rahulmathews opened 4 years ago

rahulmathews commented 4 years ago

Do you want to request a feature or report a bug? Report a Bug.

What is the current behavior? undefined when turns into ObjectId takes the time stamp of Date.now()

If the current behavior is a bug, please provide the steps to reproduce.

mongoose.Types.ObjectId(undefined) //gives the object ID when converted to time stamp gives the value of Date.now(). I can confirm the same behaviour with null.

What is the expected behavior? Most of the people without checking the values of a parameter, they feed the values directly to the mongoose.Still Javascript is unpredictable with all the wats and wtfs. I am expecting to throw an error when mongoose.Types.ObjectId() receives falsify values.

What are the versions of Node.js, Mongoose and MongoDB you are using? Note that "latest" is not a version. Node Version : 12.13.0, MongoDb Version : 4.2.1 Mongooose Version : 5.8.3

vkarpov15 commented 4 years ago

May be worth considering for a future release. But why are you calling mongoose.Types.ObjectId() directly as opposed to going through a model?

rahulmathews commented 4 years ago

May be worth considering for a future release. But why are you calling mongoose.Types.ObjectId() directly as opposed to going through a model?

In a general find() query, we can use some_model.find(_id : "some_objectId_string") as the string will be automatically converted into ObjectId while fetching the result. But when we are using aggregate() with $match, the query goes something like this :

some_model.aggregate([
     $match : {
       some_id_key : mongoose.Types.ObjectId(req.query.some_id_key)
    }
])

In the above syntax, I have to manually convert a string to ObjectId and feed it to aggregate pipeline.So the above behaviour arises when we wont send some_id_key in query parameter. Also do you consider automatically converting a string value to ObjectId when fetching in aggregate pipeline?

vkarpov15 commented 4 years ago

@AbdelrahmanHafez wrote a nice plugin that will cast aggregation framework pipelines for you, you can find it here: https://github.com/AbdelrahmanHafez/mongoose-cast-aggregation. We're considering pulling this plugin into Mongoose, see #8464.

Also, see our docs on when to use queries versus when to use aggregations.

rahulmathews commented 4 years ago

I think its better to throw a warning atleast as most people are unaware of the behaviour or just patch the fix for this library.