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

Postgres question mark escaping support #946

Merged
merged 3 commits into from Aug 25, 2015

Conversation

Projects
None yet
3 participants
@elhigu
Copy link
Collaborator

elhigu commented Aug 24, 2015

Fixes #519 and #888 by adding support for \\? escape sequence to postgresql raw queries, which does not have bindings given.

Uses solution based on https://github.com/tgriesser/knex/compare/qs-escapement which converts escaped question marks to place holders before replacing ? with $1..$n variables of postgresql prepared statement.

Plain ignoring position binding replacement if no bindings was given (suggested in #888) did not work well, because in the end where replacement is done, may have bindings from various where statements as shown in example below.

After final SQL is created, placeholders are converted to single question marks.

e.g.

knex.from('table')
  .whereRaw("(json->'field')::jsonb \\?& array['keyOne', 'keyTwo']")
  .where('id', '>', 1)

Is processed in few steps:

1# select * from "table" where (json->'field')::jsonb \\?& array['keyOne', 'keyTwo'] and "id" > ?
2# select * from "table" where (json->'field')::jsonb $Q& array['keyOne', 'keyTwo'] and "id" > ?
3# select * from "table" where (json->'field')::jsonb $Q& array['keyOne', 'keyTwo'] and "id" > $1
4# select * from "test_table_two" where (json->'field')::jsonb ?& array['keyOne', 'keyTwo'] and "id" > $1
@soldair

This comment has been minimized.

Copy link

soldair commented Aug 24, 2015

this looks like it would be a good enough fix for #888 too.

@rhys-vdw

This comment has been minimized.

Copy link
Collaborator

rhys-vdw commented Aug 25, 2015

I hate to break it to you, @elhigu, but none of your commits target source files. They're all changes to the compiled output. If you were to compile now you'd lose everything. You'll need to somehow retarget all these changes on /src.

EDIT: Pls ignore this.

@rhys-vdw

This comment has been minimized.

Copy link
Collaborator

rhys-vdw commented Aug 25, 2015

@elhigu. Sorry, I see now that that's all the changes from previous commits. Looking now:

I wonder if we can do something like this here:

  positionBindings: function(sql) {
    var questionCount = 0;
    return sql.replace(/(\\*)(\?)/g, (match, escapes) => {
      questionCount++;
      return escapes.length % 2 ? match : `$${questionCount}`;
    });
  }

That's two fewer calls to replace.

EDIT: Updated regex:

('\\\\? \\? ?').replace(/(\\*)(\?)/g, function(match, escapes) {
  return escapes.length % 2 ? match : 'R';
});
// -> "R \? R"
@elhigu

This comment has been minimized.

Copy link
Collaborator Author

elhigu commented Aug 25, 2015

@rhys-vdw I think the written solution is more clear. I don't believe that this regex is bottle neck in execution, so call count should not be relevant. Though of course I will do the change if necessary to get this merged :) Also estimating performance of regex is quite hard without testing.

@elhigu

This comment has been minimized.

Copy link
Collaborator Author

elhigu commented Aug 25, 2015

@rhys-vdw I got interested to see actual difference and wrote fixed version of both expressions. Looks like actually making 3 simple calls instead of one more complex one is slightly faster.

1 replace version (I had to add some fixes there to make ? -> ? conversion happen and to fix questionCount incrementing to be ignored if escaped ? was found):

Mikaels-MBP:~ mikaelle$ cat test1.js ; time node test1.js 
a = function(sql) {
  var questionCount = 0;
  return sql.replace(/(\\*)(\?)/g, function (match, escapes) {
    if (escapes.length % 2) { 
      return '?'; 
    } else {
      questionCount++;
      return '$' + questionCount;
    }
  });
}

console.log(a("What we have here \\? ? have here \\? ? have here \\? ? have here \\? ? have here \\? ? have here \\? ? have here \\? ? have here \\? ?"));

for (var i=0; i < 1000000; i++) {
  a("What we have here \\? ? have here \\? ? have here \\? ? have here \\? ? have here \\? ? have here \\? ? have here \\? ? have here \\? ?");
}
What we have here ? $1 have here ? $2 have here ? $3 have here ? $4 have here ? $5 have here ? $6 have here ? $7 have here ? $8

real    0m6.511s
user    0m6.484s
sys 0m0.070s

3 replaces version:

Mikaels-MBP:~ mikaelle$ cat test2.js ; time node test2.js 
a = function(sql) {
    var questionCount = 0;
    return sql.replace(/\\\?/g, function() {
      return '$Q';
    }).replace(/\?/g, function() {
      questionCount++;
      return '$' + questionCount;
    }).replace(/\$Q/g, function() {
      return '?';
    });
}

console.log(a("What we have here \\? ? have here \\? ? have here \\? ? have here \\? ? have here \\? ? have here \\? ? have here \\? ? have here \\? ?"));
for (var i=0; i < 1000000; i++) {
  a("What we have here \\? ? have here \\? ? have here \\? ? have here \\? ? have here \\? ? have here \\? ? have here \\? ? have here \\? ?");
}
What we have here ? $1 have here ? $2 have here ? $3 have here ? $4 have here ? $5 have here ? $6 have here ? $7 have here ? $8

real    0m5.168s
user    0m5.142s
sys 0m0.058s
Mikaels-MBP:~ mikaelle$ 
@elhigu

This comment has been minimized.

Copy link
Collaborator Author

elhigu commented Aug 25, 2015

@rhys-vdw I gave this one more thought... Actually one regex has an advantage over the three separate ones. Now if user has $Q string in his query, it will be replaced by ? even that it was not originally \?, so the solution without temporary $Q is more compatible.

@elhigu elhigu force-pushed the elhigu:postgres-question-mark-escaping-support branch from b403080 to fef0a09 Aug 25, 2015

@elhigu

This comment has been minimized.

Copy link
Collaborator Author

elhigu commented Aug 25, 2015

Updated pull request with more compatible substitution suggested by @rhys-vdw

@rhys-vdw

This comment has been minimized.

Copy link
Collaborator

rhys-vdw commented Aug 25, 2015

@elhigu. Thanks for profiling that. I had incorrectly assumed that building three whole query strings would be more expensive. Good example of why profiling is always necessary when discussing performance.

I hadn't considered the other advantage, but I'm glad I provided something useful. 😉

This all looks great.

rhys-vdw added a commit that referenced this pull request Aug 25, 2015

Merge pull request #946 from elhigu/postgres-question-mark-escaping-s…
…upport

Postgres question mark escaping support

@rhys-vdw rhys-vdw merged commit fc6a7e2 into tgriesser:master Aug 25, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@elhigu elhigu deleted the elhigu:postgres-question-mark-escaping-support branch Aug 25, 2015

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.