libsql / sqld

LibSQL with extended capabilities like HTTP protocol, replication, and more.
https://libsql.org
902 stars 38 forks source link

Named parameters don't work #237

Closed haaawk closed 1 year ago

haaawk commented 1 year ago

Table schema

CREATE TABLE IF NOT EXISTS counter(country TEXT, city TEXT, value INT, PRIMARY KEY(country, city)) WITHOUT ROWID

Insert/Update

curl -X POST -d '{"statements":[{"q":"insert into counter(country, city, value) values(\"US\", :city, 1)","params":{"city":"GDA"}}]}' https://haaawk:<password>@haaawk1-haaawk.turso.io
[{"error":{"message":"NOT NULL constraint failed: counter.city"}}]%

Same SQL with positional parameter works:

curl -X POST -d '{"statements":[{"q":"insert into counter(country, city, value) values(\"US\", ?, 1)","params":["GDA"]}]}' https://haaawk:<password>@haaawk1-haaawk.turso.io
[{"results":{"columns":[],"rows":[]}}]%

It seems that when using parameter name :city instead of city in params, everything works:

curl -X POST -d '{"statements":[{"q":"insert into counter(country, city, value) values(\"US\", :city, 1)","params":{":city":"NYC"}}]}' https://haaawk:<password>@haaawk1-haaawk.turso.io
[{"results":{"columns":[],"rows":[]}}]%

Same when naming parameter @city instead of :city:

% curl -X POST -d '{"statements":[{"q":"insert into counter(country, city, value) values(\"US\", $city, 1)","params":{"$city":"SEA"}}]}' https://haaawk:<password>@haaawk1-haaawk.turso.io
[{"results":{"columns":[],"rows":[]}}]%
haaawk commented 1 year ago

This is problematic because golang JSON marshaling does not allow property names that don't start with a letter: name ":country" does not begin with a letter

haaawk commented 1 year ago

I think the bug was introduced here -> https://github.com/libsql/sqld/commit/863fbbec

CodingDoug commented 1 year ago

I don't think the original code was a bug. There was a reason for requiring the prefix character (of the four allowed according to the docs). I've tagged you into the thread on Slack from a couple weeks ago.

@MarinPostma @BearLemma

MarinPostma commented 1 year ago

I can confirm, this is not a bug

haaawk commented 1 year ago

I tried with sqlite3 and using names without :/$/@ prefix when binding worked

haaawk commented 1 year ago

What was the reason not to support names without prefix?

haaawk commented 1 year ago

What GoLang sqlite3 driver is doing - it tries to append every possible prefix to the name and tries to bind such name -> https://github.com/mattn/go-sqlite3/blob/819cc0ddf2a79b3a23b4f1ac1b5579f8b8bebafa/sqlite3.go#L1916-L1927 From the user perspective, in Golang sqlite3 values for named parameters are defined using names without prefixes. If that's what other languages are doing (JavaScript, Rust, Python) then I think it's better for our HTTP api to allow that too.

haaawk commented 1 year ago

I was basing my understanding of our code on https://github.com/libsql/sqld/pull/45 and I have to say, I liked it much more than the current behaviour.

For example in Golang, there's a standard SQL framework and all DBs are providing drivers. This framework does not accept named parameters with names starting with $, : or @.

MarinPostma commented 1 year ago

The reason for not supporting un-prefixed parameters is that SQLite considers the prefix an integral part of the variable name. Moreover, there does not seem to be an agreement over what should be the right way to handle that. Go and Python decided to deviate from SQLite, rust and js decided to stick to SQLite behavior

CodingDoug commented 1 year ago

If we're going to implement a driver for an existing generic platform database API (such as Java's JDBC or go's database/sql), then I think these drivers should observe the standards required for them to work in a portable way on that platform. The developer will need to be coached (by us) to understand where the platform driver deviates from our own APIs. That means, though, that our platform driver might need to massage the inputs to then conform to our HTTP API.

haaawk commented 1 year ago

Unfortunately it's not a matter of user conforming to anything. In Golang, you just can't do:

db.Exec("insert into test(a,b) values(:a, :b)", sql.Named(":a", "testa"), sql.Named(":b", "testb"));

The platform will throw exception refusing to accept name parameter which name starts with anything but a letter. The code of our driver won't even be executed.

haaawk commented 1 year ago

On the other hand:

db.Exec("insert into test(a,b) values(:a, :b)", sql.Named("a", "testa"), sql.Named("b", "testb"));

works just fine.

haaawk commented 1 year ago

Maybe a compromise would be to support both? Coincidently those methods don't ever conflict because in sqlite way names always start with a prefix and in nonsqlite way they never do :)

CodingDoug commented 1 year ago

Couldn't the driver take the named params and prefix the colon internally before passing them along? That's what I mean by massaging the inputs.

haaawk commented 1 year ago

Couldn't the driver take the named params and prefix the colon internally before passing them along? That's what I mean by massaging the inputs.

Yeah. That was my first thought too but sqlite allows 3 prefixes :, $ and @ to know which one to add we would have to parse the SQL to check which one is being used in the statement. It's much easier for driver/client to remove prefix than to add it :)

CodingDoug commented 1 year ago

For the first try maybe we can just say that we support only a single prefix (just to get it working and out the door), then improve it later with a sql parser that can find a replace all of them?

haaawk commented 1 year ago

But why bother when we can just support both options? This PR does just that -> https://github.com/libsql/sqld/pull/239 It includes a test showing that both prefixed and non-prefixed param names work.