Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix foreign key = 0 not fetching related object #1639

merged 2 commits into from Jan 7, 2018


Copy link

@ollija ollija commented Sep 4, 2017

Fix foreign key = 0 not fetching related object


Fixes fetching relations with foreign key = 0.
Currently inside eagerPair-method the foreign keys are just checked for being falsy,
leaving the related instance out of the parent's relations if the FK is 0.

Fixes #1685.


This PR enables fetching relations with value 0 as the foreign key.

Proposed solution

Compare the groupedKey-variable (which stores the foreign key) through lodashes isNil-method inside the if-block, instead of just checking for a falsy value. This way 0 is a valid value, but null / undefined values are not.

src/relation.js Outdated
@@ -411,7 +411,7 @@ export default RelationBase.extend({
const formatted = model.format(_.clone(model.attributes));
groupedKey = formatted[keyColumn];
if (groupedKey) {
if (groupedKey !== null) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any possibility of groupedKey ever being undefined?

Copy link

@ollija Can you add some tests to check for this? I'm asking because there was already a much earlier issue exactly like this one that was deemed fixed (#49), but it must have regressed at some point. I would like to prevent that from happening again in the future.

Copy link

I just added a test for this in #1735.

@ricardograca ricardograca merged commit 83b6f65 into bookshelf:master Jan 7, 2018
ricardograca referenced this pull request Jan 7, 2018
Add test to check ability to eager load with foreign key with value 0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
None yet

Successfully merging this pull request may close these issues.

None yet

2 participants