Skip to content

Conversation

@asmyasnikov
Copy link

@asmyasnikov asmyasnikov commented Jan 6, 2025

Added YDB support to golang-migrate

This PR implements support for YDB in golang-migrate. The main changes include:

  • Added the YDB driver
  • Implemented necessary functions for migrations, including Open, Close, Lock, Unlock, Run, SetVersion, Version, and Drop.
  • Added tests for the YDB driver in ydb_test.go, including common SQL instructions (such as CREATE/ALTER/DROP TABLE) and YDB-specific instructions (such as CREATE/DROP TOPIC - YDB message bus)
  • Updated the documentation in README.md to include instructions on how to use the YDB driver.

How to Test:

  1. Set up an YDB environment.
  2. Run the unit and integration tests to ensure all functionalities are working correctly.
  3. Follow the instructions in the documentation to test real migrations using the YDB driver.

@coveralls
Copy link

coveralls commented Jan 31, 2025

Coverage Status

coverage: 56.532% (+0.2%) from 56.314%
when pulling 001b2f0 on ydb-platform:master
into 2788339 on golang-migrate:master.

@asmyasnikov
Copy link
Author

asmyasnikov commented Mar 23, 2025

@dhui Can you see this PR? We are waiting YDB support in golang-migrate very much

Copilot AI review requested due to automatic review settings November 20, 2025 13:05
Copilot finished reviewing on behalf of asmyasnikov November 20, 2025 13:11
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds support for YDB (Yandex Database) to golang-migrate, implementing a complete database driver with migration capabilities, locking mechanisms, and comprehensive test coverage.

Key Changes:

  • New YDB database driver with full migration lifecycle support (Open, Close, Lock, Unlock, Run, SetVersion, Version, Drop)
  • Extensive test suite including integration tests with Docker containers
  • Complete documentation with connection string format and configuration options

Reviewed Changes

Copilot reviewed 19 out of 20 changed files in this pull request and generated 7 comments.

