Objection.js: Subquery errors in objection v1.0.0

[jeff-kilbride]

I have several reporting queries that use subqueries. Here is a basic example of a static model method using a subquery:

    static getTrafficDailyHits(accountId, start, end) {
        const subquery = AggAffHit.query()
            .alias('h')
            .select('h.date', 'h.raw', 'h.unique')
            .joinRelation('site', { alias: 's' })
            .where('s.account_id', accountId)
            .andWhere('h.date', '>=', start)
            .andWhere('h.date', '<', end)
            .as('d');

        return AggAffHit.query()
            .select(
                'c.c_date as date',
                AggAffHit.raw('ifnull(sum(`d`.`raw`), 0) as `raw`'),
                AggAffHit.raw('ifnull(sum(`d`.`unique`), 0) as `unique`')
            )
            .from('calendar as c')
            .leftJoin(subquery, 'c.c_date', 'd.date')
            .where('c.c_date', '>=', start)
            .andWhere('c.c_date', '<', end)
            .groupBy('c.c_date');
    }

This works in v0.9.4 and produces the following SQL:

select `c`.`c_date` as `date`, 
  ifnull(sum(`d`.`raw`), 0) as `raw`, 
  ifnull(sum(`d`.`unique`), 0) as `unique` 
from `calendar` as `c` left join (
  select `h`.`date`, 
    `h`.`raw`, 
    `h`.`unique` 
  from `agg_aff_hit` as `h` 
    inner join `sites` as `s` on `s`.`id` = `h`.`site_id` 
  where `s`.`account_id` = 3 
    and `h`.`date` >= '2018-03-16' 
    and`h`.`date` < '2018-03-17'
) as `d` on `c`.`c_date` = `d`.`date` 
where `c`.`c_date` >= '2018-03-16' 
  and `c`.`c_date` < '2018-03-17' 
group by `c`.`c_date`

However, in v1.0.0, it seems that the .from() method in the main query is bleeding into the subquery. Here is the SQL from v1.0.0, which produces an error:

select `c`.`c_date` as `date`, 
  ifnull(sum(`d`.`raw`), 0) as `raw`, 
  ifnull(sum(`d`.`unique`), 0) as `unique` 
from `calendar` as `c` left join (
  select `h`.`date`, 
    `h`.`raw`, 
    `h`.`unique` 
  from `calendar` as `h` 
    inner join `sites` as `s` on `s`.`id` = `h`.`site_id` 
  where `s`.`account_id` = 3 
    and `h`.`date` >= '2018-03-16' 
    and `h`.`date` < '2018-03-17'
) as `d` on `c`.`c_date` = `d`.`date` 
where `c`.`c_date` >= '2018-03-16' 
  and `c`.`c_date` < '2018-03-17' 
group by `c`.`c_date` 

ER_BAD_FIELD_ERROR: Unknown column 'h.date' in 'field list'

Notice in v0.9.4, the table in the from clause in the subquery (which is the correct table name for the AggAffHit model):

from `agg_aff_hit` as `h`

versus the same clause in v1.0.0 (which seems to be using the table in the from() method in the main query):

from `calendar` as `h` 

I'm not sure if this is a bug or intended as a result of the refactoring done in v1.0.0.

[koskimas]

Why are you using the AggAffHit model to query calendar table? Could you use this instead

    static getTrafficDailyHits(accountId, start, end) {
        const subquery = AggAffHit.query()
            .alias('h')
            .select('h.date', 'h.raw', 'h.unique')
            .joinRelation('site', { alias: 's' })
            .where('s.account_id', accountId)
            .andWhere('h.date', '>=', start)
            .andWhere('h.date', '<', end)
            .as('d');

        return Calendar.query()
            .select(
                'c.c_date as date',
                Calendar.raw('ifnull(sum(`d`.`raw`), 0) as `raw`'),
                Calendar.raw('ifnull(sum(`d`.`unique`), 0) as `unique`')
            )
            .alias('c')
            .leftJoin(subquery, 'c.c_date', 'd.date')
            .where('c.c_date', '>=', start)
            .andWhere('c.c_date', '<', end)
            .groupBy('c.c_date')
            .castTo(AggAffHit)
    }

[koskimas]

Also moving the from call after leftJoin could do the trick.

[jeff-kilbride]

