From dafa3ca5e33d813de02d0022167b28c9f330f3f1 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 27 Jul 2026 14:14:05 +0000 Subject: [PATCH] feat(objectql)!: media value shapes enforce per deployment, once its file migration verified (#3438) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #3438 scheduled the strict flip for "a later minor once telemetry is quiet" — our telemetry, deciding for their deployments. #3617 built the mechanism that replaces that: a deployment's own migration flag. This lands the half that flag actually covers. A malformed media value now rejects with `invalid_type` instead of warning, but only where `os migrate files-to-references --apply` has run and passed its self-check. The legacy forms this rejects — inline `{url, name, …}` blobs, bare URLs — are exactly what that migration converts, so a verified deployment has been shown to hold none, while an unmigrated one is untouched rather than having every media update start failing on upgrade. ## Only media, deliberately #3438 packaged three things as one flip, and the flag covers one of them. It asserts "this deployment's FILE values are migrated and reconciled". It says nothing about whether a `lookup` id or a `location` payload is well formed, and nothing at all about D2's action parameters. Gating those on it would be borrowing evidence for a fact it does not cover — an authority answering a question it was not asked, which is the same shape as the read-resolver defect #3617 fixed one layer down. Reference and structured-JSON classes keep their own warn-first rollout until something can vouch for them. `OS_DATA_VALUE_SHAPE_STRICT_ENABLED` is unchanged (still opts every class in, still forces media strict on an unmigrated deployment). New escape hatch `OS_ALLOW_LAX_MEDIA_VALUES=1` re-opens media on a verified deployment; when both are set the lenient one wins, because a warning nobody reads costs less than an app that stops writing. ## Cost The fact lives in the database while the validator is synchronous and per-write, so the engine reads it once and memoizes — and stays dormant unless the written object declares a media field, mirroring the rule the storage module already states. One query per process for apps that store files, zero for those that do not. Every way of not knowing (no storage service, no row, unreadable table, malformed row) answers "not verified": a deployment whose evidence cannot be read keeps writing rather than starting to reject. Verified end-to-end on showcase + SQLite against one database: the same write was accepted before the migration, rejected with `invalid_type` after it, and accepted again under OS_ALLOW_LAX_MEDIA_VALUES. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01V9uWWyKq6pNPQthwCXL8ma --- ...adr-0104-d1-media-strict-per-deployment.md | 42 ++++++ content/docs/deployment/cli.mdx | 21 ++- ...0104-field-runtime-value-shape-contract.md | 30 +++- .../commands/migrate/files-to-references.ts | 4 +- packages/objectql/src/engine.test.ts | 129 ++++++++++++++++++ packages/objectql/src/engine.ts | 119 +++++++++++++++- .../src/validation/record-validator.test.ts | 90 ++++++++++++ .../src/validation/record-validator.ts | 92 +++++++++++-- 8 files changed, 504 insertions(+), 23 deletions(-) create mode 100644 .changeset/adr-0104-d1-media-strict-per-deployment.md diff --git a/.changeset/adr-0104-d1-media-strict-per-deployment.md b/.changeset/adr-0104-d1-media-strict-per-deployment.md new file mode 100644 index 000000000..e43c1f579 --- /dev/null +++ b/.changeset/adr-0104-d1-media-strict-per-deployment.md @@ -0,0 +1,42 @@ +--- +"@objectstack/objectql": minor +--- + +feat(objectql)!: media value shapes enforce once THIS deployment has verified its file migration (#3438 D1 media half, gated by #3617) + +A `file` / `image` / `avatar` / `video` / `audio` value that does not match the +stored contract (an opaque `sys_file` id) now **rejects with `invalid_type`** +instead of warning — but only on a deployment that has run +`os migrate files-to-references --apply` and passed its self-check. + +**Why this is not a version-wide flip.** The legacy media values this rejects — +inline `{url, name, …}` blobs, bare URLs — are exactly what that migration +converts. A deployment that has run it has been *shown* to hold none; a +deployment that has not would have every media-field update start failing the +moment it upgraded. So the enforcement follows the evidence, per deployment, +rather than the release. Nothing changes for a deployment until it migrates. + +**Upgrading:** + +```bash +os migrate files-to-references # dry run: reports what would convert +os migrate files-to-references --apply # convert, verify, record the flag +``` + +If a write starts failing after you migrate, the value genuinely does not match +the contract — the error names the field. `OS_ALLOW_LAX_MEDIA_VALUES=1` re-opens +media leniency while you diagnose. + +**Scope — deliberately only media.** `OS_DATA_VALUE_SHAPE_STRICT_ENABLED` is +unchanged and still opts every class into strict (and still forces media strict +on a deployment that has not migrated). Reference types (`lookup`, `user`, …) +and structured JSON (`location`, `address`, `repeater`, …) stay warn-first: the +file migration is evidence about file values and says nothing about whether a +`location` is well formed, so gating them on its flag would be borrowing +evidence for a fact it does not cover. They flip when something can vouch for +them — see #3438. + +**Cost.** Dormant unless the written object declares a media field, and the +flag read is memoized, so this is one query per process for apps that store +files and zero for those that do not. A running server picks up a +newly-recorded migration on restart, or via `engine.invalidateDataMigrationFlags()`. diff --git a/content/docs/deployment/cli.mdx b/content/docs/deployment/cli.mdx index 0cb1d7525..79ad3ab17 100644 --- a/content/docs/deployment/cli.mdx +++ b/content/docs/deployment/cli.mdx @@ -545,18 +545,31 @@ field instead. The run then reconciles what records actually hold against what `sys_file` records as each file's owner. Zero blocking discrepancies is what records the -flag — and that flag, not the version number, is what later enables features -that act irreversibly on migrated data (reclaiming the bytes of released files; -rejecting rather than warning about legacy media values). Never running it is -safe: files are simply retained forever. +flag — and that flag, not the version number, is what enables behaviour that +depends on the data actually being migrated. Never running it is safe: files +are simply retained forever, and media values keep warning instead of failing. Exit status is `0` only when the self-check passes, so CI can gate on it. +#### What the flag turns on + +| Once verified | Effect | +| :--- | :--- | +| **Media value shapes** | A malformed `file` / `image` / `avatar` / `video` / `audio` value is **rejected** (`400 invalid_type`) instead of warned about. Set `OS_ALLOW_LAX_MEDIA_VALUES=1` to re-open leniency while diagnosing. | +| **Released-file collection** | Not yet shipped — the flag is the gate it will read. | + +Other value classes are unaffected: a `lookup` or `location` value keeps its own +warn-first rollout, because this migration is evidence about *file* values and +says nothing about theirs. + A dry run writes **nothing** — not the conversions, and not the flag either, even when the self-check would pass. `--apply` is the only writing mode. A later run that *fails* its self-check clears the flag's verified state, so a database that has drifted closes its own gate. + +A running server reads the flag once; after migrating, **restart it** for +enforcement to take effect. ### Scaffolding diff --git a/docs/adr/0104-field-runtime-value-shape-contract.md b/docs/adr/0104-field-runtime-value-shape-contract.md index 3f6f3ea1f..e424177a5 100644 --- a/docs/adr/0104-field-runtime-value-shape-contract.md +++ b/docs/adr/0104-field-runtime-value-shape-contract.md @@ -419,8 +419,12 @@ Therefore the target end-state is **two enforcement points, each with one job**: - **Build/validate time → hard reject.** Net-new metadata (the AI's output) must fail loudly at authoring. This is the primary defence against AI error. - **Runtime → warn-first, then flip.** Deployed data keeps working through the - warn window; the flip to strict-by-default (tracked in #3438) closes it once - telemetry is quiet. + warn window; the flip to strict-by-default (tracked in #3438) closes it. + *(Amended by the second 2026-07-27 addendum: "once telemetry is quiet" was + our telemetry deciding for their deployments. The **media** half now flips + per deployment, when that deployment's own file-as-reference migration has + verified. The reference and structured-JSON halves still await an evidence + source of their own — the file migration does not vouch for them.)* This refines D2 (today runtime-only) and reshapes the #3438 flip: the priority is adding the **build-time** rejection for value shapes and action params, not @@ -670,6 +674,28 @@ a deployment when that deployment has completed and verified its migration — not when a version number arrives. One flag, not two gates that can disagree: they are gating on the same fact. +**Amendment while implementing this (2026-07-27, later still).** "#3438 reads +the flag" was too broad: #3438 is three things, and the flag only covers one. +The flag asserts *this deployment's file-field values have been migrated and +reconciled*. That is evidence about **media** value shapes and nothing else — +it says nothing about whether a `lookup` id or a `location` payload is well +formed, and nothing at all about D2's action parameters. Gating those on it +would be borrowing evidence for a fact it does not cover, which is the same +error one layer down: an authority answering a question it was not asked. + +So the split is: + +| | evidence | status | +|---|---|---| +| D1 media (`file`/`image`/`avatar`/`video`/`audio`) | the `adr-0104-file-references` flag | flips per deployment | +| D1 references + structured JSON | none yet — needs its own | stays warn-first | +| D2 action params | unrelated to any data migration | stays warn-first | + +The last two are not blocked on the mechanism — it exists and works. They are +blocked on someone deciding what would constitute evidence that a deployment's +`location` values are sound, which is a different question from the one this +addendum answers. + ### What cannot be automated, and does not block Whether an **external URL** (`https://cdn…`) should become an explicit `url` diff --git a/packages/cli/src/commands/migrate/files-to-references.ts b/packages/cli/src/commands/migrate/files-to-references.ts index 568a01ee8..cade4777b 100644 --- a/packages/cli/src/commands/migrate/files-to-references.ts +++ b/packages/cli/src/commands/migrate/files-to-references.ts @@ -253,7 +253,9 @@ export default class MigrateFilesToReferences extends Command { if (apply) { printSuccess( 'Self-check passed — deployment flag recorded (adr-0104-file-references). ' + - 'This deployment is now eligible for strict media enforcement and, once shipped, file collection.', + 'Media value shapes are now ENFORCED on this deployment: a malformed ' + + 'file/image value is rejected rather than warned about. ' + + '(Set OS_ALLOW_LAX_MEDIA_VALUES=1 to re-open leniency while diagnosing.)', ); } else if (result.backfill.converted > 0) { printInfo( diff --git a/packages/objectql/src/engine.test.ts b/packages/objectql/src/engine.test.ts index 4d05da81d..5373955dc 100644 --- a/packages/objectql/src/engine.test.ts +++ b/packages/objectql/src/engine.test.ts @@ -2075,3 +2075,132 @@ describe('ObjectQL Engine', () => { }); }); }); + +/** + * #3617 / #3438 — the engine carries the deployment's own migration evidence + * into the (synchronous, per-write) value-shape validator. + * + * The fact lives in `sys_migration`, so it needs a database read; the write + * path needs it on every insert/update. These prove the read happens once, + * and that every way of NOT knowing lands on lenient — a deployment whose + * evidence cannot be read keeps writing rather than starting to reject. + */ +describe('ObjectQL — file-as-reference migration flag (#3617)', () => { + let engine: ObjectQL; + let driver: IDataDriver; + + const verifiedRow = { + id: 'adr-0104-file-references', + last_run_at: '2026-07-27T00:00:00.000Z', + verified_at: '2026-07-27T00:00:00.000Z', + blocking: 0, + }; + + beforeEach(() => { + vi.clearAllMocks(); + driver = { + name: 'default-driver', + connect: vi.fn().mockResolvedValue(undefined), + disconnect: vi.fn().mockResolvedValue(undefined), + find: vi.fn().mockResolvedValue([]), + findOne: vi.fn(), + create: vi.fn().mockResolvedValue({ id: '1' }), + update: vi.fn().mockResolvedValue({ id: '1' }), + delete: vi.fn(), + count: vi.fn(), + capabilities: {} as any, + } as unknown as IDataDriver; + engine = new ObjectQL(); + engine.registerDriver(driver, true); + }); + + /** `doc` holds a legacy inline blob — exactly what the migration converts. */ + const withMediaObject = () => { + vi.mocked(SchemaRegistry.getObject).mockImplementation((name: string) => { + if (name === 'note') return { name: 'note', fields: { doc: { type: 'file' } } } as any; + if (name === 'sys_migration') return { name: 'sys_migration', fields: { id: { type: 'text' } } } as any; + return undefined; + }); + }; + const legacyBlob = { doc: { url: 'https://cdn/f.pdf', name: 'f.pdf' } }; + + it('a verified flag makes a malformed media value reject', async () => { + withMediaObject(); + vi.mocked(driver.find).mockResolvedValue([verifiedRow] as any); + + await expect(engine.insert('note', { ...legacyBlob })).rejects.toThrow(/invalid file value/i); + }); + + it('no flag row → lenient (the write goes through)', async () => { + withMediaObject(); + vi.mocked(driver.find).mockResolvedValue([]); + + await expect(engine.insert('note', { ...legacyBlob })).resolves.toBeDefined(); + }); + + it('a failed run (verified_at cleared) → lenient', async () => { + withMediaObject(); + vi.mocked(driver.find).mockResolvedValue([{ ...verifiedRow, verified_at: null, blocking: 3 }] as any); + + await expect(engine.insert('note', { ...legacyBlob })).resolves.toBeDefined(); + }); + + it('an unreadable sys_migration → lenient, and the write is not failed by the probe', async () => { + withMediaObject(); + vi.mocked(driver.find).mockRejectedValue(new Error('no such table: sys_migration')); + + await expect(engine.insert('note', { ...legacyBlob })).resolves.toBeDefined(); + }); + + /** + * Dormancy, mirroring the storage module's own rule: an object with no + * file-class field can hold no malformed media value, so it must not pay + * even one query to learn that. Nearly every object is in this case. + */ + it('costs no query for an object that declares no media field', async () => { + vi.mocked(SchemaRegistry.getObject).mockImplementation((name: string) => { + if (name === 'invoice') return { name: 'invoice', fields: { amount: { type: 'number' } } } as any; + if (name === 'sys_migration') return { name: 'sys_migration', fields: { id: { type: 'text' } } } as any; + return undefined; + }); + vi.mocked(driver.find).mockResolvedValue([verifiedRow] as any); + + await expect(engine.insert('invoice', { amount: 10 })).resolves.toBeDefined(); + expect(vi.mocked(driver.find).mock.calls.filter((c) => c[0] === 'sys_migration')).toHaveLength(0); + }); + + /** No storage service → no sys_migration object → not even a query. */ + it('costs no query on a kernel without the storage objects', async () => { + vi.mocked(SchemaRegistry.getObject).mockImplementation((name: string) => + name === 'note' ? ({ name: 'note', fields: { doc: { type: 'file' } } } as any) : undefined, + ); + vi.mocked(driver.find).mockResolvedValue([]); + + await expect(engine.insert('note', { ...legacyBlob })).resolves.toBeDefined(); + expect(driver.find).not.toHaveBeenCalled(); + }); + + it('reads the flag once per process, not once per write', async () => { + withMediaObject(); + vi.mocked(driver.find).mockResolvedValue([verifiedRow] as any); + + await expect(engine.insert('note', { doc: 'file_01' })).resolves.toBeDefined(); + await expect(engine.insert('note', { doc: 'file_02' })).resolves.toBeDefined(); + await expect(engine.insert('note', { doc: 'file_03' })).resolves.toBeDefined(); + + const flagReads = vi.mocked(driver.find).mock.calls.filter((c) => c[0] === 'sys_migration'); + expect(flagReads).toHaveLength(1); + }); + + it('invalidateDataMigrationFlags forces a re-read', async () => { + withMediaObject(); + vi.mocked(driver.find).mockResolvedValue([verifiedRow] as any); + + await engine.insert('note', { doc: 'file_01' }); + engine.invalidateDataMigrationFlags(); + await engine.insert('note', { doc: 'file_02' }); + + const flagReads = vi.mocked(driver.find).mock.calls.filter((c) => c[0] === 'sys_migration'); + expect(flagReads).toHaveLength(2); + }); +}); diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index 01d62a82b..23bad4c13 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -13,6 +13,11 @@ import { } from '@objectstack/spec/data'; import type { WriteObservabilityOptions } from '@objectstack/spec/contracts'; import { parseAutonumberFormat, renderAutonumber, missingFieldValues, isTenancyDisabled, FILE_REFERENCE_TYPES, isFileIdToken, RAW_FILE_VALUES_CONTEXT_KEY } from '@objectstack/spec/data'; +import { + DATA_MIGRATION_FLAG_OBJECT, + FILE_REFERENCES_MIGRATION_ID, + isDataMigrationFlagVerified, +} from '@objectstack/spec/system'; import { ExecutionContext, ExecutionContextSchema } from '@objectstack/spec/kernel'; import { IDataDriver, IDataEngine, Logger, createLogger, withTransientRetry, type RetryOptions } from '@objectstack/core'; import { SummaryRecomputeError, type SummaryRecomputeFailure } from './summary-errors.js'; @@ -1878,6 +1883,96 @@ export class ObjectQL implements IDataEngine { this.logger.info('ObjectQL engine initialization complete'); } + /** + * Does this object declare a media field at all? Cached per schema object — + * the registry hands back the same instance per object, so this scans a + * field map once rather than on every write. + */ + private objectHasMediaField(objectSchema: any): boolean { + if (!objectSchema?.fields) return false; + const cached = ObjectQL.mediaFieldPresence.get(objectSchema); + if (cached !== undefined) return cached; + const present = Object.values(objectSchema.fields).some( + (def: any) => def && FILE_REFERENCE_TYPES.has(def.type), + ); + ObjectQL.mediaFieldPresence.set(objectSchema, present); + return present; + } + private static readonly mediaFieldPresence = new WeakMap(); + + /** + * The media value-shape verdict for a write against `objectSchema`. + * + * Dormant by design, mirroring the storage module's own rule: an object with + * no file-class field can hold no malformed media value, so it must not pay + * even one query to learn that. Only objects that actually declare media + * consult the deployment flag — and that read is itself memoized, so the + * whole mechanism costs one query per process, and zero for an app that + * stores no files. + */ + private async mediaValueShapeStrictFor(objectSchema: any): Promise { + if (!this.objectHasMediaField(objectSchema)) return false; + return this.isFileReferencesMigrationVerified(); + } + + /** + * Has this deployment completed AND verified the ADR-0104 file-as-reference + * migration (#3617)? Read once per process from the `sys_migration` flag. + * + * Every way of not knowing answers `false` — no storage service (so no + * `sys_migration` object registered), no row, an unreadable table, a + * malformed row. That is the same posture the flag's other consumer takes: + * enforcement derives from evidence, and absent evidence is not permission. + * Here "no" means media value shapes keep warning instead of rejecting, so + * a deployment that cannot be asked keeps writing. + * + * Costs nothing on a kernel without the storage objects: the registry lookup + * short-circuits before any query. + */ + private async isFileReferencesMigrationVerified(): Promise { + if (!this.fileReferencesMigrationVerified) { + this.fileReferencesMigrationVerified = (async () => { + if (!this._registry.getObject(DATA_MIGRATION_FLAG_OBJECT)) return false; + try { + const rows = await this.find(DATA_MIGRATION_FLAG_OBJECT, { + where: { id: FILE_REFERENCES_MIGRATION_ID }, + limit: 1, + context: { isSystem: true } as ExecutionContext, + }); + const row: any = rows?.[0]; + if (!row || row.id !== FILE_REFERENCES_MIGRATION_ID) return false; + const verified = isDataMigrationFlagVerified({ + id: FILE_REFERENCES_MIGRATION_ID, + last_run_at: String(row.last_run_at ?? ''), + verified_at: row.verified_at == null ? null : String(row.verified_at), + // A non-numeric count must read as "not zero", not as 0 — a bad + // coercion lands on NaN, which fails the === 0 test. + blocking: typeof row.blocking === 'number' ? row.blocking : Number(row.blocking ?? Number.NaN), + }); + if (verified) { + this.logger.info( + '[value-shape] this deployment has verified the file-as-reference migration — ' + + 'media value shapes are enforced (ADR-0104 D1 / #3617)', + ); + } + return verified; + } catch { + return false; // unreadable evidence → stay lenient + } + })(); + } + return this.fileReferencesMigrationVerified; + } + + /** + * Drop the memoized deployment migration flags so the next write re-reads + * them. For a host that runs a data migration in-process and wants its + * effect without a restart. + */ + invalidateDataMigrationFlags(): void { + this.fileReferencesMigrationVerified = null; + } + async destroy() { this.logger.info('Destroying ObjectQL engine', { driverCount: this.drivers.size }); @@ -1903,6 +1998,20 @@ export class ObjectQL implements IDataEngine { * lazily seeded from the current max in the store. */ private readonly autonumberCounters = new Map(); + /** + * Memoized answer to "has THIS deployment completed and verified the + * ADR-0104 file-as-reference migration?" (#3617) — the fact that decides + * whether a malformed media value rejects or merely warns. + * + * Memoized as a PROMISE so concurrent first writes share one read rather + * than racing several. Deliberately not refreshed on a timer: the flag is + * written by `os migrate files-to-references --apply`, a deliberate + * operator action, and a process that has not seen it yet simply stays + * lenient — the safe direction. A host that migrates in-process can call + * {@link invalidateDataMigrationFlags} instead of waiting for a restart. + */ + private fileReferencesMigrationVerified: Promise | null = null; + /** Lazily-built index: child object name → roll-up summary descriptors on * parent objects that aggregate it. Invalidated when packages register. */ private summaryIndex: Map | null = null; @@ -2618,11 +2727,14 @@ export class ObjectQL implements IDataEngine { rowErrors[i] = e; } } + // Resolved once for the whole batch; dormant unless the object declares + // a media field, and memoized after the first object that does. + const mediaValueShapeStrict = await this.mediaValueShapeStrictFor(schemaForValidation); for (let i = 0; i < rows.length; i++) { if (rowErrors[i] !== undefined) continue; try { normalizeMultiValueFields(schemaForValidation, rows[i]); - validateRecord(schemaForValidation, rows[i], 'insert'); + validateRecord(schemaForValidation, rows[i], 'insert', { mediaValueShapeStrict }); evaluateValidationRules(schemaForValidation as any, rows[i], 'insert', { logger: this.logger, currentUser: this.buildEvalUser(opCtx.context), skipStateMachine: shouldSkipStateMachine(opCtx.context) }); } catch (e) { if (!partialMode) throw e; @@ -2889,10 +3001,11 @@ export class ObjectQL implements IDataEngine { // which silently fails when `previous` is absent. let priorRecord: Record | null = null; const updateSchema = this._registry.getObject(object); + const mediaValueShapeStrict = await this.mediaValueShapeStrictFor(updateSchema); if (hookContext.input.id) { await this.encryptSecretFields(object, hookContext.input.data as Record, opCtx.context, hookContext.input.options); normalizeMultiValueFields(updateSchema, hookContext.input.data as Record); - validateRecord(updateSchema, hookContext.input.data as Record, 'update'); + validateRecord(updateSchema, hookContext.input.data as Record, 'update', { mediaValueShapeStrict }); if (needsPriorRecord(updateSchema as any) || (this.hooks.get('afterUpdate')?.length ?? 0) > 0) { const priorAst: QueryAST = { object, where: { id: hookContext.input.id }, limit: 1 }; priorRecord = await driver.findOne(object, priorAst, hookContext.input.options as any); @@ -2918,7 +3031,7 @@ export class ObjectQL implements IDataEngine { } else if (options?.multi && driver.updateMany) { await this.encryptSecretFields(object, hookContext.input.data as Record, opCtx.context, hookContext.input.options); normalizeMultiValueFields(updateSchema, hookContext.input.data as Record); - validateRecord(updateSchema, hookContext.input.data as Record, 'update'); + validateRecord(updateSchema, hookContext.input.data as Record, 'update', { mediaValueShapeStrict }); // [#2982] Consume the middleware-composed AST seeded above, so // the injected row-scoping (RLS write filter, sharing's // editable-rows filter) actually binds the driver operation. Fail diff --git a/packages/objectql/src/validation/record-validator.test.ts b/packages/objectql/src/validation/record-validator.test.ts index 7c86fbff3..be06898db 100644 --- a/packages/objectql/src/validation/record-validator.test.ts +++ b/packages/objectql/src/validation/record-validator.test.ts @@ -417,3 +417,93 @@ describe('validateRecord — ADR-0104 value shapes (warn-first / strict)', () => }); }); }); + +/** + * #3617 / #3438 — media value shapes enforce per DEPLOYMENT, not per release. + * + * A verified deployment has RUN the file-as-reference migration and had its + * ownership ledger reconciled, so it has been shown to hold no legacy media + * values. Nothing about that migration vouches for a `lookup` or `location` + * value, so those classes keep their own (unchanged) warn-first rollout — + * gating them on this flag would borrow evidence for a fact it does not cover. + */ +describe('validateRecord — media value shapes gate on the deployment flag (#3617)', () => { + const schema = { + fields: { + doc: { type: 'file' }, + cover: { type: 'image' }, + account: { type: 'lookup', reference: 'accounts' }, + geo: { type: 'location' }, + }, + }; + // A legacy inline blob — precisely what the migration converts. + const legacyMedia = { doc: { url: 'https://cdn/f.pdf', name: 'f.pdf' } }; + const malformedNonMedia = { geo: { latitude: 1, longitude: 2 } }; + + const strict = { mediaValueShapeStrict: true }; + + const withEnv = (key: string, fn: () => void) => { + process.env[key] = '1'; + try { fn(); } finally { delete process.env[key]; } + }; + + it('unverified deployment (the default): media stays warn-first', () => { + expect(() => validateRecord(schema, { ...legacyMedia }, 'update')).not.toThrow(); + expect(() => validateRecord(schema, { ...legacyMedia }, 'update', {})).not.toThrow(); + expect(() => + validateRecord(schema, { ...legacyMedia }, 'update', { mediaValueShapeStrict: false }), + ).not.toThrow(); + }); + + it('verified deployment: a malformed media value rejects with invalid_type', () => { + try { + validateRecord(schema, { ...legacyMedia }, 'update', strict); + expect.unreachable('expected ValidationError'); + } catch (e) { + expect(e).toBeInstanceOf(ValidationError); + const err = e as ValidationError; + expect(err.fields[0]?.field).toBe('doc'); + expect(err.fields[0]?.code).toBe('invalid_type'); + } + // and the conformant reference form still passes + expect(() => validateRecord(schema, { doc: 'file_01HXYZ', cover: 'file_02' }, 'update', strict)).not.toThrow(); + }); + + /** + * The whole reason for splitting #3438: a verified file migration is not + * evidence about lookup/location values, so it must not start rejecting them. + */ + it('verified deployment does NOT flip non-media classes', () => { + expect(() => validateRecord(schema, { ...malformedNonMedia }, 'update', strict)).not.toThrow(); + expect(() => validateRecord(schema, { account: { id: 'acc_1' } }, 'update', strict)).not.toThrow(); + }); + + it('the blanket opt-in still forces media strict on an unverified deployment', () => { + withEnv('OS_DATA_VALUE_SHAPE_STRICT_ENABLED', () => { + expect(() => validateRecord(schema, { ...legacyMedia }, 'update')).toThrow(ValidationError); + }); + }); + + it('OS_ALLOW_LAX_MEDIA_VALUES re-opens media on a verified deployment', () => { + withEnv('OS_ALLOW_LAX_MEDIA_VALUES', () => { + expect(() => validateRecord(schema, { ...legacyMedia }, 'update', strict)).not.toThrow(); + }); + }); + + /** + * A contradictory configuration lands on the lenient side: a warning nobody + * reads costs less than an app that stops writing. + */ + it('opt-out beats opt-in when both are set', () => { + withEnv('OS_DATA_VALUE_SHAPE_STRICT_ENABLED', () => { + withEnv('OS_ALLOW_LAX_MEDIA_VALUES', () => { + expect(() => validateRecord(schema, { ...legacyMedia }, 'update', strict)).not.toThrow(); + }); + }); + }); + + it('applies on insert too, not only update', () => { + expect(() => validateRecord(schema, { ...legacyMedia }, 'insert', strict)).toThrow(ValidationError); + expect(() => validateRecord(schema, { ...legacyMedia }, 'insert')).not.toThrow(); + }); +}); diff --git a/packages/objectql/src/validation/record-validator.ts b/packages/objectql/src/validation/record-validator.ts index 3bcec2de3..f63ae2f81 100644 --- a/packages/objectql/src/validation/record-validator.ts +++ b/packages/objectql/src/validation/record-validator.ts @@ -225,7 +225,13 @@ export function coerceBooleanFields>( return (copy ?? row) as T; } -function validateOne(name: string, def: FieldDef, value: unknown, skipRequired = false): FieldValidationError | null { +function validateOne( + name: string, + def: FieldDef, + value: unknown, + skipRequired = false, + mediaStrict = false, +): FieldValidationError | null { // ── required ──────────────────────────────────────────────────── // `autonumber` is runtime-owned: the value is generated by the engine / // driver (the SQL driver assigns it from a persistent sequence AFTER this @@ -341,23 +347,43 @@ function validateOne(name: string, def: FieldDef, value: unknown, skipRequired = } // ── previously-opaque types: value-shape contract (ADR-0104 D1) ───── - // Single-value references (id string), file-likes (id/url string or the - // pre-D3 inline metadata object), and structured JSON payloads - // (location/address/composite/repeater/record/vector) are checked against - // the spec's `valueSchemaFor`. Warn-first rollout (ADR-0104 R1/R2): a - // violation logs instead of rejecting, so legacy rows written under the - // lax regime don't strand their records; set - // `OS_DATA_VALUE_SHAPE_STRICT_ENABLED=1` to enforce as a 400. The flip to - // error-by-default rides a later minor once telemetry is quiet. + // Single-value references (id string), file-likes (a `sys_file` id), and + // structured JSON payloads (location/address/composite/repeater/record/ + // vector) are checked against the spec's `valueSchemaFor`. + // + // Enforcement differs by class, because the EVIDENCE for enforcing differs + // (#3617, splitting what #3438 originally packaged as one flip): + // + // - **Media** (`file`/`image`/`avatar`/`video`/`audio`) enforces as soon as + // THIS DEPLOYMENT has completed and self-check-verified its + // file-as-reference migration. Its legacy values — inline blobs, bare + // URLs — are exactly what that migration converts, so a verified + // deployment has been SHOWN to have none left. `mediaStrict` carries that + // per-deployment fact in (the engine reads the `sys_migration` flag); + // ADR-0104 R7 external URLs are the deliberate exception the migration + // reports and never converts, so a deployment still holding them fails + // its self-check and stays lenient here. + // - **References and structured JSON** stay warn-first: the file migration + // says nothing about whether a `lookup` or `location` value is well + // formed, so gating them on ITS flag would be borrowing evidence for a + // fact it does not cover. They flip when something can vouch for them. if (REFERENCE_VALUE_TYPES.has(t) || FILE_REFERENCE_TYPES.has(t) || STRUCTURED_JSON_TYPES.has(t)) { const parsed = shapeSchemaFor(def).safeParse(value); if (!parsed.success) { const detail = parsed.error.issues[0]?.message ?? 'invalid value shape'; const message = `${name} has an invalid ${t} value: ${detail}`; - if (VALUE_SHAPE_STRICT()) { + const isMedia = FILE_REFERENCE_TYPES.has(t); + if (isMedia ? mediaStrictEffective(mediaStrict) : VALUE_SHAPE_STRICT()) { return { field: name, code: 'invalid_type', message }; } - warnOnce(`${t}:${name}`, `[value-shape] ${message} — accepted for now (ADR-0104 warn-first; set OS_DATA_VALUE_SHAPE_STRICT_ENABLED=1 to enforce)`); + warnOnce( + `${t}:${name}`, + `[value-shape] ${message} — accepted for now (ADR-0104 warn-first; ` + + (isMedia + ? 'run `os migrate files-to-references --apply` to migrate this deployment and enforce' + : 'set OS_DATA_VALUE_SHAPE_STRICT_ENABLED=1 to enforce') + + ')', + ); } return null; } @@ -372,6 +398,29 @@ function VALUE_SHAPE_STRICT(): boolean { return typeof process !== 'undefined' && process.env?.OS_DATA_VALUE_SHAPE_STRICT_ENABLED === '1'; } +/** + * Escape hatch: keep media value shapes lenient even on a deployment whose + * migration verified. For an operator who hits an unforeseen rejection and + * needs their app writing again before diagnosing. + */ +function LAX_MEDIA_VALUES(): boolean { + return typeof process !== 'undefined' && process.env?.OS_ALLOW_LAX_MEDIA_VALUES === '1'; +} + +/** + * Does a media value-shape violation reject, for this deployment? + * + * Three inputs, and the precedence is chosen so a CONTRADICTORY configuration + * lands on the safe side: the opt-out wins over the opt-in, because the cost + * of wrongly staying lenient is a warning nobody reads, while the cost of + * wrongly enforcing is a working app that stops writing. + */ +function mediaStrictEffective(deploymentVerified: boolean): boolean { + if (LAX_MEDIA_VALUES()) return false; + if (VALUE_SHAPE_STRICT()) return true; + return deploymentVerified; +} + const warnedShapes = new Set(); function warnOnce(key: string, message: string): void { if (warnedShapes.has(key)) return; @@ -396,6 +445,21 @@ function shapeSchemaFor(def: FieldDef): ReturnType { return schema; } +export interface ValidateRecordOptions { + /** + * Has THIS DEPLOYMENT completed and self-check-verified the ADR-0104 + * file-as-reference migration (#3617)? When true, a malformed media value + * rejects instead of warning. + * + * Passed in rather than read here because the fact lives in the database + * (`sys_migration`) while this validator is synchronous and per-write — the + * engine reads it once and memoizes. Defaults to `false`: a caller that + * cannot say stays lenient, so nothing starts rejecting writes merely + * because the evidence was unavailable. + */ + mediaValueShapeStrict?: boolean; +} + /** * Validate a payload against a list of declared fields. `objectSchema` * comes from `ObjectQL.getRegistry().getObject(name)` and exposes a @@ -407,11 +471,13 @@ export function validateRecord( objectSchema: { fields?: Record } | undefined | null, data: Record | undefined | null, mode: Mode, + options: ValidateRecordOptions = {}, ): void { if (!objectSchema?.fields || !data) return; const errors: FieldValidationError[] = []; const fields = objectSchema.fields; + const mediaStrict = options.mediaValueShapeStrict === true; if (mode === 'insert') { // Walk all declared fields — required check applies even when @@ -419,7 +485,7 @@ export function validateRecord( for (const [name, def] of Object.entries(fields)) { if (SKIP_FIELDS.has(name)) continue; if (def.system || def.readonly) continue; - const err = validateOne(name, def, data[name]); + const err = validateOne(name, def, data[name], false, mediaStrict); if (err) errors.push(err); } } else { @@ -432,7 +498,7 @@ export function validateRecord( // skipRequired: PATCH-omitted fields must not 400. (No def clone — the // registry's own field object flows through so the ADR-0104 value-shape // schema cache, keyed on def identity, hits.) - const err = validateOne(name, def, value, true); + const err = validateOne(name, def, value, true, mediaStrict); if (err) errors.push(err); } }