bookshelf / bookshelf

A simple Node.js ORM for PostgreSQL, MySQL and SQLite3 built on top of Knex.js
http://bookshelfjs.org
MIT License
6.36k stars 574 forks source link

hasOne only returns null if *all* records don't have a relation #1939

Open jwhitmarsh opened 5 years ago

jwhitmarsh commented 5 years ago

Issue Description

Following this PR change, we're expecting that all empty hasOne relations to be null, but it seems that that only applies if all instances of the result have an empty relationship. If even any result does have related data then the records with no relationship get set to empty object.

Steps to reproduce issue

Modify the existing test 'does not load "hasOne" relationship when it doesn\'t exist (site -> meta)' to fetch all Sites and run assertion against each record returned.

it('does not load "hasOne" relationship when it doesn\'t exist (site -> meta)', function() {
    return Site.fetchAll({
        withRelated: ['meta']
    }).then(function(site) {
        const sites = site.toJSON();
        for (const site of sites) {
            expect(site).to.not.have.property('meta');
        }
    });
});

Expected behaviour

The third record in the response should be { id: 3, name: 'backbonejs.org' } (no meta property)

Actual behaviour

[ { id: 1,
    name: 'knexjs.org',
    meta:
     { id: 1,
       site_id: 1,
       description: 'This is a description for the Knexjs Site' } },
  { id: 2,
    name: 'bookshelfjs.org',
    meta:
     { id: 2,
       site_id: 2,
       description: 'This is a description for the Bookshelfjs Site' } },
  { id: 3, name: 'backbonejs.org', meta: {} } ]

Just to confirm, I updated the fixtures to insert one more Site with the name 'backbonejs.org', then updated the fetch to get all records with that name, and this is the serialized response:

[ { id: 3, name: 'backbonejs.org' },
  { id: 4, name: 'backbonejs.org' } ]

Is this desired/expected behaviour so that all objects match? I understand that, but it is a bit confusing that fetches could return differently shaped objects. This might just be a documentation peice rather than an actual issue.

ricardograca commented 5 years ago

Is this desired/expected behaviour so that all objects match?

It isn't. It's supposed to be null if it doesn't exist irregardless of other models. Thanks for the report. Will look into this as time permits.