Escaping of bindings in v0.11 with PostgreSQL (or how to get v0.10 behavior?)

[moll]

Hey,

I'm upgrading from v0.10 to v0.11 and noticed non-plain object bindings given to knex.raw now get escaped by Knex before being passed to the Postgres driver (node-postgres) and its require("pg/lib/utils").prepareValue. I was depending on them being passed as-is to serialize various types in different ways, thereby expanding what the Pg driver supports independent of any layers above it.

I presume it's something to do with https://github.com/tgriesser/knex/blob/dfca38da339922f8ad8f52087ca5e6c0dcdab081/src/dialects/postgres/utils.js. Off the top of your head, would there be any risk if I [locally] scrapped that entirely and had it pass everything as-is to the Pg module's prepareValue? It's used only for bindings and not used for interpolating inside a SQL string, right?

Thanks!

[elhigu]

Could you add an example query of your use case?

[moll]

Sure:

function MyClass() {}
knex.raw("SELECT ?", [new MyClass])

That used to be sent directly to Postresql library's prepareValue. Now it's stringified beforehand.

[elhigu]

@moll that doesn't seem like complete example? how pg driver should have interpreted passing empty instance? Did you use overriding val.toPostgres or something like that?

Anyways I don't know any way to skip knex's prepare value... I don't have strong opinion if there should be way to do it... I don't know the motivation why knex has separate prepare values implementation. Maybe to have consistent functionality with knexQuery.toString() and with sending query through pg driver.

[moll]

In the example above interpreting value or identity objects is Pg's value preparation's responsibility. You could think of it as using toPostgres, even though I've got a functional, as opposed to object-oriented, equivalent on Pg's side. I was using Knex merely for its query building capabilities, expecting it to escape only values that can't be handled by placeholders, and those are usually only table or column names.

As far as I can tell now, and correct me if I'm wrong, any custom object serialization, be it of value or identity objects, is not possible in Knex v0.11 because even if Knex is made extensible, Pg will still do its own serialization, thereby escaping serialized strings twice ("foo" -> "\"foo\""). The proper solution that I can tell is to have Knex pass values untouched to Pg, which will escape or serialize values as necessary. As before. Hence this issue.

Let me know if I need to further elaborate.

[elhigu]

Can you show an example where escaping goes wrong? I was looking into the code and saw that toSQL() calls to prepBindings() which calls prepareValue() for each binding...

https://github.com/tgriesser/knex/blob/acbc8a111a5dfa5981c2bf97299d6474ed821a58/src/query/compiler.js#L72

https://github.com/tgriesser/knex/blob/6fa485302b29c7e312177d94cd1eafadf882db6f/src/dialects/postgres/index.js#L53

... but when I try it out there doesn't seem to do double escaping:

knex = require('knex')({client:'pg'})
knex("foo").where('name', `'"foo"'`).toSQL()

{ method: 'select',
  options: {},
  timeout: false,
  cancelOnTimeout: false,
  bindings: [ '\'"foo"\'' ],  // <--- no sql escaping what so ever (only the javascript escaping on ' inside string)
  __knexQueryUid: '0ee379bb-0c66-47b8-8a00-f20ceb74dda4',
  sql: 'select * from "foo" where "name" = ?' }

knex("foo").where('name', `'"foo"'`).toString()
'select * from "foo" where "name" = \'\'\'"foo"\'\'\'' // <-- here ' is escaped to '' as it should be

... toSQL().sql and toSQL().bindings are passed directly yo pg driver.

[moll]

A clarification: When I refer to prepareValue, I'm talking about require("pg/lib/utils").prepareValue. Do you see the that prepareValue being called with the original object passed to to the array of bindings, such like in my example of knex.raw above?

[moll]

The piece of code I linked to in my original post has prepareObject: https://github.com/tgriesser/knex/blob/dfca38da339922f8ad8f52087ca5e6c0dcdab081/src/dialects/postgres/utils.js. That interferes with Pg module's prepareValue which I wish to be the place for serialization of binding values.

The output of toString or toSQL on Knex object is irrelevant as the serialization I'm talking about will, or should, happen in the Pg module, not at the level of Knex.

[elhigu]

Result of toSQL() is not irrelevant, because from that output you can see exactly what is passed to pg driver to be executed. So when knex is doing something unwanted you can use toSQL() to verify what is happening and check if the data being passed to pg driver looks correct... Anyways I get your point that you would like to be able to pass bindings directly to pg driver, without knex interfering with bound values.

It is a bit strange why prepareValue() code is copied from pg driver's codebase to knex (I suppose it is done to allow knex to create complete SQL strings where escaped values are interpolated on knex side like output of toString() method)...

Maybe knex's postgres dialect could actually get prepareValue()function by require('pg/lib/utils') and use it directly that for generating output of toString() and just pass original values to pg driver when queries are executed through its APIs... @rhys-vdw @tgriesser any comments / insights about origins of prepareValue() on knex side?

[tgriesser]

Yeah this escaping behavior here is incorrect - just ran into this issue while updating some tests for the 1.0 refactor branch.

The prepBindings method on the Client should not be escaping the values as it is now, I'm having trouble tracing the origin of this change but I'm going to release an 0.12 shortly and remove this change, and then document a more robust escaping strategy for 1.0 which shouldn't break existing usages.

[tgriesser]

Closing in favor of #1661