Add missing nullable columns during db setup - #292
Open
sroussey wants to merge 1 commit into
Open
Conversation
`spac_candidate.signal_filed_sic_6770` was added to the schema after most databases had been created, and nothing adds it to an existing one: `CREATE TABLE IF NOT EXISTS` is a no-op on an existing table, `createStorage` declares no `tabularMigrations`, and `planColumnAlignment` explicitly skips a column the live schema lacks. `IdentifySpacsTask` writes full rows through `putBulk`, so `sec update spacs` fails on every pre-existing database. Fixed generically rather than with another hand-written ALTER, together with the prerequisite that makes a generic pass safe. - src/config/addMissingColumns.ts — a pure `planMissingColumns` plus a thin executor per backend, mirroring `alignPostgresColumnTypes`'s structure. Reuses that module's `nonNullType` / `admitsNull` / `declaredNullable` / `declaredStringType` rather than re-implementing them. Two rails: only NULLABLE columns are planned (SQLite rejects `ADD COLUMN NOT NULL` without a default, and there is no honest default for existing rows), and an unmappable declared type is skipped with a warning rather than guessed — a missing column fails loudly on the next write, a wrong type mismatches silently. - src/config/schemaTypeMirror.sqlite.test.ts — the prerequisite. Stating a type means mirroring a `workglow` DDL emitter this repo does not own. The test creates every registered table on real SQLite and requires the mirror to have predicted each emitted type; a column it declines must be in a short explicit allowlist. Zero mismatches across the registry, two allowlisted columns. Building it against the real emitter also corrected the type map: a bare `number` is `NUMERIC`, not `DOUBLE PRECISION`. - src/config/setupAllDatabases.ts — `SPAC_TRUST_COLUMNS` and both `ensureSpacCurrentTrustColumns*` deleted; those three columns are nullable, so the generic planner emits the same ALTERs (asserted). The new pass runs before `alignPostgresColumnTypes()` so a freshly-added column is eligible for widening in the same setup. `backfillExtractorRunsOutcome` stays hand-rolled — it seeds values from `success`. - Schema qualification, both passes. `addMissingColumnsPostgres` emits `ALTER TABLE "<schema>"."<table>"`, and `planColumnAlignment` now takes a required schema and qualifies its three statements too — it read its catalog `WHERE table_schema = current_schema()` while altering unqualified names, so on a multi-schema deployment it altered a different table than the one it measured. `quote` and `currentSchemaName` promoted out of `resetAllDatabases.ts` into `src/util/pgIdentifiers.ts`. Tests: real SQLite for the executor (including that `putBulk` throws before the pass and succeeds after, and a boolean round-tripping as null/true/false), a capturing fake pool for Postgres against a mixed-case `Staging` schema, and the mirror round-trip. Co-Authored-By: Claude <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The problem
spac_candidate.signal_filed_sic_6770was added to the schema after most databases had already been created, and nothing adds it to an existing one:CREATE TABLE IF NOT EXISTSis a no-op on a table that already exists;createStoragedeclares notabularMigrations(deliberately — the op set has noalterColumn);planColumnAlignmenthasif (!liveColumn) continue;— a missing column is explicitly skipped, since that pass aligns the types of columns that exist.IdentifySpacsTaskwrites full rows throughputBulk, sosec update spacsfails on every pre-existing database.The fix
Fixed generically rather than with another hand-written ALTER — but only together with the prerequisite that makes a generic pass safe.
src/config/addMissingColumns.tsA pure
planMissingColumns(declared, liveByTable)plus a thin executor per backend, mirroringalignPostgresColumnTypes's structure so the planner is unit-testable with no database. It reuses that module'snonNullType/admitsNull/declaredNullable/declaredStringType(now exported) rather than re-implementing them — that duplication is exactly the drift this is avoiding.Two safety rails, both load-bearing:
ADD COLUMN NOT NULLwithout a default, and there is no honest default for a signal nobody has computed for the existing rows. A NOT NULL addition needs a migration that decides what the backfill means. (signal_filed_sic_6770isTypeNullable(Type.Boolean()), so the live case is covered.)alignPostgresColumnTypestakes for a blocked ALTER.The prerequisite: a type-mirror round-trip test
Stating a type means carrying a JSON-Schema → DDL mirror of an emitter that lives in the
workglowdependency, so nothing in-repo forces it to stay honest.schemaTypeMirror.sqlite.test.tscreates every registered table from its schema on real SQLite, readsPRAGMA table_info, and requires the mirror to have predicted each emitted type. Any column the mapper returnsnullfor must be in a short explicit allowlist, so adding an unmappable type to a schema fails there rather than at 3am insidedb setup.Result: zero type mismatches across the whole registry, two allowlisted columns —
investment_offerings.exemptions(an array, where the two backends genuinely differ) andunderwriter_link.role_detail(declaredtype: ["string", "null"], a spelling the emitter itself does not recognize and answers with itsTEXT /* unknown type */fallback; mirroring a fallback that means "I do not recognize this" would then agree for every future unrecognized type too).Building the mirror against the real emitter rather than from the plan's sketch corrected it in two places worth naming: a bare
numberisNUMERIC, notDOUBLE PRECISION(that spelling needsformat: "double"), and an integer's width depends onminimum/maximumrather than being uniformlyBIGINT. Both would have created silently mistyped columns.src/config/setupAllDatabases.tsSPAC_TRUST_COLUMNSand bothensureSpacCurrentTrustColumns*deleted — verified first that all threespac.current_trust_*columns are nullable, and a test asserts the generic planner reaches them. The new pass runs beforealignPostgresColumnTypes()so a freshly-added column is eligible for widening in the samedb setup.backfillExtractorRunsOutcomestays hand-rolled: it seedsoutcomefrom the existingsuccessflag, which no generic pass can express.Schema qualification (both passes)
addMissingColumnsPostgresemitsALTER TABLE "<schema>"."<table>" ADD COLUMN IF NOT EXISTS .... AndplanColumnAlignmentnow takes a required schema and qualifies its three statements too — previously it read its catalogWHERE table_schema = current_schema()while emitting unqualifiedALTER TABLE "<table>", so on a deployment whosesearch_pathlists another schema first it altered a different table than the one whose catalog said the column was narrow.quote()andcurrentSchemaName()are promoted out ofresetAllDatabases.ts(both file-private) intosrc/util/pgIdentifiers.ts.Each ALTER is wrapped in try/catch +
console.warn: one column never abortsdb setup, which would skip the alignment pass, the view DDL, the resolver seeding and the rate-limiter tables after it.Tests
addMissingColumns.test.ts) — recreatespac_candidatewithout the column (DROP + re-CREATE rather thanDROP COLUMN: the more faithful simulation, and no SQLite ≥3.35 dependency), assertPRAGMA table_infolacks it and thatrepo.putBulkthrows — that second assertion pins the user-visible bug, not just the DDL. Then run the pass and assert the column returns,putBulksucceeds, and a boolean round-trips as null/true/false (SQLite's INTEGER affinity is exactly where a wrong mapping shows). Plus idempotence, a no-op on a fresh database, and thecurrent_trust_*subsumption.addMissingColumns.postgres.test.ts) — follows the repo's precedent,DbStatus.postgres.test.ts:vi.mock("../util/pg", …)with a capturing fake. The stubbedcurrent_schema()returns a mixed-caseStaging, which is what makes an unqualified or unquoted statement fail rather than pass by luck; the emitted statement is asserted to the character. Also: no statement for a column already present, none for a table the catalog does not report, and a NOT NULL column yielding a warn (naming both ways out) with the nullable column still added.db setupbehaviour on every existing databaseThe pass adds every nullable column currently missing across the whole registry — likely a handful nobody knew about, not just the one that motivated it. That is the point, but the first
db setupafter this ships is a real migration: run it against a copy first.Non-goals, all warned about rather than attempted: NOT NULL columns, type changes, drops, renames, backfills.
Also note the
alignPostgresColumnTypesqualification is a behaviour change on a multi-schema deployment — its ALTERs now land incurrent_schema()rather than wherever thesearch_pathresolved them.Verification
Spot-checked the other suites that drive
setupAllDatabases:Generated by Claude Code