Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions .changeset/value-shape-detail-prefers-unrecognized-keys.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
---
"@objectstack/objectql": patch
---

`os migrate value-shapes` now prescribes the key rename on a legacy `{latitude, longitude}` location, instead of reporting the missing-pair type error.

A value-shape rejection was read positionally — `parse.error.issues[0]` — at both places the value-shape detail is produced: the write path's warn-first / strict branch, and the exported `valueShapeViolation` the scan imports. zod reports per-member issues before the object-level `unrecognized_keys` one, so on a value whose keys were **renamed** the actionable message sorts last and was discarded. A `location` stored as `{latitude, longitude}` — the exact legacy shape the scan's own header names as one it exists to find — reported `Invalid input: expected number, received undefined`, leaving an operator to derive a rename that edit distance cannot reach (`latitude` -> `lat`), while `LocationValueSchema` had built the prescription and thrown it away.

Both readers now prefer the undeclared-key issue when the rejection carries one, through a single shared helper — two readings of the same rejection drifting by one clause is how one path prescribes the rename and the other does not. The affected strings are the `os migrate value-shapes` finding `detail`, the warn-first `[value-shape]` log line, and the `invalid_value_shape` error's `detail` under strict enforcement.

⛔ No verdict moves. The same values are flagged, the same writes are rejected or admitted, and the deployment gate opens on exactly the same evidence — only the operator-facing text changes.

Scoped by measurement rather than by assumption: of the sixteen types these readers cover, only `location` and `address` are backed by a key-closed object schema, so only they can emit `unrecognized_keys` at all — for the other fourteen the preference cannot change a single character. Both classes it does reach curate the alias map that makes the undeclared key the more actionable half. The defect reaches `address` as well as `location`: every address member being optional rules out a *missing*-member type error, but not a *wrong-typed* declared one, which still sorts ahead of the undeclared-key issue.
55 changes: 51 additions & 4 deletions packages/objectql/src/validation/record-validator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -487,6 +487,47 @@ export function coerceBooleanFields<T extends Record<string, unknown>>(
return (copy ?? row) as T;
}

/**
* The one parse issue an author can act on, out of everything zod reported for
* a value-shape rejection.
*
* NOT `issues[0]`. zod reports per-member issues before the object-level
* `unrecognized_keys` one, so on a value whose keys were RENAMED the actionable
* message sorts LAST. A `location` stored as `{latitude, longitude}` — the
* exact legacy shape `scan-value-shapes` names in its own header as one the
* scan exists to find — reports:
*
* [0] invalid_type lat Invalid input: expected number, received undefined
* [1] invalid_type lng Invalid input: expected number, received undefined
* [2] unrecognized_keys ... Did you mean `latitude` -> `lat`, `longitude` -> `lng`? ...
*
* The rename IS the prescription, and edit distance cannot reach it
* (`latitude` -> `lat`), which is exactly why `LocationValueSchema` curates an
* `aliases` map. Reading positionally builds that hint and then discards it,
* leaving an operator running `os migrate value-shapes` to derive the rename
* themselves while the identically-shaped `address` case is handed it.
*
* The preference is a NO-OP for every other class rather than merely harmless
* to it, which is what makes it safe as a blanket rule: of the sixteen types
* these readers cover, only `location` and `address` are backed by a
* `strictObject`, so only they can emit `unrecognized_keys` at all. The
* reference and file-reference classes are strings, `composite` / `record` /
* `repeater` / `vector` are open records and arrays, `json` is `z.unknown()`,
* and the one deliberately loose object shape (`FileValueSchema`) never refuses
* a key. For the other fourteen this cannot change a single character; both
* classes it does reach curate the alias map that makes the undeclared key the
* more actionable half of the rejection.
*
* Shared by both readers for the reason `valueShapeViolation` is exported
* rather than re-derived one layer up: two readings of the same rejection
* drifting by one clause is how one path prescribes the rename and the other
* hands out `expected number, received undefined`.
*/
function valueShapeDetail(error: { issues: ReadonlyArray<{ code: string; message: string }> }): string {
const { issues } = error;
return (issues.find((i) => i.code === 'unrecognized_keys') ?? issues[0])?.message ?? 'invalid value shape';
}

