Upgrade ClickHouse to modern v2 driver and make minor improvements #790
Conversation
database/clickhouse/clickhouse.go
Outdated
| urlParamDB := purl.Query().Get("database") | ||
| urlPathDB := strings.TrimPrefix(purl.Path, "/") | ||
| if len(urlPathDB) != 0 && len(urlParamDB) != 0 && urlPathDB != urlParamDB { | ||
| return nil, fmt.Errorf("DSN string '%s' has mismatched database in path ('%s') and query parameter ('%s')", |
There was a problem hiding this comment.
Will add a test case for this in a sec, fixing up lint issues now
|
I see now that the lint failure is unrelated to any of my changed sources, so I'll just fix them in a separate commit, in a moment |
| var ( | ||
| tableEngines = []string{"TinyLog", "MergeTree"} | ||
| opts = dktest.Options{ | ||
| Env: map[string]string{"CLICKHOUSE_USER": "user", "CLICKHOUSE_PASSWORD": "password", "CLICKHOUSE_DB": "db"}, |
There was a problem hiding this comment.
This one database db both being the default (via env var) and the only database ever used by the tested migrations was a little too happy path. It hides the fact that a caller could previously construct migrator instances (e.g., Open vs WithInstance) in ways that would apply the migrations to the connection's DB while happily creating schema migration metadata tables in the "configured" database.
I've added validations and test cases to make sure this doesn't happen (on either of the v1 or v2 drivers).
The whole thing does make me think it would be cleanest to remove all support for the clickhouse-go/v1 driver and just use v2 for clarity (it's also what the clickhouse-go maintainers recommend anyway).
Upgrades the CLI build to use the recommended v2 driver. Adds a newer ClickHouse server version to the associated tests. Also splits test coverage for the v1 driver, disabled by tag. Some improvements: * Validation of database specification (fail for DSN mismatch) * Escape table and database names in all queries
| @@ -17,8 +17,9 @@ import ( | |||
|
|
|||
| // sourceStubMigrations hold the following migrations: | |||
| // u = up migration, d = down migration, n = version | |||
| // | 1 | - | 3 | 4 | 5 | - | 7 | | |||
| // | u d | - | u | u d | d | - | u d | | |||
| // | |||
| // All other functions are tested by tests in source/testing. | ||
| // Saves you some time and makes sure all source drivers behave the same way. | ||
| // 5. Call Register in init(). | ||
| // 1. Implement this interface. |
| @@ -16,8 +16,9 @@ var ( | |||
| ) | |||
|
|
|||
| // Regex matches the following pattern: | |||
| // 123_name.up.ext | |||
| // 123_name.down.ext | |||
| // | |||
| @@ -15,8 +15,9 @@ import ( | |||
| // It assumes that the driver tests has access to the following migrations: | |||
| // | |||
| // u = up migration, d = down migration, n = version | |||
| // | 1 | - | 3 | 4 | 5 | - | 7 | | |||
| // | u d | - | u | u d | d | - | u d | | |||
| // | |||
| @@ -16,7 +16,6 @@ type MultiError struct { | |||
| // NewMultiError returns an error type holding multiple errors. | |||
| // | |||
| // Deprecated: Use github.com/hashicorp/go-multierror instead | |||
| // | |||
Note: this PR is based on some of the problems/solutions and PR review feedback in #723 and #697
The main change – v2 driver upgrade:
Some improvements:
Open questions:
How would the maintainers recommend going about executing the legacy v1 driver test for regression protection without introducing a nasty one-off in the top-level Makefile? If people are still using migrate in applications (and doing so with the v1 driver), it would be nice for CI to catch regressions before merge. On the other hand, I won't be very upset if we just categorize this as a breaking change for v1, remove all the v1 stuff, and point to ClickHouse's own recommendation to use the v2 driver.