Fix bug where BSON ObjectID's wouldn't work

[Tarang]

Fixes selector driver so documents querieid with traditional bson objectid's can be updated, removed.. etc

Might need some work on objectid validation

[glasser]

This seems a little magical to me. Why 24? Is there a link you can provide to document this?

[Tarang]

Its from the spec, ObjectID's are 12 bytes - 24 characters long
Validation could be better

http://www.mongodb.org/display/DOCS/Object+IDs

[glasser]

Thanks for the pull request. Here are some initial comments.

I also admit that I am not personally very familiar with these BSON ObjectIDs. Can you add a test to mongo_livedata_tests.js that demonstrates the issue that this solves, and ensures that we don't break your fix in the future?

Have you submitted a CLA yet? See http://contribute.meteor.com/

[glasser]

So I'm a little confused about the context where this happens. Is this only if the user actually does a collection.update({ _id: "abcdefabcdef12345612346" })? Isn't the answer just to make sure the user inserts the MongoDB.BSONPure.ObjectId call in this case? Or is there some place in Meteor where Meteor strips the ObjectId-ness? Do you have a test case for the bug?

[Tarang]

Yes using MongoDB.BSONPure.ObjectId would fix the problem but MongoDB is out of context (im not sure) within Meteor js files but I've been trying all sorts of things that way, it should just work with a string as an id.

A test case would be to connect to the mongodb via the console and add a document with an objectid:

x={ name: "joe" }
{ name : "joe" }
db.people.save(x)
{ name : "joe" , _id : ObjectId( "47cc67093475061e3d95369d" ) }

Then try accessing this via Meteor

var people = Meteor.Collection('people')
people.update('47cc67093475061e3d95369d', {name:'Bob'});
people.remove('47cc67093475061e3d95369d');

or even

people.update({'_id' : '47cc67093475061e3d95369d' }, {name:'Bob'});
people.remove({'_id' : '47cc67093475061e3d95369d' });

None of these would work but they should.

[Tarang]

In my opinion it should be this way with the strings instead of putting in a BSONPure.ObjectId because:

people.findOne('47cc67093475061e3d95369d' );

Works and it would be best to maintain a consistency of the paradigm across to update and remove

[glasser]

For what it's worth, when I tested this, people.findOne('47cc67093475061e3d95369d') did not work.

I do agree that having to type MongoDB.BSONPure.ObjectID is painful, though, but the magic treatment of 24-character strings is not great. I'd prefer either:

(a) Importing MongoDB.BSONPure.ObjectID into the Meteor namespace somewhere
or (my preference right now)
(b) Adding an option to the Meteor.Collection constructor like idsAreObjectIDs: true that tells it to apply this transformation to all queries/updates/etc that involve _id.

The latter is my preference but it's a bit annoying: Meteor.Collection doesn't take an options object yet, the _rewriteSelector function doesn't have access to the Collection object, etc. But I think in the long run, saying "You can use arbitrary JSON objects as your _id... but if they are strings they can't be 24 characters wrong" is not an interface I'd be excited about supporting.

[Tarang]

What about validating whether the id's are BSON objectids by checking they are not only 24 characters, but hex and to the spec of an objectid so that no other string can pass off as an objectid? That was my original idea but I was looking to use the BSONPure.ObjectId.isValid method but I can't seem to get it to work properly.

The way meteor is designed is to be as close to 'magic' and having it 'just work' as possible. What do you think of checking whether its an ObjectID to spec over the options you presented?

[Tarang]

That was my original plan, if I can get _isObjectId() to work efficiently so not every string can pass off, but only one which is actually an objectid then its less painstaking and more magical. The idea of meteor is to take away as much of the hassle as possible.

[Tarang]

Im able to get findOne() and find() to work with id's. They've not been created the same way in the mongo console, rather via Mongoid and rails but it should be the same thing

[badslug]

I just got bit by this one as well. It definitely seems like it would be most Meteor/magical (and best for the framework) to invisibly handle the conversion.

[badslug]

@Tarangp I can verify that findOne() and find() work for me too (whether they have ObjectId's or Meteor IDs or any other string ID). It's just the collection modification methods that are failing.

@glasser I'm not sure either option will work.

For a), I don't think on the client, we can properly detect the ObjectID-ness of an id. My brief look at the minimongo source makes me think that ObjectIds never make it to the client - they are transformed into strings in minimongo no matter what. (Of course, that could be entirely wrong...) When I do a find() on the client, the object I get has the _id field as a string whether it's an ObjectId or not in Mongo. If the ObjectId object is preserved in minimongo, we could just use 'instanceof' to properly handle it but it currently won't work... (I suppose if the _id were any immutable object, we could keep the _id object and pass it as a token and have the proper behavior).

For b) as an example, we have some documents created using Meteor which doesn't use ObjectIds for object IDs and some being imported from foreign systems that do use ObjectIDs in the same collection. A collection-wide option like b) would break update code for some of the documents either way... (unless the flag forced Meteor to also create all new documents using ObjectId's too.)

[glasser]

Taking a further look at this, I notice that even in direct Mongo shell, 24-character strings aren't handled specially:

$ meteor mongo
MongoDB shell version: 2.2.0
connecting to: 127.0.0.1:3002/meteor
> db.people.save({name: "joe"})
> db.people.findOne()
{ "_id" : ObjectId("50557455079da77ab4c9fdd2"), "name" : "joe" }
> db.people.findOne("50557455079da77ab4c9fdd2")
null
> db.people.findOne({_id: "50557455079da77ab4c9fdd2"})
null
> db.people.findOne({_id: ObjectId("50557455079da77ab4c9fdd2")})
{ "_id" : ObjectId("50557455079da77ab4c9fdd2"), "name" : "joe" }

So I don't think handling them magically in Meteor is correct. But we should probably offer some function to convert to Mongo ObjectIds (which should work in the client too).

[glasser]

I added a bunch of comments to the initial issue, #61. Because I don't believe that this pull request is the right direction towards fixing the issue, I'm going to close the pull request for now, but leave the issue open.