From d74b06f2a3f984a9b2cfd8c9ebe9c42bc4046da1 Mon Sep 17 00:00:00 2001 From: Dossy Shiobara Date: Wed, 15 Nov 2023 23:14:10 -0500 Subject: [PATCH] Add line/column/position details to Postgres migration/rollback query error messages (#495) Postgres's database driver returns the character position where a syntax error occurs, so dbmate can use that information to calculate the line and column within the query that the position refers to. Co-authored-by: Jae B Co-authored-by: Adrian Macneil --- pkg/dbmate/db.go | 4 +- pkg/dbmate/db_test.go | 76 +++++++++++++++++++++++++++++ pkg/dbmate/driver.go | 35 +++++++++++++ pkg/driver/clickhouse/clickhouse.go | 5 ++ pkg/driver/mysql/mysql.go | 5 ++ pkg/driver/postgres/postgres.go | 14 ++++++ pkg/driver/sqlite/sqlite.go | 5 ++ 7 files changed, 142 insertions(+), 2 deletions(-) diff --git a/pkg/dbmate/db.go b/pkg/dbmate/db.go index 9ebdf324..651c981f 100644 --- a/pkg/dbmate/db.go +++ b/pkg/dbmate/db.go @@ -346,7 +346,7 @@ func (db *DB) Migrate() error { // run actual migration result, err := tx.Exec(parsed.Up) if err != nil { - return err + return drv.QueryError(parsed.Up, err) } else if db.Verbose { db.printVerbose(result) } @@ -508,7 +508,7 @@ func (db *DB) Rollback() error { // rollback migration result, err := tx.Exec(parsed.Down) if err != nil { - return err + return drv.QueryError(parsed.Down, err) } else if db.Verbose { db.printVerbose(result) } diff --git a/pkg/dbmate/db_test.go b/pkg/dbmate/db_test.go index 9205a795..d0ba5c95 100644 --- a/pkg/dbmate/db_test.go +++ b/pkg/dbmate/db_test.go @@ -659,3 +659,79 @@ func TestMigrateStrictOrder(t *testing.T) { err = db.Migrate() require.Error(t, err) } + +func TestMigrateQueryErrorMessage(t *testing.T) { + u := dbutil.MustParseURL(os.Getenv("POSTGRES_TEST_URL")) + db := newTestDB(t, u) + + err := db.Drop() + require.NoError(t, err) + err = db.Create() + require.NoError(t, err) + + t.Run("ASCII SQL, error in migrate up", func(t *testing.T) { + db.FS = fstest.MapFS{ + "db/migrations/001_ascii_error_up.sql": { + Data: []byte("-- migrate:up\n-- line 2\nnot_valid_sql;\n-- migrate:down"), + }, + } + + err = db.Migrate() + require.Error(t, err) + require.Contains(t, err.Error(), "line: 3, column: 1, position: 25:") + }) + + t.Run("ASCII SQL, error in migrate down", func(t *testing.T) { + db.FS = fstest.MapFS{ + "db/migrations/002_ascii_error_down.sql": { + Data: []byte("-- migrate:up\n--migrate:down\n not_valid_sql; -- indented"), + }, + } + + err = db.Migrate() + require.NoError(t, err) + + err = db.Rollback() + require.Error(t, err) + require.Contains(t, err.Error(), "line: 2, column: 3, position: 18:") + }) + + t.Run("UTF-8 SQL, error in migrate up", func(t *testing.T) { + db.FS = fstest.MapFS{ + "db/migrations/003_utf8_error_up.sql": { + Data: []byte("-- migrate:up\n-- line 2\n/* สวัสดี hello */ not_valid_sql;\n--migrate:down"), + }, + } + + err = db.Migrate() + require.Error(t, err) + require.Contains(t, err.Error(), "line: 3, column: 20, position: 44:") + }) + + t.Run("UTF-8 SQL, error in migrate down", func(t *testing.T) { + db.FS = fstest.MapFS{ + "db/migrations/004_utf8_error_up.sql": { + Data: []byte("-- migrate:up\n-- migrate:down\n/* สวัสดี hello */ not_valid_sql;"), + }, + } + + err = db.Migrate() + require.NoError(t, err) + + err = db.Rollback() + require.Error(t, err) + require.Contains(t, err.Error(), "line: 2, column: 20, position: 36:") + }) + + t.Run("correctly count with CR-LF line endings present", func(t *testing.T) { + db.FS = fstest.MapFS{ + "db/migrations/005_cr_lf_line_endings.sql": { + Data: []byte("-- migrate:up\r\n-- line 2\r\n not_valid_sql; -- indented\r\n-- migrate:down"), + }, + } + + err = db.Migrate() + require.Error(t, err) + require.Contains(t, err.Error(), "line: 3, column: 3, position: 29:") + }) +} diff --git a/pkg/dbmate/driver.go b/pkg/dbmate/driver.go index d6f1428d..b468bcef 100644 --- a/pkg/dbmate/driver.go +++ b/pkg/dbmate/driver.go @@ -2,6 +2,7 @@ package dbmate import ( "database/sql" + "fmt" "io" "net/url" @@ -21,6 +22,7 @@ type Driver interface { InsertMigration(dbutil.Transaction, string) error DeleteMigration(dbutil.Transaction, string) error Ping() error + QueryError(string, error) error } // DriverConfig holds configuration passed to driver constructors @@ -33,6 +35,39 @@ type DriverConfig struct { // DriverFunc represents a driver constructor type DriverFunc func(DriverConfig) Driver +type QueryError struct { + Err error + Query string + Position int +} + +func (e *QueryError) Error() string { + if e.Position > 0 { + line := 1 + column := 1 + offset := 0 + for _, ch := range e.Query { + offset++ + if offset >= e.Position { + break + } + // don't count CR as a column in CR/LF sequences + if ch == '\r' { + continue + } + if ch == '\n' { + line++ + column = 1 + continue + } + column++ + } + return fmt.Sprintf("line: %d, column: %d, position: %d: %s", line, column, e.Position, e.Err.Error()) + } + + return e.Err.Error() +} + var drivers = map[string]DriverFunc{} // RegisterDriver registers a driver constructor for a given URL scheme diff --git a/pkg/driver/clickhouse/clickhouse.go b/pkg/driver/clickhouse/clickhouse.go index 0f176f64..f712eb12 100644 --- a/pkg/driver/clickhouse/clickhouse.go +++ b/pkg/driver/clickhouse/clickhouse.go @@ -369,6 +369,11 @@ func (drv *Driver) Ping() error { return err } +// Return a normalized version of the driver-specific error type. +func (drv *Driver) QueryError(query string, err error) error { + return &dbmate.QueryError{Err: err, Query: query} +} + func (drv *Driver) quotedMigrationsTableName() string { return drv.quoteIdentifier(drv.migrationsTableName) } diff --git a/pkg/driver/mysql/mysql.go b/pkg/driver/mysql/mysql.go index d5be6648..f2d508d5 100644 --- a/pkg/driver/mysql/mysql.go +++ b/pkg/driver/mysql/mysql.go @@ -313,6 +313,11 @@ func (drv *Driver) Ping() error { return db.Ping() } +// Return a normalized version of the driver-specific error type. +func (drv *Driver) QueryError(query string, err error) error { + return &dbmate.QueryError{Err: err, Query: query} +} + func (drv *Driver) quotedMigrationsTableName() string { return drv.quoteIdentifier(drv.migrationsTableName) } diff --git a/pkg/driver/postgres/postgres.go b/pkg/driver/postgres/postgres.go index 12407490..8c56d7b5 100644 --- a/pkg/driver/postgres/postgres.go +++ b/pkg/driver/postgres/postgres.go @@ -7,6 +7,7 @@ import ( "io" "net/url" "runtime" + "strconv" "strings" "github.com/amacneil/dbmate/v2/pkg/dbmate" @@ -363,6 +364,19 @@ func (drv *Driver) Ping() error { return err } +// Return a normalized version of the driver-specific error type. +func (drv *Driver) QueryError(query string, err error) error { + position := 0 + + if pqErr, ok := err.(*pq.Error); ok { + if pos, err := strconv.Atoi(pqErr.Position); err == nil { + position = pos + } + } + + return &dbmate.QueryError{Err: err, Query: query, Position: position} +} + func (drv *Driver) quotedMigrationsTableName(db dbutil.Transaction) (string, error) { schema, name, err := drv.quotedMigrationsTableNameParts(db) if err != nil { diff --git a/pkg/driver/sqlite/sqlite.go b/pkg/driver/sqlite/sqlite.go index f094e917..12c19a4c 100644 --- a/pkg/driver/sqlite/sqlite.go +++ b/pkg/driver/sqlite/sqlite.go @@ -243,6 +243,11 @@ func (drv *Driver) Ping() error { return db.Ping() } +// Return a normalized version of the driver-specific error type. +func (drv *Driver) QueryError(query string, err error) error { + return &dbmate.QueryError{Err: err, Query: query} +} + func (drv *Driver) quotedMigrationsTableName() string { return drv.quoteIdentifier(drv.migrationsTableName) }