strongloop / strong-remoting

Communicate between objects in servers, mobile apps, and other servers.
www.strongloop.com
Other
105 stars 93 forks source link

Client incorrectly coercing ObjectID #222

Closed ritch closed 9 years ago

ritch commented 9 years ago

See https://groups.google.com/forum/#!msg/loopbackjs/hYJVw8jy_PM/IwCS6901o0wJ

We should coerce the ObjectID into some intermediary on the client that preserves the type information.

pulkitsinghal commented 9 years ago

Hey @ritch, how do I +1 this on https://waffle.io/strongloop/loopback? I'd like to get this on your immediate queue. I'm blocked from releasing my product into production because of this.

pulkitsinghal commented 9 years ago

Is there some sort of a workaround maybe? Should I hack the model files manually so they explicitly get the right type assigned? How can i explicitly mark a field as ObjectId type via a json file? And how can I make sure that all the relational fields use ObjectId explicitly as well?

cc @raymondfeng

ritch commented 9 years ago

@raymondfeng might know a more clever workaround. I tend to use guid for my ID types to avoid the complexity of ObjectID entirely. But beware, there are reasons for ObjectID related to mongo's specific capabilities (IIRC replication).

ritch commented 9 years ago

@pulkitsinghal

This will use a node module to set the id type to a real ObjectID. This should work in the browser as well.

// in common/models/my-model.js
module.exports = function(MyModel) {
  var ObjectID = require('bson-objectid');

  MyModel.properties.id.type = ObjectID;

  // ...
}

Another simpler approach is to set the type on the client to string.

// in common/models/my-model.js
module.exports = function(MyModel) {
  // if the model is attached to the remote connector
  if(MyModel.dataSource.connector.name === 'loopback-connector-remote') {
    MyModel.properties.id.type = String;
  }
}

This second workaround highlights the actual issue. The mongodb connector adds some logic to handle ObjectIDs. The remote connector doesn't. The code above just avoids this issue by changing the default ID type (number) to string.

pulkitsinghal commented 9 years ago

changed 'loopback-connector-remote' to 'remote-connector'

pulkitsinghal commented 9 years ago

changed MyModel.properties.id.type = String; to: MyModel.definition.rawProperties.id.type = String; and then it worked.

But the related keys stay broken (NaN), how can I fix those?

{
  id: '55a1582e3a34bc98f74d51b4',
  userModelToReportModelId: NaN
}
pulkitsinghal commented 9 years ago

Using ObjectId has the same issue with NaN for related-models/foreignKeys:

var ObjectID = require('bson-objectid');
MyModel.definition.rawProperties.id.type = ObjectID;
...
{
  id: ObjectID(55a15a5a3a34bc98f74d51c4),
  userModelToReportModelId: NaN
}

And it also screws up the update calls:

{ [MongoError: unknown operator: $id]
  cause: 
   { [MongoError: unknown operator: $id]
     name: 'MongoError',
     status: 500,
     index: 0,
     code: 2,
     errmsg: 'unknown operator: $id' },
  isOperational: true,
  status: 500,
  index: 0,
  code: 2,
  errmsg: 'unknown operator: $id' }
pulkitsinghal commented 9 years ago

Setting the related model's id to String didn't help either, the foreignKey still ends up as NaN:

if(ReportModel.dataSource.connector.name === 'remote-connector') {
  ReportModel.definition.rawProperties.id.type = String;
  ReportModel.modelBuilder.models.UserModel.definition.rawProperties.id.type = String; // NaN for userModelToReportModelId
  //ReportModel.modelBuilder.definitions.UserModel.rawProperties.id.type = String; // same issue NaN for userModelToReportModelId
}
{
  "name": "ReportModel",
  "base": "PersistedModel",
  "relations": {
    "userModel": {
      "type": "belongsTo",
      "model": "UserModel",
      "foreignKey": "userModelToReportModelId"
    }
  }
}
pulkitsinghal commented 9 years ago

based on @doublemarked's recommendation, this workaround did it for me, but yeah would be great to get this really fixed :)

module.exports = function(ReportModel) {
  ReportModel.on('dataSourceAttached', function(obj){
    if(ReportModel.dataSource.connector.name === 'remote-connector') {
      ReportModel.definition.rawProperties.id.type = String;
      ReportModel.definition.rawProperties.userModelToReportModelId = {type: 'string'};
      ReportModel.definition.build(true);
    }
  }
}
ritch commented 9 years ago

Yeah you shouldn't have to do that.

The problem is that the behavior is in the mongodb connector. The remote connector doesn't auto set the id property to objectid.

What we need to do is figure out a way know that the model has an object ID property from the remote connector. Perhaps the mongo DB connector could write this metadata to the model settings.

ritch commented 9 years ago

@raymondfeng

ritch commented 9 years ago

@pulkitsinghal

You should be able to just define an id property in your model definition using the String type. If you set generated to true, the mongodb connector will still generate ObjectIDs. This has the side benefit of treating IDs as strings in your model.

id: {type: 'String', generated: true}

If you have already tried this and ran into issues, LMK as I would like this to be the documented way to use the remote connector with a mongodb connector on the server.

/cc @raymondfeng

pulkitsinghal commented 9 years ago

@ritch - Let me explain how I tested it and why I can't give this a 100% seal of approval but do think this is right~ish.

Usually I filter and publish only the models from my main project to NPM and then download them to my worker. But in this case I chose to edit the already downloaded models on the worker side (inside node_modules folder) for the experiment that you requested. This means anything that such a change may have done to compromise the integrity of my main project (which is now live thanks to your earlier workarounds) hasn't been tested or detected yet.

Long story short the worker was able to do its deed with what you suggested so go ahead and document it.

But I'm not so sure that the User model which is a relationship for me, honored the "id": {"type": "string", "generated": true} bit at all ... I get the sense that out-of-box or models that extend out-of-box models like User ... may crop up as problems down the line but I can't say because I'm not using relations exhaustively in my workers yet.

ritch commented 9 years ago

But I'm not so sure that the User model which is a relationship for me, honored the "id": {"type": "string", "generated": true} bit at all ...

Relations should use the type of the id property / and type information. If this isn't the case we should probably create a new issue in the juggler repo with some reproducable case.

Added the documentation here (under "Using with MongoDB connector"):

http://docs.strongloop.com/display/LB/Remote+connector