objection.js: Graph typings are incompatible with eager relation properties

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

About this issue

Original URL
State: closed
Created 6 years ago
Comments: 18 (10 by maintainers)

Commits related to this issue

Patch TS typings for graph methods (Fixes #724) Update TypeScript typings for `QueryBuilder#insertGraph` and `QueryBuilder#upsertGraph` methods to support nested eager relation properties. — committed to flipace/objection.js by yenbekbay 6 years ago

Most upvoted comments

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

yenbekbay on Apr 15, 2018

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.

jtlapp on Jan 16, 2018

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)

mceachen on Jan 16, 2018

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

koskimas on Jan 14, 2018

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 on Jan 13, 2018

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

koskimas on Jan 13, 2018

This should work:

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

  movies?: Partial<Movie>[];
}

koskimas on Jan 13, 2018