Objection.js: QueryBuilder.relate() and .patch() yield the wrong type

[jtlapp]

The typings have the relate() method of QueryBuilder returning type this and thus yielding a Model. However, relate() actually yields either a numberor an object pairing IDs to their corresponding reference fields. I don't know what the result is when providing multiple relations.

UPDATE: QueryBuilder patch() has the same problem -- should yield a number.

cc @mceachen

[mceachen]

relate() actually yields either a number (an ID) or an object pairing IDs to their corresponding reference fields

What leads you to that assertion? According to the docs, it should return a Promise to the number of related rows, not IDs.

@koskimas should relate return something like QueryBuilderUpdate<this> so the async return value is numeric?

[jtlapp]

Ah, okay that may be correct about the number of related rows -- I was trying to infer that. The express-ts example returns the following, which is the only one I've tested:

{
  movieId: silverLinings.id,
  personId: bradley.id
}

UPDATE: That's what my koa-ts spinoff is doing. Let me confirm express-ts.

[jtlapp]

Yeah, express-ts returns the same thing. The above code is from my mocha test suite. Here's the raw output:

{"movieId":1,"personId":2}

[jtlapp]

Is the output of relate() ever informative? In the above case, movieId and personId are actually handed to objection, which spits it back out.

[jtlapp]

QueryBuilder patch() has the same problem, yielding a model type instead of a number (the number of modified rows).

