feat: make association definition methods throw if the generated foreign key is incompatible with an existing attribute #14715
+200
−76
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Pull Request Checklist
yarn test
oryarn test-DIALECT
pass with this change (including linting)?Description Of Change
This PR normalizes the
allowNull
andprimaryKey
options. At least it's the initial goal. 😅Association Breaking change
That change lead to this test failing:
This fails because the attributes passed to
sequelize.define
take priority over the attributes added by association definition methods. In this case,allowNull: false
is ignored because it's already been defined bysequelize.define
.This is a good change IMO. If an attribute has already been defined, the association definition method should not be able to completely change its properties. Users should write this instead:
In order to catch this and warn the user, association definition methods will now throw an error if the foreign key they are adding is incompatible with a previously-defined attribute. with the following message:
This change is slightly annoying because
belongsToMany
associations will attempt to create two non-nullable foreign keys by default. But this is only an issue if the user also created the foreign keys on the through model, and is fixed by setting theirallowNull
option to false.Polymorphic Association Breaking change
This change lead to discovering another bug, with polymorphic associations.
In the above example, the two
belongsToMany
try to use the same foreign key to reference two different models. Before this PR,ItemTag.getAttributes().taggable_id.references
would point toPost
(notComment
).After this PR, the code throws because the second
belongsToMany
call tries to maketaggable_id
referenceComment
, but it cannot as it already referencesPost
This is imo a good change to avoid unexpected behaviors.
The solution is simple: use
foreignKeyConstraints: false
on one or bothbelongsToMany
Commit message