Skip to content

Commit

Permalink
Merge pull request #3418 from mikiher/fix-database-version-init
Browse files Browse the repository at this point in the history 
Fix MigrationManager initial run behavior
  • Loading branch information
@advplyr
advplyr committed Sep 14, 2024
2 parents 5c49a8c + 21c77dc commit 8493e56
Show file tree
Hide file tree
Showing 4 changed files with 87 additions and 17 deletions.
3 changes: 2 additions & 1 deletion package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,8 @@
"pkg": {
"assets": [
"client/dist/**/*",
"node_modules/sqlite3/lib/binding/**/*.node"
"node_modules/sqlite3/lib/binding/**/*.node",
"server/migrations/*.js"
],
"scripts": [
"prod.js",
Expand Down
4 changes: 2 additions & 2 deletions server/Database.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -176,9 +176,9 @@ class Database {
}

try {
const migrationManager = new MigrationManager(this.sequelize, global.ConfigPath)
const migrationManager = new MigrationManager(this.sequelize, this.isNew, global.ConfigPath)
await migrationManager.init(packageJson.version)
if (!this.isNew) await migrationManager.runMigrations()
await migrationManager.runMigrations()
} catch (error) {
Logger.error(`[Database] Failed to run migrations`, error)
throw new Error('Database migration failed')
Expand Down
24 changes: 21 additions & 3 deletions server/managers/MigrationManager.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,13 @@ class MigrationManager {

/**
* @param {import('../Database').sequelize} sequelize
* @param {boolean} isDatabaseNew
* @param {string} [configPath]
*/
constructor(sequelize, configPath = global.configPath) {
constructor(sequelize, isDatabaseNew, configPath = global.configPath) {
if (!sequelize || !(sequelize instanceof Sequelize)) throw new Error('Sequelize instance is required for MigrationManager.')
this.sequelize = sequelize
this.isDatabaseNew = isDatabaseNew
if (!configPath) throw new Error('Config path is required for MigrationManager.')
this.configPath = configPath
this.migrationsSourceDir = path.join(__dirname, '..', 'migrations')
Expand All @@ -42,6 +44,7 @@ class MigrationManager {

await this.fetchVersionsFromDatabase()
if (!this.maxVersion || !this.databaseVersion) throw new Error('Failed to fetch versions from the database.')
Logger.debug(`[MigrationManager] Database version: ${this.databaseVersion}, Max version: ${this.maxVersion}, Server version: ${this.serverVersion}`)

if (semver.gt(this.serverVersion, this.maxVersion)) {
try {
Expand All @@ -63,6 +66,11 @@ class MigrationManager {
async runMigrations() {
if (!this.initialized) throw new Error('MigrationManager is not initialized. Call init() first.')

if (this.isDatabaseNew) {
Logger.info('[MigrationManager] Database is new. Skipping migrations.')
return
}

const versionCompare = semver.compare(this.serverVersion, this.databaseVersion)
if (versionCompare == 0) {
Logger.info('[MigrationManager] Database is already up to date.')
Expand Down Expand Up @@ -180,7 +188,15 @@ class MigrationManager {

async checkOrCreateMigrationsMetaTable() {
const queryInterface = this.sequelize.getQueryInterface()
if (!(await queryInterface.tableExists(MigrationManager.MIGRATIONS_META_TABLE))) {
let migrationsMetaTableExists = await queryInterface.tableExists(MigrationManager.MIGRATIONS_META_TABLE)

if (this.isDatabaseNew && migrationsMetaTableExists) {
// This can happen if database was initialized with force: true
await queryInterface.dropTable(MigrationManager.MIGRATIONS_META_TABLE)
migrationsMetaTableExists = false
}

if (!migrationsMetaTableExists) {
await queryInterface.createTable(MigrationManager.MIGRATIONS_META_TABLE, {
key: {
type: DataTypes.STRING,
Expand All @@ -192,9 +208,10 @@ class MigrationManager {
}
})
await this.sequelize.query("INSERT INTO :migrationsMeta (key, value) VALUES ('version', :version), ('maxVersion', '0.0.0')", {
replacements: { version: this.serverVersion, migrationsMeta: MigrationManager.MIGRATIONS_META_TABLE },
replacements: { version: this.isDatabaseNew ? this.serverVersion : '0.0.0', migrationsMeta: MigrationManager.MIGRATIONS_META_TABLE },
type: Sequelize.QueryTypes.INSERT
})
Logger.debug(`[MigrationManager] Created migrationsMeta table: "${MigrationManager.MIGRATIONS_META_TABLE}"`)
}
}

Expand All @@ -219,6 +236,7 @@ class MigrationManager {
await fs.copy(sourceFile, targetFile) // Asynchronously copy the files
})
)
Logger.debug(`[MigrationManager] Copied migrations to the config directory: "${this.migrationsDir}"`)
}

/**
Expand Down
73 changes: 62 additions & 11 deletions test/server/managers/MigrationManager.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ describe('MigrationManager', () => {
down: sinon.stub()
}
sequelizeStub.getQueryInterface.returns({})
migrationManager = new MigrationManager(sequelizeStub, configPath)
migrationManager = new MigrationManager(sequelizeStub, false, configPath)
migrationManager.fetchVersionsFromDatabase = sinon.stub().resolves()
migrationManager.copyMigrationsToConfigDir = sinon.stub().resolves()
migrationManager.updateMaxVersion = sinon.stub().resolves()
Expand Down Expand Up @@ -131,6 +131,21 @@ describe('MigrationManager', () => {
expect(loggerInfoStub.calledWith(sinon.match('Migrations successfully applied'))).to.be.true
})

it('should log that migrations will be skipped if database is new', async () => {
// Arrange
migrationManager.isDatabaseNew = true
migrationManager.initialized = true

// Act
await migrationManager.runMigrations()

// Assert
expect(loggerInfoStub.calledWith(sinon.match('Database is new. Skipping migrations.'))).to.be.true
expect(migrationManager.initUmzug.called).to.be.false
expect(umzugStub.up.called).to.be.false
expect(umzugStub.down.called).to.be.false
})

it('should log that no migrations are needed if serverVersion equals databaseVersion', async () => {
// Arrange
migrationManager.serverVersion = '1.2.0'
Expand Down Expand Up @@ -181,7 +196,7 @@ describe('MigrationManager', () => {
// Create a migrationsMeta table and populate it with version and maxVersion
await sequelize.query('CREATE TABLE migrationsMeta (key VARCHAR(255), value VARCHAR(255))')
await sequelize.query("INSERT INTO migrationsMeta (key, value) VALUES ('version', '1.1.0'), ('maxVersion', '1.1.0')")
const migrationManager = new MigrationManager(sequelize, configPath)
const migrationManager = new MigrationManager(sequelize, false, configPath)
migrationManager.checkOrCreateMigrationsMetaTable = sinon.stub().resolves()

// Act
Expand All @@ -195,7 +210,7 @@ describe('MigrationManager', () => {
it('should create the migrationsMeta table if it does not exist and fetch versions from it', async () => {
// Arrange
const sequelize = new Sequelize({ dialect: 'sqlite', storage: ':memory:', logging: false })
const migrationManager = new MigrationManager(sequelize, configPath)
const migrationManager = new MigrationManager(sequelize, false, configPath)
migrationManager.serverVersion = serverVersion

// Act
Expand All @@ -208,14 +223,50 @@ describe('MigrationManager', () => {
value: { type: 'VARCHAR(255)', allowNull: false, defaultValue: undefined, primaryKey: false, unique: false }
})
expect(migrationManager.maxVersion).to.equal('0.0.0')
expect(migrationManager.databaseVersion).to.equal('0.0.0')
})

it('should create the migrationsMeta with databaseVersion=serverVersion if database is new', async () => {
// Arrange
const sequelize = new Sequelize({ dialect: 'sqlite', storage: ':memory:', logging: false })
const migrationManager = new MigrationManager(sequelize, true, configPath)
migrationManager.serverVersion = serverVersion

// Act
await migrationManager.fetchVersionsFromDatabase()

// Assert
const tableDescription = await sequelize.getQueryInterface().describeTable('migrationsMeta')
expect(tableDescription).to.deep.equal({
key: { type: 'VARCHAR(255)', allowNull: false, defaultValue: undefined, primaryKey: false, unique: false },
value: { type: 'VARCHAR(255)', allowNull: false, defaultValue: undefined, primaryKey: false, unique: false }
})
expect(migrationManager.maxVersion).to.equal('0.0.0')
expect(migrationManager.databaseVersion).to.equal(serverVersion)
})

it('should re-create the migrationsMeta table if it existed and database is new (Database force=true)', async () => {
// Arrange
const sequelize = new Sequelize({ dialect: 'sqlite', storage: ':memory:', logging: false })
// Create a migrationsMeta table and populate it with version and maxVersion
await sequelize.query('CREATE TABLE migrationsMeta (key VARCHAR(255), value VARCHAR(255))')
await sequelize.query("INSERT INTO migrationsMeta (key, value) VALUES ('version', '1.1.0'), ('maxVersion', '1.1.0')")
const migrationManager = new MigrationManager(sequelize, true, configPath)
migrationManager.serverVersion = serverVersion

// Act
await migrationManager.fetchVersionsFromDatabase()

// Assert
expect(migrationManager.maxVersion).to.equal('0.0.0')
expect(migrationManager.databaseVersion).to.equal(serverVersion)
})

it('should throw an error if the database query fails', async () => {
// Arrange
const sequelizeStub = sinon.createStubInstance(Sequelize)
sequelizeStub.query.rejects(new Error('Database query failed'))
const migrationManager = new MigrationManager(sequelizeStub, configPath)
const migrationManager = new MigrationManager(sequelizeStub, false, configPath)
migrationManager.checkOrCreateMigrationsMetaTable = sinon.stub().resolves()

// Act
Expand All @@ -236,7 +287,7 @@ describe('MigrationManager', () => {
// Create a migrationsMeta table and populate it with version and maxVersion
await sequelize.query('CREATE TABLE migrationsMeta (key VARCHAR(255), value VARCHAR(255))')
await sequelize.query("INSERT INTO migrationsMeta (key, value) VALUES ('version', '1.1.0'), ('maxVersion', '1.1.0')")
const migrationManager = new MigrationManager(sequelize, configPath)
const migrationManager = new MigrationManager(sequelize, false, configPath)
migrationManager.serverVersion = '1.2.0'

// Act
Expand All @@ -253,7 +304,7 @@ describe('MigrationManager', () => {
describe('extractVersionFromTag', () => {
it('should return null if tag is not provided', () => {
// Arrange
const migrationManager = new MigrationManager(sequelizeStub, configPath)
const migrationManager = new MigrationManager(sequelizeStub, false, configPath)

// Act
const result = migrationManager.extractVersionFromTag()
Expand All @@ -264,7 +315,7 @@ describe('MigrationManager', () => {

it('should return null if tag does not match the version format', () => {
// Arrange
const migrationManager = new MigrationManager(sequelizeStub, configPath)
const migrationManager = new MigrationManager(sequelizeStub, false, configPath)
const tag = 'invalid-tag'

// Act
Expand All @@ -276,7 +327,7 @@ describe('MigrationManager', () => {

it('should extract the version from the tag', () => {
// Arrange
const migrationManager = new MigrationManager(sequelizeStub, configPath)
const migrationManager = new MigrationManager(sequelizeStub, false, configPath)
const tag = 'v1.2.3'

// Act
Expand All @@ -290,7 +341,7 @@ describe('MigrationManager', () => {
describe('copyMigrationsToConfigDir', () => {
it('should copy migrations to the config directory', async () => {
// Arrange
const migrationManager = new MigrationManager(sequelizeStub, configPath)
const migrationManager = new MigrationManager(sequelizeStub, false, configPath)
migrationManager.migrationsDir = path.join(configPath, 'migrations')
const migrationsSourceDir = path.join(__dirname, '..', '..', '..', 'server', 'migrations')
const targetDir = migrationManager.migrationsDir
Expand All @@ -313,7 +364,7 @@ describe('MigrationManager', () => {

it('should throw an error if copying the migrations fails', async () => {
// Arrange
const migrationManager = new MigrationManager(sequelizeStub, configPath)
const migrationManager = new MigrationManager(sequelizeStub, false, configPath)
migrationManager.migrationsDir = path.join(configPath, 'migrations')
const migrationsSourceDir = path.join(__dirname, '..', '..', '..', 'server', 'migrations')
const targetDir = migrationManager.migrationsDir
Expand Down Expand Up @@ -484,7 +535,7 @@ describe('MigrationManager', () => {
const readdirStub = sinon.stub(fs, 'readdir').resolves(['v1.0.0-migration.js', 'v1.10.0-migration.js', 'v1.2.0-migration.js', 'v1.1.0-migration.js'])
const readFileSyncStub = sinon.stub(fs, 'readFileSync').returns('module.exports = { up: () => {}, down: () => {} }')
const umzugStorage = memoryStorage()
migrationManager = new MigrationManager(sequelizeStub, configPath)
migrationManager = new MigrationManager(sequelizeStub, false, configPath)
migrationManager.migrationsDir = path.join(configPath, 'migrations')
const resolvedMigrationNames = ['v1.0.0-migration.js', 'v1.1.0-migration.js', 'v1.2.0-migration.js', 'v1.10.0-migration.js']
const resolvedMigrationPaths = resolvedMigrationNames.map((name) => path.resolve(path.join(migrationManager.migrationsDir, name)))
Expand Down

0 comments on commit 8493e56

Please sign in to comment.