fix to-objectid crashes when id is not a valid object id

[garrefa]

I have a couple of legacy mongo collections where _id is a Meteor Random and not ObjectId. Since mongorito searches for _id in query and automatic converts the id to obj id I was having crash all the time when i try to search this collection. I tried with find({ _id }) too, but then i realize mongorito was converting to obj id in find too, which is nice feature. This small change fix the crash problemas. If the user send a invalid _id, it will return null instead of crash. And it will still work with those collections where _id is not ObjectId.

[vadimdemedes]

I think there must be a better way of checking for other kinds of IDs, instead of swallowing any errors and returning id. You could check for some documented Meteor Random properties and validate their existence on the object.

[garrefa]

I could, but the id could be another thing. In my case, i never used Meteor, but the guys that started the project were using the meteor-random lib (https://github.com/trever/meteor-random). Some of my collections have those 17 chars meteor-random ids.

Random.id([n]) Returns a unique identifier, such as "Jjwjg6gouWLXhMGKW", that is likely to be unique in the whole world. The optional argument n specifies the length of the identifier in characters and defaults to 17.

But as you can see, anyone can change the parameters. I dont agree when you say im swallowing any errors, the only errors that can occur is a crash caused by trying to create a ObjectId with a invalid string of chars. But if it was never ment to be a ObjectId so the error should not exist. If its not a ObjectId, than the mongo id is a plain string. What problem can this solution raise?

Thanks

[garrefa]

Don't you agree? I could use emails as _ids... that would be valid, but not a valid ObjectId.

[vadimdemedes]

If there are invalid IDs in the database, I want Mongorito to fail and let the developer fix the issue. With this change, Mongorito would quietly return whatever invalid value there is.

How is Meteor Random handled in other MongoDB libraries, like Mongoose or monk?

[garrefa]

Mongoose and monk dont assume all _id are ObjectId. They know that in MongoDB, any thing is a valid _id, it just have to be unique. You can have 1, 2, 3, 4 as valid _id. This pull request is aimed in keep your automatic conversion to ObjectId the way it is today, but still supporting _id that are nor ObjectId.

MongoDB Doc: https://docs.mongodb.com/manual/reference/method/db.collection.insert/

Create Collection

If the collection does not exist, then the insert() method will create the collection.

_id Field

If the document does not specify an _id field, then MongoDB will add the _id field and assign a unique ObjectId for the document before inserting. Most drivers create an ObjectId and insert the _id field, but the mongod will create and populate the _id if the driver or application does not.

If the document contains an _id field, the _id value must be unique within the collection to avoid duplicate key error.

Example

Insert a Document Specifying an _id Field¶

In the following example, the document passed to the insert() method includes the _id field. The value of _id must be unique within the collection to avoid duplicate key error.

db.products.insert( { _id: 10, item: "box", qty: 20 } )

In mongoose and monk, users.findOne({ _id: 10 }) is fine...

[vadimdemedes]

This pull request is aimed in keep your automatic conversion to ObjectId the way it is today, but still supporting _id that are nor ObjectId.

In that case, I think Mongorito should only auto convert supplied _id into ObjectId if it's a string of 24 chars. If it does not look like an ObjectId string, just pass it as it is.

[vadimdemedes]

This change should be made in util/to-object-id.js.

[vadimdemedes]

Closing, since it's out-dated, v3 is out and it doesn't support auto casting to ObjectId.