Do not return MongoDB-specific _id to client API, except if specified in model definition #19

Merged
merged 2 commits into from Feb 27, 2014

Projects

None yet

4 participants

@pandaiolo
Contributor

As discussed in the Google Group, here is a PR that removes mongodb-specific _id from API exposition.

The main issue about this, other than having client-side model having to deal with unexpected _id attribute, is that when making a PUT with the same object, hence with _id included, it triggers a MongoDB exception, because it does not accept modifications of _id property.

After this commit, only the ID specified in the model definition will be returned, beeing id or another specified name.

Note : As explained in the README.md, I added the possibility to still rely on MongoDB _id attribute in the loopback model, as I suspect that some might need it, but in this case, it needs to be explicitly defined in the model definition, with ObjectID type. Example :

var ds = app.dataSources.db;
MyModel = ds.createModel('mymodel', {_id: ds.ObjectID});

This possibility required to add a property setter for the property named _id, because the default behaviour of the property setter in the model builder is to use the type as a caster :

property = DataType(value);

The main problem is that MongoDB javascript BSON ObjectID type cannot cast itself ! It will throw an error.

The custom MongoDB connector setter will take care of this issue by calling ObjectID only if value is not already instance of it.

I implemented this setter in two model definition hooks : Connector.prototype.define and Connector.prototype.defineProperty. If there are additional ways to set the ID property, they also should be hooked to in order to enable the custom setter.

I also added a bunch of tests to double check that all behaviour work with models with _id or other type of IDs, and updated README.md

slnode commented Feb 23, 2014

All is well
Refer to this link for build results: http://ci.strongloop.com/job/loopback-connector-mongodb/144/

@ritch ritch commented on an outdated diff Feb 24, 2014
lib/mongodb.js
@@ -120,6 +120,50 @@ MongoDB.prototype.connect = function (callback) {
}
};
+/**
+ * Hook the setter if model use MongoDB _id property as ID
+ * @param {Object} modelDefinition The model definition
+ */
+MongoDB.prototype.define = function (modelDefinition) {
+ var _idIsModelID = modelDefinition &&
+ modelDefinition.properties &&
+ modelDefinition.properties._id &&
+ modelDefinition.properties._id.id;
ritch
ritch Feb 24, 2014 Owner

This should use dataSource.idName(model) === '_id'.

@ritch ritch commented on an outdated diff Feb 24, 2014
@@ -1,6 +1,6 @@
{
"name": "loopback-connector-mongodb",
- "version": "1.1.6",
+ "version": "1.1.7",
ritch
ritch Feb 24, 2014 Owner

Thanks for updating the version number, but we will do this as part of the release if this PR lands. Please remove this.

@ritch ritch and 1 other commented on an outdated diff Feb 24, 2014
test/mongodb.test.js
@@ -32,15 +38,141 @@ describe('mongodb', function () {
beforeEach(function (done) {
User.destroyAll(function () {
Post.destroyAll(function () {
+ PostWithObjectId.destroyAll(function () {
+ done();
+ });
+ });
+ });
+ });
+
+
+ it('choosing _id as a model ID should throw an error if type is not MongoDBObjectID', function (done) {
+ (function() {
+ db.define('GoodModel', {_id: {type: db.ObjectID, id: true}})
+ }).should.not.throw();
+
+ (function() {
+ db.define('BadModel', {_id: {type: String, id: true}})
ritch
ritch Feb 24, 2014 Owner

Users are using this feature to support existing mongodb data. Does this fix require removing support for this?

pandaiolo
pandaiolo Feb 24, 2014 Contributor

Yes.... and I thought that adding my code with allowing the _id to be
defined, with type ObjectID, would need it to be enforced to this type
only, but all the tests actually pass with other types as well ! I will
remove that part of the commit, and update the readme, thanks.

Raymond suggested to remove the _id availability altogether, which would
simplify the PR a lot, but do you know any user who rely on accessing _id
property ? Would this be an issue if it is removed ?

2014-02-24 2:27 GMT+01:00 Ritchie Martori notifications@github.com:

In test/mongodb.test.js:

@@ -32,15 +38,141 @@ describe('mongodb', function () {
beforeEach(function (done) {
User.destroyAll(function () {
Post.destroyAll(function () {

  •    PostWithObjectId.destroyAll(function () {
    
  •      done();
    
  •    });
    
  •  });
    
  • });
  • });
  • it('choosing _id as a model ID should throw an error if type is not MongoDBObjectID', function (done) {
  • (function() {
  •  db.define('GoodModel', {_id: {type: db.ObjectID, id: true}})
    
  • }).should.not.throw();
  • (function() {
  •  db.define('BadModel', {_id: {type: String, id: true}})
    

Users are using this feature to support existing mongodb data. Does this
fix require removing support for this?

Reply to this email directly or view it on GitHubhttps://github.com/strongloop/loopback-connector-mongodb/pull/19/files#r9979359
.

@raymondfeng

The logic seems to be for getter, not setter. Please clarify.

@raymondfeng

I guess we can just disallow _id to be a property. The id property is mapped to _id internal anyway.

Owner

_id was exposed before, so i thought existing users might rely on it. I also asked Ritchie about this. If this is not an issue, it would simplify the PR, please confirm I remove this feature. _id would not be accessible anymore behind the loopback model, whatever the config/model definition is.

@raymondfeng

Setter or getter?

Contributor

I implemented a setter only for the case when a user specifies _id as being the loopback model id. In this case, at some point the default setter would do : instance[_id] = ObjectID(callbackValue), which throws an error because callbackValue is already an ObjectID. Custom setter could be avoided by modifying datasource-juggler by doing instead :

inst[propName] = (callbackValue instanceof DataType) ? callbackValue : DataType(callbackValue)
Contributor

I realize that the setter only address an issue about ObjectID typed _id, but breaks any other situation when a user specify another type. Hence the modification in my previous comment (in datasource-juggler) would be a lot better.

All of this is not needed if _id property in the loopback model is not needed.

Let me know what you think about it.

Contributor

Note : I sent a pull request to mongodb adding the self-convertibility of BSON type ObjectID and it was just merged (mongodb/js-bson#71), so my previous comment about ObjectID casting will soon be obsolete

Owner

inst[propName] = (callbackValue instanceof DataType) ? callbackValue : DataType(callbackValue)

Would you please create a PR for loopback-datasource-juggler? Thanks.

slnode commented Feb 26, 2014

All is well
Refer to this link for build results: http://ci.strongloop.com/job/loopback-connector-mongodb/152/

Contributor

Done the PR for loopback-datasoruce-juggler, and greatly simplified the current one by removing the model definition hooks, and refactoring tests to be more consistent with already existing one, and testing different kind of _id types. Also put back the version number.

slnode commented Feb 26, 2014

All is well
Refer to this link for build results: http://ci.strongloop.com/job/loopback-connector-mongodb/153/

slnode commented Feb 26, 2014

All is well
Refer to this link for build results: http://ci.strongloop.com/job/loopback-connector-mongodb/154/

@raymondfeng raymondfeng merged commit 040b683 into strongloop:master Feb 27, 2014

1 check passed

default Merged build finished.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment