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

src/associations/belongs-to.ts

@@ -11,6 +11,7 @@ import type {
   SaveOptions,
   AttributeNames,
   Attributes,
+  BuiltModelAttributeColumOptions,
 } from '../model';
 import { Op } from '../operators';
 import * as Utils from '../utils';
@@ -22,7 +23,6 @@ import { HasMany } from './has-many.js';
 import { HasOne } from './has-one.js';
 import type { NormalizeBaseAssociationOptions } from './helpers';
 import {
-  addForeignKeyConstraints,
   defineAssociation,
   mixinMethods, normalizeBaseAssociationOptions,
 } from './helpers';
@@ -135,17 +135,34 @@ export class BelongsTo<
     }
 
     this.foreignKey = foreignKey as SourceKey;
+    const existingForeignKeyAttribute: BuiltModelAttributeColumOptions | undefined
+      = this.source.rawAttributes[this.foreignKey];
 
+    this.targetKeyField = Utils.getColumnName(this.target.getAttributes()[this.targetKey]);
+    this.targetKeyIsPrimary = this.targetKey === this.target.primaryKeyAttribute;
+
+    const fkAllowsNull = foreignKeyAttributeOptions?.allowNull ?? existingForeignKeyAttribute?.allowNull ?? true;
+
+    // Foreign Key options are selected like this:
+    // 1. if provided explicitly through options.foreignKey, use that
+    // 2. if the foreign key is already defined on the source model, use that
+    // 3. hardcoded default value
+    // Note: If options.foreignKey is provided, but the foreign key also exists on the source model,
+    //  mergeAttributesDefault will throw an error if the two options are incompatible.
     const newForeignKeyAttribute = removeUndefined({
-      type: this.target.rawAttributes[this.targetKey].type,
+      type: existingForeignKeyAttribute?.type ?? this.target.rawAttributes[this.targetKey].type,
       ...foreignKeyAttributeOptions,
-      allowNull: this.source.rawAttributes[this.foreignKey]?.allowNull ?? foreignKeyAttributeOptions?.allowNull,
+      allowNull: fkAllowsNull,
+      onDelete: foreignKeyAttributeOptions?.onDelete ?? existingForeignKeyAttribute?.onDelete ?? (fkAllowsNull ? 'SET NULL' : 'CASCADE'),
+      onUpdate: foreignKeyAttributeOptions?.onUpdate ?? existingForeignKeyAttribute?.onUpdate ?? 'CASCADE',
     });
 
