Skip to content

refactor: migrate associations to TypeScript#14280

Merged
WikiRik merged 120 commits into
mainfrom
ephys/ts-associations
May 23, 2022
Merged

refactor: migrate associations to TypeScript#14280
WikiRik merged 120 commits into
mainfrom
ephys/ts-associations

Conversation

@ephys
Copy link
Copy Markdown
Member

@ephys ephys commented Mar 28, 2022

This PR migrates the 'association' folder to TypeScript.

This PR brings makes very important changes to how associations work in Sequelize:

List of changes

  • Association names have been normalized: All associations are named, and their name always corresponds to their value in Model.associations.x.
  • Associations are unique: It's not possible to replace an association once defined (breaking change).
  • Associations are immutable: Once defined, they cannot be changed (breaking change).
  • The way associations are determined when using include has changed (breaking change).
  • Includes are now strict.

Changes to how foreign keys are created

Instead of duplicating code across all associations, BelongsTo is the only association capable of adding Foreign Keys. All other associations delegate to BelongsTo:

  • HasMany creates its inverse BelongsTo association
  • HasOne creates its inverse BelongsTo association
  • BelongsToMany creates its inverse BelongsToMany association, both of which create a HasMany association between Through and Source, which creates a BelongsTo

Association names have been normalized

Sequelize 6 would consider these two associations to be different:

Project.belongsTo(User);
Project.belongsTo(User, { as: 'User' });

This worked most of the time, but it was inconsistent internally. Both associations shared the same name and Project.associations.User be equal to the last association to have been defined.
Even more inconsistently, project.getUser and other mixin methods would belong to the first association to have been defined.

After this PR, the above code will throw an error saying that the alias is already in use and to use another one.

Associations are unique

The following code will now throw, because you can't replace an association that has already been defined anymore (your codebase would have been very fragile if you relied on this anyway, it's a very unsafe thing to do):

Project.belongsTo(User);
Project.belongsTo(User);

The way associations are determined when using include has changed

When specifying an include using a model+as, Sequelize 6 would search for an association that was defined with that model & as.

So these failed (on purpose):

Project.belongsTo(User);

Project.findAll({
  include: { model: User, as: 'User' },
});

// ----

Project.belongsTo(User, { as: 'User' });

Project.findAll({
  include: { model: User },
});

these worked:

Project.belongsTo(User);

Project.findAll({
  include: { model: User },
});

// ----

Project.belongsTo(User, { as: 'User' });

Project.findAll({
  include: { model: User, as: 'User' },
});

In Sequelize 7, the way it works is that if only one association has been defined with that model as the target, it will use that one, otherwise it will disambiguate using as.

If you're using sequelize-typescript, that is already how it works as they override our system to implement this

That means using as is exactly the same as using association: and I think we should update our documentation to promote the use of association: over the use of model:. i.e. instead of doing this:

Project.findAll({
  include: { model: User, as: 'User' },
});

do this:

Project.findAll({
  include: 'User',
});

Associations are immutable

Before this PR, associations were a bit fuzzy: Defining an association on both sides of the association would attempt to merge and reconcile their options.

A good idea in theory, but these associations are applied as soon as the method is called. This means reconciliation may need to revert changes that were already done on other objects, like renaming a previously added foreign key on a model.

Determining which association was paired with which was also tricky.

With this PR, it's still possible to define association on both sides, but their options must be perfectly compatible. For instance this is not valid:

User.belongsToMany(Countries, { foreignKey: 'user_id' });
Countries.belongsToMany(User);

but this is:

User.belongsToMany(Countries, { foreignKey: 'user_id' });
Countries.belongsToMany(User, { otherKey: 'user_id' });

To make things easier, I've introduced the long requested "inverse" option which lets a user define the association on both sides in one call.

Instead of doing this:

User.belongsToMany(Countries, { as: 'countries' });
User.belongsToMany(Countries, { as: 'citizen' });

users should now do this:

User.belongsToMany(Countries, { as: 'countries', inverse: { as: 'citizen' } });

Closes #5144

Includes are now strict

This PR normalizes Includes, which resolves a series of inconsistencies and should make them more stable.

For instance, Includes are always validated. If no association is found matching that include, an error will be thrown (including nested includes).

PENDING QUESTION: Found an issue with BelongsToMany.

Edit: resolved, see comments

Given a BelongsToMany association from User to Project through User_Project,
and using groupedLimit on the BelongsToMany association,

