Quote Entities with Double Quotes

[Jcambass]

This patch ensures that we actually quote entities (prefix, table name, column name, alias) with double quotes(").
This is important since it allows to use otherwise reserved keywords like order to be used.

The tests have been adapted to reflect this change.

Closes #38

[kevinlang]

Thanks for the pull request :)

Can you fix the linting errors? Then I can merge and issue a release sometime in the next day or so.

[warmwaffles]

I actually specifically did not add double quotes because of the documentation in sqlite https://sqlite.org/quirks.html#double_quoted_string_literals_are_accepted

2019-07-10 - Release 3.29.0

Version 3.29.0 is a regularly scheduled maintenance release of SQLite containing miscellaneous performance and feature enhancements. See the change log for details.

Beginning with this release, the double-quoted string literal misfeature is deprecated. The misfeature is still enabled by default, for legacy compatibility, however developers are encouraged to disable it at compile-time using the -DSQLITE_DQS=0 option, or at run-time using the SQLITE_DBCONFIG_DQS_DML and SQLITE_DBCONFIG_DQS_DDL actions to the sqlite3_db_config() interface. This is especially true for double-quoted string literals in CREATE TABLE and CREATE INDEX statements, as those elements can cause unexpected problems following an ALTER TABLE. See ticket 9b78184be266fd70 for an example.

It is also recommended to have DQS disabled, but I did not choose to do that out right because people may have fragments that use double quotes. https://sqlite.org/compile.html#dqs

[warmwaffles]

This PR won't hurt anything and I do think that sqlite will support it forever due to trying to maintain some backwards compatibility with mysql.

[Jcambass]

@warmwaffles Thanks for providing some context! If you feel like we shouldn't quote enties because of the mentioned drawbacks that's also understandable for me.

Maybe as an alternative we could documented that this adapter does not quote the entities and therefore only allows keywords as names in the places where sqlite supports it out of the box.

Let me know which way you prefer.

[warmwaffles]

One solution is to have some special cases that we double quote because in sqlite's documentation they suggest using double quotes for reserved keywords.

So we'd need to have a couple matching methods like this

defp quote_entity("from"), do: [~Q("from")]
defp quote_entity("with"), do: [~Q("with")]
defp quote_entity("order"), do: [~Q("order")]
# ... etc ...
defp quote_entity(val), do: [val]

But this makes me just say to quote everything.

@kevinlang do you have any opinions on this?

[kevinlang]

I think my interpretation of that document is different. It looks to be specifically referring to double-quoting literals as being incorrect (and via a compile time flag, opt-out), since double quoting is reserved for identifiers. I see that the Postgres adapter double quotes all identifiers by default. As such, I don't see any disadvantage, and only an advantage, to double quoting identifiers. Am I missing something?

I would go so far as to say that having double quoted literal be disabled by default via sqlite3_db_config() at runtime might be worth adding too (in a separate PR). That way it can be re-enabled if need be by downstream users. It looks like the compile time option just sets the default value, so we could do that too, as long as we expose a way for users to re-enable it if needed (though, honestly, that may not even be needed).

[kevinlang]

To confirm my understanding, I loaded up the sqlite3 CLI and disabled those two options:

> .dbconfig dqs_ddl off
> .dbconfig dqs_dml off

With that done, double quoting entities still works:

> create table "foo" ("bar" text);

It just raises an error when double quoting the literal:

> insert into "foo" values ("a");
Error: no such column: a

But everything else works as expected:

> insert into "foo" values ('a');

---

And sure enough, if I reload the CLI without disabling those options, we can see the behavior of the misfeature:

> create table "foo" ("bar" text);
> insert into "foo" values ("a");
(no error)

[warmwaffles]

Excellent, so let's merge this fix then. I misunderstood the misfeature.

[Jcambass]

I think my interpretation of that document is different. It looks to be specifically referring to double-quoting literals as being incorrect (and via a compile time flag, opt-out), since double quoting is reserved for identifiers. I see that the Postgres adapter double quotes all identifiers by default. As such, I don't see any disadvantage, and only an advantage, to double quoting identifiers. Am I missing something?

I think the only potential downside (the one I was thinking of when writting my previous comment) would be that it would treat wrongly spelled identifiers as literals since would always just quote them.

I would go so far as to say that having double quoted literal be disabled by default via sqlite3_db_config() at runtime might be worth adding too (in a separate PR). That way it can be re-enabled if need be by downstream users. It looks like the compile time option just sets the default value, so we could do that too, as long as we expose a way for users to re-enable it if needed (though, honestly, that may not even be needed).

I think that would actually be the best option if we feel like sqlite users don't rely on this missfeature to much.

[warmwaffles]

Thanks for the work on this.