fix: unshadow model attributes

src/model.js

@@ -3,6 +3,7 @@
 import omit from 'lodash/omit';
 import { AbstractDataType } from './dialects/abstract/data-types';
 import { intersects } from './utils/array';
+import { noNewModel } from './utils/deprecations';
 import { toDefaultValue } from './utils/dialect';
 import {
   getComplexKeys,
@@ -44,6 +45,12 @@ const validQueryKeywords = new Set(['where', 'attributes', 'paranoid', 'include'
 // List of attributes that should not be implicitly passed into subqueries/includes.
 const nonCascadingOptions = ['include', 'attributes', 'originalAttributes', 'order', 'where', 'limit', 'offset', 'plain', 'group', 'having'];
 
+/**
+ * Used to ensure Model.build is used instead of new Model().
+ * Do not expose.
+ */
+const CONSTRUCTOR_SECRET = Symbol('model-constructor-secret');
+
 /**
  * A Model represents a table in the database. Instances of this class represent a database row.
  *
@@ -66,41 +73,26 @@ export class Model extends ModelTypeScript {
   /**
    * Builds a new model instance.
    *
+   * Cannot be used directly. Use {@link Model.build} instead.
+   *
    * @param {object}  [values={}] an object of key value pairs
    * @param {object}  [options] instance construction options
    * @param {boolean} [options.raw=false] If set to true, values will ignore field and virtual setters.
    * @param {boolean} [options.isNewRecord=true] Is this a new record
-   * @param {Array}   [options.include] an array of include options - Used to build prefetched/included model instances. See
-   *   `set`
+   * @param {Array}   [options.include] an array of include options - Used to build prefetched/included model instances. See `set`
+   * @param {symbol}  secret Secret used to ensure Model.build is used instead of new Model(). Don't forget to pass it up if you define a custom constructor.
    */
-  constructor(values = {}, options = {}) {
+  constructor(values = {}, options = {}, secret) {
     super();
 
-    this.constructor.assertIsInitialized();
-
-    if (!this.constructor._overwrittenAttributesChecked) {
-      this.constructor._overwrittenAttributesChecked = true;
-
-      // setTimeout is hacky but necessary.
-      // Public Class Fields declared by descendants of this class
-      // will not be available until after their call to super, so after
-      // this constructor is done running.
-      setTimeout(() => {
-        const overwrittenAttributes = [];
-        for (const key of this.constructor.modelDefinition.attributes.keys()) {
-          if (Object.prototype.hasOwnProperty.call(this, key)) {
-            overwrittenAttributes.push(key);
-          }
-        }
-
-        if (overwrittenAttributes.length > 0) {
-          logger.warn(`Model ${JSON.stringify(this.constructor.name)} is declaring public class fields for attribute(s): ${overwrittenAttributes.map(attr => JSON.stringify(attr)).join(', ')}.`
-            + '\nThese class fields are shadowing Sequelize\'s attribute getters & setters.'
-            + '\nSee https://sequelize.org/docs/v7/core-concepts/model-basics/#caveat-with-public-class-fields');
-        }
-      }, 0);
+    if (secret !== CONSTRUCTOR_SECRET) {
+      noNewModel();
+      // TODO [>=8]: throw instead of deprecation notice
+      // throw new Error(`Use ${this.constructor.name}.build() instead of new ${this.constructor.name}()`);
     }
 
+    this.constructor.assertIsInitialized();
+
     options = {
       isNewRecord: true,
       _schema: this.constructor.modelDefinition.table.schema,
@@ -1673,7 +1665,16 @@ ${associationOwner._getAssociationDebugList()}`);
       return this.bulkBuild(values, options);
     }
 
-    return new this(values, options);
+    const instance = new this(values, options, CONSTRUCTOR_SECRET);
+
+    // Our Model class adds getters and setters for attributes on the prototype,
+    // so they can be shadowed by native class properties that are defined on the class that extends Model (See #14300).
+    // This deletes the instance properties, to un-shadow the getters and setters.
+    for (const attributeName of this.modelDefinition.attributes.keys()) {
+      delete instance[attributeName];
+    }
+
+    return instance;
   }
 
   /**

src/utils/deprecations.ts

@@ -21,3 +21,4 @@ export const noSchemaDelimiterParameter = deprecate(noop, 'The schemaDelimiter p
 export const columnToAttribute = deprecate(noop, 'The @Column decorator has been renamed to @Attribute.', 'SEQUELIZE0017');
 export const fieldToColumn = deprecate(noop, 'The "field" option in attribute definitions has been renamed to "columnName".', 'SEQUELIZE0018');
 export const noModelTableName = deprecate(noop, 'Model.tableName has been replaced with the more complete Model.modelDefinition.table, or Model.table', 'SEQUELIZE0019');
+export const noNewModel = deprecate(noop, `Do not use "new YourModel()" to instantiate a model. Use "YourModel.build()" instead. The previous option is being removed to resolve a conflict with class properties. See https://github.com/sequelize/sequelize/issues/14300#issuecomment-1355188077 for more information.`, 'SEQUELIZE0020');

test/registerEsbuild.js

@@ -33,6 +33,12 @@ function compileFor(loader) {
       format: 'cjs',
       sourcefile,
       loader,
+      tsconfigRaw: {
+        compilerOptions: {
+          target: 'node14',
+          useDefineForClassFields: true,
+        },
+      },
     });
 
     if (Object.keys(maps).length === 0) {

test/unit/decorators/attribute.test.ts

@@ -11,7 +11,7 @@ describe(`@Attribute legacy decorator`, () => {
       declare id: bigint;
     }
 
-    expect(() => new Test()).to.throw(/has not been initialized/);
+    expect(() => Test.build()).to.throw(/has not been initialized/);
   });
 
   it('prevents using Model.init', () => {
@@ -54,7 +54,7 @@ describe(`@Attribute legacy decorator`, () => {
 
     sequelize.addModels([User]);
 
-    const user = new User({});
+    const user = User.build({});
     user.name = 'Peter';
 
     expect(user.name).to.equal('My name is Peter');
@@ -75,7 +75,7 @@ describe(`@Attribute legacy decorator`, () => {
 
     sequelize.addModels([User]);
 
-    const user = new User({});
+    const user = User.build({});
     user.name = 'Peter';
 
     expect(user.name).to.equal('My name is Peter');

test/unit/decorators/table.test.ts

@@ -8,7 +8,7 @@ describe(`@Table legacy decorator`, () => {
     @Table
     class Test extends Model {}
 
-    expect(() => new Test()).to.throw(/has not been initialized/);
+    expect(() => Test.build()).to.throw(/has not been initialized/);
   });
 
   it('prevents using Model.init', () => {

test/unit/instance/changed.test.ts

@@ -107,7 +107,7 @@ describe('Model#changed()', () => {
 
       it('returns false when setting a value to the same object value', () => {
         for (const value of [null, 1, 'asdf', new Date(), [], {}, Buffer.from('')]) {
-          const t = new vars.User({
+          const t = vars.User.build({
             json: value,
           }, {
             isNewRecord: false,

test/unit/model/get-attributes.test.ts

@@ -9,7 +9,7 @@ function assertDataType(property: NormalizedAttributeOptions, dataType: DataType
 
 describe(getTestDialectTeaser('Model'), () => {
   describe('getAttributes', () => {
-    it('should return attributes with getAttributes()', () => {
+    it(`returns the model's attributes`, () => {
       const Model = sequelize.define(
         'User',
         { username: DataTypes.STRING },
@@ -41,7 +41,7 @@ describe(getTestDialectTeaser('Model'), () => {
       });
     });
 
-    it('will contain timestamps if enabled', () => {
+    it('contains timestamps if enabled', () => {
       const Model = sequelize.define('User', { username: DataTypes.STRING });
       const attributes = Model.getAttributes();
 
@@ -72,7 +72,7 @@ describe(getTestDialectTeaser('Model'), () => {
       });
     });
 
-    it('will contain timestamps if enabled', () => {
+    it('contains virtual attributes', () => {
       const Model = sequelize.define(
         'User',
         {

test/unit/model/init.test.ts

@@ -5,7 +5,7 @@ describe('Uninitialized model', () => {
   class Test extends Model {}
 
   it('throws when constructed', () => {
-    expect(() => new Test()).to.throw(/has not been initialized/);
+    expect(() => Test.build()).to.throw(/has not been initialized/);
   });
 
   it('throws when .sequelize is accessed', () => {

test/unit/model/overwriting-builtins.test.ts

@@ -1,18 +1,49 @@
 import { expect } from 'chai';
-import { DataTypes } from '@sequelize/core';
-import { getTestDialectTeaser, sequelize } from '../../support';
+import type { CreationOptional, InferAttributes, InferCreationAttributes, NonAttribute } from '@sequelize/core';
+import { DataTypes, Model } from '@sequelize/core';
+import { sequelize } from '../../support';
 
-describe(getTestDialectTeaser('Model'), () => {
-  describe('not breaking built-ins', () => {
-    it('it should not break instance.set by defining a model set attribute', () => {
-      const User = sequelize.define('OverWrittenKeys', {
-        set: DataTypes.STRING,
-      });
-
-      const user = User.build({ set: 'A' });
-      expect(user.get('set')).to.equal('A');
-      user.set('set', 'B');
-      expect(user.get('set')).to.equal('B');
+describe('Model', () => {
+  // TODO [>=8]: throw if an attribute has the same name as a built-in Model method.
+  it('does not let attributes shadow built-in model method & properties', () => {
+    const User = sequelize.define('OverWrittenKeys', {
+      set: DataTypes.STRING,
     });
+
+    const user = User.build({ set: 'A' });
+    expect(user.get('set')).to.equal('A');
+    user.set('set', 'B');
+    expect(user.get('set')).to.equal('B');
+  });
+
+  it('makes attributes take priority over class properties defined on the model', () => {
+    // Issue description: https://github.com/sequelize/sequelize/issues/14300
+    // Sequelize models add getter & setters on the model prototype for all attributes defined on the model.
+    // Class properties, which are usually used when using TypeScript or decorators, are added on the model instance,
+    // and therefore take priority over the prototype getters & setters.
+    // This test ensures that the attributes are not shadowed by the class properties.
+    class User extends Model<InferAttributes<User>, InferCreationAttributes<User>> {
+      id: CreationOptional<number> = 10;
+      firstName: string = 'abc';
+      nonAttribute: NonAttribute<string> = 'def';
+    }
+
+    User.init({
+      id: {
+        type: DataTypes.INTEGER,
+        primaryKey: true,
+        autoIncrement: true,
+      },
+      firstName: {
+        type: DataTypes.STRING,
+        allowNull: false,
+      },
+    }, { sequelize });
+
+    const user = User.build({ firstName: 'Zoe' });
+
+    expect(user.id).to.eq(null); // autoIncrement
+    expect(user.firstName).to.eq('Zoe');
+    expect(user.nonAttribute).to.eq('def');
   });
 });

test/unit/query-interface/delete.test.ts

@@ -16,7 +16,7 @@ describe('QueryInterface#delete', () => {
   it('does not parse replacements outside of raw sql', async () => {
     const stub = sinon.stub(sequelize, 'queryRaw');
 
-    const instance = new User();
+    const instance = User.build();
 
     await sequelize.getQueryInterface().delete(
       instance,

test/unit/query-interface/update.test.ts

@@ -16,7 +16,7 @@ describe('QueryInterface#update', () => {
   it('does not parse replacements outside of raw sql', async () => {
     const stub = sinon.stub(sequelize, 'queryRaw');
 
-    const instance = new User();
+    const instance = User.build();
 
     await sequelize.getQueryInterface().update(
       instance,
@@ -49,7 +49,7 @@ describe('QueryInterface#update', () => {
   it('throws if a bind parameter name starts with the reserved "sequelize_" prefix', async () => {
     sinon.stub(sequelize, 'queryRaw');
 
-    const instance = new User();
+    const instance = User.build();
 
     await expect(sequelize.getQueryInterface().update(
       instance,
@@ -67,7 +67,7 @@ describe('QueryInterface#update', () => {
   it('merges user-provided bind parameters with sequelize-generated bind parameters (object bind)', async () => {
     const stub = sinon.stub(sequelize, 'queryRaw');
 
-    const instance = new User();
+    const instance = User.build();
 
     await sequelize.getQueryInterface().update(
       instance,
@@ -97,7 +97,7 @@ describe('QueryInterface#update', () => {
   it('merges user-provided bind parameters with sequelize-generated bind parameters (array bind)', async () => {
     const stub = sinon.stub(sequelize, 'queryRaw');
 
-    const instance = new User();
+    const instance = User.build();
 
     await sequelize.getQueryInterface().update(
       instance,

test/unit/utils/model-utils.test.ts

@@ -12,7 +12,7 @@ describe('isModelStatic', () => {
   it('returns false for model instances', () => {
     const MyModel = sequelize.define('myModel', {});
 
-    expect(isModelStatic(new MyModel())).to.be.false;
+    expect(isModelStatic(MyModel.build())).to.be.false;
   });
 
   it('returns false for the Model class', () => {