This needs to be unified, although I'm not sure which choice is best. Right now I've picked the second behavior.

Other breaking changes

  • The options 'foreignKeyConstraint' and 'constraints' have been merged in 'foreignKeyConstraints' as they did the same thing.
  • The 'timestamps' option in BelongsToManyOptions has been moved to ThroughOptions.timestamps
  • The 'uniqueKey' option in BelongsToManyOptions has been merged with ThroughOptions.unique
  • HasMany / HasOne don't use 'as' as the foreign key anymore (it wouldn't make sense. You ended up with the class Project.hasMany(User) meant the model User received a foreign key called UserId.
  • The foreignKey and otherKey options are unified with ColumnOptions:
    • The 'keyType' option is now 'foreignKey.type'
    • The 'onDelete' option is not 'foreignKey.onDelete'
    • The 'onUpdate' option is not 'foreignKey.onUpdate'
  • The default value for onDelete for non-null FKs is unified. It's now always CASCADE. Previously, it was NO ACTION if defined first through belongsTo, and CASCADE if defined first through hasOne or hasMany, leading to unexpected behaviors depending on which method was called first.

Closes #13429
Closes #12782
Closes #11837

What's left to be done

What's next (in follow-up PRs)?

  • Consider renaming otherKey to inverse.foreignKey
  • Forbid defining an association if the primary key is composite. As this is not supported and breaks silently. (we can add support for this but users should be aware that it will not work).
  • Document the breaking changes in the upgrade guide
  • Update the documentation to reflect new recommended behaviors (includes, super many-to-many, inverse)
  • Document the best practices to avoid errors with associations: Use 'inverse' instead of defining the association twice, beware of creating the same association twice without naming the inverse (this.User.belongsToMany(this.Task) + this.User.belongsToMany(this.Task, {as:'association2'}))
  • Improve ORDER BY thanks to the normalization of associations

@ephys ephys self-assigned this Mar 28, 2022
@ephys ephys added the typescript For issues and PRs. Things that involve typescript, such as typings and intellisense. label Mar 28, 2022
Comment thread test/unit/esm-named-exports.test.js
@ephys
Copy link
Copy Markdown
Member Author

ephys commented Apr 21, 2022

Conflicts resolved

Copy link
Copy Markdown
Member

@WikiRik WikiRik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has been a lot. I mainly have questions to when TODOs will have to be taken care of. Maybe we should provide TODOs with some extra information.
My idea was to add something like;

  • TODO needs to be taken care of before the release of v7
  • TODO needs to be taken care of after the release of 7.0.0 and is non-breaking
  • TODO needs to be taken care of after the release of 7.0.0 and is breaking
  • TODO needs to be taken care of after we drop support for version x of whatever (Node/TypeScript/database version)

Comment thread test/support.ts Outdated
Comment thread src/dialects/abstract/query-generator.js Outdated
Comment thread test/integration/hooks/associations.test.js
Comment thread src/model.d.ts Outdated
Comment thread test/unit/associations/belongs-to-many.test.ts
Comment thread src/associations/belongs-to-many.ts Outdated
Comment thread src/associations/belongs-to-many.ts
Comment thread src/associations/belongs-to-many.ts
Comment thread src/associations/belongs-to-many.ts
Comment thread src/associations/belongs-to-many.ts Outdated
Copy link
Copy Markdown
Member

@WikiRik WikiRik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still have some questions open, but nothing that's blocking this PR. Good work!

Copy link
Copy Markdown
Member

@WikiRik WikiRik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Woops, it seemed that merging main broke some unit tests. Should have looked at that as well, before approving 😅

@ephys
Copy link
Copy Markdown
Member Author

ephys commented May 13, 2022

I mainly have questions to when TODOs will have to be taken care of

Would it be enough if I mentioned whether the change was a breaking change? As much as I'd like to have some of them fixed before v7, I can't guarantee it with the time I can allocate to this project :/

I can still sort that out before merging :)

@WikiRik WikiRik merged commit 428a85b into main May 23, 2022
@WikiRik WikiRik deleted the ephys/ts-associations branch May 23, 2022 12:51
vanthome pushed a commit to vanthome/sequelize that referenced this pull request Jun 12, 2022
* refactor: migrate associations to ts (wip)

* refactor: migrate has-one to ts

* refactor: migrate belongs-to association to TS

