From 67f8553d20656db453a1b03d0b8aebfb9fde2cfb Mon Sep 17 00:00:00 2001 From: vansh Date: Mon, 13 Jun 2022 18:42:15 +0530 Subject: [PATCH] fix: sequelize should take transaction from CLS everywhere --- src/model-internals.ts | 12 +++++++----- src/model.js | 26 +++++++++++++------------- test/integration/cls.test.js | 14 +++++++++++--- 3 files changed, 31 insertions(+), 21 deletions(-) diff --git a/src/model-internals.ts b/src/model-internals.ts index f63a28807820..b3fbef795adc 100644 --- a/src/model-internals.ts +++ b/src/model-internals.ts @@ -1,7 +1,9 @@ import NodeUtil from 'util'; import { EagerLoadingError } from './errors'; +import type { Transactionable } from './model'; +import type { Sequelize } from './sequelize'; import { isModelStatic } from './utils/model-utils.js'; - +import type { Transaction } from './index'; // TODO: strictly type this file during the TS migration of model.js // The goal of this file is to include the different private methods that are currently present on the Model class. @@ -146,11 +148,11 @@ export function throwInvalidInclude(include: any): never { Got ${NodeUtil.inspect(include)} instead`); } -export function setTransactionFromCls(options: any, sequelize: any) { - if (options.transaction === undefined && sequelize.constructor._cls) { - const t = sequelize.constructor._cls.get('transaction'); +export function setTransactionFromCls(options: Transactionable, sequelize: Sequelize): void { + if (options.transaction === undefined && sequelize.Sequelize._cls) { + const t = sequelize.Sequelize._cls.get('transaction'); if (t) { - options.transaction = t; + options.transaction = t as Transaction; } } } diff --git a/src/model.js b/src/model.js index a93fdc11bb90..9f4e8624662c 100644 --- a/src/model.js +++ b/src/model.js @@ -1366,7 +1366,7 @@ Specify a different name for either index to resolve this issue.`); indexes = this.getIndexes().filter(item1 => !indexes.some(item2 => item1.name === item2.name)).sort((index1, index2) => { if (this.sequelize.options.dialect === 'postgres') { - // move concurrent indexes to the bottom to avoid weird deadlocks + // move concurrent indexes to the bottom to avoid weird deadlocks if (index1.concurrently === true) { return 1; } @@ -1942,7 +1942,7 @@ Specify a different name for either index to resolve this issue.`); // Don't add limit if querying directly on the pk or a unique column if (!options.where || !_.some(options.where, (value, key) => (key === this.primaryKeyAttribute || uniqueSingleColumns.includes(key)) - && (Utils.isPrimitive(value) || Buffer.isBuffer(value)))) { + && (Utils.isPrimitive(value) || Buffer.isBuffer(value)))) { options.limit = 1; } } @@ -2834,9 +2834,9 @@ Specify a different name for either index to resolve this issue.`); if (associationInstance[include.association.through.model.name]) { for (const attr of Object.keys(include.association.through.model.rawAttributes)) { if (include.association.through.model.rawAttributes[attr]._autoGenerated - || attr === include.association.foreignKey - || attr === include.association.otherKey - || typeof associationInstance[include.association.through.model.name][attr] === 'undefined') { + || attr === include.association.foreignKey + || attr === include.association.otherKey + || typeof associationInstance[include.association.through.model.name][attr] === 'undefined') { continue; } @@ -2866,8 +2866,8 @@ Specify a different name for either index to resolve this issue.`); for (const instance of instances) { for (const attr in model.rawAttributes) { if (model.rawAttributes[attr].field - && instance.dataValues[model.rawAttributes[attr].field] !== undefined - && model.rawAttributes[attr].field !== attr + && instance.dataValues[model.rawAttributes[attr].field] !== undefined + && model.rawAttributes[attr].field !== attr ) { instance.dataValues[attr] = instance.dataValues[model.rawAttributes[attr].field]; delete instance.dataValues[model.rawAttributes[attr].field]; @@ -3214,7 +3214,7 @@ Specify a different name for either index to resolve this issue.`); if (updateDoneRowByRow) { result = [instances.length, instances]; } else if (_.isEmpty(valuesUse) - || Object.keys(valuesUse).length === 1 && valuesUse[this._timestampAttributes.updatedAt]) { + || Object.keys(valuesUse).length === 1 && valuesUse[this._timestampAttributes.updatedAt]) { // only updatedAt is being passed, then skip update result = [0]; } else { @@ -4117,8 +4117,8 @@ Instead of specifying a Model, either: // Transfer database generated values (defaults, autoincrement, etc) for (const attr of Object.keys(this.constructor.rawAttributes)) { if (this.constructor.rawAttributes[attr].field - && values[this.constructor.rawAttributes[attr].field] !== undefined - && this.constructor.rawAttributes[attr].field !== attr + && values[this.constructor.rawAttributes[attr].field] !== undefined + && this.constructor.rawAttributes[attr].field !== attr ) { values[attr] = values[this.constructor.rawAttributes[attr].field]; delete values[this.constructor.rawAttributes[attr].field]; @@ -4165,9 +4165,9 @@ Instead of specifying a Model, either: if (instance[include.association.through.model.name]) { for (const attr of Object.keys(include.association.through.model.rawAttributes)) { if (include.association.through.model.rawAttributes[attr]._autoGenerated - || attr === include.association.foreignKey - || attr === include.association.otherKey - || typeof instance[include.association.through.model.name][attr] === 'undefined') { + || attr === include.association.foreignKey + || attr === include.association.otherKey + || typeof instance[include.association.through.model.name][attr] === 'undefined') { continue; } diff --git a/test/integration/cls.test.js b/test/integration/cls.test.js index 0c38e6598fef..479af98814e8 100644 --- a/test/integration/cls.test.js +++ b/test/integration/cls.test.js @@ -186,16 +186,24 @@ if (current.dialect.supports.transactions) { }, }); - // hooks: ['afterFind'], failing in this but the transaction is coming testHooks({ method: 'Model.findAll', - hooks: ['beforeFind', 'beforeFindAfterExpandIncludeAll', 'beforeFindAfterOptions', 'afterFind'], + hooks: ['beforeFind', 'beforeFindAfterExpandIncludeAll', 'beforeFindAfterOptions'], optionPos: 0, async execute(User) { await User.findAll(); }, }); + testHooks({ + method: 'Model.findAll', + hooks: ['afterFind'], + optionPos: 1, + async execute(User) { + await User.findAll(); + }, + }); + testHooks({ method: 'Model.count', hooks: ['beforeCount'], @@ -286,7 +294,6 @@ if (current.dialect.supports.transactions) { }, }); - // these paranoid test getting failed the transaction is going in the runhook function but still the test is failing describe('paranoid restore', () => { beforeEach(async function () { this.ParanoidUser = this.sequelize.define('paranoid_user', { @@ -294,6 +301,7 @@ if (current.dialect.supports.transactions) { }, { paranoid: true }); await this.ParanoidUser.sync({ force: true }); + this.User = this.ParanoidUser; }); testHooks({