Field identifiers were not quoted properly in Sqlite3 dialect

[irsl]

Addressing #1048 and #1567

[elhigu]

Why this is removed?

[irsl]

By mistake, sorry for that.

[irsl]

Hm, do you have an idea why that migration test broke on Node v4?

[elhigu]

Thats one random fail in oracle test, pretty annoying one. Happens in around 20% of test runs 😞

[elhigu]

This is good now. I'll wait that test to complete and merge after. Thanks a lot :)

[carragom]

I'm sorry to question this PR but why are we switching to backticks ?. According to the docs, the standard quote character for identifiers is ". Here is an extract of the docs:

'keyword' A keyword in single quotes is a string literal.
"keyword" A keyword in double-quotes is an identifier.
[keyword] A keyword enclosed in square brackets is an identifier. This is not standard SQL. This quoting mechanism is used by MS Access and SQL Server and is included in SQLite for compatibility.
`keyword` A keyword enclosed in grave accents (ASCII code 96) is an identifier. This is not standard SQL. This quoting mechanism is used by MySQL and is included in SQLite for compatibility.

The fact that SQLite is compatible with other non-standard ways to quote identifiers like [identifier] or `identifier` does not mean we should use them. What do you think?

[irsl]

And what is your suggestion instead? Would you prefer having Knex rendered (broken) SQL statements that behave in an unexpected way?

Run the following example with and without this PR applied:

const dbFilename = "test.sqlite"
try{require("fs").unlinkSync(dbFilename)}catch(x){}

var knex = require('knex')({
  client: 'sqlite3',
  connection: {
      "filename": dbFilename
  },  
});

knex.raw("CREATE TABLE t (f TEXT)")
  .then(()=>{
     return knex("t").insert({f: "foo"})
  })
  .then(()=>{
     var q = knex("t").select().where("xxx", "foo")
     console.log("with invalid field names", q.toString())
     return q
  })  
  .then((r)=>{
     console.log("invalid field names returned", r)
  
     var q = knex("t").select().where("f", "foo")
     console.log("with valid field names", q.toString())
     return q
  })
  .then(r=>{
     console.log("valid field names returned", r)
  })

When the query is select * from "t" where "xxx" = 'foo', it just succeeds. The expected exception would be SQLITE_ERROR: no such column: xxx

[carragom]

I see the problem now, sorry I did not see it before. In this case I don't think knex is at fault, or generating broken SQL as you say. I think the fault here is at SQLite for not correctly generating an error when it should. And also for recommending to use a syntax that does not work as expected. So I guess the correct way to solve this would be to report it to SQLite and hope they fix it. But I agree that fixing this in knex should be done ASAP and if this way achieves that without introducing any problems we should take it. 👍

[irsl]

FYI: it might have been wiser to do that at the begining, anyhow I just initiated a conversation about this issue on the official sqlite-dev mailing list.

[irsl]

@carragom: The very same page of the official documentation that you linked, actually describes this behavior. I didn't notice it either:

If a keyword in double quotes (ex: "key" or "glob") is used in a context where it cannot be resolved to an identifier but where a string literal is allowed, then the token is understood to be a string literal instead of an identifier.

[carragom]

You are right, I didn't notice either. The more reason to switch to backticks then.

[kirrg001]

@elhigu

I was running into an issue today. Hopefully you can advice.

I have a sqlite3 database with double quotes and it was created with knex 0.12.
Then i've switched to knex 0.14 and ran renameColumn.
It crashed, because this line is always false and it doesn't replace the new column.

It is always false, because e.g. "column" !== `column`.

😮