Skip to content

Add missing nullable columns during db setup - #292

Open
sroussey wants to merge 1 commit into
mainfrom
claude/keen-knuth-onotd9-add-missing-columns
Open

Add missing nullable columns during db setup#292
sroussey wants to merge 1 commit into
mainfrom
claude/keen-knuth-onotd9-add-missing-columns

Conversation

@sroussey

Copy link
Copy Markdown
Contributor

The problem

spac_candidate.signal_filed_sic_6770 was added to the schema after most databases had already been created, and nothing adds it to an existing one:

  • CREATE TABLE IF NOT EXISTS is a no-op on a table that already exists;
  • createStorage declares no tabularMigrations (deliberately — the op set has no alterColumn);
  • planColumnAlignment has if (!liveColumn) continue; — a missing column is explicitly skipped, since that pass aligns the types of columns that exist.

IdentifySpacsTask writes full rows through putBulk, so sec update spacs fails 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.ts

A pure planMissingColumns(declared, liveByTable) plus a thin executor per backend, mirroring alignPostgresColumnTypes's structure so the planner is unit-testable with no database. It reuses that module's nonNullType / 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:

  1. Only NULLABLE columns are planned. SQLite rejects ADD COLUMN NOT NULL without 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_6770 is TypeNullable(Type.Boolean()), so the live case is covered.)
  2. An unmappable declared type is skipped with a warning, never guessed. A missing column fails loudly on the next write; a column created at the wrong type is accepted and mismatches silently until some value does not fit. Same posture alignPostgresColumnTypes takes 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 workglow dependency, so nothing in-repo forces it to stay honest. schemaTypeMirror.sqlite.test.ts creates every registered table from its schema on real SQLite, reads PRAGMA table_info, and requires the mirror to have predicted each emitted type. Any column the mapper returns null for must be in a short explicit allowlist, so adding an unmappable type to a schema fails there rather than at 3am inside db setup.

Result: zero type mismatches across the whole registry, two allowlisted columns — investment_offerings.exemptions (an array, where the two backends genuinely differ) and underwriter_link.role_detail (declared type: ["string", "null"], a spelling the emitter itself does not recognize and answers with its TEXT /* 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 number is NUMERIC, not DOUBLE PRECISION (that spelling needs format: "double"), and an integer's width depends on minimum/maximum rather than being uniformly BIGINT. Both would have created silently mistyped columns.

src/config/setupAllDatabases.ts

SPAC_TRUST_COLUMNS and both ensureSpacCurrentTrustColumns* deleted — verified first that all three spac.current_trust_* columns are nullable, and a test asserts the generic planner reaches them. The new pass runs before alignPostgresColumnTypes() so a freshly-added column is eligible for widening in the same db setup. backfillExtractorRunsOutcome stays hand-rolled: it seeds outcome from the existing success flag, which no generic pass can express.

Schema qualification (both passes)

addMissingColumnsPostgres emits ALTER TABLE "<schema>"."<table>" ADD COLUMN IF NOT EXISTS .... And planColumnAlignment now takes a required schema and qualifies its three statements too — previously it read its catalog WHERE table_schema = current_schema() while emitting unqualified ALTER TABLE "<table>", so on a deployment whose search_path lists another schema first it altered a different table than the one whose catalog said the column was narrow. quote() and currentSchemaName() are promoted out of resetAllDatabases.ts (both file-private) into src/util/pgIdentifiers.ts.

Each ALTER is wrapped in try/catch + console.warn: one column never aborts db setup, which would skip the alignment pass, the view DDL, the resolver seeding and the rate-limiter tables after it.

Tests

  • Real SQLite, no mocks (addMissingColumns.test.ts) — recreate spac_candidate without the column (DROP + re-CREATE rather than DROP COLUMN: the more faithful simulation, and no SQLite ≥3.35 dependency), assert PRAGMA table_info lacks it and that repo.putBulk throws — that second assertion pins the user-visible bug, not just the DDL. Then run the pass and assert the column returns, putBulk succeeds, 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 the current_trust_* subsumption.
  • Postgres (addMissingColumns.postgres.test.ts) — follows the repo's precedent, DbStatus.postgres.test.ts: vi.mock("../util/pg", …) with a capturing fake. The stubbed current_schema() returns a mixed-case Staging, 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.
  • The mirror round-trip, SQLite-only.

⚠️ This changes db setup behaviour on every existing database

The 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 setup after 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 alignPostgresColumnTypes qualification is a behaviour change on a multi-schema deployment — its ALTERs now land in current_schema() rather than wherever the search_path resolved them.

Verification

$ bun run test -- src/config/
 Test Files  15 passed | 1 skipped (16)
      Tests  94 passed | 5 skipped (99)

$ npx tsc --noEmit -p tsconfig.json
(clean)

Spot-checked the other suites that drive setupAllDatabases:

$ bun run test -- src/storage/address/AddressRegionNullableMigration.test.ts \
    src/task/spac/spacCandidateScan.test.ts \
    src/storage/form-8k-event/Form8KEventLegacyMigration.test.ts src/util/
 Test Files  13 passed (13)
      Tests  100 passed (100)

Generated by Claude Code

`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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants