Skip to content
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

Update regex for named bindings #1251

Merged
merged 5 commits into from Mar 11, 2016

Conversation

Projects
None yet
2 participants
@wubzz
Copy link
Collaborator

wubzz commented Mar 7, 2016

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

This comment has been minimized.

Copy link
Collaborator Author

wubzz commented Mar 7, 2016

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 wubzz assigned wubzz and elhigu and unassigned wubzz Mar 7, 2016

@wubzz

This comment has been minimized.

Copy link
Collaborator Author

wubzz commented Mar 7, 2016

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

@elhigu

This comment has been minimized.

Copy link
Collaborator

elhigu commented Mar 10, 2016

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

@@ -116,12 +116,16 @@ function replaceKeyBindings(raw) {
var client = raw.client
var sql = raw.sql, bindings = []

var regex = new RegExp('(^|\\s)(\\:\\w+\\:?)', 'g')
var regex = new RegExp('(\\:\\w+\\:?)', 'g')

This comment has been minimized.

@elhigu

elhigu Mar 11, 2016

Collaborator

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

This comment has been minimized.

Copy link
Collaborator

elhigu commented Mar 11, 2016

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

src/raw.js Outdated
@@ -136,6 +140,10 @@ function replaceKeyBindings(raw) {
return full.replace(key, '?')
})

emptyBindings.map(function(binding) {
sql = sql.replace(new RegExp(` ${binding}|${binding} |${binding}`, 'g'), '');

This comment has been minimized.

@elhigu

elhigu Mar 11, 2016

Collaborator

Why trailing / leading space is removed when no bindings?

This comment has been minimized.

@wubzz

wubzz Mar 11, 2016

Author Collaborator

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.

This comment has been minimized.

@wubzz

wubzz Mar 11, 2016

Author Collaborator

See this test for reference.

This comment has been minimized.

@elhigu

elhigu Mar 11, 2016

Collaborator

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

This comment has been minimized.

@wubzz

wubzz Mar 11, 2016

Author Collaborator

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

This comment has been minimized.

@elhigu

elhigu Mar 11, 2016

Collaborator

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

This comment has been minimized.

@wubzz

wubzz Mar 11, 2016

Author Collaborator

@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 wubzz force-pushed the wubzz:bugfix/named_bindings branch from 33ae7e9 to 20f8ac5 Mar 11, 2016

@wubzz

This comment has been minimized.

Copy link
Collaborator Author

wubzz commented Mar 11, 2016

@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

This comment has been minimized.

Copy link
Collaborator

elhigu commented Mar 11, 2016

@wubzz Looks good thanks for fixing this.

elhigu added a commit that referenced this pull request Mar 11, 2016

Merge pull request #1251 from wubzz/bugfix/named_bindings
Update regex for named bindings

@elhigu elhigu merged commit 3ebf795 into tgriesser:master Mar 11, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.