From ce69f0a5b396e98824b674ac8b087ba7a5f8a320 Mon Sep 17 00:00:00 2001 From: Benoit Travers Date: Sat, 8 Aug 2026 01:38:56 +0200 Subject: [PATCH] fix: reject an unpatchable patch key instead of dropping it silently MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `update()` returned `Ok` while discarding any key it could not apply — a key that is immutable, computed, or not a field of the entity at all. The caller asked for a change, got a success, and the change never happened. The patch type already excluded all three, but TypeScript's excess-property check only fires on object literals, so the common adapter shape — building a patch as a `Record` from a request body — evaded it entirely and the key vanished into a passing `Result`. A rejected key now comes back as an `InvalidEntity` carrying that key in `path`, one issue per offending key, so an adapter maps it to a 422 naming the field. Every offender reports, the same rule the invariants follow. `make` stays lenient by design: it still ignores extra keys, so a stored row carrying computed columns round-trips. Rehydrating data and patching it are different acts — one heals what is already written, the other states an intent. Closes #42 Co-Authored-By: Claude Opus 5 (1M context) --- .changeset/olive-hounds-search.md | 26 ++++++++++++++ docs/reference/declaration.md | 12 +++---- docs/reference/entry-points.md | 17 +++++++-- docs/reference/errors.md | 8 +++++ docs/tutorial/getting-started.md | 5 +-- packages/entity/src/computed.spec.ts | 21 +++++++++-- packages/entity/src/crud.spec.ts | 47 ++++++++++++++++++++++-- packages/entity/src/entity.ts | 53 ++++++++++++++++++++++------ 8 files changed, 162 insertions(+), 27 deletions(-) create mode 100644 .changeset/olive-hounds-search.md diff --git a/.changeset/olive-hounds-search.md b/.changeset/olive-hounds-search.md new file mode 100644 index 0000000..a872577 --- /dev/null +++ b/.changeset/olive-hounds-search.md @@ -0,0 +1,26 @@ +--- +"@btravstack/entity": minor +--- + +`update()` rejects a patch key it cannot apply, instead of dropping it silently. + +A patch may now carry only keys `updateInput` accepts. A key that is +`immutable`, `computed`, or not a field of the entity at all comes back as an +`InvalidEntity` with that key in `path` — every offending key reports, not +just the first. + +All three were silently discarded before while `update` returned `Ok`: the +caller asked for a change, got a success, and the change never happened. The +patch type already excluded them, but TypeScript's excess-property check only +fires on object literals, so the common adapter shape — building a patch as a +`Record` from a request body — evaded it entirely and the key +vanished into a passing `Result`. + +`make` is deliberately unchanged: it still ignores extra keys, so a stored row +carrying computed columns round-trips. Rehydrating data and patching it are +different acts — one heals what is already written, the other states an intent. + +**Breaking** for code that relied on the drop, most likely +`update(someWholeOutputObject)`. Patch only the fields you mean to change, or +narrow the object first — `updateInput.parse(body)` strips unknown keys and +gives you a patch that is accepted by construction. diff --git a/docs/reference/declaration.md b/docs/reference/declaration.md index 3fb4313..177cc28 100644 --- a/docs/reference/declaration.md +++ b/docs/reference/declaration.md @@ -43,12 +43,12 @@ Four names are reserved, because an entity installs them on every instance: ### `options` -| Option | Type | Effect | -| ------------ | ---------------------------------- | -------------------------------------------------------------------------------- | -| `generated` | `readonly (keyof fields)[]` | omitted from `createInput`; supplied by a factory's generators | -| `immutable` | `readonly (keyof output)[]` | omitted from `updateInput`; `update()` drops them even if smuggled in at runtime | -| `computed` | `{ [name]: Entity.ComputedField }` | derived fields; added to `output`, re-derived on every construction | -| `invariants` | `readonly Entity.Invariant[]` | rules spanning two or more declared fields; any failing rule rejects | +| Option | Type | Effect | +| ------------ | ---------------------------------- | ---------------------------------------------------------------------------------- | +| `generated` | `readonly (keyof fields)[]` | omitted from `createInput`; supplied by a factory's generators | +| `immutable` | `readonly (keyof output)[]` | omitted from `updateInput`; `update()` rejects them even if smuggled past the type | +| `computed` | `{ [name]: Entity.ComputedField }` | derived fields; added to `output`, re-derived on every construction | +| `invariants` | `readonly Entity.Invariant[]` | rules spanning two or more declared fields; any failing rule rejects | `generated` and `immutable` are keyed off the field names, so a typo is a compile error rather than a silently-inert entry. diff --git a/docs/reference/entry-points.md b/docs/reference/entry-points.md index b11efe2..e0e934f 100644 --- a/docs/reference/entry-points.md +++ b/docs/reference/entry-points.md @@ -54,8 +54,21 @@ carrying computed columns round-trips. ## `entity.update(patch)` → `Result` Returns a **new** entity. Re-runs the invariants and re-derives the computed -fields. `immutable` and `computed` fields are absent from the patch type and -dropped at runtime. +fields. + +The patch must contain only keys `updateInput` accepts. A key that is +`immutable`, `computed`, or not a field of the entity at all is **rejected** +with an `InvalidEntity` carrying that key in `path` — every offending key +reports, not just the first. They are absent from the patch type too, but the +compile-time guard only fires on object literals: an adapter that builds its +patch as a `Record` gets no excess-property check, which is +why the runtime check exists. + +This is the opposite of `make`, deliberately. `make` ignores extra keys so a +stored row carrying computed columns round-trips; `update` refuses them so a +change the caller asked for cannot silently not happen. Rehydrating data and +patching it are different acts: one heals what is already written, the other +states an intent. ## `entity.toJSON()` → `DeepReadonly` diff --git a/docs/reference/errors.md b/docs/reference/errors.md index cee3ce8..c3373a8 100644 --- a/docs/reference/errors.md +++ b/docs/reference/errors.md @@ -74,6 +74,7 @@ uses both. | ------------------------------------------------- | ---------------------------------------------- | | a field fails its own schema | `InvalidEntity`, issue has a `path` | | a broken `invariants` rule | `InvalidEntity`, issue has no `path` | +| a patch key `updateInput` does not accept | `InvalidEntity`, one issue per key at `[key]` | | a union payload's discriminant matches nobody | `InvalidEntity`, one issue at `[discriminant]` | | `computed` output failing its own schema | **defect** | | a `computed` function throwing | **defect** | @@ -81,6 +82,13 @@ uses both. | subclassing an entity | **defect** | | two union members claiming one discriminant value | **defect**, thrown at declaration time | +A rejected patch key reports which of the three kinds it is — `Immutable field +— cannot be patched`, `Computed field — cannot be patched, it is re-derived +from its sources`, or `Unknown field for Rental` — at the key's own path, so a +`PATCH` adapter maps it to a 422 naming the field. See +[`entity.update(patch)`](/reference/entry-points#entity-update-patch-result-someentity-invalidentity) +for why this is stricter than `make`. + The union's "Invalid discriminant" issue lists the values it knows — `Invalid discriminant "robot"; expected one of "user", "service_account"` — and sits at the discriminant's own path, so it keys a field-level response diff --git a/docs/tutorial/getting-started.md b/docs/tutorial/getting-started.md index 88a4ee5..4537f74 100644 --- a/docs/tutorial/getting-started.md +++ b/docs/tutorial/getting-started.md @@ -89,8 +89,9 @@ class Organization extends Entity("Organization")( - `generated` drops those fields from `createInput` — a create request cannot carry them. -- `immutable` drops them from `updateInput` — and `update()` discards them at - runtime even if something smuggles them past the type. +- `immutable` drops them from `updateInput` — and `update()` rejects them at + runtime even if something smuggles them past the type, so a change that + cannot happen is reported rather than quietly ignored. Both are keyed off the field names, so a typo is a compile error rather than a silently-inert entry. diff --git a/packages/entity/src/computed.spec.ts b/packages/entity/src/computed.spec.ts index 301476d..32f40b8 100644 --- a/packages/entity/src/computed.spec.ts +++ b/packages/entity/src/computed.spec.ts @@ -72,11 +72,26 @@ test("make heals a row written before the computed field existed", () => { expect(p.initials).toBe("AL"); }); -test("computed fields are absent from updateInput and dropped if smuggled in", () => { +test("computed fields are absent from updateInput and rejected if smuggled in", () => { expect(Object.keys(Person.updateInput.shape).toSorted()).toEqual(["first", "last"]); const p = Person.make(raw).getOrThrow(); - const lied = p.update({ fullName: "LIES" } as never).getOrThrow(); - expect(lied.fullName).toBe("Ada Lovelace"); + // patching a derived value is a contradiction — it would be overwritten by + // the next derivation — so it is reported rather than quietly discarded + const messages = p.update({ fullName: "LIES" } as never).match({ + ok: () => ["WRONGLY ACCEPTED"], + errCases: (m) => m.with(P.tag("InvalidEntity"), (e) => e.issues.map((i) => i.message)), + defect: () => ["DEFECT"], + }); + expect(messages).toEqual([ + "Computed field — cannot be patched, it is re-derived from its sources", + ]); + expect(p.fullName).toBe("Ada Lovelace"); +}); + +test("make still ignores extra keys, so a stored row round-trips", () => { + // `update` is strict about caller intent; `make` stays lenient about stored + // data, which carries computed columns and may predate a field + expect(Person.make({ ...raw, legacyColumn: "x" }).getOrThrow().fullName).toBe("Ada Lovelace"); }); test("toJSON round-trips through make", () => { diff --git a/packages/entity/src/crud.spec.ts b/packages/entity/src/crud.spec.ts index fcded80..8e1712c 100644 --- a/packages/entity/src/crud.spec.ts +++ b/packages/entity/src/crud.spec.ts @@ -72,10 +72,51 @@ test("update returns a new instance and leaves the original untouched", () => { expect(renamed.id).toBe(org.id); }); -test("update ignores an immutable field smuggled in at runtime", () => { +/** Every issue a rejected patch reported, as `path: message` pairs. */ +const rejectionOf = (result: ReturnType): readonly string[] => + result.match({ + ok: () => ["WRONGLY ACCEPTED"], + errCases: (m) => + m.with(P.tag("InvalidEntity"), (e) => + e.issues.map((i) => `${String(i.path?.[0] ?? "")}: ${i.message}`), + ), + defect: () => ["DEFECT"], + }); + +test("update rejects an immutable field smuggled in at runtime", () => { + const org = createOrg(input).getOrThrow(); + const rejected = org.update({ slug: "other" } as never); + // dropping it silently was the old behaviour: the caller asked for a change, + // got `Ok`, and the change never happened + expect(rejectionOf(rejected)).toEqual(["slug: Immutable field — cannot be patched"]); + expect(org.slug).toBe("acme"); +}); + +test("update rejects a key the entity does not declare", () => { + const org = createOrg(input).getOrThrow(); + // the reported case: an adapter builds `Record`, so no + // excess-property check fires, and the key used to vanish into a passing + // `Result` + expect(rejectionOf(org.update({ calculatedAmount: 500 } as never))).toEqual([ + "calculatedAmount: Unknown field for Organization", + ]); +}); + +test("update reports every unpatchable key at once, like the invariants do", () => { + const org = createOrg(input).getOrThrow(); + expect( + rejectionOf(org.update({ slug: "other", nope: 1, name: "Fine" } as never)).toSorted(), + ).toEqual( + [ + "slug: Immutable field — cannot be patched", + "nope: Unknown field for Organization", + ].toSorted(), + ); +}); + +test("a patch of only declared, mutable fields is unaffected", () => { const org = createOrg(input).getOrThrow(); - const updated = org.update({ slug: "other" } as never).getOrThrow(); - expect(updated.slug).toBe("acme"); + expect(org.update({ name: "Renamed" as never }).getOrThrow().name).toBe("Renamed"); }); test("update re-runs invariants", () => { diff --git a/packages/entity/src/entity.ts b/packages/entity/src/entity.ts index cb756fe..f8ea3e4 100644 --- a/packages/entity/src/entity.ts +++ b/packages/entity/src/entity.ts @@ -112,12 +112,11 @@ export function Entity(tag: Tag) { const immutableKeys = options?.immutable ?? []; /** - * Every key `update` refuses: the declared immutable ones, plus the + * Every key `updateInput` omits: the declared immutable ones, plus the * computed ones. A computed field is not patchable because it is derived — * `update` re-runs `from` like every other construction path, so patching * it would only be overwritten. Typed at the widened runtime element type - * so both uses below — the `.omit()` mask and `update`'s drop-list — take - * it without a cast. + * so the `.omit()` mask takes it without a cast. */ const frozenKeys: readonly PropertyKey[] = [ ...immutableKeys, @@ -138,6 +137,34 @@ export function Entity(tag: Tag) { const dataKeys = Object.keys(output.shape) as unknown as readonly (keyof OutputShape)[]; + /** + * Why `update` refuses this key, or `undefined` if it accepts it. + * + * The three answers are the three ways a patch key can fail to be part of + * `updateInput`, which is the schema of what a caller may send to update. + * All three were silently dropped before, which is the one outcome that is + * neither the change the caller asked for nor an error: an adapter that + * builds its patch as a `Record` gets no excess-property + * check, so the key vanished into a passing `Result` and surfaced later as + * missing data. Reported here, it lands at the call site. + * + * `make` deliberately stays lenient in the other direction — a stored row + * carries computed columns and may predate a field, so extra keys are + * ignored there. Rehydrating data and patching it are different acts: one + * heals what is already written, the other states an intent. + */ + const immutableNames = new Set(immutableKeys as readonly string[]); + const computedNames = new Set(computedFields.map(([key]) => key)); + const declaredNames = new Set(dataKeys.map(String)); + + const unpatchable = (key: string): string | undefined => { + if (immutableNames.has(key)) return "Immutable field — cannot be patched"; + if (computedNames.has(key)) { + return "Computed field — cannot be patched, it is re-derived from its sources"; + } + return declaredNames.has(key) ? undefined : `Unknown field for ${tag}`; + }; + // Each field's schema goes to `deepFreeze` with its value, so the walk can // skip a passed-through value wherever it sits — not only when it *is* the // field. `freeze.ts` promises a `z.custom(...)`/`z.instanceof(...)` value is @@ -384,15 +411,19 @@ export function Entity(tag: Tag) { /** a partial of the mutable fields → a NEW entity */ update(this: Base, patch: PatchOf): Result { - const current = project(this) as Record; - const applied = { ...current }; - for (const [k, v] of Object.entries(patch as object)) { - // frozen keys — declared immutable, or computed — are a - // compile error already; drop them at runtime too, so a patch that - // reached here as `unknown` cannot desynchronise a computed field - // from the source it was derived from. - if (!frozenKeys.includes(k)) applied[k] = v; + const entries = Object.entries(patch as object); + // Every offending key reports, not just the first — the same rule the + // invariants follow. `path` carries the key, so an adapter can key a + // field-level response off it exactly as it does for a parse failure. + const rejected = entries + .map(([key]) => [key, unpatchable(key)] as const) + .filter((pair): pair is readonly [string, string] => pair[1] !== undefined) + .map(([key, message]) => ({ path: [key] as readonly PropertyKey[], message })); + if (rejected.length > 0) { + return Err(new InvalidEntity({ entity: tag, issues: rejected })); } + const applied = { ...(project(this) as Record) }; + for (const [k, v] of entries) applied[k] = v; const Ctor = this.constructor as unknown as { make: (state: unknown) => Result; };