Skip to content

Remove unreachable backend code and fix two defects it was hiding - #281

Open
roncodes wants to merge 2 commits into
feature/fleetops-server-coverage-100from
feature/fleetops-dead-code-cleanup
Open

Remove unreachable backend code and fix two defects it was hiding#281
roncodes wants to merge 2 commits into
feature/fleetops-server-coverage-100from
feature/fleetops-dead-code-cleanup

Conversation

@roncodes

Copy link
Copy Markdown
Member

Summary

Removes backend code that no input can reach, and fixes two latent defects found while proving that unreachability.

This came out of the coverage campaign in #277. Pushing server/src line coverage to 99.4%+ meant examining every remaining uncovered line, and a large share turned out not to be untested but unreachable — branches shadowed by a broader arm above them, guards on values a type declaration forbids, and fallbacks sitting after an unconditional assignment. No test can execute those, so they block the --fail-under=100 gate permanently.

Two of them were not harmless leftovers. They are fixed here.

Stacked on #277. Base branch is feature/fleetops-server-coverage-100, so this diff shows only the cleanup. Merge #277 first.

The two commits are split deliberately so the behaviour changes can be reviewed apart from the deletions:

Commit Contents
20fef6bb The two real defects, with their test updates
2aa143f3 Unreachable-code removal and four deliberate annotations

Defects fixed

1. Invalid coordinates silently reverse-geocoded Null Island

Internal\v1\GeocoderController::reverse() validated coordinates after converting them:

$coordinates = Utils::getPointFromCoordinates($query);   // : Point — never null

if (!$coordinates instanceof Point) {
    return response()->error('Invalid coordinates provided.');   // unreachable
}

getPointFromCoordinates() is typed : Point and returns new Point(0, 0) for unusable input, so the guard never fired and garbage input was reverse-geocoded at 0, 0 instead of being rejected. Now resolved with the existing strict variant (getPointFromCoordinatesStrict(): ?Point) so the guard works as written.

An existing test asserted the old behaviour — reverseCalls[0] === [0.0, 0.0], i.e. it documented the Null Island lookup. It now asserts the error is returned and that no lookup is attempted.

2. Polygon writes could not bind on update

Casts\Polygon returned the raw geometry from its GeometryInterface arm, while Casts\Point and Casts\MultiPolygon return a SpatialExpression. That is not cosmetic:

  • SpatialTrait overrides only performInsert — there is no performUpdate. Inserts survive either shape because performInsert() wraps the attribute itself.
  • On updates the cast's return value is bound directly, and BaseBuilder::cleanBindings() only expands a SpatialExpression into the WKT + SRID bindings that ST_GeomFromText(?, ?) requires. A bare Geometry passes through untouched, and it has no __toString, so PDO cannot bind it.

Casts\Polygon is now aligned with the other two.

This is the riskiest hunk in the PR and the one worth the closest look. It is isolated in 20fef6bb and can be reverted alone. Note that an insert-based test cannot validate it — the added RulesAndCastsTest case asserts at the binding level and was verified to fail on the old shape and pass on the new one.

Unreachable code removed

Location Why it could not execute
Casts/{Polygon,MultiPolygon} concrete-type arms shadowed by instanceof GeometryInterface (Polygon → MultiLineString → GeometryCollection → Geometry implements GeometryInterface)
Models/Place::createFromMixed re-tested isCoordinatesStrict() inside a branch already gated on it one arm above
Models/Place::insertFromMixed instanceof GoogleAddress arm sat below is_array || is_object, which swallows every object — reordered so a GoogleAddress routes to insertFromGoogleAddress() instead of being flattened to an array
Models/Place::insertFromMixed address-key check sat in the is_string($place) branch, where empty() on a non-numeric string offset is always true — moved into the array branch where an address key can exist
Integrations/Lalamove::__callStatic instance is a declared public static, so PHP never routes it here
Models/ServiceRate::getLngLatFromPlace guarded getLocationAsPoint(): SpatialPoint, non-nullable, and a SpatialPoint always exposes getLat/getLng
Internal/v1/MetricsController::resolvePeriod guarded $request->date(), which returns ?Carbon; both ?? fallbacks yield DateTime
Resources/v1/{Maintenance,MaintenanceSchedule,WorkOrder,Order} JsonResource fallbacks after Find::httpResourceForModel(), which always resolves a class (falls back to FleetbaseResource)
Resources/v1/{PurchaseRate,TrackingStatus} middle branch guarded by method_exists($this, 'loadMissing'); no class in the hierarchy declares it — it only resolves via __call, which method_exists cannot see
Api/v1/DriverController::create company re-check after an early return already guaranteed it
Api/v1/OrderController::create type fallback after line 72 assigns it unconditionally — replaced with ?? 'transport' at the assignment
Models/Payload::setPlace createFromMixed(): ?Place always yields a Model, so the Str::isUuid() arm and trailing else were both shadowed
Support/Utils::coordsToCircle loop ran 0..360 inclusive, so the ring was already closed and the closing push never ran — loop is now exclusive, which also removes a duplicated vertex. Output is otherwise byte-identical (verified: 121 points, closed, zero duplicate interior vertices)

Kept and annotated, not deleted

