-
Notifications
You must be signed in to change notification settings - Fork 376
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
MongoDB find()
with String _id gives an error
#390
Comments
You should be able to solve this by explicitly defining an If you've already done this and are still experiencing an issue then let us know and we'll look at finding a solution. |
Here's my model:
(this actually was going to store arbitrary objects keyed by a string). Then, I try to query for a value:
Both produce an error at DML/mongodb.js:416:
Also, observe the code above:
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? |
Tried to change in the properties definition:
same result. |
If you use |
Can't test, but quite sure it does not: the driver always casts the |
… _ids on MongoDB driver
Just looking over your pull request, I'm not awefully happy about sniffing Unfortunately it appears that at no point outside of My suggestion would be to include the model in the
|
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). ApproachWe 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. ConcernsOne 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 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 An alternate approach would be to add a Things to watch out forWhen implementing this, it is important to keep in mind that the 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. |
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 |
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? |
Working on it, will add to pull request. |
Having a hard time getting those tests to work! In the meantime, @kolypto, feel free to clone my fork and use it :) |
@rafaelkaufmann, having multiple issues in mind, I actually ended up creating a new ORM for MongoDB & PostgreSQL.. :) Thanks anyway! |
@kolypto if the automatic conversion of |
@dresende , not really... Imagine a hex field which is not an ObjectID. |
Using the MongoDB driver, when the
get()
method is given a string, it tries to convert it to ObjectID and gets the following error:In fact, there's nothing wrong with plain String _id fields, and this annoying issue prevents us from using orm with such collections.
The text was updated successfully, but these errors were encountered: