API: a refused company or interview write still keeps the changes it made before the refusal #435

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, reading the order of saves and checks in the API's write calls and sending refused writes through the test client.

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

Three API calls save first and check the rest afterwards. A refusal is an HttpError, which django-ninja's exception handler (api/problems.py:209-222) turns into a response inside the view, so ATOMIC_REQUESTS sees a view that returned normally and commits. The client is told 422, and half of what it sent is kept. The applications router states the rule these three break: "Looked up before anything is made, so a refusal leaves no company behind it" (api/routers/applications.py:94).

Where:
- api/routers/companies.py:54-70, add_company: finds or makes the company, sets website, careers_url, location, notes and kind, saves, adds industries, and only then calls identifiers_or_422.
- api/routers/companies.py:89-102, patch_company: saves every changed field and replaces the industries before identifiers_or_422(..., replace=True).
- api/routers/interviews.py:136-154, change_interview: saves kind, location and notes (interview.save(), line 150) before _contacts (line 152) refuses a contact who is not at the application's company.
What happens (each checked against the snapshot with the test client):
- POST /api/v1/companies with name: "Mesa", a website and an identifier {"scheme": "lei", "value": "not-a-lei"} answers 422, and the company Mesa with that website exists afterwards. When the name matched an existing company, that company's fields have been overwritten instead.
- PATCH /api/v1/companies/{id} with name: "Renamed" and a malformed identifier answers 422, and the company is called Renamed; industries sent in the same call have replaced the old ones.
- PATCH /api/v1/interviews/{id} with location: "Moved" and a contact_ids entry that is not at the company answers 422, and the location is Moved.
Why it matters: a client that is told its write was refused reasonably corrects it and sends it again, or gives up, believing nothing changed; the record has in fact changed, silently, with no timeline entry for it. A company created by a refused POST is a stray record the person never asked for.
Fix: do every check that can refuse before the first write: clean the identifiers first (split the validation half of jobs/services.py:set_identifiers into a function that returns the cleaned list or raises, and call it at the top of both company calls), and resolve _contacts before interview.save(). Where reordering is awkward, wrap the body in with transaction.atomic():, so the HttpError rolls back the savepoint before ninja renders it. Rolling back the whole request from the error handler instead would be simpler but wrong: the rate limits count in the database cache inside the same transaction, and a refused request must still be counted.

Done when

tests/test_api.py sends each of the three refused writes above and asserts a 422 and that the company or interview is exactly as it was before (no new company, same name, same industries, same location).

Found on 2026-09-29 in a read-only review of postulo at 95348908d, reading the order of saves and checks in the API's write calls and sending refused writes through the test client. Checked again on 2026-10-01 against `main` at `0a5cad587`: still present. Three API calls save first and check the rest afterwards. A refusal is an `HttpError`, which django-ninja's exception handler (`api/problems.py:209-222`) turns into a response inside the view, so `ATOMIC_REQUESTS` sees a view that returned normally and commits. The client is told 422, and half of what it sent is kept. The applications router states the rule these three break: "Looked up before anything is made, so a refusal leaves no company behind it" (`api/routers/applications.py:94`). - **Where:** - `api/routers/companies.py:54-70`, `add_company`: finds or makes the company, sets `website`, `careers_url`, `location`, `notes` and `kind`, saves, adds industries, and only then calls `identifiers_or_422`. - `api/routers/companies.py:89-102`, `patch_company`: saves every changed field and replaces the industries before `identifiers_or_422(..., replace=True)`. - `api/routers/interviews.py:136-154`, `change_interview`: saves `kind`, `location` and `notes` (`interview.save()`, line 150) before `_contacts` (line 152) refuses a contact who is not at the application's company. - **What happens** (each checked against the snapshot with the test client): - `POST /api/v1/companies` with `name: "Mesa"`, a `website` and an identifier `{"scheme": "lei", "value": "not-a-lei"}` answers 422, and the company `Mesa` with that website exists afterwards. When the name matched an existing company, that company's fields have been overwritten instead. - `PATCH /api/v1/companies/{id}` with `name: "Renamed"` and a malformed identifier answers 422, and the company is called `Renamed`; industries sent in the same call have replaced the old ones. - `PATCH /api/v1/interviews/{id}` with `location: "Moved"` and a `contact_ids` entry that is not at the company answers 422, and the location is `Moved`. - **Why it matters:** a client that is told its write was refused reasonably corrects it and sends it again, or gives up, believing nothing changed; the record has in fact changed, silently, with no timeline entry for it. A company created by a refused `POST` is a stray record the person never asked for. - **Fix:** do every check that can refuse before the first write: clean the identifiers first (split the validation half of `jobs/services.py:set_identifiers` into a function that returns the cleaned list or raises, and call it at the top of both company calls), and resolve `_contacts` before `interview.save()`. Where reordering is awkward, wrap the body in `with transaction.atomic():`, so the `HttpError` rolls back the savepoint before ninja renders it. Rolling back the whole request from the error handler instead would be simpler but wrong: the rate limits count in the database cache inside the same transaction, and a refused request must still be counted. ## Done when - `tests/test_api.py` sends each of the three refused writes above and asserts a 422 and that the company or interview is exactly as it was before (no new company, same name, same industries, same location).

tiagoagueda added this to the 0.5.0 milestone

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

tiagoagueda added the

bug

label

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

tiagoagueda referenced this issue from a commit

2026-10-04 10:05:00 +00:00

Undo a refused API write to a company or an interview

tiagoagueda closed this issue

2026-10-04 10:05:00 +00:00