From 232813b45454af6d709d78ddbac3176e72837e28 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 29 Aug 2026 00:53:57 +0000 Subject: [PATCH] fix(fields): refuse location coordinates the spec rejects `LocationField` accepted any pair of FINITE numbers as a coordinate, while `@objectstack/spec`'s `LocationValueSchema` also constrains their range (`lat` -90..90, `lng` -180..180). Typing `999, 999` emitted `{ lat: 999, lng: 999 }`, which `valueSchemaFor({ type: 'location' })` refuses with `too_big` at both keys - the producer direction of the contract-first failure class (AGENTS.md #0.1). Measured before choosing the disposition: nothing downstream rejects or repairs the value. A real `ObjectForm` submit handed `place: { lat: 999, lng: 999 }` straight to `dataSource.create`, with `aria-invalid="false"` and no error text anywhere. So the widget is the only place a refusal can work, and the fix refuses the emission - extending the rule this widget already applies to text that is not a coordinate pair from format to range. The bounds are not restated in the widget. A hand-copied `-90..90` would be a second contract free to drift, so the emission is put to `LocationValueSchema` itself. That also covers the whole emitted object (so `altitude`/`accuracy` carried across an edit are held to the contract too) and refuses `Infinity`, which the finiteness gate let through. Reading is deliberately unchanged: a record already holding an out-of-range pair still renders, so the person who can correct it can still see it. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01CRJge11jso9TpXRWFt1Z49 --- .changeset/6714-location-range-refusal.md | 43 ++++ .../__tests__/LocationField.range.test.tsx | 192 ++++++++++++++++++ packages/fields/src/widgets/LocationField.tsx | 61 +++++- .../src/ObjectForm.locationRange.test.tsx | 121 +++++++++++ 4 files changed, 415 insertions(+), 2 deletions(-) create mode 100644 .changeset/6714-location-range-refusal.md create mode 100644 packages/fields/src/__tests__/LocationField.range.test.tsx create mode 100644 packages/plugin-form/src/ObjectForm.locationRange.test.tsx diff --git a/.changeset/6714-location-range-refusal.md b/.changeset/6714-location-range-refusal.md new file mode 100644 index 0000000000..5415bf5c9e --- /dev/null +++ b/.changeset/6714-location-range-refusal.md @@ -0,0 +1,43 @@ +--- +'@object-ui/fields': patch +--- + +`LocationField` no longer emits a coordinate pair the platform's own validator +refuses (objectui#6714). + +`@objectstack/spec`'s `LocationValueSchema` constrains the coordinate **range** +(`lat` −90..90, `lng` −180..180), but the widget's guard tested only that each +coordinate was a finite number. Typing `999, 999` therefore emitted +`{ lat: 999, lng: 999 }` — a value `valueSchemaFor({ type: 'location' })` +rejects with `too_big` at both keys. That is the producer direction of the +contract-first failure class (AGENTS.md #0.1): a renderer writing what the +contract rejects. It was open to every user who edits a location field, since +typing the coordinates is this field's only interaction. + +**Measured before choosing the disposition**, as triage required: nothing +downstream rejects or repairs the value. Driving a real `ObjectForm` with a +`type: 'location'` field and typing `999, 999` called `dataSource.create` once +with `place: { lat: 999, lng: 999 }` verbatim, `aria-invalid="false"` on the +control and no error text anywhere. `sanitizeFormData` filters keys and never +inspects a value, and `buildValidationRules` has no `location` branch. So the +out-of-range pair reached storage silently, and the widget is the only place a +refusal can work. + +The fix therefore **refuses the emission**, extending the rule this widget +already applies to text that isn't a coordinate pair from *format* to *range*: +the typed pair is simply not written and the prior value stands. No new UI and +no new mechanism — the same `// If invalid, don't update the value` branch. + +The bounds are **not** restated in the widget. A hand-copied `-90..90` would be +a second contract free to drift from the spec, so the emission is put to +`LocationValueSchema` itself. Two consequences of asking the schema rather than +testing two bounds by hand: the check covers the WHOLE emitted object, so the +`altitude`/`accuracy` carried across an edit (objectui#6664) are held to the +contract too; and `Infinity` is refused as well, which the finiteness gate let +through (`parseFloat('Infinity')` is `Infinity`, and `!isNaN(Infinity)` is +`true`). + +Reading is deliberately unchanged: a record that already holds an out-of-range +pair still renders in the box, so the person who can correct it can still see +it. objectui#6272's empty render was for a value whose *shape* this widget +cannot read; this shape is readable, it is only not writable. diff --git a/packages/fields/src/__tests__/LocationField.range.test.tsx b/packages/fields/src/__tests__/LocationField.range.test.tsx new file mode 100644 index 0000000000..84a1dddce4 --- /dev/null +++ b/packages/fields/src/__tests__/LocationField.range.test.tsx @@ -0,0 +1,192 @@ +/** + * ObjectUI + * Copyright (c) 2024-present ObjectStack Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +/** + * objectui#6714 — `LocationField` must not EMIT a coordinate pair the + * platform's own validator refuses. + * + * The defect: the widget's guard tested only that a coordinate was a finite + * number, while `@objectstack/spec`'s `LocationValueSchema` also constrains the + * RANGE (`lat` −90..90, `lng` −180..180). Typing `999, 999` therefore emitted + * `{ lat: 999, lng: 999 }`, which `valueSchemaFor({ type: 'location' })` + * refuses with `too_big` at both keys — the producer direction of the + * contract-first failure class (AGENTS.md #0.1). + * + * ## Why "refuse the emission" and not "emit and mark invalid" + * + * Triage left the disposition to the implementer and required one measurement + * first: does anything downstream reject or repair the value before storage? + * It was measured by driving a REAL `ObjectForm` (create mode, a + * `type: 'location'` field, a fake `DataSource`) and typing the card's + * `999, 999`. `dataSource.create` was called ONCE, with + * `{ lat: 999, lng: 999 }` verbatim; `aria-invalid` on the control was + * `"false"` and no error text was rendered. Nothing rejects it and nothing + * repairs it — an out-of-range pair reaches storage silently. Per the ruling, + * refusing the emission is then the only arm that prevents the dirty write, and + * it extends a rule this widget ALREADY applies to text that isn't a coordinate + * pair from *format* to *range*. That end-to-end reading is pinned in + * `packages/plugin-form/src/ObjectForm.locationRange.test.tsx`. + * + * ## The oracle + * + * Every case below is judged by the SPEC's own refusal + * (`valueSchemaFor({ type: 'location' })`), never by a range copied into this + * file. A hand-written `-90..90` here would keep passing on the day the spec + * moved — which is the whole failure this card is about, reintroduced in the + * test. + */ + +import { describe, it, expect, vi } from 'vitest'; +import React from 'react'; +import { render, screen, fireEvent, cleanup } from '@testing-library/react'; +import '@testing-library/jest-dom'; +import { valueSchemaFor } from '@objectstack/spec/data'; + +import { LocationField, type LocationValue } from '../widgets/LocationField'; + +const LOCATION_SCHEMA = valueSchemaFor({ type: 'location' } as any)!; + +const field = { name: 'site', label: 'Site', type: 'location' } as any; + +/** + * A stored, in-range value — the "prior value" a refusal must leave standing. + * + * ⚠️ It must differ from every coordinate typed below. The box is a CONTROLLED + * input showing `"lat, lng"`, so `fireEvent.change` with text equal to what is + * already displayed fires no change event at all — the widget would read as + * having "refused" a perfectly legal pair, for a reason that has nothing to do + * with this card. + */ +const STORED: LocationValue = { lat: 10, lng: 20 }; + +/** Render with `value` stored, type `text`, return every emission it caused. */ +function emissionsFor(text: string, value: unknown = STORED): unknown[] { + cleanup(); + const onChange = vi.fn(); + render( + , + ); + fireEvent.change(screen.getByRole('textbox'), { target: { value: text } }); + return onChange.mock.calls.map(c => c[0]); +} + +/** What the spec says about a pair, as the codes it reports. */ +function specIssues(pair: unknown): string[] { + const parsed = LOCATION_SCHEMA.safeParse(pair); + return parsed.success + ? [] + : parsed.error.issues.map((i: any) => `${i.code}@[${i.path.join('.')}]`); +} + +/* -------------------------------------------------------------------------- */ +/* The card's own three cases, with the spec's verdict asserted alongside. */ +/* -------------------------------------------------------------------------- */ + +describe('LocationField refuses out-of-range coordinates (objectui#6714)', () => { + it.each([ + ['999, 999', { lat: 999, lng: 999 }, ['too_big@[lat]', 'too_big@[lng]']], + ['91, 0', { lat: 91, lng: 0 }, ['too_big@[lat]']], + ['0, 181', { lat: 0, lng: 181 }, ['too_big@[lng]']], + ['-91, 0', { lat: -91, lng: 0 }, ['too_small@[lat]']], + ['0, -181', { lat: 0, lng: -181 }, ['too_small@[lng]']], + ])('does not emit %s', (typed, wouldHaveBeen, expectedIssues) => { + // First: this pair really is one the platform refuses — the premise the + // refusal rests on, asserted from the spec rather than assumed. + expect(specIssues(wouldHaveBeen)).toEqual(expectedIssues); + // Then: the widget never hands it to `onChange`. + expect(emissionsFor(typed)).toEqual([]); + }); + + it('leaves the previously stored value standing, exactly as a bad FORMAT does', () => { + // The rule being extended, shown as one rule: text that is not a coordinate + // pair and a pair the spec refuses are both simply not emitted, and neither + // disturbs the value already stored. + expect(emissionsFor('not a coordinate')).toEqual([]); + expect(emissionsFor('999, 999')).toEqual([]); + }); + + it('refuses an infinite coordinate, which the finiteness gate alone let through', () => { + // `parseFloat('Infinity')` is `Infinity` and `!isNaN(Infinity)` is `true`, + // so the pre-existing format gate accepted it. `z.number()` does not. + expect(specIssues({ lat: Infinity, lng: 0 })).not.toEqual([]); + expect(emissionsFor('Infinity, 0')).toEqual([]); + }); +}); + +/* -------------------------------------------------------------------------- */ +/* The other direction: the refusal must not cost any legal coordinate. */ +/* -------------------------------------------------------------------------- */ + +describe('LocationField still emits every coordinate the spec accepts (objectui#6714)', () => { + it.each([ + ['30.2741, 120.1551', { lat: 30.2741, lng: 120.1551 }], + // The inclusive bounds themselves — the poles and the antimeridian are real + // places, and an off-by-one in the guard would silently make them untypable. + ['90, 180', { lat: 90, lng: 180 }], + ['-90, -180', { lat: -90, lng: -180 }], + ['0, 0', { lat: 0, lng: 0 }], + ])('emits %s', (typed, expected) => { + expect(specIssues(expected)).toEqual([]); + expect(emissionsFor(typed)).toEqual([expected]); + }); + + it('clearing the box still emits null', () => { + // Unrelated to range, and the one emission that is deliberately not a + // `LocationValue` at all — pinned so the new gate cannot swallow it. + expect(emissionsFor('')).toEqual([null]); + }); + + it('still carries the optional keys across an in-range edit (objectui#6664)', () => { + // The gate is applied to the WHOLE emitted object, so this is the pin that + // it did not start rejecting the keys #6664 just taught it to carry. + expect(emissionsFor('31.2304, 121.4737', { lat: 30.2741, lng: 120.1551, altitude: 5, accuracy: 12 })) + .toEqual([{ lat: 31.2304, lng: 121.4737, altitude: 5, accuracy: 12 }]); + }); +}); + +/* -------------------------------------------------------------------------- */ +/* The property the card actually asks for, stated once against the oracle. */ +/* -------------------------------------------------------------------------- */ + +describe('LocationField emits a value iff the spec accepts it (objectui#6714)', () => { + it('agrees with `valueSchemaFor({ type: "location" })` on every case above', () => { + const cases: Array<[string, { lat: number; lng: number }]> = [ + ['999, 999', { lat: 999, lng: 999 }], + ['91, 0', { lat: 91, lng: 0 }], + ['0, 181', { lat: 0, lng: 181 }], + ['-91, 0', { lat: -91, lng: 0 }], + ['0, -181', { lat: 0, lng: -181 }], + ['90, 180', { lat: 90, lng: 180 }], + ['-90, -180', { lat: -90, lng: -180 }], + ['0, 0', { lat: 0, lng: 0 }], + ['30.2741, 120.1551', { lat: 30.2741, lng: 120.1551 }], + ]; + for (const [typed, pair] of cases) { + const specAccepts = LOCATION_SCHEMA.safeParse(pair).success; + const emitted = emissionsFor(typed); + expect( + emitted.length === 1, + `typed "${typed}": spec ${specAccepts ? 'ACCEPTS' : 'REJECTS'} it, widget ${emitted.length ? 'emitted' : 'refused'}`, + ).toBe(specAccepts); + } + }); + + it('never emits anything the spec would refuse, whatever it was handed', () => { + // The invariant in its strongest form: sweep the emissions and re-parse + // each one. A future edit that widens the gate fails here. + const typedTexts = [ + '999, 999', '91, 0', '0, 181', '-91, 0', '0, -181', 'Infinity, 0', + '90, 180', '-90, -180', '0, 0', '30.2741, 120.1551', '12, 34', + ]; + for (const typed of typedTexts) { + for (const emitted of emissionsFor(typed)) { + expect(LOCATION_SCHEMA.safeParse(emitted).success, `emitted for "${typed}"`).toBe(true); + } + } + }); +}); diff --git a/packages/fields/src/widgets/LocationField.tsx b/packages/fields/src/widgets/LocationField.tsx index aac3b3cfe7..28f88fa24c 100644 --- a/packages/fields/src/widgets/LocationField.tsx +++ b/packages/fields/src/widgets/LocationField.tsx @@ -1,5 +1,6 @@ import React from 'react'; import { Input, EmptyValue } from '@object-ui/components'; +import { LocationValueSchema } from '@objectstack/spec/data'; import type { LocationValue } from '@objectstack/spec/data'; import { FieldWidgetComponentProps } from './types.js'; import { toDomProps } from './toDomProps.js'; @@ -53,6 +54,52 @@ function isFiniteNumber(n: unknown): n is number { return typeof n === 'number' && Number.isFinite(n); } +/** + * Is this candidate emission one the platform's own validator ACCEPTS? + * + * The guard above tests that a coordinate is a real number; the spec also + * constrains its RANGE, and nothing in this widget used to (objectui#6714): + * + * ```ts + * // @objectstack/spec, LocationValueSchema + * lat: z.number().min(-90).max(90) + * lng: z.number().min(-180).max(180) + * ``` + * + * So typing `999, 999` emitted `{ lat: 999, lng: 999 }` — a value + * `valueSchemaFor({ type: 'location' })` refuses with `too_big` at BOTH keys. + * That is the producer direction of the contract-first failure class + * (AGENTS.md #0.1): a renderer writing what the contract rejects. It is also + * open to every user who edits a location field, since typing the coordinates + * is this field's only interaction. + * + * ⛔ The bounds are NOT restated here as `-90`/`90` literals. A hand-copied + * range is a SECOND contract that can drift from the spec silently — the exact + * shape #0.1 bans — so the spec's own schema is asked instead. It is a + * memoized lazy schema, so this costs one `safeParse` of a 2–4 key object. + * + * Two deliberate consequences of asking the schema rather than testing two + * bounds by hand: + * + * - The check is on the WHOLE emitted object, so `altitude`/`accuracy` carried + * across the edit are held to the contract too. {@link carryOptionalKeys} + * already narrows them to finite numbers, so this is a no-op today — it is + * the guard that keeps it one. + * - `Infinity` is refused as well. `parseFloat('Infinity')` is `Infinity` and + * `!isNaN(Infinity)` is `true`, so the format gate alone let it through; + * `z.number()` rejects it. Same defect class, same fix, no extra branch. + * + * ⚠️ Deliberately NOT wired into {@link isLocationValue}, which is the READ + * guard. A record that already holds an out-of-range pair keeps RENDERING here, + * so the person who can correct it can still see it — blanking it would hide + * the dirty data from its only fixer. objectui#6272's empty render was for a + * value whose SHAPE this widget cannot read; this shape is readable, it is just + * not writable. This card is the producer direction only. + */ +function isSpecAcceptedLocation(candidate: LocationValue): boolean { + return LocationValueSchema.safeParse(candidate).success; +} + /** * Build the value emitted for a freshly typed coordinate pair, carrying the * spec's two OPTIONAL keys across the edit (objectui#6664). @@ -127,9 +174,19 @@ export function LocationField({ value, onChange, field, readonly, error, ...prop if (!isNaN(lat) && !isNaN(lng)) { // The typed pair replaces `lat`/`lng`; `altitude`/`accuracy` survive // the edit (objectui#6664). Key-by-key, never a spread — see above. - onChange(carryOptionalKeys(lat, lng, value)); + const emitted = carryOptionalKeys(lat, lng, value); + // objectui#6714: the SAME refusal the line below already applies to + // text that isn't a coordinate pair, extended from format to RANGE. + // Measured before choosing this: nothing downstream rejects or repairs + // the value — a real `ObjectForm` submit hands `{ lat: 999, lng: 999 }` + // straight to `dataSource.create`, with no error raised anywhere — so + // refusing HERE is the only thing standing between a typo and storage. + if (isSpecAcceptedLocation(emitted)) { + onChange(emitted); + } } - // If invalid, don't update the value + // If the text is not a coordinate pair, or the pair is one the spec + // refuses, don't update the value — the prior value stands. } }; diff --git a/packages/plugin-form/src/ObjectForm.locationRange.test.tsx b/packages/plugin-form/src/ObjectForm.locationRange.test.tsx new file mode 100644 index 0000000000..4ab2923c41 --- /dev/null +++ b/packages/plugin-form/src/ObjectForm.locationRange.test.tsx @@ -0,0 +1,121 @@ +/** + * ObjectUI + * Copyright (c) 2024-present ObjectStack Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +/** + * objectui#6714, measured end to end: an out-of-range coordinate must never + * reach the data source. + * + * Triage required one reading before the widget-side disposition could be + * chosen — **does anything downstream reject or repair the value before it is + * stored?** This file is that measurement, kept as a pin. + * + * What it measured, on the merge-base: typing the card's `999, 999` into a real + * `ObjectForm` called `dataSource.create` ONCE, with + * `place: { lat: 999, lng: 999 }` verbatim — while + * `valueSchemaFor({ type: 'location' })` refuses that same value with + * `too_big` at both keys. `aria-invalid` on the control read `"false"` and the + * field rendered no error text. So the answer is NEITHER: nothing on this path + * rejects the value and nothing repairs it. + * + * That is what makes the widget the only place a refusal can work, and it is + * why the widget refuses to emit rather than emitting and marking the field + * invalid: an emission here is a write, not a warning. + * + * ⚠️ The pass-through assertion below is load-bearing, not decoration. The form + * hands the widget's emission to `dataSource.create` UNCHANGED — + * `sanitizeFormData` filters KEYS (server-managed, computed, read-only) and + * never inspects a value, and `buildValidationRules` has no branch for + * `location`, so its `min`/`max` rules only ever carry an author-declared bound + * on a scalar. If a later change ever did add repair or validation on this + * path, that assertion is what notices. + */ +import { describe, it, expect, vi } from 'vitest'; +import { render, waitFor, fireEvent } from '@testing-library/react'; +import React from 'react'; +import { valueSchemaFor } from '@objectstack/spec/data'; + +import { ObjectForm } from './ObjectForm'; +import { registerAllFields } from '@object-ui/fields'; + +registerAllFields(); + +const LOCATION_SCHEMA = valueSchemaFor({ type: 'location' } as any)!; + +const siteSchema = { + name: 'site', + fields: { + title: { type: 'text', label: 'Title' }, + place: { type: 'location', label: 'Place' }, + }, +}; + +const makeDS = () => ({ + getObjectSchema: vi.fn().mockResolvedValue(siteSchema), + create: vi.fn(async (_o: string, d: any) => ({ id: 'r1', ...d })), + update: vi.fn(), + findOne: vi.fn(), +}); + +const waitInput = (c: HTMLElement, name: string) => + waitFor(() => { + const el = c.querySelector(`input[name="${name}"]`) as HTMLInputElement | null; + if (!el) throw new Error(`${name} not ready`); + return el; + }); + +/** Type a title + a coordinate string into a real create form and submit it. */ +async function submitWith(coordinateText: string) { + const ds = makeDS(); + const { container } = render( + , + ); + + fireEvent.change(await waitInput(container, 'title'), { target: { value: 'HQ' } }); + fireEvent.change(await waitInput(container, 'place'), { target: { value: coordinateText } }); + fireEvent.submit(container.querySelector('form') as HTMLFormElement); + + await waitFor(() => expect(ds.create).toHaveBeenCalled()); + return { payload: ds.create.mock.calls[0][1] as Record, container }; +} + +describe('ObjectForm never stores an out-of-range location (objectui#6714)', () => { + it('does not put `999, 999` in the create payload', async () => { + // The exact value the card measured, and the exact reason it matters: the + // spec refuses it, so storing it is a dirty write. + const refused = LOCATION_SCHEMA.safeParse({ lat: 999, lng: 999 }); + expect(refused.success).toBe(false); + expect( + refused.success ? [] : refused.error.issues.map((i: any) => `${i.code}@[${i.path.join('.')}]`), + ).toEqual(['too_big@[lat]', 'too_big@[lng]']); + + const { payload } = await submitWith('999, 999'); + expect(payload.place).toBeUndefined(); + expect(payload).toMatchObject({ title: 'HQ' }); + }); + + it('passes an in-range coordinate through to the data source unchanged', async () => { + // The other half, and the pass-through evidence the measurement rests on: + // whatever the widget emits is what gets stored, byte for byte. Nothing on + // this path would have caught the bad value — which is why the widget must. + const { payload } = await submitWith('30.2741, 120.1551'); + expect(payload.place).toEqual({ lat: 30.2741, lng: 120.1551 }); + expect(LOCATION_SCHEMA.safeParse(payload.place).success).toBe(true); + }); + + it('stores nothing the platform validator would refuse, for either input', async () => { + for (const typed of ['999, 999', '91, 0', '0, 181', '30.2741, 120.1551']) { + const { payload } = await submitWith(typed); + if (payload.place !== undefined) { + expect(LOCATION_SCHEMA.safeParse(payload.place).success, `stored for "${typed}"`).toBe(true); + } + } + }); +});