strongloop / strong-remoting

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

remoteObjects.invoke() destroys `included` related models #228

Closed simoami closed 8 years ago

simoami commented 9 years ago

I just discovered that if you invoke a rest method with remoteObjects.invoke(), the included related models are stripped. This is a side effect of converting the returned json into a model instance in HttpInvocation logic.

  1. A request is created and the response is transformed:
HttpInvocation.prototype.invoke = function(callback) {
  ...
  request(this.req, function(err, res, body) {
   ...
    self.transformResponse(res, body, callback); // <<<----- HERE
  });
};
model = new PersistedModel(res.body); 
  1. The transformer then converts json response into a model instance:
HttpInvocation.prototype.transformResponse = function(res, body, callback) {
  var callbackArgs = [null]; // null => placeholder for err
  val = res.body;
  ...
  dynamic = new Dynamic(val, ctx);
  val = dynamic.to(type); <<< ------ Conversion happens HERE
  ...
  callbackArgs.push(val);

  callback.apply(this, callbackArgs);
};
  1. Dynamic converts the dynamic value to the given type:
Dynamic.prototype.to = function(type) {
  var converter = this.constructor.getConverter(type);
  return converter(this.val, this.ctx); <<< ------ Conversion happens HERE
};
  1. This model converter is called:
// setup a remoting type converter for this model
    RemoteObjects.convert(typeName, function(val) {
      return val ? new ModelCtor(val) : val; <<< ----- the included property is finally LOST here
    });

Demonstration:

includes_filter_issue

raymondfeng commented 9 years ago

The included object is not a regular property of the model. The constructor probably should pass in a flag to ensure schema-less conversion.

simoami commented 9 years ago

I used RemoteObjects.invoke() because it was the only way I could run acl and before / after hooks for a given method. Any other ways you'd suggest?

raymondfeng commented 9 years ago

Model.checkAccess() has the logic to trigger ACLs.

simoami commented 9 years ago

hmm, I considered that path a week ago. if I am to to recode the whole logic for orchestrating calls (checkACL, call before hooks, call after hooks...) , my implementation could become vulnerable to future updates.

I see that the Model constructor has a strict property, which could potentially be used for retaining related properties.

what do you think?

simoami commented 9 years ago

or even better, have a flag to skip model transformation or keep source JSON in the model, like __sourceData.

simoami commented 9 years ago

Update: I see that the relations are not exactly lost. They're being stored in model.__cachedRelations.

I think I'd just merge that with __data into a new json object.

richardpringle commented 8 years ago

@simoami Looks like you solved the issue. Anyway, I think if any related issues come up they should go to LoopBack since it involves both the Model Class as well as strong-remoting.

simoami commented 8 years ago

@richardpringle I did not solve the issue. I just helped figure this out and offering ways to solve it. It has been some time and I can't remember if I ended up using __cachedRelations. In any case, fine to close for now.

richardpringle commented 8 years ago

@simoami, sorry to hear that it didn't get completely solved. As I said, better to open in LoopBack next time. Can always mention me directly to try and get the issue solved a little faster too.