-    this.targetKeyField = Utils.getColumnName(this.target.getAttributes()[this.targetKey]);
-    this.targetKeyIsPrimary = this.targetKey === this.target.primaryKeyAttribute;
-
-    addForeignKeyConstraints(newForeignKeyAttribute, this.target, this.options, this.targetKeyField);
+    if (options.foreignKeyConstraints !== false) {
+      newForeignKeyAttribute.references = {
+        model: this.target.getTableName(),
+        key: this.targetKeyField,
+      };
+    }
 
     this.source.mergeAttributesDefault({
       [this.foreignKey]: newForeignKeyAttribute,

src/associations/has-one.ts

@@ -135,7 +135,7 @@ export class HasOne<
     T extends Model,
     SourceKey extends AttributeNames<S>,
     TargetKey extends AttributeNames<T>,
-    >(
+  >(
     secret: symbol,
     source: ModelStatic<S>,
     target: ModelStatic<T>,

src/associations/helpers.ts

@@ -6,7 +6,7 @@ import lowerFirst from 'lodash/lowerFirst';
 import omit from 'lodash/omit';
 import type { Class } from 'type-fest';
 import { AssociationError } from '../errors/index.js';
-import type { Model, ModelAttributeColumnOptions, ModelStatic } from '../model';
+import type { Model, ModelStatic } from '../model';
 import type { Sequelize } from '../sequelize';
 import * as deprecations from '../utils/deprecations.js';
 import type { OmitConstructors } from '../utils/index.js';
@@ -25,32 +25,6 @@ export function checkNamingCollision(source: ModelStatic<any>, associationName:
   }
 }
 
-export function addForeignKeyConstraints(
-  newAttribute: ModelAttributeColumnOptions,
-  source: ModelStatic<Model>,
-  options: AssociationOptions<string>,
-  key: string,
-): void {
-  // FK constraints are opt-in: users must either set `foreignKeyConstraints`
-  // on the association, or request an `onDelete` or `onUpdate` behavior
-
-  if (options.foreignKeyConstraints !== false) {
-    // Find primary keys: composite keys not supported with this approach
-    const primaryKeys = Object.keys(source.primaryKeys)
-      .map(primaryKeyAttribute => source.getAttributes()[primaryKeyAttribute].field || primaryKeyAttribute);
-
-    if (primaryKeys.length === 1 || !primaryKeys.includes(key)) {
-      newAttribute.references = {
-        model: source.getTableName(),
-        key: key || primaryKeys[0],
-      };
-
-      newAttribute.onDelete = newAttribute.onDelete ?? (newAttribute.allowNull !== false ? 'SET NULL' : 'CASCADE');
-      newAttribute.onUpdate = newAttribute.onUpdate ?? 'CASCADE';
-    }
-  }
-}
-
 /**
  * Mixin (inject) association methods to model prototype
  *

src/dialects/abstract/query-generator.js

@@ -66,16 +66,26 @@ export class AbstractQueryGenerator {
 
     const self = this;
 
-    return {
+    const out = {
       tableName: param.tableName || param,
       table: param.tableName || param,
       name: param.name || param,
       schema: param._schema,
       delimiter: param._schemaDelimiter || '.',
-      toString() {
+    };
+
+    // TODO: remove
+    // toString should *not* be used, but model uses it to stringify a table name and use it as object keys.
+    // until that behavior is changed, this stays
+    // it's at least non-enumerable to allow for deep comparisons
+    Object.defineProperty(out, 'toString', {
+      enumerable: false,
+      value: function toString() {
         return self.quoteTable(this);
       },
-    };
+    });
+
+    return out;
   }
 
   dropSchema(tableName, options) {

src/model.d.ts

@@ -1786,6 +1786,12 @@ export interface BuiltModelAttributeColumOptions<M extends Model = Model> extend
    */
   fieldName: string;
 
+  /** @inheritDoc */
+  allowNull: boolean;
+
+  /** @inheritDoc */
+  primaryKey: boolean;
+
   references?: ModelAttributeColumnReferencesOptions;
 }
 
@@ -2283,7 +2289,7 @@ export abstract class Model<TModelAttributes extends {} = any, TCreationAttribut
    * Only use if you know what you're doing.
    *
    * Warning: Attributes are not replaced, they are merged.
-   * The existing configuration for an attribute takes priority over the new configuration.
+   * Throws if an attribute already exists and one of the properties assigned to it is incompatible.
    *
    * @param newAttributes
    */

src/model.js

@@ -1120,10 +1120,18 @@ Specify a different name for either index to resolve this issue.`);
         definition.field = Utils.underscoredIf(name, this.underscored);
       }
 
-      if (definition.primaryKey === true) {
+      // attributes are not primary keys by default
+      definition.primaryKey = definition.primaryKey ?? false;
+
+      if (definition.primaryKey) {
         this.primaryKeys[name] = definition;
       }
 
+      if (definition.allowNull == null) {
+        // non-primary-key attributes are nullable by default
+        definition.allowNull = !definition.primaryKey;
+      }
+
       this.fieldRawAttributesMap[definition.field] = definition;
 
       if (definition.type._sanitize) {
@@ -1260,11 +1268,60 @@ Specify a different name for either index to resolve this issue.`);
    * Only use if you know what you're doing.
    *
    * Warning: Attributes are not replaced, they are merged.
+   * Throws if an attribute already exists and one of the properties assigned to it is incompatible.
    *
    * @param {object} newAttributes
    */
   static mergeAttributesDefault(newAttributes) {
-    Utils.mergeDefaults(this.rawAttributes, newAttributes);
+    for (const attrName of Object.keys(newAttributes)) {
+      const newAttribute = newAttributes[attrName];
+      if (this.rawAttributes[attrName] == null) {
+        this.rawAttributes[attrName] = newAttribute;
+        continue;
+      }
+
+      const existingAttribute = this.rawAttributes[attrName];
+
+      for (const propertyName of Object.keys(newAttribute)) {
+        if (existingAttribute[propertyName] === undefined) {
+          existingAttribute[propertyName] = newAttribute[propertyName];
+
+          continue;
+        }
+
+        if (existingAttribute[propertyName] === newAttribute[propertyName]) {
+          continue;
+        }
+
+        if (propertyName === 'references') {
+          const newReferencesOption = newAttribute[propertyName];
+          const existingReferencesOption = existingAttribute[propertyName];
+
+          for (const referencesPropertyName of Object.keys(newReferencesOption)) {
+            if (existingReferencesOption[referencesPropertyName] === undefined) {
+              existingReferencesOption[referencesPropertyName] = newReferencesOption[referencesPropertyName];
+
+              continue;
+            }
+
+            // using isEqual because 'model' can be an object with the shape `{ tableName: string }`
+            if (_.isEqual(existingReferencesOption[referencesPropertyName], newReferencesOption[referencesPropertyName])) {
+              continue;
+            }
+
+            throw new Error(`Cannot merge attributes: The property "references.${referencesPropertyName}" is already defined for attribute "${this.name}"."${attrName}" and is incompatible with the new definition.
+Existing value: ${NodeUtil.inspect(existingReferencesOption[referencesPropertyName])}
+New Value: ${NodeUtil.inspect(newReferencesOption[referencesPropertyName])}`);
+          }
+
+          continue;
+        }
+
+        throw new Error(`Cannot merge attributes: The property "${propertyName}" is already defined for attribute "${this.name}"."${attrName}" and is incompatible with the new definition.
+Existing value: ${NodeUtil.inspect(existingAttribute[propertyName])}
+New Value: ${NodeUtil.inspect(newAttribute[propertyName])}`);
+      }
+    }
 
     this.refreshAttributes();
 

test/integration/associations/belongs-to-many.test.js

@@ -1717,7 +1717,7 @@ describe(Support.getTestDialectTeaser('BelongsToMany'), () => {
         id: { type: DataTypes.INTEGER, primaryKey: true, autoIncrement: true },
         tag_id: { type: DataTypes.INTEGER, unique: false },
         taggable: { type: DataTypes.STRING },
-        taggable_id: { type: DataTypes.INTEGER, unique: false },
+        taggable_id: { type: DataTypes.INTEGER, unique: false, allowNull: false },
       });
       const Tag = this.sequelize.define('Tag', {
         id: { type: DataTypes.INTEGER, primaryKey: true, autoIncrement: true },
@@ -1735,11 +1735,13 @@ describe(Support.getTestDialectTeaser('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,
       });
 
       await this.sequelize.sync({ force: true });
@@ -1770,7 +1772,7 @@ describe(Support.getTestDialectTeaser('BelongsToMany'), () => {
         id: { type: DataTypes.INTEGER, primaryKey: true, autoIncrement: true },
         tag_id: { type: DataTypes.INTEGER, unique: false },
         taggable: { type: DataTypes.STRING },
-        taggable_id: { type: DataTypes.INTEGER, unique: false },
+        taggable_id: { type: DataTypes.INTEGER, unique: false, allowNull: false },
       });
       const Tag = this.sequelize.define('Tag', {
         id: { type: DataTypes.INTEGER, primaryKey: true, autoIncrement: true },
@@ -1788,11 +1790,13 @@ describe(Support.getTestDialectTeaser('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,
       });
 
       await this.sequelize.sync({ force: true });
@@ -3172,7 +3176,15 @@ describe(Support.getTestDialectTeaser('BelongsToMany'), () => {
     beforeEach(function () {
       this.Task = this.sequelize.define('task', { title: DataTypes.STRING });
       this.User = this.sequelize.define('user', { username: DataTypes.STRING });
-      this.UserTasks = this.sequelize.define('tasksusers', { userId: DataTypes.INTEGER, taskId: DataTypes.INTEGER });
+      this.UserTasks = this.sequelize.define('tasksusers', {
+        userId: {
+          type: DataTypes.INTEGER,
+          allowNull: false,
+        }, taskId: {
+          type: DataTypes.INTEGER,
+          allowNull: false,
+        },
+      });
     });
 
     it('can cascade deletes both ways by default', async function () {
@@ -3326,9 +3338,11 @@ describe(Support.getTestDialectTeaser('BelongsToMany'), () => {
         },
         id_user_very_long_field: {
           type: DataTypes.INTEGER(1),
+          allowNull: false,
         },
         id_task_very_long_field: {
           type: DataTypes.INTEGER(1),
+          allowNull: false,
         },
       }, {
         tableName: 'table_user_task_with_very_long_name',

test/integration/associations/scope.test.js

@@ -457,6 +457,7 @@ describe(Support.getTestDialectTeaser('associations'), () => {
               tag_id: {
                 type: DataTypes.INTEGER,
                 unique: 'item_tag_taggable',
+                allowNull: false,
               },
               taggable: {
                 type: DataTypes.STRING,
@@ -466,6 +467,7 @@ describe(Support.getTestDialectTeaser('associations'), () => {
                 type: DataTypes.INTEGER,
                 unique: 'item_tag_taggable',
                 references: null,
+                allowNull: false,
               },
             });
             this.Tag = this.sequelize.define('tag', {

test/integration/dialects/mariadb/dao-factory.test.js

@@ -63,7 +63,7 @@ if (dialect === 'mariadb') {
             User.rawAttributes,
           ),
         ).to.deep.equal(
-          { username: 'VARCHAR(255) PRIMARY KEY' },
+          { username: 'VARCHAR(255) NOT NULL PRIMARY KEY' },
         );
       });
 
@@ -148,7 +148,7 @@ if (dialect === 'mariadb') {
             User.primaryKeys,
           ),
         ).to.deep.equal(
-          { foo: 'VARCHAR(255) PRIMARY KEY' },
+          { foo: 'VARCHAR(255) NOT NULL PRIMARY KEY' },
         );
       });
     });

test/integration/dialects/mariadb/errors.test.js

@@ -27,8 +27,16 @@ if (dialect === 'mariadb') {
       beforeEach(function () {
         this.Task = this.sequelize.define('task', { title: DataTypes.STRING });
         this.User = this.sequelize.define('user', { username: DataTypes.STRING });
-        this.UserTasks = this.sequelize.define('tasksusers',
-          { userId: DataTypes.INTEGER, taskId: DataTypes.INTEGER });
+        this.UserTasks = this.sequelize.define('tasksusers', {
+          userId: {
+            type: DataTypes.INTEGER,
+            allowNull: false,
+          },
+          taskId: {
+            type: DataTypes.INTEGER,
+            allowNull: false,
+          },
+        });
 
         this.User.belongsToMany(this.Task, { foreignKey: { onDelete: 'RESTRICT' }, through: 'tasksusers', otherKey: { onDelete: 'RESTRICT' } });
 

test/integration/dialects/mysql/dao-factory.test.js

@@ -44,7 +44,7 @@ if (dialect === 'mysql') {
         const User = this.sequelize.define(`User${Support.rand()}`, {
           username: { type: DataTypes.STRING, primaryKey: true },
         }, { timestamps: false });
-        expect(this.sequelize.getQueryInterface().queryGenerator.attributesToSQL(User.rawAttributes)).to.deep.equal({ username: 'VARCHAR(255) PRIMARY KEY' });
+        expect(this.sequelize.getQueryInterface().queryGenerator.attributesToSQL(User.rawAttributes)).to.deep.equal({ username: 'VARCHAR(255) NOT NULL PRIMARY KEY' });
       });
 
       it('adds timestamps', function () {
@@ -82,7 +82,7 @@ if (dialect === 'mysql') {
           foo: { type: DataTypes.STRING, primaryKey: true },
           bar: DataTypes.STRING,
         });
-        expect(this.sequelize.getQueryInterface().queryGenerator.attributesToSQL(User.primaryKeys)).to.deep.equal({ foo: 'VARCHAR(255) PRIMARY KEY' });
+        expect(this.sequelize.getQueryInterface().queryGenerator.attributesToSQL(User.primaryKeys)).to.deep.equal({ foo: 'VARCHAR(255) NOT NULL PRIMARY KEY' });
       });
     });
   });