fix(geocoder): stop a provider mismatch from crashing the server - #1243
Open
TurtIeSocks wants to merge 1 commit into
Open
fix(geocoder): stop a provider mismatch from crashing the server#1243TurtIeSocks wants to merge 1 commit into
TurtIeSocks wants to merge 1 commit into
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.
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.
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.