hasAndBelongsToMany() with include syntax (again)

[mshick]

Sorry to open another ticket for this, but I've been stepping my way through the code and can confirm that this is not working as it should be.

lib/include.js seems to be invoking the adapter's "all" method with a mismatched model & filter when attempting to load relationships generated via hasAndBelongsToMany(). The issue seems to be that it is skipping relation table, and attempting to load the related items directly.

So, for instance, in the jugglingdb-rethink adapter I am using, when attempting:

Post = schema.define('Post', ...);
Post.hasAndBelongsToMany('assets');
Post.all({ include: ['assets'] }, callback);

The rethink adapter gets the following params passed to it:

RethinkDB.prototype.all = function all(model, filter, callback) {
// model = 'Asset'
// filter.where = { postId: 
//   { inq: 
//      [ '6a48cc99-dd5c-49e0-9e04-8d91b62eaa69',
//        '6dd1657d-e7b7-4de5-b1d5-8d1294157189' ] }

That filter.where obj is correct if run on the relation table, PostAsset but is invalid when run on the Asset table that is being provided.

When using the other approach to this, a standard relation, ie post.assets(err, results), the relation table is invoked, and my related assets are found as I'd expect.

This is basically a dupe issue of the one I opened here, but I'm not certain this is a bug and not my general ignorance: https://github.com/1602/jugglingdb/issues/358

I do hope I can come up with a fix and submit a PR for this...

[mshick]

I've really been pounding my head on this one. It seems like the details of the scope only ever live in the scoped method that gets dangled off of a vivified instance. For instance, post.assets in my example above.

Because the adapters are sending pure data and the model constructor, not instances, to the include function I don't see where I can grab this information, except perhaps by storing the important relationship details (params.through) on the constructor model, so that I can access them in include.js, and then load the appropriate "through" model, to finally get at the assets on the other end.

OR, perhaps AbstractClass.all could handle the includes in the this.schema.adapter.all callback, after it has vivified them? That removes placing the include logic / burden on adapter providers (it doesn't seem like any of the core adapters, other than memory, actually implement include relations).

I can try to take a stab at this, but I'm not sure I understand some of the philosophy / desired goals here. Perhaps there's a reason for obfuscating relation details? Maybe there's also a reason for implementing the include call in the adapter using data objects, and not instances?

Here are some relevant lines of code:

all method: https://github.com/1602/jugglingdb/blob/master/lib/model.js#L466-L507

memory implementation: https://github.com/1602/jugglingdb/blob/master/lib/adapters/memory.js#L149-L150

[1602]

I don't think there's a reason or philosophy, just human factor, because these two pieces was written in different time by different people, and refactored by third people. So, it doesn't work together. I think 'include' functionality just should be fixed to work well with many-to-many. If you fix it just make sure you added relevant test case to lib/relations.js

[mshick]

Gotcha, that's what it looked like to me...

Just submitted a PR here: https://github.com/1602/jugglingdb/pull/368

If that looks alright just hit me back and I'll work out a test. Would you prefer it in include.test.js or relations.test.js? Seems like the include.test might be a more natural home.

[odupuy]

Should it be closed as the PR is merged?