Fix valuesForUndefined actual query being executed. Fixes #1268

[wubzz]

This was a doosey and a half.. The main issue was that prepBindings was not being called in the QueryCompiler.

Interestingly however, some prepBindings that weren't being called also resulted in invalid tests, specifically for postgres. Reason for this is that the pg module and knex's own prepBindings -> prepareValue converts Numbers to Strings prior to executing the query. I'm mentioning this so you know why I had to change a few tests.

I also had to do a hacky-fix to one of the tests that involved undefined values for sqlite3 to prevent the TypeError being thrown. Since it does not support valueForUndefined, the entire test crashed after applying prepBindings, which was to be expected.

cc @rhys-vdw @elhigu @chrisfrancis27

[elhigu]

I'll review this. I didn't even knew that other drivers had prepBindings too I thought it was only pg thing.

[elhigu]

I'm not sure if this is correct place to put prepare bindings... e.g. in that case it will be ran multiple times for queryBuilder.toString() or queryBuilder.toQuery() methods (https://github.com/tgriesser/knex/blob/master/src/interface.js#L7).

Looks like there is already code in dialect side, which tries to do it in MSSQL https://github.com/tgriesser/knex/blob/master/src/dialects/mssql/index.js#L103 and oracle https://github.com/tgriesser/knex/blob/master/src/dialects/oracle/index.js#L129
but for some reason postgres doesn't have it in place https://github.com/tgriesser/knex/blob/master/src/dialects/postgres/index.js#L132 it wasn't there either for SQLite and I think that oracles stream version of query didn't call that either...

Probably it would be safest to call it somewhere in common code, like maybe here, but in that case we need to remove calls to it from other places. And all the places where toSQL is used should be checked that preparing bindings doesn't get called multiple times as a side effect...

It could make sense though that when we get bindings we prepare them right away and then bindings everywhere would be already prepared... only bad thing in that would be that if client is given to e.g. knex.raw later on, bindings will not be updated correctly for that client.

[elhigu]

@wubzz I'm not sure yet, how and where bindings should be prepared... do you have any new ideas how to do this consistent way? I'm not a big fan of putting that call to every dialect either...

[wubzz]

@elhigu 👍 I also dislike keeping such fundamental function calls seperated in each dialect, because that's doomed to fail one day when someone adds a new dialect and forgets to call prepBindings. The fact that postgres (and mysql?) was missed to begin with is proof of that.

What do you think about removing all current prepBindings calls and append it to the master class instead? (client)
https://github.com/tgriesser/knex/blob/master/src/client.js#L128
https://github.com/tgriesser/knex/blob/master/src/client.js#L139

The case about toSQL(), toString() and toQuery() calling prepBindings, the only way I can see of avoiding that is by calling prepBindings whenever we receive bindings, which would mean every single function in the API today that takes a set of bindings. That would result in even more calls, so I don't see it as a worthy tradeoff. Reason why it would have to be in every call is because you can do this for example:

var qb = knex(tableName);

qb.where({test: 1});
console.log(qb.toString());
qb.orWhere({test: 2});
console.log(qb.toString());

Both .toString() calls would have to be properly formatted to keep this consistent, which means .where and .orWhere would have to call .prepBindings. 👎

[elhigu]

@wubuzz how does it sound if prepBindings is ran when .toSQL is called, but original bindings are not thrown away so that if client is attached/changed to query builder, prepBindings may be ran again on next .toSQL call?

[wubzz]

@elhigu I like that idea. Pushed changes!

[elhigu]

Is here possible that this.client doesn't exist? e.g. in case require('knex').raw('? ?', [undefined, 'wat']).toSQL();

EDIT: looks like raw has its own toSQL... could there be any other case where this.client is undefined?

[wubzz]

Good catch! Didn't think about raw being exposed on the main knex import.

[elhigu]

@rhys-vdw do you have any idea about that tz argument discussed here?

[rhys-vdw]

do you have any idea about that tz argument discussed here?

No, we could perhaps blame it? Generally I'd say that if a feature is undocumented and untested then it is an implementation detail. But with Knex there seems to be a large amount of "discoverable" functionality. Might @bendrucker or @tgriesser be able to shed any light on this?

[wubzz]

As this PR fixes a pretty crucial bug and is not a breaking change I am going to merge this so that it's included in the next release.

Should some information come to light regarding tz we can deal with that in the future. For now, it's still included in the source so there are no breaking changes in that regard.

[elhigu]

good call 👍

[rhys-vdw]

Original issue #1268