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

Merged
merged 5 commits into from Jan 27, 2016

Conversation

Projects
None yet
5 participants
@gdyr
Contributor

gdyr commented Dec 14, 2015

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.

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Dec 14, 2015

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

  val = val.replace(/(\\\?)|[\0\n\r\b\t\\\'\"\x1a]/g, function(s) {
    switch(s) {
      case "\0": return "\\0";
      case "\n": return "\\n";
      case "\r": return "\\r";
      case "\b": return "\\b";
      case "\t": return "\\t";
      case "\x1a": return "\\Z";
      case "\\?": return "?";
      case "\'": return "''";
      default: return "\\"+s;
    }
  });

part as a separate function and override that in postgres and sqlite dialect.

Also now I noticed that PostgreSQL's implementation of SqlString.escape seems to be a based on old version of the base implementation. So when this is refactored it will be updated too to use common start part of the SqlString.escape.

@gdyr

This comment has been minimized.

Contributor

gdyr commented Dec 14, 2015

Sure, I can do that. Suggestions for that function's name? escapeCharacters perhaps?

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Dec 14, 2015

Actually looks like also the replace part of postgresql's escape implementation is old. I have to check out why \? escaping tests are not failing for postgresql currently.

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Dec 14, 2015

I think .escapeStringLiteral is pretty much what that function should do.

@gdyr

This comment has been minimized.

Contributor

gdyr commented Dec 14, 2015

Cool. Will do.

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Dec 14, 2015

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:

  testquery(createBuilder(), {
    postgres: 'select * from "users" where "id" = \'1\' and "jsonColumn" ? \'jsonKey?\''
  });

and another for sqlite since it is going to have also have separate implementation.

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Dec 17, 2015

@gmichael225 any plans when this could be ready? Would be awesome to get this to next release.

@gdyr

This comment has been minimized.

Contributor

gdyr commented Dec 18, 2015

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.
Whilst MySQL supports backslash escaping, SQLite, WebSQL, Oracle and PostgreSQL do not. As such, I suggest that escaping by doubling up on quotes be the new default behaviour. This is supported by all dialects.

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Dec 18, 2015

Sounds reasonable to me to have standard SQL behaviour as default. @tgriesser @rhys-vdw any ideas why default is currently backlash escaping?

@gdyr

This comment has been minimized.

Contributor

gdyr commented Dec 22, 2015

Best way to proceed with this?

@rhys-vdw

This comment has been minimized.

Collaborator

rhys-vdw commented Dec 22, 2015

@gmichael225

As such, I suggest that escaping by doubling up on quotes be the new default behaviour. This is supported by all dialects.

Yes please, that would be great. 👍

@elhigu

any ideas why default is currently backlash escaping?

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.

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Jan 19, 2016

@gmichael225 do you have any plans of completing this one? Just checking, no pressure :) Would be nice to get this fix to next release.

@gdyr

This comment has been minimized.

Contributor

gdyr commented Jan 19, 2016

Sorry, I may have missed something obvious - what's incomplete? I switched escaping to double-quoting globally in latest commit

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Jan 19, 2016

@gmichael225 I mean that in comments above there was talk about separating .escapeStringLiteral part from .escape method so that the whole method wouldn't get duplicated. And there is currently conflict in this branch so it cannot be merged to master before that is solved.

@gdyr

This comment has been minimized.

Contributor

gdyr commented Jan 19, 2016

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

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Jan 19, 2016

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

@snowcxt

This comment has been minimized.

snowcxt commented Jan 24, 2016

When this can be merged? I do need the. Thank you

@gdyr

This comment has been minimized.

Contributor

gdyr commented Jan 24, 2016

Conflict resolved. :)

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Jan 25, 2016

@gmichael225 thanks! File lib/dialects/sqlite3/query/string.js still needs to be removed. Generated code under lib are not committed to repository anymore.

@gdyr

This comment has been minimized.

Contributor

gdyr commented Jan 27, 2016

Whoops, yep. Fixed.

elhigu added a commit that referenced this pull request Jan 27, 2016

@elhigu elhigu merged commit 1f2d53f into tgriesser:master Jan 27, 2016

1 check passed

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

This comment has been minimized.

Collaborator

elhigu commented Jan 27, 2016

@gmichael225 awesome thanks!

@snowcxt

This comment has been minimized.

snowcxt commented Jan 27, 2016

Could you publish the new version to npm?

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Jan 29, 2016

@snowcxt I think next release is coming soon, there has been lots of fixes and features added lately

@snowcxt

This comment has been minimized.

snowcxt commented Feb 11, 2016

any update?

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Feb 11, 2016

@snowcxt 0.10.0-rc1 is already in npm, 0.10.0 should be out this week

@snowcxt

This comment has been minimized.

snowcxt commented Feb 11, 2016

awesome! thank you

@snowcxt

This comment has been minimized.

snowcxt commented Feb 15, 2016

Could you export this format function? I can use it for the raw query.

@gdyr gdyr deleted the gdyr:sqlite3-dialect-escaping branch Feb 15, 2016

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Feb 16, 2016

@snowcxt depends what you are trying to do... you can do e.g.

knex('mymom').whereRaw('?? = ?', ['id', 1]);

Check http://knexjs.org/#Raw-Bindings

@snowcxt

This comment has been minimized.

snowcxt commented Feb 17, 2016

Cool! Thank you

@villelahdenvuo

This comment has been minimized.

Contributor

villelahdenvuo commented on src/dialects/postgres/index.js in 4b2c0ef Feb 19, 2016

@gmichael225 This broke migrations because this.client.SqlString.format(sql, bindings, tz); is used. #1211

@gdyr

This comment has been minimized.

Contributor

gdyr commented Feb 19, 2016

@tuhoojabotti Sorry, d'you mind elaborating on that?

@villelahdenvuo

This comment has been minimized.

Contributor

villelahdenvuo commented Feb 19, 2016

@gmichael225 See https://github.com/tgriesser/knex/blob/master/src/interface.js#L20 You removed the SqlString from postgres dialect thus client.SqlString is undefined.

@gdyr

This comment has been minimized.

Contributor

gdyr commented Feb 19, 2016

Postgres' client.SqlString should be inherited from the base client, as per 56d808f

@villelahdenvuo

This comment has been minimized.

Contributor

villelahdenvuo commented Feb 19, 2016

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

@gdyr

This comment has been minimized.

Contributor

gdyr commented Feb 19, 2016

No worries. Let me know if I can be of any further assistance.

@villelahdenvuo

This comment has been minimized.

Contributor

villelahdenvuo commented Feb 19, 2016

@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 .defaultTo(knex.raw('now()')))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment