Interviews API: a PATCH with an explicit null answers 500 #436

New issue

Closed

opened 2026-10-01 10:46:41 +00:00 by tiagoagueda · 0 comments

tiagoagueda commented

2026-10-01 10:46:41 +00:00

Owner

Found on 2026-09-29 in a read-only review of postulo at 95348908d, comparing how the three PATCH calls read their payloads and sending each interview field as null through the test client.

Checked again on 2026-10-01 against main at 0a5cad587: still present.

InterviewPatch declares every field X | None = None, and change_interview reads it with payload.dict(exclude_unset=True), which drops the fields a client left out but keeps the ones it sent as null. Three of them then crash rather than being refused: the client gets Django's HTML error page with a 500 instead of a problem document. patch_company skips None values, and change_reminder skips them wherever null has no meaning of its own; this call does not.

Where: api/routers/interviews.py:136-154, change_interview; api/schemas.py:407-413, InterviewPatch.
What happens (each checked against the snapshot):
- {"starts_at": null}: line 142 takes None as the new start, and line 144 computes None + interview.duration, a TypeError: 500.
- {"location": null} or {"notes": null}: line 149 sets None on a NOT NULL text column and interview.save() raises IntegrityError: 500.
- {"kind": null} is refused with a 422 by choice_or_422; {"ends_at": null} and {"contact_ids": null} are treated as "unchanged", which is what patch_company does for every field.
Why it matters: a generated or generic client that sends every field of a patch, with null for "leave it", gets a crash for an ordinary request, and each one is logged as a server error. The same call treats null three different ways depending on the field.
Fix: drop None values after payload.dict(exclude_unset=True), as patch_company does (api/routers/companies.py:94-96), so null means "unchanged" for every field; or refuse a null with a 422 naming the field. Either way, document it on InterviewPatch.

Done when

tests/test_api.py sends PATCH /api/v1/interviews/{id} with each field of InterviewPatch set to null, parametrised over the fields, and asserts a 200 (or a 422 problem document) and an unchanged interview, never a 500.

Found on 2026-09-29 in a read-only review of postulo at 95348908d, comparing how the three `PATCH` calls read their payloads and sending each interview field as `null` through the test client. Checked again on 2026-10-01 against `main` at `0a5cad587`: still present. `InterviewPatch` declares every field `X | None = None`, and `change_interview` reads it with `payload.dict(exclude_unset=True)`, which drops the fields a client left out but keeps the ones it sent as `null`. Three of them then crash rather than being refused: the client gets Django's HTML error page with a 500 instead of a problem document. `patch_company` skips `None` values, and `change_reminder` skips them wherever `null` has no meaning of its own; this call does not. - **Where:** `api/routers/interviews.py:136-154`, `change_interview`; `api/schemas.py:407-413`, `InterviewPatch`. - **What happens** (each checked against the snapshot): - `{"starts_at": null}`: line 142 takes `None` as the new start, and line 144 computes `None + interview.duration`, a `TypeError`: 500. - `{"location": null}` or `{"notes": null}`: line 149 sets `None` on a `NOT NULL` text column and `interview.save()` raises `IntegrityError`: 500. - `{"kind": null}` is refused with a 422 by `choice_or_422`; `{"ends_at": null}` and `{"contact_ids": null}` are treated as "unchanged", which is what `patch_company` does for every field. - **Why it matters:** a generated or generic client that sends every field of a patch, with `null` for "leave it", gets a crash for an ordinary request, and each one is logged as a server error. The same call treats `null` three different ways depending on the field. - **Fix:** drop `None` values after `payload.dict(exclude_unset=True)`, as `patch_company` does (`api/routers/companies.py:94-96`), so `null` means "unchanged" for every field; or refuse a `null` with a 422 naming the field. Either way, document it on `InterviewPatch`. ## Done when - `tests/test_api.py` sends `PATCH /api/v1/interviews/{id}` with each field of `InterviewPatch` set to `null`, parametrised over the fields, and asserts a 200 (or a 422 problem document) and an unchanged interview, never a 500.

tiagoagueda added this to the 0.6.0 milestone

2026-10-01 10:46:41 +00:00

tiagoagueda added the

tier

bug

labels

2026-10-01 10:46:41 +00:00

tiagoagueda referenced this issue from a commit

2026-10-03 22:56:00 +00:00

Treat a null in an interview patch as "unchanged"

tiagoagueda closed this issue

2026-10-03 22:56:00 +00:00