Closed peterldowns closed 1 year ago
This PR adds a test verify that DisableCreateTable = true
results in the migrations being applied successfully without a migrations table being created. Unfortunately this test fails:
❯ go test ./...
----------------------------------------------------------------------
FAIL: migrate_test.go:745: SqliteMigrateSuite.TestRunMigrationObjDisableCreateTable
migrate_test.go:755:
c.Assert(err, IsNil)
... value sqlite3.Error = sqlite3.Error{Code:1, ExtendedCode:1, SystemErrno:0x0, err:"no such table: gorp_migrations"} ("no such table: gorp_migrations")
OOPS: 40 passed, 1 FAILED
--- FAIL: Test (0.01s)
FAIL
FAIL github.com/rubenv/sql-migrate 0.136s
ok github.com/rubenv/sql-migrate/sql-migrate 0.140s [no tests to run]
ok github.com/rubenv/sql-migrate/sqlparse 0.193s
FAIL
I believe this has uncovered another bug, which is that DisableCreateTable
simply doesn't work. I've added another test which I think shows the problem. When DisableCreateTable = true
, the migrations table will (correctly) not be created, but the ms.GetMigrationRecords
method always queries this table to try to find which migrations exist:
func (ms MigrationSet) GetMigrationRecords(db *sql.DB, dialect string) ([]*MigrationRecord, error) {
dbMap, err := ms.getMigrationDbMap(db, dialect)
if err != nil {
return nil, err
}
var records []*MigrationRecord
query := fmt.Sprintf("SELECT * FROM %s ORDER BY %s ASC", dbMap.Dialect.QuotedTableForQuery(ms.SchemaName, ms.getTableName()), dbMap.Dialect.QuoteField("id"))
_, err = dbMap.Select(&records, query)
if err != nil {
return nil, err
}
return records, nil
}
I can picture three potential ways to fix this problem:
DisableCreateTable
as an option entirely, since it does not work, and it's unlikely that anyone is relying on it working.GetMigrationRecords
to always return nil, nil
when DisableCreateTable == true
, as if no migrations had ever been applied.GetMigrationRecords
to use a different query that returns nil, nil
iff the migrations table doesn't exist, but if it does exist it returns any records from the table.I don't think I can make this choice for you, but please feel free to build on top of this PR if you'd like to use it to help solve the problem!
I think the point of disabling the creation of the table is for those cases where you want to manage that table yourself.
However, you do need it to track state (it's not just reading it: after executing a migration it also inserts there).
I wonder what your use case is to run completely without a migration table?
Update GetMigrationRecords to use a different query that returns nil, nil iff the migrations table doesn't exist, but if it does exist it returns any records from the table.
As a general advice: never do things like this, as it will silently hide configuration errors, then do the thing you probably do not want in such a scenario.
I think the point of disabling the creation of the table is for those cases where you want to manage that table yourself.
However, you do need it to track state (it's not just reading it: after executing a migration it also inserts there).
OK, I'll update the tests to just confirm that sql-migrate will not attempt to create the table. Thank you for clarifying. I wasn't sure what was the intended behavior with DisableCreateTable = true
because I could not find any documentation or discussion of its use.
I wonder what your use case is to run completely without a migration table?
I am working on a library for quickly creating postgres databases for testing. As part of creating a test database, I assume users would like to run their migrations. I have therefore implemented adapters for all of the most popoular golang migration libraries, including an adaptor for sql-migrate.
The sqlmigrator adapter uses a MigrationSet
instance to run migrations, which is how I discovered the problem with DisableCreateTable
not working.
I personally do not use sql-migrate, and I don't know why someone would want to use DisableCreateTable
, I just want to support the use case in which another developer does.
@rubenv this PR is now ready for review, it contains the bugfix and tests to confirm it 🥳
Merged, thanks a lot!
@rubenv thank you for reviewing and merging! You may want to close #236 since it also attempted to fix this issue, and is no longer necessary.
Previously the DisableCreateTable setting was always being read from the global
migSet
instance. This meant that settingms.DisableCreateTable
on a givenvar ms MigrationSet
instance had no effect.This commit fixes that bug, and adds a test to confirm the fix. Regardless of the global
migSet.DisableCreateTable
value,ms.getMigrationDbMap()
will always read fromms.DisableCreateTable
.