diff --git a/.changeset/adr-0104-gate-announcement-tells-the-truth.md b/.changeset/adr-0104-gate-announcement-tells-the-truth.md new file mode 100644 index 0000000000..d797226b40 --- /dev/null +++ b/.changeset/adr-0104-gate-announcement-tells-the-truth.md @@ -0,0 +1,40 @@ +--- +"@objectstack/objectql": patch +--- + +fix(objectql): the boot gate announcement stops firing where it is false, and stops counting fields nothing enforces (#3438) + +The startup line that names an open value-shape gate (#4253) fired on every +deployment there is, and said something untrue on any deployment that had +already settled the question with an environment switch. Both are the same +failure — an advisory that speaks where it does not apply is how readers learn +to ignore it — and neither is reachable from the suite that shipped with it, +because `engine.test.ts` mocks the registry away and a mocked registry hands +the engine exactly the fields the test wrote. + +`objectHasCoveredValueField` — the dormancy short-circuit that is supposed to +spare an object with no covered field the flag query — tested raw type +membership, while the real registry INJECTS covered-type fields into every +object it registers: `organization_id` and `owner_id` (both `system`), +`created_by` and `updated_by` (both in `SKIP_FIELDS`), four `lookup`s. +`validateRecord` skips every one of them before it reaches the value-shape +check, so the short-circuit answered `true` for literally every object, never +fired, and its WeakMap memoized a constant. Counting is now by the validator's +own `isScannableValueShapeField`, the predicate the scanner already imports — +three readings of "a covered field" drifting by one clause is how a gate ends +up governing fields nothing enforces. + +The announcement also consulted no environment switch, while both postures it +reports on short-circuit ahead of the deployment flag. Under +`OS_DATA_VALUE_SHAPE_STRICT_ENABLED` enforcement is already on for both +classes, so "checked but NOT enforced here" was simply false; under either +opt-out the operator chose leniency deliberately, so naming a migration that +cannot change what they get is noise. Each gate now consults its own pair +(`mediaPostureSetByEnv` / `valueShapePostureSetByEnv`, siblings for the reason +`mediaStrictEffective` and `valueShapeStrictEffective` are siblings), since the +opt-outs are per-class while the opt-in opens both. Cheapest test first, so a +kernel with nothing to say still reaches no flag query. + +Enforcement is unchanged: same gates, same flags, same default. The flag read +was already memoized per process, so what this corrects is the property +ADR-0104 states, not a user-visible cost. diff --git a/content/docs/releases/v17.mdx b/content/docs/releases/v17.mdx index 9ed5d1bb51..69e490a1a1 100644 --- a/content/docs/releases/v17.mdx +++ b/content/docs/releases/v17.mdx @@ -1025,6 +1025,21 @@ can correct — so it reports the object, field, type, count, sample record ids and parse issue, and you re-run until it is clean. `OS_ALLOW_LAX_VALUE_SHAPES=1` re-opens leniency while diagnosing. +**You are told about both, rather than having to find this page.** `os migrate +meta` — the command a 16→17 upgrade already runs — ends by naming the two data +migrations and the gate each records, including on a run that rewrote nothing, +since canonical metadata says nothing about stored values. A deployment that +boots holding covered fields with no verified gate row logs one line naming the +command that ends warn mode. Neither reads as "done": until a gate is recorded, +the classes it covers keep warning. + +That boot line is scoped to the deployments it can actually help. It counts only +fields a write would check — not the `lookup`s the registry injects into every +object — and says nothing once an environment switch has already settled the +posture, since `OS_DATA_VALUE_SHAPE_STRICT_ENABLED` enforces both classes +outright and either `OS_ALLOW_LAX_*` opt-out is a deliberate choice the scan +would not change. + **A database created by 17 attests both flags at creation** (#3438), so a new deployment enforces from its first boot instead of waiting for someone to run a migration that, for an empty store, does nothing. The platform attests only a diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index 425c00fa8a..e8880c780d 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -12,7 +12,7 @@ import { type DroppedFieldsEvent } from '@objectstack/spec/data'; import type { WriteObservabilityOptions } from '@objectstack/spec/contracts'; -import { parseAutonumberFormat, renderAutonumber, missingFieldValues, isTenancyDisabled, FILE_REFERENCE_TYPES, REFERENCE_VALUE_TYPES, STRUCTURED_JSON_TYPES, isFileIdToken, RAW_FILE_VALUES_CONTEXT_KEY } from '@objectstack/spec/data'; +import { parseAutonumberFormat, renderAutonumber, missingFieldValues, isTenancyDisabled, FILE_REFERENCE_TYPES, REFERENCE_VALUE_TYPES, isFileIdToken, RAW_FILE_VALUES_CONTEXT_KEY } from '@objectstack/spec/data'; import { DATA_MIGRATION_FLAG_OBJECT, FILE_REFERENCES_MIGRATION_ID, @@ -69,7 +69,7 @@ import { ExpressionEngine } from '@objectstack/formula'; import type { Expression } from '@objectstack/spec'; import { isAggregatedViewContainer, expandViewContainer } from '@objectstack/spec'; import { bindHooksToEngine } from './hook-binder.js'; -import { validateRecord, normalizeMultiValueFields, coerceBooleanFields, ValidationError } from './validation/record-validator.js'; +import { validateRecord, normalizeMultiValueFields, coerceBooleanFields, ValidationError, valueShapePostureSetByEnv, mediaPostureSetByEnv, isScannableValueShapeField } from './validation/record-validator.js'; import { evaluateValidationRules, needsPriorRecord, stripReadonlyWhenFields, stripReadonlyWhenFieldsMulti, hasReadonlyWhenInPayload, stripReadonlyFields } from './validation/rule-validator.js'; import { applyInMemoryAggregation } from './in-memory-aggregation.js'; @@ -2149,17 +2149,28 @@ export class ObjectQL implements IDataEngine { } /** - * Does this object declare a reference or structured-JSON field? Same - * per-schema cache and same dormancy rule as {@link objectHasMediaField}: an - * object holding none of these types can hold no violation of them, so it - * must not pay a query to learn that. + * Does this object declare a reference or structured-JSON field that a write + * would actually check? Same per-schema cache and same dormancy rule as + * {@link objectHasMediaField}: an object holding none can hold no violation + * of them, so it must not pay a query to learn that. + * + * The membership test is `isScannableValueShapeField` — the validator's own + * — and not raw type membership, because the registry INJECTS covered-type + * fields into every object it registers: `organization_id` and `owner_id` + * (both `system`), plus `created_by` / `updated_by` (both in `SKIP_FIELDS`), + * are all `lookup`s. `validateRecord` skips every one of them before it ever + * reaches the value-shape check, so counting them made this answer `true` for + * literally every object — the dormancy rule above never fired, and this + * cache memoized a constant. Same predicate as the scanner for the same + * reason the scanner imports it: three readings of "a covered field" drifting + * by one clause is how a gate ends up governing fields nothing enforces. */ private objectHasCoveredValueField(objectSchema: any): boolean { if (!objectSchema?.fields) return false; const cached = ObjectQL.coveredValueFieldPresence.get(objectSchema); if (cached !== undefined) return cached; - const present = Object.values(objectSchema.fields).some( - (def: any) => def && (REFERENCE_VALUE_TYPES.has(def.type) || STRUCTURED_JSON_TYPES.has(def.type)), + const present = Object.entries(objectSchema.fields).some( + ([name, def]: [string, any]) => isScannableValueShapeField(name, def), ); ObjectQL.coveredValueFieldPresence.set(objectSchema, present); return present; @@ -2282,6 +2293,15 @@ export class ObjectQL implements IDataEngine { * own metadata says the gate is about something it stores; the verified case * already logs from the flag read. * + * A gate whose posture an environment switch has already settled says + * nothing either, because the flag this line reports on is not what decides + * that deployment's posture: under `OS_DATA_VALUE_SHAPE_STRICT_ENABLED` + * enforcement is already on, so "checked but NOT enforced here" is simply + * false, and under either opt-out the operator chose leniency deliberately, + * so naming a migration that would not change what they get is noise. Each + * gate consults its own pair, since the opt-outs are per-class. Cheapest + * test first: a kernel with nothing to say never reaches the flag query. + * * Costs one flag read per applicable gate (both memoized, and both already * paid by the first write to such an object), and nothing at all for an app * that declares neither class of field. @@ -2290,12 +2310,16 @@ export class ObjectQL implements IDataEngine { if (this.migrationGatesAnnounced) return; this.migrationGatesAnnounced = true; try { + const mediaByEnv = mediaPostureSetByEnv(); + const coveredByEnv = valueShapePostureSetByEnv(); + if (mediaByEnv && coveredByEnv) return; + let media = false; let covered = false; for (const obj of this._registry.getAllObjects() as any[]) { - if (!media && this.objectHasMediaField(obj)) media = true; - if (!covered && this.objectHasCoveredValueField(obj)) covered = true; - if (media && covered) break; + if (!media && !mediaByEnv && this.objectHasMediaField(obj)) media = true; + if (!covered && !coveredByEnv && this.objectHasCoveredValueField(obj)) covered = true; + if ((media || mediaByEnv) && (covered || coveredByEnv)) break; } if (media && !(await this.isFileReferencesMigrationVerified())) { diff --git a/packages/objectql/src/validation/record-validator.ts b/packages/objectql/src/validation/record-validator.ts index 537221b9e3..9374c772b4 100644 --- a/packages/objectql/src/validation/record-validator.ts +++ b/packages/objectql/src/validation/record-validator.ts @@ -594,6 +594,41 @@ function valueShapeStrictEffective(deploymentVerified: boolean): boolean { return deploymentVerified; } +/** + * Has an environment switch already settled the reference / structured-JSON + * posture, so the boot advisory for the scan (#3438) would be untrue or + * useless? + * + * `OS_DATA_VALUE_SHAPE_STRICT_ENABLED=1` means enforcement is already on for + * every class, so a line announcing "warn mode is in effect" would be false. + * `OS_ALLOW_LAX_VALUE_SHAPES=1` means the operator opted out deliberately — + * running the scan would not change what they get, so pointing at it is noise. + * + * Exported rather than re-read in the engine for the same reason the scanner + * imports `valueShapeViolation`: two readings of these variables drifting by + * one clause is how a deployment gets told to run a migration that would not + * change its posture, or gets told nothing while it still would. + * + * The predicate is exactly the pair {@link valueShapeStrictEffective} consults + * ahead of the deployment flag — the announcement stays silent in precisely + * the cases where the flag it reports on cannot decide the posture. + */ +export function valueShapePostureSetByEnv(): boolean { + return LAX_VALUE_SHAPES() || VALUE_SHAPE_STRICT(); +} + +/** + * The media counterpart, standing in the same relation to + * {@link mediaStrictEffective}. A sibling rather than a parameter for the + * reason the two `*StrictEffective` functions are siblings: each gate names + * the switches it answers to at its own call site, and the two sets differ by + * more than a name — `OS_DATA_VALUE_SHAPE_STRICT_ENABLED` opens both, while + * each opt-out reaches only its own class. + */ +export function mediaPostureSetByEnv(): boolean { + return LAX_MEDIA_VALUES() || VALUE_SHAPE_STRICT(); +} + /** * Is `value` a violation of `def`'s declared stored shape — the SAME question * `validateOne` asks, exported so the `os migrate value-shapes` scanner counts diff --git a/packages/objectql/src/value-shape-scan-advisory.test.ts b/packages/objectql/src/value-shape-scan-advisory.test.ts new file mode 100644 index 0000000000..04895f2491 --- /dev/null +++ b/packages/objectql/src/value-shape-scan-advisory.test.ts @@ -0,0 +1,182 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * `announceOpenMigrationGates` against the REAL registry (ADR-0104 D1 rollout / + * #3438, correcting #4235 and #4253). + * + * The announcement itself is covered in `engine.test.ts`, which mocks + * `./registry` away. That mock is why these cases live in their own file: the + * genuine `registerApp → registry` path INJECTS covered-type fields into every + * object it registers, and a mocked registry hands the engine exactly the + * fields the test wrote. The two defects pinned below are both invisible under + * the mock and unconditional in production, so the suite that can see them has + * to build its objects the way a deployment does. + * + * The risk with any such line is not silence, it is speaking where the line is + * untrue or useless — that trains readers to ignore it. So each silent case is + * pinned, not just the speaking one. + */ + +import { describe, it, expect, afterEach, vi } from 'vitest'; +import { ObjectQL } from './engine'; +import type { IDataDriver } from '@objectstack/spec/contracts'; + +/** Minimal driver whose `find` serves whatever rows the test seeded. */ +function makeDriver(seed: Record>> = {}): IDataDriver { + const store = new Map>>(Object.entries(seed)); + return { + name: 'default', + version: '1.0.0', + async connect() {}, + async disconnect() {}, + async find(object: string) { return store.get(object) ?? []; }, + async findOne() { return null; }, + async count() { return 0; }, + async create(_o: string, data: any) { return data; }, + async update(_o: string, id: string, data: any) { return { ...data, id }; }, + async delete() { return true; }, + async bulkCreate(_o: string, rows: any[]) { return rows; }, + async syncSchema() {}, + async dropTable() {}, + } as unknown as IDataDriver; +} + +const COVERED = { + name: 'contact', + fields: { + id: { type: 'text' }, + account: { type: 'lookup', reference: 'account' }, + geo: { type: 'location' }, + }, +}; + +/** Declares no covered field of its own — but the registry will inject four. */ +const UNCOVERED = { + name: 'note', + fields: { id: { type: 'text' }, body: { type: 'textarea' } }, +}; + +/** The `sys_migration` shape the flag reader needs to find a row at all. */ +const FLAG_OBJECT = { + name: 'sys_migration', + fields: { + id: { type: 'text' }, + last_run_at: { type: 'datetime' }, + verified_at: { type: 'datetime' }, + blocking: { type: 'number' }, + }, +}; + +function makeEngine(opts: { + objects?: any[]; + flagRows?: Array>; +} = {}) { + const engine = new ObjectQL(); + engine.registerDriver( + makeDriver(opts.flagRows ? { sys_migration: opts.flagRows } : {}), + true, + ); + engine.registerApp({ + id: 'advisory_pkg', + name: 'Advisory', + objects: opts.objects ?? [COVERED, UNCOVERED], + } as any); + return engine; +} + +/** Everything the announcement logged, as one string. */ +async function announce(engine: ObjectQL): Promise { + const info = vi.spyOn((engine as any).logger, 'info'); + await engine.announceOpenMigrationGates(); + return info.mock.calls.map((c: any[]) => String(c[0])).join('\n'); +} + +afterEach(() => { + vi.restoreAllMocks(); + delete process.env.OS_DATA_VALUE_SHAPE_STRICT_ENABLED; + delete process.env.OS_ALLOW_LAX_VALUE_SHAPES; + delete process.env.OS_ALLOW_LAX_MEDIA_VALUES; +}); + +describe('announceOpenMigrationGates, real registry (ADR-0104 D1 / #3438)', () => { + it('names the command that ends warn mode on a warn-first deployment', async () => { + expect(await announce(makeEngine())).toMatch(/os migrate value-shapes/); + }); + + it('stays silent when no object declares a covered field — the gate is moot here', async () => { + // The regression guard for the predicate the dormancy short-circuit counts + // with. `note` declares only text, but the REGISTRY injects + // `organization_id` and `owner_id` (both `system`) plus `created_by` / + // `updated_by` (both in `SKIP_FIELDS`) — four `lookup`s, a covered TYPE + // apiece. `validateRecord` skips every one of them before it reaches the + // value-shape check, so counting by raw type answered true for literally + // every object that exists: the short-circuit never fired, its WeakMap + // memoized a constant, and this line fired on every deployment there is. + // Counting by the validator's own `isScannableValueShapeField` — the + // predicate the scanner already imports — is what makes it mean something. + // + // Unreachable through `engine.test.ts`: under its registry mock `note` + // carries the two fields written above and nothing else. + const said = await announce(makeEngine({ objects: [UNCOVERED] })); + expect(said).not.toMatch(/os migrate/); + }); + + it('stays silent under the all-class opt-in — "NOT enforced here" would be false', async () => { + // `VALUE_SHAPE_STRICT()` short-circuits both `*StrictEffective` functions + // ahead of the deployment flag, so enforcement is already on and the flag + // this line reports on decides nothing. + process.env.OS_DATA_VALUE_SHAPE_STRICT_ENABLED = '1'; + expect(await announce(makeEngine())).not.toMatch(/os migrate/); + }); + + it('stays silent under the value-shape escape hatch — the operator opted out on purpose', async () => { + // Running the scan cannot change this deployment's posture, so naming it is + // noise, not help. + process.env.OS_ALLOW_LAX_VALUE_SHAPES = '1'; + expect(await announce(makeEngine())).not.toMatch(/os migrate value-shapes/); + }); + + it('applies each opt-out to its own class only', async () => { + // The two opt-outs are per-class, so silencing one must not silence the + // other. `contact` declares a `location`; `note` gets a `file` here so both + // gates are live and exactly one is switched off. + process.env.OS_ALLOW_LAX_MEDIA_VALUES = '1'; + const said = await announce(makeEngine({ + objects: [COVERED, { name: 'note', fields: { id: { type: 'text' }, doc: { type: 'file' } } }], + })); + expect(said).not.toMatch(/os migrate files-to-references/); + expect(said).toMatch(/os migrate value-shapes/); + }); + + it('stays silent once the gate is open (verified row)', async () => { + const said = await announce(makeEngine({ + objects: [COVERED, UNCOVERED, FLAG_OBJECT], + flagRows: [ + { + id: 'adr-0104-value-shapes', + last_run_at: '2026-07-30T00:00:00.000Z', + verified_at: '2026-07-30T00:00:00.000Z', + blocking: 0, + }, + ], + })); + expect(said).not.toMatch(/os migrate value-shapes/); + }); + + it('still speaks when a row exists but did not pass its self-check', async () => { + // A failed run is not permission — the same "absent evidence is not + // permission" rule the gate itself runs on, so the advice still applies. + const said = await announce(makeEngine({ + objects: [COVERED, UNCOVERED, FLAG_OBJECT], + flagRows: [ + { + id: 'adr-0104-value-shapes', + last_run_at: '2026-07-30T00:00:00.000Z', + verified_at: null, + blocking: 3, + }, + ], + })); + expect(said).toMatch(/os migrate value-shapes/); + }); +});