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

v0.13.0 to v0.14.4 breaking sql string change for on query event #2549

Closed
Gregroam opened this Issue Mar 28, 2018 · 13 comments

Comments

Projects
None yet
3 participants
@Gregroam

Gregroam commented Mar 28, 2018

My project is currently on version 0.13.0, and I'm using the query event to print the full sql with the bindings injected into the string.

Typescript code:

this.knex.client.on('query', (data: {sql: string; bindings: any; }) => {
    var printSql = this.knex.raw(data.sql, data.bindings).toString();
    console.log(printSql);
});

This was possible because the data.sql string returns the string with ? and ?? as the placeholder values, so using it in a knex raw object with the bindings would produce the complete query.

However when upgrading to 0.14.4, the data.sql string now returns the query string using the $1, $2, $3... placeholders which makes it impossible to easily log the complete query.

Examples:
On v0.13.0 it would return something like
select * from "tableName" where "id" = ?

Now on 0.14.4 it returns something like
select * from "tableName" where "id" = $1

Was this an intended change, and is there an easier way to compute and print the complete query?

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Mar 29, 2018

This might be unintended change. Though both forms of sql are quite useful; the native with $1 etc. bindings and the one with knex’s abstracted ? bindings.

I need to investigate why this happen (maybe event is triggered in different phase than before) and figure out what would be the most reasonable way for this to work.

Also remember if you are using knex.raw(sql,bindings).toString() it is useful for logging only. If you run that query it might be insecure.

@wubzz

This comment has been minimized.

Collaborator

wubzz commented Mar 29, 2018

Seems related to #2237, specifically this line.

I also think both are useful. Though I also think the native string should only be returned when explicitly asked for via .toSQL().toNative().

@Gregroam

This comment has been minimized.

Gregroam commented Mar 30, 2018

Yeah we're just using that little trick for logging the query. I think a nice solution might be to have the event emit both the normal sql (using the ?/?? placeholders) as the sql property, and the native sql (using the $1, $2... placeholders as the nativeSQL property in the event data.

For now my solution was to use regex to replace question marks with escaped question marks (if the original query escapes a question mark, the native sql just displays it as a question mark on its own, so must re-escape these question marks), and then to replace all instances of $1, $2... with a question mark, and then use knex.raw

let printSql = this.knex.raw(data.sql.replace(/\?/g, "\\?").replace(/\$\d+/g, "?"), data.bindings).toString();

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Mar 30, 2018

Actually now that I think of it @wubzz was there a way to access the original query builder in .on(’query’,...) event? That would resolve this problem. I dont have access right now to any computer to check it out.

@wubzz

This comment has been minimized.

Collaborator

wubzz commented Mar 31, 2018

I don't believe that there is, as that event is emitted right before sql + bindings is run, with no knowledge of the builder. The 'start' event might work, though.

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Apr 2, 2018

Yeah. As a work around 'start' event might work https://runkit.com/embed/2fchlx7mvdcc . Would be nice to have query builder in other events too, or at least queryContext() of ran query.

@wubzz

This comment has been minimized.

Collaborator

wubzz commented Apr 11, 2018

Given that one must explicitly call .toNative() it doesn't feel quite right to let the query event emit with native sql string since that was not explicitly asked for. Our events in general need quite the overhaul to be honest, and many aren't even documented. Nevertheless, I think this should be changed back to how the event was previously emitted.

If we really want to include native in the event, then maybe it could simply include both in the same object?

{
    sql: 'select * from "tableName" where "id" = ?',
    sqlNative: 'select * from "tableName" where "id" = $1',
    bindings: [...]
}
@elhigu

This comment has been minimized.

Collaborator

elhigu commented Apr 11, 2018

if this really is wanted to be changed back, then probably toNative() getter would be nicer way or otherwise we would need bindings there twice too.

In some cases knex does some transformation to the passed bindings too at the same time when sql is transformed to native sql.

@wubzz

This comment has been minimized.

Collaborator

wubzz commented Apr 11, 2018

@elhigu That's true, but based on #2237 prepBindings has always been called before emitting the event. I think a simple solution would be to just move the obj.sql = this.positionBindings(obj.sql) below this.emit(...) since the emitted object is not a reference.

I'll make a PR to show what I mean.

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Apr 11, 2018

@wubzz yeah, looks like it has been like that earlier too (I consider that a bug though).

I really don't have strong opinion which format should be the correct one, so to me that PR is just fine.

I still believe that the correct fix for these events would be to pass also original query builder there to have more complete information and all the formats and one likes and queryContext available.

@Gregroam

This comment has been minimized.

Gregroam commented Apr 11, 2018

At that point, any modifications to the query builder within the query event callback won't change anything about how the query executes, it would be purely for debugging, right?

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Apr 11, 2018

Yep. Pretty much just for debugging / statistics / etc. Changing it wouldn't change currently executed query. Some weirdo could trigger that query again in that event though :D

@Gregroam

This comment has been minimized.

Gregroam commented Apr 11, 2018

Oh boy infinite loop

@wubzz wubzz closed this in #2566 Apr 12, 2018

wubzz added a commit that referenced this issue Apr 12, 2018

Bugfix/query event sql string (#2566)
* Restore 'query' event to its original form (no native sql string). Fixes #2549

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