feat: make association definition methods throw if the generated foreign key is incompatible with an existing attribute

[ephys]

Pull Request Checklist

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

Description Of Change

This PR normalizes the allowNull and primaryKey options. At least it's the initial goal. 😅

Association Breaking change

That change lead to this test failing:

const User = sequelize.define('User');
const Project = sequelize.define('Project', {
  userId: DataTypes.INTEGER,
});

Project.belongsTo(User, {
  foreignkey: {
    name: 'userId',
    allowNull: false,
  },
});

expect(Project.getAttributes().userId.allowNull).to.eq(false);

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 by sequelize.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:

const User = sequelize.define('User');
const Project = sequelize.define('Project', {
  userId: DataTypes.INTEGER,
  allowNull: false,
});

Project.belongsTo(User, {
  foreignkey: {
    name: 'userId',
  },
});

expect(Project.getAttributes().userId.allowNull).to.eq(false);

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:

Cannot merge attributes: The property "allowNull" is already defined for attribute "Project"."userUid" and is incompatible with the new definition.
Existing value: true
New Value: false

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 their allowNull option to false.

Polymorphic Association Breaking change

This change lead to discovering another bug, with polymorphic associations.

const Tag = sequelize.define('Tag', {
  name: DataTypes.STRING,
});

const Comment = sequelize.define('Comment', {
  name: DataTypes.STRING,
});

const Post = sequelize.define('Post', {
  name: DataTypes.STRING,
});

const ItemTag = sequelize.define('ItemTag', {
  taggable: { type: DataTypes.STRING },
});

Post.belongsToMany(Tag, {
  through: { model: ItemTag, unique: false, scope: { taggable: 'post' } },
  foreignKey: 'taggable_id',
});

Comment.belongsToMany(Tag, {
  through: { model: ItemTag, unique: false, scope: { taggable: 'comment' } },
  foreignKey: 'taggable_id',
});

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 to Post (not Comment).

After this PR, the code throws because the second belongsToMany call tries to make taggable_id reference Comment, but it cannot as it already references Post

This is imo a good change to avoid unexpected behaviors.

The solution is simple: use foreignKeyConstraints: false on one or both belongsToMany

Post.belongsToMany(Tag, {
  through: { model: ItemTag, unique: false, scope: { taggable: 'post' } },
  foreignKey: 'taggable_id',
  foreignKeyConstraints: false,
});

Comment.belongsToMany(Tag, {
  through: { model: ItemTag, unique: false, scope: { taggable: 'comment' } },
  foreignKey: 'taggable_id',
  foreignKeyConstraints: false,
});

Commit message

BREAKING CHANGE: The 4 association definition methods (`belongsTo`, `hasMany`, `hasOne`, `belongsToMany`) will now throw an error if the foreign key they are trying to add is incompatible with an attributes that has already been defined. This means that if you are defining your foreign keys in `sequelize.define` or `Model.init`, you cannot change its properties through the `foreignKey` or `otherKey` options in your association definition method call.
BREAKING CHANGE: Associations will not allow using the same foreign key to reference two different tables by default anymore. This is mainly used by polymorphic associations. You can resolve this issue by setting the `foreignKeyConstraints` option to `false` in your association definition method option bag See PR for more information.

[WikiRik]

There's some failing integration tests. And apparently postgres does a much better stacktrace than the other dialects, that might be something to look into in the future

[ephys]

The difference is the quality of error messages is caused by mocha.

We use the native cause property. It was added in node 16.9, so we modify the error message when .cause is not available to make the message still readable

mocha doesn't support .cause yet, so in our CI the error message is still missing a lot of information even though node supports it

[ephys]

Looks like I have to draft this PR until #14687 is complete.

This change results in sync({ alter: true }) calling changeColumn with instructions that result in

ALTER TABLE "testSyncs" 
  ALTER COLUMN "age" SET DATA TYPE VARCHAR(255) 
  ALTER COLUMN "age" DROP NOT NULL

The issue here is that for some reason, db2 will not allow you to modify the same column twice in the same alter statement. It rejects the query with the message

[IBM][CLI Driver][DB2/LINUXX8664] SQL0612N  "age" is a duplicate name.  SQLSTATE=42711

That is something that will need to be fixed by that other PR

[voxpelli]

@ephys Could you perhaps add a review or such to my Mocha PR to give it a bit more weight and push it forward to a possible inclusion for the benefit of us all? :)