fix: always escape string attributes (#15374)

BREAKING CHANGE: string attributes are always escaped as identifiers, including "*". See #15374 for details.

src/dialects/abstract/index.ts

@@ -63,10 +63,7 @@ export type DialectSupports = {
   finalTable: boolean,
 
   /* does the dialect support returning values for inserted/updated fields */
-  returnValues: false | {
-    output: boolean,
-    returning: boolean,
-  },
+  returnValues: false | 'output' | 'returning',
 
   /* features specific to autoIncrement values */
   autoIncrement: {

src/dialects/abstract/query-generator.d.ts

@@ -10,7 +10,7 @@ import type {
   WhereOptions,
 } from '../../model.js';
 import type { QueryTypes } from '../../query-types.js';
-import type { Literal, SequelizeMethod } from '../../utils/sequelize-method.js';
+import type { Literal, SequelizeMethod, Col } from '../../utils/sequelize-method.js';
 import type { DataType } from './data-types.js';
 import type { QueryGeneratorOptions } from './query-generator-typescript.js';
 import { AbstractQueryGeneratorTypeScript } from './query-generator-typescript.js';
@@ -39,7 +39,7 @@ type InsertOptions = ParameterOptions & SearchPathable & {
   updateOnDuplicate?: string[],
   ignoreDuplicates?: boolean,
   upsertKeys?: string[],
-  returning?: boolean | string[],
+  returning?: boolean | Array<string | Literal | Col>,
 };
 
 type BulkInsertOptions = ParameterOptions & {
@@ -48,7 +48,7 @@ type BulkInsertOptions = ParameterOptions & {
   updateOnDuplicate?: string[],
   ignoreDuplicates?: boolean,
   upsertKeys?: string[],
-  returning?: boolean | string[],
+  returning?: boolean | Array<string | Literal | Col>,
 };
 
 type UpdateOptions = ParameterOptions & {
@@ -60,7 +60,7 @@ type DeleteOptions = ParameterOptions & {
 };
 
 type ArithmeticQueryOptions = ParameterOptions & {
-  returning?: boolean | string[],
+  returning?: boolean | Array<string | Literal | Col>,
 };
 
 export type WhereItemsQueryOptions = ParameterOptions & {

src/dialects/abstract/query-generator.js

@@ -55,13 +55,6 @@ export const REMOVE_COLUMN_QUERY_SUPPORTABLE_OPTIONS = new Set(['ifExists']);
  * @private
  */
 export class AbstractQueryGenerator extends AbstractQueryGeneratorTypeScript {
-  constructor(options) {
-    super(options);
-
-    // wrap quoteIdentifier with common logic
-    this._initQuoteIdentifier();
-  }
-
   createDatabaseQuery() {
     if (this.dialect.supports.multiDatabases) {
       throw new Error(`${this.dialect.name} declares supporting databases but createDatabaseQuery is not implemented.`);
@@ -439,7 +432,7 @@ export class AbstractQueryGenerator extends AbstractQueryGeneratorTypeScript {
       outputFragment = returnValues.outputFragment || '';
 
       // ensure that the return output is properly mapped to model fields.
-      if (!this.dialect.supports.returnValues.output && options.returning) {
+      if (this.dialect.supports.returnValues !== 'output' && options.returning) {
         options.mapToModel = true;
       }
     }
@@ -1013,20 +1006,13 @@ export class AbstractQueryGenerator extends AbstractQueryGeneratorTypeScript {
     throw new Error(`Unknown structure passed to order / group: ${util.inspect(collection)}`);
   }
 
-  _initQuoteIdentifier() {
-    this._quoteIdentifier = this.quoteIdentifier;
-    this.quoteIdentifier = function quoteIdentifier(identifier, force) {
-      if (identifier === '*') {
-        return identifier;
-      }
-
-      return this._quoteIdentifier(identifier, force);
-    };
-  }
-
   /**
    * Split a list of identifiers by "." and quote each part.
    *
+   * ⚠️ You almost certainly want to use `quoteIdentifier` instead!
+   * This method splits the identifier by "." into multiple identifiers, and has special meaning for "*".
+   * This behavior should never be the default and should be explicitly opted into by using {@link Col}.
+   *
    * @param {string} identifiers
    *
    * @returns {string}
@@ -1038,18 +1024,14 @@ export class AbstractQueryGenerator extends AbstractQueryGeneratorTypeScript {
       const head = identifiers.slice(0, -1).join('->');
       const tail = identifiers[identifiers.length - 1];
 
-      return `${this.quoteIdentifier(head)}.${this.quoteIdentifier(tail)}`;
+      return `${this.quoteIdentifier(head)}.${tail === '*' ? '*' : this.quoteIdentifier(tail)}`;
     }
 
-    return this.quoteIdentifier(identifiers);
-  }
-
-  quoteAttribute(attribute, model) {
-    if (model && attribute in model.rawAttributes) {
-      return this.quoteIdentifier(attribute);
+    if (identifiers === '*') {
+      return '*';
     }
 
-    return this.quoteIdentifiers(attribute);
+    return this.quoteIdentifier(identifiers);
   }
 
   /**
@@ -1517,10 +1499,8 @@ export class AbstractQueryGenerator extends AbstractQueryGeneratorTypeScript {
         if (attr[0] instanceof SequelizeMethod) {
           attr[0] = this.handleSequelizeMethod(attr[0], undefined, undefined, options);
           addTable = false;
-        } else if (!attr[0].includes('(') && !attr[0].includes(')')) {
-          attr[0] = this.quoteIdentifier(attr[0]);
         } else {
-          deprecations.noRawAttributes();
+          attr[0] = this.quoteIdentifier(attr[0]);
         }
 
         let alias = attr[1];
@@ -1531,10 +1511,7 @@ export class AbstractQueryGenerator extends AbstractQueryGeneratorTypeScript {
 
         attr = [attr[0], this.quoteIdentifier(alias)].join(' AS ');
       } else {
-        // TODO: attributes should always be escaped as identifiers, not escaped as strings
-        attr = !attr.includes(TICK_CHAR) && !attr.includes('"')
-          ? this.quoteAttribute(attr, options.model)
-          : this.escape(attr, undefined, options);
+        attr = this.quoteIdentifier(attr, options.model);
       }
 
       if (!_.isEmpty(options.include) && (!attr.includes('.') || options.dotNotation) && addTable) {
@@ -1869,8 +1846,26 @@ export class AbstractQueryGenerator extends AbstractQueryGeneratorTypeScript {
     let returningFragment = '';
     let tmpTable = '';
 
+    const returnValuesType = this.dialect.supports.returnValues;
+
     if (Array.isArray(options.returning)) {
-      returnFields.push(...options.returning.map(field => this.quoteIdentifier(field)));
+      returnFields.push(...options.returning.map(field => {
+        if (typeof field === 'string') {
+          return this.quoteIdentifier(field);
+        } else if (field instanceof Literal) {
+          // Due to how the mssql query is built, using a literal would never result in a properly formed query.
+          // It's better to warn early.
+          if (returnValuesType === 'output') {
+            throw new Error(`literal() cannot be used in the "returning" option array in ${this.dialect.name}. Use col(), or a string instead.`);
+          }
+
+          return this.handleSequelizeMethod(field);
+        } else if (field instanceof Col) {
+          return this.handleSequelizeMethod(field);
+        }
+
+        throw new Error(`Unsupported value in "returning" option: ${NodeUtil.inspect(field)}. This option only accepts true, false, or an array of strings, col() or literal().`);
+      }));
     } else if (modelAttributes) {
       _.each(modelAttributes, attribute => {
         if (!(attribute.type instanceof DataTypes.VIRTUAL)) {
@@ -1881,13 +1876,13 @@ export class AbstractQueryGenerator extends AbstractQueryGeneratorTypeScript {
     }
 
     if (_.isEmpty(returnFields)) {
-      returnFields.push('*');
+      returnFields.push(`*`);
     }
 
-    if (this.dialect.supports.returnValues.returning) {
-      returningFragment = ` RETURNING ${returnFields.join(',')}`;
-    } else if (this.dialect.supports.returnValues.output) {
-      outputFragment = ` OUTPUT ${returnFields.map(field => `INSERTED.${field}`).join(',')}`;
+    if (returnValuesType === 'returning') {
+      returningFragment = ` RETURNING ${returnFields.join(', ')}`;
+    } else if (returnValuesType === 'output') {
+      outputFragment = ` OUTPUT ${returnFields.map(field => `INSERTED.${field}`).join(', ')}`;
 
       // To capture output rows when there is a trigger on MSSQL DB
       if (options.hasTrigger && this.dialect.supports.tmpTableTrigger) {

src/dialects/abstract/query-interface.d.ts

@@ -14,7 +14,7 @@ import type {
 } from '../../model';
 import type { Sequelize, QueryRawOptions, QueryRawOptionsWithModel } from '../../sequelize';
 import type { Transaction } from '../../transaction';
-import type { Fn, Literal } from '../../utils/sequelize-method.js';
+import type { Fn, Literal, Col } from '../../utils/sequelize-method.js';
 import type { DataType } from './data-types.js';
 import type { TableNameOrModel } from './query-generator-typescript';
 import type { AbstractQueryGenerator, AddColumnQueryOptions, RemoveColumnQueryOptions } from './query-generator.js';
@@ -29,23 +29,23 @@ interface Replaceable {
 interface QiOptionsWithReplacements extends QueryRawOptions, Replaceable {}
 
 export interface QiInsertOptions extends QueryRawOptions, Replaceable {
-  returning?: boolean | string[];
+  returning?: boolean | Array<string | Literal | Col>;
 }
 
 export interface QiSelectOptions extends QueryRawOptions, Replaceable, Filterable<any> {
 
 }
 
 export interface QiUpdateOptions extends QueryRawOptions, Replaceable {
-  returning?: boolean | string[];
+  returning?: boolean | Array<string | Literal | Col>;
 }
 
 export interface QiDeleteOptions extends QueryRawOptions, Replaceable {
   limit?: number | Literal | null | undefined;
 }
 
 export interface QiArithmeticOptions extends QueryRawOptions, Replaceable {
-  returning?: boolean | string[];
+  returning?: boolean | Array<string | Literal | Col>;
 }
 
 export interface QiUpsertOptions<M extends Model> extends QueryRawOptionsWithModel<M>, Replaceable {

src/dialects/mssql/index.ts

@@ -13,9 +13,7 @@ export class MssqlDialect extends AbstractDialect {
     'DEFAULT VALUES': true,
     'LIMIT ON UPDATE': true,
     migrations: false,
-    returnValues: {
-      output: true,
-    },
+    returnValues: 'output',
     schemas: true,
     multiDatabases: true,
     autoIncrement: {

src/dialects/postgres/index.ts

@@ -14,9 +14,7 @@ export class PostgresDialect extends AbstractDialect {
     EXCEPTION: true,
     'ON DUPLICATE KEY': false,
     'ORDER NULLS': true,
-    returnValues: {
-      returning: true,
-    },
+    returnValues: 'returning',
     bulkDefault: true,
     schemas: true,
     multiDatabases: true,

src/model.d.ts

@@ -1038,7 +1038,7 @@ export interface CreateOptions<TAttributes = any>
   /**
    * Return the affected rows (only for postgres)
    */
-  returning?: boolean | Array<keyof TAttributes>;
+  returning?: boolean | Array<keyof TAttributes | Literal | Col>;
 
   /**
    * If false, validations won't be run.
@@ -1096,7 +1096,7 @@ export interface UpsertOptions<TAttributes = any> extends Logging, Transactionab
   /**
    * Fetch back the affected rows (only for postgres)
    */
-  returning?: boolean | Array<keyof TAttributes>;
+  returning?: boolean | Array<keyof TAttributes | Literal | Col>;
 
   /**
    * Run validations before the row is inserted
@@ -1157,7 +1157,7 @@ export interface BulkCreateOptions<TAttributes = any> extends Logging, Transacti
   /**
    * Return all columns or only the specified columns for the affected rows (only for postgres)
    */
-  returning?: boolean | Array<keyof TAttributes>;
+  returning?: boolean | Array<keyof TAttributes | Literal | Col>;
 }
 
 /**
@@ -1273,7 +1273,7 @@ export interface UpdateOptions<TAttributes = any> extends Logging, Transactionab
    *
    * @default false
    */
-  returning?: boolean | Array<keyof TAttributes>;
+  returning?: boolean | Array<keyof TAttributes | Literal | Col>;
 
   /**
    * How many rows to update
@@ -1326,7 +1326,7 @@ export interface IncrementDecrementOptions<TAttributes = any>
   /**
    * Return the affected rows (only for postgres)
    */
-  returning?: boolean | Array<keyof TAttributes>;
+  returning?: boolean | Array<keyof TAttributes | Literal | Col>;
 }
 
 /**
@@ -1406,7 +1406,7 @@ export interface SaveOptions<TAttributes = any> extends Logging, Transactionable
   /**
    * Return the affected rows (only for postgres)
    */
-  returning?: boolean | Array<keyof TAttributes>;
+  returning?: boolean | Array<keyof TAttributes | Literal | Col>;
 }
 
 /**

src/utils/deprecations.ts

@@ -2,7 +2,6 @@ import { deprecate } from 'node:util';
 
 const noop = () => { /* noop */ };
 
-export const noRawAttributes = deprecate(noop, 'Use sequelize.fn / sequelize.literal to construct attributes', 'SEQUELIZE0001');
 export const noTrueLogging = deprecate(noop, 'The logging-option should be either a function or false. Default: console.log', 'SEQUELIZE0002');
 export const noStringOperators = deprecate(noop, 'String based operators are deprecated. Please use Symbol based operators for better security, read more at https://sequelize.org/docs/v7/core-concepts/model-querying-basics/#deprecated-operator-aliases', 'SEQUELIZE0003');
 export const noBoolOperatorAliases = deprecate(noop, 'A boolean value was passed to options.operatorsAliases. This is a no-op with v5 and should be removed.', 'SEQUELIZE0004');

test/integration/data-types/methods.test.ts

@@ -162,7 +162,7 @@ describe('DataType Methods', () => {
     });
   }
 
-  if (dialect.supports.returnValues && dialect.supports.returnValues.returning) {
+  if (dialect.supports.returnValues === 'returning') {
     it(`updating a model calls 'parseDatabaseValue' on returned values`, async () => {
       const user = await models.User.create({ name: 'foo' });
       user.name = 'bob';

test/integration/instance/decrement.test.js

@@ -79,7 +79,7 @@ describe(Support.getTestDialectTeaser('Instance'), () => {
       });
     }
 
-    if (current.dialect.supports.returnValues.returning) {
+    if (current.dialect.supports.returnValues === 'returning') {
       it('supports returning', async function () {
         const user1 = await this.User.findByPk(1);
         await user1.decrement('aNumber', { by: 2 });

test/integration/instance/increment.test.js

@@ -80,7 +80,7 @@ describe(Support.getTestDialectTeaser('Instance'), () => {
       });
     }
 
-    if (current.dialect.supports.returnValues.returning) {
+    if (current.dialect.supports.returnValues === 'returning') {
       it('supports returning', async function () {
         const user1 = await this.User.findByPk(1);
         await user1.increment('aNumber', { by: 2 });

test/integration/model.test.js

@@ -4,7 +4,7 @@ const chai = require('chai');
 
 const expect = chai.expect;
 const Support = require('./support');
-const { DataTypes, Sequelize, Op, AggregateError } = require('@sequelize/core');
+const { DataTypes, Sequelize, Op, AggregateError, col } = require('@sequelize/core');
 
 const dialectName = Support.getTestDialect();
 const dialect = Support.sequelize.dialect;
@@ -1054,7 +1054,7 @@ describe(Support.getTestDialectTeaser('Model'), () => {
             ibmi: `UPDATE "users1" SET "secretValue"=?,"updatedAt"=? WHERE "id" = ?;`,
           });
         },
-        returning: ['*'],
+        returning: [col('*')],
       });
       expect(test).to.be.true;
     });

test/integration/model/bulk-create.test.js

@@ -4,7 +4,7 @@ const chai = require('chai');
 
 const expect = chai.expect;
 const Support = require('../support');
-const { DataTypes, Op } = require('@sequelize/core');
+const { DataTypes, Op, col } = require('@sequelize/core');
 
 const dialectName = Support.getTestDialect();
 const dialect = Support.sequelize.dialect;
@@ -845,7 +845,7 @@ describe(Support.getTestDialectTeaser('Model'), () => {
             {},
             {},
           ], {
-            returning: ['*'],
+            returning: [col('*')],
           });
 
           const actualUsers0 = await User.findAll();

test/integration/model/update.test.js

@@ -7,7 +7,6 @@ const sinon = require('sinon');
 
 const expect = chai.expect;
 const current = Support.sequelize;
-const _ = require('lodash');
 
 describe(Support.getTestDialectTeaser('Model'), () => {
   describe('update', () => {
@@ -118,7 +117,7 @@ describe(Support.getTestDialectTeaser('Model'), () => {
       expect(account.ownerId).to.be.equal(ownerId);
     });
 
-    if (_.get(current.dialect.supports, 'returnValues.returning')) {
+    if (current.dialect.supports.returnValues === 'returning') {
       it('should return the updated record', async function () {
         const account = await this.Account.create({ ownerId: 2 });
 
@@ -135,7 +134,7 @@ describe(Support.getTestDialectTeaser('Model'), () => {
       });
     }
 
-    if (_.get(current.dialect.supports, 'returnValues.output')) {
+    if (current.dialect.supports.returnValues === 'output') {
       it('should output the updated record', async function () {
         const account = await this.Account.create({ ownerId: 2 });