No real reason other than that the calendar table is a one-field lookup table containing date strings and didn't feel like it needed it's own model. It's only purpose is to prevent holes in the data with the left join. Also, this particular method is inside the AggAffHit model, so I tend to use the containing model for .query(), .raw(), etc... calls. And, of course, up to v0.9.4 it just worked!

I tried a quick and dirty re-ordering, putting the from call after leftJoin, and it didn't work. I wouldn't want to rely on the order of method calls, anyway. It feels too easy to get wrong.

Adding a simple Calendar model does the trick:

/**
 * Calendar.js
 *
 * @description :: Model for the calendar table.
 */

'use strict';

class Calendar extends Model {
    static get tableName() {
        return 'calendar';
    }
}

module.exports = Calendar;

Using it as in your example produces the same SQL as v0.9.4, so it's a simple enough change. However, I think I'm going to have to look at every instance of how I use .from(). In the past, in certain cases, I've used Model.query().from() more like a generic knex instance (as in the original example). If the Model is more tightly coupled to changes of the table name, now, I may have to rethink some of it.

The change is understandable, though, and probably simplifies the objection code. I'm wondering if it diminishes the usefulness of the .from() method to the point where it's not needed anymore. Maybe we shouldn't be able to change Model tables on the fly like this. Prior to having the .alias() method, I used .from() a lot for table name aliasing, e.g. AggAffHit.query().from('agg_aff_hit as h') -- but, obviously, that's not necessary anymore.

The .from() method makes sense for knex, where there are no defined Models with associated table names. Are there legitimate use cases in objection for changing the name of the table being used by a particular model? Or does it add unneeded complexity?

[koskimas]

Are there legitimate use cases in objection for changing the name of the table being used by a particular model?

Views for example.

[jeff-kilbride]

Ok. I'm not using views with mysql, so I don't know what that would look like. For my code, I have some fairly complex UNION ALL queries that are using .from(). For example:

// Create a UNION ALL of agg_aff_trans and agg_aff_hit as a derived table.
const derived = AggAffTrans.query()
    .select(
        'site_id as id',
        'type',
        'count',
        'amount',
        AggAffTrans.raw('0 as `raw`'),
        AggAffTrans.raw('0 as `unique`')
    )
    .where('affiliate_id', accountId)
    .andWhere('date', '>=', params.start)
    .andWhere('date', '<', params.end)
    .unionAll(function() {
        this.from('agg_aff_hit')
            .select(
                'site_id',
                AggAffTrans.raw('"I"'),
                AggAffTrans.raw('0'),
                AggAffTrans.raw('0'),
                'raw',
                'unique'
            )
            .where('affiliate_id', accountId)
            .andWhere('date', '>=', params.start)
            .andWhere('date', '<', params.end)
            .andWhere('landing_page', '!=', 2); // exclude 'Payment Gateway'
    })
    .as('d');

So, I'm thinking I should change this.from('agg_aff_hit').select() to just AggAffHit.select(). I think I was under the impression that I needed to use this inside the callback.

Anyway, I see that you've closed this issue, so I won't add anymore. I'll do some refactoring and let you know if I run into any problems.

[koskimas]

I'm reopening this since it seems like there's something that we can do for this without breaking too much existing code. I'll post here when I know more. The changes could break some existing code, so any changes need to wait for 2.0. You need to refactor your code anyway.

It should be ok to use this the way you do in subqueries. The query builder created that way is a lower level wrapper, without any knowlege of models or relations. You can also use the first argument passed to that function if you want to use arrow functions instead. If you use a separate query builder, you should leave out the function.

[jeff-kilbride]

I've already refactored everything and have it all working with v1.0.0 now. 😃

For the .unionAll() stuff, I was stuck thinking I had to use a callback based on the example in the knex docs. After looking through some of my other code, I realized I could just pass an objection object directly instead. So:

.unionAll(
    AggAffHit.query()
        .select(...)
)

which is simpler and works great, and I don't have to worry about the .from() method.

Any improvements to make it more flexible are good. However, I didn't find the changes I needed to make very difficult, so I wouldn't consider it a priority. Going forward, I will probably avoid using .from(). Creating a simple model, even for lookup tables, feels cleaner and easier to understand.

[koskimas]

Nice to hear that the update wasn't a major PITA!

The fact that the table is inherited by subqueries feels like a bug, and I'll see if that can be changed back without breaking something else.