diff --git a/backend/__tests__/unit/models/threadFollowByParticipation.test.js b/backend/__tests__/unit/models/threadFollowByParticipation.test.js index aa13423a3..07d80a753 100644 --- a/backend/__tests__/unit/models/threadFollowByParticipation.test.js +++ b/backend/__tests__/unit/models/threadFollowByParticipation.test.js @@ -22,7 +22,7 @@ const mockDb = newDb(); const mockPool = new (mockDb.adapters.createPg().Pool)(); jest.mock('../../../config/db-pg', () => ({ pool: mockPool })); -const { createTableFor } = require('../../utils/schemaTable'); +const { applyTable } = require('../../utils/schemaTable'); const ThreadUserState = require('../../../models/pg/ThreadUserState'); const POD = 'pod-1'; @@ -49,9 +49,9 @@ beforeAll(async () => { // The real dependency chain. thread_user_state -> messages -> pods, and the // hand-written fixture had none of it — another thing building from the // shipped DDL surfaces rather than hides. - await mockPool.query(createTableFor('pods')); - await mockPool.query(createTableFor('messages')); - await mockPool.query(createTableFor('thread_user_state')); + await applyTable(mockPool, 'pods'); + await applyTable(mockPool, 'messages'); + await applyTable(mockPool, 'thread_user_state'); await mockPool.query("INSERT INTO pods (id, name, type, created_by) VALUES ($1,'p','chat','u')", [POD]); }); diff --git a/backend/__tests__/unit/models/threadRootResolver.test.js b/backend/__tests__/unit/models/threadRootResolver.test.js index b4e519eb4..c5fe78d78 100644 --- a/backend/__tests__/unit/models/threadRootResolver.test.js +++ b/backend/__tests__/unit/models/threadRootResolver.test.js @@ -9,7 +9,7 @@ * what the reconciliation DOES with two possibly-disagreeing inputs. */ const { newDb } = require('pg-mem'); -const { createTableFor, applyTable } = require('../../utils/schemaTable'); +const { applyTable } = require('../../utils/schemaTable'); const mockDb = newDb(); const mockPool = new (mockDb.adapters.createPg().Pool)(); @@ -30,9 +30,12 @@ const msg = async (id, podId, rootId = null, replyTo = null) => { }; beforeAll(async () => { - await mockPool.query(createTableFor('pods')); - // applyTable, not createTableFor: `payload` and `thread_root_id` are added - // by ALTER, so the CREATE alone is not the table this code talks to. + // applyTable everywhere. `pods` has no ALTER retrofits TODAY, which is the + // only reason the bare CREATE was ever correct here — a property of this + // week's schema, not of this table. `payload` and `thread_root_id` are added + // to `messages` by ALTER, so the CREATE alone is not the table this code + // talks to; the next column added to `pods` makes that true of `pods` too. + await applyTable(mockPool, 'pods'); await applyTable(mockPool, 'users'); // findById LEFT JOINs it await applyTable(mockPool, 'messages'); for (const p of [POD, OTHER]) { diff --git a/backend/__tests__/unit/models/threadStateReadContract.test.js b/backend/__tests__/unit/models/threadStateReadContract.test.js index 68ddd5a0c..e7df874ef 100644 --- a/backend/__tests__/unit/models/threadStateReadContract.test.js +++ b/backend/__tests__/unit/models/threadStateReadContract.test.js @@ -55,7 +55,7 @@ jest.mock('../../../services/podWriteAccessService', () => ({ callerHasPodWriteAccess: async () => true, })); -const { createTableFor } = require('../../utils/schemaTable'); +const { applyTable } = require('../../utils/schemaTable'); const ThreadUserState = require('../../../models/pg/ThreadUserState'); const { listThreadState } = require('../../../controllers/threadStateController'); @@ -74,10 +74,10 @@ beforeAll(async () => { // The real dependency chain. thread_user_state -> messages -> pods, and the // hand-written fixture had none of it — another thing building from the // shipped DDL surfaces rather than hides. - await mockPool.query(createTableFor('pods')); - await mockPool.query(createTableFor('messages')); - await mockPool.query(createTableFor('thread_user_state')); - await mockPool.query(createTableFor('migration_records')); + await applyTable(mockPool, 'pods'); + await applyTable(mockPool, 'messages'); + await applyTable(mockPool, 'thread_user_state'); + await applyTable(mockPool, 'migration_records'); }); // Roots must be real message rows — thread_user_state has a live FK to // messages in the shipped schema, which the old hand-written fixture omitted. diff --git a/backend/__tests__/unit/services/threadWakeScope.test.js b/backend/__tests__/unit/services/threadWakeScope.test.js index f98eaa8ac..aaf121645 100644 --- a/backend/__tests__/unit/services/threadWakeScope.test.js +++ b/backend/__tests__/unit/services/threadWakeScope.test.js @@ -11,7 +11,7 @@ const mockDb = newDb(); const mockPool = new (mockDb.adapters.createPg().Pool)(); jest.mock('../../../config/db-pg', () => ({ pool: mockPool })); -const { createTableFor } = require('../../utils/schemaTable'); +const { applyTable } = require('../../utils/schemaTable'); const { effectiveFollowerIds, narrowToThread } = require('../../../services/threadWakeScopeService'); const ThreadUserState = require('../../../models/pg/ThreadUserState'); @@ -43,9 +43,9 @@ beforeAll(async () => { // Shipped DDL, not hand-written — same correction as 2/4's suites // (@sprint-review 56811). This file still had a hand-rolled `messages` and // `thread_user_state`, so every constraint it leaned on was one typed here. - await mockPool.query(createTableFor('pods')); - await mockPool.query(createTableFor('messages')); - await mockPool.query(createTableFor('thread_user_state')); + await applyTable(mockPool, 'pods'); + await applyTable(mockPool, 'messages'); + await applyTable(mockPool, 'thread_user_state'); await mockPool.query("INSERT INTO pods (id, name, type, created_by) VALUES ($1,'p','chat','u')", [POD]); }); diff --git a/backend/__tests__/utils/schemaTable.js b/backend/__tests__/utils/schemaTable.js index 4615948b9..83c677ab9 100644 --- a/backend/__tests__/utils/schemaTable.js +++ b/backend/__tests__/utils/schemaTable.js @@ -11,10 +11,21 @@ * Using this instead means a constraint dropped from schema.sql breaks the * tests that depend on it, which is the only arrangement where the test is * evidence about production. + * + * The rule the whole module exists to enforce: **a fixture built from part of + * the schema is a different schema.** It is the same family as pg-mem + * accepting a self-referential `ON DELETE CASCADE` and then not performing it + * (#1207) — in both cases the suite is green, the green means less than it + * looks, and the gap stays invisible until one specific column or constraint + * is exercised. Build the whole table, or the test is evidence about a + * database nobody ships. */ const fs = require('fs'); const path = require('path'); +// Module-private like the two getters. A test that legitimately needs the raw +// schema text can re-export it at that point, with a consumer to justify it — +// the standard applied to `createTableFor`, applied to its siblings. const SCHEMA_PATH = path.join(__dirname, '../../config/schema.sql'); /** @@ -22,6 +33,13 @@ const SCHEMA_PATH = path.join(__dirname, '../../config/schema.sql'); * Throws if absent — a silently-missing table would show up as a confusing * "relation does not exist" much later, which is the failure mode this whole * exercise is about. + * + * NOT EXPORTED, deliberately. A table with retrofits is never correctly built + * from this alone, so the pool-taking `applyTable` is the only thing a fixture + * can reach. The doc comment below said as much and four suites used it wrong + * anyway — @sprint-review's read: that is a signature problem, not a + * documentation problem, and a warning you have to obey is weaker than an + * export you cannot misuse. */ function createTableFor(name) { const sql = fs.readFileSync(SCHEMA_PATH, 'utf8'); @@ -34,6 +52,38 @@ function createTableFor(name) { /** * The `ALTER TABLE ADD COLUMN IF NOT EXISTS ...` retrofits for a table. * + * NOT EXPORTED, for the same reason as `createTableFor` and on the same + * evidence: zero consumers outside this file. @sprint-review caught that the + * commit removing one phantom export left two more of identical shape — + * `retrofitsFor` and `SCHEMA_PATH` — so the finding was two-thirds undone by + * the fix that made it. + * + * One correction to the reasoning, recorded so the wrong reason does not + * become the record: this is the SAFER of the two, not the worse one. Alone it + * throws `relation "messages" does not exist` on the first statement — + * measured, not assumed. `createTableFor` alone was the dangerous export + * precisely because it succeeded and produced a usable table missing the + * ALTER-only columns; the damage waited for a projection to touch `payload`. + * Loud-on-misuse versus silent-on-misuse is the whole distinction, and it runs + * the other way here. Unexported for consistency of the phantom-export rule, + * not because it was hazardous. + * + * That measurement has a PRECONDITION the first write-up omitted, and omitting + * it inverts the result rather than merely weakening it (@sprint-review). Seed + * `pods` first — `messages.pod_id REFERENCES pods(id)`. On a genuinely empty + * db both arms throw, and a reproducer concludes the asymmetry was imagined: + * + * fresh db retrofitsFor → `relation "messages" does not exist` + * createTableFor → `relation "pods" does not exist` + * pods seeded retrofitsFor → `relation "messages" does not exist` + * createTableFor → DDL ok, `payload` MISSING ← the finding + * + * Only `createTableFor` is sensitive to the seed; `retrofitsFor` throws the + * same error in both cells. So the confound could only ever make the dangerous + * export look safe, which is the direction that costs something. `users` is + * NOT required despite the obvious guess — `messages.user_id` is a bare + * VARCHAR(24) with no REFERENCES, and `pods` carries no FK of its own. + * * `createTableFor` alone is not the table. Late columns are added by ALTER, not * inside the CREATE — that is the two-declaration rule this repo learned the * hard way, and it applies to fixtures too. A suite that only ran the CREATE @@ -46,7 +96,25 @@ function retrofitsFor(name) { return sql.match(re) || []; } -/** The whole table as it exists after boot DDL: CREATE plus its retrofits. */ +/** + * The whole table as it exists after boot DDL: CREATE plus its retrofits. + * + * Prefer this over `createTableFor` in every fixture. @sprint-review found two + * suites still on the bare CREATE, and the reason it had not bitten them is + * pure luck rather than proof they are fine: `thread_root_id` happens to be + * declared in BOTH the CREATE and an ALTER, so the column the threading tests + * care about arrived either way. `payload` is declared ONLY in the ALTER, so + * those fixtures were carrying a `messages` with no `payload` — latent until + * one of them exercised a projection that selects it, which is a long way + * from the line that would have to change. + * + * This is now the only way in. An earlier draft of this comment kept + * `createTableFor` exported "because `retrofitsFor` and the guard tests need + * to read the two halves separately" — both consumers were phantom. + * `retrofitsFor` is a sibling in this file and never calls it, and no guard + * test imports this module at all (`git grep schemaTable` was the whole + * check). Checklist rule 7, inside the PR fixing the class it names. + */ async function applyTable(pool, name) { await pool.query(createTableFor(name)); for (const stmt of retrofitsFor(name)) { @@ -55,4 +123,6 @@ async function applyTable(pool, name) { } } -module.exports = { createTableFor, retrofitsFor, applyTable, SCHEMA_PATH }; +// One export. A fixture can build the whole table or nothing — there is no +// longer a reachable way to build part of one. +module.exports = { applyTable };