New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve bindings debug information when a binding in the where clause is missing. #1557

Closed
vincentheet opened this Issue Jul 6, 2016 · 5 comments

Comments

Projects
None yet
4 participants
@vincentheet

vincentheet commented Jul 6, 2016

I just upgraded our project from knex 0.10.0 to 0.11.7 and noticed the new bindings check on the where clause, great work but I think it can be improved.

[2016-07-06T12:03:13.983Z] ERROR: u-rest-js/13896 on Macintosh-128k.local: Undefined binding(s) detected when compiling SELECT query: select "Student".* from "Student" where "Student"."id" = ? and Student.removed = ? limit ?
Error: Undefined binding(s) detected when compiling SELECT query: select "Student".* from "Student" where "Student"."id" = ? and Student.removed = ? limit ?
at QueryCompiler_SQLite3.toSQL (/Users/user/GIT/u-base/node_modules/knex/lib/query/compiler.js:78:13)

Since i'm running in debug mode it would be useful to have a bindings array to debug which binding is missing in the query. Maybe the standard debug object can be added to the error? This was the debug object I got using knex 0.10.0 which makes it clear for me that "Student"."id" is undefined and that is what I should fix.

{ method: 'select',
options: {},
bindings: [ undefined, 0, 1 ],
sql: 'select "Student".* from "Student" where "Student"."id" = ? and Student.removed = ? limit ?' }

@vincentheet vincentheet changed the title from Missing bindings debug data when a binding is missing. to Improve bindings debug information when a binding in the where clause is missing. Jul 6, 2016

@wubzz

This comment has been minimized.

Collaborator

wubzz commented Jul 8, 2016

This needs some extra thought before implementing something. I intentionally only included the actual query string due to cases where you may have a massive query with tons of bindings. I agree bindings would be great to have in there tho. Preferably the .toString() of the querybuilder would be the best, but that too will throw this error. 😏

Another idea I had in mind, which im not sure is really optimal, is to create an "initiation stack" when a query builder is initiated. That way we could technically even point directly to the part of the users code that is sending undefined values to knex. Maybe this is overkill tho.

Would work by appending this to the Raw's constructor (and any other builder)

Error.captureStackTrace(this, this.constructor)
@zacronos

This comment has been minimized.

Contributor

zacronos commented Aug 16, 2016

This would be great to have, and the initiation stack would be AMAZING -- it can be really frustrating to have knex tell me something is wrong, but not give me either the data I passed in or a stack trace showing where in my code to look. I have a very large webapp with lots of moving parts and backend supporting tasks, so I wind up having to look at the unbound query string output and think backwards: "where in the code do we do something like this?", and sometimes have to check a few different places since I don't have the data bindings to help.

I would caution about calling Error.captureStackTrace every time a query builder is initiated, however -- I believe there is significant overhead to that call. If it could be toggled on as an option for debugging though, it would be great.

@zacronos

This comment has been minimized.

Contributor

zacronos commented Aug 22, 2016

I've thought a bit more about this (due to use cases for it occurring during my work), and if there is enough concern about keeping large numbers of bindings around that the default needs to be the current behavior of NOT keeping the bindings, then maybe it should be an initialization setting. Unless there's some overhead I'm not thinking about, I know in my code we never have so many bindings that their usefulness for troubleshooting would be trumped by the performance hit.

Really there are 2 separate feature requests here, but both serve the goal of making knex errors easier to troubleshoot when you don't know what query/code actually caused the error: data bindings and initiation stack. I think they would both be VERY useful enhancements -- the initiation stack would be useful in situations where the data bindings aren't the issue, though the added overhead is higher. My suggestion is to make each of them a separate initialization setting. This is because honestly both of those features would, I think, be useful far more often than simply turning on full debug: true and seeing all queries, or turning on a knex:<something> logger; these new options would just be used to enhance the info on error objects. Something like dataBindingsOnError: true and reportInitiationStack: true.

@jeffreybrowning

This comment has been minimized.

jeffreybrowning commented Sep 10, 2016

I am also having issues debugging raw queries without being given the parameters. It's a real headache.

tgriesser added a commit that referenced this issue Sep 16, 2016

tgriesser added a commit that referenced this issue Sep 16, 2016

Merge branch 'master' into refactor
* master:
  release 0.12.1
  Add DEBUG=knex:bindings for #1557
  Fix #1669
@wubzz

This comment has been minimized.

Collaborator

wubzz commented Feb 22, 2018

The bindings are since 2016 logged whenDEBUG=knex:bindings / DEBUG=knex:*. I know this doesn't solve much for production use.

I don't think adding the bindings in plaintext to the error message is such a good idea since generally error messages will be logged to some error log file and the bindings can contain very sensitive information in some cases. Showing the SQL without bindings at least does not expose any sensitive information.

Stacktraces is an issue in and of itself and is an area where we can maybe improve a bit. Currently being discussed in terms of async/await in #2500 .

@wubzz wubzz closed this Feb 22, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment