fix(migrations): correct vendor_contacts column corruption from buggy 0028#1097
Conversation
steilerDev
left a comment
There was a problem hiding this comment.
[product-architect]
Architecture Review: fix(migrations) — vendor_contacts column corruption
What was reviewed
- Migration 0028 fix (explicit column lists replacing
SELECT *) - Corrective migration 0029 (detect-and-reshuffle for already-corrupted databases)
- Integration test suite (5 scenarios)
Findings
Column shift analysis — verified correct. I traced through the full column order transformation:
- 0026 creates
vendor_contactswith columns: id, vendor_id, name, role, phone, email, notes, created_at, updated_at - 0027 appends via ALTER TABLE: ..., first_name, last_name (positions 9, 10)
- 0028 recreates with new order: id, vendor_id, name, first_name, last_name, role, phone, email, notes, created_at, updated_at
- The buggy
SELECT *would read in old order and insert positionally into new order, shifting role→first_name, phone→last_name, etc. - Migration 0029's reverse mapping is correct — each SET reads from the column that holds the displaced value
Detection strategy — sound. The GLOB pattern [0-9][0-9][0-9][0-9]-[0-9][0-9]-[0-9][0-9]* on the email column correctly identifies ISO datetime strings (the shifted created_at value). No valid email address starts with YYYY-MM-DD. The EXISTS subquery correctly applies the fix to ALL rows when ANY corruption is detected (table-wide backup/restore means all-or-nothing corruption).
SQLite atomicity — correctly leveraged. The comment about SQLite evaluating all RHS expressions before applying SET is accurate. This ensures the single UPDATE statement can safely reshuffle without temporary variables.
COALESCE fallback — defensive and appropriate. Since created_at and updated_at are NOT NULL in the schema, the corrupted email/notes columns (which hold the original timestamps) should never be NULL. The COALESCE(..., datetime('now')) is purely a safety net. Good defensive practice.
Idempotency — correctly designed. After the first run, email contains a real email address (not a datetime string), so the GLOB pattern won't match and the UPDATE becomes a no-op. Test scenario 5 validates this.
PRAGMA foreign_keys = OFF/ON — appropriate. The corrective migration disables FK checks during the column reshuffle since it's only moving data within the same row, not changing FK relationships.
Test quality — thorough. All 5 scenarios cover the important cases: empty table, clean data, corrupted data, NULL handling, and idempotency. The test correctly simulates corruption by applying the inverse column shift before running the fix.
Informational notes (no action required)
vendor_contactstable is missing from Schema.md wiki — this is a pre-existing gap from the EPIC that introduced migration 0026, not introduced by this PR. Should be addressed separately.- Migration 0028's fix (explicit column lists) is the right permanent fix — ensures fresh installs never encounter corruption. Migration 0029 is the corrective path for existing databases. Both approaches together provide complete coverage.
Verdict: Approve. The migration fix is architecturally sound. The detection, correction, and idempotency properties are all correctly designed and well-tested.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…d add recovery in 0029 Migration 0028 used SELECT * to backup vendor_contacts, but the actual column order in the old table (from 0026 + 0027 ALTER TABLE appends) differed from the new table schema, causing positional data shift during INSERT. Changes: - 0028: Use explicit column list in backup SELECT to preserve source order - 0028: Use explicit column mapping in INSERT to match new table schema - 0029: Add recovery migration to detect and fix corrupted databases This prevents future corruption and repairs existing databases that have shifted column values (e.g., created_at in email field detected by GLOB pattern). Co-Authored-By: Claude backend-developer (Haiku 4.5) <noreply@anthropic.com>
…uption fix Covers 5 scenarios: - No-op on empty vendor_contacts table - No-op on correctly-ordered data (real email not detected as datetime) - Corrects columns shifted by the buggy 0028 SELECT * pattern - Handles NULL first_name/last_name with COALESCE timestamp fallback - Idempotency: running 0029 twice leaves data unchanged Co-Authored-By: Claude qa-integration-tester (Sonnet 4.5) <noreply@anthropic.com>
… first_name Co-Authored-By: Claude qa-integration-tester (Sonnet 4.5) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
0171a4c to
6331380
Compare
|
🎉 This PR is included in version 2.1.1-beta.7 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 2.2.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Summary
SELECT *to backupvendor_contactsbefore recreating the table, but the old schema (columns appended viaALTER TABLEin 0027) had a different column order than the new table — causing values to shift to the wrong columns on the recreated tableemailvalue looks like an ISO datetime string (the shiftedcreated_atvalue), then performs a single-statementUPDATEto reshuffle all columns back to their correct positions usingCOALESCEfallback for timestamps that were originallyNULLTest Coverage
5 integration tests in
server/src/db/migrations/0029_fix_vendor_contacts_columns.test.ts:vendor_contactstableNULLfirst_name/last_nameare restored asNULL;COALESCEensures timestamps are neverNULLafter repairTest plan
🤖 Generated with Claude Code