include passed bindings when "Error: Undefined binding(s)" is thrown

[acarl005]

We've had issues in our code which causes this error:

Error: Undefined binding(s) detected when compiling RAW query
<SQL query with some ? bindings>

We know the problem is one of the values of the bindings is undefined. However, we don't know which one. It would improve the debugging experience if the error message that was thrown includes the values of the bindings. Something like this perhaps:

Error: Undefined binding(s) detected when compiling RAW query
<SQL query with some ? bindings>
bindings: [ 'foo', 'bar', undefined, 1 ]

Then we would know which one of our bindings was the culprit.

[richraid21]

I'm not sure this is a good idea. Exposing possible sensitive information through errors/logs should be avoided.

Of course, open for discussion!

[maxstoyanov]

This behavior could be enabled with the debug switch. Since debug mode is usually not engaged in production environments important information would not be leaked into logs.

[richraid21]

KNEX_MODE= dev | prod ?

[acarl005]

Agreed that persisting the bindings could expose sensitive data. It certainly should not be the default behavior. Perhaps something like DEBUG=knex:bindings to enable it?

[wubzz]

Definitely only under a debug flag.

Currently DEBUG=knex:bindings logs the bindings: https://github.com/tgriesser/knex/blob/master/src/query/compiler.js#L75

I think that's a bit stale and should also include the sql.

There are two additional sources of information that can be used:

Fully compiled SQL: Use this.client._formatQuery(val.sql, defaults.bindings) (+Add support here to preserve the undefined value)

Column name: Can be picked up here via stmt.column

Maybe I'm overthinking it. The bindings alone is probably enough.

[wubzz]

Instead of spewing this out to the console / forcing the use of debug flags, what about one of the following?

• onUndefinedBindings in knex config - a function that receives the builder, the sql, the bindings, the client.
• An event onUndefinedBindings - an event that receives the builder, the sql, the bindings, the client.

This way knex doesn't spew out sensitive information. The responsibility falls completely on the user how they wish to handle this occurance. Knex still also has to throw the actual Error though.

[anhcao142]

Any update on this? Currently, for me, the error message is really not helpful in detecting errors.

I think we could enable/disable this feature using some config e.g. turning secureError: false (default secureError is true)

[elhigu]

@boylove if you are getting that error and you dont know where you are passing undefined parameters, then you should definitely not turn those errors off 😮 also I wouldnt want to even allow to globally configure anything like that. We haven’t even defined what different methods should do when undefined is passed to api. Some method for building query that allows to pass undefined to where clauses like .skipUndefinedInWhere could make sense though.

Edit: I might understood earlier message wrong :) if it was only about telling configuration if bindings should be shown or not, then it sounds fine... even that I dont care much that bindings are listed in exceptions always.

[jpike88]

This is a major pain in the butt, I have many queries that take in 3-5 bindings, and instead of playing whack a mole I'd like the option to turn on the binding values.

[kibertoad]

@jpike88 PR is welcome!

[KristjanTammekivi]

How about if bindings is array, it shows what indices are undefined, and if bindings is a dictionary, it shows what keys are undefined? This would solve the no-sensitive-data issue and it would be appropriate for production?

[elhigu]

@jpike88 You can write knex.on('query', ...) hook which prints your query SQL and bindings. I don't think there is any reason to add new feature for this. It is trivial also to setup hook in a way that it prints error only on undefined bindings.

@KristjanTammekivi That is a really good idea, I would merge PR for that 👍