(Oddly, VSCode indicates that patch() yields a Model in its popup code hints, but actually requires it to be a Model[] for purposes of matching the return value in an interface's method signature. Weird. Appending .first() resolves the discrepancy, even though the runtime value is just a number.)

NOTE: I do need the patch count to know whether the target existed and the patch succeeded. When it doesn't exist, the query succeeds and return a count of zero.

[koskimas]

@jtlapp patch typings do return a number. There's even this test in the test file for it:

const rowsPatched: Promise<number> = qb.patch({});

[koskimas]

relate return value depends on the actual operation. For HasMany and BelongsToOne relations relate is an update operation and the number of affected rows is returned. For ManyToManyRelation relate is an insert, and in that case the inserted model is returned (just like with insert). We could return a hardcoded 1 in that case too, but that would be less than useless. By returning the inserted model, you can do this .relate(foo).returning('*') and get back whatever the database did for the inserted row (default values, generated values etc.).

[koskimas]

It seems that Where<T> and friends should possibly be updated. This works:

const numUpdated: number = await Person.query().where('id', '<', 100).patch(obj);

but this doesn't

const numUpdated: number = await Person.query().patch(obj).where('id', '<', 100);

Where methods return QueryBuilder<T> when they probably should return this.

[koskimas]

I think the whole QueryBuilder type jungle could be refactored so that there is only one QueryBuilder type with two template arguments QueryBuilder<M, R> where M is the model type and R is the return type. This way both types could be passed to Where<M, R> etc. and the return type would persist through other method calls. The model type needs to be carried too because returning (and maybe others) method turns the return type back from number to M[] or M and first returns the return type from R[] to R.

Actually we would probably need three arguments:

1. M model type
2. SR "single" return type
3. R the actual return type

maybe that would be too big of a mess...

[koskimas]

That way we could maybe get rid of the million query builder types. Also why is there a QueryInterface interface when only QueryBuilderBase implements that? Couldn't we just move the methods over to QueryBuilderBase.

[koskimas]

Actually it would be even nastier, but this works in almost all cases:

interface SingleOrNot<M> {
  single: M;
  array: M[]
}

interface QueryBuilder<M, R, S extends keyof SingleOrNot<R>> extends Promise<SingleOrNot<R>[S]> {
  where(): QueryBuilder<M, R, S>;
  first(): QueryBuilder<M, R, 'single'>;
  returning(): QueryBuilder<M, M, 'array'>
  patch(): QueryBuilder<M, number, 'single'>
}

class Model {

}

let qb: QueryBuilder<Model, Model, 'array'>;

const x1: Promise<Model[]> = qb.where();
const x2: Promise<Model> = qb.where().first();
const x3: Promise<Model> = qb.first().where();
const x4: Promise<number> = qb.patch().where();
const x5: Promise<number> = qb.where().patch();
const x6: Promise<Model[]> = qb.where().patch().returning();
const x7: Promise<Model[]> = qb.patch().where().returning();
const x8: Promise<Model> = qb.patch().where().returning().first();
const x9: Promise<Model> = qb.patch().returning().first().where();

@mceachen What do you think? This would need to be refined some more, but maybe something to consider?

[koskimas]

No actually this would probably be enough. I don't remember why I thought the SingleOrNot was necessary.

interface QueryBuilder<M, SR, R> extends Promise<R> {
  where(): QueryBuilder<M, SR, R>;
  first(): QueryBuilder<M, SR, SR>;
  returning(): QueryBuilder<M, M, M[]>
  patch(): QueryBuilder<M, number, number>
}

class Model {

}

let qb: QueryBuilder<Model, Model, Model[]>;

const x1: Promise<Model[]> = qb.where();
const x2: Promise<Model> = qb.where().first();
const x3: Promise<Model> = qb.first().where();
const x4: Promise<number> = qb.patch().where();
const x5: Promise<number> = qb.where().patch();
const x6: Promise<Model[]> = qb.where().patch().returning();
const x7: Promise<Model[]> = qb.patch().where().returning();
const x8: Promise<Model> = qb.patch().where().returning().first();
const x9: Promise<Model> = qb.patch().returning().first().where();

[jtlapp]

patch typings do return a number. There's even this test in the test file for i

Yes, I'm sorry. Here's my problem code:

    return this.Model.query().patch(mods).where('id', personID).then((patchCount: any) => {
      return (patchCount === 1); // TBD: remove 'any' type when patch type gets fixed
    });

[jtlapp]

Despite QueryBuilder taking more parameters in the above, the typings may be easier to understand! I typically look to typedefs to help me understand what something is or does, in first pass, but I don't find these typedefs very helpful in that regard. Anything for more clarity!

[koskimas]

For now you can work around this by adding the patch or relate last. relate typings cannot be fixed, an you need to use a cast.

[jtlapp]

The following tests all fail under the current typings. I have revised typings that makes them all pass, using a variation of @koskimas' above proposal. I'm just working on two more problems: query builder callback types and custom query builder types.

const findByIdSelect: Promise<Person> = Person.query().findById(32).select('firstName').throwIfNotFound();
const findByIdJoin: Promise<Person> = Person.query().findById(32).join('tablename', 'column1', '=', 'column2').throwIfNotFound();
const findByIdJoinRaw: Promise<Person> = Person.query().findById(32).joinRaw('raw sql').throwIfNotFound();
const findOneWhere: Promise<Person> = Person.query().findOne('raw sql').where('more raw sql').throwIfNotFound(); // JTL
const findOneSelect: Promise<Person> = Person.query().findOne('raw sql').select('firstName').throwIfNotFound(); // JTL
const findOneWhereIn: Promise<Person> = Person.query().findOne('raw sql').whereIn('status', ['active', 'pending']).throwIfNotFound(); // JTL
const findOneWhereJson: Promise<Person> = Person.query().findOne('raw sql').whereJsonSupersetOf('x:y', 'abc').throwIfNotFound(); // JTL
const findOneWhereJsonIsArray: Promise<Person> = Person.query().findOne('raw sql').whereJsonIsArray('x:y').throwIfNotFound(); // JTL
const patchWhere: Promise<number> = Person.query().patch({firstName: 'Mo'}).where('id', 32); // JTL
const patchWhereIn: Promise<number> = Person.query().patch({firstName: 'Mo'}).whereIn('id', [1, 2, 3]); // JTL
const patchWhereJson: Promise<number> = Person.query().patch({firstName: 'Mo'}).whereJsonSupersetOf('x:y', 'abc'); // JTL
const patchWhereJsonIsArray: Promise<number> = Person.query().patch({firstName: 'Mo'}).whereJsonIsArray('x:y'); // JTL
const updateWhere: Promise<number> = Person.query().update({firstName: 'Mo'}).where('id', 32); // JTL;
const updateWhereIn: Promise<number> = Person.query().update({firstName: 'Mo'}).whereIn('id', [1, 2, 3]); // JTL;
const updateWhereJson: Promise<number> = Person.query().update({firstName: 'Mo'}).whereJsonSupersetOf('x:y', 'abc'); // JTL;
const updateWhereJsonIsArray: Promise<number> = Person.query().update({firstName: 'Mo'}).whereJsonIsArray('x:y'); // JTL;
const deleteWhere: Promise<number> = Person.query().delete().where('raw sql');
const deleteWhereIn: Promise<number> = Person.query().delete().whereIn('id', [1, 2, 3]);

[koskimas]

Query callbacks that don't need to return anything:

• modifyEager
• where callbacks
• select callbacks
• from callbacks

Actually all subquery or where grouping callbacks don't need to return anything and the return value and query builder return type are irrelevant.

If there are other callbacks, ask.

[koskimas]

By the way, have you talked this refactoring through with @mceachen? He's the supreme overlord of typing related issues. I don't feel I can merge any large pull request regarding typings since mceachen has written 99% of them.

[koskimas]

Maybe a separate issue should be opened for this

[jtlapp]

By the way, have you talked this refactoring through with @mceachen?

I hear from him once every day or two, which isn't enough interaction for me to get this done. So I'm just doing it and he can look it over.

[mceachen]

Yeah, turns out I'm doing this on my own time (like @koskimas). I pinged you on gitter, but send me an email and we can schedule a time together to share screen and bang out your issue list.

[mceachen]

Resolved by #744 and #753

[jtlapp]

Given the wide variety of return types available to .relate(), I wonder if it should return type any.

For example, the typings might have it returning Person[] despite needing to be a number. Typescript won't allow typecasting Person[] (or Person) to number. We have to do this instead:

const numberOut: Promise<number> = <Promise<number>><Promise<any>>qb.relate();