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

Fix valuesForUndefined actual query being executed. Fixes #1268 #1269

Merged
merged 4 commits into from Mar 27, 2016

Conversation

Projects
None yet
3 participants
@wubzz
Collaborator

wubzz commented Mar 11, 2016

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

This comment has been minimized.

Collaborator

elhigu commented Mar 14, 2016

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

if(this.client && this.client.prepBindings) {
defaults.bindings = this.client.prepBindings(defaults.bindings);
}

This comment has been minimized.

@elhigu

elhigu Mar 15, 2016

Collaborator

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

This comment has been minimized.

Collaborator

elhigu commented Mar 15, 2016

@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

This comment has been minimized.

Collaborator

wubzz commented Mar 15, 2016

@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

This comment has been minimized.

Collaborator

elhigu commented Mar 15, 2016

@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

This comment has been minimized.

Collaborator

wubzz commented Mar 15, 2016

@elhigu I like that idea. Pushed changes!

if(this.client && this.client.prepBindings) {
defaults.bindings = this.client.prepBindings(defaults.bindings);
}
defaults.bindings = this.client.prepBindings(defaults.bindings || []);

This comment has been minimized.

@elhigu

elhigu Mar 15, 2016

Collaborator

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?

This comment has been minimized.

@wubzz

wubzz Mar 15, 2016

Collaborator

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

@@ -14,9 +14,6 @@ module.exports = function(Target) {
// Format the query as sql, prepping bindings as necessary.
Target.prototype._formatQuery = function(sql, bindings, tz) {
if (this.client && this.client.prepBindings) {
bindings = this.client.prepBindings(bindings, tz);
}

This comment has been minimized.

@elhigu

elhigu Mar 15, 2016

Collaborator

should prepBindings take also tz as a parameter? Looks like functionality is changing here a bit.

This comment has been minimized.

@elhigu

elhigu Mar 15, 2016

Collaborator

Maybe toSQL() could have tz parameter for this case

This comment has been minimized.

@wubzz

wubzz Mar 15, 2016

Collaborator

@elhigu I'm unsure on this one. It was added in 5b9336a28453fe62e4b412f1c021512a65ae6027 2 years ago, but I cant find toQuery() being called with an argument. I also don't know if this has any significance to the exposed toQuery() function seeing as this tz argument let alone toQuery is not documented at all. Only toSQL and toString are in the docs. I'll add it as an argument to toSQL for now, like you said.

@tgriesser Can you weigh in here?

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Mar 16, 2016

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

@rhys-vdw

This comment has been minimized.

Collaborator

rhys-vdw commented Mar 16, 2016

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 wubzz added the 0.11 label Mar 24, 2016

@wubzz

This comment has been minimized.

Collaborator

wubzz commented Mar 27, 2016

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.

@wubzz wubzz merged commit 34d9a76 into tgriesser:master Mar 27, 2016

1 check passed

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

This comment has been minimized.

Collaborator

elhigu commented Mar 27, 2016

good call 👍

@rhys-vdw

This comment has been minimized.

Collaborator

rhys-vdw commented May 5, 2016

Original issue #1268

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