* refactor: fix typing errors in has-one, belongs-to

* refactor: migrate has-many to ts

* refactor: migrate has-many to ts

* refactor: finish migrating has-many.ts

* refactor: migrate association/index.ts

* refactor: continue association migration to ts

* build: accept @typeparam, @internal typedoc tags

* feat: improve typings of hasOne.set

* fix: make HasOneGetAssociationMixin's output nullable

* fix: improve typing of BelongsTo set method

* feat: update hasMany

* refactor: complete typing associations

* fix: fix failing tests

* refactor: migrate sequelize export to esm

* fix: fix associations based on unit tests

* fix: fix associations based on intergration tests

* test: add test to prevent duplicate associations aliases

* fix: fix Sequelize typings

* test: unify groupedLimit m-n test with the integration one

* feat: forbid instanciating Association/ManyAssociation

* docs: revert accidental comment removal

* fix: normalize includes before merging them

* fix: replace query generator trying to create a new association

* refactor: revert changes to reduce noise

* test: fix mssql query generator test

* feat: forbid users from instantiating association Classes

* fix: dedupe BelongsToMany association

* fix: use isSameModel to compare models

* feat: improve error message for 'this association was already created'

* fix: continue work to fix tests

* feat: fix associations based on unit tests

* refactor: migrate mixin.js to typescript

* refactor: rename 'mixin.ts' -> 'define-association-methods.ts'

* fix: fix accidental usage of _.eq instead of _.isEqual

* test: fix m:n tests

* refactor: move association definitions methods inside their classes

* refactor: continue refactoring associations

* feat: improve association deduplication

* fix: remove accidental export

* test: support 'cause' property when checking error message

* fix: normalize belongs to many opts, improve error msg

* feat: print error cause on systems without support

* test: fix belongs to many tests

* fix: continue fixing based on tests

* fix: normalize the right timestamps option

* feat: add Model.{getInitialModel,withoutScope}, rename schema+(un)?scope

* fix: add deprecation IDs

* feat: improve 'invalid scope' error message

* fix: new batch of fixes

* fix: make association.get return Map for multiple

* docs: remove todos

* fix: fix failing tests

* fix: rollback test update

* test: fix mariadb/mysql test

* test: fix test

* test: fix mysql8 test

* feat(types): strongly type association mixins

* test: migrate association.test to ts

* test: migrate belongs-to test

* feat(types): strongly type belongsToMany FKs

* test: test deep through options version

* test: test sourceKey/targetKey typing

* test: migrate has-one unit test to typescript

* test: migrate has-many unit tests to ts

* test: migrate belongs-to-many unit test to ts

* test: fix test

* test: fix support.js

* test: test two associations sharing the same pair

* docs: update tsdoc of associations

* feat: refactor associations to support 'inverse' option

* feat: remove onUpdate, onDelete, inverse.{onUpdate,onDelete}

They already exist as foreignKey.{onDelete,onUpdate}, otherkey.{onDelete,onUpdate}

* fix: make CASCADE default onDelete for non-null FKs

* refactor: normalize otherKey option

* feat: rename constraints, foreignKeyConstraint to foreignKeyConstraints

* fix: fix wrong scope option being passed to belongsTo

* feat: support 'inverse' in belongs-to

* feat: force all self-associations to be named

* fix: simplify the name of foreignKeys on through models

* fix: special case through models in order by

* test: fix tests

* test: fix broken test

* fix: fix sqlite treating allowNull undefined as false

* test: improve sql minifier

* feat: use through model name in groupedLimit through include

* refactor: remove unnecessary changes

* test: mute type error due to missing typing

* refactor: update comments

* refactor: update comment

* test: update unit tests

Co-authored-by: Fauzan "Evan Edrok" 0x <fncolon@pm.me>
Co-authored-by: Rik Smale <13023439+WikiRik@users.noreply.github.com>
@github-actions
Copy link
Copy Markdown
Contributor

🎉 This PR is included in version 7.0.0-alpha.14 🎉

The release is available on:

Your semantic-release bot 📦🚀

@bminer

This comment was marked as outdated.

@bminer
Copy link
Copy Markdown

bminer commented Aug 16, 2024

Please ignore my prior comment. I misunderstood this change, sorry! I'm going to hide it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

released on @v7 typescript For issues and PRs. Things that involve typescript, such as typings and intellisense.

Projects

None yet

5 participants