From d7f9999849149b6c1fa2af37eb4b39d6cb1deff1 Mon Sep 17 00:00:00 2001 From: Mark Duncan Date: Tue, 22 Nov 2022 08:17:43 +0000 Subject: [PATCH 1/5] More verbose undefined parameter messages Adds list of undefined named parameters/indexes to error message --- lib/connection.js | 31 +++++----- package.json | 3 +- .../test-execute-undefined-errors.js | 57 +++++++++++++++++++ 3 files changed, 77 insertions(+), 14 deletions(-) create mode 100644 test/integration/connection/test-execute-undefined-errors.js diff --git a/lib/connection.js b/lib/connection.js index 56153d28eb..02db06d9e1 100644 --- a/lib/connection.js +++ b/lib/connection.js @@ -550,6 +550,8 @@ class Connection extends EventEmitter { if (convertNamedPlaceholders === null) { convertNamedPlaceholders = require('named-placeholders')(); } + // Store list of named parameters in the order they appear in the sql so we can reference against value array if needed + options.names = options.sql.match(/:\w*/g); unnamed = convertNamedPlaceholders(options.sql, options.values); options.sql = unnamed[0]; options.values = unnamed[1]; @@ -640,25 +642,28 @@ class Connection extends EventEmitter { 'Bind parameters must be array if namedPlaceholders parameter is not enabled' ); } - options.values.forEach(val => { - //If namedPlaceholder is not enabled and object is passed as bind parameters - if (!Array.isArray(options.values)) { - throw new TypeError( - 'Bind parameters must be array if namedPlaceholders parameter is not enabled' - ); - } - if (val === undefined) { - throw new TypeError( - 'Bind parameters must not contain undefined. To pass SQL NULL specify JS null' - ); - } + if (typeof val === 'function') { throw new TypeError( 'Bind parameters must not contain function(s). To pass the body of a function as a string call .toString() first' ); } - }); + + if (options.names) { + // Create deduped list of parameter names that are undefined by cross referencing the named list with the values array + const undefinedParams = options.names.filter((n, i, a) => a.indexOf(n) === i && options.values[i] === undefined).join(', '); + if(undefinedParams.length) { + throw new TypeError(`Bind parameters must not contain undefined (parameters ${undefinedParams}). To pass SQL NULL specify JS null`); + } + } else { + // Create list of the indexes of any undefined values + const undefinedIndexes = options.values.reduce((acc, v, i) => v === undefined ? acc.concat(i) : acc, []).join(', '); + if (undefinedIndexes.length) { + throw new TypeError(`Bind parameters must not contain undefined (indexes ${undefinedIndexes}). To pass SQL NULL specify JS null`); + } + } } + const executeCommand = new Commands.Execute(options, cb); const prepareCommand = new Commands.Prepare(options, (err, stmt) => { if (err) { diff --git a/package.json b/package.json index 790a474ce5..929ed96515 100644 --- a/package.json +++ b/package.json @@ -76,6 +76,7 @@ "assert-diff": "^3.0.2", "benchmark": "^2.1.4", "c8": "^7.10.0", + "cross-env": "^7.0.3", "error-stack-parser": "^2.0.3", "eslint": "^8.27.0", "eslint-plugin-async-await": "0.0.0", @@ -90,4 +91,4 @@ "urun": "0.0.8", "utest": "0.0.8" } -} +} \ No newline at end of file diff --git a/test/integration/connection/test-execute-undefined-errors.js b/test/integration/connection/test-execute-undefined-errors.js new file mode 100644 index 0000000000..84d7997e56 --- /dev/null +++ b/test/integration/connection/test-execute-undefined-errors.js @@ -0,0 +1,57 @@ +'use strict'; + +const createConnection = require('../../common.js').createConnection; +const createPool = require('../../common.js').createPool; +const test = require('utest'); +const assert = require('assert'); + +test('Test error messages for undefined parameters are correctly reported ', { + 'Error message lists named parameter that was undefined': () => { + const conn = createConnection({namedPlaceholders: true}); + try { + conn.execute({sql: 'select id, email from test_table where id = :id and email = :email and name = :name'}, {email: 'test@email.com'}, err => { + assert.fail(`Expected error to be thrown, but got ${err}`); + }); + } catch (err) { + if (err.message.indexOf("(parameters :id, :name)") === -1) { + assert.fail( + `Expected error message to list undefined named parameter (:id):\n ${err}` + ); + } + } finally { + conn.end(); + } + }, + 'Error message lists undefined named parameter once if it appears multiple times in the query': () => { + const conn = createConnection({namedPlaceholders: true}); + try { + conn.execute({sql: 'select id, email from test_table where id = :id and created < :day and created > :day - interval 7 day'}, {}, err => { + assert.fail(`Expected error to be thrown, but got ${err}`); + }); + } catch (err) { + if (err.message.indexOf("(parameters :id, :day)") === -1) { + assert.fail( + `Expected error message to list undefined named parameter (:id):\n ${err}` + ); + } + } finally { + conn.end(); + } + }, + 'Error message lists parameter indexes that were undefined': () => { + const conn = createConnection({namedPlaceholders: true}); + try { + conn.execute({sql: 'select id, email from test_table where id = ?, email = ?, name = ?'}, [undefined, 'test@test.com', undefined], (err, row) => { + assert.fail(`Expected error to be thrown, but got ${err}`); + }); + } catch (err) { + if (err.message.indexOf("(indexes 0, 2)") === -1) { + assert.fail( + `Expected error message to list undefined parameter indexes (0,2):\n ${err}` + ); + } + } finally { + conn.end(); + } + } +}); \ No newline at end of file From 3b01c7c569ada9723971de3c4850942998502c6b Mon Sep 17 00:00:00 2001 From: Mark Duncan Date: Tue, 22 Nov 2022 08:34:19 +0000 Subject: [PATCH 2/5] removed cross-env was only using for my convenience, probably better to just remove --- package.json | 1 - 1 file changed, 1 deletion(-) diff --git a/package.json b/package.json index 929ed96515..6e7f74b03a 100644 --- a/package.json +++ b/package.json @@ -76,7 +76,6 @@ "assert-diff": "^3.0.2", "benchmark": "^2.1.4", "c8": "^7.10.0", - "cross-env": "^7.0.3", "error-stack-parser": "^2.0.3", "eslint": "^8.27.0", "eslint-plugin-async-await": "0.0.0", From 4298b12f2ecd73616e0be309b533183880f26a5d Mon Sep 17 00:00:00 2001 From: Mark Duncan Date: Tue, 22 Nov 2022 09:11:04 +0000 Subject: [PATCH 3/5] Fixed linting issues --- lib/connection.js | 12 ++++++------ .../connection/test-execute-undefined-errors.js | 1 - 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/lib/connection.js b/lib/connection.js index 02db06d9e1..0b4dcb8db2 100644 --- a/lib/connection.js +++ b/lib/connection.js @@ -643,18 +643,18 @@ class Connection extends EventEmitter { ); } - if (typeof val === 'function') { - throw new TypeError( - 'Bind parameters must not contain function(s). To pass the body of a function as a string call .toString() first' - ); - } + if (typeof val === 'function') { + throw new TypeError( + 'Bind parameters must not contain function(s). To pass the body of a function as a string call .toString() first' + ); + } if (options.names) { // Create deduped list of parameter names that are undefined by cross referencing the named list with the values array const undefinedParams = options.names.filter((n, i, a) => a.indexOf(n) === i && options.values[i] === undefined).join(', '); if(undefinedParams.length) { throw new TypeError(`Bind parameters must not contain undefined (parameters ${undefinedParams}). To pass SQL NULL specify JS null`); - } + } } else { // Create list of the indexes of any undefined values const undefinedIndexes = options.values.reduce((acc, v, i) => v === undefined ? acc.concat(i) : acc, []).join(', '); diff --git a/test/integration/connection/test-execute-undefined-errors.js b/test/integration/connection/test-execute-undefined-errors.js index 84d7997e56..dac4f048c8 100644 --- a/test/integration/connection/test-execute-undefined-errors.js +++ b/test/integration/connection/test-execute-undefined-errors.js @@ -1,7 +1,6 @@ 'use strict'; const createConnection = require('../../common.js').createConnection; -const createPool = require('../../common.js').createPool; const test = require('utest'); const assert = require('assert'); From 0ee22efc35124399d8c4c25fe8f38259ee65c065 Mon Sep 17 00:00:00 2001 From: Mark Duncan Date: Tue, 22 Nov 2022 16:54:31 +0000 Subject: [PATCH 4/5] Fix: now correctly errors when values contains function. --- lib/connection.js | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/lib/connection.js b/lib/connection.js index 0b4dcb8db2..467405593f 100644 --- a/lib/connection.js +++ b/lib/connection.js @@ -642,13 +642,9 @@ class Connection extends EventEmitter { 'Bind parameters must be array if namedPlaceholders parameter is not enabled' ); } - - if (typeof val === 'function') { - throw new TypeError( - 'Bind parameters must not contain function(s). To pass the body of a function as a string call .toString() first' - ); + if(options.values.some(v => typeof v === 'function')) { + throw new TypeError('Bind parameters must not contain function(s). To pass the body of a function as a string call .toString() first'); } - if (options.names) { // Create deduped list of parameter names that are undefined by cross referencing the named list with the values array const undefinedParams = options.names.filter((n, i, a) => a.indexOf(n) === i && options.values[i] === undefined).join(', '); From eb3f54cb355d291d8a192e304528471430fb3d8a Mon Sep 17 00:00:00 2001 From: Mark Duncan Date: Tue, 22 Nov 2022 20:52:55 +0000 Subject: [PATCH 5/5] fixed linting error --- test/integration/connection/test-execute-undefined-errors.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/integration/connection/test-execute-undefined-errors.js b/test/integration/connection/test-execute-undefined-errors.js index dac4f048c8..d0fc3e5287 100644 --- a/test/integration/connection/test-execute-undefined-errors.js +++ b/test/integration/connection/test-execute-undefined-errors.js @@ -40,7 +40,7 @@ test('Test error messages for undefined parameters are correctly reported ', { 'Error message lists parameter indexes that were undefined': () => { const conn = createConnection({namedPlaceholders: true}); try { - conn.execute({sql: 'select id, email from test_table where id = ?, email = ?, name = ?'}, [undefined, 'test@test.com', undefined], (err, row) => { + conn.execute({sql: 'select id, email from test_table where id = ?, email = ?, name = ?'}, [undefined, 'test@test.com', undefined], err => { assert.fail(`Expected error to be thrown, but got ${err}`); }); } catch (err) {