From af1bd14383a60c9b3a2d99cca06310e60e15044d Mon Sep 17 00:00:00 2001 From: Ansh Date: Wed, 18 Mar 2026 01:29:44 +0530 Subject: [PATCH] fix: propagate fatal flag across all promise wrapper error paths MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The promise wrapper layer had two related issues: 1. fatal flag was silently dropped in ping(), connect(), prepare(), changeUser() in PromiseConnection, and end() in PromisePool. These methods manually copied a fixed set of error properties (message, code, errno, sqlState, sqlMessage) but omitted fatal, meaning promise clients could not distinguish fatal errors from non-fatal ones — unlike callback clients which always receive it. 2. The same manual copy block was duplicated across 5 methods with no shared abstraction, making it easy to miss properties (as demonstrated by the missing fatal flag). Fix: extract assignError() helper in make_done_cb.js that copies all error properties including fatal, and use it in all promise wrapper error paths. Also fixes PromisePool.execute args check (if (args) -> if (args !== undefined)) to be consistent with PromisePool.query. --- lib/promise/connection.js | 25 ++---- lib/promise/make_done_cb.js | 18 ++-- lib/promise/pool.js | 9 +- ...omise-connection-error-properties.test.mts | 82 +++++++++++++++++++ 4 files changed, 102 insertions(+), 32 deletions(-) create mode 100644 test/integration/promise-wrappers/test-promise-connection-error-properties.test.mts diff --git a/lib/promise/connection.js b/lib/promise/connection.js index 75ccea5e07..72aa7d1d39 100644 --- a/lib/promise/connection.js +++ b/lib/promise/connection.js @@ -3,6 +3,7 @@ const EventEmitter = require('events').EventEmitter; const PromisePreparedStatementInfo = require('./prepared_statement_info.js'); const makeDoneCb = require('./make_done_cb.js'); +const { assignError } = makeDoneCb; const inheritEvents = require('./inherit_events.js'); const BaseConnection = require('../base/connection.js'); @@ -105,11 +106,7 @@ class PromiseConnection extends EventEmitter { return new this.Promise((resolve, reject) => { c.ping((err) => { if (err) { - localErr.message = err.message; - localErr.code = err.code; - localErr.errno = err.errno; - localErr.sqlState = err.sqlState; - localErr.sqlMessage = err.sqlMessage; + assignError(localErr, err); reject(localErr); } else { resolve(true); @@ -124,11 +121,7 @@ class PromiseConnection extends EventEmitter { return new this.Promise((resolve, reject) => { c.connect((err, param) => { if (err) { - localErr.message = err.message; - localErr.code = err.code; - localErr.errno = err.errno; - localErr.sqlState = err.sqlState; - localErr.sqlMessage = err.sqlMessage; + assignError(localErr, err); reject(localErr); } else { resolve(param); @@ -144,11 +137,7 @@ class PromiseConnection extends EventEmitter { return new this.Promise((resolve, reject) => { c.prepare(options, (err, statement) => { if (err) { - localErr.message = err.message; - localErr.code = err.code; - localErr.errno = err.errno; - localErr.sqlState = err.sqlState; - localErr.sqlMessage = err.sqlMessage; + assignError(localErr, err); reject(localErr); } else { const wrappedStatement = new PromisePreparedStatementInfo( @@ -167,11 +156,7 @@ class PromiseConnection extends EventEmitter { return new this.Promise((resolve, reject) => { c.changeUser(options, (err) => { if (err) { - localErr.message = err.message; - localErr.code = err.code; - localErr.errno = err.errno; - localErr.sqlState = err.sqlState; - localErr.sqlMessage = err.sqlMessage; + assignError(localErr, err); reject(localErr); } else { resolve(); diff --git a/lib/promise/make_done_cb.js b/lib/promise/make_done_cb.js index 124303f256..557164e61d 100644 --- a/lib/promise/make_done_cb.js +++ b/lib/promise/make_done_cb.js @@ -1,14 +1,19 @@ 'use strict'; +function assignError(localErr, err) { + localErr.message = err.message; + localErr.code = err.code; + localErr.errno = err.errno; + localErr.fatal = err.fatal; + localErr.sql = err.sql; + localErr.sqlState = err.sqlState; + localErr.sqlMessage = err.sqlMessage; +} + function makeDoneCb(resolve, reject, localErr) { return function (err, rows, fields) { if (err) { - localErr.message = err.message; - localErr.code = err.code; - localErr.errno = err.errno; - localErr.sql = err.sql; - localErr.sqlState = err.sqlState; - localErr.sqlMessage = err.sqlMessage; + assignError(localErr, err); reject(localErr); } else { resolve([rows, fields]); @@ -17,3 +22,4 @@ function makeDoneCb(resolve, reject, localErr) { } module.exports = makeDoneCb; +module.exports.assignError = assignError; diff --git a/lib/promise/pool.js b/lib/promise/pool.js index 24a8ca93c2..97275a1859 100644 --- a/lib/promise/pool.js +++ b/lib/promise/pool.js @@ -2,6 +2,7 @@ const EventEmitter = require('events').EventEmitter; const makeDoneCb = require('./make_done_cb.js'); +const { assignError } = makeDoneCb; const PromisePoolConnection = require('./pool_connection.js'); const inheritEvents = require('./inherit_events.js'); const BasePool = require('../base/pool.js'); @@ -59,7 +60,7 @@ class PromisePool extends EventEmitter { } return new this.Promise((resolve, reject) => { const done = makeDoneCb(resolve, reject, localErr); - if (args) { + if (args !== undefined) { corePool.execute(sql, args, done); } else { corePool.execute(sql, done); @@ -73,11 +74,7 @@ class PromisePool extends EventEmitter { return new this.Promise((resolve, reject) => { corePool.end((err) => { if (err) { - localErr.message = err.message; - localErr.code = err.code; - localErr.errno = err.errno; - localErr.sqlState = err.sqlState; - localErr.sqlMessage = err.sqlMessage; + assignError(localErr, err); reject(localErr); } else { resolve(); diff --git a/test/integration/promise-wrappers/test-promise-connection-error-properties.test.mts b/test/integration/promise-wrappers/test-promise-connection-error-properties.test.mts new file mode 100644 index 0000000000..8d0d9db85a --- /dev/null +++ b/test/integration/promise-wrappers/test-promise-connection-error-properties.test.mts @@ -0,0 +1,82 @@ +import { describe, it, strict } from 'poku'; +import { createConnection } from '../../common.test.mjs'; + +type MysqlError = Error & { + code?: string; + errno?: number; + fatal?: boolean; + sqlState?: string; + sqlMessage?: string; +}; + +await describe('promise error propagation: fatal flag in connection methods', async () => { + await it('ping rejection should carry fatal flag', async () => { + // Connect to an invalid host so ping fails with a fatal error + const conn = createConnection({ host: '127.0.0.1', port: 1 }).promise(); + + let caughtErr: MysqlError | undefined; + try { + await conn.ping(); + } catch (err) { + caughtErr = err as MysqlError; + } + + strict.ok(caughtErr, 'Expected ping to throw'); + strict.ok('code' in caughtErr!, 'code should be propagated'); + strict.equal( + caughtErr!.fatal, + true, + 'fatal should be true for connection errors' + ); + }); + + await it('connect rejection should carry fatal flag', async () => { + const conn = createConnection({ host: '127.0.0.1', port: 1 }).promise(); + + let caughtErr: MysqlError | undefined; + try { + await conn.connect(); + } catch (err) { + caughtErr = err as MysqlError; + } + + strict.ok(caughtErr, 'Expected connect to throw'); + strict.ok('code' in caughtErr!, 'code should be propagated'); + strict.equal( + caughtErr!.fatal, + true, + 'fatal should be true for connection errors' + ); + }); +}); + +await describe('promise error propagation: fatal flag in prepare and changeUser', async () => { + const conn = createConnection().promise(); + + await it('prepare rejection should propagate error properties', async () => { + let caughtErr: MysqlError | undefined; + try { + await conn.prepare('SELECT * FROM nonexistent_table_xyz_abc'); + } catch (err) { + caughtErr = err as MysqlError; + } + strict.ok(caughtErr, 'Expected prepare to throw'); + strict.ok('errno' in caughtErr!, 'errno should be propagated'); + strict.ok('code' in caughtErr!, 'code should be propagated'); + strict.ok('sqlMessage' in caughtErr!, 'sqlMessage should be propagated'); + }); + + await it('changeUser rejection should propagate error properties', async () => { + let caughtErr: MysqlError | undefined; + try { + await conn.changeUser({ user: 'nonexistent_user_xyz' }); + } catch (err) { + caughtErr = err as MysqlError; + } + strict.ok(caughtErr, 'Expected changeUser to throw'); + strict.ok('errno' in caughtErr!, 'errno should be propagated'); + strict.ok('code' in caughtErr!, 'code should be propagated'); + }); + + await conn.end(); +});