Repository: knex/knex
Pull request #5282 (2022-07-28 07:24:02)

Make compiling SQL in error message optional 
Hello! 

**Description**
Currently the only and default behaviour of Knex is to add compiled SQL query into the error message, while it is good for debugging and visibility purposes, it also has a major drawback that it may leak sensitive information to logs.

**Changes**
- Created a new configuration option: `compileSqlOnError` ([Documentation PR](https://github.com/knex/documentation/pull/436)).
- When `compileSqlOnError` is true, then compiled SQL will be added to error, otherwise parameterized SQL will be added

**Default value**
By default the parameterized SQL will be added to the error message instead of the compiled SQL as it is now. Why did I make it default to parameterized? Because not having compiled SQL come up in error messages by default seems to be a much safer option, as it avoids leaking sensitive information to the logs, If a developer needs to debug anything then there is always the option to turn it on by enabling `compileSqlOnError`

**Extra notes**
Please let me know if you have any remarks about this and if you think the default behaviour should be the opposite, I'm sure we can find a way integrate this option into Knex as this is something that can save leaking someones information.

kibertoad: it would be a semver major if default is "no"
coveralls: 
[![Coverage Status](https://coveralls.io/builds/51287525/badge)](https://coveralls.io/builds/51287525)

Coverage decreased (-0.04%) to 92.228% when pulling **436be4251b83c6d9925d11a2af45bb3c8eee58f7 on JakobJoonas:hide-sensitive-data** into **72065c179772c18dc4707a1dfd5990ec8b09fd46 on knex:master**.

JakobJoonas: Thanks for the answer, here I'd like to get your feedback about this as maintainers.

I would prefer it to be "no" as default because it is a much safer option and when I get your validation about this I can update the tests to not fail. However I do understand that this requires a major version increase, I have not contributed a lot to open source projects, would I have to do anything to make the major version happen or is this part handled by the maintainers?
JakobJoonas: @kibertoad or @OlivierCavadenti what would you think about making this a major upgrade as mentioned above?
JakobJoonas: Okay, to move on with this, I made the field to be true as default, so no major upgrade would be needed, but this is something that I think should be though of with the next major upgrade :)
OlivierCavadenti: hi, seems you still have one test fail on postgres.
JakobJoonas: > 

Indeed it was, tests had a bit different setup, should be fixed now, tests pass on local machine

Patch:
diff --git a/lib/execution/internal/query-executioner.js b/lib/execution/internal/query-executioner.js
index 66b3b20c..8ca97843 100644
--- a/lib/execution/internal/query-executioner.js
+++ b/lib/execution/internal/query-executioner.js
@@ -35,10 +35,14 @@ function enrichQueryObject(connection, queryParam, client) {
 
 function executeQuery(connection, queryObject, client) {
   return client._query(connection, queryObject).catch((err) => {
-    err.message =
-      formatQuery(queryObject.sql, queryObject.bindings, undefined, client) +
-      ' - ' +
-      err.message;
+    if (client.config && client.config.compileSqlOnError === false) {
+      err.message = queryObject.sql + ' - ' + err.message;
+    } else {
+      err.message =
+        formatQuery(queryObject.sql, queryObject.bindings, undefined, client) +
+        ' - ' +
+        err.message;
+    }
     client.emit(
       'query-error',
       err,
diff --git a/package.json b/package.json
index 74206563..cad0bb17 100644
--- a/package.json
+++ b/package.json
@@ -141,8 +141,8 @@
     "tedious": "^14.4.0",
     "toxiproxy-node-client": "^2.0.6",
     "ts-node": "^10.9.1",
-    "tsd": "^0.23.0",
-    "typescript": "4.8.2"
+    "tsd": "^0.24.1",
+    "typescript": "4.8.3"
   },
   "buildDependencies": [
     "rimraf"
diff --git a/types/index.d.ts b/types/index.d.ts
index b246bbe5..a0e308f4 100644
--- a/types/index.d.ts
+++ b/types/index.d.ts
@@ -2717,6 +2717,7 @@ export declare namespace Knex {
     searchPath?: string | readonly string[];
     asyncStackTraces?: boolean;
     log?: Logger;
+    compileSqlOnError?: boolean;
   }
 
   type StaticConnectionConfig =


Test patch:
diff --git a/test/unit/knex.js b/test/unit/knex.js
index a5bf1886..aa030487 100644
--- a/test/unit/knex.js
+++ b/test/unit/knex.js
@@ -753,6 +753,44 @@ describe('knex', () => {
       expect(errorArgs.queryContext).to.equal(context);
     });
 
+    it('should show compiled sql on error message when compileSqlOnError is true', async function () {
+      const spy = sinon.spy();
+      const knex = Knex({ ...sqliteConfig, compileSqlOnError: true })
+        .from('test')
+        .on('query-error', spy);
+
+      try {
+        await knex.insert({ foo: 'bar' });
+        // eslint-disable-next-line no-empty
+      } catch (_e) {}
+
+      expect(spy).to.be.calledOnce;
+      const [[error]] = spy.args;
+      expect(error).to.be.instanceOf(Error);
+      expect(error.message).to.equal(
+        "insert into `test` (`foo`) values ('bar') - SQLITE_ERROR: no such table: test"
+      );
+    });
+
+    it('should show parameterized sql on error message when compileSqlOnError is false', async function () {
+      const spy = sinon.spy();
+      const knex = Knex({ ...sqliteConfig, compileSqlOnError: false })
+        .from('test')
+        .on('query-error', spy);
+
+      try {
+        await knex.insert({ foo: 'bar' });
+        // eslint-disable-next-line no-empty
+      } catch (_e) {}
+
+      expect(spy).to.be.calledOnce;
+      const [[error]] = spy.args;
+      expect(error).to.be.instanceOf(Error);
+      expect(error.message).to.equal(
+        'insert into `test` (`foo`) values (?) - SQLITE_ERROR: no such table: test'
+      );
+    });
+
     // TODO: Consider moving these somewhere that tests the
     //       QueryBuilder interface more directly.
     context('qb = knex.select(1)', function () {