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

Per-dialect escaping #886

Merged
merged 8 commits into from Dec 13, 2015

Conversation

Projects
None yet
10 participants
@bblack
Contributor

bblack commented Jul 2, 2015

Introduces per-dialect escaping so that e.g. single quotes can be correctly escaped in postgres.

Is this too hamfisted? Would it be better to refactor src/string.js so that some of it can be reused, or is it better to expect that the entire thing will be implemented per-dialect?

Addresses #869 and #828, and is a major step toward #658 .

@parkan

This comment has been minimized.

parkan commented Nov 2, 2015

👍

incorrect single quote escaping with pg is a showstopper for users of managed pgsql like heroku so it would be great to see this merged

@yusefnapora

This comment has been minimized.

yusefnapora commented Nov 2, 2015

👍 yes please!

@rhys-vdw

This comment has been minimized.

Collaborator

rhys-vdw commented Nov 2, 2015

Hey @bblack. I've just seen this PR now. I'll try to have a look at this in the coming days, but to be honest I probably know the code base less well than yourself, ha.

Out of interest, could I ask why I've never encountered any problems with postgres and knex before? (I've certainly stored many apostrophes in my time)

@bblack

This comment has been minimized.

Contributor

bblack commented Nov 3, 2015

@rhys-vdw I doubt I know the code base better than you--this pull request is the first time I've looked at it.

