fix: update() rejects an unpatchable patch key instead of dropping it silently - #44
Merged
Merged
Conversation
`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<string, unknown>` 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) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes Entity.update(patch) so it no longer returns Ok while silently discarding patch keys it can’t apply (unknown fields, immutable fields, or computed fields). Instead, update() now rejects such patches with an InvalidEntity containing one issue per offending key (with path: [key]), while keeping make() intentionally lenient about extra keys for stored-row rehydration.
Changes:
- Add runtime validation in
update()to reject unknown / immutable / computed patch keys and return anInvalidEntitylisting all offending keys. - Update and expand tests to pin the new rejection behavior (including “all offenders at once”) and to ensure
make()remains lenient about extra keys. - Update tutorial/reference docs and add a changeset documenting the (intentional) breaking behavior change.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| packages/entity/src/entity.ts | Adds unpatchable() key classification and makes update() reject unpatchable patch keys with per-key issues. |
| packages/entity/src/crud.spec.ts | Rewrites/extends tests to assert update() rejects immutable + unknown keys and reports all offending keys. |
| packages/entity/src/computed.spec.ts | Updates computed-field patch test to assert rejection and adds a pin that make() still ignores extra keys. |
| docs/tutorial/getting-started.md | Updates tutorial wording to reflect update() rejecting immutable keys at runtime. |
| docs/reference/errors.md | Documents the new “rejected patch key” InvalidEntity case and its per-key path. |
| docs/reference/entry-points.md | Clarifies update() strictness vs make() leniency and why runtime checks exist beyond TS excess-property checks. |
| docs/reference/declaration.md | Updates the options table to reflect rejection (not silent dropping) for immutable keys in update(). |
| .changeset/olive-hounds-search.md | Adds a minor-version changeset describing the behavior change and migration guidance. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #42.
The bug
update()returnedOkwhile discarding any key it could not apply. Reproduced all three kinds against the current source before touching anything:calculatedAmounton aRental)Ok, key gonemakeid)Ok, unchangedfrozenKeysfiltershout)Ok, unchangedThe issue reports the first. The other two are the same failure: the caller asked for a change, got a success, and the change never happened.
As #42 notes, the type-level guard does not cover this —
PatchOfrejects all three in an object literal, but TypeScript's excess-property check does not survive the shape real adapters have (Record<string, unknown>built conditionally from a request body), which is exactly where the key silently disappeared.The fix
A patch may carry only keys
updateInputaccepts. Anything else comes back as anInvalidEntitywith the key inpath— one issue per offending key, every offender reported, the same rule the invariants follow:The reporter's union design is now expressible: patching a
Rentalwith a co-owner-only field fails and maps to a 422, while theCoOwnervariant accepts the identical patch. Verified both, including through theRecord<string, unknown>adapter path.makeis deliberately unchanged — it still ignores extra keys so a stored row carrying computed columns round-trips. That asymmetry is now stated in the reference: rehydrating data and patching it are different acts, one heals what is already written, the other states an intent.Scope note — this goes past what the issue asked
#42 asks for unknown keys. I extended it to immutable and computed keys because the reasoning applies verbatim, and two tests previously pinned that silent drop (
crud.spec.ts,computed.spec.ts); both are rewritten to pin rejection. The guarantee those tests protected — an immutable or computed field cannot be changed through a patch — is preserved and strengthened: it still cannot change, and now the caller is told. Happy to narrow this to unknown-keys-only if you would rather keep the drop for the other two.Breaking
Code relying on the drop breaks, most plausibly
update(someWholeOutputObject). No documented or example usage does this — every one passes a narrow partial — and the changeset documents the migration (patch only what you mean to change, or narrow withupdateInput.parse(body)first).Test plan
makestays lenient about extra keys, so the asymmetry cannot regressformat --check·lint·typecheck(all six targets incl. the 5.9.3 consumer pass) ·test(161 package + 22 example) ·knip·build— green in CI order🤖 Generated with Claude Code