Skip to content

Comments

exclude quoted text from named query parsing#85

Closed
JamesClonk wants to merge 5 commits intojmoiron:masterfrom
JamesClonk:quotes
Closed

exclude quoted text from named query parsing#85
JamesClonk wants to merge 5 commits intojmoiron:masterfrom
JamesClonk:quotes

Conversation

@JamesClonk
Copy link

compileNamedQuery should ignore any text inside quotes when looking for bind params

@jmoiron
Copy link
Owner

jmoiron commented Sep 18, 2014

The reason for the current behavior is that there are lots of insidious caveats to this. First off, some databases (like MySQL without ANSI_QUOTES SQL mode, or sqlite3) allow use of " quoted strings, and potentially others allow other novel string syntaxes.

Say we documented it like this: "Colons in standard SQL single-quoted string syntax will be ignored." What happens with something like this:

> create table foo ( "hi'" text);
> insert into foo ("hi'" ) values ('hello');

This may seem contrived but it's the kind of landmine that kept me from approaching this problem. However, I have to acknowledge that right now there's a class of query that is not possible to write because of the current behavior. I think some kind of escaping mechanism for : which is only for the named query parser might be a better approach, maybe something like:

ns, err = db.PrepareNamed(`
        SELECT 'a\:b\:c' || first_name, '\:\:ABC\:_\:'
        FROM person WHERE first_name=:first_name AND last_name=:last_name`)

This is pretty ugly though, and introduces a new class of impossible queries. Maybe following SQL's lead and using :: for a single colon would work better, but having used regex in Python (where '\\' matches single slash due to two layers of \ escape) these kinds of things concern me.

edit Ironically, because markdown also uses slashe escapes, explaining the python issue required 8 \.

@JamesClonk
Copy link
Author

Hmm yeah, you're right. I guess I was too naive in my approach.
I changed the code a little bit, it should now work on your first example. (But I really hope no one would ever create a table with a ' in its name).
I'd rather not have to modify my SQL statements to escape for : if it can be avoided.

Strange though, the SQLite FAQ explicitly states that: "The SQL standard specifies that single-quotes in strings are escaped by putting two single quotes in a row". I mainly work with Oracle and there its the same.

@JamesClonk
Copy link
Author

I just saw http://dev.mysql.com/doc/refman/5.1/en/string-literals.html and am about to throw the towel. MySQL really allows/uses \ to escape quotes? Oh boy..
Help! :|

@jmoiron
Copy link
Owner

jmoiron commented Sep 19, 2014

Yeah the SQL standard is like a holy book; people pull out passages when arguing, but nobody actually follows all of its tenets.

Trying to parse SQL, even something simple like this, is asking for pain. I'm still very much open to adding in some kind of : escape (more and more I'm thinking of :: to mimic ''); the fact that there are queries which you cannot write is an incongruence that is too awful to bear.

It would be simple to document and use, work reliably, and if you are fine with the code in the PR, you could just use that function to auto-escape your own queries and never have to live with that ugliness. There's no hook to get this done automatically (and I don't plan on adding one), but you wouldn't have to actually write or think about it.

@JamesClonk
Copy link
Author

OK, sounds better. :)

@JamesClonk JamesClonk closed this Sep 20, 2014
@jmoiron
Copy link
Owner

jmoiron commented Sep 22, 2014

I was thinking about whether or not this would have to be optional, since it's a behavioral change that will not affect compilation, but currently the only way to have a :: in your code that works is if you have a named var named :, which is possible via maps, but so crazy that frankly I'm fine breaking that.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants