Update regex for named bindings

[wubzz]

Fixes #1228 . I'm unsure why the first condition of the regular expression was there, as it shouldn't matter at all?

It should now properly match named bindings, and will not require a space before the binding (see related issue).

I also added a test for this based on examples in documentation.

See http://regexr.com/3cv16

---

TODO

• Change regular expression so that spaces are not mandatory for named bindings
• If a binding value is undefined, leave the binding in the sql untouched
• Update docs explaining these changes in detail.

[wubzz]

From the failing tests I can assume the previous condition in the regexp was for preventing an extra space if a named binding was left undefined. I'll try find another solution to that issue.

[wubzz]

@elhigu Would you mind reviewing this? I think you're the right person as you worked with this code previously.

[elhigu]

@wubzz sorry, I hadn't went through github notifications last few days. I'll check this out tomorrow. btw. pinging me in knex gitter chat might be easier to get my attention time to time.

[elhigu]

Earlier bindings has been working in a way that there had to be space before start of named binding. Old regexp was: \\s(\\:\\w+\\:?) so I had changed it to allow also be in start of line (^|\\s)(\\:\\w+\\:?).

I don't know about original motivation why named bindings were not allowed to be used without leading white space.

[elhigu]

@wubuzz I believe that there shouldn't be any reason why named bindings would require leading whitespace. They are just converted to positional bindings anyways and as far as I know one can give them without leading spaces.

[elhigu]

Why trailing / leading space is removed when no bindings?

[wubzz]

The previous regular expression matched by forcing a trailing space, so it would convert the following:

SELECT :col
to
SELECT

Whereas removing the space requirement from the regular expression would instead convert

SELECT :col
to
SELECT  (Space remains)

Generally this shouldn't be an issue execution wise, but from the failing tests I figured why not keep it that way. Basically if a binding is supplied in the query like :col, but the binding value is undefined, the previous implementation would remove that binding from the query and its trailing space. That's what I'm doing in that code to supplement the removal of space from the regex.

[wubzz]

See this test for reference.

[elhigu]

@wubuzz probably the best choice here would be just to replace named binding with ? and if value is not found from bindings, then it would do the same that giving undefined to positional binding:

require('knex')({client: 'pg'}).raw('?', [undefined]).toString()
// 'DEFAULT'
require('knex')({client: 'pg'}).raw('?').toString()
// '?'

So e.g.

require('knex')({client: 'pg'}).raw(':binding').toString()
// ':binding'

require('knex')({client: 'pg'}).raw(':binding', []).toString()
// should throw an error, since named bindings require an object 

require('knex')({client: 'pg'}).raw(':binding', {}).toString()
// 'DEFAULT'

require('knex')({client: 'pg'}).raw(':binding', {binding: 'tadaa'}).toString()
// '\'tadaa\''

This way functionality would be consistent with positional bindings. To me current functionality is really inconsistent.

How does this sound?

[wubzz]

@elhigu It sounds reasonable for most use cases, such as named bindings for setting or inserting values, but when you start using the named bindings for columns in tablenames it might seem odd to replace it with DEFAULT, or really anything at all.

knex.raw('SELECT * FROM table WHERE :column: = :value', {column: 'users.name', value: 'knex'}).toString();
///... WHERE "users"."name" = 'knex'

knex.raw('SELECT * FROM table WHERE :column: = :value', {column: void 0, value: 'knex'}).toString();
//... WHERE DEFAULT = 'knex'

I do however agree that completely erasing the binding from the query which we did previously, and currently still are, is very obnoxious and should not be done.

Perhaps if it's a regular binding such as :val we can use the DEFAULT approach, but if it's :column: we leave it the way it is? I don't know.. Can't come up with something that is consistent throughout..

[elhigu]

@wubzz it should be enough that named bindings are converted to positional bindings with as little magic as possible and then it is job of some other part to decide how to handle undefined in positional binding.

I agree that replacing undefined with DEFAULT is not good functionality... probably throwing an error would be the best way to handle it for positional bindings and for named binding. Converting it to DEFAULT is just an accidental undefined behavior... Anyways discussion how to handle undefined in case of positional bindings should be done in separate pull request.

[wubzz]

@elhigu I agree. This function should only replace bindings when it's possible. My proposal is this:

If a binding is present in the query, but is undefined in the bindings object, then simply leave the binding in the sql untouched. That will in itself throw a query error, and thanks to not replacing the binding, it will be obvious what went wrong via the error message. In other words, delete the code that deals with emptyBindings.

knex.raw('SELECT :val', {val: 1}); //SELECT 1

knex.raw('SELECT :val', {val: void 0}); //SELECT :val -- Throws error

You agree and good with merging after this? I expect a few tests to change to accomodate for removing that code, like the references above. I also plan to add this to the docs.

[wubzz]

@elhigu I've updated the code, test, and documentation. I feel this is a good solution for now, the processing of undefined bindings like you said is another issue.

[elhigu]

@wubzz Looks good thanks for fixing this.