function validateOne(
name: string,
def: FieldDef,
Expand Down Expand Up @@ -792,7 +833,7 @@ function validateOne(
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 detail = valueShapeDetail(parsed.error);
const isMedia = FILE_REFERENCE_TYPES.has(t);
if (isMedia ? mediaStrictEffective(mediaStrict) : valueShapeStrictEffective(valueStrict)) {
return fail('invalid_type', { type: t, detail }, 'invalid_value_shape');
Expand Down Expand Up @@ -931,7 +972,9 @@ export function mediaPostureSetByEnv(): boolean {
* not recognise, which is precisely the borrowed-evidence failure the ADR's
* addendum forbids one layer up.
*
* Returns the first parse issue's message, or `null` when the value conforms.
* Returns the actionable parse issue's message (see `valueShapeDetail` — the
* undeclared-key one when present, which is the half carrying the rename),
* or `null` when the value conforms.
* A value that is missing (per `isMissing`) is never a violation: absence is
* the `required` check's business, not the shape contract's.
*/
Expand All @@ -943,7 +986,7 @@ export function valueShapeViolation(def: FieldDef, value: unknown): string | nul
}
const parsed = shapeSchemaFor(def).safeParse(value);
if (parsed.success) return null;
return parsed.error.issues[0]?.message ?? 'invalid value shape';
return valueShapeDetail(parsed.error);
}

/**
Expand Down Expand Up @@ -1009,7 +1052,11 @@ export interface AdmittedValueShapeViolation {
field: string;
/** The declared field type, so a report can group by what went wrong. */
type: string;
/** The first parse issue — the prescription an author acts on. */
/**
* The prescription an author acts on — `valueShapeDetail`'s reading of the
* rejection, the same string the warn-first log line and the
* `os migrate value-shapes` finding carry. Not positional.
*/
detail: string;
}

Expand Down
40 changes: 40 additions & 0 deletions packages/objectql/src/validation/scan-value-shapes.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,27 @@ describe('scanValueShapes (ADR-0104 D1 / #3438)', () => {
).not.toThrow();
});

it('#15490: an undeclared key outranks a same-value member error — on address too, not just location', async () => {
// The sweep this fix owes measured every class the two readers cover, and
// found only `location` and `address` able to emit `unrecognized_keys` at
// all (the rest are strings, open records, arrays, or `z.unknown()`). It
// also refuted the reason `address` was believed immune: every member being
// OPTIONAL rules out a MISSING-member `invalid_type`, but says nothing about
// a WRONG-TYPED declared one — which still sorts ahead of the object-level
// issue. So the defect reaches this class too; the contrast case that made
// it look location-only just happened to carry no type error.
const engine = makeEngine({
contact: [{ id: 'c1', addr: { street: 5, postal_code: '98101' } }],
});
const report = await scanValueShapes(engine, silent);

expect(report.blocking).toBe(1);
const addr = report.findings.find((f) => f.field === 'addr')!;
// Positionally this rejection reads `[invalid_type(street), unrecognized_keys]`.
expect(addr.detail).toContain('`postal_code` \u2192 `postalCode`');
expect(addr.detail).not.toContain('expected string, received number');
});

it('the scan counts exactly what strict mode rejects — one predicate, not two', async () => {
// The anti-drift property: every value the scan flags must also be a write
// rejection under the strict gate, and every value it passes must write.
Expand All @@ -186,6 +207,25 @@ describe('scanValueShapes (ADR-0104 D1 / #3438)', () => {
const report = await scanValueShapes(engine, silent);
expect(report.blocking).toBe(1);

// #15490: the fixture above was already the renamed pair, but the pin
// asserted only the COUNT — so the operator-facing prescription was free to
// be the wrong half of the rejection and every gate stayed green. zod sorts
// the two missing-member `invalid_type` issues (`lat`, `lng`) ahead of the
// object-level `unrecognized_keys` one, so a positional `issues[0]` read
// reported `expected number, received undefined` and threw away the rename
// the schema curates an `aliases` map to produce. Assert the PRESCRIPTION,
// not just that something was flagged: `detail` is documented as "the
// prescription an author acts on".
const geo = report.findings.find((f) => f.field === 'geo')!;
expect(geo.detail).toContain('`latitude` \u2192 `lat`');
expect(geo.detail).toContain('`longitude` \u2192 `lng`');
expect(geo.detail).toContain('this location value');
// The half that was being shown instead — naming it keeps the pin able to
// see a regression back to the positional read.
expect(geo.detail).not.toContain('expected number, received undefined');
// Customer-facing refusal text carries no internal issue id.
expect(geo.detail).not.toMatch(/#\d+/);

expect(() =>
validateRecord(OBJECTS.contact, { ...flagged }, 'update', { valueShapeStrict: true }),
).toThrow(ValidationError);
Expand Down
9 changes: 8 additions & 1 deletion packages/objectql/src/validation/scan-value-shapes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,14 @@ export interface ValueShapeFinding {
count: number;
/** A few offending record ids, so an operator can go and look. */
sampleRecordIds: string[];
/** The first parse issue seen — the prescription an author acts on. */
/**
* The prescription an author acts on: the undeclared-key issue when the
* rejection carries one (it names the key AND the rename its schema
* curates), else the first issue seen. Deliberately not positional —
* zod sorts per-member issues ahead of the object-level one, so on the
* renamed `{latitude, longitude}` location this header names, the
* actionable message is last.
*/
detail: string;
}

Expand Down
Loading