Sqlite3 dialect escaping

[gdyr]

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]

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]

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

[elhigu]

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]

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

[gdyr]

Cool. Will do.

[elhigu]

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]

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

[gdyr]

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]

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

[gdyr]

Best way to proceed with this?

[rhys-vdw]

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

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

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

[elhigu]

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

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]

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

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

[gdyr]

Conflict resolved. :)

[elhigu]

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

Whoops, yep. Fixed.

[elhigu]

@gmichael225 awesome thanks!

[snowcxt]

Could you publish the new version to npm?

[elhigu]

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

[snowcxt]

any update?

[elhigu]

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

[snowcxt]

awesome! thank you

[snowcxt]

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

[elhigu]

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

Cool! Thank you

[gdyr]

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

[villelahdenvuo]

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

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

[villelahdenvuo]

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

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

[villelahdenvuo]

@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()')))