Objection.js: sqlite ambiguous column name regression

[BrendanBall]

objection v1.6.2 introduces a breaking change resulting in sqlite throwing an error saying ambiguous column name. I've reproduced this with the reproduction template as simply changing the query as follows:

  const jennifer = await Person.query().debug()
    .joinEager()
    .findOne({ firstName: 'Jennifer' })
    .eager('pets')
    .orderBy('id', 'asc')

To make the code work with v1.6.2 you need to change .orderBy('id', 'asc') to .orderBy('person.id', 'asc').
v1.6.1 produces the following sql:

SELECT 
    `Person`.`id` AS `id`,
    `Person`.`parentId` AS `parentId`,
    `Person`.`firstName` AS `firstName`,
    `Person`.`lastName` AS `lastName`,
    `Person`.`age` AS `age`,
    `Person`.`address` AS `address`,
    `pets`.`id` AS `pets:id`,
    `pets`.`ownerId` AS `pets:ownerId`,
    `pets`.`name` AS `pets:name`,
    `pets`.`species` AS `pets:species`
FROM
    `Person`
        LEFT JOIN
    `Animal` AS `pets` ON `pets`.`ownerId` = `Person`.`id`
WHERE
    `firstName` = ?
ORDER BY `id` ASC

v1.6.2 produces the following sql:

SELECT 
    `Person`.`id`,
    `Person`.`parentId`,
    `Person`.`firstName`,
    `Person`.`lastName`,
    `Person`.`age`,
    `Person`.`address`,
    `pets`.`id` AS `pets:id`,
    `pets`.`ownerId` AS `pets:ownerId`,
    `pets`.`name` AS `pets:name`,
    `pets`.`species` AS `pets:species`
FROM
    `Person`
        LEFT JOIN
    `Animal` AS `pets` ON `pets`.`ownerId` = `Person`.`id`
WHERE
    `firstName` = ''
ORDER BY `person`.`id` ASC

Note this is only broken on sqlite, mysql does not complain about the sql produced by v1.6.2

[koskimas]

Oh crap! sqlite uses the aliases in order by. Didn't know about that.

Thanks for bringing this up. I'll add a test and fix this soon. In the meanwhile, stick with 1.6.1.

[koskimas]

But it's a good idea to always be explicit about your column references. It's difficult, even for people reading that code, to know which id this means.

[koskimas]

By the way, why are you calling both joinEager and eager? There's no reason to do that

[BrendanBall]

https://vincit.github.io/objection.js/api/query-builder/eager-methods.html#joineager
Shorthand for eagerAlgorithm(Model.JoinEagerAlgorithm).eager(expr)

[koskimas]

Yes, you just proved my point :smile:

  const jennifer = await Person.query().debug()
    .findOne({ firstName: 'Jennifer' })
    .joinEager('pets')
    .orderBy('id', 'asc')

is the same as

  const jennifer = await Person.query().debug()
    .findOne({ firstName: 'Jennifer' })
    .eagerAlgorithm(Model.JoinEagerAlgorithm)
    .eager('pets')
    .orderBy('id', 'asc')

but you wrote

  const jennifer = await Person.query().debug()
    .joinEager()
    .findOne({ firstName: 'Jennifer' })
    .eager('pets')
    .orderBy('id', 'asc')

which is the same as

  const jennifer = await Person.query().debug()
    .findOne({ firstName: 'Jennifer' })
    .eagerAlgorithm(Model.JoinEagerAlgorithm)
    .eager()
    .eager('pets')
    .orderBy('id', 'asc')

[BrendanBall]

Ah, the docs don't show that it takes arguments which is probably why I did that. Thanks for the tip. Just saw more of the docs now and mergeEager has examples which clarify that at least.

[koskimas]

Oh, that's true :confused: Sorry, I didn't realise that. I'll improve the documentation.

[shzmilydahlke]

Hi [email protected] breaks objection-find [email protected] in that a query like
const findQuery = require('objection-find');

router.get('/', (req, res) => {
findQuery(Campaign)
.build(req.query)
.then(allCampaigns => sendSuccess(res, allCampaigns))
.catch(err => sendError(res, err));
})
where the query param has a subquery as:
qs : {
'statusId:eq' : 1, // active
'sources.typeId' : 1, // <------ this breaks
'startDate:lt' : now,
'endDate:gt' : now,
'eager' : '[sources, priority]'
}
It seems to work fine with [email protected]
The error stack has :
TypeError: subQuery.build is not a function
at PropertyRef.buildFilter (/Users/milydahlke/dev/web-services/node_modules/objection-find/lib/PropertyRef.js:147:36)
at FindQueryBuilder._buildFilter (/Users/milydahlke/dev/web-services/node_modules/objection-find/lib/FindQueryBuilder.js:420:11)
at _.each.param (/Users/milydahlke/dev/web-services/node_modules/objection-find/lib/FindQueryBuilder.js:408:12)
at arrayEach (/Users/milydahlke/dev/web-services/node_modules/objection-find/node_modules/lodash/lodash.js:516:11)

[kibertoad]

@shzmilydahlke Support for latest Objection.js version was fixed in objection-find 2.0.0. Could you please try updating and report back if it works well for you?