As for why you haven't seen any problems with postgres and knex before: my guess is you've either used an older version of postgres than 9.1, or you have standard_conforming_strings off. (see http://www.postgresql.org/docs/9.2/static/runtime-config-compatible.html#GUC-STANDARD-CONFORMING-STRINGS and http://www.postgresql.org/docs/9.2/static/sql-syntax-lexical.html)

As a separate but related issue - those docs suggest that the SQL standard way to escape a single quote is by preceding it with another single quote. If true, it seems like that would be a better default behavior in knex.

@bblack bblack changed the title from Dialect escaping to Per-dialect escaping Nov 3, 2015

@chrisbroome

This comment has been minimized.

chrisbroome commented Nov 3, 2015

Yeah the SQL standard way is to use two consecutive single quotes to escape a single quote in a string. This should be supported by every dialect.

Edit:

SELECT 'Patches O''Houlihan';
-- output:
-- Patches O'Houlihan

Works as expected in the following DBMSes;

  • MySQL
  • Postgres
  • Sqlite3
@rhys-vdw

This comment has been minimized.

Collaborator

rhys-vdw commented Nov 11, 2015

@bblack Sorry I haven't reviewed this one yet. Does this help solve #1021 perchance?

@bblack

This comment has been minimized.

Contributor

bblack commented Nov 11, 2015

Based on my understanding, no, but I can take a closer look later this week.
On Nov 11, 2015 5:49 PM, "Rhys van der Waerden" notifications@github.com
wrote:

@bblack https://github.com/bblack Sorry I haven't reviewed this one
yet. Does this help solve #1021
#1021 perchance?


Reply to this email directly or view it on GitHub
#886 (comment).

@bblack

This comment has been minimized.

Contributor

bblack commented Nov 13, 2015

@rhys-vdw i've pushed a commit that i think will solve #1021.

@rhys-vdw

This comment has been minimized.

Collaborator

rhys-vdw commented Nov 18, 2015

Alright. @bblack, I've just reviewed the PR. For anyone interested, I discovered you can filter out the whitespace changes with this GitHub query param ?w=1.

If we are to take this approach it will require some changes. I'm uncomfortable with the amount of duplication between src/dialects/postgres/string.js and src/string.js as it stands.

I've just spent a lot of time trying to get familiar with the code and the nature of the problem, and I must admit I haven't got my head around it entirely.

As for why you haven't seen any problems with postgres and knex before: my guess is you've either used an older version of postgres than 9.1, or you have standard_conforming_strings off.

If I were inserting and retrieving PostgreSQL via knex, would I see the added \ on the way out? Or does knex filter this out, hiding the problem? Because I was definitely using a more recent version of PostgreSQL with default configuration.

Either way, @chrisbroome suggests that '' is the standard escape for single quotes and is supported by other dialects. If this is the case then perhaps it's worth trying this change universally in QueryString for starters? If that works then #828 is down at least

@@ -21,6 +21,8 @@ assign(Client_MariaSQL.prototype, {
driverName: 'mariasql',
SqlString: require('../../query/string'),

This comment has been minimized.

@elhigu

elhigu Nov 19, 2015

Collaborator

This should not be needed since src/client.js base class already sets it to src/query/string.js

SchemaCompiler: SchemaCompiler,
TableCompiler: TableCompiler,
ColumnCompiler: ColumnCompiler,
SqlString: require('../../query/string'),

This comment has been minimized.

@elhigu

elhigu Nov 19, 2015

Collaborator

This should not be needed since src/client.js base class already sets it to src/query/string.js

@@ -26,11 +26,13 @@ assign(Client_MySQL2.prototype, {
// The "dialect", for reference elsewhere.
driverName: 'mysql2',
SqlString: require('../../query/string'),

This comment has been minimized.

@elhigu

elhigu Nov 19, 2015

Collaborator

This should not be needed since src/client.js base class already sets it to src/query/string.js

@@ -38,6 +38,8 @@ assign(Client_Oracle.prototype, {
return require('oracle')
},
SqlString: require('../../query/string'),

This comment has been minimized.

@elhigu

elhigu Nov 19, 2015

Collaborator

This should not be needed since src/client.js base class already sets it to src/query/string.js

@@ -34,6 +34,8 @@ assign(Client_PG.prototype, {
SchemaCompiler: SchemaCompiler,
SqlString: require('./string'),

This comment has been minimized.

@elhigu

elhigu Nov 19, 2015

Collaborator

string.js should be moved to ./query/string.js

@@ -0,0 +1,115 @@
'use strict'

This comment has been minimized.

@elhigu

elhigu Nov 19, 2015

Collaborator

This should be made to be inherited from src/query/string.js, and both, base and inherited instance should be refactored to only override small part of class in inherited implementation. (SqlString must be instantiated as well to make inheritance work nice)

@@ -39,6 +39,8 @@ assign(Client_SQLite3.prototype, {
TableCompiler: TableCompiler,
SqlString: require('../../query/string'),

This comment has been minimized.

@elhigu

elhigu Nov 19, 2015

Collaborator

Shouldn't be necessary to set here since client base implementation already does that.

@@ -20,6 +20,8 @@ inherits(Client_WebSQL, Client_SQLite3);
assign(Client_WebSQL.prototype, {
SqlString: require('../../query/string'),

This comment has been minimized.

@elhigu

elhigu Nov 19, 2015

Collaborator

Shouldn't be necessary to set here since client base implementation already does that.

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Nov 19, 2015

Hi @bblack thanks for taking time to implement this! The approach of having separate SqlString implementation for each client sounds good.

However as @rhys-vdw said there should be more shared functionality between base implementation and client implementation. Usually knex has been using inheritance to implement it check e.g. dialects/*/query/compiler.js or dialects/*/transaction.js etc.

AlsoSqlString.escape should be refactored in a way that implementation specific parts are moved to separate function, so that it is enough to rewrite smaller part in client specific implementation.

About the code I have two notes here and rest are in comments in changed files tab.

  1. In base implementation string.js is located in src/query/string.js so dialect specific class should be in dialects/*/query/string.js instead of dialects/*/string.js.
  2. In few places one should use clients implementation of SqlString instead the one coming from require:
    https://github.com/tgriesser/knex/pull/886/files#diff-cf27c1d543e886c89cd9ac8b8aeaf05bR127
    https://github.com/tgriesser/knex/pull/886/files#diff-c8ff07bea2af41f7c0247b860dbd9d39R67
    https://github.com/tgriesser/knex/pull/886/files#diff-c25c8a30a271df8c36bc7249d7b2be87R81
@zacronos

This comment has been minimized.

Contributor

zacronos commented Nov 19, 2015

FWIW, I've been using knex a lot with Postgres 9.4.4 with standard_conforming_strings = on, and I've certainly stored/retrieved single-quotes via knex without running into an error. And in fact, much of my use is on heroku, so I'm not sure why this bug would be a showstopper as @parkan suggested.

With that said, I don't want to confuse things -- it sounds like this PR is moving toward something good, so the question of when the bug is actually a problem may be moot. I just wanted to add another data point since there is some confusion over when the problem raises its ugly head.

@parkan

This comment has been minimized.

parkan commented Nov 20, 2015

@zacronos: I just tried some simple inserts and queries again and they do seem to succeed; however, we were definitely seeing errors like below with default heroku configuration (also 9.4.4/standard conforming strings on)

"insert into "tags" (name, user_id) select 'Shaquille O\'Neal', '66844f26-743f-11e5-a03f-c39c8223bd8f' where not exists (select 1 from "tags" where "name" = 'Shaquille O\'Neal' limit '1') - syntax error at or near "', '""

I think this may have to do with using raw calls, will investigate.

In the meantime, we've been running the @bblack fork and have not run into any trouble.

@studds

This comment has been minimized.

Contributor

studds commented Nov 20, 2015

Like @parkan, I saw this error with raw calls, but haven't seen it otherwise. I used the ALTER DATABASE $DB_NAME SET standard_conforming_strings=false workaround.

@bblack

This comment has been minimized.

Contributor

bblack commented Nov 25, 2015

@rhys-vdw @elhigu do you guys prefer to squash the changes or simply append new commits? (i have to rebase anyway to resolve conflicts with master).

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Nov 26, 2015

Just rebase + add new commits is fine for me. It might make rebasing easier if you have commits, which just updates generated code separated from real changes. I usually drop generated code commits when doing rebase -i.

@bblack any estimates when this could be ready? It would be nice to get this to next release.

@bblack

This comment has been minimized.

Contributor

bblack commented Nov 30, 2015

I've addressed your comments (@rhys-vdw and @elhigu) with additional commits, and rebased to resolve conflicts.

The first few commits include build objects, and the last commit doesn't--is this okay, or do I need to rebase -i and remove those changes?

@FrankZZ

This comment has been minimized.

FrankZZ commented Dec 8, 2015

+1

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Dec 8, 2015

@rhys-vdw @bblack Looks good to me. I like the way that class is kept as a set of static functions and just partly overridden 👍

elhigu added a commit that referenced this pull request Dec 13, 2015

@elhigu elhigu merged commit c22a51e into tgriesser:master Dec 13, 2015

1 check passed

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

This comment has been minimized.

Contributor

Vanuan commented Dec 15, 2015

It is actually fixed? I see that test still uses backslash for escaping:

      postgres: 'select * from "users" where "last_name" = \'O\'\'Brien\'',
@bblack

This comment has been minimized.

Contributor

bblack commented Dec 15, 2015

that's javascript escaping, since the string literal is given in single quotes. the resulting query looks like

select * from "users" where "last_name" = 'O''Brien'
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment