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

[Gregroam]

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 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]

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]

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]

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]

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]

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]

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]

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]

@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]

@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]

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]

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]

Oh boy infinite loop