Avoid loading belongsTo relations with falsey keys attempt 3

[robertdp]

No description provided.

[jmdobry]

@techniq Can you merge and cut a release?

[techniq]

Sure

On Tue, Jan 19, 2016 at 1:47 PM Jason Dobry [email protected]
wrote:

@techniq https://github.com/techniq Can you merge and cut a release?

—
Reply to this email directly or view it on GitHub
#53 (comment).

[techniq]

@robertdp Care to throw a test around this?

[techniq]

Mixing arrow functions and non-arrow functions on the same line. Seems like we should be consistent

let ids = DSUtils.filter(items.map(item => DSUtils.get(item, def.localKey)), x => x)

This could also be even more concise by using ES5 functions instead of DSUtils/mout

let ids = items.map(item => DSUtils.get(item, def.localKey)).filter(x => x)

DSUtils.get() is still needed as it supports getting a nested property value (it uses mout under the hood)

[robertdp]

shrugs
It's the same call that was there before. If you want consistency then there are a lot of other changes that will have to be made as well.

[techniq]

ok, my bad. I was just doing a quick review before merging and saw this. I have plans to remove most of our use of DSUtils as ES5 functions serve our needs. Just leave it as is and I'll clean it up (along with the rest of the code base) later.

[robertdp]

If I get some time later, sure. I'm a bit swamped at the moment. No ETA though.

If there were any existing tests covering optional relations, then this problem would have been caught a while ago. I'm surprised it's only me that's been affected, unless this adapter isn't being used on complex apps...

I consider js-data-sql broken without this.

[techniq]

I'm using it on a fairly complex project (hence helping as one of the maintainers and adding some features, I have a few more coming down the pipe to fulfill some of my needs).

There are tests both under this project (https://github.com/js-data/js-data-sql/tree/develop/test) as well as under the shared js-data/js-data-adapter-tests project, which we try to move most tests up to to maintain consistently between the different adapters. I typically write tests in this project and then look to move them up to js-data-adapter-tests once they've been vetted.

[robertdp]

I've looked at the tests for this adapter, and it's not clear how to test this patch.

Testing relation loading would require executing a find() of findAll() and examining all the resulting queries' SQL. Is there any mechanism that allows this? Without it, tests can only catch errors, which are just indicative of part of the problem.

[robertdp]

In the unpatched loadWithRelations(), the belongsTo find() call was resulting in queries ending in WHERE id = '', whereas the findAll() call resulted in WHERE 1 = 0.

Only the first throws an error and it wrongly bubbles it to the top, so the original record you were querying for results in a Not Found! error, even if it has already been found.

The second doesn't throw an error because it's valid SQL and functionally the same as the query that was actually being made (WHERE id IN ()).

[techniq]

You can toString() the result of the filterQuery to get the executed SQL (see tests here - https://github.com/js-data/js-data-sql/blob/develop/test/filterQuery.spec.js), but this isn't available within loadWithRelations().

I'm going to go ahead and merge and cut a release for you and we'll deal with creating a test later. I created issue #54 to track the test.

If you do come up with a nice way to test this, just send it as a separate PR.

Thanks for your contribution.

[techniq]

@robertdp Deployed to NPM (and tagged in Git) as 0.11.8

[robertdp]

Thanks @techniq