objection.js icon indicating copy to clipboard operation
objection.js copied to clipboard

Incorrect typing of `idColumn`

Open jackbonaguro opened this issue 1 year ago • 1 comments

According to documentation, idColumn should be set to null in the event a table does not have an id column: https://vincit.github.io/objection.js/api/model/static-properties.html#static-idcolumn

But due to the typings, null is not a possible valid value for idColumn - only string | string[]: https://github.com/Vincit/objection.js/blob/d1e194a3ba58019a90579464459473e26668a4ab/typings/objection/index.d.ts#L1541

This causes an issue in my code on version 3.1.4. Doing either:

static idColumn = null;

or

static get idColumn() {
    return null;
}

Results in a compiler error:

  Types of property 'idColumn' are incompatible.
    Type 'null' is not assignable to type 'string | string[]```

Only by casting like `static idColumn = null as unknown as string;` can I work around the issue. Not setting idColumn causes runtime errors.

jackbonaguro avatar Sep 04 '24 17:09 jackbonaguro

Thanks for pointing this out @jackbonaguro. I'm happy to accept a PR that fixes this.

lehni avatar Sep 25 '24 11:09 lehni

maybe it's better not to null, but never?

static get idColumn(): never {
    throw new Error(`undefined idColumn`);
  }

@lehni

after all, if there is no idColumn, then there should be no access to it at all and this should generate an error at runtime

related issue https://github.com/Vincit/objection.js/issues/2745

by the way, failed behavior:

https://github.com/Vincit/objection.js/blob/main/lib/queryBuilder/operations/InsertOperation.js#L42

if (!isSqlite(builder.knex()) && !isMySql(builder.knex()) && !builder.has(/returning/)) {
      // If the user hasn't specified a `returning` clause, we make sure
      // that at least the identifier is returned.
      knexBuilder = knexBuilder.returning(builder.modelClass().getIdColumn());
    }

if you don't specify returning in the query, then idColumn will be substituted ( => knexBuilder.returning(null);), but if it is null, then an error will occur at the query execution level... it is better if it is at the query compilation level. so: https://github.com/Vincit/objection.js/pull/2746/files

salisbury-espinosa avatar Feb 26 '25 03:02 salisbury-espinosa