fix(table): return 400 from checkValidId on invalid primary keys

[cb1kenobi]

Summary

A client PATCH/PUT to /<Resource>/<literal> (where <literal> is undefined, null, NaN, true, etc. — common artifacts of template-stringifying a missing value into a URL) used to surface inconsistently depending on the primary key type:

PK type	Pre-fix
Numeric (Int/Long/Float/BigInt)	/undefined, /NaN, /true → 400 (via coerceType regex). /null → 500 — coerceType silently returned null, checkValidId then threw a plain Error.
Any	All of /undefined, /null, /true → 500 (autoCast produces null/boolean; checkValidId rejects). /NaN → 204, silently persisted with NaN id — data corruption.
String	Accepts all literals as legitimate string ids (correct — unchanged).

Per @kriszyp's review on the previous approach, the right boundary to fix this is checkValidId itself: convert all throws to ClientError(400) and explicitly reject NaN. That turns every invalid-PK code path into a clean 400 regardless of how the bad value arrived (URL, autoCast, direct API call).

Diff in one place

resources/Table.ts's checkValidId — five throw new Error(...) → throw new ClientError(..., 400), plus if (isNaN(id)) throw ... added to the number branch.

Behavior matrix after this PR

PK type	/undefined	/null	/NaN	/true
Numeric	400	400 (was 500)	400	400
Any	400 (was 500)	400 (was 500)	400 (was 204 — corruption!)	400 (was 500)
String	204 (round-trips)	204	204	204

Test plan

• New unitTests/apiTests/undefinedIdInUrl-test.mjs — 18 integration tests via real HTTP server, covering all combinations above. Includes a sanity test that PUT /DogIntPK/42 still works and an explicit non-regression for String PKs.
• New tables DogIntPK, DogStringPK, DogAnyPK in the test schema.
• npm run test:unit:resources — 501 passing, no regressions.
• npm run lint:required and npm run format:write — clean.
• No associated ticket — discovered during user's hands-on testing.

Generated by Claude (Opus 4.7, 1M context). Force-push replaces the rejected previous approach (broad coerceId rejection) with the targeted fix at the right boundary.

[claude]

Reviewed; no blockers found.

[kriszyp]

If a deeper goal is better error response if an undefined PK (value, not string) is used, we could update checkValidId to throw with a 400 status code.

BTW, is there an associated ticket with this?

[kriszyp]

These literals are already being rejected in coerceType, aren't they? (line 4723 and rejectNaN). The only literal that seems to have the possibility of coercion is 'null', and it seems like the correct fix would be to reject 'null' conditionally in coerceType based on the nullable property of the attribute.

[cb1kenobi]

Yes, coerceType() does throw for undefined, but not null. Handling null is an easy fix. I wrote the test first and I thought I was able to reproduce it. Now I'm tracing through the REST handler because I'm confident there's a bug. I'm wondering if undefined is getting stringified into null and we know 'null' returns null.

[cb1kenobi]

Reworked per your review feedback — force-pushed. The branch now contains a single commit that updates checkValidId to throw ClientError(400) (with NaN also rejected), instead of trying to gate at coerceId.

Summary table of probed behavior (live HTTP requests, all PK types × all literals) is in the PR description. The two corruption cases this catches that weren't in the original report:

• /HasBigInt/null and /DogIntPK/null were already returning 500 — coerceType's if (value === 'null') return null swallows the bad input. (You're right that "undefined" was already throwing — only "null" is the gap for typed numeric PKs.)
• /DogAnyPK/NaN was silently persisting a record keyed by NaN. autoCast("NaN") returns the actual NaN number, checkValidId's typeof id === 'number' branch returned true without checking isNaN. Caught by the new isNaN check.

No associated ticket — let me know if you'd like me to file one.

[cb1kenobi]

@kriszyp ok, it's fixed for real this time :)

[kriszyp]

Looks good!