diff --git a/.changeset/10156-edit-form-writes-only-changed-fields.md b/.changeset/10156-edit-form-writes-only-changed-fields.md index 7f45d5200b..91ea2e1e07 100644 --- a/.changeset/10156-edit-form-writes-only-changed-fields.md +++ b/.changeset/10156-edit-form-writes-only-changed-fields.md @@ -16,7 +16,7 @@ An edit form now writes only the fields that changed (objectui#10156). - A save with nothing changed still sends the full sanitized payload. It stays a real request, with the same concurrency guard and a real server record for `onSuccess`. - A form that did not read the record itself still sends every field. That covers a create, a record supplied as `initialData`, and inline `customFields`. - After a successful save, the form counts the fields it just wrote as saved. A form that stays open compares its next save with the record as it is now. Changing a field back to its first-read value is therefore still sent. -- The concurrency guard is unchanged. The update still carries `ifMatch` = the `updated_at` the form read, and a `409` still offers **Keep editing** or **Overwrite**. **Overwrite** now resends only the changed fields. +- The concurrency guard is unchanged by this rule. The update carries `ifMatch` = the `updated_at` the form read (after a save, a form that stays open sends the `updated_at` that save returned instead, objectui#10565), and a `409` still offers **Keep editing** or **Overwrite**. **Overwrite** now resends only the changed fields. - ⚠️ A host `submitHandler` on an edit form receives the payload the form would have written. That is the changed fields, or the full sanitized payload when nothing changed. A host that needs the whole record must read it itself. In this repository, only `MasterDetailForm` passes a `submitHandler` to an edit form. Its header form receives the changed fields. Its row editor has no `recordId`, so it still receives every value. - The JSDoc of `ObjectFormSchema.submitHandler` in `@object-ui/types`, and its copies on `ModalFormSchema` and `DrawerFormSchema`, now say what an edit-mode handler receives. diff --git a/.changeset/10565-occ-token-after-save.md b/.changeset/10565-occ-token-after-save.md new file mode 100644 index 0000000000..7d32993aa3 --- /dev/null +++ b/.changeset/10565-occ-token-after-save.md @@ -0,0 +1,17 @@ +--- +'@object-ui/plugin-form': patch +--- + +An edit form that stays open no longer meets the conflict dialog over its own earlier save (objectui#10565). + +**Clause-②: no.** No exported symbol, type or prop changes. The change is in which `ifMatch` an edit save sends. + +**Before.** Every edit layout (the simple form, `modal`, `drawer`, `tabbed`, `split` and `wizard`) sent `ifMatch` = the `updated_at` of the record it read with `findOne`, on every save. None of them refreshes that record after a save. The first save moves the stored `updated_at`, so the second save from the same open form sent a version the server no longer held. A server that enforces `ifMatch` answered `409 CONCURRENT_UPDATE`, and the user was offered **Keep editing** or **Overwrite** over a conflict with their own first save. + +**What changed, in observable terms.** + +- After a save lands, the next save of the same record from the same open form sends the `updated_at` that save returned, for every layout. The **Overwrite** retry counts as a save, so the save after it sends the version the overwrite returned. +- A genuine conflict still answers `409`: if someone else saves the record between two saves, the dialog still opens. +- If the form reads the record again and gets a different `updated_at`, the new read's token is sent. +- A record read without an `updated_at` is still saved without `ifMatch`. When a data source's `update` resolves without an `updated_at`, the next save sends the read's token, as before. +- What a save writes is unchanged: an edit still sends only the fields that differ from the record the form read (objectui#10156). diff --git a/content/docs/plugins/plugin-form.mdx b/content/docs/plugins/plugin-form.mdx index 58389ba8a7..4461547c49 100644 --- a/content/docs/plugins/plugin-form.mdx +++ b/content/docs/plugins/plugin-form.mdx @@ -431,7 +431,9 @@ caller's field-level security refuses — which every layout renders disabled. A `''`, and a lookup id and its expanded object all count as different. A save with nothing changed still sends the full payload, as it always has. The `ifMatch` concurrency guard is unchanged, and its **Overwrite** choice now resends only the -changed fields. A host `submitHandler` receives the same payload in edit mode. +changed fields. A form that stays open after a save sends the `updated_at` that +save returned as its next `ifMatch`, so its second save is not refused over its +own first one. A host `submitHandler` receives the same payload in edit mode. The package README has the full rule, under "What an edit save writes". ## Examples diff --git a/packages/plugin-form/README.md b/packages/plugin-form/README.md index 57f7c285dd..e43cf33b24 100644 --- a/packages/plugin-form/README.md +++ b/packages/plugin-form/README.md @@ -917,10 +917,12 @@ After a successful save, the form treats the fields it just wrote as saved. A form that stays open therefore compares its next save with the record as it stands now, not as it was first read. -The concurrency guard is unchanged. The update still carries -`ifMatch` = the `updated_at` the form read, and a `409` still offers -**Keep editing** or **Overwrite**. **Overwrite** now resends only the changed -fields, so it no longer rewrites fields this user never touched. +The concurrency guard is unchanged by this rule. The update carries +`ifMatch` = the `updated_at` the form read. Once a form that stays open has +saved the record, its next save carries the `updated_at` that save returned +instead, so the form is not refused over its own earlier save. A `409` still +offers **Keep editing** or **Overwrite**. **Overwrite** now resends only the +changed fields, so it no longer rewrites fields this user never touched. ⚠️ A host `submitHandler` gets the same payload in edit mode. Normally that is the changed fields; after a save with nothing changed, it is the full payload. diff --git a/packages/plugin-form/src/occSave.secondSave.test.tsx b/packages/plugin-form/src/occSave.secondSave.test.tsx new file mode 100644 index 0000000000..bdd73f986b --- /dev/null +++ b/packages/plugin-form/src/occSave.secondSave.test.tsx @@ -0,0 +1,295 @@ +/** + * 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#10565: the SECOND save from an edit form that stays open. + * + * Every layout hands `useOccSave` the record it READ as `baseRecord`, and none + * of them refreshes it after a save. The hook used to send that read's + * `updated_at` as `ifMatch` on every save. The first save moves the stored + * `updated_at`, so the second save sent a token the server no longer holds and + * was refused `409 CONCURRENT_UPDATE`: the user met the conflict dialog over + * their own earlier save. + * + * ── The double, and why it is shaped this way ───────────────────────────── + * `enforcingDataSource` stands in for the adapter over a server that enforces + * `ifMatch`. Its rule and its shapes were taken from one measurement against a + * real ObjectStack stack (the reading is on objectui#10565; nothing here + * re-derives it): + * - no token → no check; a token naming another instant → 409; + * - every accepted write stamps a strictly later `updated_at` + * (ISO-8601 with milliseconds and `Z`) and returns the whole record, + * which is what the adapter's `update` resolves with; + * - a refusal carries the measured 409 body, built into an `Error` the way + * `@objectstack/client` builds one and passed through the adapter's real + * `normaliseClientError`, so the hook sees the error the adapter throws. + * + * ── Direction of these pins ─────────────────────────────────────────────── + * - the single-save control is GREEN before and after; + * - every row that makes a second save from the same read is RED before the + * fix: it asserts the token that save sent. + */ + +import React from 'react'; +import { describe, it, expect, vi, afterEach } from 'vitest'; +import { render, screen, fireEvent, cleanup, waitFor, act } from '@testing-library/react'; +import { normaliseClientError } from '@object-ui/data-objectstack'; +import { registerAllFields } from '@object-ui/fields'; +import { ObjectForm } from './ObjectForm'; +import type { DataSource } from '@object-ui/types'; +import { useOccSave, type OccSaveArgs, type OccSaveOutcome } from './occSave'; + +registerAllFields(); + +const OBJECT = 'device'; +/** The measured spelling of a version: ISO-8601, milliseconds, `Z`. */ +const READ_VERSION = '2026-09-25T13:12:11.097Z'; + +type Row = Record; + +/** The 409 the adapter throws for the measured wire body. */ +function concurrentUpdate(object: string, id: string, row: Row, expected: string) { + const current = String(row.updated_at); + const body = { + error: `Record ${object}/${id} was modified by another user (current version ${current}, expected ${expected})`, + code: 'CONCURRENT_UPDATE', + currentVersion: current, + currentRecord: { ...row }, + object, + }; + // `@objectstack/client`: message from the body's `error` string, `code` + // from the body, `httpStatus` from the response, and the whole body as + // `details` because it carries no `details` of its own. + return normaliseClientError( + Object.assign(new Error(body.error), { code: body.code, httpStatus: 409, details: body }), + ); +} + +function enforcingDataSource(rows: Record) { + let clock = Date.parse(READ_VERSION); + const stamp = () => new Date((clock += 1_000)).toISOString(); + const update = vi.fn( + async (object: string, id: string | number, data: Row, opts?: { ifMatch?: string }) => { + const row = rows[String(id)]; + const expected = opts?.ifMatch; + if (expected && Date.parse(expected) !== Date.parse(String(row.updated_at))) { + throw concurrentUpdate(object, String(id), row, expected); + } + Object.assign(row, data, { updated_at: stamp() }); + return { ...row }; + }, + ); + return { + dataSource: { + getObjectSchema: vi.fn(async () => ({ + name: OBJECT, + fields: { name: { type: 'text', label: 'Name' } }, + })), + findOne: vi.fn(async (_o: string, id: string | number) => ({ ...rows[String(id)] })), + create: vi.fn(), + update, + }, + /** The `ifMatch` each `update` call carried, in call order. */ + sent: () => update.mock.calls.map((c) => c[3]?.ifMatch), + /** Someone else saves the record, outside this form. */ + otherWriter: (id: string, data: Row) => { + Object.assign(rows[id], data, { updated_at: stamp() }); + }, + }; +} + +const rowsOf = (...ids: string[]): Record => + Object.fromEntries(ids.map((id) => [id, { id, name: 'Mine', updated_at: READ_VERSION }])); + +/** The smallest honest host: the real hook, its real dialog, nothing between. */ +function mountHook() { + const handle: { save?: (args: OccSaveArgs) => Promise } = {}; + const Host: React.FC = () => { + const { saveWithOcc, conflictDialog } = useOccSave(); + handle.save = saveWithOcc; + return <>{conflictDialog}; + }; + render(); + /** Start a save without awaiting it: a refused one waits on the dialog. */ + return async (args: Omit) => { + let pending!: Promise; + await act(async () => { + pending = handle.save!({ objectName: OBJECT, ...args }); + }); + return pending; + }; +} + +/** + * A save the test expects to land. A refused save waits on the conflict + * dialog and never settles, so the dialog opening is the failure, not a hang. + */ +async function landed(pending: Promise) { + const outcome = await Promise.race([ + pending, + screen.findByText('Keep editing').then(() => { + throw new Error('the conflict dialog opened: the save was refused 409'); + }), + ]); + expect(outcome.status).toBe('saved'); + return (outcome as { status: 'saved'; result: Row }).result; +} + +afterEach(() => { + cleanup(); + vi.restoreAllMocks(); +}); + +describe('useOccSave: the version the last write returned is the next save\'s token (#10565)', () => { + it('control: a single save sends the updated_at the form read, and lands', async () => { + const { dataSource, sent } = enforcingDataSource(rowsOf('r1')); + const save = mountHook(); + const read = await dataSource.findOne(OBJECT, 'r1'); + + const result = await landed(save({ dataSource, recordId: 'r1', payload: { name: 'v2' }, baseRecord: read })); + + expect(sent()).toEqual([READ_VERSION]); + expect(result.name).toBe('v2'); + }); + + it('the second save from the same read sends the version the first write returned, and lands', async () => { + const { dataSource, sent } = enforcingDataSource(rowsOf('r1')); + const save = mountHook(); + const read = await dataSource.findOne(OBJECT, 'r1'); + + const first = await landed(save({ dataSource, recordId: 'r1', payload: { name: 'v2' }, baseRecord: read })); + const second = save({ dataSource, recordId: 'r1', payload: { name: 'v3' }, baseRecord: read }); + + await waitFor(() => expect(dataSource.update).toHaveBeenCalledTimes(2)); + expect(sent()[1]).toBe(first.updated_at); + expect((await landed(second)).name).toBe('v3'); + expect(screen.queryByText('Keep editing')).toBeNull(); + }); + + it('a third save sends what the SECOND write returned: the last write, not the first', async () => { + const { dataSource, sent } = enforcingDataSource(rowsOf('r1')); + const save = mountHook(); + const read = await dataSource.findOne(OBJECT, 'r1'); + + await landed(save({ dataSource, recordId: 'r1', payload: { name: 'v2' }, baseRecord: read })); + const second = await landed(save({ dataSource, recordId: 'r1', payload: { name: 'v3' }, baseRecord: read })); + await landed(save({ dataSource, recordId: 'r1', payload: { name: 'v4' }, baseRecord: read })); + + expect(sent()[2]).toBe(second.updated_at); + }); + + it('another writer between the two saves still earns the conflict dialog', async () => { + const { dataSource, sent, otherWriter } = enforcingDataSource(rowsOf('r1')); + const save = mountHook(); + const read = await dataSource.findOne(OBJECT, 'r1'); + + const first = await landed(save({ dataSource, recordId: 'r1', payload: { name: 'v2' }, baseRecord: read })); + otherWriter('r1', { name: 'Theirs' }); + const second = save({ dataSource, recordId: 'r1', payload: { name: 'v3' }, baseRecord: read }); + + fireEvent.click(await screen.findByText('Keep editing')); + expect(await second).toEqual({ status: 'cancelled' }); + // The guard fired on the token this form last wrote, not on its stale read. + expect(sent()).toEqual([READ_VERSION, first.updated_at]); + }); + + it('after Overwrite, the next save sends the version the overwrite returned', async () => { + const { dataSource, sent, otherWriter } = enforcingDataSource(rowsOf('r1')); + const save = mountHook(); + const read = await dataSource.findOne(OBJECT, 'r1'); + otherWriter('r1', { name: 'Theirs' }); + + const first = save({ dataSource, recordId: 'r1', payload: { name: 'v2' }, baseRecord: read }); + fireEvent.click(await screen.findByText('Overwrite')); + const overwritten = await landed(first); + const next = await landed(save({ dataSource, recordId: 'r1', payload: { name: 'v3' }, baseRecord: read })); + + expect(sent()[2]).toBe(overwritten.updated_at); + expect(next.name).toBe('v3'); + }); + + it('a newer read of the record wins over the remembered version', async () => { + const { dataSource, sent, otherWriter } = enforcingDataSource(rowsOf('r1')); + const save = mountHook(); + const read = await dataSource.findOne(OBJECT, 'r1'); + + await landed(save({ dataSource, recordId: 'r1', payload: { name: 'v2' }, baseRecord: read })); + otherWriter('r1', { name: 'Theirs' }); + const reread = await dataSource.findOne(OBJECT, 'r1'); + await landed(save({ dataSource, recordId: 'r1', payload: { name: 'v3' }, baseRecord: reread })); + + expect(sent()[1]).toBe(reread.updated_at); + }); + + it('a version is remembered per record: another record sends its own read', async () => { + const { dataSource, sent } = enforcingDataSource(rowsOf('r1', 'r2')); + const save = mountHook(); + const read1 = await dataSource.findOne(OBJECT, 'r1'); + const read2 = await dataSource.findOne(OBJECT, 'r2'); + + await landed(save({ dataSource, recordId: 'r1', payload: { name: 'v2' }, baseRecord: read1 })); + await landed(save({ dataSource, recordId: 'r2', payload: { name: 'v2' }, baseRecord: read2 })); + + expect(sent()[1]).toBe(read2.updated_at); + }); + + it('a read without updated_at stays unguarded, even after a write returns one', async () => { + const { dataSource, sent } = enforcingDataSource(rowsOf('r1')); + const save = mountHook(); + const read = { id: 'r1', name: 'Mine' }; + + await landed(save({ dataSource, recordId: 'r1', payload: { name: 'v2' }, baseRecord: read })); + await landed(save({ dataSource, recordId: 'r1', payload: { name: 'v3' }, baseRecord: read })); + + expect(sent()).toEqual([undefined, undefined]); + }); + + it('a write result carrying no version: the next save falls back to the read', async () => { + // Not the measured server: a data source whose `update` resolves without + // the record's version. Nothing is invented in its place. + const update = vi.fn(async (_o: string, id: string | number, d: Row, _opts?: { ifMatch?: string }) => ({ id, ...d })); + const save = mountHook(); + const read = { id: 'r1', name: 'Mine', updated_at: READ_VERSION }; + + await landed(save({ dataSource: { update }, recordId: 'r1', payload: { name: 'v2' }, baseRecord: read })); + await landed(save({ dataSource: { update }, recordId: 'r1', payload: { name: 'v3' }, baseRecord: read })); + + expect(update.mock.calls.map((c) => c[3]?.ifMatch)).toEqual([READ_VERSION, READ_VERSION]); + }); +}); + +describe('ObjectForm edit: two submits from one mounted form (#10565)', () => { + it('the second submit carries the first write\'s version and lands without the dialog', async () => { + const { dataSource, sent } = enforcingDataSource(rowsOf('r1')); + const onSuccess = vi.fn(); + const { container } = render( + , + ); + const input = await waitFor(() => { + const el = container.querySelector('input[name="name"]') as HTMLInputElement | null; + if (!el) throw new Error('name not ready'); + return el; + }); + + fireEvent.change(input, { target: { value: 'v2' } }); + fireEvent.submit(container.querySelector('form') as HTMLFormElement); + await waitFor(() => expect(onSuccess).toHaveBeenCalledTimes(1)); + const first = onSuccess.mock.calls[0][0] as Row; + + fireEvent.change(input, { target: { value: 'v3' } }); + fireEvent.submit(container.querySelector('form') as HTMLFormElement); + await waitFor(() => expect(dataSource.update).toHaveBeenCalledTimes(2)); + + expect(sent()).toEqual([READ_VERSION, first.updated_at]); + await waitFor(() => expect(onSuccess).toHaveBeenCalledTimes(2)); + expect(screen.queryByText('Keep editing')).toBeNull(); + }); +}); diff --git a/packages/plugin-form/src/occSave.tsx b/packages/plugin-form/src/occSave.tsx index dae90cb65e..f0d66a0234 100644 --- a/packages/plugin-form/src/occSave.tsx +++ b/packages/plugin-form/src/occSave.tsx @@ -17,7 +17,9 @@ * forms were the only link in the chain not sending it. * * `useOccSave` centralises the guarded write: - * 1. `update` is issued with `ifMatch` = the `updated_at` the form read. + * 1. `update` is issued with `ifMatch` = the `updated_at` the form read — + * or, once this hook has itself saved that record, the version its last + * successful write returned (objectui#10565; see `versionToSend`). * 2. On `409 CONCURRENT_UPDATE` a dialog offers **Keep editing** (stay in * the form, draft intact) or **Overwrite** (retry re-keyed to the * server's `currentVersion` — "I know it changed, mine wins"). @@ -95,10 +97,57 @@ export interface OccSaveArgs { /** * The record as the form read it (`findOne` result) — supplies the * `updated_at` version token. Optional: without it the write is unguarded. + * It decides WHETHER the write is guarded; after this hook's own save of + * the same record it no longer decides WHICH version is sent (see + * `versionToSend`). */ baseRecord?: unknown; } +/** + * What this hook remembers about its last successful write to one record. + */ +interface WrittenVersion { + /** `occVersionOf(baseRecord)` when that write was made: the read it superseded. */ + readVersion: string; + /** `occVersionOf(result)`: the version that write returned. */ + version: string; +} + +/** One key per record, so a save to one record never carries another's version. */ +function recordKey(objectName: string, recordId: string | number): string { + return JSON.stringify([objectName, String(recordId)]); +} + +/** + * The `ifMatch` a save sends (objectui#10565). + * + * The forms hand in the record they READ as `baseRecord`, and a form that + * stays open after a save still holds that read. Its `updated_at` stops being + * current the moment this hook's own write lands: the server stamps a new one + * and returns it with the saved record. Sending the read's token again made + * the second save from a form that stays open answer `409 CONCURRENT_UPDATE`, + * and the user met the conflict dialog over their own earlier save. That was + * measured once against a real ObjectStack stack; the reading is on + * objectui#10565 and nothing in this package re-derives it. The pins in + * `occSave.secondSave.test.tsx` hold this rule against a double shaped like + * that measurement. + * + * So once a write through this hook has landed, the version it returned is + * sent instead, but only while the form still holds the SAME read. A + * `baseRecord` with a different token is a newer read of the record (a + * reload), and its token wins over anything remembered. With no token on + * the read the save stays unguarded exactly as before: this changes which + * version a guarded save sends, never whether a save is guarded. + */ +function versionToSend( + written: WrittenVersion | undefined, + readVersion: string | undefined, +): string | undefined { + if (!readVersion) return undefined; + return written && written.readVersion === readVersion ? written.version : readVersion; +} + interface ConflictState { /** Latest server-side version from the 409, for the overwrite retry. */ currentVersion?: string; @@ -144,6 +193,10 @@ export function useOccSave(): { const [conflict, setConflict] = React.useState(null); // Resolver for the promise `saveWithOcc` awaits while the dialog is open. const decisionRef = React.useRef<((overwrite: boolean) => void) | null>(null); + // Per record, the version this hook's last successful write returned + // (objectui#10565). It lives exactly as long as the form that mounted this + // hook; a remounted form reads the record again. + const writtenRef = React.useRef(new Map()); const settle = React.useCallback((overwrite: boolean) => { decisionRef.current?.(overwrite); @@ -157,11 +210,22 @@ export function useOccSave(): { const saveWithOcc = React.useCallback( async ({ dataSource, objectName, recordId, payload, baseRecord }: OccSaveArgs): Promise => { - const ifMatch = occVersionOf(baseRecord); + const key = recordKey(objectName, recordId); + const readVersion = occVersionOf(baseRecord); + const ifMatch = versionToSend(writtenRef.current.get(key), readVersion); + // After a write lands, remember the version it returned for the next + // save of this record. A result without one clears the entry, so that + // save falls back to the read's token rather than to an older write's. + const remember = (result: unknown) => { + const version = occVersionOf(result); + if (readVersion && version) writtenRef.current.set(key, { readVersion, version }); + else writtenRef.current.delete(key); + }; try { const result = await dataSource.update( objectName, recordId, payload, ifMatch ? { ifMatch } : undefined, ); + remember(result); return { status: 'saved', result }; } catch (err) { if (!isConcurrentUpdateError(err)) throw err; @@ -190,6 +254,9 @@ export function useOccSave(): { const result = await dataSource.update( objectName, recordId, payload, cv ? { ifMatch: cv } : undefined, ); + // The overwrite is a write like any other: the next save sends the + // version it returned, not the stale read and not the 409's version. + remember(result); return { status: 'saved', result }; } },