Static type-checking for required properties in insertGraph()/upsertGraph()

[jtlapp]

Hey @mceachen, I'm posting this question as an issue because you don't seem to hang out on gitter.

insertGraph() takes a Partial<T>, which doesn't allow compile-time type-checking of required model properties. Is this the best we can do? I thought I'd point it out in case it's not.

Maybe it's this way to deal with readonly properties like id or createdAt/updatedAt? upsertGraph() complicates things by having an optional id.

[mceachen]

Not all fields are required, and we don't have stringent requirements to specify fields, so until we do, the Partial is the best we can do.

[jtlapp]

Right, I was hoping to impose static type-checking on fields that are known to be required. E.g., in the examples, 'name' is always a required field of a Movie model, but insertGraph() and upsertGraph() aren't going to tell me at compile time if I forgot it somewhere.

[mceachen]

Related comments: #709 (comment).

I'd be fine with requiring fields to be specified in model subclasses--it's certainly how I use Objection.

Is a vote in order? Thumbs up this comment if you are fine removing the Partial from the *Graph methods, and requiring Objection projects in TypeScript to have fully-specified fields.

@koskimas, I thought I remember seeing a comment that you were ok with this, but I can't find it.

[jtlapp]

I'm still thinking this through. Completely accurate type enforcement (not necessarily practical) would look something like the following (ignoring available inheritance):

type ModelRef = {id: number} | {'#ref': string} | {'#dbRef': number};
// *I'm not sure whether '#ref' and '#dbRef' are available to upsertGraph()

interface MovieSpec { // object passed to fromJson()
  name: string;
  directorId: number;
}

interface MovieMods { // object passed to patch()
  id?: number;
  name?: string;
  directorId?: number;
}

type MovieInsertGraphSpec = ModelRef | { // object passed to insertGraph()
  '#id'?: string;
  name: string;
  directorId: number;
  director?: PersonInsertGraphSpec
  actors?: PersonInsertGraphSpec[];
}

type MovieUpsertGraphSpec = MovieInsertGraphSpec | { // object passed to upsertGraph()
  'id'?: string;
  name?: string;
  directorId?: number;
  director?: PersonUpsertGraphSpec;
  actors?: PersonUpsertGraphSpec[];
}

class Movie { // object that objection returns
  id: number;
  name: string;
  directorId: number;
  director?: Person;
  actors?: Person[];
}

This is just a first approximation, but it suggests that inducing typescript to always restrict Objection parameters to exactly what is valid might be a bit complicated. It also suggests that the graph methods can't simply take models.

We further complicate things by making movie.id and perhaps movie.createdAt and movie.updatedAt readonly -- because movie.id would be readonly in Movie, but not in the patch and upserts, while createdAt and updatedAt might always be readonly.

[jtlapp]

We might want to look for something other than Partial<Model> anyway, because when using noImplicityAny, the following:

let pp: Partial<Person> = {};
pp['#id'] = 'hello';

Errors: "Element implicitly has an 'any' type because type 'Partial' has no index signature."

Would this be considered an error in the typings requiring correction?

(Maybe it's best to make these graphs of type any? Still thinking...)

[jtlapp]

The following works, but we need a special TS typedef trick to apply it:

type GraphModel = Partial<Model> & {
  '#id'?: string,
  '#ref'?: string,
  '#dbRef'?: number
};
let gm: GraphModel = {};
gm['#id'] = 'hello';

This would at least limit type-checks to the fields possible among all graphs, which is significantly better than any.

But to handle eager relation properties within a model, we'd need a TS trick for recursively redefining Model and Model[] to GraphModel and GraphModel[] -- unless we ask users to define a GraphModel alternative to each Model and supply that as a type parameter (yeesh!).

[jtlapp]

@koskimas, are there any glaring mistakes in my series of types, 3 comments above? I'd like to make sure I understand the type requirements before trying to find a solution.

[jtlapp]

Another possibility is to add the following to the Model type:

  '#id'?: string,
  '#ref'?: string,
  '#dbRef'?: number

Then models could serve as types in graphs. We may still need Partial<T> for insertGraph() because {id: 123}, {'#id': 'myID'}, etc., are each complete and valid models within the insert graph. I'm not sure how we'd otherwise do it as a disjunction of types.

UPDATE: This is problematic in eager relation properties. Given:

class Movie extends Model {
  readonly id: number;
  name: string;
  directorId: number;
  director?: Person;
}

Partial<Movie> doesn't change the director?: Person property, but this property needs to change to director?: Partial<Person>.

So again I'm left without any solution but any.

[jtlapp]

It's doable! I borrowed the technique found here. The following works!

type DeepPartial<T> = {
  [P in keyof T]: Partial<DeepPartial<T[P]>>;
};

class Model {
  '#id'?: string;
  '#ref'?: string;
  '#dbRef'?: number;
}

class Person extends Model {
  firstName: string;
  lastName: string;
}

class Movie extends Model {
  name: string;
  directorId: number;
  director?: Person;
}

type GraphMovie = DeepPartial<Partial<Movie>>;

const graph1: GraphMovie = {
  '#id': 'theMatrix',
  name: 'The Matrix'
};

const graph2: GraphMovie = {
  name: 'The Matrix',
  director: {
      firstName: 'Fred',
      notThere: 'bad bad bad' // errors as expected!!!
  }
};

UPDATE: This isn't working for eager relation properties that are arrays (e.g. actors?: Person[]). :-(