fix: treat an empty relationship as absent instead of raising - #313
Merged
roncodes merged 1 commit intoSep 6, 2026
Merged
Conversation
`ConvertEmptyStringsToNull` turns `"driver": ""` into null before the
controller sees it, and `has('driver')` is still true afterwards — `has()`
reports that the key is present, not that it holds anything. A null therefore
reaches these lookups whenever a client serialises an unselected field as an
empty string.
That was harmless for years: the lookups were plain queries, `where('public_id',
null)` matched nothing, and the caller's `if ($driver)` skipped the assignment.
2c7de8f extracted those queries into helpers and typed them `string` while
making only the return nullable, so "no driver" became a TypeError. Note that
`vehicle`, assigned three lines below `driver` via `Utils::getUuid()`, never
broke — the regression belongs to the extraction, not to the pattern.
Widen the eight seams fed directly from request input. Every body is unchanged
and each was already null-safe, so this adds no statements and no new
behaviour: a widened parameter is strictly more permissive, and the only calls
that behave differently are the ones that used to crash.
The `has()`/`empty()` guards on update paths are deliberately untouched.
Switching them to `filled()` would read as the tidier fix and would silently
break clearing — `VehicleController::update` unassigns a driver on an empty
value, and `PUT {"vendor": ""}` must keep nulling `vendor_id`.
Also mark `service_area` and `zone` nullable on service rate creation. Both are
optional, so an empty one has to read as absent rather than draw "the selected
service area is invalid".
The regression test derives its seam list by scanning for
`$this->seam($request->input(...))` rather than pinning today's eight, so a
future extraction that types a new seam `string` fails at the commit that
introduces it. Exemptions have to name the call-site guard that excludes null,
and the scan asserts it actually matched something so it cannot pass vacuously.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
An empty relationship —
"driver": ""onPOST /v1/orders— returns a 500 instead of being ignored. This restores the historical behaviour and adds a test that prevents the whole class from coming back.Targets
feature/public-fleet-resource-api(#311).What broke, and when
ConvertEmptyStringsToNullis global middleware, so"driver": ""reaches the controller asnull, and$request->has('driver')is still true —has()means the key is present, not that it holds anything. A null therefore arrives at the lookup on every request where a client serialises an unselected dropdown as an empty string.That was harmless for years because the lookup was a plain query:
where('public_id', null)matches nothing, and the caller'sif ($driver)skipped the assignment. The order was created without a driver, which is what the caller meant.It broke in
2c7de8fb(coverage refactor, released in v0.6.59), which extracted the inline query into a helper and typed itstring $publicIdwhile making only the return nullable:Nothing about the lookup changed — only the parameter's willingness to be asked. "No driver" became a
TypeError.Worth noting what did not break:
vehicle, three lines belowdriverin the same block, still goes throughUtils::getUuid()and has always answered null for a null. The regression is specific to the seams that were extracted, not to the assignment pattern.The fix
Widen the parameter on the eight lookup seams that are fed directly from request input:
OrderControllerfindDriverByPublicIdEntityControllerfindPayloadByPublicIdTrackingStatusControllergetOrderTrackingNumberUuidServiceAreaController/ZoneControllerserviceAreaUuidVehicleControllerfindDriverIssueController/FuelReportControllerfindDriverRecordEvery body is unchanged, byte for byte. Each was already null-safe —
where('public_id', null)matches nothing,serviceAreaUuidnever reads$publicIdat all, andfindRecordOrFailreports not-found, which its call sites already turn into a 404. The only thing the narrow type added was the crash.Also adds
nullabletoservice_areaonCreateServiceRateRequest, so an empty optional relationship reads as absent rather than answering "The selected service area is invalid."zoneon the same request has the identical defect and is fixed with it — say the word if you'd rather I split it out.Compatibility
A widened parameter is strictly more permissive: every call that worked before passes the same value into the same body and gets the same answer. Only the calls that previously raised a
TypeErrorbehave differently, and they now do what they did before v0.6.59.Deliberately not changed: the
has()/empty()guards on update paths. Switching them tofilled()would have been the tidier-looking fix and would have silently broken clearing —PUT {"vendor": ""}must keep settingvendor_idto null, andVehicleController::updatemust keep callingunassignDriver()on an empty driver. Those semantics are verified and untouched.Of the eight seams, three are reachable with a null over HTTP today (
Order,Entity,TrackingStatus); the other five are already guarded by!emptyor by arequiredvalidation rule. Widening them is defence-in-depth so the next call site added against them cannot reintroduce the bug.Tests
NullRelationshipInputTesthas three tests:$this->seam($request->input(...))call in the public v1 controllers and fails if the first parameter is non-nullable. Exemptions require a named call-site guard, so the list can't quietly rot into a rubber stamp.The guard was mutation-checked: reverting
findDriverByPublicIdtostringfails it with the exact original signature named. That is the test that would have caught2c7de8fbat the commit that introduced it.Coverage stays at 100%.