fix(geocoder): stop a provider mismatch from crashing the server - #1243
Conversation
A webhook pointing nominatimUrl at Photon without setting geocoderProvider took
down the process rather than failing the request.
node-geocoder decides how to read a response by its shape: an array is a result
list, anything else is a single result. Photon answers with a GeoJSON object, so
the entire FeatureCollection was handed to _formatResult as though it were one
place. node-geocoder 4.4.1 guards its own address lookup and returns undefined
fields, but the patch ReactMap layers on top did not, so result.address.suburb
threw.
That throw never reached geocoder()'s catch. node-geocoder resolves through
bluebird's asCallback, so a throw inside _formatResult surfaces as an uncaught
exception and kills the process. The try/catch reads as though every failure
returns {}, and this one could not be caught there at all.
Three changes. The patch now optional-chains the address, so a response without
one cannot throw. nominatimGeocoder awaits its results and rejects with a
message naming the fix when the body is a GeoJSON FeatureCollection, which
rejects normally and is caught. photonGeocoder does the mirror check for a JSON
array, so the opposite mismatch reports itself instead of returning an empty
result set with no reason.
Four tests drive geocoder() over a real HTTP server for both mismatches and both
matched pairs. Removing the optional chaining hangs the runner rather than
failing it, which is the same fatality seen in production.
|
The primary Photon-on-Nominatim crash is addressed, but the newly added symmetric mismatch handling remains incomplete for the application's reverse-geocoding path. Review comment:
|
The mismatch check only recognised a JSON array, which is Nominatim's /search
shape. Its /reverse answers with a single object, and that is the path
resolvers.js takes for every gym lookup, so a Photon provider pointed at a
Nominatim endpoint fell through to an empty result set and a generic formatter
failure rather than the error naming the misconfiguration.
isNominatimResponse now recognises both shapes. The test is positive: it looks
for fields a Nominatim result carries rather than treating anything without
`features` as suspect, because fetchJson returns a Response object on a failed
request and a no-match answers with {"error":"Unable to geocode"}. Neither is a
provider mismatch, and an empty result is already the right outcome for both.
The regression tests assert against photonGeocoder rather than geocoder().
geocoder() catches everything and returns {}, so a mismatch and a plain miss are
indistinguishable from there. A first attempt asserted at that level and passed
against the unfixed code, which is the reason for the inner-function tests: the
change is about which error reaches the log, and only photonGeocoder exposes it.
|
Right, fixed in 73d14bd. The check only recognised Nominatim's
Three tests: a Nominatim reverse object and a Nominatim search array both raising the provider error, and a no-match object still resolving to an empty list. Worth flagging how the first version of those tests went, since it bears on the rest of this PR. I wrote them against 🤖 Addressed by Claude Code |
|
The new classifier cannot observe a standard Nominatim reverse response because the request does not select JSON. Consequently, the provider-mismatch diagnosis remains broken for the scenario the commit intends to fix. Review comment:
|
…ominatim host The classifier could not see a Nominatim reverse response, because the request never asked for one. Nominatim's /reverse defaults to XML, verified against nominatim.openstreetmap.org: no format parameter returns text/xml with HTTP 200. fetchJson cannot parse that and returns undefined, so isNominatimResponse never saw a body and the caller got the generic geocoding failure instead of the provider error. The reverse request now sends format=json. Photon ignores the parameter. The forward path was broken the same way and for a different reason. Photon serves /api and Nominatim has no such endpoint, so a Nominatim host answers 404 there whatever format is asked for, verified the same way. A Nominatim search array could never have arrived on that path. fetchJson returns the Response itself for any failed request, so that is now surfaced as an error naming the URL and status rather than an empty result set. That also covers a Photon instance returning 500, which previously resolved to no results and no reason. The tests were the reason none of this showed: the stub answered the same Nominatim JSON on every path, so both checks looked like they worked. The regression server now behaves like a real Nominatim host, 404 on /api and XML on /reverse unless format=json is asked for, and a separate test asserts the outgoing request carries the parameter. Removing format=json fails two of them; removing the status check fails a third.
|
Correct, fixed in 170258c. I checked it against the real service rather than reasoning about it, and it is worse than reported: the forward path was broken too. Verified against nominatim.openstreetmap.org:
The reverse request now sends On the request contract you asked for: the regression server now behaves like a real Nominatim host rather than answering the same JSON on every path, which is precisely why neither check showed as broken. It answers 404 on That is twice now on this PR that the tests passed against code that could not work, both times because the stub was more accommodating than the real thing. Worth weighing when you decide how much of this to trust. 🤖 Addressed by Claude Code |
Validated against a running Photon instance for the first time, which contradicted three things this mapping assumed. format=json is reverted. Photon rejects any parameter outside its own allow list with a 400, so the previous commit broke every reverse lookup against a correctly configured Photon. The live instance answers: Unknown query parameter 'format'. Allowed parameters are: [include, debug, dedupe, query_string_filter, lon, layer, limit, osm_tag, distance_sort, geometry, exclude, lang, radius, lat]. The reverse mismatch is now recognised from the unparseable body instead, which needs no extra parameter. fetchJson does not catch that itself: it does `return response.json()` inside its try block, so the rejection escapes its own catch and arrives here. locality is Nominatim's neighbourhood, not a smaller settlement. The live index returns locality "Wicker Park" alongside a city, and Nominatim's own answer for the same building carries quarter "Wicker Park". A genuine hamlet does not arrive this way at all: Bootjack comes back as osm_value=locality with its name in `name` and no locality field. It is no longer treated as a city fallback, which would have named a neighbourhood as a town. district is Nominatim's suburb. For 1521 North Hoyne Avenue, Photon sends district "West Town" and Nominatim sends suburb "West Town", the same string for the same building. address.suburb was previously left empty on the grounds that the two were different concepts, which the comparison disproves. Photon's Manhattan for the Statue of Liberty matches Nominatim's suburb there too. Both now appear in the formatted address between the street and the settlement, where Nominatim places the levels they correspond to. Every fixture in the tests is a captured live response rather than a constructed one.
|
I finally ran this against a live Photon index instead of a stub, and it contradicted three things at once, including the fix I pushed two hours ago. Corrected in d3d9347. format=json is reverted, because it broke Photon. Photon rejects any parameter outside its own allow list with a 400, so that commit would have taken down every reverse lookup on a correctly configured instance. The live answer: Your diagnosis was right and the suggested remedy is not available. The reverse mismatch is recognised from the unparseable body instead, which needs no extra parameter. Two mapping errors surfaced in the same run.
Both now sit between the street and the settlement in the formatted address, where Nominatim puts the levels they correspond to. Nine live queries, forward and reverse, now produce output matching the captured Nominatim structurally, including Every fixture in the tests is now a captured live response rather than one I constructed. For the record on this PR: three of your findings were things my tests could not see, and in each case the reason was the same. I was validating against my model of Photon rather than against Photon, and the stub agreed with the model. The endpoint had been available to me for most of that time. 🤖 Addressed by Claude Code |
|
The patch turns transient Photon HTTP failures into an invalid GraphQL list result and leaves direct district searches without the newly supported suburb value. Full review comments:
|
…t results
Two fixes, both verified against a live Photon index.
The error fallback now matches the shape the caller's schema declares.
Query.geocoder is [Geocoder] and Gym.formatted is String, so returning {} made
GraphQL discard the field with "Expected Iterable" or "String cannot represent
value" rather than degrading to an empty answer. Surfacing a non-2xx as a throw
turned a transient Photon 429 or 500 from an empty list into a discarded field,
which is a worse outcome than the silence it replaced. The failure is still
reported: it is logged before the fallback returns.
That object fallback predates this branch and was already wrong for both
callers, but it was only reachable when the URL was missing entirely. Making
upstream failures throw is what turned a rare bug into a routine one.
The type map now includes district. A suburb searched by name comes back as its
own result, with the label in `name` and no `district` field, so the value being
searched for was the one value missing from the answer. Live index, q=West Town
Chicago: {name: "West Town", type: "district", osm_key: "place", osm_value:
"suburb"} now yields suburb "West Town" instead of an empty string.
Both regressions fail their tests without the fix: 34/35 without the district
entry, 32/35 with the object fallback restored.
|
Both correct, fixed in 0e168b6, and both verified against the live index rather than reasoned about. The list-shaped fallback. You are right that surfacing a non-2xx as a throw made things worse for the common case: a transient Photon 429 or 500 went from an empty list to a discarded field. The fallback now matches the shape the caller's schema declares, Worth separating what I broke from what I found: the The district promotion. Confirmed live, {"name": "West Town", "type": "district", "osm_key": "place",
"osm_value": "suburb", "city": "West Chicago Township", "county": "Cook County"}The label is in Both fail without their fix, 34/35 for the district entry and 32/35 with the object fallback restored. This is the second finding on this PR that came from the 🤖 Addressed by Claude Code |
|
The crash guard and failure-shape changes are useful, but standard provider-mismatch handling still misses Photon's real request contract. The new locality mapping also does not cross the public schema and formatter boundary. Full review comments:
|
Reproduced against a live deployment. A webhook that inherits its URL from
Poracle and sets no geocoderProvider returned a single blank result, with
nothing in the log to explain it.
node-geocoder never gets far enough to receive a FeatureCollection, so the
previous check could not fire. It calls /search, which Photon does not serve,
and _forceParams puts format and addressdetails on every request, which Photon
rejects outright. A live instance answers:
GET /search?q=..&format=json&addressdetails=1
404 {"title":"Endpoint GET /search not found","status":404,...}
GET /reverse?lat=..&lon=..&format=json&addressdetails=1
400 {"message":"Unknown query parameter 'format'. Allowed parameters
are: [include, debug, dedupe, ...]"}
node-geocoder ignores the status and parses the body regardless, so both reach
_formatResult as an object with no address and format into an empty string. Both
shapes are now recognised, and the error names the setting that fixes it.
The regression server enforces Photon's actual contract rather than accepting
anything: it serves /api and /reverse only, answers 404 elsewhere with Javalin's
body, and rejects any parameter outside the allow list.
Separately, locality reached no consumer. node-geocoder emits `neighbourhood`,
the GraphQL schema exposes `neighborhood` and formatter() templates on
`neighborhoods`, so the value was invisible to raw clients and to configured
address formats alike. Both providers now carry the alias and formatter accepts
either spelling, which also makes the field work for Nominatim deployments,
where it had been dead for the same reason.
|
Both correct, fixed in c1e868e. I reproduced the first one against a live deployment before fixing it, and it is the exact failure that was reported from production. Photon errors on Nominatim routes. You are right that the The Javalin body even carries a The regression server enforces Photon's contract as you asked, rather than accepting anything: The neighborhood contract. Also right, and worse than described: there are three spellings in play. node-geocoder emits One thing your review surfaced indirectly: the parity test duplicated the 🤖 Addressed by Claude Code |
Clearing the search box sends an empty string, and there is nothing to look up.
Photon rejects it with a 400 and {"message":"q parameter is required when no
include categories are specified"}, which now surfaces as an error, so a user
deleting their own input filled the log with failures the input had caused.
Whitespace-only input does the same. The request is no longer made at all.
The guard also covers a null search, which needed a second fix to reach: typeof
null is 'object', so a null was taken for a coordinate pair and sent to /reverse
with no lat or lon. That predates this branch and was only reachable through the
same empty-input path.
A coordinate pair at 0,0 is a real lookup and is still sent, so the guard cannot
be confused with a falsy coordinate.
The shared empty answer moves into a helper, since the fallback shape now has
two callers and has to stay the shape the caller's schema declares.
|
The patch can misdiagnose generic upstream failures and still loses neighborhood data for quarter-shaped Nominatim responses and formatted GraphQL calls. These are observable contract and diagnostic defects. Full review comments:
|
… to end
Three fixes from review.
The Photon diagnosis no longer fires on a generic title/status pair, which is
RFC 7807's problem-details shape: a rate-limiting proxy in front of a healthy
Nominatim answers {"title":"Too Many Requests","status":429}, and diagnosing
that as a provider mismatch advises the operator to break a working
configuration to fix a transient failure. The 404 match now requires Javalin's
own documentation URL in the body's type field, which is what a real Photon
sends and nothing else does.
The neighbourhood pair falls back to address.quarter. Nominatim reports
neighbourhood-level data under quarter for many places -- 1521 N Hoyne Ave
carries quarter "Wicker Park" and no neighbourhood key -- and node-geocoder only
reads address.neighbourhood, so both spellings came out empty for exactly the
responses the alias exists to surface.
The formatted branch keeps the mapped fields. Query.geocoder resolves the
Geocoder type's neighborhood, suburb and the rest straight off these objects,
and returning only the formatted triple made every one of them null whenever an
addressFormat was configured, which is the resolver path every webhook user
takes. The result spreads through with formatted added rather than being
rebuilt.
Each fix has a test that fails without it: the 7807 body resolving through the
generic path rather than throwing the mismatch advice, the Javalin body still
throwing it, a quarter-shaped response populating both spellings, and a
formatted call retaining neighborhood, suburb and streetNumber.
nominatimUrl already falls back to Poracle's providerURL, so a deployment can get its geocoder URL from Poracle without restating it here. The backend type had no such fallback, which left half the pair auto-populating and the other half not: an operator whose URL came from Poracle still had to hand-set geocoderProvider locally, or every request silently took the Nominatim branch. PoracleNG now reports the backend behind providerURL as `provider`, so geocoderProvider follows the same rule as its URL. Local config still wins, matching how providerURL and addressFormat already behave. Poracle also offers google and none. Neither has a ReactMap equivalent, so they resolve to undefined rather than being mapped onto nominatim, which would claim something untrue about the backend. The match is exact, so a differently-cased value is ignored rather than guessed at. `provider` is destructured out of remoteConfig whether or not it is usable. this.provider is the *webhook* provider, an unrelated field that happens to share the name, and letting Poracle's value through Object.assign(this, rest) would silently overwrite it. The resolution is a small exported function because #fetchConfig is private and would otherwise need a stubbed HTTP round trip to reach. Four tests cover the derivation, local precedence, the ignored backends, and the webhook provider surviving a remote payload that carries a geocoding provider.
The fallback was unconditional, so a webhook with a local nominatimUrl and no
geocoderProvider inherited Photon from a Photon-backed Poracle while keeping its
own Nominatim URL. That is the default shape of every existing Nominatim
deployment, and it made searches and reverse lookups return {} against an
endpoint that had been working.
A URL and the protocol used to talk to it are one setting in two fields, and
splitting them across two sources cannot produce a working pair by accident.
The backend type is now inherited only when the URL was inherited with it.
An explicit local geocoderProvider still wins in either case, which is the only
way to run a backend Poracle does not report.
resolveGeocoderProvider takes a named argument object rather than a third
positional boolean, so the pairing rule is legible at the call site.
|
All three correct, fixed in 70eedcd. The narrowed signature. The title/status pair is RFC 7807's shape, not Photon's, and your failure case is the damaging one: a rate-limiting proxy in front of a healthy Nominatim would have been diagnosed with advice to change The quarter fallback. Both spellings now fall back to The formatted branch. The result now spreads through with Each fix fails its test when reverted individually: 44/45, 44/45, and 42/45. 🤖 Addressed by Claude Code |
…oracle feat(geocoder): derive geocoderProvider from Poracle's remote config
|
The narrowed classifier now treats transient problem JSON as a successful blank location. This breaks the public geocoder contract under the proxy-error scenario the commit explicitly targets. Review comment:
|
… a blank result
Narrowing the Photon signature reopened the path it had been (wrongly) closing:
an RFC 7807 problem body from a proxy in front of Nominatim fell through the
check, and node-geocoder had already mapped it onto one entry with undefined
coordinates. geocoder(..., false, ...) then resolved with that entry instead of
[], and Location.jsx pans the map to [null, null].
An object body carrying none of lat, display_name, address or place_id is now
rejected with a generic upstream error naming the body. Generic on purpose: this
is a failing upstream, not a misconfigured provider, and the earlier broad match
was reverted precisely because it advised changing geocoderProvider to cure a
transient 429. {"error": ...} bodies never reach the check, since node-geocoder
converts those to a thrown Error itself, and every captured /reverse fixture
carries lat, display_name and address, so genuine results pass.
The 7807 test now asserts the rejection, that its message is not the provider
advice, and that the public entry point returns [] rather than a result with
null coordinates.
|
Correct, fixed in 6b37fd3. Narrowing reopened the exact path the broad match had been closing, badly: the 7807 body fell through, node-geocoder had already mapped it onto one entry with undefined coordinates, and An object body carrying none of The test now asserts three things: the rejection fires, its message is not the provider advice, and the public entry point returns 🤖 Addressed by Claude Code |
|
Will review the new commit in a bit. This is for the old. The patch improves failure shapes but still turns Nominatim problem responses into invalid successful results and mislabels transient Photon failures as provider misconfiguration. Both issues affect normal operational failure paths. Full review comments:
|
|
The tested problem body is rejected, but the presence-only classifier still accepts malformed or extended problem bodies that produce the same null-coordinate result. The intended failure-path fix is therefore incomplete. Review comment:
|
…f provider advice
Two fixes from review.
The Nominatim body classifier validates coordinate values instead of key
presence. RFC 7807 permits extension members, so {"title":"Too Many Requests",
"status":429,"address":null} passed the presence check and still formatted into
an entry with undefined coordinates, reopening the null-coordinate result the
check exists to prevent. A body is accepted only when lat and lon both parse to
finite numbers, which every one of the 42 captured fixtures does and which is
also the minimum a result must carry to be usable by any caller.
Photon's non-2xx handling reserves provider advice for provider evidence. Only
a 404 on /api says anything about what the host is; a 429 or 500 from a healthy
but struggling Photon, or a 401/403 from an authentication proxy in front of
one, is an operational failure, and telling the operator to remove
geocoderProvider there prompts them to break a working configuration. Other
statuses now report as an upstream failure with no configuration advice.
Non-vacuity confirmed for both: restoring the presence check fails one test,
restoring the advice on non-404 statuses fails another. A matched-pair test also
pins that a genuine reverse result passes the coordinate validation, so the
stricter check cannot silently reject what it protects.
|
Both remaining findings fixed in fb398d7. (The first item of the earlier comment was the fall-through that 6b37fd3 addressed; the two below are the ones still open.) Coordinate validation over key presence. You are right that the presence check was defeatable by design: RFC 7807 extension members mean Transient Photon failures. Provider advice is now reserved for provider evidence: only the 404 on Non-vacuity confirmed for both: restoring the presence check fails one test (52/53), restoring the advice on non-404 statuses fails another (52/53). 🤖 Addressed by Claude Code |
|
The patch introduces a public provider-field asymmetry and false-positive provider diagnostics for valid or operational Photon responses. These can return inaccurate API data and prompt operators to break a valid backend configuration. Full review comments:
|
There was a problem hiding this comment.
Pull request overview
Hardens geocoding failures while expanding provider detection and address mapping.
Changes:
- Detects mismatched Photon/Nominatim responses safely.
- Improves neighborhood mapping and fallback result shapes.
- Inherits Poracle provider metadata and adds integration coverage.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
server/src/services/geocoder.js |
Adds validation, aliases, and schema-safe fallbacks. |
server/src/services/photonGeocoder.js |
Detects mismatches and expands address mapping. |
server/src/services/Poracle.js |
Inherits geocoder provider metadata. |
packages/types/lib/config.d.ts |
Documents provider inheritance. |
server/test/geocoder.test.js |
Adds provider and HTTP integration tests. |
Suppressed comments (1)
server/src/services/photonGeocoder.js:341
- This branch also handles reverse requests, but its message always claims that
/apireturned 404. WhenisReverseis true the requested route was/reverse, and that 404 does not establish that the host lacks Photon's/api; the current diagnostic can therefore send operators toward the wrong configuration change. Name the actual requested path and avoid the unsupported conclusion.
if (response.status === 404) {
throw new Error(
`${photonUrl} answered 404 for Photon's /api endpoint, so it is not a Photon instance. Check the URL, or remove "geocoderProvider": "photon" from this webhook.`,
)
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| /{{(streetNumber|streetName|city|state|country|zipcode|latitude|longitude|countryCode|neighborhoods|neighborhood|neighbourhood|suburb|town|village)}}/g, | ||
| (a, b) => result[b] || '', |
| throw new Error( | ||
| `${photonUrl} returned a body that is not JSON. Nominatim answers /reverse with XML unless asked otherwise, so this is usually a Nominatim URL on the Photon provider. Remove "geocoderProvider": "photon" from this webhook, or point the URL at a Photon instance. (${err instanceof Error ? err.message : err})`, | ||
| ) |
| this.geocoderProvider = resolveGeocoderProvider({ | ||
| remote: provider, | ||
| local: this.geocoderProvider, | ||
| usingRemoteURL, | ||
| }) |
| neighbourhood: properties.locality || '', | ||
| // The same value under the spelling the GraphQL schema and formatter use. |
|
🎉 This PR is included in version 1.51.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
What happens today
A webhook whose
nominatimUrlpoints at Photon withoutgeocoderProvider: "photon"takes the process down rather than failing the request. Reported from production after #1242 shipped.The error is
TypeError: Cannot read properties of undefined (reading 'suburb').Why
node-geocoder decides how to read a response by its shape: an array is a result list, anything else is a single result. Photon answers with a GeoJSON object, so the entire
FeatureCollectionis handed to_formatResultas though it were one place.node-geocoder 4.4.1 guards its own address lookup and returns undefined fields rather than throwing. The patch ReactMap layers on top of it did not, so
result.address.suburbthrew.That throw never reached
geocoder()'s catch. node-geocoder resolves through bluebird'sasCallback, so a throw inside_formatResultsurfaces as an uncaught exception and kills the process. Thetry/catchreads as though every failure ends inreturn {}, and this one could not be caught there at all. Confirmed directly: atry/catcharound theawaitstill lets the process die.Worth noting for anyone reading the lockfile:
"node-geocoder": "^4.2.0"resolves to 4.4.1, and 4.2.0's_formatResultis unguarded. The two versions fail differently, and 4.4.1 is what actually runs.Changes
The
_formatResultpatch optional-chains the address, so a response without one cannot throw.nominatimGeocoderawaits its results and rejects when the raw body is a GeoJSONFeatureCollection, with a message naming the fix. Rejecting from an async function reaches the existing catch normally, so the operator gets a log line instead of a dead process.photonGeocoderdoes the mirror check for a JSON array, so the opposite mismatch reports itself rather than returning an empty result set with no reason.results.rawsurvives node-geocoder's wrapper and separates the two cleanly: an Array for Nominatim,type: "FeatureCollection"for Photon.What this does not do
It stops the crash. It does not make a misconfigured webhook geocode. A Photon backend still requires the opt-in:
{ "webhooks": [{ "nominatimUrl": "http://127.0.0.1:2322", "geocoderProvider": "photon" }] }Auto-switching on the response shape was considered and left out on purpose. It would let a request round-trip decide behaviour and would hide the misconfiguration permanently, rather than surfacing it once.
Testing
Four new cases drive
geocoder()over a real HTTP server rather than testing the mapping in isolation, which is the gap that let this ship: both mismatched pairs, and both matched pairs to show the checks do not reject valid responses.yarn lintpassesyarn buildpassesyarn prettierpassesnode --test server/test/geocoder.test.jspasses 26/26Removing the optional chaining hangs the test runner instead of failing it, because the process dies mid-run. That is the same fatality seen in production, and it is worth knowing that this particular regression would show up in CI as a timeout rather than a clean failure.
yarn testalso runsserver/test/rocketPokemonFiltering.test.js, which fails withNo database selected for React Map Tables. That is unrelated and predates this branch:server/src/db/knexfile.cjscallsprocess.exit(9)at import when no schema hasuserin itsuseFor, and the test reaches it throughservices/state. It has failed on every CI run since it landed.