clickhouse.rs
clickhouse.rs copied to clipboard
loyd
Variable binding fails if `?` appears within a string in query sql.
When I preform a query looking like SELECT * FROM table WHERE field LIKE '%?%' I get a panic like: unbound query argument ? or ?fields. This is bad on multiple levels. Panicking in response to invalid input doesn't seem great, but it's even worse that this panic is for a valid sql query.
It seems that at least some rudimentary tokenizing should be done in order to only respond to ? that are not parts of string literals.
As a kludge we could alternatively have a method that says to ignore any ? that are present.
Panicking in response to invalid input doesn't seem great, but it's even worse that this panic is for a valid sql query.
It's a controversial statement. Generally, we should panic if some error is obviously a programmer's error, not operational, thus there is no valid code to handle this error, and it must be fixed in the code.
On the other hand, if the query arrived dynamically from another place (for instance, by users), it becomes an operational error. However, this library doesn't have any way to work with a dynamic set of columns (https://github.com/loyd/clickhouse.rs/issues/44), it's focused totally on typed requests. Thus, I believe it can be only a programmer's error (for now). Ideally, it must be a compile-time error, but it duplicates sqlx logic, so I hope sqlx will support CH in the future and it will be the preferable way to do SELECT queries.
It seems that at least some rudimentary tokenizing should be done in order to only respond to ? that are not parts of string literals.
Initially, I was against the idea of doing any kind of SQL parsing, even tokenization. Because next related question will be "why to provide Identifier instead of detecting where is ? is used".
Alternatively, some kind of escape can be provided, like double ??.
Perhaps you could just provide a mechanism to opt out of ? interpolation? I've implemented the necessary escaping in my query generation (replacing ? with \xx style escape), so this bug isn't directly affecting me at the moment.
The bug was definitely not triggered by a programmer's error, I carefully implemented an IN query on binary data, escaping the binary data as clickhouse strings.
Perhaps you're thinking that rather than escaping the strings myself I should have used your automatic escaping somehow to generate the IN query with a dynamic number of strings? I suppose this is possible, but it seems very roundabout, and less efficient given that I have several queries to perform with the same IN expression.
I still think that panicking on a valid and correct SQL expression is a bug.
This is affecting us now as well. Some behavior which allows us to opt out of ? interpolation would be awesome
opt out means disabling binding at all, right?
(by the way, we're going to support server-side binding instead, but it is controversion)