feat: allow host in dsn and use statement based transactions #10
Conversation
google-cla
bot
added
the
cla: yes
|
|
||
| --- | ||
|
|
||
| gorm cannot use the driver as it-is but @rakyll has been working on a dialect. | ||
| She doesn't have bandwidth to ship a fully featured dialect right now but contact | ||
| her if you would like to contribute. | ||
|
|
||
| --- | ||
|
|
||
| `error = <use T(nil), not nil>`: Use a typed nil, instead of just nil. |
There was a problem hiding this comment.
This is no longer a problem. Untyped nil parameters are now supported.
driver.go
Outdated
| @@ -33,67 +36,151 @@ import ( | |||
| ) | |||
|
|
|||
| const userAgent = "go-sql-driver-spanner/0.1" | |||
| const dsnRegExpString = "((?P<HOSTGROUP>[\\w.-]+(?:\\.[\\w\\.-]+)*[\\w\\-\\._~:/?#\\[\\]@!\\$&'\\(\\)\\*\\+,;=.]+)/)?projects/(?P<PROJECTGROUP>(([a-z]|[-.:]|[0-9])+|(DEFAULT_PROJECT_ID)))(/instances/(?P<INSTANCEGROUP>([a-z]|[-]|[0-9])+)(/databases/(?P<DATABASEGROUP>([a-z]|[-]|[_]|[0-9])+))?)?(([\\?|;])(?P<PARAMSGROUP>.*))?" | |||
There was a problem hiding this comment.
Support setting a host name in the connection string. The format of the connection string is the same as for Java / JDBC, except it is not prefixed by [jdbc:]cloudspanner:.
There was a problem hiding this comment.
Can we parse this regex during the compile time? A ref: https://github.com/googleapis/google-cloud-go/blob/f08c73a75e2d2a8b9a0b184179346cb97c82e9e5/spanner/client.go#L56
Also, can we give an example in the comment? It is difficult to see what dsn format should be.
There was a problem hiding this comment.
Thanks, I had forgotten about that possibility. I've also added some additional documentation and an example.
| sql.Register("spanner", &Driver{}) | ||
| } | ||
|
|
||
| // Driver represents a Google Cloud Spanner database/sql driver. | ||
| type Driver struct { | ||
| // Config represents the optional advanced configuration to be used | ||
| // by the Google Cloud Spanner client. | ||
| Config spanner.ClientConfig |
There was a problem hiding this comment.
The driver should not contain any client configuration, that should be in the connector.
| project string | ||
| instance string | ||
| database string | ||
| params map[string]string |
There was a problem hiding this comment.
params is a map that contains configuration like usePlainText=true, readonly=true, etc.
| } | ||
|
|
||
| func openDriverConn(ctx context.Context, d *Driver, name string) (driver.Conn, error) { | ||
| if d.Config.NumChannels == 0 { | ||
| d.Config.NumChannels = 1 // TODO(jbd): Explain database/sql has a high-level management. |
There was a problem hiding this comment.
As we are using the client library, the management of this should be kept in the client library, and not set to a reduced value here.
| } | ||
|
|
||
| func (c *conn) PrepareContext(ctx context.Context, query string) (driver.Stmt, error) { | ||
| // TODO(jbd): Mention emails need to be escaped. |
There was a problem hiding this comment.
No longer needed, as the new parser is able to detect string literals and comments that may contain email addresses and other strings that could otherwise be seen as parameter names.
| } | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return &result{rowsAffected: rowsAffected}, nil | ||
| } | ||
|
|
||
| func isDdl(query string) (bool, error) { |
There was a problem hiding this comment.
Moved to statement_parser.go
| "testing" | ||
| ) | ||
|
|
||
| func TestRowsAtomicTypes(t *testing.T) { |
There was a problem hiding this comment.
Integration tests have been moved to integration_test.go and are now skipped if -short has been specified.
driver.go
Outdated
| return &connector{ | ||
| driver: d, | ||
| connectorConfig: connectorConfig, | ||
| config: config, |
There was a problem hiding this comment.
can we have a better name for config? We have connectorConfig and config in this struct. It is unclear what config is for.
There was a problem hiding this comment.
Good point, changed to spannerClientConfig.
driver.go
Outdated
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return &conn{client: client, adminClient: adminClient, name: name}, nil | ||
| return &conn{client: client, adminClient: adminClient, database: databaseName}, nil | ||
| } | ||
|
|
||
| func createAdminClient(ctx context.Context) (adminClient *adminapi.DatabaseAdminClient, err error) { |
There was a problem hiding this comment.
Is this used any where?
There was a problem hiding this comment.
No, not anymore, so removed.
driver.go
Outdated
| func (c *conn) Close() error { | ||
| c.client.Close() | ||
| return nil | ||
| } | ||
|
|
||
| func (c *conn) Begin() (driver.Tx, error) { | ||
| panic("Using BeginTx instead") | ||
| return nil, fmt.Errorf("use BeginTx instead") |
There was a problem hiding this comment.
Can we call BeginTx without a driver.TxOptions? E.g.,
return c.BeginTx(context.Background(), driver.TxOptions{})
-shortmode.@.Fixes #11
Fixes #12