Skip to content

fix(geocoder): stop a provider mismatch from crashing the server - #1243

Open
TurtIeSocks wants to merge 1 commit into
mainfrom
fix/geocoder-photon-misconfig
Open

fix(geocoder): stop a provider mismatch from crashing the server#1243
TurtIeSocks wants to merge 1 commit into
mainfrom
fix/geocoder-photon-misconfig

Conversation

@TurtIeSocks

Copy link
Copy Markdown
Collaborator

What happens today

A webhook whose nominatimUrl points at Photon without geocoderProvider: "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 FeatureCollection is handed to _formatResult as 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.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 ends in return {}, and this one could not be caught there at all. Confirmed directly: a try/catch around the await still 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 _formatResult is unguarded. The two versions fail differently, and 4.4.1 is what actually runs.

Changes

The _formatResult patch optional-chains the address, so a response without one cannot throw.

nominatimGeocoder awaits its results and rejects when the raw body is a GeoJSON FeatureCollection, 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.

photonGeocoder does the mirror check for a JSON array, so the opposite mismatch reports itself rather than returning an empty result set with no reason.

results.raw survives 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 lint passes
  • yarn build passes
  • yarn prettier passes
  • node --test server/test/geocoder.test.js passes 26/26

Removing 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 test also runs server/test/rocketPokemonFiltering.test.js, which fails with No database selected for React Map Tables. That is unrelated and predates this branch: server/src/db/knexfile.cjs calls process.exit(9) at import when no schema has user in its useFor, and the test reaches it through services/state. It has failed on every CI run since it landed.

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.
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