Show a summary per file
File Description
internal/cli/build_ydb.go Build tag configuration to conditionally include YDB driver in CLI builds
go.mod / go.sum Added YDB SDK dependencies (v3.100.0) and related transitive dependencies
database/ydb/ydb.go Main driver implementation with connection handling, migrations, versioning, and locking
database/ydb/ydb_test.go Comprehensive test suite with Docker-based integration tests for multiple YDB versions
database/ydb/examples/migrations/*.sql Example migrations demonstrating YDB-specific features (tables, columns, indexes, topics)
database/ydb/README.md Detailed documentation covering connection strings, authentication methods, TLS options, and troubleshooting
README.md Added YDB to the list of supported database drivers
Makefile Added ydb to the DATABASE variable for build/test targets

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +45 to +46
If golang-migrate fails to acquire the lock and no migrations are currently running.
This may indicate that one of the migrations did not complete successfully.
Copy link

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The sentence is incomplete. Consider revising to: "If golang-migrate fails to acquire the lock when no migrations are currently running, this may indicate that one of the migrations did not complete successfully." This makes it a complete, grammatically correct sentence.

Suggested change
If golang-migrate fails to acquire the lock and no migrations are currently running.
This may indicate that one of the migrations did not complete successfully.
If golang-migrate fails to acquire the lock when no migrations are currently running, this may indicate that one of the migrations did not complete successfully.

Copilot uses AI. Check for mistakes.
if err := tx.Commit(); err != nil {
return &database.Error{OrigErr: err, Err: "transaction commit failed"}
}
return err
Copy link

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function should return nil on success, not err (which is nil at this point but makes the intent unclear). Change to return nil for consistency and clarity.

Suggested change
return err
return nil

Copilot uses AI. Check for mistakes.
var (
ErrNilConfig = fmt.Errorf("no config")
ErrNoDatabaseName = fmt.Errorf("no database name")
ErrUnsupportedTLSVersion = fmt.Errorf("unsupported tls version: use 1.0, 1.1, 1,2 or 1.3")
Copy link

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo in error message: "1,2" should be "1.2" to be consistent with the other version numbers in the list.

Suggested change
ErrUnsupportedTLSVersion = fmt.Errorf("unsupported tls version: use 1.0, 1.1, 1,2 or 1.3")
ErrUnsupportedTLSVersion = fmt.Errorf("unsupported tls version: use 1.0, 1.1, 1.2 or 1.3")

Copilot uses AI. Check for mistakes.
case err != nil:
return 0, false, &database.Error{OrigErr: err, Query: []byte(getVersionQuery)}
default:
return int(v), dirty, err
Copy link

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function returns err in the default case, but at this point err is always nil (since we didn't hit the error cases above). This should be return int(v), dirty, nil for clarity and correctness.

Suggested change
return int(v), dirty, err
return int(v), dirty, nil

Copilot uses AI. Check for mistakes.
Comment on lines +315 to +316
getLockQuery := fmt.Sprintf("SELECT * FROM %s WHERE lock_id = '%s'", y.config.LockTable, aid)
rows, err := tx.Query(getLockQuery, aid)
Copy link

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The SQL query at line 315 already has aid interpolated (WHERE lock_id = '%s'), but line 316 attempts to pass aid as an additional parameter to tx.Query. This will cause an error or unexpected behavior. Either:

  1. Remove the aid parameter from the tx.Query call (line 316), or
  2. Use parameterized queries with placeholders instead of string interpolation (recommended for security).

The same issue exists at line 332 where the query has aid interpolated but the parameters should be passed separately.

Copilot uses AI. Check for mistakes.
config.LockTable = defaultLockTable
}

conn, err := instance.Conn(context.TODO())
Copy link

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code uses both context.TODO() (lines 76, 126, 225, 261, 349) and context.Background() (lines 210, 300, 369, 399) inconsistently. For consistency with other database drivers in this codebase (e.g., Postgres uses context.Background() throughout), consider using context.Background() consistently across all operations.

Suggested change
conn, err := instance.Conn(context.TODO())
conn, err := instance.Conn(context.Background())

Copilot uses AI. Check for mistakes.
Comment on lines +217 to +241
deleteVersionQuery := fmt.Sprintf(`
DELETE FROM %s
`, y.config.MigrationsTable)

insertVersionQuery := fmt.Sprintf(`
INSERT INTO %s (version, dirty, created) VALUES (%d, %t, CurrentUtcTimestamp())
`, y.config.MigrationsTable, version, dirty)

tx, err := y.conn.BeginTx(context.TODO(), &sql.TxOptions{})
if err != nil {
return &database.Error{OrigErr: err, Err: "transaction start failed"}
}

if _, err := tx.Exec(deleteVersionQuery); err != nil {
if errRollback := tx.Rollback(); errRollback != nil {
err = multierror.Append(err, errRollback)
}
return &database.Error{OrigErr: err, Query: []byte(deleteVersionQuery)}
}

// Also re-write the schema version for nil dirty versions to prevent
// empty schema version for failed down migration on the first migration
// See: https://github.com/golang-migrate/migrate/issues/330
if version >= 0 || (version == database.NilVersion && dirty) {
if _, err := tx.Exec(insertVersionQuery, version, dirty); err != nil {
Copy link

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are two issues with this implementation:

  1. The SQL query uses string interpolation for values (%d, %t) at line 222, which is a security concern and should use parameterized queries instead.
  2. Line 241 attempts to pass version and dirty as parameters to tx.Exec, but the query already has these values interpolated, causing a mismatch.

The correct approach is to build the query without interpolating the values (use placeholders like ? or positional parameters if YDB supports them), then pass version and dirty as parameters to the Exec call. This is also the pattern used in other database drivers (e.g., Postgres uses $1, $2).

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants