only position bindings if bindings

[soldair]

if you have a query with a '?' mark in it, it is replaced with a numbered param for pg prepared statements.
if you provide no bindings your queries are not run as a prepared statements and ? should be preserved in text fields.

...
trx.raw("insert into table(name) values('foo?');")
...

right now this will insert a row in your "table" table with a name value of "foo$1"

select * from table;
[name]
foo$1

after this fix "?" is preserved as it should be.

note: you cannot use bindings when you are running batch queries/ multiples statements so you will have a time where you need to pass escaped user input in the sql string.

[rhys-vdw]

Hi @soldair. Thanks for the PR. Sounds like a reasonable change. Would you be able to test coverage for the problem for posterity?

[elhigu]

Sounds like the same issue with #519 and old pull request to fixit, which was never taken in https://github.com/tgriesser/knex/compare/qs-escapement

[soldair]

happy to write the test :)
ill take a peek at the other pull to see if it's better

[rhys-vdw]

@soldair Thanks mate. I'm actually not very familiar with the knex code base - just been granted collaborator status because I'm working on Bookshelf. So I have to be cautious with what I let through. Thanks for understanding.

[rhys-vdw]

@soldair Just a thought, but this correction could be taken further to make the variable substitution smarter. If I understand correctly this would still break with your fix:

trx.raw("insert into table(id, name) values(?, 'foo?');", [5])

Not sure of the exact rules that would be correct. Perhaps just checking for ?s enclosed in quotes.

Anyway, happy to take the less complete fix of course.

[soldair]

you are correct. it will still break. you will have to pass 'foo?' as a bound param.
I'm going to stay away from more complicated parsing for now because i don't have the time to take the security implications into account. I'll ping again when i have the tests.

Hindsight but.. I think raw should build a plain query not a prepared statement and perhaps a prepared statement should be another call on its own because it impacts the kinds of queries postgres allows. Most notably only allowing single statements per call to query.

[rhys-vdw]

@soldair Cool. Beyond my code base knowledge, I just wanted to mention that for consideration. Any work here is welcome and I'm happy to merge future PRs. Cheers! 👍

[soldair]

i still have to figure out the right way to add tests for this.

[elhigu]

I did some digging and wrote new suggestion how to handle this with allowing question mark escape sequence and based on @tgriesser's fix for #519. @soldair will this work for your problem as well?

[soldair]

will be resolved by merging #946

[rhys-vdw]

Okay, I'll close this one. Thanks guys.