|
| 1 | +# Notes — shipping `PUT /users/:id` |
| 2 | + |
| 3 | +## The plan |
| 4 | + |
| 5 | +Two files, one small change each: a `updateUser` helper in `db/store.js`, and a |
| 6 | +`PUT /:id` route in `routes/users.js`. I read `tests/update-user.test.js` first |
| 7 | +and let it define the contract — 200 with the updated body, 404 for an unknown |
| 8 | +id, 400 for a missing field — then planned the edges the tests don't cover. |
| 9 | + |
| 10 | +## Choices |
| 11 | + |
| 12 | +**Full replace, not a partial update.** The provided test sends `{ name: "Only |
| 13 | +a name" }` and expects a 400, so `PUT` here means "replace both fields." That |
| 14 | +matches `PUT` semantics anyway; a merge-style partial update would be a separate |
| 15 | +`PATCH` endpoint. Both `name` and `email` must be non-empty strings, and the |
| 16 | +email is shape-checked against a deliberately loose pattern — enough to catch an |
| 17 | +obvious typo without pretending to implement RFC 5322. |
| 18 | + |
| 19 | +**Validate the body before looking the user up.** A malformed request is a client |
| 20 | +error whether or not the id happens to exist, so the 400 fires first. The |
| 21 | +provided tests pass under either ordering (the 404 case sends a valid body, the |
| 22 | +400 case targets an id that exists), so this was a judgement call, not a |
| 23 | +constraint. |
| 24 | + |
| 25 | +**Validation in the route, persistence in the store.** `updateUser` returns |
| 26 | +`undefined` when the id is missing rather than throwing or sending a response, |
| 27 | +so the "not found" decision stays in the route layer — exactly how `GET /:id` |
| 28 | +already works. It reuses `getUserById` instead of a second `users.find(...)`, |
| 29 | +and mutates the record in place so `GET /users` reflects the change immediately. |
| 30 | + |
| 31 | +**A known inconsistency, left alone on purpose.** `PUT /users/abc` returns 400 |
| 32 | +("id must be an integer"), but `GET /users/abc` returns 404 today — `Number("abc")` |
| 33 | +is `NaN`, so the lookup just misses. Making the two agree means touching `GET`, |
| 34 | +which is outside this change; flagging it here beats silently widening the diff. |
| 35 | + |
| 36 | +## Model and commits |
| 37 | + |
| 38 | +Written with Claude Opus 5 in Claude Code, planned in plan mode before any edit. |
| 39 | +Three commits, each independently reviewable: the store helper, then the route, |
| 40 | +then this write-up. |
| 41 | + |
| 42 | +## What the review caught |
| 43 | + |
| 44 | +Two things worth fixing. The first draft leaned on the same falsy check as |
| 45 | +`POST /users` (`!name || !email`), which lets `{ name: " " }` through as a valid |
| 46 | +name — hence `isNonEmptyString` and trimming the values before they reach the |
| 47 | +store. The second was the invalid-id gap above: I noticed it only when writing |
| 48 | +the smoke tests by hand, and chose to document rather than quietly change `GET`. |
| 49 | + |
| 50 | +Verified with `npm test` (9/9), `npm run lint` clean, and a manual `curl` pass |
| 51 | +over all five response paths plus `GET /users` to confirm the update persisted. |
0 commit comments