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

Do not load belongsTo related model when FK is null #1563

Merged
merged 3 commits into from Jun 29, 2017

Conversation

Projects
None yet
5 participants
@fcarreiro
Contributor

fcarreiro commented May 2, 2017

(based on #1562)

The problem

As explained in #1299, there is an inconsistency on how eager load of related models when the foreign key is null. Take the following context:

A table books with id, and author_id; two rows

{ id: 1, author_id: 3 },
{ id: 2, author_id: null }

A model

var Book = bookshelf.Model.extend({
  tableName: 'books',
  author () {
    return this.belongsTo('Author');
  }
});

(and some author tables and models); then:

(1) If you do Book.where({id:2}).fetch({ withRelated: 'author' }) you will get

{
  id: 2,
  author_id: null
}

as expected.

(2) if you do Book.fetchAll({ withRelated: 'author' }) you will get

[{
  id: 1,
  author_id: 3,
  author: { author object }
},
{
  id: 2,
  author_id: null,
  author: { }
}]

Notice the {} in author.

(3) moreover, when doing the select the authors in (2), bookshelf will add the condition where IN [3,null] as reported in the issue.

The explanation

(1) works because of the condition here which correctly handles the null case. However, for collections (case 2) the parentFk attribute is undefined. I think that changing the line for

if (!relatedData.parentFk) return;

would not fix things, since we don't want that. The undefined comes, I think, from the this.model() here which seems to be a way for the collection to say "don't bother about the target".

(3) comes from this not expecting to possibly have null values.

(2) comes from creating an empty instance here for the null FK.

The solution

This pull request adds checks for (2) and (3) and also some tests.

fcarreiro added some commits May 1, 2017

@fcarreiro

This comment has been minimized.

Contributor

fcarreiro commented May 13, 2017

Hi guys @vellotis @kirrg001 could you give it a quick check to see if I miss some requirement for your PRs? :)

@jwickens

This comment has been minimized.

jwickens commented May 23, 2017

Hi this would be great to get merged

@Playrom Playrom added performance fixed and removed performance labels Jun 29, 2017

@Playrom

This comment has been minimized.

Contributor

Playrom commented Jun 29, 2017

Thanks for your work! :)

merged 👍

@Playrom Playrom merged commit e56aa0a into bookshelf:master Jun 29, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@pbassut

This comment has been minimized.

pbassut commented Feb 22, 2018

This is happening with hasOne too.
Lost a few hours on it :)

Thanks for the great work!

@ricardograca ricardograca added bug relations and removed fixed labels Feb 22, 2018

@ricardograca

This comment has been minimized.

Member

ricardograca commented Feb 22, 2018

@pbassut Can you open a new issue explaining the problem?

@pbassut

This comment has been minimized.

pbassut commented Feb 22, 2018

@ricardograca I was going to when I brought it up. Was in a hurry but wanted to post this as sort of a reminder.

@pbassut

This comment has been minimized.

pbassut commented Feb 22, 2018

Done: #1771

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment