Graph typings are incompatible with eager relation properties

[jtlapp]

UPDATE: This is actually a Typescript-specific usage problem whose solution is not currently modeled in the examples.

The typings for insertGraph() and upsertGraph() don't allow graphs that include eager relation properties.

The problem is that these methods take Partial<T extends Model> as input parameters, but this only makes the eager relation properties optional and not also their properties, recursively. In particular, they expect an eager relation property to have all the methods of Objection.Model.

Here are some examples and their errors:

import * as Objection from 'objection';

class Person extends Objection.Model {
  id?: number;
  firstName: string;
  lastName: string;

  // Commenting the following out doesn't help, as then movies wouldn't be valid in Partial<T>
  movies?: Movie[];
}

class Movie extends Objection.Model {
  id?: number
  name: string;
}

Person
  .query()
  .insertGraph({
    firstName: 'Jennifer',
    lastName: 'Lawrence',

    movies: [{ // ERROR: missing all of Model's properties
      // id: 32 (adding this wouldn't help)
      name: 'American Hustle'
    }]
  });

Person
  .query()
  .insertGraph({
    firstName: 'Jennifer',
    lastName: 'Lawrence',

    movies: [{ // ERROR: missing all of Model's properties
      id: 32
      // name: 'American Hustle' (adding this wouldn't help)
    }]
  }, {
    relate: true
  });

Person
  .query()
  .insertGraph([{
    firstName: 'Jennifer',
    lastName: 'Lawrence',

    movies: [{ // ERROR: does not match movies property
      "#id": 'silverLiningsPlaybook'
      name: 'Silver Linings Playbook'
    }]
  }, {
    firstName: 'Bradley',
    lastName: 'Cooper',

    movies: [{ // ERROR: does not match movies property
      "#ref": 'silverLiningsPlaybook'
    }]
  }]);

Person
  .query()
  .upsertGraph({
    id: 1,
    firstName: 'Jonnifer',

    movies: [{ // ERROR: missing all of Model's properties
      id: 1
      //name: 'American Hustle' (adding this wouldn't help)
    }, { // ERROR: missing all of Model's properties
      id: 1253
      // name: 'Passengers' (adding this wouldn't help)
    }]
  });

I've been exploring ways to improve these typings in #714, but have not found anything that works other than the any type.

I looked at adding [key: string]: any to the Partial<T> parameter of insertGraph() and upsertGraph(), but then if the model ever defines an eager relation property, TS expect that property to match the model definition, not [key: string]: any. However, this approach works fine if the models simply do not define types for their eager relation properties.

cc @mceachen

[jtlapp]

I'll shortly be exploring ways to capture graphs in domain-specific interfaces, so there's a chance I might discover a way to get some static type checking. Meantime, if you can think of anything better than any, I'd love to hear it.

[koskimas]

This should work:

class Person extends Objection.Model {
  id?: number;
  firstName: string;
  lastName: string;

  movies?: Partial<Movie>[];
}

[jtlapp]

Oh cool! Just need a small addition to make the errors go away:

declare module 'objection' {
    export interface Model {
        '#id'?: string;
        '#ref'?: string;
        '#dbRef'?: number;
    }
}

But there is a serious drawback for eager loading: TS will require the app to check (or bang-override) the presence of every single model property.

[jtlapp]

Or do this to avoid adding graph model ref properties to Model:

type EagerModel<T extends Objection.Model> = Partial<T> & {
  '#id'?: string;
  '#ref'?: string;
  '#dbRef'?: number;
};

class Person extends Objection.Model {
  id?: number;
  firstName: string;
  lastName: string;
  movies?: EagerModel<Movie>[];
}

But this is still potentially annoying for eager loading.

[koskimas]

But there is a serious drawback for eager loading: TS will require the app to check (or bang-override) the presence of every single model property.

What do you mean by that?

You can also create a base class and define the #ref etc. properties there and then inherit your models from that. Those are not added to Model because you can alter them using the refProp, dbRefProp etc. Model fields.

[koskimas]

But this is still potentially annoying for eager loading.

I think Partial is the correct way to type the relations since you can easily select only a subset of properties in eager queries (and I for one very often do).

[jtlapp]

I think Partial is the correct way to type the relations since you can easily select only a subset of properties in eager queries (and I for one very often do).

Ah perfect!

You can also create a base class and define the #ref etc. properties there and then inherit your models from that. Those are not added to Model because you can alter them using the refProp, dbRefProp etc. Model fields.

I missed that. So the app would have to either add them to its models or define something like EagerModel<T>, in order to use insertGraph() and upsertGraph().

[koskimas]

If you use #id, #ref or #dbRef. Very few people actually do.

[jtlapp]

I guess none of these changes are actually needed for the express-ts example, because it only ever receives any input and returns any output, avoiding type-checking.

They should probably be in some example though, maybe even express-ts to show best practices (or even how to do it without casting to anyfirst).

[jtlapp]

How are you restricting the columns returned in eager queries? The relation expression only seems to filter for eager properties (whole models).

[jtlapp]

I guess this would be more accurate:

type EagerModel<T extends Objection.Model> =
  {'#id'?: string;} & Partial<T> |
  {'#ref': string;} |
  {'#dbRef': number;};

UPDATE: This works fine on insert or upsert, but eager loads are expecting every eagerly-loaded model to match each of these model alternatives, which is impossible. I guess I'm stuck with the prior version of EagerModel, which I now call EagerRelation in my code (I'm also using it on interfaces used to define models).

[jtlapp]

How are you restricting the columns returned in eager queries? The relation expression only seems to filter for eager properties (whole models).

I got my answer: pick() and omit(). Okay, I'm convinced we have a solution.

Only question is, should we revise the express-tsexample? I'll be making a DI-capable example available, but I don't know if @koskimas will want it checked in.

[koskimas]

You need to ask @mceachen about changing the example project.

[mceachen]

As I've said in other comments, I think there's value in keeping the express-* trees consistent, so the only difference between express-es7 and express-ts is what is required by TypeScript to compile in strict mode.

IOW, @koskimas, is it OK to add pick and omit examples to all the express-* APIs? (It seems reasonable to me)

[jtlapp]

I spent a huge amount of time trying to figure out how to handle eager relations within Typescript because there were no examples of properly typed eager relations. The only example was of a Person-type property, which I copied by adding Animal and Movies, but which we've now decided should be Partial<Person>, etc., for non-cyclic graphs.

So the question is whether to have an example that'll save others the same headache. At a minimum, we'd need to change the relation types to partials. A more thorough example would demo model references in cyclic graphs.

[yenbekbay]

With Conditional types introduced in TypeScript 2.8, we can update typings as follows:

type GraphModel<T> =
  | ({'#id'?: string; '#ref'?: never; '#dbRef'?: never} & T)
  | ({'#id'?: never; '#ref': string; '#dbRef'?: never} & {
      [P in keyof T]?: never
    })
  | ({'#id'?: never; '#ref'?: never; '#dbRef': number} & {
      [P in keyof T]?: never
    });

type NonFunctionPropertyNames<T> = {
  [K in keyof T]: T[K] extends Function ? never : K
}[keyof T];

interface DeepPartialGraphArray<T> extends Array<DeepPartialGraph<T>> {}

type DeepPartialGraphModel<T> =
  | GraphModel<{[P in NonFunctionPropertyNames<T>]?: DeepPartialGraph<T[P]>}>
  | Partial<T>;

type DeepPartialGraph<T> =
  T extends (any[] | ReadonlyArray<any>) ? DeepPartialGraphArray<T[number]> :
  T extends Model ? DeepPartialGraphModel<T> :
  T;

interface InsertGraph<QM extends Model> {
  (
    modelsOrObjects?: DeepPartialGraph<QM>[],
    options?: InsertGraphOptions,
  ): QueryBuilder<QM, QM[]>;
  (
    modelOrObject?: DeepPartialGraph<QM>,
    options?: InsertGraphOptions,
  ): QueryBuilder<QM, QM>;
  (): this;
}

Then there's no need to type model's relation properties with Partial.

Would you be willing to accept a PR with this change? @koskimas

[thunderball1]

Ping @koskimas

[mceachen]

@yenbekbay your types LGTM, thanks for taking the time to do that. A PR would be great!