From 7021adbbf9837ccf321d7f0b55e5d917244c5191 Mon Sep 17 00:00:00 2001 From: Lane Campbell Date: Thu, 9 Apr 2026 14:34:56 -0700 Subject: [PATCH] fix: detect and re-run migration 029 when forms tables are missing (#762) Migration ID 029 was reassigned from "Ai Search Plugin" to "Add Forms System" between releases. Users upgrading from older versions had 029 marked as applied for the wrong migration, causing forms tables to never be created and cascading failures for migrations 030 and 033. - Add bidirectional auto-detection for migration 029 in autoDetectAppliedMigrations(): if 029 is recorded but forms tables don't exist, remove the stale record so it re-runs - Include per-migration error details in admin API response instead of generic "Failed to run migrations" - Add unit tests for all four detection scenarios Co-Authored-By: Claude Opus 4.6 --- .../plans/fix-762-migration-029-reuse-plan.md | 42 +++ packages/core/src/routes/admin-api.ts | 7 +- packages/core/src/services/migrations.test.ts | 241 ++++++++++++++++++ packages/core/src/services/migrations.ts | 32 ++- 4 files changed, 316 insertions(+), 6 deletions(-) create mode 100644 docs/ai/plans/fix-762-migration-029-reuse-plan.md create mode 100644 packages/core/src/services/migrations.test.ts 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 } }