From 8f825e445ed1b6e935aae97d2b27f5ca557de79c Mon Sep 17 00:00:00 2001 From: jubnl Date: Thu, 24 Sep 2026 05:22:31 +0200 Subject: [PATCH] =?UTF-8?q?fix(server):=20plan=204=20=E2=80=94=20un-skip?= =?UTF-8?q?=20PARITY-009=20(FK=20deleteRule=20and=20index-name=20parity)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- server/scripts/generate-entities.ts | 173 +++++++++++++++++- .../entities/AssignmentParticipants.entity.ts | 4 +- server/src/db/entities/BudgetItems.entity.ts | 2 +- .../db/entities/BudgetSettlements.entity.ts | 2 +- .../db/entities/DawarichConnections.entity.ts | 2 +- .../db/entities/JourneyContributors.entity.ts | 2 +- server/src/db/entities/OauthTokens.entity.ts | 2 +- server/src/db/entities/PlaceRegions.entity.ts | 2 +- .../entities/ReservationTravelers.entity.ts | 4 +- .../db/entities/RoadtripDayTracks.entity.ts | 2 +- server/src/db/entities/TripMembers.entity.ts | 2 +- .../db/entities/VacayUserSettings.entity.ts | 2 +- .../unit/db/entity-schema-parity.test.ts | 128 +++++++++---- .../tests/unit/db/generate-entities.test.ts | 135 ++++++++++++++ 14 files changed, 409 insertions(+), 53 deletions(-) diff --git a/server/scripts/generate-entities.ts b/server/scripts/generate-entities.ts index b2c7339d9..3681e177c 100644 --- a/server/scripts/generate-entities.ts +++ b/server/scripts/generate-entities.ts @@ -968,6 +968,125 @@ export function RULE12_fixGarbledCheckExpressions(metadata: EntityMetadata[]): C return fixups; } +/** A relation whose `deleteRule` RULE13 had to pin explicitly because MikroORM's own runtime default would otherwise disagree with `PRAGMA foreign_key_list`. */ +export interface DeleteRuleFixup { + className: string; + propName: string; + from: string | undefined; + to: string; +} + +/** + * RULE13 (metadata level; DB-dependent — see `collectForeignKeyDeleteRules` + * below for the half that queries the schema): every owning to-one + * relation's EFFECTIVE `deleteRule` — explicit if the entity sets one, + * otherwise whatever `@mikro-orm/core`'s `MetadataDiscovery` infers by + * default at runtime — must equal `PRAGMA foreign_key_list`'s `on_delete` + * for its column. Plan 4 Task 7's own measurement + * (`tests/unit/db/entity-schema-parity.test.ts`, PARITY-009b) found exactly + * 10 owning relations across 120 entities where it does not. + * + * The root cause, read directly out of + * `node_modules/@mikro-orm/core/metadata/MetadataDiscovery.js` (not + * assumed): a nullable owning relation defaults to `deleteRule ??= 'set + * null'` (`initManyToOneFields`/`initOneToOneFields`); separately, when + * EVERY one of an entity's primary keys is itself a relation (composite or + * single FK-as-PK), each of those relations defaults to `deleteRule ??= + * 'cascade'` (`processEntity`'s `fkPks` block) — and the nullable check runs + * FIRST, so a relation that is both nullable and part of an all-FK PK still + * gets `'set null'`, never `'cascade'`. Neither default is announced by the + * generated source when the introspected value happens to equal it — Rule 7 + * above strips the redundant `deleteRule('no action')` noise, and + * `@mikro-orm/entity-generator`'s OWN `cleanUpReferentialIntegrityRules` + * (`EntityGenerator.js`) strips a redundant `'cascade'`/`'set null'` the + * same way for the FK-as-PK, fixed-order-pivot and nullable-relation shapes + * — but neither cleanup step re-derives the SAME default this rule computes + * here, so when the physical schema's real `on_delete` differs from what + * MikroORM would infer left implicit, the generated entity silently carries + * the wrong effective rule. Concretely, of the 10: 5 are a nullable relation + * whose column has no `ON DELETE` clause at all in its migration (SQLite ⇒ + * `NO ACTION`) — `trip_members.invited_by`, `oauth_tokens.parent_token_id`, + * `budget_settlements.created_by_user_id`, `budget_items.paid_by_user_id`, + * and `roadtrip_day_tracks.day_id` (nullable AND the table's sole PK — its + * migration DOES say `ON DELETE CASCADE`, so this one goes the other way: + * physical is `cascade`, the nullable default silently gives `set null`); + * `journey_contributors.user_id` is a composite-PK member whose migration + * has no `ON DELETE` (physical `no action`) while its FK-as-PK sibling + * relation, `journey_id`, DOES say `ON DELETE CASCADE` (so its own implicit + * `cascade` default is correct — only `user_id` disagrees); the remaining 4 + * (`reservation_travelers.reservation_id`/`.user_id`, + * `assignment_participants.assignment_id`/`.user_id`) are plain non-nullable + * relations on a table with its own `id INTEGER PRIMARY KEY AUTOINCREMENT` + * — no implicit default applies to them at all (MikroORM has none for that + * shape) — but `@mikro-orm/entity-generator`'s pivot-shaped-table cleanup + * (Case 2: a single autoincrement PK + exactly 2 many-to-one relations and + * no other columns) strips their introspected `'cascade'` on the mistaken + * assumption that MikroORM would restore it as a default, which it does not + * for a non-composite-PK entity; their migrations DO say `ON DELETE CASCADE`. + * + * Every one of the 10 is, per the task's own ruling, an entity that never + * matched its migration's physical FK to begin with — not a migration + * defect — so this rule always fixes the ENTITY side: it pins an explicit + * `deleteRule` equal to the physical value whenever leaving the property + * alone (explicit-if-set, else the same implicit default computed above) + * would produce the wrong effective rule, and otherwise leaves the property + * untouched — matching Rule 7's own "no noise where the implicit default is + * already correct" intent instead of stamping every relation explicitly. + * + * One more wrinkle, found empirically (probed `t.orm.getMetadata()` against + * the real `ALL_ENTITIES`, not assumed): a relation that is BOTH nullable + * AND its entity's sole/composite FK-as-PK member — `RoadtripDayTracks.day`, + * `DawarichConnections.user`, `PlaceRegions.place`, `VacayUserSettings.user`, + * every one of them shaped exactly like `.primary().ref().nullable()` with + * no explicit `deleteRule` — is where BOTH candidate defaults above apply at + * once, and the doc comment's "nullable runs first" precedence is only true + * WITHIN a single `processEntity(meta)` call. Across entities, `Days`'/ + * `Users`'/`Places`' own inverse `.mappedBy(...)` side of the same relation + * also runs `initManyToOneFields`, and depending on `ALL_ENTITIES` + * discovery order that can re-touch the owning prop before ITS OWN + * `processEntity` runs — measured outcome: `RoadtripDayTracks.day` really + * does resolve to `'set null'` at runtime (matching this task's 10), while + * the other three resolve to `'cascade'` (NOT in the 10) despite identical + * source. That order dependency is exactly the kind of fact a future entity + * addition could silently flip, so this rule does not trust "already + * correct" for this one shape — `isAmbiguousNullablePrimaryFk` below always + * pins an explicit `deleteRule` on it, matching physical, even on the three + * that happen to already be correct today. + */ +export function RULE13_pinDeleteRuleDrift( + metadata: EntityMetadata[], + fkDeleteRules: ReadonlyMap>, +): DeleteRuleFixup[] { + const fixups: DeleteRuleFixup[] = []; + for (const meta of metadata) { + const byColumn = fkDeleteRules.get(meta.tableName); + if (!byColumn) continue; + const pks = meta.getPrimaryProps().filter((pk): pk is EntityProperty => pk !== undefined); + const allPksAreFk = pks.length > 0 && pks.every((pk) => pk.kind !== undefined && pk.kind !== ReferenceKind.SCALAR); + for (const prop of meta.relations) { + if (!isOwningToOne(prop) || prop.fieldNames.length !== 1) continue; + const physical = byColumn.get(prop.fieldNames[0]); + if (physical === undefined) continue; // no db FK on this column at all — PARITY-009 proper's job, not this rule's + // The two candidate implicit defaults collide on this one shape — see + // the doc comment's "one more wrinkle" — so never trust "already + // correct" here; always pin it explicitly. + const isAmbiguousNullablePrimaryFk = !!prop.nullable && !!prop.primary && allPksAreFk; + if (!isAmbiguousNullablePrimaryFk) { + // The same precedence MetadataDiscovery applies at runtime within a single + // entity's own processEntity() call: nullable wins over FK-as-PK. + const implicitDefault = prop.nullable ? 'set null' : prop.primary && allPksAreFk ? 'cascade' : 'no action'; + const effectiveIfLeftAsIs = (prop.deleteRule ?? implicitDefault).toLowerCase(); + if (effectiveIfLeftAsIs === physical) continue; // already correct, explicit or by the implicit default alike + } else if ((prop.deleteRule ?? '').toLowerCase() === physical) { + continue; // already pinned explicitly to the right value — nothing to change + } + fixups.push({ className: meta.className, propName: prop.name, from: prop.deleteRule, to: physical }); + prop.deleteRule = physical; + } + } + return fixups; +} + /** A scalar property whose literal default the renderer's own heuristic drops or mis-renders — see below. */ export interface DefaultFixup { className: string; @@ -1048,17 +1167,21 @@ export interface RuleFixups { referencedColumns: ReferencedColumnsFixup[]; /** Rule 12's garbled-CHECK-expression repairs. */ checkExpressions: CheckExpressionFixup[]; + /** Rule 13's deleteRule-drift pins — reporting only, no text pass depends on it (the renderer already emits an explicit `.deleteRule(...)` from `prop.deleteRule` alone). */ + deleteRules: DeleteRuleFixup[]; } /** * Every rule above, composed into the single hook MikroORM calls. Order - * matters (see comments). `implicitUniques` defaults to empty so a caller - * (the fixture tests) that doesn't care about Rule 9 need not pass it. + * matters (see comments). `implicitUniques`/`fkDeleteRules` default to empty + * so a caller (the fixture tests) that doesn't care about Rule 9/Rule 13 + * need not pass them. */ export function applyRules( metadata: EntityMetadata[], _platform: Platform, implicitUniques: ReadonlyMap = new Map(), + fkDeleteRules: ReadonlyMap> = new Map(), ): RuleFixups { const retypedByRule1 = RULE1_fixUnknownScalarTypes(metadata); const jsonColumns = RULE1b_markJsonColumns(metadata); @@ -1089,6 +1212,11 @@ export function applyRules( // the renderer dumps verbatim — no text pass depends on it, same as // RULE7/RULE9); placed last only by convention (highest rule number). const checkExpressions = RULE12_fixGarbledCheckExpressions(metadata); + // Must run after RULE7 (which already dropped the genuinely redundant + // `deleteRule('no action')` noise) so `prop.deleteRule` reflects what the + // generated source would actually carry before this rule decides whether + // that's still wrong against the live schema. + const deleteRules = RULE13_pinDeleteRuleDrift(metadata, fkDeleteRules); return { joinColumns, defaults, @@ -1098,6 +1226,7 @@ export function applyRules( repositoryMarkers, referencedColumns, checkExpressions, + deleteRules, }; } @@ -1139,6 +1268,31 @@ export async function collectImplicitUniqueIndexes( return result; } +/** + * DB-dependent half of Rule 13: for every table name given, `PRAGMA + * foreign_key_list`'s `on_delete` per `from` (owning) column, lowercased — + * the same physical fact `tests/unit/db/entity-schema-parity.test.ts`'s + * PARITY-009b reads at test time, queried once here so `RULE13_pinDeleteRuleDrift` + * stays a pure function over plain data, same shape as Rule 9's split. + */ +export async function collectForeignKeyDeleteRules( + connection: Connection, + tableNames: readonly string[], +): Promise>> { + const result = new Map>(); + for (const tableName of tableNames) { + const fkRows = (await connection.execute(`pragma foreign_key_list(\`${tableName}\`)`, [], 'all')) as { + from: string; + on_delete: string; + }[]; + if (fkRows.length === 0) continue; + const byColumn = new Map(); + for (const row of fkRows) byColumn.set(row.from, row.on_delete.toLowerCase()); + result.set(tableName, byColumn); + } + return result; +} + // --------------------------------------------------------------------------- // onImport — resolves the two identifiers the rules above add to a file's // entityImports that are NOT sibling `*.entity.ts` classes: `DbTimestampType` @@ -1749,6 +1903,7 @@ export async function generateEntities(): Promise { repositoryMarkers: [], referencedColumns: [], checkExpressions: [], + deleteRules: [], }; const rawFiles = await generator.generate({ entityDefinition: 'defineEntity', @@ -1762,13 +1917,12 @@ export async function generateEntities(): Promise { onImport, onProcessedMetadata: async (metadata, platform) => { // Same connection the generator itself just introspected the schema - // through (`orm`, in scope from just above) — Rule 9 needs no - // second connection or a second migrated temp DB. - const implicitUniques = await collectImplicitUniqueIndexes( - orm.em.getConnection(), - metadata.map((meta) => meta.tableName), - ); - fixups = applyRules(metadata, platform, implicitUniques); + // through (`orm`, in scope from just above) — Rule 9/Rule 13 need + // no second connection or a second migrated temp DB. + const tableNames = metadata.map((meta) => meta.tableName); + const implicitUniques = await collectImplicitUniqueIndexes(orm.em.getConnection(), tableNames); + const fkDeleteRules = await collectForeignKeyDeleteRules(orm.em.getConnection(), tableNames); + fixups = applyRules(metadata, platform, implicitUniques, fkDeleteRules); }, }); @@ -1786,6 +1940,7 @@ export async function generateEntities(): Promise { repositoryMarkers: fixups.repositoryMarkers.filter((f) => f.className === className), referencedColumns: fixups.referencedColumns.filter((f) => f.className === className), checkExpressions: fixups.checkExpressions.filter((f) => f.className === className), + deleteRules: fixups.deleteRules.filter((f) => f.className === className), }; files.set(`${className}.entity.ts`, applyTextPasses(raw, relevantFixups)); } diff --git a/server/src/db/entities/AssignmentParticipants.entity.ts b/server/src/db/entities/AssignmentParticipants.entity.ts index f9216bcf0..d1b4dd36a 100644 --- a/server/src/db/entities/AssignmentParticipants.entity.ts +++ b/server/src/db/entities/AssignmentParticipants.entity.ts @@ -18,9 +18,9 @@ export const AssignmentParticipantsSchema = defineEntity({ uniques: [{ properties: ['assignment', 'user'] }], properties: { id: p.integer().primary(), - assignment: () => p.manyToOne(DayAssignments).ref().hidden().index('idx_assignment_participants_assignment'), + assignment: () => p.manyToOne(DayAssignments).ref().deleteRule('cascade').hidden().index('idx_assignment_participants_assignment'), assignment_id: p.integer().persist(false).index('idx_assignment_participants_assignment'), - user: () => p.manyToOne(Users).ref().hidden(), + user: () => p.manyToOne(Users).ref().deleteRule('cascade').hidden(), user_id: p.integer().persist(false), }, }); diff --git a/server/src/db/entities/BudgetItems.entity.ts b/server/src/db/entities/BudgetItems.entity.ts index 4132ea1e1..ad9b2bcc6 100644 --- a/server/src/db/entities/BudgetItems.entity.ts +++ b/server/src/db/entities/BudgetItems.entity.ts @@ -52,7 +52,7 @@ export const BudgetItemsSchema = defineEntity({ note: p.text().nullable(), sort_order: p.integer().nullable(), created_at: p.type(DbTimestampType).nullable().defaultRaw(`CURRENT_TIMESTAMP`), - paidByUser: () => p.manyToOne(Users).ref().nullable().hidden(), + paidByUser: () => p.manyToOne(Users).ref().deleteRule('no action').nullable().hidden(), paid_by_user_id: p.integer().nullable().persist(false), expense_date: p.text().nullable().defaultRaw(`NULL`), reservation: () => p.manyToOne(Reservations).ref().nullable().hidden().defaultRaw(`NULL`), diff --git a/server/src/db/entities/BudgetSettlements.entity.ts b/server/src/db/entities/BudgetSettlements.entity.ts index da3513563..418a164ae 100644 --- a/server/src/db/entities/BudgetSettlements.entity.ts +++ b/server/src/db/entities/BudgetSettlements.entity.ts @@ -35,7 +35,7 @@ export const BudgetSettlementsSchema = defineEntity({ to_user_id: p.integer().persist(false), amount: p.double().defaultRaw(`0`), created_at: p.type(DbTimestampType).nullable().defaultRaw(`CURRENT_TIMESTAMP`), - createdByUser: () => p.manyToOne(Users).ref().nullable().hidden(), + createdByUser: () => p.manyToOne(Users).ref().deleteRule('no action').nullable().hidden(), created_by_user_id: p.integer().nullable().persist(false), currency: p.text().nullable(), exchange_rate: p.double().defaultRaw(`1`), diff --git a/server/src/db/entities/DawarichConnections.entity.ts b/server/src/db/entities/DawarichConnections.entity.ts index b07c380af..cad8fff72 100644 --- a/server/src/db/entities/DawarichConnections.entity.ts +++ b/server/src/db/entities/DawarichConnections.entity.ts @@ -24,7 +24,7 @@ export const DawarichConnectionsSchema = defineEntity({ class: DawarichConnections, repository: () => DawarichConnectionsRepository, properties: { - user: () => p.oneToOne(Users).primary().ref().nullable().hidden(), + user: () => p.oneToOne(Users).primary().ref().deleteRule('cascade').nullable().hidden(), user_id: p.integer().nullable().persist(false), url: p.text().nullable(), api_key: p.text().nullable(), diff --git a/server/src/db/entities/JourneyContributors.entity.ts b/server/src/db/entities/JourneyContributors.entity.ts index c69bdf62d..f0a60a3cb 100644 --- a/server/src/db/entities/JourneyContributors.entity.ts +++ b/server/src/db/entities/JourneyContributors.entity.ts @@ -22,7 +22,7 @@ export const JourneyContributorsSchema = defineEntity({ properties: { journey: () => p.manyToOne(Journeys).primary().ref().hidden(), journey_id: p.integer().persist(false), - user: () => p.manyToOne(Users).primary().ref().hidden().index('idx_journey_contributors_user'), + user: () => p.manyToOne(Users).primary().ref().deleteRule('no action').hidden().index('idx_journey_contributors_user'), user_id: p.integer().persist(false).index('idx_journey_contributors_user'), role: p.text(), added_at: p.integer(), diff --git a/server/src/db/entities/OauthTokens.entity.ts b/server/src/db/entities/OauthTokens.entity.ts index 89a0bcac1..b7e912614 100644 --- a/server/src/db/entities/OauthTokens.entity.ts +++ b/server/src/db/entities/OauthTokens.entity.ts @@ -44,7 +44,7 @@ export const OauthTokensSchema = defineEntity({ refresh_token_expires_at: p.type(DbTimestampType), revoked_at: p.type(DbTimestampType).nullable(), created_at: p.type(DbTimestampType).nullable().defaultRaw(`CURRENT_TIMESTAMP`), - parentToken: () => p.manyToOne(OauthTokens).ref().nullable().hidden().index('idx_oauth_tokens_parent'), + parentToken: () => p.manyToOne(OauthTokens).ref().deleteRule('no action').nullable().hidden().index('idx_oauth_tokens_parent'), parent_token_id: p.integer().nullable().persist(false).index('idx_oauth_tokens_parent'), audience: p.text().nullable(), oauth_tokens_collection: () => p.oneToMany(OauthTokens).mappedBy('parentToken').hidden(), diff --git a/server/src/db/entities/PlaceRegions.entity.ts b/server/src/db/entities/PlaceRegions.entity.ts index 0a108d35c..11beadcf7 100644 --- a/server/src/db/entities/PlaceRegions.entity.ts +++ b/server/src/db/entities/PlaceRegions.entity.ts @@ -16,7 +16,7 @@ export const PlaceRegionsSchema = defineEntity({ class: PlaceRegions, repository: () => PlaceRegionsRepository, properties: { - place: () => p.oneToOne(Places).primary().ref().nullable().hidden(), + place: () => p.oneToOne(Places).primary().ref().deleteRule('cascade').nullable().hidden(), place_id: p.integer().nullable().persist(false), country_code: p.text().index('idx_place_regions_country'), region_code: p.text().index('idx_place_regions_region'), diff --git a/server/src/db/entities/ReservationTravelers.entity.ts b/server/src/db/entities/ReservationTravelers.entity.ts index 59a2eb591..0ac8313e1 100644 --- a/server/src/db/entities/ReservationTravelers.entity.ts +++ b/server/src/db/entities/ReservationTravelers.entity.ts @@ -18,9 +18,9 @@ export const ReservationTravelersSchema = defineEntity({ uniques: [{ properties: ['reservation', 'user'] }], properties: { id: p.integer().primary(), - reservation: () => p.manyToOne(Reservations).ref().hidden().index('idx_reservation_travelers_res'), + reservation: () => p.manyToOne(Reservations).ref().deleteRule('cascade').hidden().index('idx_reservation_travelers_res'), reservation_id: p.integer().persist(false).index('idx_reservation_travelers_res'), - user: () => p.manyToOne(Users).ref().hidden().index('idx_reservation_travelers_user'), + user: () => p.manyToOne(Users).ref().deleteRule('cascade').hidden().index('idx_reservation_travelers_user'), user_id: p.integer().persist(false).index('idx_reservation_travelers_user'), }, }); diff --git a/server/src/db/entities/RoadtripDayTracks.entity.ts b/server/src/db/entities/RoadtripDayTracks.entity.ts index 4f1b4fa88..7c0387499 100644 --- a/server/src/db/entities/RoadtripDayTracks.entity.ts +++ b/server/src/db/entities/RoadtripDayTracks.entity.ts @@ -18,7 +18,7 @@ export const RoadtripDayTracksSchema = defineEntity({ class: RoadtripDayTracks, repository: () => RoadtripDayTracksRepository, properties: { - day: () => p.oneToOne(Days).primary().ref().nullable().hidden(), + day: () => p.oneToOne(Days).primary().ref().deleteRule('cascade').nullable().hidden(), day_id: p.integer().nullable().persist(false), place: () => p.manyToOne(Places).ref().deleteRule('cascade').hidden().index('idx_roadtrip_day_tracks_place'), place_id: p.integer().persist(false).index('idx_roadtrip_day_tracks_place'), diff --git a/server/src/db/entities/TripMembers.entity.ts b/server/src/db/entities/TripMembers.entity.ts index 7a02c13a6..a08f68207 100644 --- a/server/src/db/entities/TripMembers.entity.ts +++ b/server/src/db/entities/TripMembers.entity.ts @@ -28,6 +28,6 @@ export const TripMembersSchema = defineEntity({ user_id: p.integer().persist(false).index('idx_trip_members_user_id'), invited_by: p.integer().nullable().persist(false), added_at: p.type(DbTimestampType).nullable().defaultRaw(`CURRENT_TIMESTAMP`), - invitedByRef: () => p.manyToOne(Users).ref().joinColumn('invited_by').nullable().hidden(), + invitedByRef: () => p.manyToOne(Users).ref().joinColumn('invited_by').deleteRule('no action').nullable().hidden(), }, }); diff --git a/server/src/db/entities/VacayUserSettings.entity.ts b/server/src/db/entities/VacayUserSettings.entity.ts index b229fe229..b5260e1a0 100644 --- a/server/src/db/entities/VacayUserSettings.entity.ts +++ b/server/src/db/entities/VacayUserSettings.entity.ts @@ -17,7 +17,7 @@ export const VacayUserSettingsSchema = defineEntity({ class: VacayUserSettings, repository: () => VacayUserSettingsRepository, properties: { - user: () => p.oneToOne(Users).primary().ref().nullable().hidden(), + user: () => p.oneToOne(Users).primary().ref().deleteRule('cascade').nullable().hidden(), user_id: p.integer().nullable().persist(false), year_type: p.text().default('calendar'), year_start_month: p.integer().default(1), diff --git a/server/tests/unit/db/entity-schema-parity.test.ts b/server/tests/unit/db/entity-schema-parity.test.ts index b9a44dcda..1a0170b85 100644 --- a/server/tests/unit/db/entity-schema-parity.test.ts +++ b/server/tests/unit/db/entity-schema-parity.test.ts @@ -317,8 +317,17 @@ describe('entity ↔ migrated-schema parity', () => { * `school_holiday_regions.country -> school_holiday_countries.code` * included (`code` IS that table's primary key, so the generator's default * assumption was already correct there — unchanged by Rule 11, confirmed - * by this same test). The deleteRule half stays skipped — see - * `PARITY-009b` immediately below for why and by how many rows. + * by this same test). The deleteRule half is un-skipped by Plan 4 Task 7 — + * see `PARITY-009b` immediately below. + * + * `school_holiday_regions.country` re-verified one more time at Task 7's + * own HEAD, directly against the live migration DDL (not the entity, not + * an earlier report): `Migration20200101033600_create_school_holiday_countries.ts` + * declares `country TEXT NOT NULL REFERENCES school_holiday_countries(code)` + * — a real, DB-enforced FK, confirmed here by this very test passing for + * that column too (`SchoolHolidayRegions.countryRef`'s `.joinColumn('country')`, + * Plan 2 Task 3). 3f's inventory was right; there is no schema gap here to + * document for the user. */ it('PARITY-009: FK referenced columns agree with PRAGMA foreign_key_list', () => { const failures: string[] = []; @@ -350,36 +359,43 @@ describe('entity ↔ migrated-schema parity', () => { }); /** - * The deleteRule half of the same check (Task 0, Step 3). Task 0 found - * deleteRule disagreeing on 11 of 180 owning relations; Task 3's rewrite - * fixed one of those eleven as a side effect (`school_holiday_regions. - * country` now has a real, explicit `joinColumn`, so it reports a genuine - * FK row) and re-ran this check against the other 124 rewritten/added - * entities, composite-PK relations included (`packing_item_contributors`, - * `vacay_user_settings`, `document_connections`, `trip_document_links` — - * their `.entity.ts` source never spells `.deleteRule(...)` on those - * relations, but MikroORM discovery still resolves the correct rule at - * runtime, confirmed by running this check, so they are not drift): the - * count was 10. Plan 3b interlude A's `RULE11_referencedColumns` only - * touches the referenced COLUMN, never `deleteRule` (`OauthTokens.client`/ - * `OauthConsents.client` already agreed on `deleteRule('cascade')` — - * unaffected either way), so re-running this exact query still finds - * **10** disagreements, unchanged: `trip_members.invited_by` entity=set - * null vs db=no action, `roadtrip_day_tracks.day_id` entity=set null vs - * db=cascade, `reservation_travelers.reservation_id`/`.user_id` and - * `assignment_participants.assignment_id`/`.user_id` entity=no action vs - * db=cascade, `oauth_tokens.parent_token_id` entity=set null vs db=no - * action, `journey_contributors.user_id` entity=cascade vs db=no action, - * `budget_settlements.created_by_user_id`/`budget_items.paid_by_user_id` - * entity=set null vs db=no action. None of these are in this task's scope - * (or Task 0's/Task 3's); an allow-list would need 10 one-line entries for - * drift no task so far was asked to fix. Index-name parity looked clean on - * inspection but was not exhaustively checked either, and stays out of - * scope here too. Left skipped, with the count pinned above (Plan 4 owns - * turning this into either fixes or a real allow-list — the number to - * reconcile against is exactly 10, not "some"). + * The deleteRule half of the same check (Task 0, Step 3), un-skipped by + * Plan 4 Task 7. Task 0 found deleteRule disagreeing on 11 of 180 owning + * relations; Task 3's rewrite fixed one of those eleven as a side effect + * (`school_holiday_regions.country`) and re-measured the other 124 + * rewritten/added entities at 10. Task 7 re-measured again at its own HEAD + * (program rule: re-measure, never assume a prior count) and found the + * same 10, unchanged: `trip_members.invited_by`, `oauth_tokens. + * parent_token_id`, `budget_settlements.created_by_user_id`, `budget_items. + * paid_by_user_id` (a nullable relation whose column has no `ON DELETE` at + * all in its migration — SQLite ⇒ `no action` — while the entity's bare + * `.nullable()` defaults to MikroORM's own `'set null'`), `roadtrip_day_ + * tracks.day_id` (nullable AND the table's sole PK — migration says + * `ON DELETE CASCADE`, entity defaulted to `'set null'` the same way), + * `journey_contributors.user_id` (a composite-PK member whose migration + * has no `ON DELETE`, while the entity's FK-as-PK shape defaults to + * `'cascade'`), and `reservation_travelers.reservation_id`/`.user_id` + + * `assignment_participants.assignment_id`/`.user_id` (migrations say + * `ON DELETE CASCADE`; the entity had no default at all for that shape, + * landing on `'no action'`). + * + * All 10 are the ENTITY side never having matched its migration's physical + * FK to begin with (never a migration defect — program rule: migrations + * are append-only, and every one of the 10 migrations above is verified + * correct against its own table's real production behavior). Fixed via a + * new generator rule, `RULE13_pinDeleteRuleDrift` (`scripts/generate- + * entities.ts`), which pins an explicit `.deleteRule(...)` wherever + * leaving the relation implicit would produce the wrong effective rule — + * see that rule's own doc comment for the full mechanism, including one + * more wrinkle it found empirically: `DawarichConnections.user`, + * `PlaceRegions.place` and `VacayUserSettings.user` share the exact same + * "nullable AND sole FK-as-PK" shape as `roadtrip_day_tracks.day_id` but + * were NOT among the 10 — their runtime default already happens to land on + * `'cascade'` (matching physical) purely because of `ALL_ENTITIES` + * discovery order, not because the shape is safe. `RULE13` pins all four + * explicitly rather than trust that order to hold. */ - it.skip('PARITY-009b (report-only, deferred to Plan 4): FK deleteRule agrees — 10 known disagreements, see doc comment', () => { + it('PARITY-009b: FK deleteRule agrees with PRAGMA foreign_key_list', () => { const failures: string[] = []; for (const meta of tableBackedMetas()) { const table = meta.tableName; @@ -409,6 +425,56 @@ describe('entity ↔ migrated-schema parity', () => { expect(failures).toEqual([]); }); + /** + * The index-name half of the same PRAGMA-allow-listed check (Task 0, Step + * 3) — per 3a's own ledger note, "PARITY-009's index half was never + * implemented (only deleteRule)." Task 7 writes it fresh. + * + * `PRAGMA index_list()` names every index SQLite actually built — + * a plain `CREATE INDEX ON ...` or `CREATE UNIQUE INDEX + * ON ...`, one row each — MINUS the unnamed `sqlite_autoindex_
_` + * entries a bare `PRIMARY KEY`/inline `UNIQUE(...)` constraint gets for + * free (excluded the same way `collectImplicitUniqueIndexes` in + * `scripts/generate-entities.ts` excludes them — those are RULE9's + * `uniques:` territory, a DIFFERENT, already-covered check, not a named + * index this one can compare against). The entity side collects every + * NAMED index a `defineEntity` call can declare: a property-level + * `.index('name')`/`.unique('name')` (`prop.index`/`prop.unique`, a string + * when named — the codebase never uses the unnamed `boolean` form, see the + * `grep` a future author can re-run: `grep -rn '\.index()\|\.unique()' src/db/entities/*.entity.ts` + * finds nothing), and every entity-level `indexes:`/`uniques:` block's own + * `.name` (composite indexes, e.g. `Notifications.idx_notifications_target_scope`). + * The two sets must be equal — a name in one and not the other means + * either a migration added/renamed/dropped an index without the entity + * following, or the reverse. Measured clean on today's schema (the + * generator's own introspection already names every entity index after + * its real DB index, RULE9's unnamed `uniques:` entries excluded by + * construction from both sides) — this is a drift GUARD for a future + * migration, not a fix for a found gap. + */ + it('PARITY-009c: index names agree with PRAGMA index_list', () => { + const failures: string[] = []; + for (const meta of tableBackedMetas()) { + const table = meta.tableName; + const dbIndexRows = testDb.prepare(`PRAGMA index_list("${table}")`).all() as { name: string; unique: number }[]; + const dbNames = new Set(dbIndexRows.filter((row) => !row.name.startsWith('sqlite_autoindex_')).map((row) => row.name)); + const entityNames = new Set(); + for (const prop of meta.props) { + if (typeof prop.index === 'string') entityNames.add(prop.index); + if (typeof prop.unique === 'string') entityNames.add(prop.unique); + } + for (const idx of meta.indexes) if (idx.name) entityNames.add(idx.name); + for (const uniq of meta.uniques) if (uniq.name) entityNames.add(uniq.name); + for (const name of dbNames) { + if (!entityNames.has(name)) failures.push(`${table}: db index "${name}" has no matching entity index declaration`); + } + for (const name of entityNames) { + if (!dbNames.has(name)) failures.push(`${table}: entity declares index "${name}" with no matching db index`); + } + } + expect(failures).toEqual([]); + }); + /** * D1's own rule (the naming strategy derives the same column from a * camelCase or a snake_case property, so a column-level check can never diff --git a/server/tests/unit/db/generate-entities.test.ts b/server/tests/unit/db/generate-entities.test.ts index e02a203fe..5755fea2e 100644 --- a/server/tests/unit/db/generate-entities.test.ts +++ b/server/tests/unit/db/generate-entities.test.ts @@ -11,6 +11,8 @@ import { assertFilesGenerated, BOOLEAN_COLUMNS, CheckExpressionFixup, + collectForeignKeyDeleteRules, + DeleteRuleFixup, JoinColumnFixup, JsonColumnFixup, KNOWN_DIFFS, @@ -31,6 +33,7 @@ import { RULE10_repositoryTypeMarker, RULE11_referencedColumns, RULE12_fixGarbledCheckExpressions, + RULE13_pinDeleteRuleDrift, RULE_normalizeLiteralDefaults, applyTextPasses, checkEntities, @@ -548,6 +551,137 @@ describe('RULE12_fixGarbledCheckExpressions', () => { }); }); +describe('RULE13_pinDeleteRuleDrift', () => { + it('RULE13-001: a plain relation (not nullable, not part of an FK-as-PK) whose physical rule is "cascade" gets pinned — no implicit default covers this shape (the real reservation_travelers/assignment_participants case)', () => { + const id = fixtureProp({ name: 'id', primary: true, autoincrement: true }); + const rel = fixtureProp({ name: 'reservation', kind: ReferenceKind.MANY_TO_ONE, fieldNames: ['reservation_id'] }); + const meta = fixtureMeta('ReservationTravelers', 'reservation_travelers', [id, rel]); + const fixups = RULE13_pinDeleteRuleDrift([meta], new Map([['reservation_travelers', new Map([['reservation_id', 'cascade']])]])); + expect(fixups).toEqual([ + { className: 'ReservationTravelers', propName: 'reservation', from: undefined, to: 'cascade' }, + ]); + expect(rel.deleteRule).toBe('cascade'); + }); + + it('RULE13-002: a nullable relation whose physical rule is "no action" (no ON DELETE at all in its migration) gets pinned — the implicit "set null" default would be wrong (the real trip_members.invited_by case)', () => { + const rel = fixtureProp({ name: 'invitedByRef', kind: ReferenceKind.MANY_TO_ONE, fieldNames: ['invited_by'], nullable: true }); + const meta = fixtureMeta('TripMembers', 'trip_members', [rel]); + const fixups = RULE13_pinDeleteRuleDrift([meta], new Map([['trip_members', new Map([['invited_by', 'no action']])]])); + expect(fixups).toEqual([{ className: 'TripMembers', propName: 'invitedByRef', from: undefined, to: 'no action' }]); + expect(rel.deleteRule).toBe('no action'); + }); + + it('RULE13-003: a plain relation whose physical rule already agrees with the "no action" fallback is left untouched (the common case — no noise)', () => { + const rel = fixtureProp({ name: 'trip', kind: ReferenceKind.MANY_TO_ONE, fieldNames: ['trip_id'] }); + const meta = fixtureMeta('X', 'x', [rel]); + const fixups = RULE13_pinDeleteRuleDrift([meta], new Map([['x', new Map([['trip_id', 'no action']])]])); + expect(fixups).toEqual([]); + expect(rel.deleteRule).toBeUndefined(); + }); + + it('RULE13-004: a composite-PK member (every PK on the entity is itself a relation) whose physical rule is "no action" gets pinned — the implicit "cascade" default for FK-as-PK would be wrong (the real journey_contributors.user_id case)', () => { + const journey = fixtureProp({ name: 'journey', kind: ReferenceKind.MANY_TO_ONE, fieldNames: ['journey_id'], primary: true }); + const user = fixtureProp({ name: 'user', kind: ReferenceKind.MANY_TO_ONE, fieldNames: ['user_id'], primary: true }); + const meta = fixtureMeta('JourneyContributors', 'journey_contributors', [journey, user]); + const fixups = RULE13_pinDeleteRuleDrift( + [meta], + new Map([ + [ + 'journey_contributors', + new Map([ + ['journey_id', 'cascade'], + ['user_id', 'no action'], + ]), + ], + ]), + ); + expect(fixups).toEqual([{ className: 'JourneyContributors', propName: 'user', from: undefined, to: 'no action' }]); + expect(journey.deleteRule).toBeUndefined(); // already correct via the implicit cascade default — left alone + expect(user.deleteRule).toBe('no action'); + }); + + it('RULE13-005: a relation that is BOTH nullable AND its entity\'s sole FK-as-PK is always pinned explicitly, even when the implicit default happens to already agree with physical (the DawarichConnections/PlaceRegions/VacayUserSettings wrinkle — order-dependent at runtime, never trusted)', () => { + const rel = fixtureProp({ name: 'user', kind: ReferenceKind.ONE_TO_ONE, owner: true, fieldNames: ['user_id'], primary: true, nullable: true }); + const meta = fixtureMeta('DawarichConnections', 'dawarich_connections', [rel]); + const fixups = RULE13_pinDeleteRuleDrift([meta], new Map([['dawarich_connections', new Map([['user_id', 'cascade']])]])); + expect(fixups).toEqual([{ className: 'DawarichConnections', propName: 'user', from: undefined, to: 'cascade' }]); + expect(rel.deleteRule).toBe('cascade'); + }); + + it('RULE13-006: the same ambiguous nullable-FK-as-PK shape is left alone once it already carries the correct explicit deleteRule (idempotent — a second generator run produces no fixup)', () => { + const rel = fixtureProp({ + name: 'day', + kind: ReferenceKind.ONE_TO_ONE, + owner: true, + fieldNames: ['day_id'], + primary: true, + nullable: true, + deleteRule: 'cascade', + }); + const meta = fixtureMeta('RoadtripDayTracks', 'roadtrip_day_tracks', [rel]); + const fixups = RULE13_pinDeleteRuleDrift([meta], new Map([['roadtrip_day_tracks', new Map([['day_id', 'cascade']])]])); + expect(fixups).toEqual([]); + expect(rel.deleteRule).toBe('cascade'); + }); + + it('RULE13-007: a table absent from the fkDeleteRules map (no FK columns queried, or a routine/enum entity) is left untouched, never thrown on', () => { + const rel = fixtureProp({ name: 'trip', kind: ReferenceKind.MANY_TO_ONE, fieldNames: ['trip_id'] }); + const meta = fixtureMeta('X', 'x', [rel]); + expect(() => RULE13_pinDeleteRuleDrift([meta], new Map())).not.toThrow(); + expect(RULE13_pinDeleteRuleDrift([meta], new Map())).toEqual([]); + }); + + it('RULE13-008: a column the map has no entry for (no db FK on it at all) is skipped — PARITY-009 proper\'s job, not this rule\'s', () => { + const rel = fixtureProp({ name: 'trip', kind: ReferenceKind.MANY_TO_ONE, fieldNames: ['trip_id'] }); + const meta = fixtureMeta('X', 'x', [rel]); + const fixups = RULE13_pinDeleteRuleDrift([meta], new Map([['x', new Map([['other_id', 'cascade']])]])); + expect(fixups).toEqual([]); + expect(rel.deleteRule).toBeUndefined(); + }); + + it('RULE13-009: an inverse relation (mappedBy set, not owning) is never a candidate, even when its column name collides with a physical FK entry', () => { + const inverse = fixtureProp({ name: 'trips_collection', kind: ReferenceKind.ONE_TO_MANY, mappedBy: 'x', fieldNames: ['trip_id'] }); + const meta = fixtureMeta('X', 'x', [inverse]); + const fixups = RULE13_pinDeleteRuleDrift([meta], new Map([['x', new Map([['trip_id', 'cascade']])]])); + expect(fixups).toEqual([]); + }); + + it('RULE13-010: a multi-column owning relation (fieldNames.length !== 1) is never a candidate', () => { + const rel = fixtureProp({ name: 'composite', kind: ReferenceKind.MANY_TO_ONE, fieldNames: ['a', 'b'] }); + const meta = fixtureMeta('X', 'x', [rel]); + const fixups = RULE13_pinDeleteRuleDrift([meta], new Map([['x', new Map([['a', 'cascade']])]])); + expect(fixups).toEqual([]); + }); +}); + +describe('collectForeignKeyDeleteRules', () => { + it('COLLECTFKDR-001: reads PRAGMA foreign_key_list(table).on_delete per column, lowercased, against the real migrated schema', async () => { + const db = createSnapshotTestDb(); + const t = await createTestOrm(db); + try { + const result = await collectForeignKeyDeleteRules(t.orm.em.getConnection(), ['trip_members', 'reservation_travelers']); + expect(result.get('trip_members')?.get('trip_id')).toBe('cascade'); + expect(result.get('trip_members')?.get('invited_by')).toBe('no action'); + expect(result.get('reservation_travelers')?.get('reservation_id')).toBe('cascade'); + } finally { + await t.close(); + db.close(); + } + }); + + it('COLLECTFKDR-002: a table with no foreign keys at all is absent from the result map, not present with an empty one', async () => { + const db = createSnapshotTestDb(); + const t = await createTestOrm(db); + try { + const result = await collectForeignKeyDeleteRules(t.orm.em.getConnection(), ['users']); + expect(result.has('users')).toBe(false); + } finally { + await t.close(); + db.close(); + } + }); +}); + describe('RULE9_addImplicitUniqueConstraints', () => { it('RULE9-001: a table with a matching implicit-unique entry gets a uniques: block referencing the columns as properties — a plain (non-FK) column keeps its own name', () => { const userId = fixtureProp({ name: 'user_id', primary: false }); @@ -979,6 +1113,7 @@ describe('applyTextPasses', () => { repositoryMarkers: [{ className: 'X', repositoryClassName: 'XRepository' }], referencedColumns: [{ className: 'X', propName: 'countryRef', referencedColumnNames: ['code'] }], checkExpressions: [], + deleteRules: [], }); expect(out).toContain(".joinColumn('country')"); expect(out).toContain(".referencedColumnNames('code')");