feat: validated request parameters, so malformed input answers 400 instead of 500 - #19
Conversation
tuj
changed the title
There was a problem hiding this comment.
Approved with comments. Below is the issues Claude found. I don't consider them blockers, but there are some inconsistencies that it would be nice to clean up, like the differences in how an empty array is handled in 1). Also the change in the bahavior in the deleted endpoint, to now default to everything.
These might be necessary and intentional for compatibility reasons, but might also be simple omissions, I don't have the full picture on the Economics (client) side.
1. projectIds: [] bypasses the filter entirely — Repositories/ApiDataRepository.php:40,57,75
The PR now formally supports and tests empty-list semantics (testAnEmptyIdsArrayStaysAnEmptyFilter — "keeps the existing behaviour of matching no rows"). That holds for ids, which uses !== null:
->when($ids !== null, fn ($query) => $query->whereIn("ticket.id", $ids)) // [] → WHERE 0 = 1 → no rows
->when($projectIds != null, fn ($query) => $query->whereIn(..., $projectIds)) // [] == null → filter skipped → ALL rowsRequestParameters::toIds([]) returns [], and [] != null is false in PHP. So POST {"projectIds": []} — reachable through the documented body form — returns rows from every project instead of none. Opposite of what the caller asked for. Three != → !== fixes it, and this PR is the natural place since it just codified the empty-list contract.
2. The deleted endpoint has no cap, and its default request is now the largest one
Defaulting types to all four (Model/DeletedRequestParameters.php:67) is right for the 500, but getDeleted takes no limit and deleted is optional — so a bare GET /apidata/api/deleted now scans four tables and returns every deleted id ever recorded. Previously that request did nothing. The PR caps entity endpoints at MAX_LIMIT = 1000 for exactly this reason; the deleted endpoint is left unbounded. Not a blocker, but worth a follow-up issue at minimum.
3. Nit: array elements aren't trimmed, comma-string elements are — Model/CoercesRequestInput.php:53-60
?types=tickets,%20timesheets works (trimmed); ?types[]=tickets%20 is a 400, because a type is matched with a strict in_array(). Cheap to align by trimming scalars in the array branch too. Note this only bites types — ids survives either form, since is_numeric() accepts leading and (as of PHP 8.0) trailing whitespace.
| * so the message reaches the client — name the parameter and the expected shape, | ||
| * never the value that was sent. | ||
| */ | ||
| class InvalidRequestException extends \InvalidArgumentException {} |
There was a problem hiding this comment.
Could we rename to BadRequestException to match HTTP spec terminology? https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Status/400
An empty projectIds list was compared loosely, so it dropped the filter and answered with every row instead of none, while an empty ids list correctly matched nothing. Elements sent in array form are now trimmed like the comma separated form, so ?types[]=tickets%20 is no longer a 400 against the strict type comparison. types is required on the deleted endpoint, missing or empty. The endpoint has no limit, so defaulting it let a bare request return every deleted id ever recorded, and an empty list would answer 200 with nothing, which a sync client reads as "nothing was deleted". Renamed InvalidRequestException to BadRequestException, matching the 400 it turns into.
2 participants
Link to ticket
https://leantime.itkdev.dk/#/tickets/showTicket/8000
Description
Checklist