fix(go/adbc/driver/snowflake): fix setting database and schema context after initial connection #2169
Conversation
davidhcoe
requested review from
zeroshade,
lidavidm and
CurtHagenlocher
as code owners
davidhcoe
changed the title
| public static string CurrentDbSchemaOption = "adbc.connection.db_schema"; | ||
| public static string ReadOnlyOption = "adbc.connection.readonly"; | ||
| public static string AutoCommitOption = "adbc.connection.autocommit"; | ||
|
|
There was a problem hiding this comment.
These are currently in the AdbcOptions class e.g. AdbcOptions.Connection.CurrentCatalog. Are you thinking that that class should be deprecated in favor of distributing them into the connection/statement/etc. classes? I seem to recall that there were some cross-cutting concerns that made it desirable to keep them under AdbcOptions.
There was a problem hiding this comment.
honestly, just didn't realize those were even there :/
| @@ -239,13 +261,13 @@ func (c *connectionImpl) GetCurrentDbSchema() (string, error) { | |||
|
|
|||
| // SetCurrentCatalog implements driverbase.CurrentNamespacer. | |||
| func (c *connectionImpl) SetCurrentCatalog(value string) error { | |||
| _, err := c.cn.ExecContext(context.Background(), "USE DATABASE ?", []driver.NamedValue{{Value: value}}) | |||
| _, err := c.cn.ExecContext(context.Background(), fmt.Sprintf("USE DATABASE %s", value), nil) | |||
There was a problem hiding this comment.
Bobby Tables says never to forget about quoting SQL identifiers properly. Snowflake identifiers can contain spaces and double-quotes and semicolons, and are quoted by using delimiting with double-quotes and doubling any contained double-quotes. That is, A -> "A", A B -> "A B", A B / C" ; D->"A B / C"" ; D"`. (This is pretty consistent with most SQL dialects.
There was a problem hiding this comment.
Bah, I've succeeded in confusing the "code" feature of Markdown. That last substitution should be
A B / C" ; D -> "A B / C"" ; D"
There was a problem hiding this comment.
Could we use snowflake's IDENTIFIER(...) function here?
e.g. fmt.Sprintf("USE DATABASE IDENTIFIER('%s')", value)
That should eliminate possible injection scenarios, right?
There was a problem hiding this comment.
This doesn't work, unfortunately. (I tried it specifically, and it's also not usually how SQL grammars work: you can't usually supply an expression here, only an identifier.)
There was a problem hiding this comment.
(I also feel obliged to point out that any single quotes in the identifier would need to be escaped in that variant, if it did happen to work.)
There was a problem hiding this comment.
went with Matt's suggestion
There was a problem hiding this comment.
I thought we had a proper quote function? Can we use that?
There was a problem hiding this comment.
we have quoteTblName(...)
There was a problem hiding this comment.
quoteTblName is what we should be using
| @@ -32,6 +32,7 @@ public abstract class AdbcConnection11 : IDisposable | |||
| , IAsyncDisposable | |||
| #endif | |||
| { | |||
|
|
|||
There was a problem hiding this comment.
Consider removing added whitespace
This PR removes the parameterization for calls to USE DATABASE and USE SCHEMA, which isn't supported for Snowflake DDL. It also modifies the GetObjects call to set the context to the database and schema, since the GetObjects call uses a sql.DB connection context instead of the snowflakeConn connection context.
Fixes #2167
Fixes #2168