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

Per-dialect escaping #886

Merged
merged 8 commits into from Dec 13, 2015
Merged

Per-dialect escaping #886

merged 8 commits into from Dec 13, 2015

Conversation

@bblack
Copy link
Contributor

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

@bblack bblack force-pushed the bblack:dialect_escaping branch from a2ef614 to 6636e0b Oct 25, 2015
@parkan
Copy link

@parkan 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
Copy link

@yusefnapora yusefnapora commented Nov 2, 2015

👍 yes please!

@rhys-vdw
Copy link
Member

@rhys-vdw 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
Copy link
Contributor Author

@bblack 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 Dialect escaping Per-dialect escaping Nov 3, 2015
@chrisbroome
Copy link

@chrisbroome 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
Copy link
Member

@rhys-vdw rhys-vdw commented Nov 11, 2015

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

@bblack
Copy link
Contributor Author

@bblack 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
Copy link
Contributor Author

@bblack bblack commented Nov 13, 2015

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

@rhys-vdw
Copy link
Member

@rhys-vdw 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
Member

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
Member

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
Member

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
Member

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
Member

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
Member

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
Member

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
Member

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

@elhigu
Copy link
Member

@elhigu 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
Copy link
Contributor

@zacronos 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
Copy link

@parkan 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
Copy link
Contributor

@studds 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
Copy link
Contributor Author

@bblack 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
Copy link
Member

@elhigu 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 bblack force-pushed the bblack:dialect_escaping branch from f4f057b to 3e9cc31 Nov 26, 2015
@bblack bblack force-pushed the bblack:dialect_escaping branch from 2670192 to 23201cb Nov 29, 2015
@bblack bblack force-pushed the bblack:dialect_escaping branch from 23201cb to ad3e152 Nov 29, 2015
@bblack
Copy link
Contributor Author

@bblack 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
Copy link

@FrankZZ FrankZZ commented Dec 8, 2015

+1

@elhigu
Copy link
Member

@elhigu 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 knex:master Dec 13, 2015
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Vanuan
Copy link
Contributor

@Vanuan 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
Copy link
Contributor Author

@bblack 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
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

10 participants