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

Check for undefined bindings in WHERE clauses and RAW queries during… #1459

Merged
merged 5 commits into from May 30, 2016

Conversation

Projects
None yet
4 participants
@wubzz
Collaborator

wubzz commented May 28, 2016

… compile and throw an error.

This is what I had in mind for #1448 . We mentioned an entire suite for testing undefined in the issue, but I feel this is a good starting point. The idea is to throw an error during compile if a SELECT query contains an undefined binding, or if a RAW query contains an undefined binding.

The check is currently recursive. Not sure if this is needed or not.

CC @elhigu @jurko-gospodnetic @rhys-vdw

qb().from('accounts').where({Login: ['1', ['2', [void 0]]]})
];
qbuilders.forEach(function(qbuilder) {
try {

This comment has been minimized.

@elhigu

elhigu May 29, 2016

Collaborator

Probably you could use http://chaijs.com/api/bdd/#method_throw here

expect(function () {
... throwing code ...
}).to.throw(expected error message or regex checking error or something like that);

Do you think we should test here also for example if .select(function () { this.where('id', undefined); }) or .select(['col', undefined]) etc. works?

@elhigu

This comment has been minimized.

Collaborator

elhigu commented May 29, 2016

Code looks good, but is missing documentation.

@wubzz

This comment has been minimized.

Collaborator

wubzz commented May 29, 2016

@elhigu Yeah about that, I wasn't sure what part of docs to put this in. No existing section is good to place it, imo.

@elhigu

This comment has been minimized.

Collaborator

elhigu commented May 29, 2016

I agree... maybe new subsection somewhere under QueryBuilder (http://knexjs.org/#Builder), which explains about what parameters can be passed and where... E.g. where is ok to pass other query builder instances / subquery functions / knex.raw etc. that place could be ok also for telling that undefined should throw error always except with inserts.

For now it could contain only info about undefined, and when all functionality is well specified also other parameters could be explained there.

@wubzz

This comment has been minimized.

Collaborator

wubzz commented May 30, 2016

@elhigu I kind of feel like these types of heavy validation decisions should be given its own 'big' section so that it's not easy to miss. Essentially what this change does is having the user check their data before calling the library, which is something you'd want to know sooner rather than later.

What about something like

General information

Lorem ipsum...

Undefined values in INSERT/Update

Lorem ipsum...

Undefined values in SELECT / Raw

Lorem ipsum...

Put it under the Installation section or something.

@wubzz

This comment has been minimized.

Collaborator

wubzz commented May 30, 2016

@elhigu So with this we've covered INSERT, SELECT and RAW. Do we do anything for UPDATE ?

@elhigu

This comment has been minimized.

Collaborator

elhigu commented May 30, 2016

@wubzz documentation says that with update undefined value is ignored

@wubzz

This comment has been minimized.

Collaborator

wubzz commented May 30, 2016

@elhigu Yes, that's what it says now, but is there any reason to keep treating it differently?

In my eyes the two scenarios below are both equally dangerous.

  1. Run a SELECT query to load rows to process programatically, with an undefined where binding.

knex('account').select().where({col: undefined});
//ERROR! Undefined binding(s) detected when compiling SELECT query

  1. Run an UPDATE query with an undefined where binding.

knex('account').where({login: undefined}).update({password: '123'}).toString();
//update "account" set "password" = '123'

@elhigu

This comment has been minimized.

Collaborator

elhigu commented May 30, 2016

@wubzz Uh I thought you meant that how .update({col1: 1, col2: undefined }) is handled, where it kind of makes sense to ignore value

But yeah if anywhere in .where('col1', undefined) in insert / select / update / delete / etc. then the same error should be thrown. Dropping part of where statement is equally dangerous everywhere.

@wubzz

This comment has been minimized.

Collaborator

wubzz commented May 30, 2016

Then essentially the better place to put this would be in the compiler's where function instead. Makes the validation global. I'll try that.

@wubzz

This comment has been minimized.

Collaborator

wubzz commented May 30, 2016

@elhigu Added docs under "where clauses" -- seemed most appropiate. Ready to merge if you're ok with it.

@wubzz wubzz changed the title from Check for undefined bindings in SELECT queries and RAW queries during… to Check for undefined bindings in WHERE clauses and RAW queries during… May 30, 2016

@elhigu elhigu merged commit dfca38d into tgriesser:master May 30, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@peralmq

This comment has been minimized.

peralmq commented Jun 16, 2016

Just wanted to say thanks for adding this! Highlighted a bug in our system that we were then able to track down and fix. :)

@aspyatkin

This comment has been minimized.

aspyatkin commented Oct 21, 2016

If you try to save Buffer(0) into a binary column, it will fail since internally it's treated as undefined

voidxnull added a commit to Lokiedu/libertysoil-site that referenced this pull request May 7, 2017

Update knex to 0.13
- Removed method `Geotag#city` and associations from `withRelated`
  because the `geotags` table doesn't contain the `city_id` column
  which was referenced in the method. After tgriesser/knex#1459 was
  released, calling `fetch` with 'city' in `withRelated` started to
  throw an error because `geotag.attributes.city_id` was undefined.
- Removed test/regression/knexTest.js because now that the reason of
  the error is known this test is useless.
- Removed function cleanUpExtraEscapes in bin/utils/query.js because
  knex escapes strings properly since version 0.11.

voidxnull added a commit to Lokiedu/libertysoil-site that referenced this pull request May 8, 2017

Update knex to 0.13
- Removed method `Geotag#city` and associations from `withRelated`
  because the `geotags` table doesn't contain the `city_id` column
  which was referenced in the method. After tgriesser/knex#1459 was
  released, calling `fetch` with 'city' in `withRelated` started to
  throw an error because `geotag.attributes.city_id` was undefined.
- Removed test/regression/knexTest.js because now that the reason of
  the error is known this test is useless.
- Removed function cleanUpExtraEscapes in bin/utils/query.js because
  knex escapes strings properly since version 0.11.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment