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

Field identifiers were not quoted properly in Sqlite3 dialect #2087

Merged
merged 2 commits into from May 28, 2017

Conversation

Projects
None yet
4 participants
@irsl
Contributor

irsl commented May 27, 2017

Addressing #1048 and #1567

@@ -469,60 +469,6 @@ module.exports = function(knex) {
});
});
describe('addColumn', function() {

This comment has been minimized.

@elhigu

elhigu May 28, 2017

Collaborator

Why this is removed?

This comment has been minimized.

@irsl

irsl May 28, 2017

Contributor

By mistake, sorry for that.

@irsl

This comment has been minimized.

Contributor

irsl commented May 28, 2017

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

@elhigu

This comment has been minimized.

Collaborator

elhigu commented May 28, 2017

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

@elhigu

This comment has been minimized.

Collaborator

elhigu commented May 28, 2017

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

@elhigu elhigu merged commit a47a077 into tgriesser:master May 28, 2017

1 check passed

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

This comment has been minimized.

Contributor

carragom commented Jun 4, 2017

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

This comment has been minimized.

Contributor

irsl commented Jun 4, 2017

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

This comment has been minimized.

Contributor

carragom commented Jun 4, 2017

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

This comment has been minimized.

Contributor

irsl commented Jun 4, 2017

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

This comment has been minimized.

Contributor

irsl commented Jun 5, 2017

@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

This comment has been minimized.

Contributor

carragom commented Jun 5, 2017

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

@kirrg001

This comment has been minimized.

Collaborator

kirrg001 commented Aug 12, 2018

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

😮

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