Make compiling SQL in error message optional

[JakobJoonas]

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).
• 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.

[OlivierCavadenti]

perfect, thx for the PR

[OlivierCavadenti]

seems good for me, but I think we need to set this boolean to true by default no ? to have same behavior that before, or need to fix tests

[kibertoad]

it would be a semver major if default is "no"

[coveralls]

Coverage increased (+0.007%) to 92.334% when pulling 83d18cd on JakobJoonas:hide-sensitive-data into dc6dbbf on knex:master.

[JakobJoonas]

Thanks for the answer, here I'd like to get your feedback about this.

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

[OlivierCavadenti]

@JakobJoonas Thanks and sorry again for the delay.
Can you check that merged documentation is ok in knex/documentation repo since you change init value for the boolean ?

[JakobJoonas]

@OlivierCavadenti Thanks for reminding that, created a PR: knex/documentation#463

[samad123qQQ]

seems good for me, but I think we need to set this boolean to true by default no ? to have same behavior that before, or need to fix tests