Four guards are unreachable today but deliberately retained with @codeCoverageIgnore and an explanatory comment:

  • Casts/Point's SpatialExpression arm. The guard that shadows it (instanceof Expression) returns without recording $model->geometries[$key], so deleting it would erase evidence of a real asymmetry. Recording it would currently be a no-op anyway, since performInsert()'s restore loop would assign the same expression back. There is a genuine latent gap here — a SpatialExpression assigned to a point attribute never becomes a Point after save — but fixing that means recording the geometry behind the expression, which is a behaviour change and out of scope.
  • Support/Utils globe ISO check — all 255 features in the bundled globe.json carry both ISO_A3 and ISO_A2 (verified); the guard protects against future data.
  • Api/v1/OrderController::capturePhoto's empty-photo check — defensive after a validate() that already requires a non-empty photos array.
  • Api/v1/OrderController::updateActivity's not-found guard — blocked on an upstream defect: core-api's Model::findByIdOrFail() calls a non-existent getModelNotFoundException(), so it raises BadMethodCallException instead of ModelNotFoundException and a missing order returns 500 rather than 404. Remove the annotation once core-api is patched. Details in this comment on #277.

Coverage impact

Metric Before After
Lines 99.42% 99.52%
Methods 96.69% 97.19%
Classes 81.38% 82.92%
Uncovered statements 198 165

The --fail-under=100 gate is still red: 165 statements remain, of which two are the annotated upstream-blocked lines and the rest are residue not yet classified.

Verification

Run on the host runtime (asdf PHP 8.4 with Xdebug), no Docker:

  • composer test:lint — clean, 0 of 1140 files needing fixes
  • XDEBUG_MODE=coverage COMPOSER_PROCESS_TIMEOUT=0 composer coverage:baselineall suites green
  • Per-file slice coverage confirming each newly reachable line actually executes
  • php scripts/coverage-summary.php coverage/clover.xml --fail-under=100

Behavioural canaries specific to this change:

  • Spatial write paths: ZoneControllerBordersAndSeamsTest (4/4) and ServiceAreaControllerBorderAndSeamsTest (3/3) snapshotted before and after, including a new polygon update round-trip that rewrites a border and reads the vertices back.
  • Binding-level cast assertion verified to discriminate: fails on the old Casts/Polygon shape, passes on the new one.
  • coordsToCircle: asserted the ring still opens and closes at the same coordinate with no duplicated interior vertex.
  • Group-by-group: every pre-existing test still passes, since a behavioural diff on a supposedly-dead line would mean it was not actually dead.

Three tests that asserted the old behaviour were updated (the Null Island lookup, and two that documented the Casts/Polygon return shape). Each is called out in its diff with a comment explaining what changed and why.

Notes for review

  • Four lines became genuinely reachable as a result of these fixes and are now covered rather than ignored: the invalid-coordinates error, insertFromMixed(GoogleAddress), and two guards whose callee can in fact return null (Utils::getPointFromMixed() returns null for an unresolvable place_*/driver_* id — worth knowing, it is easy to assume otherwise from the signature).
  • Not included, flagged for separate consideration: Casts/Point returns a raw Point from its isCoordinates arm while its GeometryInterface arm wraps — the same class of inconsistency as the Polygon fix, left alone to keep this diff's blast radius contained.

roncodes and others added 2 commits July 30, 2026 13:19
Two defects surfaced while auditing unreachable code.

GeocoderController::reverse validated coordinates only after converting them
with Utils::getPointFromCoordinates(), which is typed `: Point` and falls back
to Point(0, 0) for unusable input. The 'Invalid coordinates provided.' branch
was therefore unreachable and garbage input silently reverse-geocoded Null
Island. Resolve strictly instead so the guard works. An existing test asserted
the old behaviour (reverseCalls[0] === [0.0, 0.0]) and now asserts the error
fires with no lookup attempted.

Casts/Polygon returned the raw geometry from its GeometryInterface arm while
Casts/Point and Casts/MultiPolygon return a SpatialExpression. Only inserts
survive the raw form, because SpatialTrait::performInsert wraps the attribute
itself; there is no performUpdate, so on updates the value is bound directly and
BaseBuilder::cleanBindings only expands a SpatialExpression into the WKT and SRID
bindings that ST_GeomFromText(?, ?) needs. A bare Geometry has no __toString and
cannot be bound. Align Polygon with the other two casts.

Co-Authored-By: Claude Opus 4.8 <[email protected]>
Deletes branches no input can reach: concrete spatial-type arms shadowed by
instanceof GeometryInterface; a duplicate isCoordinatesStrict test inside a
branch already gated on it; Lalamove __callStatic's 'instance' case, which a
declared public static never routes there; guards a type declaration makes
redundant (getLocationAsPoint(): SpatialPoint, Request::date()'s Carbon,
Find::httpResourceForModel() always resolving, Str::isUuid on a model);
DriverController's company re-check after an early return; and the order-type
fallback after an unconditional assignment, replaced by ?? at the assignment.

Relocates Place::insertFromMixed's address-key check out of the is_string branch
(empty() on a non-numeric string offset is always true) into the array branch,
and moves the GoogleAddress arm above is_array||is_object so it is not swallowed
and flattened. Makes coordsToCircle's loop exclusive so the ring is closed
explicitly rather than by recomputing the 0-degree vertex, removing a duplicated
point; output is otherwise identical.

Keeps and annotates three guards that are deliberate: Casts/Point's
SpatialExpression arm (the guard shadowing it skips the geometries bookkeeping,
so the asymmetry is documented rather than erased), the globe-data ISO check
(all 255 bundled features carry both codes), the post-validate photo check, and
updateActivity's not-found guard pending the upstream core-api findByIdOrFail
fix.

Co-Authored-By: Claude Opus 4.8 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant