Ensure Remote-Connector Converts Types in Array

[BerkeleyTrue]

Fixes strongloop/loopback#886

[slnode]

Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test."

[bajtos]

@slnode ok to test

[bajtos]

@BerkeleyTrue thank you for the contribution. Please add a unit-test verifying the fix.

@ritch you are more familiar with this code than I am, could you please review?

[bajtos]

@BerkeleyTrue we use 50/72 rule for commit messages, please reword your commit to use the imperative form (Ensure remote-connector..., Fix...)

[BerkeleyTrue]

Actually I think this is going to take more work than I thought. If I call the find method with an include belongsTo query, the data is stored in __cachedRelations.

For example. I have a model Mat with belongsTo Job and Job has a relationship belongsTo Client. My query looks like Mat.find({include: {job: 'client'}}). On callback, mats have the job properties, but it is a function that attempts to call the remote method mat.getJob, which is also broken.

I am not sure where to take it from here and would like some guidance who has a better understanding of what is going on during model building.

[raymondfeng]

Let me provide some insights. Taking the example of Job belongsTo Client. The Job instance will have a dynamic property called client. The value of client is a function. Related items are stored in __cachedRelations. For include to work, we need to store the included client object under __data (IIRC) so that the client property can be serialized into JSON. I believe toJSON() method on the job will produce a full plain JSON object.

[BerkeleyTrue]

Is client being a function intentional? If so, what was the reasoning? It seems intuitive to me to have client only available on the job object if the data is there. Otherwise the client property should not be there.

Or is the case that the client function looks under job.__data for the data?

[raymondfeng]

Yes, see http://docs.strongloop.com/display/LB/BelongsTo+relations#BelongsTorelations-Methodsaddedtothemodel. It follows RubyOnRails conventions to inject a few methods to support the relations.

[BerkeleyTrue]

Does that mean when I call mat.job() after an include query it should return the job object? Or does it expect a callback?

[raymondfeng]

mat.job(cb) should always work even without include.

[BerkeleyTrue]

Yes, but that is not very intuitive after an include for a belongsTo relationship. As a user, I expect mat.job to be the actual object(instance or json). If I have to further call another async function after an async function it makes it hard to work with models in views.

Why not have mat.job(cb) overwrite when the model with the belongsTo include query present.

Further it is intuitive to have an array present for hasMany include queries instead of a function.

[raymondfeng]

mat.toJSON() should give you a plain object that contains job.

[BerkeleyTrue]

I am guessing toJSON() removes prototype methods?

[raymondfeng]

yes

[BerkeleyTrue]

Why not instead have mat.getJob be the async function and mat.job be the property? This would be intuitive as you would expect met.getJob to be an async function and mat.job to be a property.

[bajtos]

Why not instead have mat.getJob be the async function and mat.job be the property?

Note that relations add extra functions on the getter function, e.g. user.accessTokens() and user.accessTokens.destroy(). The new API would have to cover them too.

Further it is intuitive to have an array present for hasMany include queries instead of a function.

I agree it is intuitive to have the array of related models present. The trouble is that you don't want to always fetch all related models on every request, as that adds unwanted performance penalty.

The current API allows you to do something like this:

Mat.findById(1, function(err, mat) {
  if (/* some condition */) {
    mat.job(function(err, job) {
      // process job
    });
  } else {
    // no need to fetch the job object from the DB
  }
});

---

The discussion about an API for relations is IMO off topic. Is there a way how to finish this patch while preserving the current API?

[BerkeleyTrue]

Yes, I am see your point. I still need to add test cases, but that will have to wait till the morning.

[BerkeleyTrue]

Tests Added

[bajtos]

@ritch ping, could you please review?

[ritch]

LGTM

Thanks for the patch!