MongoDB `find()` with String _id gives an error

[kolypto]

Using the MongoDB driver, when the get() method is given a string, it tries to convert it to ObjectID and gets the following error:

Error: Argument passed in must be a single String of 12 bytes or a string of 24 hex characters

In fact, there's nothing wrong with plain String _id fields, and this annoying issue prevents us from using orm with such collections.

[notheotherben]

You should be able to solve this by explicitly defining an _id field on the model you're manipulating and by setting its type to 'text'. You'll find that orm defines a default id field regardless of database engine if one is not explicitly set (SQL uses INT, MongoDB just uses the _id field).

If you've already done this and are still experiencing an issue then let us know and we'll look at finding a solution.

[kolypto]

Here's my model:

var Config = db.define('Config', {
        _id: String
    }, {
        id: '_id',
        table: 'config'
    });

(this actually was going to store arbitrary objects keyed by a string).

Then, I try to query for a value:

Config.get('anything', function(){...});
Config.find({ _id: 'anything' }, function(){...});

Both produce an error at DML/mongodb.js:416:

if (key == "_id" && typeof value == "string") {
    value = new mongodb.ObjectID(value);
}

Also, observe the code above:

var val       = (key != "_id" ? value.val : new mongodb.ObjectID(value.val));

Changing this would require to ask users for explicit ObjectID coertion, but leaving as is - is hardcode.

What about requiring the users to explicitly define ObjectID as the property type? Or add an option for MongoDB models which enables this behavior?

[kolypto]

Tried to change in the properties definition:

_id: { type: 'text' },

same result.

[dresende]

If you use Config.get('your_id', cb) does it work?

[kolypto]

Can't test, but quite sure it does not: the driver always casts the _id field to ObjectID, and in my case the _id field is not going to be ObjectID.

[rafaelkaufmann]

Hi @kolypto, I've submitted a pull request to fix this a while ago (#387), but I guess @dresende hasn't gotten around to reviewing it yet. :/

[notheotherben]

Just looking over your pull request, I'm not awefully happy about sniffing _id the way you are doing it (assuming that if it's 12/24 characters long then it's an ObjectID). A slightly better (but still messy) solution would be to use a `/.{12}|[a-f0-9]{24}/ RegExp to test it - which will at least ensure that an error is never thrown when creating the ID but the best option would be to check the property's type.

Unfortunately it appears that at no point outside of sync does the database DML Driver have access to the model it's working with (which would be necessary for this fine grained control).

My suggestion would be to include the model in the opts object passed to all driver methods, thus allowing the driver to check the following conditions:

• Is the _id field explicitly defined in the model? If it is not then use ObjectID
• If it is defined, ensure that the value is of the correct type - as defined in the model - before continuing

[notheotherben]

So, to make things concise and easy to follow, I'll keep this post updated with concerns - approaches and things to look out for when implementing this. I'd like to get feedback from @dresende before adding the required functionality since it may break other things, or there may be a better way to go about handling it (he's vastly more knowledgable about the codebase than I am).

Approach

We need to treat "_id" as the MongoDB default (ObjectID) in any cases where the _id or id field is not defined for a model. If, however, it is defined then we should use the defined property type from the model instead of ObjectID.

While this doesn't perfectly fit with the way IDs are handled with SQL based engines, it represents a compromise between the limitations of MongoDB and the behaviour most developers expect from orm.

Concerns

One of the primary concerns with this approach is the need for access to the relevant model from within the DML driver. Without access to the model it is impossible to determine the declared type for the _id property. The easiest way to give the DML driver this information, without changing the driver API, would be to include it in the opts object which most driver methods receive.

Unfortunately, there are two problems with this approach, the first is that it requires hunting around the rest of the codebase to find all calls to DML driver methods and adding the model to their opts parameter, and the second is that some methods (insert, update and remove) don't take an opts object.

An alternate approach would be to add a model parameter to all DML driver methods and refactor the other SQL drivers to match the new API. This is something which would require the go-ahead of @dresende before we could do it as it will mean breaking any 3rd party plugin drivers and will likely mean a major version revision (2.x -> 2.y).

Things to watch out for

When implementing this, it is important to keep in mind that the hasMany method within the MongoDB DML driver also exibits the same ObjectID behaviour, and will have to be modified to handle it correctly - as will all of its handler functions (has, get, add, del).

Please feel free to comment on this, give any suggestions etc, and I will try to keep this post up to date with the latest info.

[rafaelkaufmann]

IMO, this is definitely the appropriate approach. However, while we're doing this, are there any major objections to getting my ugly hack into the codebase? :) It is more or less guaranteed not to break anything that currently works, since the condition checked to infer if value is not an ObjectID (value != null && 'number' != typeof value && (value.length != 12 && value.length != 24))) is exactly the same which is tested inside mongodb.ObjectID (and which throws an exception if true). Thus, an application would only break with this patch if it were currently counting on getting this specific exception thrown by mongodb.ObjectID and doing something conditional on it.

[notheotherben]

My only issue with implementing your solution would be that something like 'hello world!' in the "_id" field (which we can obviously see is not an ObjectID) would still be converted for everyone.

Maybe add a config entry (defaulting to off) which enables your code until we can get something more robust in?

[rafaelkaufmann]

Working on it, will add to pull request.

[rafaelkaufmann]

Having a hard time getting those tests to work! In the meantime, @kolypto, feel free to clone my fork and use it :)

[kolypto]

@rafaelkaufmann, having multiple issues in mind, I actually ended up creating a new ORM for MongoDB & PostgreSQL.. :) Thanks anyway!

[dresende]

@kolypto if the automatic conversion of _id fields would match the format of ObjectIDs and just convert if in a valid format, perhaps this could be solved?

[kolypto]

@dresende , not really... Imagine a hex field which is not an ObjectID. You need to introduce new field type for it, and apply the convertion for this type only.