diff --git a/.changeset/drift-multi-value-column-names-remedy-command.md b/.changeset/drift-multi-value-column-names-remedy-command.md new file mode 100644 index 0000000000..d1ca89503b --- /dev/null +++ b/.changeset/drift-multi-value-column-names-remedy-command.md @@ -0,0 +1,34 @@ +--- +'@objectstack/driver-sql': patch +--- + +The stale multi-value column warning now names `os migrate multi-value-columns`, +instead of telling operators ObjectStack will never fix the column + +The finding that reports a multi-value field left on a stale `varchar`/`text` +column opened its remedy with **"ObjectStack will NOT change this column for +you. Migrate it by hand"** and then printed raw SQL. That was true when it was +written and became false the moment `os migrate multi-value-columns` shipped: +there is now an operator-run command that does exactly this, with a dry run as +the default, a confirmation prompt, and a post-run re-detection that exits +non-zero if the finding has not cleared. Operators were being sent to hand-write +DDL on a production table while the safer route sat one command away, unnamed. + +The message now leads with the command and keeps the hand-run statement after it +for anyone without the CLI. Both surfaces an operator meets this on pick the +change up, because both print `message` verbatim: the boot warning +(`[schema-drift] …` on every restart) and `os migrate plan`. + +What has **not** changed is what the finding gates. It stays `severity: 'error'`, +`category: 'needs_confirm'` — the artifact boot gate refuses a boot on +`category === 'destructive'` and on nothing else, and every database this finding +describes is already serving, so making the report louder must never be the thing +that stops one from starting. No load-time or write-time refusal was added; the +platform still never migrates the column on its own, per the ruling that it warns +and ships an explicit operator-run migration rather than altering a customer's +production table unattended. + +The dialect-specific statement stays embedded **verbatim**, which is a contract +rather than formatting: a `ManagedDriftEntry` carries no dialect, so the CLI +command recovers one by testing which dialect's statement the message contains. +That coupling is now pinned from the emitting side as well as the consuming one. diff --git a/packages/drivers/driver-sql/src/schema-drift.base-type-mismatch.test.ts b/packages/drivers/driver-sql/src/schema-drift.base-type-mismatch.test.ts index 440bd93150..93553edd90 100644 --- a/packages/drivers/driver-sql/src/schema-drift.base-type-mismatch.test.ts +++ b/packages/drivers/driver-sql/src/schema-drift.base-type-mismatch.test.ts @@ -20,13 +20,22 @@ * RIGHT DIAGNOSIS, and the neighbouring shapes that must stay silent are pinned * as silent in the same breath. * - * ## The detection half only + * ## Detection, and the remedy it now points at * - * ObjectStack does NOT migrate the column. Whether it should is the other half - * of #11535 and a live maintainer decision — a migration over existing rows plus - * an index drop/rebuild is destructive and hard to roll back. This suite pins - * the reporting, and pins that the reporting changes no deployment's ability to - * boot (see the category case, which is not a tautology — read its comment). + * ObjectStack still does NOT migrate the column on its own. That is the ruling, + * not a gap: ruled C on #11700 (maintainer, 2026-08-24) — the platform warns and + * ships an explicit, operator-run migration, and never runs it at boot. + * Unattended auto-migration was rejected as the only route that alters a + * customer's production table with nobody watching. + * + * What changed since the detection half landed is that the migration now EXISTS: + * `os migrate multi-value-columns` (#11733). So the message stopped describing a + * problem and started naming the way out, and this suite pins the naming in both + * directions — the shapes that must carry the recommendation and the shapes that + * must not, including a live database re-booted after the repair. + * + * It also pins that none of this changes a deployment's ability to boot (see the + * category case, which is not a tautology — read its comment). * * ## Three dialects, and SQLite's absence is a MEASUREMENT * @@ -39,7 +48,13 @@ import { describe, it, expect, beforeAll, afterAll } from 'vitest'; import { SqlDriver } from './sql-driver.js'; -import { diffManagedTable, manualJsonConversionSql, type PhysicalColumn, type SqlDialectName } from './schema-drift.js'; +import { + diffManagedTable, + manualJsonConversionSql, + MULTI_VALUE_COLUMN_REMEDY_COMMAND, + type PhysicalColumn, + type SqlDialectName, +} from './schema-drift.js'; import { DIALECT_CELLS, declareDialectCell, type DialectCell } from './live-dialect-matrix.testkit.js'; const MATRIX = 'multi-value base-type drift'; @@ -108,6 +123,71 @@ describe('diffManagedTable — multi-value field over a stale textual column (#1 expect(entry.message).not.toContain('json_build_array'); }); + // ── the message NAMES the remedy command (#11535, remaining half) ──────── + // + // When the detection half landed there was no command to name, so the message + // handed the operator raw SQL and opened with "ObjectStack will NOT change + // this column for you". `os migrate multi-value-columns` (#11733) both + // falsified that sentence and gave the message something better to say. + + it('names `os migrate multi-value-columns`, and names it BEFORE the hand-run SQL', () => { + for (const dialect of ['postgres', 'mysql'] as const) { + const [entry] = diffTags({ type: 'lookup', multiple: true }, staleColumn('character varying', 255), dialect); + + expect(entry.message).toContain(MULTI_VALUE_COLUMN_REMEDY_COMMAND); + + // Order is the assertion, not decoration. Both routes repair the column; + // only one of them dry-runs first, prompts, and re-checks the finding + // afterwards. An operator who stops reading at the first `ALTER TABLE` + // they see should have already passed the command. + const commandAt = entry.message.indexOf(MULTI_VALUE_COLUMN_REMEDY_COMMAND); + const sqlAt = entry.message.indexOf(manualJsonConversionSql(dialect, 'proj_task', 'tags')); + expect(commandAt).toBeGreaterThanOrEqual(0); + expect(sqlAt).toBeGreaterThan(commandAt); + + // The dry run is the default and is worth a full sentence: an operator who + // reads "run this" on a production database needs to know it writes + // nothing until they ask again. + expect(entry.message).toMatch(/dry run/i); + expect(entry.message).toContain('--apply'); + expect(entry.message).toMatch(/backup/i); + } + }); + + it('no longer claims ObjectStack will not migrate the column — that became false when #11733 landed', () => { + // A regression guard on a specific false sentence, kept because the failure + // it describes is invisible: the message would still be loud, still name the + // right column, and still print working SQL, while telling the operator that + // the command two lines below it does not exist. + const [entry] = diffTags({ type: 'lookup', multiple: true }, staleColumn('character varying', 255), 'postgres'); + expect(entry.message).not.toMatch(/will NOT change this column/i); + + // What IS still true, and must stay said: nothing migrates the column + // unattended. Ruled C on #11700 — the platform warns and ships an + // operator-run command; it never runs it at boot. + expect(entry.message).toMatch(/never migrates this column on its own/i); + }); + + it('keeps the statement VERBATIM, because the CLI recovers the dialect by containment', () => { + // ⚠️ Cross-package contract, pinned from the emitting side. A + // `ManagedDriftEntry` carries no dialect, so `planStaleColumnTargets` + // (packages/cli/.../migrate/multi-value-columns.ts) identifies one by asking + // which dialect's statement the MESSAGE contains. A reword that paraphrases + // the SQL, wraps it, or breaks it across a line makes every finding + // `remedy_not_recognized` — the command this message now points at would + // refuse to run, and nothing in driver-sql's own suite would notice. + // This reproduces that probe rather than describing it. + const probe = (message: string) => + (['postgres', 'mysql'] as const).filter((d) => message.includes(manualJsonConversionSql(d, 'proj_task', 'tags'))); + + for (const dialect of ['postgres', 'mysql'] as const) { + const [entry] = diffTags({ type: 'lookup', multiple: true }, staleColumn('character varying', 255), dialect); + // Exactly one — a message matching both would make the probe's answer + // depend on array order. + expect(probe(entry.message)).toEqual([dialect]); + } + }); + // ── the shapes that must stay SILENT ───────────────────────────────────── it('says nothing when the column is already `json` — the healthy database', () => { @@ -131,6 +211,42 @@ describe('diffManagedTable — multi-value field over a stale textual column (#1 expect(diffTags({ type: 'datetime', multiple: true }, staleColumn('timestamp with time zone'), 'postgres')).toEqual([]); }); + it('the remedy command is named by THIS finding and by nothing else', () => { + // The other half of the non-vacuity pair. `os migrate multi-value-columns` + // converts a column to `json`; a message that recommended it for a healthy + // column, a single-value field, or a plain width difference would be + // pointing an operator at a type change nothing here asked for. A signal + // that fires on everything reads exactly as green as one that fires + // correctly, so the shapes that must NOT carry it are enumerated. + const mustNotName: Array<[string, Parameters[0], PhysicalColumn[], SqlDialectName]> = [ + // already migrated — the repair has been done + ['migrated json column', { type: 'lookup', multiple: true }, staleColumn('json'), 'postgres'], + // never multi-value — the column is right and always was + ['single-value field', { type: 'string' }, staleColumn('character varying', 255), 'postgres'], + // a real finding, but a WIDTH one: `os migrate apply` handles it + ['single-value width drift', { type: 'string', maxLength: 50 }, staleColumn('character varying', 255), 'postgres'], + ['single-value width widen', { type: 'string', maxLength: 500 }, staleColumn('character varying', 255), 'postgres'], + // dialects/types where the stale column corrupts nothing + ['sqlite', { type: 'lookup', multiple: true }, staleColumn('varchar', 255), 'sqlite'], + ['stale integer column', { type: 'integer', multiple: true }, staleColumn('integer'), 'postgres'], + ]; + + for (const [label, field, columns, dialect] of mustNotName) { + const out = diffTags(field, columns, dialect); + for (const entry of out) { + expect(entry.message, `${label} must not recommend the column-type migration`) + .not.toContain(MULTI_VALUE_COLUMN_REMEDY_COMMAND); + expect(entry.op.type, label).not.toBe('manual_column_type_change'); + } + } + + // And the fixture is not vacuous in the other direction: two of those rows + // DO produce a finding, so the loop above is reading real messages rather + // than passing over empty arrays. + expect(diffTags({ type: 'string', maxLength: 50 }, staleColumn('character varying', 255), 'postgres')).toHaveLength(1); + expect(diffTags({ type: 'string', maxLength: 500 }, staleColumn('character varying', 255), 'postgres')).toHaveLength(1); + }); + it('leaves the single-value varchar-width branch (#11431) exactly where it was', () => { // The guard added for multi-value fields must not have cost the neighbouring // branch its reach — a fix that silences the thing next to it is a @@ -188,6 +304,25 @@ const singleValueMeta = [{ name: TABLE, fields: { name: { type: 'string' }, tags const multiValueMeta = [{ name: TABLE, fields: { name: { type: 'string' }, tags: { type: 'string', multiple: true } } }]; class DriftProbeDriver extends SqlDriver { + /** + * Every line the boot path logged — the operator's ACTUAL view. + * + * `detectManagedDrift()` returns objects; what an operator meets on a restart + * is `reconcileAndWarnDrift` putting `d.message` through the logger. Asserting + * only on the returned object would leave the delivery unpinned, which is the + * half this card is about: the finding was already correct, and still told the + * operator to go write SQL by hand. + */ + public logged: string[] = []; + + constructor(config: ConstructorParameters[0]) { + super(config); + (this as unknown as { logger: { warn: (m: string) => void; error: (m: string) => void } }).logger = { + warn: (m: string) => this.logged.push(m), + error: (m: string) => this.logged.push(m), + }; + } + columnsOf(table: string) { return this.introspectColumns(table); } @@ -199,6 +334,8 @@ function declareBaseTypeDriftSuite(cell: DialectCell): void { let driver: DriftProbeDriver; let physicalType: string; let readBack: unknown; + /** Exactly what the boot in step 2 logged — snapshotted before anything else runs. */ + let bootLines: string[] = []; beforeAll(async () => { driver = new DriftProbeDriver(cell.config()); @@ -211,7 +348,9 @@ function declareBaseTypeDriftSuite(cell: DialectCell): void { // 2. the metadata change + reboot. `initObjects` is additive-only: nothing // is missing, so nothing is added, and the column is never revisited. + driver.logged = []; await driver.initObjects(multiValueMeta as any); + bootLines = [...driver.logged]; physicalType = (await driver.columnsOf(TABLE)).find((c) => c.name === 'tags')!.type; @@ -278,6 +417,38 @@ function declareBaseTypeDriftSuite(cell: DialectCell): void { expect(found[0].category).toBe('needs_confirm'); }); + it(corrupts + ? 'the BOOT tells the operator to run `os migrate multi-value-columns`' + : 'the BOOT says nothing at all, so no operator is sent to migrate a healthy column', () => { + // The delivery, not the detection. This is the line a restart actually + // prints — `reconcileAndWarnDrift` handing `d.message` to the logger — + // captured from the real boot in step 2 rather than reconstructed. + const driftLines = bootLines.filter((l) => l.includes('[schema-drift]')); + + if (!corrupts) { + // SQLite: the value round-trips as a real array (pinned above), so a + // recommendation to convert the column would send an operator to alter + // a database that has nothing wrong with it. + expect(driftLines.filter((l) => l.includes(MULTI_VALUE_COLUMN_REMEDY_COMMAND))).toEqual([]); + return; + } + + const named = driftLines.filter((l) => l.includes(MULTI_VALUE_COLUMN_REMEDY_COMMAND)); + expect(named).toHaveLength(1); + + // One line has to carry the whole diagnosis AND the way out: an operator + // reading a boot log is not going to go find the source. + expect(named[0]).toContain(`${TABLE}.tags`); + expect(named[0]).toContain(physicalType); + expect(named[0]).toMatch(/dry run/i); + expect(named[0]).toContain('--apply'); + + // And the statement survived the trip through the logger intact — this is + // the string the CLI matches on to recover the dialect. + const dialect = cell.id === 'pg' ? 'postgres' : 'mysql'; + expect(named[0]).toContain(manualJsonConversionSql(dialect, TABLE, 'tags')); + }); + it.skipIf(!corrupts)('the remedy the finding prints actually works, and clears the finding', async () => { // An operator-facing remedy nobody runs is a remedy that drifts into being // wrong. This runs the emitted statement verbatim against the live server, @@ -301,6 +472,31 @@ function declareBaseTypeDriftSuite(cell: DialectCell): void { const after = await driver.detectManagedDrift(); expect(after.filter((d) => d.op.type === 'manual_column_type_change')).toEqual([]); + // …and so is the BOOT LINE. The negative direction on a live database, and + // the one an operator actually experiences: having run the command the + // message recommended, the next restart must stop recommending it. A + // signal that keeps firing after the repair trains operators to ignore it, + // which costs exactly as much as never firing. + // + // ⚠️ A SECOND DRIVER, not another `initObjects` on this one. `driftWarned` + // is a per-instance throttle keyed by `driftKey(d)` — the same instance + // stays silent on its second boot whether or not the drift is still there, + // so re-booting `driver` would assert nothing at all. A fresh instance is + // what a restart actually is. + const rebooted = new DriftProbeDriver(cell.config()); + try { + await rebooted.connect(); + await rebooted.initObjects(multiValueMeta as any); + expect(rebooted.logged.filter((l) => l.includes(MULTI_VALUE_COLUMN_REMEDY_COMMAND))).toEqual([]); + } finally { + await rebooted.disconnect().catch(() => {}); + } + + // Non-vacuity: a fresh instance booting the SAME metadata against the + // stale column did name it (`bootLines`, step 2 above), so the silence + // belongs to the repair rather than to a fixture that stopped booting. + expect(bootLines.filter((l) => l.includes(MULTI_VALUE_COLUMN_REMEDY_COMMAND))).toHaveLength(1); + // And the data is in the shape the declaration promises, for every row // state: the corrupted array is an array again, a legacy single value has // become a one-element array, and NULL/'' stay empty rather than becoming diff --git a/packages/drivers/driver-sql/src/schema-drift.ts b/packages/drivers/driver-sql/src/schema-drift.ts index 4d47c83ebd..00e154b8c7 100644 --- a/packages/drivers/driver-sql/src/schema-drift.ts +++ b/packages/drivers/driver-sql/src/schema-drift.ts @@ -474,6 +474,33 @@ function multiValueColumnTypeIsLoadBearing(dialect: SqlDialectName): boolean { return dialect === 'postgres' || dialect === 'mysql'; } +/** + * The operator-run command that repairs a stale multi-value column (#11535). + * + * Named in the finding's `message` because a message that only DESCRIBES a + * problem leaves the operator to invent the repair. Until `os migrate + * multi-value-columns` shipped (#11733) there was nothing else to say, so the + * message told them to run raw SQL by hand — and the sentence it opened with, + * "ObjectStack will NOT change this column for you", became false the moment + * that command landed. The hand-run route is still printed below it, because + * the statement is what the command runs and an operator without the CLI still + * needs it; it is no longer the FIRST thing offered. + * + * ⛔ Module-exported so this package's own suites can pin the spelling, and + * deliberately NOT added to `index.ts`: naming a CLI command in a warning is a + * string, and a published export is the step that would let the CLI import its + * own id back out of the driver it boots. `schema-drift.base-type-mismatch.test.ts` + * asserts the emitted message contains it; the command id itself lives in + * `packages/cli/src/commands/migrate/multi-value-columns.ts`. + * + * ⚠️ It changes NOTHING about how loud this finding is or what it gates: + * `severity: 'error'` and `category: 'needs_confirm'` are untouched, so the + * finding still refuses no boot (see the emission site's comment — the boot + * gate reads CATEGORY, and `destructive` is the value that would stop an + * already-serving database from starting). + */ +export const MULTI_VALUE_COLUMN_REMEDY_COMMAND = 'os migrate multi-value-columns'; + /** * The hand-run statement that converts a stale textual column to `json`, * spelled for the dialect the operator is actually on. @@ -674,9 +701,34 @@ export function diffManagedTable(args: { // // The residue is stated rather than hidden: `os migrate apply` hands a // `needs_confirm` entry to the reconciler, which — having no arm for this op - // by design — declines it (`applied=0, skipped=1`) and says so. A finding - // that is reported every time and applied never is exactly the contract - // while the automatic migration remains the maintainer's open decision. + // by design — declines it (`applied=0, skipped=1`) and says so. + // + // ## The message NAMES the remedy, because there is now a remedy to name + // + // Ruled C on #11700 (maintainer, 2026-08-24): the platform warns and ships + // an explicit, operator-run migration, and never runs it at boot. + // ⛔ Quoted verbatim, not translated: + // 「11700 11693 不需要考虑历史数据，其他按照你的建议继续」 + // + // That command is `os migrate multi-value-columns` (#11733, landed + // `0e5bea6`), which is what changes this message's job. Before it existed + // the finding could only DESCRIBE the problem and hand over raw SQL; it + // opened its remedy with "ObjectStack will NOT change this column for you", + // a sentence the command falsified. It now names the command first — the + // route with a dry run, a confirmation prompt, and a post-run re-detection + // that exits non-zero if the finding has not cleared — and keeps the + // hand-run statement after it for an operator without the CLI. + // + // ⚠️ The raw statement stays in the message VERBATIM, and that is a + // contract, not prose: `planStaleColumnTargets` + // (packages/cli/src/commands/migrate/multi-value-columns.ts) recovers the + // DIALECT by testing `message.includes(manualJsonConversionSql(d, …))` for + // each corrupting dialect — a `ManagedDriftEntry` carries no dialect of its + // own. Reword this message so the statement no longer appears character for + // character and the command refuses every finding with + // `remedy_not_recognized`, i.e. the remedy this text points at stops + // working. Pinned from this side by the `toContain(manualJsonConversionSql(…))` + // cases in `schema-drift.base-type-mismatch.test.ts`. const declaresJsonColumn = field.multiple === true; if (declaresJsonColumn && multiValueColumnTypeIsLoadBearing(dialect) && acceptsStringifiedJson(col.type)) { out.push({ @@ -694,12 +746,16 @@ export function diffManagedTable(args: { `column is \`${col.type}\` — the database was created while the field was single-value and the ` + `additive sync never migrates a column's type. Arrays are being written as the STRINGIFIED ` + `literal (e.g. '["a","b"]') and read back as a string, so anything consuming the value ` + - `receives one opaque id instead of a list (#11535). ObjectStack will NOT change this column ` + - `for you. Migrate it by hand, in a transaction, with a backup taken first — dropping any ` + - `index on the column first, since a json column cannot carry a plain btree: ` + + `receives one opaque id instead of a list (#11535). REMEDY: run ` + + `"${MULTI_VALUE_COLUMN_REMEDY_COMMAND}" — it is a DRY RUN by default that executes nothing ` + + `and prints the statements; take a backup, then re-run it with --apply. ObjectStack never ` + + `migrates this column on its own: the boot path only reports it and "os migrate apply" ` + + `skips it, so nothing changes until you run that command. To do it by hand instead, in a ` + + `transaction and with a backup taken first — dropping any index on the column first, since ` + + `a json column cannot carry a plain btree: ` + `${manualJsonConversionSql(dialect, table, fieldName)} ` + `Rows written while the column was stale may already hold a stringified array in a RELATED ` + - `single-value column; those are not repaired by the statement above.`, + `single-value column; neither route repairs those.`, }); }