Read GeoJSON fallback coordinates in GeoJSON order - #284
Merged
Conversation
getPointFromMixed() resolves a GeoJSON envelope in two passes. When the first declines and Point::fromJson() then throws, two fallback arms recursed with the bare coordinate value — which hands it to the positional array reader in the same method, where index 0 is read as the latitude and 1 as the longitude. That is the reverse of GeoJSON's [lng, lat], so any pair reaching either arm came back transposed. Well-formed points never get that far: pointFromGeoJson() returns early for them. Reaching the fallback needs an envelope isGeoJson() accepts but Point::fromJson() rejects, whose coordinate value is still a single pair — a Point carrying extra members, or a multi-coordinate type whose coordinates is a flat pair rather than an array of pairs. Both arms now read the value as GeoJSON first, through a thin wrapper over the existing pointFromGeoJson(), whose mapping is already correct. It returns null for anything that is not a usable numeric pair, so nested Polygon and LineString rings decline there and still fall through to the old recursion untouched. Only the bare-pair case changes. PointResolutionTest asserted the transposed result with a comment recording that it documented the defect rather than the intent. That expectation is flipped here, deliberately, since this is a behaviour change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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
Utils::getPointFromMixed()returned latitude and longitude the wrong way round for one class of GeoJSON input. This corrects it and pins the behaviour with tests.Found while writing coverage for the two fallback arms on
dev-v0.6.59; split out into its own PR because it is a behaviour change on the location write path rather than a coverage commit.The defect
Support/Utils.phpresolves a GeoJSON envelope in two passes. The first,pointFromGeoJson(), handles well-formedPointandFeature-wrappedPointvalues and maps them correctly —coordinates[0]to longitude,coordinates[1]to latitude.Anything it declines falls through to
Point::fromJson(), and when that throws, two fallback arms recursed with the bare coordinate value:Recursing hands the pair to the positional array reader further down the same method:
Index
0is read as the latitude and1as the longitude — the reverse of GeoJSON's[lng, lat]. So a pair reaching either arm came back transposed.Which inputs were affected
Narrower than it first looks, because well-formed points never reach the fallback —
pointFromGeoJson()returns early for them. Reaching it requires an envelope thatisGeoJson()accepts butPoint::fromJson()rejects, with a coordinate value that is still a single pair. In practice:Pointenvelope carrying extra members thatPoint::fromJson()will not parseLineString,MultiPoint, …) whosecoordinatesis a flat[lng, lat]pair rather than an array of pairs — malformed, but the kind of thing hand-built payloads and looser API clients producePolygon and LineString rings — nested arrays — were not affected and are not affected now; they fall through to the same positional reader as before.
The fix
Read the fallback value as GeoJSON before falling back to positional interpretation, reusing the mapping that is already correct:
pointFromGeoJsonCoordinates()is a thin wrapper over the existingpointFromGeoJson(), which already declines anything that is not a usable numeric pair. So nested rings returnnullthere and still reach the old recursion untouched — only the bare-pair case changes, which is the defective one.Tests
server/tests/PointResolutionTest.phppreviously asserted the transposed result, with a comment recording that it was documenting the defect rather than the intent. That expectation is flipped here, deliberately — this is a behaviour change, so the test moves with it.Two cases are added alongside: the top-level
coordinatesarm is preferred over nestedgeometry.coordinatesand read in the same order, and a Polygon ring still routes to the positional reader.Verification
Both fallback arms and both sides of the
??are covered — confirmed by slice coverage, not just passing assertions. The full suite runs clean apart fromOrderControllerUpstreamNotFoundTest, which fails by design ondev-v0.6.59until core-api 1.6.55 ships.Worth a manual MySQL pass before this merges. It sits on the location write path, so zones, service areas, waypoint and place location save/update, and reverse geocoding are the things to exercise. The in-memory SQLite suite proves the branch logic, not real-database behaviour.