diff --git a/.changeset/single-create-schema-change.md b/.changeset/single-create-schema-change.md new file mode 100644 index 000000000..13cccd093 --- /dev/null +++ b/.changeset/single-create-schema-change.md @@ -0,0 +1,33 @@ +--- +"nextly": patch +"create-nextly-app": patch +"@nextlyhq/admin": patch +"@nextlyhq/admin-css": patch +"@nextlyhq/blocks-engine": patch +"@nextlyhq/blocks-react": patch +"@nextlyhq/ui": patch +"@nextlyhq/adapter-drizzle": patch +"@nextlyhq/adapter-postgres": patch +"@nextlyhq/adapter-mysql": patch +"@nextlyhq/adapter-sqlite": patch +"@nextlyhq/storage-s3": patch +"@nextlyhq/storage-uploadthing": patch +"@nextlyhq/storage-vercel-blob": patch +"@nextlyhq/plugin-form-builder": patch +"@nextlyhq/plugin-page-builder": patch +"@nextlyhq/plugin-seo": patch +"@nextlyhq/plugin-sdk": patch +"@nextlyhq/eslint-config": patch +"@nextlyhq/prettier-config": patch +"@nextlyhq/telemetry": patch +"@nextlyhq/tsconfig": patch +--- + +Creating a Single from the Schema Builder now writes its table and its registry row as one +operation, so an interrupted create leaves a record of what it was doing instead of a table nothing +knows about. + +On MySQL, retrying any Schema Builder migration that stopped part way now succeeds instead of +reporting the already-correct schema as a failed migration. MySQL cannot express "create this index +only if it is missing", so the retry previously failed the same way every time and left no way +forward. diff --git a/packages/nextly/src/di/register.ts b/packages/nextly/src/di/register.ts index e9b19ff26..e01f087bc 100644 --- a/packages/nextly/src/di/register.ts +++ b/packages/nextly/src/di/register.ts @@ -62,6 +62,7 @@ import { builtByFor } from "../domains/schema/pipeline/registered-collections"; import type { DesiredCollection } from "../domains/schema/pipeline/types"; import type { ColumnOrigin } from "../domains/schema/services/field-column-descriptor"; import type { SingleEntryService } from "../domains/singles/services/single-entry-service"; +import type { SingleMetadataService } from "../domains/singles/services/single-metadata-service"; import type { SingleRegistryService, CodeFirstSingleConfig, @@ -327,6 +328,8 @@ export interface ServiceMap { mediaService: UnifiedMediaService; singleRegistryService: SingleRegistryService; singleEntryService: SingleEntryService; + /** Owns a Single's table change together with the registry write that records it. */ + singleMetadataService: SingleMetadataService; fieldGroupRegistryService: FieldGroupRegistryService; fieldGroupSchemaService: FieldGroupSchemaService; fieldGroupDataService: FieldGroupDataService; diff --git a/packages/nextly/src/di/registrations/register-singles.ts b/packages/nextly/src/di/registrations/register-singles.ts index c9af714de..5c246553a 100644 --- a/packages/nextly/src/di/registrations/register-singles.ts +++ b/packages/nextly/src/di/registrations/register-singles.ts @@ -15,6 +15,7 @@ import type { RBACAccessControlService } from "../../domains/auth/services/rbac- import { MetaRetentionGate } from "../../domains/retention/gate"; import { buildRetentionRunner } from "../../domains/retention/passes"; import { SingleEntryService } from "../../domains/singles/services/single-entry-service"; +import { SingleMetadataService } from "../../domains/singles/services/single-metadata-service"; import { SingleRegistryService } from "../../domains/singles/services/single-registry-service"; import type { WebhookFastDrainScheduler } from "../../domains/webhooks/after-drain"; import type { CacheRevalidator } from "../../revalidation/types"; @@ -46,6 +47,20 @@ export function registerSingleServices(ctx: RegistrationContext): void { } ); + // Schema changes for a Single, holding the table change and the registry write together. It is + // registered rather than built per request so a single wrapper here governs every caller: the + // migration lock has to enclose both halves, and a lock applied at one call site leaves the + // others uncovered. + container.registerSingleton( + "singleMetadataService", + () => + new SingleMetadataService( + container.get("singleRegistryService"), + logger, + adapter + ) + ); + container.registerSingleton("singleEntryService", () => { const singleRegistryService = container.get( "singleRegistryService" diff --git a/packages/nextly/src/dispatcher/handlers/__tests__/single-dispatcher-ddl.test.ts b/packages/nextly/src/dispatcher/handlers/__tests__/single-dispatcher-ddl.test.ts index b405c7330..b3929ec5d 100644 --- a/packages/nextly/src/dispatcher/handlers/__tests__/single-dispatcher-ddl.test.ts +++ b/packages/nextly/src/dispatcher/handlers/__tests__/single-dispatcher-ddl.test.ts @@ -19,12 +19,17 @@ import { beforeEach, describe, expect, it, vi } from "vitest"; vi.mock("../../helpers/di", () => ({ getSingleRegistryFromDI: vi.fn(), getSingleEntryServiceFromDI: vi.fn(), + getSingleMetadataServiceFromDI: vi.fn(), getComponentRegistryFromDI: vi.fn().mockReturnValue(undefined), getAdapterFromDI: vi.fn(), // Reached by the companion reconciliation a localized create runs. Omitted, calling it throws and // the handler catches that as a companion-provisioning failure — so the assertions below would // describe a FAILED migration while passing. getConfigFromDI: vi.fn(() => undefined), + // Reached by the companion's runtime registration. Answering `undefined` is what a server with no + // schema registry does; omitting it throws inside the registration instead, which is a different + // path and not one a create ever takes. + getSchemaRegistryFromDI: vi.fn(() => undefined), })); const executed: string[] = []; @@ -64,12 +69,24 @@ vi.mock("../../../di/container", () => ({ }, })); +import { SingleMetadataService } from "../../../domains/singles/services/single-metadata-service"; +import type { SingleRegistryService } from "../../../domains/singles/services/single-registry-service"; +import type { Logger } from "../../../shared/types"; import { getSingleEntryServiceFromDI, + getSingleMetadataServiceFromDI, getSingleRegistryFromDI, } from "../../helpers/di"; import { dispatchSingles } from "../single-dispatcher"; +/** Silent: these tests read the statements, not the log. */ +const logger: Logger = { + debug: vi.fn(), + info: vi.fn(), + warn: vi.fn(), + error: vi.fn(), +}; + function wireRegistry() { const registry = { listSingles: vi.fn(), @@ -77,6 +94,9 @@ function wireRegistry() { getAllSingles: vi.fn().mockResolvedValue([]), getSingleBySlug: vi.fn(), registerSingle: vi.fn(async (row: unknown) => row), + // The confirm write. A create persists its intent as `pending` before touching the table and + // records the outcome here afterwards, so a double without it fails the whole create. + updateMigrationStatus: vi.fn(), updateSingle: vi.fn(), deleteSingle: vi.fn(), }; @@ -87,24 +107,54 @@ function wireRegistry() { get: vi.fn(), update: vi.fn(), } as unknown as ReturnType); + // The REAL service over the same doubles, because the create path's behaviour IS this service's + // behaviour: what the request forwards into the DDL is decided inside it, and a stub standing in + // for it would leave these assertions describing the stub. + vi.mocked(getSingleMetadataServiceFromDI).mockReturnValue( + new SingleMetadataService( + registry as unknown as SingleRegistryService, + logger, + adapter as unknown as ConstructorParameters< + typeof SingleMetadataService + >[2] + ) + ); return registry; } -/** Everything the adapter was asked to run, as one string. */ +/** + * Everything the adapter was asked to run, as one string. + * + * 🔴 The create's OWN recorded outcome is checked before any of it is returned, and that check is + * here rather than in each test because it is a precondition of the entire file: statements that + * ran on the way to a failure prove nothing about what a working create emits. Breaking a rule + * shows a test CAN fail; it does not show the code reached the state the test describes. A create + * that gave up part-way still leaves its earlier statements in `executed`, so every assertion below + * would keep passing while describing a run that never finished. + * + * `migration_status` is the product's own success signal — the value the admin reads back — so + * nothing is computed here that does not already exist, and a regression names itself: + * `expected 'failed' to be 'applied'`. + */ async function ddlFor( payload: Record, dialect: "postgresql" | "mysql" | "sqlite" = "postgresql" ): Promise { executed.length = 0; adapter = makeAdapter(dialect); - wireRegistry(); + const registry = wireRegistry(); await dispatchSingles("createSingle", {}, payload); + + const recorded = registry.updateMigrationStatus.mock.calls[0]?.[1]; + expect(recorded, "the create recorded its own outcome").toBe("applied"); + return executed.join("\n"); } beforeEach(() => { vi.mocked(getSingleRegistryFromDI).mockReset(); vi.mocked(getSingleEntryServiceFromDI).mockReset(); + vi.mocked(getSingleMetadataServiceFromDI).mockReset(); }); describe("createSingle — what the request forwards into the DDL", () => { diff --git a/packages/nextly/src/dispatcher/handlers/__tests__/single-dispatcher-shapes.test.ts b/packages/nextly/src/dispatcher/handlers/__tests__/single-dispatcher-shapes.test.ts index 60a46f14e..79278b60d 100644 --- a/packages/nextly/src/dispatcher/handlers/__tests__/single-dispatcher-shapes.test.ts +++ b/packages/nextly/src/dispatcher/handlers/__tests__/single-dispatcher-shapes.test.ts @@ -19,6 +19,7 @@ import { beforeEach, describe, expect, it, vi } from "vitest"; vi.mock("../../helpers/di", () => ({ getSingleRegistryFromDI: vi.fn(), getSingleEntryServiceFromDI: vi.fn(), + getSingleMetadataServiceFromDI: vi.fn(), getComponentRegistryFromDI: vi.fn().mockReturnValue(undefined), getAdapterFromDI: vi.fn(), })); @@ -53,15 +54,28 @@ vi.mock( } ); +import { SingleMetadataService } from "../../../domains/singles/services/single-metadata-service"; +import type { SingleRegistryService } from "../../../domains/singles/services/single-registry-service"; +import type { Logger } from "../../../shared/types"; import { getSingleEntryServiceFromDI, + getSingleMetadataServiceFromDI, getSingleRegistryFromDI, } from "../../helpers/di"; import { dispatchSingles } from "../single-dispatcher"; +/** Silent: these tests read Response shapes, not the log. */ +const logger: Logger = { + debug: vi.fn(), + info: vi.fn(), + warn: vi.fn(), + error: vi.fn(), +}; + type Registry = { listSingles: ReturnType; registerSingle: ReturnType; + updateMigrationStatus: ReturnType; getSingleBySlug: ReturnType; getAllSingles: ReturnType; updateSingle: ReturnType; @@ -76,6 +90,9 @@ function makeRegistry(overrides: Partial = {}): Registry { return { listSingles: vi.fn(), registerSingle: vi.fn(), + // A create persists its intent as `pending` and records the outcome here afterwards, so a + // double without it fails the request rather than the assertion under test. + updateMigrationStatus: vi.fn(), getSingleBySlug: vi.fn(), // Answers "no single owns that table" so a create reaches the path under test. The handler // asks before it emits any DDL, so a double that cannot answer fails the request instead. @@ -101,11 +118,21 @@ function wireDi(registry: Registry, entry: Entry) { vi.mocked(getSingleEntryServiceFromDI).mockReturnValue( entry as unknown as ReturnType ); + // The real service over the same double, with no adapter — which is the state this file puts the + // container in, and the state that leaves a create recorded as `pending`. A stub here would make + // the pending message an assumption of the test rather than a consequence of the code. + vi.mocked(getSingleMetadataServiceFromDI).mockReturnValue( + new SingleMetadataService( + registry as unknown as SingleRegistryService, + logger + ) + ); } beforeEach(() => { vi.mocked(getSingleRegistryFromDI).mockReset(); vi.mocked(getSingleEntryServiceFromDI).mockReset(); + vi.mocked(getSingleMetadataServiceFromDI).mockReset(); }); describe("dispatchSingles, paginated lists (respondList)", () => { diff --git a/packages/nextly/src/dispatcher/handlers/collection-dispatcher.ts b/packages/nextly/src/dispatcher/handlers/collection-dispatcher.ts index 0950e6fbc..4afcded04 100644 --- a/packages/nextly/src/dispatcher/handlers/collection-dispatcher.ts +++ b/packages/nextly/src/dispatcher/handlers/collection-dispatcher.ts @@ -851,6 +851,9 @@ const COLLECTIONS_METHODS: Record< } } } + // 🔴 STRICT on purpose, matching the other two companion paths: tolerating a re-run + // would make a half-finished localization ENABLE look like success, because the planner + // cannot tell that state from an orphan repair. A loud failure is the safer outcome. for (const stmt of plan.statements) { await adapter.executeQuery(stmt); } diff --git a/packages/nextly/src/dispatcher/handlers/component-dispatcher.ts b/packages/nextly/src/dispatcher/handlers/component-dispatcher.ts index e6d2db8cd..39481d8b3 100644 --- a/packages/nextly/src/dispatcher/handlers/component-dispatcher.ts +++ b/packages/nextly/src/dispatcher/handlers/component-dispatcher.ts @@ -36,6 +36,7 @@ import { buildCompanionRuntimeTable } from "../../domains/i18n/runtime/companion import { translatePipelinePreviewToLegacy } from "../../domains/schema/legacy-preview/translate"; import { RealClassifier } from "../../domains/schema/pipeline/classifier/classifier"; import { extractDatabaseNameFromUrl } from "../../domains/schema/pipeline/database-url"; +import { buildDesiredTableFromComponentFields } from "../../domains/schema/pipeline/diff/build-from-fields"; import { RealPreCleanupExecutor } from "../../domains/schema/pipeline/pre-cleanup/executor"; import { previewDesiredSchema } from "../../domains/schema/pipeline/preview"; import { @@ -51,9 +52,11 @@ import { RegexRenameDetector } from "../../domains/schema/pipeline/rename-detect import type { Resolution } from "../../domains/schema/pipeline/resolution/types"; import { isIdempotencyError } from "../../domains/schema/pipeline/sql-statement-utils"; import type { DesiredFieldGroup } from "../../domains/schema/pipeline/types"; +import { applyMigrationStatements } from "../../domains/schema/services/apply-migration-statements"; import { DrizzleStatementExecutor } from "../../domains/schema/services/drizzle-statement-executor"; import type { FieldResolution } from "../../domains/schema/services/schema-change-types"; import { calculateSchemaHash } from "../../domains/schema/services/schema-hash"; +import { shapeMismatches } from "../../domains/schema/services/verify-applied-shape"; import { resolveComponentTableName } from "../../domains/schema/utils/resolve-table-name"; import { NextlyError } from "../../errors"; import { getProductionNotifier } from "../../runtime/notifications/index"; @@ -112,31 +115,6 @@ function offsetPaginationToMeta(args: { }; } -// ============================================================ -// Migration SQL execution helper -// ============================================================ - -async function executeMigrationStatements( - adapter: DrizzleAdapter, - migrationSQL: string -): Promise { - const statements = migrationSQL - .split("--> statement-breakpoint") - .map(s => s.trim()) - .filter(s => s.length > 0); - - for (const statement of statements) { - const cleanStatement = statement - .split("\n") - .filter(line => !line.trim().startsWith("--")) - .join("\n") - .trim(); - if (cleanStatement) { - await adapter.executeQuery(cleanStatement); - } - } -} - // Refresh the cached Drizzle table so the next entry query joining this // component uses the new column layout without a server restart. /** @@ -305,6 +283,18 @@ async function reconcileComponentCompanion(args: { } } } + // 🔴 STRICT on purpose: the companion does NOT tolerate a re-run. + // + // Tolerating it would make a half-finished localization ENABLE look like success. Interrupted + // after the companion is created but before the seed and the main-column drops, the retry is + // indistinguishable from an orphan repair — the planner cannot tell them apart, because the + // signal that separates them (`existingMainColumns`) is consumed only on the disable path — so + // the duplicate columns would be swallowed and the entity recorded as localized while its + // default-locale content is still stranded on the main table. + // + // A loud failure is the worse experience and the safer outcome. It costs the repair of a + // localized entity whose create half-failed; it prevents silently reporting a migration that did + // not happen. for (const stmt of plan.statements) { await adapter.executeQuery(stmt); } @@ -428,10 +418,37 @@ const COMPONENTS_METHODS: Record> = { if (container.has("adapter")) { const diAdapter = container.get("adapter"); - await executeMigrationStatements(diAdapter, migrationSQL); + await applyMigrationStatements(diAdapter, migrationSQL); const tableExists = await diAdapter.tableExists(tableName); - if (tableExists) { + // 🔴 Existence is not enough now that a re-run is tolerated. `CREATE TABLE IF NOT + // EXISTS` no-ops against a field group's table left by an earlier attempt whose + // registry row is gone, and the index statements after it are tolerated, so a repair + // with a changed field set emits no error at all. Compared through the same builder + // the schema diff uses, so this cannot disagree with the pipeline about a column's + // name or its type. + const mismatches = tableExists + ? await shapeMismatches( + diAdapter, + dialect, + tableName, + buildDesiredTableFromComponentFields( + tableName, + b.fields as unknown as FieldDefinition[], + dialect, + // A field group's table is made by the field-group creator, which reads a text + // field's width from a different key than the collection creator does — so the + // desired column shape depends on saying which builder made it. + { localized: isLocalized, builtBy: "fieldGroup" } + ) + ) + : []; + if (mismatches.length > 0) { + migrationStatus = "failed"; + console.error( + `[Components] Table "${tableName}" does not match this schema: ${mismatches.join("; ")}` + ); + } else if (tableExists) { migrationStatus = "applied"; registerComponentRuntimeSchema( diAdapter, diff --git a/packages/nextly/src/dispatcher/handlers/single-dispatcher.ts b/packages/nextly/src/dispatcher/handlers/single-dispatcher.ts index c72afe811..d9cc789b0 100644 --- a/packages/nextly/src/dispatcher/handlers/single-dispatcher.ts +++ b/packages/nextly/src/dispatcher/handlers/single-dispatcher.ts @@ -39,16 +39,12 @@ import { container } from "../../di/container"; import { DynamicCollectionSchemaService } from "../../domains/dynamic-collections/services/dynamic-collection-schema-service"; import { teardownEntityComponentData } from "../../domains/field-groups/services/teardown-entity-field-group-data"; import { resolveLocalizedFieldNames } from "../../domains/i18n/classify-fields"; -import { buildCompanionTransitionStatements } from "../../domains/i18n/migration/reconcile-companion"; import { teardownEntityI18n } from "../../domains/i18n/migration/teardown-entity-i18n"; -import { - companionHasStatusColumn, - localizedColumnsOnMain, -} from "../../domains/i18n/runtime/companion-io"; import { buildCompanionRuntimeTable } from "../../domains/i18n/runtime/companion-registration"; import { translatePipelinePreviewToLegacy } from "../../domains/schema/legacy-preview/translate"; import { RealClassifier } from "../../domains/schema/pipeline/classifier/classifier"; import { extractDatabaseNameFromUrl } from "../../domains/schema/pipeline/database-url"; +import { buildDesiredTableFromFields } from "../../domains/schema/pipeline/diff/build-from-fields"; import { readForeignKeyColumns, readIndexNames, @@ -67,15 +63,18 @@ import { } from "../../domains/schema/pipeline/pushschema-pipeline-stubs"; import { RegexRenameDetector } from "../../domains/schema/pipeline/rename-detector"; import type { Resolution } from "../../domains/schema/pipeline/resolution/types"; -import { isIdempotencyError } from "../../domains/schema/pipeline/sql-statement-utils"; import type { DesiredSingle } from "../../domains/schema/pipeline/types"; +import { applyMigrationStatements } from "../../domains/schema/services/apply-migration-statements"; import { DrizzleStatementExecutor } from "../../domains/schema/services/drizzle-statement-executor"; import { columnsDeclaredBy } from "../../domains/schema/services/field-column-descriptor"; import { generateRuntimeSchema } from "../../domains/schema/services/runtime-schema-generator"; import type { FieldResolution } from "../../domains/schema/services/schema-change-types"; import { calculateSchemaHash } from "../../domains/schema/services/schema-hash"; +import { shapeMismatches } from "../../domains/schema/services/verify-applied-shape"; +import { reconcileSingleCompanion } from "../../domains/singles/services/reconcile-single-companion"; import { resolveSingleTableName } from "../../domains/singles/services/resolve-single-table-name"; import type { SingleEntryService } from "../../domains/singles/services/single-entry-service"; +import type { SingleMetadataService } from "../../domains/singles/services/single-metadata-service"; import type { SingleRegistryService } from "../../domains/singles/services/single-registry-service"; import { resolveBuilderVersions } from "../../domains/versions/builder-versions"; import { resolveBuilderWebhooks } from "../../domains/webhooks/builder-webhooks"; @@ -85,10 +84,6 @@ import { resolveBuilderRevalidate } from "../../revalidation/builder-revalidate" import { getProductionNotifier } from "../../runtime/notifications/index"; import { isReservedResourceSlug } from "../../schemas/_zod/rbac"; import type { FieldDefinition } from "../../schemas/dynamic-collections"; -import { - getI18nArchiveDdl, - getI18nArchiveIndexRepairDdl, -} from "../../schemas/nextly-i18n-archive"; import { isSuperAdmin, listEffectivePermissions, @@ -103,10 +98,10 @@ import { buildFullDesiredSchema } from "../helpers/desired-schema"; import { getAdapterFromDI, getComponentRegistryFromDI, - getConfigFromDI, getMigrationJournalFromDI, getSchemaRegistryFromDI, getSingleEntryServiceFromDI, + getSingleMetadataServiceFromDI, getSingleRegistryFromDI, } from "../helpers/di"; import { @@ -197,31 +192,6 @@ function injectSingleDefaultFields( }; } -// ============================================================ -// Migration SQL execution helper -// ============================================================ - -async function executeMigrationStatements( - adapter: DrizzleAdapter, - migrationSQL: string -): Promise { - const statements = migrationSQL - .split("--> statement-breakpoint") - .map(s => s.trim()) - .filter(s => s.length > 0); - - for (const statement of statements) { - const cleanStatement = statement - .split("\n") - .filter(line => !line.trim().startsWith("--")) - .join("\n") - .trim(); - if (cleanStatement) { - await adapter.executeQuery(cleanStatement); - } - } -} - // ============================================================ // Singles services bundle // ============================================================ @@ -229,167 +199,8 @@ async function executeMigrationStatements( interface SinglesServices { registry: SingleRegistryService; entry: SingleEntryService; -} - -// ============================================================ -// i18n helpers -// ============================================================ - -/** - * Provision (create / ADD-DROP columns / drop) the single's companion `single__locales` - * table out-of-band after a schema apply, then register its runtime table so per-language - * reads/writes resolve without a restart. The push pipeline excludes companion tables, so every - * single write/create/apply path that changes the localized field set goes through here. - * - * Shared by createSingle, updateSingleSchema and applySingleSchemaChanges so the three stay in - * lockstep. No-op when the single isn't localized (a non-localized single has no companion). - * The DDL reconcile throws on failure (data-integrity critical); the runtime registration is - * best-effort (recovered on next restart). - */ -async function reconcileSingleCompanion(args: { - slug: string; - tableName: string; - oldFields: FieldDefinition[]; - newFields: FieldDefinition[]; - /** Localization state AFTER this save (requested). */ - localized: boolean; - /** Localization state BEFORE this save (persisted). Drives enable/disable detection. */ - wasLocalized: boolean; - status: boolean; - /** - * Whether the single had Draft/Published BEFORE this apply. - * - * Separate from `status` because the disable restore asks a different question: not what the - * single is being saved as, but whether main carried `status` and the companion `_status` - * beforehand — a copy from columns that were not there fails the whole migration. - */ - wasStatus: boolean; - adapter: DrizzleAdapter; -}): Promise { - const { - slug, - tableName, - oldFields, - newFields, - localized, - status, - wasStatus, - adapter, - } = args; - const wasLocalized = args.wasLocalized; - // Nothing to do when the single was and remains non-localized. - if (!wasLocalized && !localized) return; - - const dialect = adapter.dialect; - const companionTable = `${tableName}_locales`; - const companionExists = await adapter.tableExists(companionTable); - // Only introspect `_status` when it can matter: an existing companion that stays localized - // (a later Draft/Published toggle must ADD/DROP `_status`). - const companionHasStatus = - companionExists && wasLocalized && localized - ? await companionHasStatusColumn(adapter, companionTable) - : undefined; - - // The seed (enable) and restore (disable) copy the default-locale value to/from the companion; - // read the configured default locale (falls back to "en" when localization isn't configured). - const defaultLocale = getConfigFromDI()?.localization?.defaultLocale ?? "en"; - - const plan = buildCompanionTransitionStatements({ - // The companion mirrors the main table, and a single's table comes from the same builder as a collection's. - builtBy: "collection" as const, - slug, - tableName, - dialect, - defaultLocale, - status, - wasLocalized, - isLocalized: localized, - oldFields, - newFields, - companionExists, - companionHasStatus, - wasStatus, - // Which translatable columns the main table still carries. A disable must not re-add one that - // is already there, and must still restore it: presence says the column exists, never that its - // value is current, because every localized write went to the companion alone. - existingMainColumns: await localizedColumnsOnMain( - adapter, - tableName, - oldFields - ).then(cols => cols.map(c => c.name)), - }); - - // A disable archives non-default translations, so ensure `nextly_i18n_archive` exists first - // (Builder entities have no `nextly migrate` step to provision it). Idempotent. - if (plan.needsArchive) { - for (const stmt of getI18nArchiveDdl(dialect)) { - await adapter.executeQuery(stmt); - } - // MySQL's table DDL cannot restore an index the table is missing, and - // index-only drift produces no reconcile operations, so the repair runs - // here. Tolerated rather than checked first: attempting it and accepting - // "duplicate key name" is one round trip instead of two, and the same - // tolerance the schema executor already applies. - const indexRepair = getI18nArchiveIndexRepairDdl(dialect); - if (indexRepair) { - try { - await adapter.executeQuery(indexRepair); - } catch (err) { - if (!isIdempotencyError(err)) throw err; - } - } - } - for (const stmt of plan.statements) { - await adapter.executeQuery(stmt); - } - - // The transition record describes a companion that no longer exists, so it stops being true the - // moment the disable succeeds. Left behind, it would refuse the next enable's real source locale - // — the check that protects a live transition would block a legitimate one instead. - if (plan.companionDropped) { - // The other half of "this companion is gone": readiness remembers only that one exists. - const { forgetCompanionReadiness } = await import( - "../../domains/i18n/runtime/companion-readiness" - ); - forgetCompanionReadiness(adapter, `${tableName}_locales`); - const { resolveTransitionStore } = await import( - "../../domains/i18n/migration/transition-recorder" - ); - const { forgetI18nTransition } = await import( - "../../domains/i18n/migration/transition-state" - ); - await forgetI18nTransition( - await resolveTransitionStore(adapter), - "single", - slug - ); - } - - // Register the companion runtime table (best-effort — next boot re-registers it). Skipped when - // the plan dropped the companion (disable) or the single is no longer localized. - if (!plan.companionDropped && localized) { - try { - const companion = buildCompanionRuntimeTable({ - slug, - tableName, - fields: newFields, - dialect, - localized: true, - status, - }); - if (companion) { - getSchemaRegistryFromDI()?.registerDynamicSchema( - companion.companionTableName, - companion.table - ); - } - } catch (err) { - const msg = err instanceof Error ? err.message : String(err); - console.warn( - `[reconcileSingleCompanion] Companion runtime registration failed for '${slug}': ${msg}.` - ); - } - } + /** Owns the pairing of a table change with the registry write that records it. */ + metadata: SingleMetadataService; } // ============================================================ @@ -655,7 +466,13 @@ const SINGLES_METHODS: Record> = { const owner = (await svc.registry.getAllSingles()).find( s => s.tableName === tableName ); - if (owner) { + // 🔴 Except when the owner is an unfinished attempt at THIS single. A create writes its + // intent before touching the database, so an interrupted one leaves a row that still owns + // the name. Refusing here would make that row a permanent blocker rather than the recovery + // aid it is meant to be — the service adopts it and re-runs the idempotent DDL instead. + const ownerIsUnfinishedRetry = + owner?.slug === b.slug && owner?.migrationStatus !== "applied"; + if (owner && !ownerIsUnfinishedRetry) { throw NextlyError.duplicate({ logContext: { reason: "single-table-conflict", @@ -666,159 +483,37 @@ const SINGLES_METHODS: Record> = { }); } - // Generate migration SQL for the Single's data table. Passing - // isSingle: true skips the slug column and auto-adds updated_at. - // Pass hasStatus so the data table also gets a `status` column - // when the user opted into Draft/Published — without it the - // runtime schema would expect a column the DDL never created. - // The dialect comes from the adapter that will run this DDL, not from - // the service's own DB_DIALECT default — that variable is optional and - // falls back to "postgresql", so an app configured with only a MySQL or - // SQLite DATABASE_URL would create this table as PostgreSQL. - // - // Read the same optional way the execution below reads it: with no - // adapter registered the statements are generated and never run, so the - // service keeps its own default rather than this path demanding a - // connection it is not going to use. - const createDialect = container.has("adapter") - ? container.get("adapter").getCapabilities().dialect - : undefined; - const schemaService = new DynamicCollectionSchemaService( - undefined, - createDialect - ); - const isLocalized = b.localized === true; - const migrationSQL = schemaService.generateMigrationSQL( - tableName, - b.fields as unknown as FieldDefinition[], - // i18n: omit translatable columns from the main table when localized — they live in the - // companion single__locales table (provisioned below), mirroring collections. - { isSingle: true, hasStatus: b.status === true, localized: isLocalized } - ); - - // Run migration immediately (same semantics as Collections). - let migrationStatus: "pending" | "applied" | "failed" = "pending"; - - try { - if (container.has("adapter")) { - const adapter = container.get("adapter"); - - await executeMigrationStatements(adapter, migrationSQL); - - const tableExists = await adapter.tableExists(tableName); - if (tableExists) { - migrationStatus = "applied"; - - // Register runtime schema so the adapter can resolve this - // table immediately without a server restart. - try { - const { generateRuntimeSchema } = await import( - "../../domains/schema/services/runtime-schema-generator" - ); - const dialect = adapter.getCapabilities().dialect; - const { table: runtimeTable } = generateRuntimeSchema( - tableName, - b.fields as unknown as FieldDefinition[], - dialect, - // i18n: main runtime table omits translatable columns for a localized single. - { status: b.status === true, localized: isLocalized } - ); - const resolver = ( - adapter as unknown as { - tableResolver?: { - registerDynamicSchema?: ( - name: string, - table: unknown - ) => void; - }; - } - ).tableResolver; - if ( - resolver && - typeof resolver.registerDynamicSchema === "function" - ) { - resolver.registerDynamicSchema(tableName, runtimeTable); - } - } catch { - // Non-fatal: schema will be registered on next server restart. - } - - // i18n: provision the companion single__locales table for a localized single - // (create-only — the single is brand new) and register its runtime table. The push - // pipeline excludes companions, so this is the only place it gets created on create. - try { - await reconcileSingleCompanion({ - slug: b.slug, - tableName, - oldFields: [], - newFields: b.fields as unknown as FieldDefinition[], - localized: isLocalized, - // A brand-new single was never localized before, so a localized create is a - // create-only companion (no seed/drop) rather than an enable transition. - wasLocalized: false, - // A single being created has no prior state at all. - wasStatus: false, - status: b.status === true, - adapter, - }); - } catch (companionErr) { - migrationStatus = "failed"; - const m = - companionErr instanceof Error - ? companionErr.message - : String(companionErr); - console.error( - `[Singles] Companion provisioning failed for "${tableName}": ${m}` - ); - } - } else { - migrationStatus = "failed"; - console.error( - `[Singles] Table "${tableName}" was not created after migration` - ); - } - } else { - console.warn( - "[Singles] No adapter found in container, migration not executed" - ); - } - } catch (migrationError) { - migrationStatus = "failed"; - const message = - migrationError instanceof Error - ? migrationError.message - : String(migrationError); - console.error("[Singles] Migration execution failed:", message); - console.error("[Singles] Migration SQL was:", migrationSQL); - } - - const single = await svc.registry.registerSingle({ - slug: b.slug, - label: b.label, - tableName, - description: b.description, - fields: b.fields, - admin: b.admin, - source: "ui", - locked: false, - // Forward the Draft/Published flag so admin-created Singles that - // opt in light up the Save Draft / Publish split. - status: b.status === true, - // i18n: persist the Internationalization flag so the single reads/writes per language. - localized: isLocalized, - // Persist version history from the create payload; without it a Single - // created with the switch on is written unversioned and the switch - // reads as off the moment the editor loads. Retention rides along. - versions: resolveBuilderVersions(b.versions, b.versionsMaxPerDoc), - // Cache-revalidation opt-out from the create payload (null = standard - // tags, { disable: true } = off), so the write path reads it back. - revalidate: resolveBuilderRevalidate(b.revalidate), - // Webhook recording opt-out from the create payload (null = record, - // { record: false } = off), so boot reads it back after a restart. - webhooks: resolveBuilderWebhooks(b.webhooks), - schemaHash, - migrationStatus, - }); + // The table change and the registry row are one operation, so they are issued as one: + // the service persists the intent, applies the DDL, provisions the localized companion + // and records the outcome. Splitting them here is what left a created table with no row + // describing it whenever the process stopped in between. + const { record: single, migrationStatus } = + await svc.metadata.createSingle({ + slug: b.slug, + label: b.label, + tableName, + description: b.description, + fields: b.fields, + admin: b.admin, + source: "ui", + locked: false, + // Forward the Draft/Published flag so admin-created Singles that + // opt in light up the Save Draft / Publish split. + status: b.status === true, + // i18n: persist the Internationalization flag so the single reads/writes per language. + localized: b.localized === true, + // Persist version history from the create payload; without it a Single + // created with the switch on is written unversioned and the switch + // reads as off the moment the editor loads. Retention rides along. + versions: resolveBuilderVersions(b.versions, b.versionsMaxPerDoc), + // Cache-revalidation opt-out from the create payload (null = standard + // tags, { disable: true } = off), so the write path reads it back. + revalidate: resolveBuilderRevalidate(b.revalidate), + // Webhook recording opt-out from the create payload (null = record, + // { record: false } = off), so boot reads it back after a restart. + webhooks: resolveBuilderWebhooks(b.webhooks), + schemaHash, + }); // Auto-seed read/update permissions for the new single. if (container.has("permissionSeedService")) { @@ -847,10 +542,20 @@ const SINGLES_METHODS: Record> = { // Migration status drives the toast copy so admins see "table // applied" vs "run migrations" without an extra round-trip. + // 🔴 Three outcomes, not two. `failed` used to be rare enough to hide behind the pending + // wording; the shape verification makes it a routine answer, and "run migrations" is then + // advice that cannot work — no migration repairs a table whose columns do not match. Telling + // an admin their Single was created when it cannot be read is the worst of the three. + // + // Still 201, and still carrying the row: the Single WAS registered, `migrationStatus` rides + // in the body for a client that branches on it, and keeping the record is what makes the + // retry a resume rather than a duplicate. const message = migrationStatus === "applied" ? `Single "${b.slug}" created and table applied!` - : `Single "${b.slug}" created. Run migrations to apply the table.`; + : migrationStatus === "failed" + ? `Single "${b.slug}" was created but its table could not be applied. It cannot be read until the schema change succeeds.` + : `Single "${b.slug}" created. Run migrations to apply the table.`; return respondMutation(message, single, { status: 201 }); }, }, @@ -1297,7 +1002,7 @@ const SINGLES_METHODS: Record> = { // Same boundary as the ALTER branch below: once a statement is sent, a failure // has left the table partly built rather than untouched. migrationBegan = true; - await executeMigrationStatements(adapter, createSQL); + await applyMigrationStatements(adapter, createSQL); } else { const db = adapter.getDrizzle(); const liveDialect = adapter.getCapabilities().dialect; @@ -1321,11 +1026,39 @@ const SINGLES_METHODS: Record> = { // Past this point a statement may have run, so a failure is a partly-applied // migration rather than an edit that never started. migrationBegan = true; - await executeMigrationStatements(adapter, migrationSQL); + await applyMigrationStatements(adapter, migrationSQL); } const tableExistsAfter = await adapter.tableExists(tableName); - if (tableExistsAfter) { + // 🔴 Existence is not enough now that a re-run is tolerated. Retrying an ALTER that + // already added a column swallows the duplicate-column error, so a retry whose field + // set changed shape in between would record the NEW description over the OLD physical + // column. Compared through the same builder the schema diff uses, so this cannot + // disagree with the pipeline about a column's type. + const shapeProblems = tableExistsAfter + ? await shapeMismatches( + adapter, + adapter.getCapabilities().dialect, + tableName, + buildDesiredTableFromFields( + tableName, + normalizedNewFields, + adapter.getCapabilities().dialect, + { + hasStatus, + localized: isLocalized, + // A single's table comes from the same builder as a collection's. + builtBy: "collection", + } + ) + ) + : []; + if (shapeProblems.length > 0) { + migrationStatus = "failed"; + console.error( + `[Singles] Table "${tableName}" does not match this schema after the update: ${shapeProblems.join("; ")}` + ); + } else if (tableExistsAfter) { migrationStatus = "applied"; // Re-register runtime schema with updated fields. @@ -1822,11 +1555,13 @@ export function dispatchSingles( ): Promise { const singleRegistry = getSingleRegistryFromDI(); const singleEntryService = getSingleEntryServiceFromDI(); + const singleMetadataService = getSingleMetadataServiceFromDI(); - if (!singleRegistry || !singleEntryService) { + if (!singleRegistry || !singleEntryService || !singleMetadataService) { const missing: string[] = []; if (!singleRegistry) missing.push("singleRegistryService"); if (!singleEntryService) missing.push("singleEntryService"); + if (!singleMetadataService) missing.push("singleMetadataService"); let containerStatus = "unknown"; try { @@ -1847,7 +1582,11 @@ export function dispatchSingles( const handler = SINGLES_METHODS[method]; if (!handler) throw new Error(`Unknown method: ${method}`); return handler.execute( - { registry: singleRegistry, entry: singleEntryService }, + { + registry: singleRegistry, + entry: singleEntryService, + metadata: singleMetadataService, + }, params, body ); diff --git a/packages/nextly/src/dispatcher/helpers/di.ts b/packages/nextly/src/dispatcher/helpers/di.ts index ed8659b64..d773d6a0c 100644 --- a/packages/nextly/src/dispatcher/helpers/di.ts +++ b/packages/nextly/src/dispatcher/helpers/di.ts @@ -14,6 +14,7 @@ import { container } from "../../di/container"; import type { NextlyServiceConfig } from "../../di/register"; import type { DrizzleMigrationJournal } from "../../domains/schema/journal/migration-journal"; import type { SingleEntryService } from "../../domains/singles/services/single-entry-service"; +import type { SingleMetadataService } from "../../domains/singles/services/single-metadata-service"; import type { SingleRegistryService } from "../../domains/singles/services/single-registry-service"; import type { CollectionRegistryService } from "../../services/collections/collection-registry-service"; import type { CollectionsHandler } from "../../services/collections-handler"; @@ -71,6 +72,19 @@ export function getSingleRegistryFromDI(): SingleRegistryService | undefined { return undefined; } +export function getSingleMetadataServiceFromDI(): + | SingleMetadataService + | undefined { + try { + if (container.has("singleMetadataService")) { + return container.get("singleMetadataService"); + } + } catch { + // DI not initialized + } + return undefined; +} + export function getSingleEntryServiceFromDI(): SingleEntryService | undefined { try { if (container.has("singleEntryService")) { diff --git a/packages/nextly/src/domains/schema/services/__tests__/apply-migration-statements.test.ts b/packages/nextly/src/domains/schema/services/__tests__/apply-migration-statements.test.ts new file mode 100644 index 000000000..57adfd37a --- /dev/null +++ b/packages/nextly/src/domains/schema/services/__tests__/apply-migration-statements.test.ts @@ -0,0 +1,109 @@ +/** + * What the Builder's shared migration runner tolerates, and what it must still refuse. + * + * The tolerance exists because re-running a half-applied migration is the repair path, and on MySQL + * it was impossible: `CREATE INDEX` has no `IF NOT EXISTS`, so the second run reported a schema that + * was in fact correct as a failed migration and the retry failed identically every time. + * + * The refusal matters at least as much. `Duplicate entry ... for key` is MySQL's runtime DATA + * conflict, not a DDL artefact — swallowing it would let a rebuild's copy fail silently and the + * following drop destroy the rows that never copied. Both directions are asserted here because a + * tolerance is only safe if its edge is. + */ +import { describe, expect, it, vi } from "vitest"; + +import { applyMigrationStatements } from "../apply-migration-statements"; + +/** Records what it was asked to run, and fails on whichever statements are named. */ +function runner(failures: Record = {}) { + const executed: string[] = []; + return { + executed, + executeQuery: vi.fn(async (sql: string) => { + executed.push(sql); + const failure = failures[sql]; + if (failure) throw failure; + return []; + }), + }; +} + +const TWO_STATEMENTS = [ + "CREATE TABLE `single_page` (`id` varchar(36) NOT NULL)", + "--> statement-breakpoint", + "CREATE INDEX `idx_single_page_created_at` ON `single_page` (`created_at`)", +].join("\n"); + +describe("applyMigrationStatements", () => { + it("runs each statement the migration declares", async () => { + const adapter = runner(); + + await applyMigrationStatements(adapter, TWO_STATEMENTS); + + expect(adapter.executed).toEqual([ + "CREATE TABLE `single_page` (`id` varchar(36) NOT NULL)", + "CREATE INDEX `idx_single_page_created_at` ON `single_page` (`created_at`)", + ]); + }); + + /** + * The MySQL repair case, in the exact wording the driver produces. PostgreSQL and SQLite never + * reach here because they emit `IF NOT EXISTS` for indexes; MySQL cannot, so without this a + * create that stopped half way could never be finished. + */ + it("tolerates an index MySQL says is already there, and keeps going", async () => { + const index = + "CREATE INDEX `idx_single_page_created_at` ON `single_page` (`created_at`)"; + const adapter = runner({ + [index]: new Error("Duplicate key name 'idx_single_page_created_at'"), + }); + + await expect( + applyMigrationStatements( + adapter, + `${index}\n--> statement-breakpoint\nSELECT 1` + ) + ).resolves.toBeUndefined(); + + // The statement AFTER the tolerated one still ran: tolerating must not abandon the migration. + expect(adapter.executed).toContain("SELECT 1"); + }); + + it("tolerates a table that already exists", async () => { + const create = "CREATE TABLE `single_page` (`id` varchar(36) NOT NULL)"; + const adapter = runner({ + [create]: new Error("Table 'single_page' already exists"), + }); + + await expect( + applyMigrationStatements(adapter, create) + ).resolves.toBeUndefined(); + }); + + /** + * 🔴 The edge that makes the tolerance safe. This is a row conflict from an INSERT..SELECT during + * a table rebuild, and its wording is one word away from the index case above. Swallowed, the + * rebuild's copy fails silently and the drop that follows destroys the rows that did not copy. + */ + it("still fails on a duplicate ROW, which is data loss rather than a re-run", async () => { + const copy = "INSERT INTO `single_page__new` SELECT * FROM `single_page`"; + const adapter = runner({ + [copy]: new Error("Duplicate entry 'abc' for key 'single_page.PRIMARY'"), + }); + + await expect(applyMigrationStatements(adapter, copy)).rejects.toThrow( + /Duplicate entry/ + ); + }); + + it("still fails on an ordinary broken statement", async () => { + const broken = "CREATE TABLE `single_page` (`id` NOT A TYPE)"; + const adapter = runner({ + [broken]: new Error("You have an error in your SQL syntax"), + }); + + await expect(applyMigrationStatements(adapter, broken)).rejects.toThrow( + /SQL syntax/ + ); + }); +}); diff --git a/packages/nextly/src/domains/schema/services/apply-migration-statements.ts b/packages/nextly/src/domains/schema/services/apply-migration-statements.ts new file mode 100644 index 000000000..afdc8ad92 --- /dev/null +++ b/packages/nextly/src/domains/schema/services/apply-migration-statements.ts @@ -0,0 +1,65 @@ +/** + * Run a generated migration against a live adapter, one statement at a time. + * + * ## Why this is shared rather than written where it is needed + * + * Every Schema-Builder path that changes a table does the same two things: split the generated SQL + * into statements, and run them. Both dispatchers had their own copy of that, and a private copy of + * a splitting rule is exactly what `splitStatements` exists to prevent — its header records two + * earlier copies drifting and causing a real bug. + * + * ## Re-running has to be safe, and on MySQL it was not + * + * A create or an apply that stops half way leaves schema behind. Finishing it means running the + * same statements again over what is already there, so "already exists" has to be tolerated: + * + * - PostgreSQL and SQLite emit `IF NOT EXISTS` for tables AND indexes, so they tolerate it + * themselves. + * - **MySQL has no such form for `CREATE INDEX`.** A second run died on the index and the caller + * recorded a schema that was in fact correct as a failed migration, with no way forward — the + * retry failed the same way every time. + * + * The boot-time and pipeline paths (`fresh-push`, `DrizzleStatementExecutor`) reached this + * conclusion long ago and already tolerate it. Only the Builder's own request paths were missed. + * + * 🔴 The tolerance is deliberately narrow. `isIdempotencyError` is anchored to the DDL wordings — + * "already exists", duplicate column, duplicate KEY NAME — and refuses to match MySQL's + * `Duplicate entry ... for key` (error 1062), which is a runtime DATA conflict. Swallowing that + * would let a rebuild's copy fail silently and the following drop destroy the rows that did not + * copy. + * + * @module domains/schema/services/apply-migration-statements + */ + +import { + isIdempotencyError, + splitStatements, +} from "../pipeline/sql-statement-utils"; + +/** + * The adapter surface this needs. + * + * Declared structurally rather than importing `DrizzleAdapter`, so the helper states its one + * requirement and any caller holding something narrower can still use it. + */ +export interface MigrationStatementRunner { + executeQuery(sql: string, params?: unknown[]): Promise; +} + +/** + * Apply a generated migration, tolerating statements the schema already satisfies. + * + * Throws on anything else, so a caller can still record the change as failed. + */ +export async function applyMigrationStatements( + adapter: MigrationStatementRunner, + migrationSQL: string +): Promise { + for (const statement of splitStatements([migrationSQL])) { + try { + await adapter.executeQuery(statement); + } catch (error) { + if (!isIdempotencyError(error)) throw error; + } + } +} diff --git a/packages/nextly/src/domains/schema/services/verify-applied-shape.ts b/packages/nextly/src/domains/schema/services/verify-applied-shape.ts new file mode 100644 index 000000000..85d83e212 --- /dev/null +++ b/packages/nextly/src/domains/schema/services/verify-applied-shape.ts @@ -0,0 +1,143 @@ +/** + * Did the table the migration just ran against actually end up the shape it was asked for? + * + * ## Why "the table exists" stopped being enough + * + * Every Builder apply path used to prove success with `tableExists`. That was never a strong check + * and it became a weak one the moment those paths started tolerating "already exists" errors so a + * half-applied migration could be finished. The tolerance is right — MySQL has no `IF NOT EXISTS` + * for `CREATE INDEX`, and a companion's `ADD COLUMN` has none on any dialect, so without it a + * correct schema reported `failed` forever. But it removes the loud failure that used to make a + * MISMATCHED table obvious: + * + * - `CREATE TABLE IF NOT EXISTS` is a no-op against an existing table, on every dialect. + * - The index statements after it are now tolerated. + * - So a repair over a table left by an earlier attempt emits no error at all, even when the field + * set, the lifecycle columns or a field's TYPE have changed in between. + * + * The result was a registry row recording `applied` and a runtime schema describing columns the + * database does not have, which surfaces later as a failing read far from its cause. + * + * ## What is compared, and why it is not hand-rolled + * + * The desired shape comes from `buildDesiredTableFromFields` — the same builder the schema diff + * compares against — so this cannot disagree with the pipeline about what a column is called or + * what type it should be. That matters for three things a hand-written check kept missing: + * + * - **System columns.** `status` and the lifecycle columns come from create OPTIONS, not from the + * field list, so a check derived from fields alone passes while they are absent. + * - **Localized fields.** Translatable columns belong to the companion and must NOT be demanded on + * the main table. + * - **Provenance.** A text field with no stated width has no single right column; the builder that + * made the table decides. `builtBy` carries that. + * + * ## 🔴 Presence and nullability ONLY, and the limit is deliberate + * + * Types and index names are NOT compared, because the desired spec is the DIFF ENGINE's ideal + * schema and the Builder's own generators do not render exactly that. Three measured divergences, + * each of which made this verifier fail a create that works: + * + * - a `number` field with `format: "float"` is written `decimal(10,2)` by the direct create DDL + * while the descriptor says `float8`/`double`; + * - a `unique` field becomes an INLINE constraint, so the database names it itself + * (`__key` on PostgreSQL, a filtered `sqlite_autoindex_*` on SQLite) rather than + * the `uq_
_` the spec expects; + * - the field-group generator emits no `created_at` index at all, while the desired spec declares + * one for every component that has the column. + * + * Those disagreements are real and already tracked as their own work. Until the generators and the + * descriptor agree, comparing types or index names here reports a correct table as broken — which + * is a worse failure than the one this exists to catch, because it blocks work that was fine. + * Presence and nullability were measured against every fixture on all three dialects and produce + * no such false positives. + * + * @module domains/schema/services/verify-applied-shape + */ + +import type { SupportedDialect } from "@nextlyhq/adapter-drizzle/types"; + +import type { ColumnSpec, TableSpec } from "../pipeline/diff/types"; + +/** The adapter surface this needs: a Drizzle handle to introspect through. */ +export interface ShapeVerifyAdapter { + getDrizzle(): T; +} + +/** + * How the live table differs from the desired one, in wording an operator can act on. + * + * Empty means it matches. A column that is absent and one that is enforced but no longer declared + * are reported together because the caller treats them the same way: the migration did not + * converge. + */ +export async function shapeMismatches( + adapter: ShapeVerifyAdapter, + dialect: SupportedDialect, + tableName: string, + desired: TableSpec +): Promise { + let live; + try { + const { introspectLiveSnapshot } = await import( + "../pipeline/diff/introspect-live" + ); + const snapshot = await introspectLiveSnapshot( + adapter.getDrizzle(), + dialect, + [tableName] + ); + live = snapshot.tables.find(t => t.name === tableName); + } catch { + // 🔴 A check that cannot RUN must not invent a failure for a migration that reported success. + // Introspection needs a live catalog, and a caller may hold a connection that cannot answer — + // failing the schema change because the verification was unavailable would be strictly worse + // than the unverified behaviour this replaced. + return []; + } + + // Nothing came back for the table. That is the introspection failing to see it rather than the + // table having no columns, and existence is the caller's question to ask, not this one's — so + // reporting every column as missing here would turn an unreadable catalog into a false failure. + if (!live || live.columns.length === 0) return []; + + const liveByName = new Map( + live.columns.map(column => [column.name, column]) + ); + + const problems: string[] = []; + for (const column of desired.columns) { + const actual = liveByName.get(column.name); + if (!actual) { + problems.push(`${column.name} is missing`); + continue; + } + // Nullability is part of the shape, not a detail of it: a column the Builder now calls optional + // while the database still has it NOT NULL accepts every write the Builder considers valid and + // then fails the constraint. Reported after the type so one column produces one problem. + if (column.nullable !== actual.nullable) { + problems.push( + `${column.name} is ${actual.nullable ? "nullable" : "NOT NULL"}, expected ${ + column.nullable ? "nullable" : "NOT NULL" + }` + ); + } + } + // 🔴 What the table has and the schema no longer wants, which a desired-side walk cannot see. + // + // A repair that REMOVES a field leaves its column in place, because the re-run no-ops rather + // than dropping anything. If that column was required, writes that omit the removed field now + // fail its NOT NULL constraint — the Builder considers them valid and the database does not. + // Only NOT NULL columns are reported: a leftover nullable column accepts every write and is a + // tidiness question rather than a correctness one, and reporting it would fail repairs that are + // functionally complete. + const wanted = new Set(desired.columns.map(column => column.name)); + for (const column of live.columns) { + if (!wanted.has(column.name) && !column.nullable) { + problems.push( + `${column.name} is still NOT NULL but is no longer declared` + ); + } + } + + return problems; +} diff --git a/packages/nextly/src/domains/singles/__tests__/single-create-schema-change.integration.test.ts b/packages/nextly/src/domains/singles/__tests__/single-create-schema-change.integration.test.ts new file mode 100644 index 000000000..3114ab660 --- /dev/null +++ b/packages/nextly/src/domains/singles/__tests__/single-create-schema-change.integration.test.ts @@ -0,0 +1,534 @@ +/** + * Proof that a Schema-Builder create REQUEST leaves a Single that actually works: the physical + * table exists, the registry row records it as applied, and a localized create also gets its + * companion. + * + * ## Why this is an integration test and why it did not exist + * + * Every other test of this path stops short of a database. The dispatcher's forwarding suites pin + * the statements the adapter is handed; the generators' snapshots pin what each renders. Neither + * runs a statement, so neither can tell a create that works from one that emits perfectly-shaped + * DDL a real engine then rejects. + * + * That gap has been exploited before: a change to how a `slug` column's width was resolved made + * MySQL refuse the `CREATE UNIQUE INDEX` that follows the table, so the table was never created at + * all. The full unit suite read identical before and after. Only a live database disagreed. + * + * The registry row and the table are written by one service and the assertions check BOTH, because + * the failure this guards against is precisely the two halves disagreeing: a row saying "applied" + * over a table that is not there is the state a user meets as "no such table" on first read. + * + * ## What the second create proves + * + * The service claims to be recoverable and idempotent rather than atomic — the only claim + * available, since MySQL commits DDL implicitly and no ordering makes the pair atomic there. What + * makes that claim worth anything is that the DDL half can be re-run over a table it already + * created. So the repair case is exercised directly: the registry row is removed while the table + * stays, and the same create is issued again. It has to succeed and report `applied`. + * + * Self-skips per dialect on the standard rule: SQLite always runs, the servers run when their URL + * is set. Each dialect gets its own throwaway database. + */ +import { afterEach, describe, expect, it } from "vitest"; + +import { dispatchSingles } from "../../../dispatcher/handlers/single-dispatcher"; +import { + createTestNextly, + getConfiguredTestDialects, + type TestNextly, +} from "../../../plugins/test-nextly"; +let current: TestNextly | undefined; + +afterEach(async () => { + await current?.destroy(); + current = undefined; +}); + +/** The registry's own view of a slug, or undefined when it holds none. */ +async function registryRow( + instance: TestNextly, + slug: string +): Promise<{ migrationStatus?: string; tableName?: string } | undefined> { + const registry = instance.getService("singleRegistryService"); + return (await registry.getSingleBySlug(slug)) ?? undefined; +} + +for (const dialect of getConfiguredTestDialects()) { + describe(`createSingle applies its schema change — ${dialect}`, () => { + /** + * Slugs are per-dialect rather than shared: the suites run in one process, and a name reused + * across them turns a leaked row into a duplicate-slug rejection in a later suite rather than a + * failure where the leak happened. + */ + const plain = `sc_${dialect.slice(0, 2)}_plain`; + const localized = `sc_${dialect.slice(0, 2)}_loc`; + + it("creates the table and records it as applied", async () => { + current = await createTestNextly({ dialect }); + + await dispatchSingles( + "createSingle", + {}, + { + slug: plain, + label: "Plain", + fields: [ + { name: "body", type: "text" }, + { name: "views", type: "number" }, + ], + } + ); + + const row = await registryRow(current, plain); + + // FIRST, because it is the informative failure: the service's own recorded outcome names the + // problem, where a bare "table missing" only says the end state is wrong. + expect(row?.migrationStatus).toBe("applied"); + expect(await current.adapter.tableExists(`single_${plain}`)).toBe(true); + }); + + it("gives a localized single its companion table", async () => { + current = await createTestNextly({ + dialect, + localization: { locales: ["en", "es"], defaultLocale: "en" }, + }); + + await dispatchSingles( + "createSingle", + {}, + { + slug: localized, + label: "Localized", + localized: true, + fields: [ + { name: "headline", type: "text", localized: true }, + { name: "views", type: "number" }, + ], + } + ); + + const table = `single_${localized}`; + const row = await registryRow(current, localized); + + expect(row?.migrationStatus).toBe("applied"); + expect(await current.adapter.tableExists(table)).toBe(true); + // The other half of a localized single's storage. Without it the translatable value has + // nowhere to live, because the main table no longer carries the column either. + expect(await current.adapter.tableExists(`${table}_locales`)).toBe(true); + }); + + /** + * 🔴 The generator VALIDATES as well as renders, so a rejected create must leave nothing. + * + * A required relationship declaring `onDelete: "set null"` is refused: no database can null a + * reference the column forbids. That refusal comes from `generateMigrationSQL`, which means it + * happens on the same path that writes the table — so if the registry row were persisted + * first, this request would strand a `pending` Single with its permissions seeded, and the + * corrected retry would collide with the slug it had just created instead of succeeding. + */ + it("leaves no row behind when the generator rejects the fields", async () => { + current = await createTestNextly({ dialect }); + + const rejected = { + slug: `${plain}_bad`, + label: "Bad", + fields: [ + // `target` is the key the generator's relationship branch reads; the referenced + // collection need not exist, because the refusal happens while the DDL is still being + // rendered and no statement is ever run. + { + name: "author", + type: "relationship", + required: true, + options: { target: "authors", onDelete: "set null" }, + }, + ], + }; + + await expect( + dispatchSingles("createSingle", {}, rejected) + ).rejects.toThrow(); + + // Nothing persisted, so the corrected retry is a fresh create rather than a slug collision. + expect(await registryRow(current, rejected.slug)).toBeUndefined(); + expect(await current.adapter.tableExists(`single_${rejected.slug}`)).toBe( + false + ); + }); + + it("re-applies over a table it already created", async () => { + current = await createTestNextly({ dialect }); + + const payload = { + slug: plain, + label: "Plain", + fields: [{ name: "body", type: "text" }], + }; + + await dispatchSingles("createSingle", {}, payload); + expect(await current.adapter.tableExists(`single_${plain}`)).toBe(true); + + // The interrupted state the guarantee is about: the table landed, the row describing it did + // not survive. Recovery has to be able to finish the operation over what is already there. + const registry = current.getService("singleRegistryService"); + // `force` because a Single is site-wide configuration the registry refuses to drop casually; + // here the row is being removed on purpose to recreate an interrupted create. + await registry.deleteSingle(plain, { force: true }); + expect(await registryRow(current, plain)).toBeUndefined(); + + await dispatchSingles("createSingle", {}, payload); + + const row = await registryRow(current, plain); + expect(row?.migrationStatus).toBe("applied"); + expect(await current.adapter.tableExists(`single_${plain}`)).toBe(true); + }); + + /** + * 🔴 Exactly ONE retry may own a failed create, even when two start together. + * + * Reading `failed` and then writing is not enough: both retries can observe it before either + * writes, and both would then run their own DDL against one slug. The claim puts the status in + * the WHERE clause so the database picks the winner. Issued concurrently here rather than in + * sequence, because a sequential pair would pass even with the check-then-act version. + */ + it("lets only one of two concurrent retries take over", async () => { + current = await createTestNextly({ dialect }); + + const payload = { + slug: plain, + label: "Plain", + fields: [{ name: "body", type: "text" }], + }; + await dispatchSingles("createSingle", {}, payload); + + const registry = current.getService("singleRegistryService"); + await registry.updateMigrationStatus(plain, "failed"); + + // The same payload on purpose: that is the retry a user actually makes, and the one whose + // unchanged field hash stops `updateSingle` from re-flagging the row. + const outcomes = await Promise.allSettled([ + dispatchSingles("createSingle", {}, payload), + dispatchSingles("createSingle", {}, payload), + ]); + + const claimed = outcomes.filter(o => o.status === "fulfilled"); + expect(claimed).toHaveLength(1); + expect((await registryRow(current, plain))?.migrationStatus).toBe( + "applied" + ); + }); + + /** + * A normal schema UPDATE on a Single must stay `applied`. + * + * 🔴 The ALTER path builds its verification shape from a normalized field list that carries a + * synthetic `title` so the generator can see the existing system column. If that list reaches + * the descriptor unchanged, `title` is described as a Builder text field rather than the system + * one — and on MySQL the system column is `varchar(255)` while a Builder text field is not, so + * an ordinary successful field update would be recorded as failed. + */ + it("keeps a schema update applied after adding a field", async () => { + current = await createTestNextly({ dialect }); + + await dispatchSingles( + "createSingle", + {}, + { + slug: plain, + label: "Plain", + fields: [{ name: "body", type: "text" }], + } + ); + expect((await registryRow(current, plain))?.migrationStatus).toBe( + "applied" + ); + + await dispatchSingles( + "updateSingleSchema", + { slug: plain }, + { + fields: [ + { name: "body", type: "text" }, + { name: "subtitle", type: "text" }, + ], + } + ); + + expect((await registryRow(current, plain))?.migrationStatus).toBe( + "applied" + ); + }); + + /** + * 🔴 The other half of adoption: a create still in flight must NOT be taken over. + * + * `pending` is indistinguishable from "running right now". Adopting one would overwrite its + * row with a second payload while its DDL is still building the first schema, after which the + * original confirms `applied` against a description that is no longer its own. Refusing keeps + * the two requests from interleaving; serialising them properly is the migration lock's job. + */ + it("refuses to take over a create that has not recorded an outcome", async () => { + current = await createTestNextly({ dialect }); + + const payload = { + slug: plain, + label: "Plain", + fields: [{ name: "body", type: "text" }], + }; + await dispatchSingles("createSingle", {}, payload); + + const registry = current.getService("singleRegistryService"); + await registry.updateMigrationStatus(plain, "pending"); + + await expect( + dispatchSingles("createSingle", {}, payload) + ).rejects.toThrow(); + }); + + /** + * 🔴 A removed REQUIRED field leaves a NOT NULL column the schema no longer declares. + * + * The re-run no-ops rather than dropping anything, so writes that omit the removed field now + * fail its constraint — the Builder considers them valid and the database does not. Only a + * desired-side walk would miss this, because the column is not in the desired set at all. + */ + it("refuses when a removed required column is still enforced", async () => { + current = await createTestNextly({ dialect }); + + await dispatchSingles( + "createSingle", + {}, + { + slug: plain, + label: "Plain", + fields: [ + { name: "body", type: "text" }, + { name: "subtitle", type: "text", required: true }, + ], + } + ); + + const registry = current.getService("singleRegistryService"); + await registry.deleteSingle(plain, { force: true }); + + // `subtitle` is gone from the schema, but its NOT NULL column survives on the table. + await dispatchSingles( + "createSingle", + {}, + { + slug: plain, + label: "Plain", + fields: [{ name: "body", type: "text" }], + } + ); + + expect((await registryRow(current, plain))?.migrationStatus).toBe( + "failed" + ); + }); + + /** + * 🔴 Nullability is part of the shape, not a detail of it. + * + * A column the Builder now calls optional while the database still has it NOT NULL accepts + * every write the Builder considers valid and then fails the constraint — a failure that + * surfaces at write time, far from the migration that caused it. + */ + it("refuses when an existing column has the wrong nullability", async () => { + current = await createTestNextly({ dialect }); + + await dispatchSingles( + "createSingle", + {}, + { + slug: plain, + label: "Plain", + fields: [{ name: "body", type: "text", required: true }], + } + ); + + const registry = current.getService("singleRegistryService"); + await registry.deleteSingle(plain, { force: true }); + + // Same name, same type, now optional. The physical column keeps its NOT NULL. + await dispatchSingles( + "createSingle", + {}, + { + slug: plain, + label: "Plain", + fields: [{ name: "body", type: "text", required: false }], + } + ); + + expect((await registryRow(current, plain))?.migrationStatus).toBe( + "failed" + ); + }); + + /** + * 🔴 A LOCALIZED single's repair is REFUSED, deliberately, and this pins that decision. + * + * The companion reconcile is told `wasLocalized: false` and `oldFields: []`, because from the + * registry's point of view this is a brand-new localized single — the row describing the + * previous attempt is gone. Meeting a companion that already exists, its plan asks to ADD + * columns that are already there, and the companion path does NOT tolerate that. + * + * It could: the same tolerance the main table has would make this repair succeed. It is + * withheld because a half-finished localization ENABLE reaches this code in the identical + * state — the planner cannot tell them apart, since `existingMainColumns` is consumed only on + * the disable path — and tolerating it there would report success while default-locale content + * is still stranded on the main table. + * + * So a localized repair fails loudly and an operator sees it, rather than a half-migrated + * entity being recorded as applied. Reverse this only together with a way to detect a partial + * enable. + */ + it("refuses to repair a localized single's tables, loudly", async () => { + current = await createTestNextly({ + dialect, + localization: { locales: ["en", "es"], defaultLocale: "en" }, + }); + + const payload = { + slug: localized, + label: "Localized", + localized: true, + fields: [ + { name: "headline", type: "text", localized: true }, + { name: "views", type: "number" }, + ], + }; + + await dispatchSingles("createSingle", {}, payload); + const table = `single_${localized}`; + expect(await current.adapter.tableExists(`${table}_locales`)).toBe(true); + + const registry = current.getService("singleRegistryService"); + await registry.deleteSingle(localized, { force: true }); + + await dispatchSingles("createSingle", {}, payload); + + const row = await registryRow(current, localized); + expect(row?.migrationStatus).toBe("failed"); + // The tables are untouched by the refusal — nothing is destroyed, the operator is told. + expect(await current.adapter.tableExists(`${table}_locales`)).toBe(true); + }); + + /** + * 🔴 The dangerous half of tolerating a re-run: the table is there, but it is the WRONG table. + * + * `CREATE TABLE IF NOT EXISTS` no-ops against an existing table on every dialect, and the + * index statements that follow are tolerated as already-applied, so a repair over an orphan + * left by an earlier create emits no error even when the field set has changed. Existence + * alone would record a schema the database does not have, and every later read would address + * columns that are not there. So "applied" has to mean the columns are present, not merely + * that something of that name is. + */ + it("refuses to call it applied when an existing table lacks the new columns", async () => { + current = await createTestNextly({ dialect }); + + await dispatchSingles( + "createSingle", + {}, + { + slug: plain, + label: "Plain", + fields: [{ name: "body", type: "text" }], + } + ); + + // The orphan state: the table survives, the row describing it does not. + const registry = current.getService("singleRegistryService"); + await registry.deleteSingle(plain, { force: true }); + + // The same slug, now asking for a column the surviving table does not have. + await dispatchSingles( + "createSingle", + {}, + { + slug: plain, + label: "Plain", + fields: [ + { name: "body", type: "text" }, + { name: "subtitle", type: "text" }, + ], + } + ); + + const row = await registryRow(current, plain); + expect(row?.migrationStatus).toBe("failed"); + }); + + /** + * 🔴 An unfinished attempt must not become a permanent blocker. + * + * Writing the intent first is only worth doing if something can finish it. The row owns the + * slug the moment it is written, so a create interrupted before it could record its outcome + * would otherwise be refused as a duplicate for ever, leaving the user no way forward short of + * editing the registry by hand — strictly worse than the orphan table this ordering prevents, + * because an orphan at least left the slug free. + */ + it("resumes a create that recorded a failure", async () => { + current = await createTestNextly({ dialect }); + + const payload = { + slug: plain, + label: "Plain", + fields: [{ name: "body", type: "text" }], + }; + + await dispatchSingles("createSingle", {}, payload); + + // The state that is safe to take over: the attempt RECORDED its own failure, so nothing is + // still running against this slug. A `pending` row is deliberately NOT adopted — it is + // equally the state of a create that is in flight right now, and serialising that is the + // migration lock's job rather than a second mechanism here. + const registry = current.getService("singleRegistryService"); + await registry.updateMigrationStatus(plain, "failed"); + expect((await registryRow(current, plain))?.migrationStatus).toBe( + "failed" + ); + + // The retry a user would make. It has to succeed rather than collide with its own leftovers. + await dispatchSingles("createSingle", {}, payload); + + expect((await registryRow(current, plain))?.migrationStatus).toBe( + "applied" + ); + }); + + /** + * 🔴 The lifecycle columns come from create OPTIONS, not from the field list. + * + * A check derived from the fields alone passes here while `status` is absent, and the runtime + * schema then writes a Draft/Published column the table does not have. Caught only because the + * desired shape is built by the same builder the schema diff uses, which injects the system + * columns the options ask for. + */ + it("refuses when the existing table lacks the lifecycle column now asked for", async () => { + current = await createTestNextly({ dialect }); + + const fields = [{ name: "body", type: "text" }]; + await dispatchSingles( + "createSingle", + {}, + { slug: plain, label: "Plain", fields } + ); + + const registry = current.getService("singleRegistryService"); + await registry.deleteSingle(plain, { force: true }); + + // Same fields, but Draft/Published now switched on. The table is untouched by the re-run. + await dispatchSingles( + "createSingle", + {}, + { slug: plain, label: "Plain", fields, status: true } + ); + + expect((await registryRow(current, plain))?.migrationStatus).toBe( + "failed" + ); + }); + }); +} diff --git a/packages/nextly/src/domains/singles/services/reconcile-single-companion.ts b/packages/nextly/src/domains/singles/services/reconcile-single-companion.ts new file mode 100644 index 000000000..8f03c16cc --- /dev/null +++ b/packages/nextly/src/domains/singles/services/reconcile-single-companion.ts @@ -0,0 +1,200 @@ +/** + * Provision (create / ADD-DROP columns / drop) a Single's companion `single__locales` + * table out-of-band after a schema apply, then register its runtime table so per-language + * reads/writes resolve without a restart. The push pipeline excludes companion tables, so every + * single write/create/apply path that changes the localized field set goes through here. + * + * Shared by every path that changes a Single's schema so they stay in lockstep. No-op when the + * single isn't localized (a non-localized single has no companion). The DDL reconcile throws on + * failure (data-integrity critical); the runtime registration is best-effort (recovered on next + * restart). + * + * It lives beside the Singles services rather than inside the request handler because the handler + * is not the only caller: the schema-changing paths that own both the table change and the + * registry write need the same companion step, and a second copy is how two implementations of + * one rule drift apart. + * + * @module domains/singles/services/reconcile-single-companion + */ + +import type { DrizzleAdapter } from "@nextlyhq/adapter-drizzle"; + +import { + getConfigFromDI, + getSchemaRegistryFromDI, +} from "../../../dispatcher/helpers/di"; +import type { FieldDefinition } from "../../../schemas/dynamic-collections"; +import { + getI18nArchiveDdl, + getI18nArchiveIndexRepairDdl, +} from "../../../schemas/nextly-i18n-archive"; +import { buildCompanionTransitionStatements } from "../../i18n/migration/reconcile-companion"; +import { + companionHasStatusColumn, + localizedColumnsOnMain, +} from "../../i18n/runtime/companion-io"; +import { buildCompanionRuntimeTable } from "../../i18n/runtime/companion-registration"; +import { isIdempotencyError } from "../../schema/pipeline/sql-statement-utils"; + +/** Everything the companion reconcile needs about the save that triggered it. */ +export interface ReconcileSingleCompanionArgs { + slug: string; + tableName: string; + oldFields: FieldDefinition[]; + newFields: FieldDefinition[]; + /** Localization state AFTER this save (requested). */ + localized: boolean; + /** Localization state BEFORE this save (persisted). Drives enable/disable detection. */ + wasLocalized: boolean; + status: boolean; + /** + * Whether the single had Draft/Published BEFORE this apply. + * + * Separate from `status` because the disable restore asks a different question: not what the + * single is being saved as, but whether main carried `status` and the companion `_status` + * beforehand — a copy from columns that were not there fails the whole migration. + */ + wasStatus: boolean; + adapter: DrizzleAdapter; +} + +export async function reconcileSingleCompanion( + args: ReconcileSingleCompanionArgs +): Promise { + const { + slug, + tableName, + oldFields, + newFields, + localized, + status, + wasStatus, + adapter, + } = args; + const wasLocalized = args.wasLocalized; + // Nothing to do when the single was and remains non-localized. + if (!wasLocalized && !localized) return; + + const dialect = adapter.dialect; + const companionTable = `${tableName}_locales`; + const companionExists = await adapter.tableExists(companionTable); + // Only introspect `_status` when it can matter: an existing companion that stays localized + // (a later Draft/Published toggle must ADD/DROP `_status`). + const companionHasStatus = + companionExists && wasLocalized && localized + ? await companionHasStatusColumn(adapter, companionTable) + : undefined; + + // The seed (enable) and restore (disable) copy the default-locale value to/from the companion; + // read the configured default locale (falls back to "en" when localization isn't configured). + const defaultLocale = getConfigFromDI()?.localization?.defaultLocale ?? "en"; + + const plan = buildCompanionTransitionStatements({ + // The companion mirrors the main table, and a single's table comes from the same builder as a collection's. + builtBy: "collection" as const, + slug, + tableName, + dialect, + defaultLocale, + status, + wasLocalized, + isLocalized: localized, + oldFields, + newFields, + companionExists, + companionHasStatus, + wasStatus, + // Which translatable columns the main table still carries. A disable must not re-add one that + // is already there, and must still restore it: presence says the column exists, never that its + // value is current, because every localized write went to the companion alone. + existingMainColumns: await localizedColumnsOnMain( + adapter, + tableName, + oldFields + ).then(cols => cols.map(c => c.name)), + }); + + // A disable archives non-default translations, so ensure `nextly_i18n_archive` exists first + // (Builder entities have no `nextly migrate` step to provision it). Idempotent. + if (plan.needsArchive) { + for (const stmt of getI18nArchiveDdl(dialect)) { + await adapter.executeQuery(stmt); + } + // MySQL's table DDL cannot restore an index the table is missing, and + // index-only drift produces no reconcile operations, so the repair runs + // here. Tolerated rather than checked first: attempting it and accepting + // "duplicate key name" is one round trip instead of two, and the same + // tolerance the schema executor already applies. + const indexRepair = getI18nArchiveIndexRepairDdl(dialect); + if (indexRepair) { + try { + await adapter.executeQuery(indexRepair); + } catch (err) { + if (!isIdempotencyError(err)) throw err; + } + } + } + // 🔴 STRICT on purpose: the companion does NOT tolerate a re-run. + // + // Tolerating it would make a half-finished localization ENABLE look like success. Interrupted + // after the companion is created but before the seed and the main-column drops, the retry is + // indistinguishable from an orphan repair — the planner cannot tell them apart, because the + // signal that separates them (`existingMainColumns`) is consumed only on the disable path — so + // the duplicate columns would be swallowed and the entity recorded as localized while its + // default-locale content is still stranded on the main table. + // + // A loud failure is the worse experience and the safer outcome. It costs the repair of a + // localized entity whose create half-failed; it prevents silently reporting a migration that did + // not happen. + for (const stmt of plan.statements) { + await adapter.executeQuery(stmt); + } + + // The transition record describes a companion that no longer exists, so it stops being true the + // moment the disable succeeds. Left behind, it would refuse the next enable's real source locale + // — the check that protects a live transition would block a legitimate one instead. + if (plan.companionDropped) { + // The other half of "this companion is gone": readiness remembers only that one exists. + const { forgetCompanionReadiness } = await import( + "../../i18n/runtime/companion-readiness" + ); + forgetCompanionReadiness(adapter, `${tableName}_locales`); + const { resolveTransitionStore } = await import( + "../../i18n/migration/transition-recorder" + ); + const { forgetI18nTransition } = await import( + "../../i18n/migration/transition-state" + ); + await forgetI18nTransition( + await resolveTransitionStore(adapter), + "single", + slug + ); + } + + // Register the companion runtime table (best-effort — next boot re-registers it). Skipped when + // the plan dropped the companion (disable) or the single is no longer localized. + if (!plan.companionDropped && localized) { + try { + const companion = buildCompanionRuntimeTable({ + slug, + tableName, + fields: newFields, + dialect, + localized: true, + status, + }); + if (companion) { + getSchemaRegistryFromDI()?.registerDynamicSchema( + companion.companionTableName, + companion.table + ); + } + } catch (err) { + const msg = err instanceof Error ? err.message : String(err); + console.warn( + `[reconcileSingleCompanion] Companion runtime registration failed for '${slug}': ${msg}.` + ); + } + } +} diff --git a/packages/nextly/src/domains/singles/services/single-metadata-service.ts b/packages/nextly/src/domains/singles/services/single-metadata-service.ts new file mode 100644 index 000000000..99b8e711b --- /dev/null +++ b/packages/nextly/src/domains/singles/services/single-metadata-service.ts @@ -0,0 +1,439 @@ +/** + * Schema changes for a Single, owned in one place with the registry write they belong to. + * + * ## Why this exists + * + * A Single's table used to be created by the request handler: the handler generated the DDL, ran + * it, and then wrote the registry row. That split is why a lock cannot cover the pair — a lock + * taken inside the registry service is acquired after the tables have already changed. Collections + * already avoid this by owning both halves in one method; this is the same shape for Singles. + * + * ## What it guarantees, and what it does not + * + * **Recoverable and idempotent. NOT atomic.** That is not a compromise, it is the only honest + * claim: MySQL commits DDL implicitly, so a table change and a row write cannot be made atomic + * there by any ordering or any transaction. The migration engine reached the same conclusion and + * says so in `field-groups/migration/steps.ts` — "sequenced with repair rather than atomic, and + * every half idempotent to make that repair possible". Promising atomicity would be a promise that + * silently does not hold on one of the three supported databases. + * + * So the guarantee is: whatever happens, the database is left in a state that can be described and + * finished. + * + * ## How that is achieved: the intent is written first + * + * The row is persisted as `pending` BEFORE the table is touched, and confirmed afterwards. The + * order matters more than it looks: + * + * - Written last (the previous behaviour) a crash between the DDL and the row leaves a table that + * nothing has any record of — an orphan findable only by guessing at table names. + * - Written first, every interrupted operation leaves a durable row saying what was being + * attempted, and recovery is a query rather than an inference. + * + * That query already exists: `SingleRegistryService.getPendingMigrations()`. It had no callers + * before this service, because nothing ever left a row in `pending`. + * + * The ordering also moves the registry's own rejections (a taken slug, a name reserved by a global + * resource) to BEFORE the DDL rather than after it. Those checks used to run once the table had + * already been created. + * + * 🔴 Everything else that can REJECT a create has to run before `createSingle` is called, or a + * rejected request strands a `pending` row. Field validation, the reserved-slug check and the + * table-name conflict check all belong to the caller and all happen first. + */ + +import type { DrizzleAdapter } from "@nextlyhq/adapter-drizzle"; + +import { NextlyError } from "../../../errors"; +import type { FieldDefinition } from "../../../schemas/dynamic-collections"; +import type { + DynamicSingleInsert, + DynamicSingleRecord, + SingleMigrationStatus, +} from "../../../schemas/dynamic-singles/types"; +import type { Logger } from "../../../shared/types"; +import type { TableSpec } from "../../schema/pipeline/diff/types"; +import { applyMigrationStatements } from "../../schema/services/apply-migration-statements"; +import { shapeMismatches } from "../../schema/services/verify-applied-shape"; + +import type { SingleRegistryService } from "./single-registry-service"; + +/** + * 🔴 The schema generators, the runtime-schema builder and the companion reconcile are loaded on + * demand, NOT at the top of this file. + * + * This service is registered in the DI container, and the registration module is imported during + * boot by anything that touches the container. A static import here would pull the whole schema and + * i18n machinery into that graph for every consumer, including every process that never creates a + * Single. `di/register.ts` avoids exactly this with 37 `await import()` calls covering these same + * three modules, and a static import from a registration module quietly undoes that work — measured + * at +41% on the package's own test suite, enough to push its slowest files past their timeout. + * + * The cost of loading them here is paid once, on a path that is already writing DDL to a database. + */ + +/** + * The registry row to create, minus the one field this service owns. + * + * Deliberately the registry's own insert type rather than a hand-listed subset: a bespoke input + * shape silently drops whatever it forgets, and the fields most easily forgotten here (version + * retention, revalidation, webhook recording) are the ones whose absence is invisible until a + * user notices a switch reading as off. + */ +export type CreateSingleInput = Omit; + +/** What the caller gets back: the row, and how far the schema change actually got. */ +export interface CreateSingleResult { + record: DynamicSingleRecord; + migrationStatus: SingleMigrationStatus; +} + +/** + * The rendered DDL plus what the table must look like once it has run. + * + * Produced before anything is persisted, so the generator's own validation rejects a bad request + * while there is still nothing to clean up. + */ +interface CreateDdlPlan { + migrationSQL: string; + /** What the main table must look like afterwards, columns and types both. */ + desiredTable: TableSpec; + fields: FieldDefinition[]; + isLocalized: boolean; + hasStatus: boolean; +} + +/** + * The resolver the adapter uses to answer table lookups. + * + * Registering a freshly created table with it is what lets the very next read resolve without a + * server restart. Declared structurally because the adapter holds it as a protected member; this + * names the one method used rather than reaching in untyped. + */ +interface DynamicSchemaResolver { + registerDynamicSchema?: (name: string, table: unknown) => void; +} + +export class SingleMetadataService { + constructor( + private readonly registry: SingleRegistryService, + private readonly logger: Logger, + /** + * Optional on purpose, and it changes what this service does rather than whether it works. + * + * With no adapter registered the statements are generated and never run — the behaviour the + * request handler had before this service existed. Demanding a connection here would turn a + * configuration this product supports into a crash. + */ + private readonly adapter?: DrizzleAdapter + ) {} + + /** + * The dialect the DDL is generated for. + * + * Read from the adapter that will RUN the statements, never from the schema service's own + * default. `DB_DIALECT` is optional and falls back to `postgresql`, so an app configured with + * only a MySQL or SQLite URL would otherwise have its table created as PostgreSQL. + */ + private get dialect(): "postgresql" | "mysql" | "sqlite" | undefined { + return this.adapter?.getCapabilities().dialect; + } + + /** + * Create a Single's table and its registry row. + * + * The caller has already validated the input and established that no other Single owns this + * table name. Rejecting after this point would leave a `pending` row behind. + */ + async createSingle(input: CreateSingleInput): Promise { + // 0. PLAN, and deliberately BEFORE the intent write. + // + // 🔴 The generator is a validator as well as a renderer: a required relationship declaring + // `onDelete: "set null"` is refused there, because no database can null a reference the column + // forbids. Generating after the row was written would let that rejection strand a `pending` + // Single with its permissions seeded, and the corrected retry would then collide with the slug + // it had just created. Nothing is persisted until this has succeeded. + const plan = await this.planCreate(input); + + // 1. INTENT. Durable before anything is touched, so an interruption from here on leaves a row + // that `getPendingMigrations()` can find and finish. + // + // 🔴 Which is only worth writing if something can finish it. An unfinished attempt owns the + // slug, so without this the write-ahead row is not a recovery aid but a permanent blocker: the + // retry is refused as a duplicate and the user has no way forward short of editing the registry + // by hand. That would be strictly worse than the orphan table intent-first exists to prevent — + // an orphan at least left the slug free. + const record = await this.adoptOrRegister(input); + + // 2. APPLY. Idempotent, so it can run over whatever the interrupted attempt managed to create. + const migrationStatus = await this.applyCreateDdl(input, plan); + + // 3. CONFIRM. Recorded against the row written in step 1. + await this.registry.updateMigrationStatus(input.slug, migrationStatus); + + // The row as it now stands. `migrationStatus` is the only field of it a caller reads that the + // confirm write changed, so it is carried over rather than re-fetched; returning the step-1 + // copy unchanged would report every applied create as still pending. + return { record: { ...record, migrationStatus }, migrationStatus }; + } + + /** + * Write the intent, taking over an unfinished attempt at the same Single rather than colliding + * with it. + * + * A row that is not `applied` describes an operation that never reported success, so a create + * naming the same slug is a retry of it. The row is re-stated from THIS request — a corrected + * retry usually differs from the attempt that failed, and confirming the old field set while + * applying the new DDL would leave the registry describing a table nobody asked for. + * + * An `applied` Single is left alone: that is a genuine duplicate and belongs to whoever holds it. + */ + private async adoptOrRegister( + input: CreateSingleInput + ): Promise { + const existing = await this.registry.getSingleBySlug(input.slug); + if (!existing || existing.migrationStatus === "applied") { + return this.registry.registerSingle({ + ...input, + migrationStatus: "pending", + }); + } + + // 🔴 Only `failed` is adopted, and only by CLAIMING it, which are two separate points. + // + // WHICH state: `failed` is a FINISHED attempt — it recorded its own outcome, so nothing is + // still running against this slug. `pending` cannot say that; it is equally the state of a + // create in flight right now, and taking that over would overwrite its row with a second + // payload while its DDL is still building the first schema. + // + // HOW: reading `failed` and then writing is not enough. Two retries can both read it before + // either writes, and both would then run DDL against one slug. The claim puts the status in the + // WHERE clause so the database picks the winner — and it cannot go through `updateSingle`, + // which writes `migration_status` only when the field hash or status flag changes and so would + // leave the commonest retry of all, the same payload again, looking unclaimed for the whole + // time its DDL was running. + // + // Serialising `pending` as well is the migration lock this relocation exists to unblock. This + // claim is not a substitute for it; it is the narrower guarantee available without one. + if (existing.migrationStatus !== "failed") { + throw NextlyError.conflict({ + logContext: { + reason: "single-create-in-flight", + slug: input.slug, + migrationStatus: existing.migrationStatus, + hint: "A create for this slug has not recorded an outcome yet. Retry once it has.", + }, + }); + } + + if (!(await this.registry.claimFailedForRetry(input.slug))) { + throw NextlyError.conflict({ + logContext: { + reason: "single-create-claimed-elsewhere", + slug: input.slug, + hint: "Another retry took over this failed create first.", + }, + }); + } + + this.logger.info(`[Singles] Resuming a failed create for "${input.slug}"`); + return this.registry.updateSingle(input.slug, { + ...input, + migrationStatus: "pending", + }); + } + + /** + * Render the DDL and work out what the table must look like once it has run. + * + * Separated from the apply because the two have opposite contracts: this one is allowed to + * REJECT the request and must do so before anything is persisted, while the apply must never + * throw so a failure is still recorded against a row. + */ + private async planCreate(input: CreateSingleInput): Promise { + const isLocalized = input.localized === true; + const hasStatus = input.status === true; + const fields = input.fields as unknown as FieldDefinition[]; + const { DynamicCollectionSchemaService } = await import( + "../../dynamic-collections/services/dynamic-collection-schema-service" + ); + const schemaService = new DynamicCollectionSchemaService( + undefined, + this.dialect + ); + + const migrationSQL = schemaService.generateMigrationSQL( + input.tableName, + fields, + // i18n: translatable columns are omitted from the main table when localized — they live in + // the companion `
_locales`, provisioned separately. `isSingle` skips the slug column + // and adds `updated_at`; `hasStatus` adds the column the runtime schema expects when the user + // opted into Draft/Published. + { isSingle: true, hasStatus, localized: isLocalized } + ); + + // What the main table must look like afterwards, so "applied" can be checked rather than + // assumed. Built by the same builder the schema diff compares against, which is what makes it + // account for the system lifecycle columns (they come from OPTIONS, not from the field list), + // omit translatable columns (they belong to the companion), and resolve a widthless text field + // the way the Schema Builder's own creator does. + const { buildDesiredTableFromFields } = await import( + "../../schema/pipeline/diff/build-from-fields" + ); + const desiredTable = buildDesiredTableFromFields( + input.tableName, + fields, + this.dialect ?? "postgresql", + { hasStatus, localized: isLocalized, builtBy: "collection" } + ); + + return { migrationSQL, desiredTable, fields, isLocalized, hasStatus }; + } + + /** + * Run the create DDL, reporting how far it got. + * + * Never throws: a schema change that fails is recorded rather than raised, so the caller still + * has a row describing what was attempted. That is the same choice the request handler made + * before this service existed, and it is what makes the state repairable instead of lost. + */ + private async applyCreateDdl( + input: CreateSingleInput, + plan: CreateDdlPlan + ): Promise { + const { migrationSQL, fields, isLocalized, hasStatus } = plan; + const adapter = this.adapter; + if (!adapter) { + this.logger.warn( + "[Singles] No adapter registered, migration not executed" + ); + return "pending"; + } + + try { + // The shared runner, not a private copy: it owns both the splitting rule and the tolerance + // that makes re-running over half-applied schema the repair case rather than a dead end. + await applyMigrationStatements(adapter, migrationSQL); + } catch (error) { + this.logger.error( + `[Singles] Migration execution failed for "${input.tableName}": ${ + error instanceof Error ? error.message : String(error) + }` + ); + return "failed"; + } + + // Observed, not assumed. "Applied" has to mean the table is there. + if (!(await adapter.tableExists(input.tableName))) { + this.logger.error( + `[Singles] Table "${input.tableName}" was not created after migration` + ); + return "failed"; + } + + // 🔴 And that it is the table that was asked for, columns AND types. + // + // `CREATE TABLE IF NOT EXISTS` is a no-op against a table that already exists, on every + // dialect, and the index statements that follow are tolerated as already-applied. So a repair + // run over an ORPHANED table left by an earlier create — one whose registry row is gone, which + // is exactly the state this service exists to make recoverable — emits no error at all even + // when the field set, the Draft/Published option or a field's TYPE has changed in between. + // Existence alone would then record a schema the database does not have. + const mismatches = await shapeMismatches( + adapter, + adapter.getCapabilities().dialect, + input.tableName, + plan.desiredTable + ); + if (mismatches.length > 0) { + this.logger.error( + `[Singles] Table "${input.tableName}" does not match this schema: ${mismatches.join( + "; " + )}` + ); + return "failed"; + } + + await this.registerRuntimeSchema( + input, + fields, + adapter, + hasStatus, + isLocalized + ); + + // The companion is the other half of a localized single's storage, so failing to provision it + // leaves translatable values with nowhere to live. Reported as a failed migration rather than + // thrown: the main table exists and the row describes it, which is what makes a retry possible. + try { + const { reconcileSingleCompanion } = await import( + "./reconcile-single-companion" + ); + await reconcileSingleCompanion({ + slug: input.slug, + tableName: input.tableName, + oldFields: [], + newFields: fields, + localized: isLocalized, + // A brand-new single was never localized before, so a localized create is a create-only + // companion (no seed/drop) rather than an enable transition. + wasLocalized: false, + // A single being created has no prior state at all. + wasStatus: false, + status: hasStatus, + adapter, + }); + } catch (error) { + this.logger.error( + `[Singles] Companion provisioning failed for "${input.tableName}": ${ + error instanceof Error ? error.message : String(error) + }` + ); + return "failed"; + } + + return "applied"; + } + + /** + * Bind the new table to the running server so the next read resolves it. + * + * Best-effort by design: the registry is rebuilt from the database on the next boot, so a + * failure here costs a restart rather than the table. Taken from the adapter that ran the DDL + * rather than from the container, because that adapter is the one whose reads have to resolve + * the name and a caller may hold one the container has never seen. + */ + private async registerRuntimeSchema( + input: CreateSingleInput, + fields: FieldDefinition[], + adapter: DrizzleAdapter, + hasStatus: boolean, + isLocalized: boolean + ): Promise { + try { + const { generateRuntimeSchema } = await import( + "../../schema/services/runtime-schema-generator" + ); + const { table } = generateRuntimeSchema( + input.tableName, + fields, + adapter.getCapabilities().dialect, + // i18n: the main runtime table omits translatable columns for a localized single, matching + // the DDL above. + { status: hasStatus, localized: isLocalized } + ); + const resolver = ( + adapter as unknown as { tableResolver?: DynamicSchemaResolver } + ).tableResolver; + if (resolver && typeof resolver.registerDynamicSchema === "function") { + resolver.registerDynamicSchema(input.tableName, table); + } + } catch (error) { + this.logger.warn( + `[Singles] Runtime schema registration failed for "${input.tableName}": ${ + error instanceof Error ? error.message : String(error) + }` + ); + } + } +} diff --git a/packages/nextly/src/domains/singles/services/single-registry-service.ts b/packages/nextly/src/domains/singles/services/single-registry-service.ts index 2ad143627..265767d70 100644 --- a/packages/nextly/src/domains/singles/services/single-registry-service.ts +++ b/packages/nextly/src/domains/singles/services/single-registry-service.ts @@ -25,6 +25,8 @@ * @since 1.0.0 */ +import { randomUUID } from "node:crypto"; + import type { DrizzleAdapter } from "@nextlyhq/adapter-drizzle"; import type { TransactionContext } from "@nextlyhq/adapter-drizzle/types"; @@ -305,6 +307,53 @@ export class SingleRegistryService extends BaseRegistryService< return this.getRecordsWithPendingMigrations(); } + /** + * Take ownership of a Single whose last schema change FAILED, so exactly one retry can proceed. + * + * 🔴 A compare-and-swap, not a read followed by a write. Two retries can both observe `failed` + * before either writes, and both would then run their own DDL against one slug while each + * believes it owns the row. The database decides instead: the status is part of the WHERE, so the + * update matches for the first caller and matches nothing for the second. + * + * `updateSingle` cannot serve this. It writes `migration_status` only when the field hash or the + * status flag changes, so the commonest retry of all — the same payload again — would leave the + * row saying `failed` for the whole time the DDL was running, and every concurrent retry would + * see an unclaimed row. + * + * @returns true when this caller claimed it, false when someone else already had. + */ + async claimFailedForRetry(slug: string): Promise { + // 🔴 A compare-and-swap that does not depend on RETURNING or on an affected-row count. + // + // Counting returned rows is wrong on MySQL, which has no RETURNING: the adapter emulates it by + // re-reading with the same predicate, and this update has just falsified that predicate, so a + // successful claim reports zero rows on the one dialect it succeeded on. + // + // So the claim leaves a mark instead. Only one caller's update can match while the status is + // still `failed`; every other one matches nothing and cannot have written its token. Reading + // the row back therefore names the winner on every dialect, with no driver-specific plumbing. + const token = randomUUID(); + await this.adapter.update( + await this.resolveRegistryTableName(), + { + migration_status: "pending", + last_migration_id: token, + updated_at: this.formatDateForDb(), + }, + { + and: [ + { column: "slug", op: "=", value: slug }, + // The WHERE side addresses the Drizzle property; the data side writes the physical + // column, which is the split `updateRecordMigrationStatus` already uses. + { column: "migrationStatus", op: "=", value: "failed" }, + ], + } + ); + + const claimed = await this.getSingleBySlug(slug); + return claimed?.lastMigrationId === token; + } + // ============================================================ // Single-Specific: Registration // ============================================================