Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Incorrect typing of idColumn #2693

Open
jackbonaguro opened this issue Sep 4, 2024 · 2 comments · May be fixed by #2744
Open

Incorrect typing of idColumn #2693

jackbonaguro opened this issue Sep 4, 2024 · 2 comments · May be fixed by #2744

Comments

@jackbonaguro
Copy link

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[]:

objection.js/typings/objection/index.d.ts

Line 1541 in d1e194a

static idColumn: string | string[];

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.
@lehni
Copy link
Collaborator

lehni commented Sep 25, 2024

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

This was referenced Feb 24, 2025
Closed
Open
@salisbury-espinosa
Copy link

salisbury-espinosa commented Feb 26, 2025 •
edited
Loading

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 #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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix idColumn type geeksilva97/objection.js
Update index.d.ts geeksilva97/objection.js
3 participants
@lehni @jackbonaguro @salisbury-espinosa