-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Sqlite3 dialect escaping #1095
Sqlite3 dialect escaping #1095
Conversation
Hi, thanks for the fix. It seems that now there are 3 copies escape method where just replace part is slightly different. Could you just implement
part as a separate function and override that in postgres and sqlite dialect. Also now I noticed that PostgreSQL's implementation of |
Sure, I can do that. Suggestions for that function's name? escapeCharacters perhaps? |
Actually looks like also the replace part of postgresql's escape implementation is old. I have to check out why |
I think |
Cool. Will do. |
I also found out why tests are not failing... When escaping ? inside of string literal is tested only for base and mysql implementations, but postgresql implementation wasn't tested. It should be it("query \\\\? escaping", function() {
function createBuilder() {
return qb().select('*').from('users').where('id', '=', 1)
.whereRaw('?? \\? ?', ['jsonColumn', 'jsonKey\\?']);
}
// need to test each platform separately because QueryBuilder.clone does only shallow copy
// and cached raw query strings are not re-evaluated when query builder client is changed
testquery(createBuilder(), {
mysql: 'select * from `users` where `id` = 1 and `jsonColumn` ? \'jsonKey?\''
});
testquery(createBuilder(), {
postgres: 'select * from "users" where "id" = \'1\' and "jsonColumn" ? \'jsonKey?\''
});
testquery(createBuilder(), {
default: 'select * from "users" where "id" = 1 and "jsonColumn" ? \'jsonKey?\''
});
}); Could you add postgres test there:
and another for sqlite since it is going to have also have separate implementation. |
@gmichael225 any plans when this could be ready? Would be awesome to get this to next release. |
Hi @elhigu, I've been looking further into this. It seems that Knex defaults to using backslash escaping, which is not part of the SQL standard. |
Sounds reasonable to me to have standard SQL behaviour as default. @tgriesser @rhys-vdw any ideas why default is currently backlash escaping? |
Best way to proceed with this? |
@gmichael225
Yes please, that would be great. 👍
My impression it was an incorrect assumption that has been in knex for some time. Apparently it is supported by many DBMS's, so it went unnoticed until @bblack brought it up. The only discussion of this topic that I'm aware of is in #886. |
@gmichael225 do you have any plans of completing this one? Just checking, no pressure :) Would be nice to get this fix to next release. |
Sorry, I may have missed something obvious - what's incomplete? I switched escaping to double-quoting globally in latest commit |
@gmichael225 I mean that in comments above there was talk about separating |
escape / escapeStringLiteral shouldn't need to be duplicated, as we're now using SQL-compliant escaping. Okay, I'll take a look at resolving those conflicts |
@gmichael225 ah you are right, I was looking that there was separate implementation for sqlite, but it was generated file actually. please make sure that there are no leftovers from old generated files, which doesn't exist anymore. |
When this can be merged? I do need the. Thank you |
# Conflicts: # lib/dialects/sqlite3/index.js
Conflict resolved. :) |
@gmichael225 thanks! File |
Whoops, yep. Fixed. |
@gmichael225 awesome thanks! |
Could you publish the new version to npm? |
@snowcxt I think next release is coming soon, there has been lots of fixes and features added lately |
any update? |
@snowcxt 0.10.0-rc1 is already in npm, 0.10.0 should be out this week |
awesome! thank you |
Could you export this format function? I can use it for the raw query. |
@snowcxt depends what you are trying to do... you can do e.g. knex('mymom').whereRaw('?? = ?', ['id', 1]); |
Cool! Thank you |
@tuhoojabotti Sorry, d'you mind elaborating on that? |
@gmichael225 See https://github.com/tgriesser/knex/blob/master/src/interface.js#L20 You removed the |
Postgres' client.SqlString should be inherited from the base client, as per 56d808f |
@gmichael225 You are right, if I console.log the client it does work once and after that it is undefined for some reason. I have to investigate further. |
No worries. Let me know if I can be of any further assistance. |
@Grimace1975 I found the issue, we had a migration that was requiring knex instead of using the one passed to the migration, thus the client was an empty object. (Used in |
Found a bug with single-quote escaping when using an SQLite3 or WebSQL database.
http://www.sqlite.org/lang_expr.html specifies that "A string constant is formed by enclosing the string in single quotes ('). A single quote within the string can be encoded by putting two single quotes in a row - as in Pascal. C-style escapes using the backslash character are not supported because they are not standard SQL."
As such, have modified the existing unit test (as per @bblack's changes for Postgres) for single-quote escaping to check for double single quote escaping, rather than backslash.
Then copied the default SqlString.escape function, added double single quote escaping, test passes.