From 55164803b07222467d1bfaf88ab08f8ebe32391d Mon Sep 17 00:00:00 2001 From: mikiher Date: Sat, 14 Sep 2024 08:01:32 +0300 Subject: [PATCH] Fix migrationMeta database version initial value, and move isDatabaseNew logic inside MigrationManager --- server/Database.js | 4 +- server/managers/MigrationManager.js | 24 +++++- test/server/managers/MigrationManager.test.js | 73 ++++++++++++++++--- 3 files changed, 85 insertions(+), 16 deletions(-) diff --git a/server/Database.js b/server/Database.js index 289bef09..a24be809 100644 --- a/server/Database.js +++ b/server/Database.js @@ -171,9 +171,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') diff --git a/server/managers/MigrationManager.js b/server/managers/MigrationManager.js index 53db461b..706e359c 100644 --- a/server/managers/MigrationManager.js +++ b/server/managers/MigrationManager.js @@ -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') @@ -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 { @@ -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.') @@ -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, @@ -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}"`) } } @@ -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}"`) } /** diff --git a/test/server/managers/MigrationManager.test.js b/test/server/managers/MigrationManager.test.js index ae28c0d1..ae94cd75 100644 --- a/test/server/managers/MigrationManager.test.js +++ b/test/server/managers/MigrationManager.test.js @@ -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() @@ -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' @@ -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 @@ -195,7 +210,26 @@ 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 + 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('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 @@ -211,11 +245,28 @@ describe('MigrationManager', () => { 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 @@ -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 @@ -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() @@ -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 @@ -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 @@ -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 @@ -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 @@ -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)))