feat(geocoder): add native Photon provider - #1242
Conversation
Photon speaks GeoJSON rather than Nominatim's JSON, so it cannot be driven through node-geocoder's openstreetmap provider. This adds a small module that talks to Photon directly and returns entries in the same shape that provider produces, so formatter and the webhook resolvers cannot tell which backend answered. Selected per webhook with geocoderProvider, which defaults to nominatim. The existing Nominatim path is unchanged in behaviour; it moves into its own function so the two branches read side by side. nominatimUrl keeps its name and holds the base URL for either provider, so no existing config needs editing. Two parts of the mapping are not mechanical. Photon reports a result's own label only in properties.name and uses the hierarchy fields purely for what contains the result, while Nominatim echoes that name into the matching address field. Without reproducing the echo, searching for a city returns a result with no city in it. osm_key and osm_value decide which field the name belongs in, and a value Photon already supplied always wins. formattedAddress is composed rather than read, since Photon has no display_name. Components are joined most specific first, absent parts are skipped, and a component is never repeated: a postcode search puts the same value in both the name and the postcode field, and Nominatim renders it once. Where a component recurs further down the hierarchy the broader one is kept, so a city sharing its state's name does not cost the address line its state. address.suburb and address.neighbourhood are always empty. Photon's nearest field is district, which is a different OSM concept, and equating them would be an invention rather than a translation. The tests cover the mapping and assert that both providers emit the same entry for the same address, using node-geocoder's own _formatResult with the same patch geocoder.js applies.
There was a problem hiding this comment.
Pull request overview
Adds Photon as a native geocoding backend while preserving Nominatim compatibility.
Changes:
- Implements Photon GeoJSON requests and response mapping.
- Adds per-webhook provider selection.
- Adds provider parity and formatting tests.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
server/test/geocoder.test.js |
Tests Photon mapping and Nominatim parity. |
server/src/services/photonGeocoder.js |
Implements Photon integration. |
server/src/services/geocoder.js |
Dispatches requests by provider. |
server/src/graphql/resolvers.js |
Passes webhook provider selection. |
packages/types/lib/config.d.ts |
Defines the provider configuration option. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The length check alone accepted a coordinate pair of [null, null] and emitted an entry with a null latitude, which contradicts the filtering contract the function documents. Checking the values rather than the shape also covers NaN, Infinity, undefined and numeric strings. The added test fails without the guard.
|
The documented Photon configuration never reaches the provider dispatch because the setting is dropped when constructing the webhook service instance, leaving the feature unusable. Review comment:
|
Event.webhookObj holds PoracleAPI instances rather than the raw webhook config, and the constructor copies fields across one at a time. geocoderProvider was not among them, so it was undefined by the time the resolvers read it and every request took the Nominatim branch. Configuring Photon did nothing at all. The mapping tests all passed because they exercised the response mapping and never the config to instance to resolver path. Two tests now cover that boundary: one asserting a configured provider survives construction alongside nominatimUrl and addressFormat, and one asserting it stays undefined when unset, which is what keeps existing configs on Nominatim. Both fail without the constructor change.
|
You are right, and the feature was dead on arrival. Fixed in f33185d.
The constructor now carries it alongside On the boundary coverage: Worth naming why the existing tests missed this: they all exercised the response mapping, so they were green while the config never reached it. Mapping correctness and plumbing are different claims and I had only tested one. Still outstanding from the PR description: this has not been exercised against a live Photon instance through the dev server. 🤖 Addressed by Claude Code |
|
The provider plumbing works, but valid Photon locality hierarchy data is discarded. This produces incomplete formatted locations for addresses outside Photon's city layer. Review comment:
|
Photon's address hierarchy runs city, district, locality, street. An address
whose containing settlement sits below the city layer, such as a house in a
hamlet, carries that name in properties.locality and has no city at all. The
fallback only read city, so those addresses lost their settlement from both the
city field and formattedAddress, and any format using {{city}} rendered a blank
component in forward search and in gym reverse geocoding alike.
city still wins where Photon sends both, matching its own hierarchy.
The local variable is now called settlement rather than locality, so it is not
mistaken for the Photon field it falls back to.
Two fixtures cover it: a house in a hamlet, and a response carrying both city
and locality. The first fails without the change.
|
Correct, fixed in 38654d1. I had mapped Photon's hierarchy as though it stopped at The fallback now reads The local variable is now Two fixtures, both Photon-shaped: a house in a hamlet carrying Still deliberately unmapped, so it is a decision rather than an oversight: 🤖 Addressed by Claude Code |
|
Photon provider plumbing works, but valid administrative and top-level results lose their own address-layer value during conversion, producing incomplete formatted searches. Review comment:
|
Scope
Adds Photon as a first-class geocoding backend alongside Nominatim.
Photon returns GeoJSON rather than Nominatim's JSON, so it cannot be driven through
node-geocoder'sopenstreetmapprovider at all.server/src/services/photonGeocoder.jstalks to it directly and returns entries in the same shape that provider produces, soformatterand the webhook resolvers cannot tell which backend answered.Selected per webhook:
{ "webhooks": [ { "nominatimUrl": "http://127.0.0.1:2322", "geocoderProvider": "photon" } ] }geocoderProviderdefaults tonominatim, so existing configs are untouched.nominatimUrlkeeps its name and now holds the base URL for whichever provider is selected. Renaming it would break every deployment for no functional gain.The existing Nominatim path is unchanged in behaviour. It moves into its own
nominatimGeocoderfunction so the two branches read side by side, which is most of the diff ingeocoder.js.Why
Photon is a good fit for a self-hosted geocoder: the prebuilt index is a single download, it needs no PostGIS, and it runs comfortably on ARM. The one thing it cannot do is answer as Nominatim, which until now was the only shape ReactMap could consume.
The two parts of the mapping that are not mechanical
Photon reports a result's own label only in
properties.name, and uses the hierarchy fields purely for what contains the result. Nominatim echoes that name into the matching address field, which is why searching "Denver" gives youaddress.cityof Denver. Without reproducing that echo, a city search returns a result with no city in it, which is the commonest query there is.osm_keyandosm_valuedecide which field the name belongs in, and a value Photon already supplied always wins.formattedAddressis composed rather than read, since Photon has nodisplay_name. Components go most specific first, absent parts are skipped, and no component repeats. Two cases drove that:62704, Leland Grove, Sangamon County, Illinois, United Stateswith the value appearing once.Known gap
address.suburbandaddress.neighbourhoodare always empty for Photon results. Photon's nearest field isdistrict, which is a different OSM concept, and equating them would be an invention rather than a translation. Nominatim users are unaffected.Unrelated to this change, but noticed while reading
formatter: it templates on{{neighborhoods}}whilenode-geocoderemitsneighbourhood, so that placeholder resolves to an empty string for every provider including the existing Nominatim one. Left alone here rather than folded into an unrelated PR.Testing
server/test/geocoder.test.js, 12 cases, no network required:[lon, lat]city/town/village/hamletecho, and a road taking its name asstreetNamecitywinning over the echoed nameformattedAddresscomposition, including both duplicate cases aboveNaNThe last case is the one worth reviewing. It builds
node-geocoder'sopenstreetmapprovider exactly asgeocoder.jsdoes, patch included, runs a Nominatim response for the same address through the real_formatResult, and asserts the two providers produce an identical entry. Not merely the same keys, the same values.Ran locally on Node 22:
yarn lintpassesyarn buildpassesyarn prettierpassesyarn config:checkandyarn config:envproduce no changesnode --test server/test/geocoder.test.jspasses 12/12yarn testalso runsserver/test/rocketPokemonFiltering.test.js, which fails on this machine withNo database selected for React Map Tables. It fails the same way on a clean checkout ofmain, so it is a local database configuration gap rather than a regression from this change.Not yet exercised against a live Photon instance through the dev server. Worth doing before merge if you have one to hand.