Type safe creation of models

[RobinBuschmann]

Pull Request check-list

Please make sure to review and check all of these items:

• Does npm run test or npm run test-DIALECT pass with this change (including linting)?
• Does the description below contain a link to an existing issue (Closes #[issue]) or a description of the issue you are solving?
• Have you added new tests to prevent regressions?
• Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
• Did you update the typescript typings accordingly (if applicable)?
• Did you follow the commit message conventions explained in CONTRIBUTING.md?

Motivation

Currently there is no way to create model instances type safely. So no matter what properties are defined on model, for every object which gets passed to build, create and so on the compiler doesn't complain:

class User extends Model {
  name!: string;
}

User.create({
  anything: 'is allowed here' // :(
});
new User({
  anything: 'is allowed here' // :(
})

Description of change

This PR changes this by implementing the types ModelAttributeKeys<M> and ModelValues<M>. (So this PR only includes changes of typings)

• ModelAttributeKeys<M> creates a union type of M (a class type extending Model), which includes all keys that are likely to be attribute keys. This is done by removing all function keys of M and all keys of base model Model
• ModelValues<M> uses ModelAttributeKeys<M> to create a new type which includes all model attribute properties.
• (Also removed custom Omit type, since it is already built in with used TypeScript version)

Examples

class UserGroup extends Model {
  name!: string;
  users?: User[];
}
class User extends Model {
  name!: string;
  age!: number;
  group: UserGroup; 
  toString() {
    return `This is ${this.name} (${this.age})`;
  }
}

If a new instance of User is created now we're getting autocompletion and type safety:

User.create({name: 'Marty'}); // ok
User.create({firstName: 'Marty'}); // error

User.findOrCreate({where: {name: 'Marty'}, defaults: {name: 'Marty'}}); // ok
User.findOrCreate({where: {name: 'Marty'}, defaults: {firstName: 'Marty'}}); // error

This also works for included models:

User.create({name: 'Marty', group: {name: 'BTTF'}}, {include: [UserGroup]}); // ok
User.create({name: 'Marty', group: {title: 'BTTF'}}, {include: [UserGroup]}); // error

UserGroup.create({name: 'BTTF', users: [{name: 'Marty'}]}, {include: [User]}); // ok
UserGroup.create({name: 'BTTF', users: [{firstName: 'Marty'}]}, {include: [User]}); // error

For instantiation with new, generic type of Model need to be set explicitly: class User extends Model<User> { ... }:

new User({name: 'Marty'}); // ok
new User({firstName: 'Marty'}); // error

fixes sequelize/sequelize-typescript#726
fixes sequelize/sequelize-typescript#728

[codecov]

Codecov Report

Merging #11820 into master will increase coverage by 21.48%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           master   #11820       +/-   ##
===========================================
+ Coverage   74.85%   96.34%   +21.48%     
===========================================
  Files          86       95        +9     
  Lines        8373     9220      +847     
===========================================
+ Hits         6268     8883     +2615     
+ Misses       2105      337     -1768     

Impacted Files	Coverage Δ
lib/dialects/mariadb/query.js	87.78% <0.00%> (ø)
lib/dialects/sqlite/connection-manager.js	100.00% <0.00%> (ø)
lib/dialects/sqlite/query.js	98.65% <0.00%> (ø)
lib/dialects/postgres/index.js	100.00% <0.00%> (ø)
lib/dialects/mariadb/index.js	100.00% <0.00%> (ø)
lib/dialects/postgres/connection-manager.js	95.83% <0.00%> (ø)
lib/dialects/mariadb/connection-manager.js	100.00% <0.00%> (ø)
lib/dialects/postgres/query.js	97.84% <0.00%> (ø)
lib/dialects/sqlite/index.js	100.00% <0.00%> (ø)
lib/model.js	96.60% <0.00%> (+1.06%)	⬆️
... and 28 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a1ec8a1...d3bf1c7. Read the comment docs.

[RobinBuschmann]

I'm not quite sure where to put these helper types. The custom Omit type was defined in model.d.ts as well, but it doesn't feel like the right place.

[papb]

I think it's fine to leave them here for now, we can worry about refactoring on another time

[papb]

Hello, thank you very much for this PR!

Are you testing that type safety is indeed being guaranteed? In other words, is there a test to catch what you mentioned?

User.create({
  anything: 'is allowed here' // :(
});

From a quick look, your PR does not seem to include a test that ensures a compilation error in the above case... Let me know if I'm mistaken, thanks!

[RobinBuschmann]

Hey @papb, you're right, there is no test verifying this behavior. Unfortunately it seems so that the test setup only allows positive testing. Since just the tsc cli is used to check the typings, a compiler error would cause to fail this kind of tests, but this is what we would expect from a negative test here. Or probably I’ve overseen something?

I could offer to create another PR which allows such test cases by using something like https://www.npmjs.com/package/tsd or something similar - but we should discuss this before - maybe in another issue(?)

[GuskiS]

Shouldn't this be done for where as well? 🤔

[papb]

@RobinBuschmann excellent idea, please see #11829

[papb]

Shouldn't this be done for where as well? thinking

Probably a good idea! However should be done in another PR I think - PR welcome :)

[GuskiS]

Is there a reason for this? Those Ts in Model<T = any, T2 = any> doesn't seem to be used.

[papb]

@GuskiS indeed they are not used, they are going to be removed, see #11682

[RobinBuschmann]

The first generic type is used with this PR in order to get autocompletion for new MyModel(/* here */)

[papb]

@RobinBuschmann ah, so this means that I'm wrong here? good to know, please mention it on that PR so we don't forget about that, thanks

[GuskiS]

@RobinBuschmann Oh, I see. For me, it is very confusing that it takes class that is being created on the same line.

[RobinBuschmann]

@GuskiS I know what you mean, but unfortunately it isn't possible to use this for the constructor params (TS2526: A 'this' type is available only in a non-static member of a class or interface.), which makes sense, since the type of this isn't determined at this point.

[GuskiS]

I wonder, can't we do something like this? https://www.typescriptlang.org/play/#code/JYOwLgpgTgZghgYwgAgLIE9UHsAmEA2AgmGFMAEYCukAzsgN4BQyywOAXMiJQLbnQBuZskgAPMJxqlQAcyEBfRowT44NOgCE1EbHnwAeACoA+BksXLV6tJlwFkEcRBA5N23QX0YPREmSq0psA8AA74EDzOYHTedr7SARB0TIzyQA

This way you won't have to remove the model stuff from the type.
MyModelAttributes interface could be reused for other parts of the codebase.

[RobinBuschmann]

Hey @GuskiS, yes this is an option. But then you would need to define interfaces for every model. For each interface it is very likely that it is only implemented by one class and you would have to define the properties one more time on the model itself. So inferring the value types of the model class itself is much more comfortable for the consumer of this library. And in addition nothing prevents you from defining an interface anyway. What do you think?

[GuskiS]

Yea, that makes sense.
However, in my app we still create that interface because we are using it to define express server's request body type. Using the Model type in this is would be incorrect.

Anyways, that was just a thought and an example :) Waiting for this get merged 👼

[todd]

Perhaps both are achievable? Default the attributes to ModelValues<M>, but allow them to be overridden using the pattern @GuskiS is proposing?

[todd]

Greetings from #11457! 👋

I believe there's some crossover with the discussion that was happening there and the work that's been done here. Taking a peek at what's been committed so far, it appears that the original issue I opened my PR for (type safety for the create mixin methods) isn't being covered here yet, it should be relatively trivial to add support for those with the work that's being done here.

@RobinBuschmann Let me know if you want any help with any of this. Totally willing to help test this against one or two of my codebases as well if you need some external validation.

🍻

[papb]

@todd Thank you 😬

You and @RobinBuschmann please feel free to ping me if you need something to make this PR move forward :)

[RobinBuschmann]

@todd Thanks for pointing this out. I think we should focus on the static model functions here and create a new PR for the mixins afterwards. What do you think?

@papb Sry, for the late reply.

please feel free to ping me if you need something to make this PR move forward :)

It is still blocked by #11829 :(

[todd]

@RobinBuschmann Sounds good to me. More than happy to tackle that when the time is right. In the mean time, let me know if I can be of any help here.

[todd]

Howdy folks - any update on what's happening here? Anything I can pick up?

[RobinBuschmann]

Hey @todd thanks for asking - I’m waiting for feedback on #11879

[todd]

@RobinBuschmann Roger that. Let me know when I can be useful.

[RobinBuschmann]

This is the only test file which includes enhanced testing. Which means, that it tests

• if type inference works
  • for just the target model values
  • for included models
  • for included models where included models is an array
• if it throws in case of
  • wrong property name in target model values
  • right property name but wrong type
  • wrong property name in included models
  • wrong property name in included models, where included models is an array.

Since all other functions like findOrBuild, bulkBuild and so on are using the same type, which is ModelValues <M> , they mostly testing if type inference work and if it throws an error if anything is wrong.

[RobinBuschmann]

@papb @SimonSchick Is this an issue codecov/project — 95.68% (+-0.48%) compared to a283dc0? Otherwise this is ready to be reviewed again :)

[sushantdhiman]

@SimonSchick / @eseliger / @papb Please merge this PR if it looks OK (Typescript stuff)

[savioserra]

Hi guys, sorry to bother. Any update on this?

[tgouala]

@SimonSchick @papb Hi guys, is this PR ready to move forward ? Really looking forward this typing inference. Thanks !

[intellix]

I guess it's conflicted now: #12405
Been waiting on this PR to update from Sequelize 4 and typescript-sequelize 0.6 as there's no point moving across for less type safety.

I don't understand though, it looks like the other PR was created 4 days ago and has already been merged and released. Looks like it delivers the same stuff as this one which was opened in January and left to stagnate with no interest

[hsource]

I guess it's conflicted now: #12405
Been waiting on this PR to update from Sequelize 4 and typescript-sequelize 0.6 as there's no point moving across for less type safety.

I don't understand though, it looks like the other PR was created 4 days ago and has already been merged and released. Looks like it delivers the same stuff as this one which was opened in January and left to stagnate with no interest

Hi! I'm the author of the other PR (#12405) - I aimed to replicate not only the type-safe creation of models but also type-safe querying (find, insert, update) as much as possible.

As a case study, I managed to update my Sequelize 4 code (for https://wanderlog.com) to the new set of typings with purely search/replace (with some regexps), and about an hour or so of cleanup. This is because the new typings also use the 2 same generic types as the Sequelize v4 typings: TAttributes (an object with types for all attributes) and TCreationAttributes (like TAttributes, but with some keys marked as optional). The typings I submitted are stricter and the upgrade managed to catch a few bugs in our codebase. I hope you try them out - happy to resolve any issues in them.

[ephys]

I'll close this PR as it looks like #12405 resolved the same issue.
I can't give any information regarding why we didn't go forward with this one as I wasn't there at the time but sorry for abandoning this PR.

If there is anything in this PR that isn't covered by the current version of the codebase, could you either open a new PR with those changes or update this PR (I'll reopen it).