diff --git a/docs/ai/plans/fix-762-migration-029-reuse-plan.md b/docs/ai/plans/fix-762-migration-029-reuse-plan.md new file mode 100644 index 000000000..e7b52d2ba --- /dev/null +++ b/docs/ai/plans/fix-762-migration-029-reuse-plan.md @@ -0,0 +1,42 @@ +# Fix #762: Migration ID 029 Reused Across Versions + +## Overview +Migration ID `029` was reassigned from "Ai Search Plugin" to "Add Forms System" between SonicJS releases. Users upgrading from older versions have `029` marked as applied (for AI Search), so the forms migration is skipped, causing cascading failures for migrations `030` and `033` which depend on the `forms` and `form_submissions` tables. + +## Requirements +- [x] Add auto-detection for migration 029 (forms tables) in `autoDetectAppliedMigrations()` +- [x] If 029 is marked as applied but `forms` table doesn't exist, remove it so it re-runs (same pattern as migration 011) +- [x] Improve error messaging in admin API to include per-migration failure details +- [x] Add unit tests for the new detection logic + +## Technical Approach + +### Architecture +This follows the existing pattern used for migration `011` (managed column detection), which already handles the case where a migration is marked as applied but its artifacts don't exist. We extend this pattern to migration `029`. + +### File Changes +| File | Action | Description | +|------|--------|-------------| +| `packages/core/src/services/migrations.ts` | Modify | Add 029 auto-detection with existence check (like 011 pattern) | +| `packages/core/src/routes/admin-api.ts` | Modify | Include per-migration error details in response | +| `packages/core/src/services/migrations.test.ts` | Create/Modify | Unit tests for new detection logic | + +### Database Changes +None — this only changes detection logic, not schema. + +## Implementation Steps +1. Add forms table auto-detection for migration 029 in `autoDetectAppliedMigrations()` using the same bidirectional pattern as migration 011 +2. Improve admin API error response to include specific migration failure details +3. Write unit tests +4. Run existing tests to verify no regressions + +## Testing Strategy + +### Unit Tests +- Migration 029 marked as applied + forms table exists → keep as applied +- Migration 029 marked as applied + forms table missing → remove, mark as pending +- Migration 029 not applied + forms table exists → mark as applied +- Migration 029 not applied + forms table missing → leave as pending + +## Approval +- [ ] Plan reviewed and approved by user diff --git a/packages/core/src/routes/admin-api.ts b/packages/core/src/routes/admin-api.ts index 4f94ad8b5..9b165fe2d 100644 --- a/packages/core/src/routes/admin-api.ts +++ b/packages/core/src/routes/admin-api.ts @@ -719,13 +719,16 @@ adminApiRoutes.post('/migrations/run', async (c) => { return c.json({ success: result.success, message: result.message, - applied: result.applied + applied: result.applied, + errors: result.errors }) } catch (error) { console.error('Error running migrations:', error) + const errorMessage = error instanceof Error ? error.message : String(error) return c.json({ success: false, - error: 'Failed to run migrations' + error: `Failed to run migrations: ${errorMessage}`, + errors: [errorMessage] }, 500) } }) diff --git a/packages/core/src/services/migrations.test.ts b/packages/core/src/services/migrations.test.ts new file mode 100644 index 000000000..933e2d626 --- /dev/null +++ b/packages/core/src/services/migrations.test.ts @@ -0,0 +1,241 @@ +/** + * Migration Service Tests - Auto-detection for migration 029 + * Tests the fix for issue #762: Migration ID 029 reused across versions + */ + +import { describe, it, expect, beforeEach, vi } from 'vitest' +import { MigrationService } from './migrations' + +// Mock D1Database with configurable responses +function createMockDb(options: { + appliedMigrations?: Array<{ id: string; name: string; filename: string; applied_at: string }>; + existingTables?: string[]; + existingColumns?: Array<{ table: string; column: string }>; +} = {}) { + const { + appliedMigrations = [], + existingTables = [], + existingColumns = [] + } = options + + const mockRun = vi.fn().mockResolvedValue({ success: true }) + const mockFirst = vi.fn() + const mockAll = vi.fn() + const mockBind = vi.fn() + + // Track queries to respond appropriately + let lastQuery = '' + + const chainable = { + bind: (...args: any[]) => { + mockBind(...args) + // For table existence checks + if (lastQuery.includes('sqlite_master')) { + const tableName = args[0] + return { + first: vi.fn().mockResolvedValue( + existingTables.includes(tableName) ? { name: tableName } : null + ), + all: mockAll, + run: mockRun, + bind: chainable.bind + } + } + // For column existence checks + if (lastQuery.includes('pragma_table_info')) { + const [table, column] = args + const exists = existingColumns.some(c => c.table === table && c.column === column) + return { + first: vi.fn().mockResolvedValue(exists ? { name: column } : null), + all: mockAll, + run: mockRun, + bind: chainable.bind + } + } + // For migration table queries + if (lastQuery.includes('SELECT id, name, filename, applied_at FROM migrations')) { + return { + first: mockFirst, + all: vi.fn().mockResolvedValue({ results: appliedMigrations }), + run: mockRun, + bind: chainable.bind + } + } + // For migration applied check (SELECT COUNT) + if (lastQuery.includes('SELECT COUNT')) { + const migrationId = args[0] + const exists = appliedMigrations.some(m => m.id === migrationId) + return { + first: vi.fn().mockResolvedValue({ count: exists ? 1 : 0 }), + all: mockAll, + run: mockRun, + bind: chainable.bind + } + } + return { + first: mockFirst, + all: mockAll, + run: mockRun, + bind: chainable.bind + } + }, + first: mockFirst, + all: vi.fn().mockResolvedValue({ results: appliedMigrations }), + run: mockRun + } + + const mockPrepare = vi.fn((query: string) => { + lastQuery = query + // For CREATE TABLE (init migrations table) + if (query.includes('CREATE TABLE IF NOT EXISTS migrations')) { + return { run: mockRun, bind: chainable.bind, first: mockFirst, all: mockAll } + } + // For SELECT from migrations (applied migrations list) + if (query.includes('SELECT id, name, filename, applied_at FROM migrations')) { + return { + all: vi.fn().mockResolvedValue({ results: appliedMigrations }), + bind: chainable.bind, + first: mockFirst, + run: mockRun + } + } + return chainable + }) + + return { + prepare: mockPrepare, + _mocks: { prepare: mockPrepare, bind: mockBind, first: mockFirst, all: mockAll, run: mockRun } + } +} + +describe('MigrationService', () => { + describe('autoDetectAppliedMigrations - migration 029 (forms)', () => { + it('should mark 029 as applied when forms tables exist but migration is not recorded', async () => { + const db = createMockDb({ + appliedMigrations: [], + existingTables: ['forms', 'form_submissions', 'form_files'], + existingColumns: [] + }) + + const service = new MigrationService(db as any) + const migrations = await service.getAvailableMigrations() + + const migration029 = migrations.find(m => m.id === '029') + expect(migration029?.applied).toBe(true) + + // Verify markMigrationApplied was called for 029 + const insertCalls = db._mocks.prepare.mock.calls.filter( + (call: any[]) => call[0].includes('INSERT OR REPLACE') + ) + const marked029 = insertCalls.some((call: any[]) => { + // The bind call after INSERT would contain '029' + return true // We just verify the INSERT was called + }) + expect(insertCalls.length).toBeGreaterThan(0) + }) + + it('should remove 029 from applied when marked as applied but forms tables are missing', async () => { + const db = createMockDb({ + appliedMigrations: [ + { id: '029', name: 'Ai Search Plugin', filename: '029_ai_search_plugin.sql', applied_at: '2024-01-01' } + ], + existingTables: [], // No forms tables + existingColumns: [] + }) + + const service = new MigrationService(db as any) + const migrations = await service.getAvailableMigrations() + + const migration029 = migrations.find(m => m.id === '029') + expect(migration029?.applied).toBe(false) + + // Verify removeMigrationApplied was called (DELETE query) + const deleteCalls = db._mocks.prepare.mock.calls.filter( + (call: any[]) => call[0].includes('DELETE FROM migrations') + ) + expect(deleteCalls.length).toBeGreaterThan(0) + }) + + it('should keep 029 as applied when marked as applied and forms tables exist', async () => { + const db = createMockDb({ + appliedMigrations: [ + { id: '029', name: 'Add Forms System', filename: '029_add_forms_system.sql', applied_at: '2024-01-01' } + ], + existingTables: ['forms', 'form_submissions', 'form_files'], + existingColumns: [] + }) + + const service = new MigrationService(db as any) + const migrations = await service.getAvailableMigrations() + + const migration029 = migrations.find(m => m.id === '029') + expect(migration029?.applied).toBe(true) + }) + + it('should leave 029 as pending when not applied and forms tables do not exist', async () => { + const db = createMockDb({ + appliedMigrations: [], + existingTables: [], // No forms tables + existingColumns: [] + }) + + const service = new MigrationService(db as any) + const migrations = await service.getAvailableMigrations() + + const migration029 = migrations.find(m => m.id === '029') + expect(migration029?.applied).toBe(false) + }) + + it('should not mark 029 as applied when only some forms tables exist', async () => { + const db = createMockDb({ + appliedMigrations: [], + existingTables: ['forms'], // Missing form_submissions and form_files + existingColumns: [] + }) + + const service = new MigrationService(db as any) + const migrations = await service.getAvailableMigrations() + + const migration029 = migrations.find(m => m.id === '029') + expect(migration029?.applied).toBe(false) + }) + }) + + describe('runPendingMigrations', () => { + it('should include errors array in response', async () => { + const db = createMockDb({ + appliedMigrations: [], + existingTables: ['users', 'content', 'collections', 'media'], + existingColumns: [] + }) + + const service = new MigrationService(db as any) + const result = await service.runPendingMigrations() + + expect(result).toHaveProperty('errors') + expect(Array.isArray(result.errors)).toBe(true) + }) + + it('should return empty errors when all migrations are up to date', async () => { + // Create a db where all bundled migrations are already applied + const db = createMockDb({ + appliedMigrations: [], + existingTables: ['users', 'content', 'collections', 'media'], + existingColumns: [] + }) + + const service = new MigrationService(db as any) + // Mock getMigrationStatus to return no pending + vi.spyOn(service, 'getMigrationStatus').mockResolvedValue({ + totalMigrations: 0, + appliedMigrations: 0, + pendingMigrations: 0, + migrations: [] + }) + + const result = await service.runPendingMigrations() + expect(result.errors).toEqual([]) + expect(result.success).toBe(true) + }) + }) +}) diff --git a/packages/core/src/services/migrations.ts b/packages/core/src/services/migrations.ts index e6a684f3e..9c863b2ec 100644 --- a/packages/core/src/services/migrations.ts +++ b/packages/core/src/services/migrations.ts @@ -228,6 +228,27 @@ export class MigrationService { } } + // Check if forms tables exist (migration 029) + // Migration 029 was reassigned between releases: older versions used it for "Ai Search Plugin", + // newer versions use it for "Add Forms System". This handles the case where 029 is marked as + // applied (from the old AI Search migration) but the forms tables don't actually exist. + const hasFormsTables = await this.checkTablesExist(['forms', 'form_submissions', 'form_files']) + if (!appliedMigrations.has('029') && hasFormsTables) { + appliedMigrations.set('029', { + id: '029', + applied_at: new Date().toISOString(), + name: 'Add Forms System', + filename: '029_add_forms_system.sql' + }) + await this.markMigrationApplied('029', 'Add Forms System', '029_add_forms_system.sql') + } else if (appliedMigrations.has('029') && !hasFormsTables) { + // Migration was marked as applied (possibly from old "Ai Search Plugin" migration) + // but forms tables don't exist - remove it so the forms migration will re-run + console.log('[Migration] Migration 029 marked as applied but forms tables missing - will re-run') + appliedMigrations.delete('029') + await this.removeMigrationApplied('029') + } + // Check if user_profiles table exists (migration 032) // Table may already exist from app-level migration (my-sonicjs-app/migrations/018_user_profiles.sql) if (!appliedMigrations.has('032')) { @@ -361,7 +382,7 @@ export class MigrationService { /** * Run pending migrations */ - async runPendingMigrations(): Promise<{ success: boolean; message: string; applied: string[] }> { + async runPendingMigrations(): Promise<{ success: boolean; message: string; applied: string[]; errors: string[] }> { await this.initializeMigrationsTable() const status = await this.getMigrationStatus() @@ -371,7 +392,8 @@ export class MigrationService { return { success: true, message: 'All migrations are up to date', - applied: [] + applied: [], + errors: [] } } @@ -399,7 +421,8 @@ export class MigrationService { return { success: false, message: `Failed to apply migrations: ${errors.join('; ')}`, - applied + applied, + errors } } @@ -408,7 +431,8 @@ export class MigrationService { message: applied.length > 0 ? `Applied ${applied.length} migration(s)${errors.length > 0 ? ` (${errors.length} failed)` : ''}` : 'No migrations applied', - applied + applied, + errors } }