From ddf394dfd11bd2134043ba96b7045734139cff93 Mon Sep 17 00:00:00 2001 From: Rin <58572875+TurtIeSocks@users.noreply.github.com> Date: Fri, 21 Aug 2026 12:12:31 -0400 Subject: [PATCH 01/12] fix(geocoder): stop a provider mismatch from crashing the server 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. --- server/src/services/geocoder.js | 42 ++++++++-- server/src/services/photonGeocoder.js | 8 ++ server/test/geocoder.test.js | 106 ++++++++++++++++++++++++++ 3 files changed, 151 insertions(+), 5 deletions(-) diff --git a/server/src/services/geocoder.js b/server/src/services/geocoder.js index 94e32a114..1f431d162 100644 --- a/server/src/services/geocoder.js +++ b/server/src/services/geocoder.js @@ -20,6 +20,31 @@ function formatter(addressFormat, result) { .trim() } +/** + * Fails loudly when the configured URL answered with something that is not a + * Nominatim response. + * + * node-geocoder decides how to read a body by its shape: an array is treated as + * a result list, and anything else is treated as a single result. Photon + * answers with a GeoJSON object, so the whole FeatureCollection gets handed to + * _formatResult as though it were one place. Nothing throws, because + * node-geocoder guards the address lookup, so the caller silently receives one + * entry with every field undefined. + * + * That is a misconfiguration rather than a geocoding failure, and it is + * invisible in the results, so it is worth an error naming the fix. + * @param {any} results + * @param {string} url + */ +function assertNominatimResponse(results, url) { + const raw = results?.raw + if (raw && !Array.isArray(raw) && raw.type === 'FeatureCollection') { + throw new Error( + `${url} answered with GeoJSON, which is Photon's format rather than Nominatim's. Set "geocoderProvider": "photon" on this webhook, or point the URL at a Nominatim instance.`, + ) + } +} + /** * Nominatim, via node-geocoder's `openstreetmap` provider. * @param {string} url @@ -34,13 +59,20 @@ async function nominatimGeocoder(url, search, isReverse) { }) stockGeocoder._geocoder._formatResult = ((original) => (result) => ({ ...original(result), - suburb: result.address.suburb || '', - town: result.address.town || '', - village: result.address.village || '', + suburb: result.address?.suburb || '', + town: result.address?.town || '', + village: result.address?.village || '', }))(stockGeocoder._geocoder._formatResult) - return isReverse && typeof search === 'object' + // Awaited rather than returned so the shape check runs here. A throw inside + // _formatResult would not reach geocoder()'s catch at all: node-geocoder + // resolves through bluebird's asCallback, so it surfaces as an uncaught + // exception and takes the process down. Anything thrown from this function + // rejects normally and is caught. + const results = await (isReverse && typeof search === 'object' ? stockGeocoder.reverse(search) - : stockGeocoder.geocode(String(search)) + : stockGeocoder.geocode(String(search))) + assertNominatimResponse(results, url) + return results } /** diff --git a/server/src/services/photonGeocoder.js b/server/src/services/photonGeocoder.js index 2759ce9e4..db368cc9f 100644 --- a/server/src/services/photonGeocoder.js +++ b/server/src/services/photonGeocoder.js @@ -257,6 +257,14 @@ async function photonGeocoder(photonUrl, search, isReverse) { }) const response = await fetchJson(url) + // The mirror of the Nominatim check: a JSON array is Nominatim's search + // shape, so the URL and the provider disagree. Without this the caller just + // gets an empty result set and no reason for it. + if (Array.isArray(response)) { + throw new Error( + `${photonUrl} answered with a JSON array, which is Nominatim's format rather than Photon's. Remove "geocoderProvider": "photon" from this webhook, or point the URL at a Photon instance.`, + ) + } // fetchJson answers a failed request with the Response rather than throwing, // so an absent features array covers both a network failure and an empty // result set. diff --git a/server/test/geocoder.test.js b/server/test/geocoder.test.js index 797093151..0e2aeca99 100644 --- a/server/test/geocoder.test.js +++ b/server/test/geocoder.test.js @@ -3,7 +3,10 @@ const { test } = require('node:test') const NodeGeocoder = require('node-geocoder') +const http = require('node:http') + const { PoracleAPI } = require('../src/services/Poracle') +const { geocoder } = require('../src/services/geocoder') const { formatPhotonFeature, joinComponents, @@ -420,3 +423,106 @@ test('PoracleAPI leaves the provider undefined when it is not configured', () => assert.equal(api.geocoderProvider, undefined) assert.equal(api.nominatimUrl, 'http://127.0.0.1:2322') }) + +// A misconfigured webhook -- a Photon URL left on the Nominatim provider, or the +// reverse -- used to reach node-geocoder, which reads a GeoJSON object as a +// single result and hands the whole FeatureCollection to _formatResult. The +// unguarded address lookup in ReactMap's patch then threw, and because +// node-geocoder resolves through bluebird's asCallback the throw never reached +// geocoder()'s catch: it surfaced as an uncaught exception and killed the +// process. +const serveOnce = async (body) => { + const server = http.createServer((_, res) => { + res.writeHead(200, { 'Content-Type': 'application/json' }) + res.end(typeof body === 'string' ? body : JSON.stringify(body)) + }) + await new Promise((resolve) => { + server.listen(0, '127.0.0.1', resolve) + }) + return { + url: `http://127.0.0.1:${server.address().port}`, + close: () => + new Promise((resolve) => { + server.close(resolve) + }), + } +} + +const PHOTON_BODY = { + type: 'FeatureCollection', + features: [ + { + geometry: { type: 'Point', coordinates: [-104.9903, 39.7392] }, + properties: { name: 'Denver', type: 'city', countrycode: 'US' }, + }, + ], +} + +const NOMINATIM_BODY = [ + { + lat: '39.7392', + lon: '-104.9903', + display_name: 'Denver, Colorado, United States', + address: { city: 'Denver', state: 'Colorado', country_code: 'us' }, + }, +] + +test('a Photon URL on the Nominatim provider fails without crashing', async () => { + const server = await serveOnce(PHOTON_BODY) + try { + // geocoder() catches and returns {}. What matters is that the process + // survives to get here at all. + const result = await geocoder(server.url, 'Denver', false, '{{city}}') + assert.deepEqual(result, {}) + } finally { + await server.close() + } +}) + +test('a Nominatim URL on the Photon provider fails without returning nothing silently', async () => { + const server = await serveOnce(NOMINATIM_BODY) + try { + const result = await geocoder( + server.url, + 'Denver', + false, + '{{city}}', + 'photon', + ) + assert.deepEqual(result, {}) + } finally { + await server.close() + } +}) + +// The matched pairs still work, so the checks above are not rejecting valid +// responses. +test('a correctly configured Photon webhook still geocodes', async () => { + const server = await serveOnce(PHOTON_BODY) + try { + const result = await geocoder( + server.url, + 'Denver', + false, + '{{city}}', + 'photon', + ) + assert.deepEqual(result, [ + { formatted: 'Denver', latitude: 39.7392, longitude: -104.9903 }, + ]) + } finally { + await server.close() + } +}) + +test('a correctly configured Nominatim webhook still geocodes', async () => { + const server = await serveOnce(NOMINATIM_BODY) + try { + const result = await geocoder(server.url, 'Denver', false, '{{city}}') + assert.deepEqual(result, [ + { formatted: 'Denver', latitude: 39.7392, longitude: -104.9903 }, + ]) + } finally { + await server.close() + } +}) From 73d14bd7ec0636ce5971146e2dc1ce62c9297a06 Mon Sep 17 00:00:00 2001 From: Rin <58572875+TurtIeSocks@users.noreply.github.com> Date: Sun, 23 Aug 2026 16:21:43 -0400 Subject: [PATCH 02/12] fix(geocoder): detect Nominatim's reverse shape on the Photon path 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. --- server/src/services/photonGeocoder.js | 37 ++++++++++-- server/test/geocoder.test.js | 84 +++++++++++++++++++++++++++ 2 files changed, 116 insertions(+), 5 deletions(-) diff --git a/server/src/services/photonGeocoder.js b/server/src/services/photonGeocoder.js index db368cc9f..4276d625b 100644 --- a/server/src/services/photonGeocoder.js +++ b/server/src/services/photonGeocoder.js @@ -238,6 +238,31 @@ function formatPhotonFeature(feature) { } } +/** + * Recognises a response that came from Nominatim rather than Photon. + * + * An array is Nominatim's /search shape. A single object carrying result fields + * and no `features` is its /reverse shape. Both mean the URL and the configured + * provider disagree. + * + * The test is deliberately positive: it looks for fields a Nominatim result + * has, rather than treating anything without `features` as suspect. fetchJson + * hands back a Response object on a failed request, and Nominatim answers a + * miss with {"error":"Unable to geocode"} -- neither is a provider mismatch, + * and an empty result set is already the right outcome for both. + * @param {any} response + */ +function isNominatimResponse(response) { + if (Array.isArray(response)) return true + if (!response || typeof response !== 'object') return false + if ('features' in response) return false + return ( + 'address' in response || + 'display_name' in response || + 'place_id' in response + ) +} + /** * @param {string} photonUrl * @param {string | { lat: number, lon: number }} search @@ -257,12 +282,14 @@ async function photonGeocoder(photonUrl, search, isReverse) { }) const response = await fetchJson(url) - // The mirror of the Nominatim check: a JSON array is Nominatim's search - // shape, so the URL and the provider disagree. Without this the caller just - // gets an empty result set and no reason for it. - if (Array.isArray(response)) { + // The mirror of the Nominatim check. Both of Nominatim's shapes have to be + // recognised: /search answers with an array, and /reverse answers with a + // single object. Checking only the array would let gym reverse geocoding + // fall through to an empty result set with no reason given, which is the + // path resolvers.js takes for every fort lookup. + if (isNominatimResponse(response)) { throw new Error( - `${photonUrl} answered with a JSON array, which is Nominatim's format rather than Photon's. Remove "geocoderProvider": "photon" from this webhook, or point the URL at a Photon instance.`, + `${photonUrl} answered in Nominatim's format rather than Photon's. Remove "geocoderProvider": "photon" from this webhook, or point the URL at a Photon instance.`, ) } // fetchJson answers a failed request with the Response rather than throwing, diff --git a/server/test/geocoder.test.js b/server/test/geocoder.test.js index 0e2aeca99..da3545ba1 100644 --- a/server/test/geocoder.test.js +++ b/server/test/geocoder.test.js @@ -10,6 +10,7 @@ const { geocoder } = require('../src/services/geocoder') const { formatPhotonFeature, joinComponents, + photonGeocoder, preferBroader, } = require('../src/services/photonGeocoder') @@ -526,3 +527,86 @@ test('a correctly configured Nominatim webhook still geocodes', async () => { await server.close() } }) + +// Nominatim answers /reverse with a single object rather than the array /search +// returns, and gym reverse geocoding is the path resolvers.js takes for every +// fort lookup. Checking only for an array let that mismatch through as an empty +// result set with no reason given. +const NOMINATIM_REVERSE_BODY = { + place_id: 315787599, + lat: '39.7392', + lon: '-104.9903', + display_name: '1437, Bannock Street, Denver, Colorado, 80202, United States', + address: { + house_number: '1437', + road: 'Bannock Street', + city: 'Denver', + country_code: 'us', + }, +} + +// Asserted against photonGeocoder rather than geocoder(), deliberately. +// geocoder() catches everything and returns {}, so a mismatch and a plain miss +// look identical from there -- the whole point of this change is which error +// reaches the log, and only the inner function exposes that. +test('a Nominatim reverse response on the Photon provider raises a provider error', async () => { + const server = await serveOnce(NOMINATIM_REVERSE_BODY) + try { + await assert.rejects( + () => photonGeocoder(server.url, { lat: 39.7392, lon: -104.9903 }, true), + /Nominatim's format/, + ) + } finally { + await server.close() + } +}) + +test('a Nominatim search response on the Photon provider raises a provider error', async () => { + const server = await serveOnce(NOMINATIM_BODY) + try { + await assert.rejects( + () => photonGeocoder(server.url, 'Denver', false), + /Nominatim's format/, + ) + } finally { + await server.close() + } +}) + +// A miss is not a mismatch. An unmatched reverse lookup carries no result +// fields, and an empty result is already the right outcome, so it must not be +// reported as a provider error. +test('a no-match reverse response is not mistaken for a provider mismatch', async () => { + const server = await serveOnce({ error: 'Unable to geocode' }) + try { + const results = await photonGeocoder(server.url, { lat: 0, lon: 0 }, true) + assert.deepEqual(results, []) + } finally { + await server.close() + } +}) + +// The matched pair still works on the reverse path. +test('a correctly configured Photon webhook still reverse geocodes', async () => { + const server = await serveOnce({ + type: 'FeatureCollection', + features: [ + { + geometry: { type: 'Point', coordinates: [-104.9903, 39.7392] }, + properties: { city: 'Denver', state: 'Colorado', countrycode: 'US' }, + }, + ], + }) + try { + const result = await geocoder( + server.url, + { lat: 39.7392, lon: -104.9903 }, + true, + '{{city}}', + 'photon', + ) + assert.equal(result, 'Denver') + } finally { + await server.close() + } +}) From 170258ce9e2d1e1089b08f282f4911bb42bea6ed Mon Sep 17 00:00:00 2001 From: Rin <58572875+TurtIeSocks@users.noreply.github.com> Date: Sun, 23 Aug 2026 20:38:28 -0400 Subject: [PATCH 03/12] fix(geocoder): make the provider mismatch observable against a real Nominatim 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. --- server/src/services/photonGeocoder.js | 17 ++++++ server/test/geocoder.test.js | 88 ++++++++++++++++++++++++++- 2 files changed, 103 insertions(+), 2 deletions(-) diff --git a/server/src/services/photonGeocoder.js b/server/src/services/photonGeocoder.js index 4276d625b..0195d9796 100644 --- a/server/src/services/photonGeocoder.js +++ b/server/src/services/photonGeocoder.js @@ -1,5 +1,7 @@ // @ts-check +const { Response } = require('node-fetch') + const { fetchJson } = require('../utils/fetchJson') /** @@ -275,6 +277,12 @@ async function photonGeocoder(photonUrl, search, isReverse) { lat: search.lat, lon: search.lon, limit: 1, + // Photon ignores this parameter. Nominatim's /reverse defaults to + // XML, which fetchJson cannot parse, so a misconfigured webhook + // would produce an unreadable body rather than one this function can + // recognise and report. Asking for JSON costs nothing and keeps the + // mismatch diagnosable. + format: 'json', }) : buildUrl(photonUrl, '/api', { q: String(search), @@ -287,6 +295,15 @@ async function photonGeocoder(photonUrl, search, isReverse) { // single object. Checking only the array would let gym reverse geocoding // fall through to an empty result set with no reason given, which is the // path resolvers.js takes for every fort lookup. + // fetchJson hands back the Response itself when the request failed, so this + // covers every non-2xx. Photon serves /api, and a Nominatim host has no such + // endpoint at all: it answers 404 whatever format is asked for, which is the + // forward half of the same misconfiguration. + if (response instanceof Response) { + throw new Error( + `${photonUrl} answered ${response.status} for Photon's ${response.status === 404 ? '/api endpoint, so it is not a Photon instance' : 'request'}. Check the URL, or remove "geocoderProvider": "photon" from this webhook.`, + ) + } if (isNominatimResponse(response)) { throw new Error( `${photonUrl} answered in Nominatim's format rather than Photon's. Remove "geocoderProvider": "photon" from this webhook, or point the URL at a Photon instance.`, diff --git a/server/test/geocoder.test.js b/server/test/geocoder.test.js index da3545ba1..45c8144c6 100644 --- a/server/test/geocoder.test.js +++ b/server/test/geocoder.test.js @@ -549,8 +549,49 @@ const NOMINATIM_REVERSE_BODY = { // geocoder() catches everything and returns {}, so a mismatch and a plain miss // look identical from there -- the whole point of this change is which error // reaches the log, and only the inner function exposes that. +// Behaves the way a real Nominatim host does, rather than answering the same +// JSON on every path. That distinction matters: a stub that always returns +// Nominatim JSON made these checks look like they worked when they did not. +// +// /api Photon's endpoint. Nominatim has none, so it answers 404. +// /reverse defaults to XML unless format=json is asked for. +// /search the array shape. +const serveNominatim = async () => { + const server = http.createServer((req, res) => { + const url = new URL(req.url, 'http://127.0.0.1') + const json = (code, body) => { + res.writeHead(code, { 'Content-Type': 'application/json' }) + res.end(JSON.stringify(body)) + } + if (url.pathname === '/reverse') { + if (url.searchParams.get('format') !== 'json') { + res.writeHead(200, { 'Content-Type': 'text/xml' }) + res.end('') + return + } + json(200, NOMINATIM_REVERSE_BODY) + return + } + if (url.pathname === '/search') { + json(200, NOMINATIM_BODY) + return + } + json(404, { title: '404 Not Found' }) + }) + await new Promise((resolve) => { + server.listen(0, '127.0.0.1', resolve) + }) + return { + url: `http://127.0.0.1:${server.address().port}`, + close: () => + new Promise((resolve) => { + server.close(resolve) + }), + } +} + test('a Nominatim reverse response on the Photon provider raises a provider error', async () => { - const server = await serveOnce(NOMINATIM_REVERSE_BODY) + const server = await serveNominatim() try { await assert.rejects( () => photonGeocoder(server.url, { lat: 39.7392, lon: -104.9903 }, true), @@ -561,7 +602,50 @@ test('a Nominatim reverse response on the Photon provider raises a provider erro } }) -test('a Nominatim search response on the Photon provider raises a provider error', async () => { +// The forward half of the same misconfiguration. Nominatim has no /api, so it +// answers 404 rather than a readable body, and the previous check could never +// have seen a Nominatim search array here. +test('a Nominatim host on the Photon provider raises an error on the forward path', async () => { + const server = await serveNominatim() + try { + await assert.rejects( + () => photonGeocoder(server.url, 'Denver', false), + /not a Photon instance/, + ) + } finally { + await server.close() + } +}) + +// The classifier only ever sees a JSON body because the request asks for one. +// Without format=json this endpoint answers XML, fetchJson cannot parse it, and +// the mismatch goes unreported. +test('the reverse request asks Nominatim for JSON so the mismatch is visible', async () => { + const seen = [] + const server = http.createServer((req, res) => { + seen.push(req.url) + res.writeHead(200, { 'Content-Type': 'application/json' }) + res.end(JSON.stringify({ type: 'FeatureCollection', features: [] })) + }) + await new Promise((resolve) => { + server.listen(0, '127.0.0.1', resolve) + }) + try { + await photonGeocoder( + `http://127.0.0.1:${server.address().port}`, + { lat: 39.7392, lon: -104.9903 }, + true, + ) + assert.match(seen[0], /format=json/) + } finally { + await new Promise((resolve) => { + server.close(resolve) + }) + } +}) + +// A JSON array is still Nominatim's /search shape, whatever served it. +test('a Nominatim search array on the Photon provider raises a provider error', async () => { const server = await serveOnce(NOMINATIM_BODY) try { await assert.rejects( From d3d9347356afb7d88ebbb1c76464d10315776707 Mon Sep 17 00:00:00 2001 From: Rin <58572875+TurtIeSocks@users.noreply.github.com> Date: Sun, 23 Aug 2026 20:45:56 -0400 Subject: [PATCH 04/12] fix(geocoder): correct the Photon mapping against a live index 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. --- server/src/services/photonGeocoder.js | 77 +++++++++------ server/test/geocoder.test.js | 129 ++++++++++++++++++-------- 2 files changed, 139 insertions(+), 67 deletions(-) diff --git a/server/src/services/photonGeocoder.js b/server/src/services/photonGeocoder.js index 0195d9796..d98378615 100644 --- a/server/src/services/photonGeocoder.js +++ b/server/src/services/photonGeocoder.js @@ -21,8 +21,13 @@ const { fetchJson } = require('../utils/fetchJson') * @property {string} [street] * @property {string} [postcode] * @property {string} [city] - * @property {string} [locality] A settlement below Photon's city layer, such - * as the hamlet a rural address sits in. Present when `city` is not. + * @property {string} [locality] The finest named area containing the result, + * equivalent to Nominatim's `neighbourhood` or `quarter`. Verified against a + * live index: Photon's `locality` for 1521 N Hoyne Ave is "Wicker Park", and + * Nominatim's `quarter` for the same building is "Wicker Park". + * @property {string} [district] The city subdivision containing the result, + * equivalent to Nominatim's `suburb` or `borough`. Same building: Photon's + * `district` is "West Town", Nominatim's `suburb` is "West Town". * @property {string} [county] * @property {string} [state] * @property {string} [country] @@ -145,12 +150,19 @@ function preferBroader(parts) { function buildFormattedAddress(properties, settlement) { const street = joinComponents([properties.housenumber, properties.street]) + // locality and district sit between the street and the settlement, which is + // where Nominatim puts the levels they correspond to. For 1521 N Hoyne Ave it + // renders "1521, North Hoyne Avenue, Wicker Park, West Town, ...", matching + // the order of Nominatim's own quarter and suburb for that building. + // // The result's own name leads. When it was already echoed into a hierarchy // field, joinComponents drops the repeat rather than printing it twice. return joinComponents([ properties.name, ...preferBroader([ street, + properties.locality, + properties.district, settlement, properties.county, properties.state, @@ -198,31 +210,24 @@ function formatPhotonFeature(feature) { const town = named('town') const village = named('village') - // Photon's hierarchy runs city > district > locality > street, so an address - // whose containing settlement sits below the city layer carries that name in - // `locality` and has no `city` at all. Without it a rural address loses its - // settlement from both `city` and formattedAddress. Named `settlement` rather - // than `locality` so it is not confused with the Photon field it falls back - // to. // properties.city covers both what Photon sent and what resolveSelfReference // placed there from the type layer; named('city') covers a place=city tagged // result that carries no type at all. + // + // `locality` is deliberately not in this chain. It is neighbourhood-level + // rather than a smaller settlement: a live index returns locality "Wicker + // Park" alongside a city, and a genuine hamlet such as Bootjack arrives as + // osm_value=locality with its name in `name` and no `locality` field at all. + // Reporting it as the city would name a neighbourhood as a town. const settlement = - properties.city || - named('city') || - town || - village || - named('hamlet') || - properties.locality || - '' + properties.city || named('city') || town || village || named('hamlet') || '' return { latitude, longitude, formattedAddress: buildFormattedAddress(properties, settlement), country: properties.country, - // Mirrors _formatResult's own city/town/village/hamlet fallback, with - // Photon's locality layer appended to it. + // Mirrors _formatResult's own city/town/village/hamlet fallback. city: settlement || undefined, state: properties.state, zipcode: properties.postcode, @@ -233,8 +238,11 @@ function formatPhotonFeature(feature) { // Photon already sends this upper-case, which is what node-geocoder // produces after upper-casing Nominatim's lower-case value. countryCode: properties.countrycode, - neighbourhood: '', - suburb: '', + // Both verified against a live Photon index rather than inferred: for the + // same building, Photon's locality and district carry the values Nominatim + // returns as quarter and suburb. + neighbourhood: properties.locality || '', + suburb: properties.district || '', town: town || '', village: village || '', } @@ -277,28 +285,39 @@ async function photonGeocoder(photonUrl, search, isReverse) { lat: search.lat, lon: search.lon, limit: 1, - // Photon ignores this parameter. Nominatim's /reverse defaults to - // XML, which fetchJson cannot parse, so a misconfigured webhook - // would produce an unreadable body rather than one this function can - // recognise and report. Asking for JSON costs nothing and keeps the - // mismatch diagnosable. - format: 'json', }) : buildUrl(photonUrl, '/api', { q: String(search), limit: SEARCH_LIMIT, }) - const response = await fetchJson(url) + let response + try { + response = await fetchJson(url) + } catch (err) { + // fetchJson does not catch this itself: it does `return response.json()` + // inside its try block, so a body that fails to parse rejects the promise + // it already returned and escapes its local catch. + // + // This is the reverse half of the provider mismatch. Nominatim's /reverse + // defaults to XML and only sends JSON when asked, and Photon rejects any + // parameter outside its allow list with a 400, so asking would break every + // correctly configured Photon. Recognising the unparseable body instead + // costs nothing and needs no extra parameter. + 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})`, + ) + } // The mirror of the Nominatim check. Both of Nominatim's shapes have to be // recognised: /search answers with an array, and /reverse answers with a // single object. Checking only the array would let gym reverse geocoding // fall through to an empty result set with no reason given, which is the // path resolvers.js takes for every fort lookup. // fetchJson hands back the Response itself when the request failed, so this - // covers every non-2xx. Photon serves /api, and a Nominatim host has no such - // endpoint at all: it answers 404 whatever format is asked for, which is the - // forward half of the same misconfiguration. + // covers every non-2xx. Photon serves /api and a Nominatim host has no such + // endpoint, so a forward search against a misconfigured webhook answers 404 + // and is reported here. + if (response instanceof Response) { throw new Error( `${photonUrl} answered ${response.status} for Photon's ${response.status === 404 ? '/api endpoint, so it is not a Photon instance' : 'request'}. Check the URL, or remove "geocoderProvider": "photon" from this webhook.`, diff --git a/server/test/geocoder.test.js b/server/test/geocoder.test.js index 45c8144c6..efae0001f 100644 --- a/server/test/geocoder.test.js +++ b/server/test/geocoder.test.js @@ -137,56 +137,74 @@ test('echoes the result name into its locality field', () => { // sends no `city` at all. Reading only `city` drops the settlement from both // the entry and the formatted address, so a {{city}} format renders blank for // every rural address. -test('falls back to Photon locality when there is no city', () => { +// Captured verbatim from a live Photon index: reverse at 41.9088,-87.6796. +// Nominatim's own answer for the same building carries quarter "Wicker Park" +// and suburb "West Town", which is what fixes these two mappings. +const WICKER_PARK_REVERSE = { + city: 'West Chicago Township', + country: 'United States', + countrycode: 'US', + county: 'Cook County', + district: 'West Town', + housenumber: '1521', + locality: 'Wicker Park', + osm_key: 'building', + osm_value: 'yes', + postcode: '60647', + state: 'Illinois', + street: 'North Hoyne Avenue', + type: 'house', +} + +test("maps Photon's locality to neighbourhood and district to suburb", () => { const got = formatPhotonFeature( - feature( - { - housenumber: '4', - street: 'County Road 15', - locality: 'Bootjack', - county: 'Mariposa County', - state: 'California', - postcode: '95338', - country: 'United States', - countrycode: 'US', - osm_key: 'building', - osm_value: 'yes', - }, - [-119.9515, 37.4744], - ), + feature(WICKER_PARK_REVERSE, [-87.6796, 41.9088]), + ) + assert.equal(got.neighbourhood, 'Wicker Park') + assert.equal(got.suburb, 'West Town') + // Neither is the settlement. Reporting a neighbourhood as the city would name + // an area of Chicago as a town. + assert.equal(got.city, 'West Chicago Township') +}) + +test('places locality and district between the street and the settlement', () => { + const got = formatPhotonFeature( + feature(WICKER_PARK_REVERSE, [-87.6796, 41.9088]), ) - assert.equal(got.city, 'Bootjack') assert.equal( got.formattedAddress, - '4, County Road 15, Bootjack, Mariposa County, California, 95338, United States', + '1521, North Hoyne Avenue, Wicker Park, West Town, West Chicago Township, Cook County, Illinois, 60647, United States', ) }) -// Photon's own hierarchy puts city above locality, so a response carrying both -// must not demote the city. -test('prefers city over locality when Photon sends both', () => { +// A hamlet does not arrive in `locality`. Live index, q=Bootjack California: +// osm_value=locality, type=other, name carries the hamlet, no locality field. +test('a place tagged as a locality keeps its name out of the address fields', () => { const got = formatPhotonFeature( feature({ - city: 'Mariposa', - locality: 'Bootjack', + name: 'Bootjack', + type: 'other', + osm_key: 'place', + osm_value: 'locality', + county: 'Mariposa', state: 'California', country: 'United States', countrycode: 'US', }), ) - assert.equal(got.city, 'Mariposa') + assert.equal(got.neighbourhood, '') + assert.equal( + got.formattedAddress, + 'Bootjack, Mariposa, California, United States', + ) }) -// Photon reports the result's own layer in properties.type, and a city, state -// or country is usually an administrative boundary in OSM, arriving as -// osm_key=boundary. Gating self-reference on osm_key=place alone would keep -// only the exception and drop the common case. test('places the result name using the Photon type layer', () => { const cases = [ { type: 'city', name: 'Denver', field: 'city' }, { type: 'state', name: 'Illinois', field: 'state' }, { type: 'country', name: 'United States', field: 'country' }, - { type: 'locality', name: 'Bootjack', field: 'city' }, + { type: 'locality', name: 'Wicker Park', field: 'neighbourhood' }, { type: 'street', name: 'Lake Shore Drive', field: 'streetName' }, ] cases.forEach(({ type, name, field }) => { @@ -296,7 +314,7 @@ test("Photon's own city wins over the echoed name", () => { // Photon's nearest field is `district`, a different OSM concept. Equating them // would be an invention rather than a translation. -test('leaves suburb and neighbourhood empty', () => { +test('leaves suburb and neighbourhood empty when Photon sends neither', () => { const got = formatPhotonFeature(STREET_ADDRESS) assert.equal(got.suburb, '') assert.equal(got.neighbourhood, '') @@ -590,12 +608,21 @@ const serveNominatim = async () => { } } +// Documents a limitation rather than a behaviour. Nominatim's /reverse answers +// XML unless format=json is asked for, and Photon rejects any parameter outside +// its allow list with a 400, so asking would break every correctly configured +// Photon. The body therefore fails to parse and is logged as a failed fetch. +// The forward path below is where this misconfiguration is actually reported. +// Nominatim's /reverse answers XML unless format=json is asked for, and Photon +// rejects any parameter outside its allow list with a 400, so asking would +// break every correctly configured Photon. The unparseable body is the signal +// instead. test('a Nominatim reverse response on the Photon provider raises a provider error', async () => { const server = await serveNominatim() try { await assert.rejects( () => photonGeocoder(server.url, { lat: 39.7392, lon: -104.9903 }, true), - /Nominatim's format/, + /not a Photon instance|Photon provider|answers \/reverse with XML/, ) } finally { await server.close() @@ -620,7 +647,27 @@ test('a Nominatim host on the Photon provider raises an error on the forward pat // The classifier only ever sees a JSON body because the request asks for one. // Without format=json this endpoint answers XML, fetchJson cannot parse it, and // the mismatch goes unreported. -test('the reverse request asks Nominatim for JSON so the mismatch is visible', async () => { +// The request must carry nothing outside Photon's allow list. A live instance +// answers 400 to any unknown parameter, so an extra one added to help diagnose +// a misconfiguration would break every correctly configured deployment. +test('the outgoing requests send only parameters Photon accepts', async () => { + const PHOTON_PARAMS = new Set([ + 'include', + 'debug', + 'dedupe', + 'query_string_filter', + 'lon', + 'layer', + 'limit', + 'osm_tag', + 'distance_sort', + 'geometry', + 'exclude', + 'lang', + 'radius', + 'lat', + 'q', + ]) const seen = [] const server = http.createServer((req, res) => { seen.push(req.url) @@ -630,13 +677,19 @@ test('the reverse request asks Nominatim for JSON so the mismatch is visible', a await new Promise((resolve) => { server.listen(0, '127.0.0.1', resolve) }) + const url = `http://127.0.0.1:${server.address().port}` try { - await photonGeocoder( - `http://127.0.0.1:${server.address().port}`, - { lat: 39.7392, lon: -104.9903 }, - true, - ) - assert.match(seen[0], /format=json/) + await photonGeocoder(url, 'Denver', false) + await photonGeocoder(url, { lat: 39.7392, lon: -104.9903 }, true) + seen.forEach((requested) => { + const params = new URL(requested, 'http://127.0.0.1').searchParams + params.forEach((_, key) => { + assert.ok( + PHOTON_PARAMS.has(key), + `${key} is not a parameter Photon accepts (${requested})`, + ) + }) + }) } finally { await new Promise((resolve) => { server.close(resolve) From 0e168b6f1118feb7bc6df6d58bf18642606fe9f4 Mon Sep 17 00:00:00 2001 From: Rin <58572875+TurtIeSocks@users.noreply.github.com> Date: Sun, 23 Aug 2026 23:56:08 -0400 Subject: [PATCH 05/12] fix(geocoder): keep the caller's shape on failure and resolve district 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. --- server/src/services/geocoder.js | 7 ++- server/src/services/photonGeocoder.js | 5 +++ server/test/geocoder.test.js | 65 ++++++++++++++++++++++++++- 3 files changed, 74 insertions(+), 3 deletions(-) diff --git a/server/src/services/geocoder.js b/server/src/services/geocoder.js index 1f431d162..61fcdf608 100644 --- a/server/src/services/geocoder.js +++ b/server/src/services/geocoder.js @@ -108,7 +108,12 @@ async function geocoder(nominatimUrl, search, reverse, format, provider) { : results } catch (e) { log.warn(TAGS.geocoder, 'Unable to geocode for', search, e) - return {} + // The fallback has to match the shape the caller's schema expects. + // 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. The failure is + // still reported: it is logged immediately above. + return reverse ? '' : [] } } diff --git a/server/src/services/photonGeocoder.js b/server/src/services/photonGeocoder.js index d98378615..6a8d43a15 100644 --- a/server/src/services/photonGeocoder.js +++ b/server/src/services/photonGeocoder.js @@ -69,6 +69,11 @@ const TYPE_TO_PROPERTY = { state: 'state', county: 'county', city: 'city', + // A search for a suburb by name comes back as type=district with the label in + // `name` and no `district` field, so without this the result being asked for + // is the one value missing from the answer. Live index, q=West Town Chicago: + // {name: "West Town", type: "district", osm_key: "place", osm_value: "suburb"} + district: 'district', locality: 'locality', street: 'street', } diff --git a/server/test/geocoder.test.js b/server/test/geocoder.test.js index efae0001f..2cbe0720e 100644 --- a/server/test/geocoder.test.js +++ b/server/test/geocoder.test.js @@ -492,7 +492,7 @@ test('a Photon URL on the Nominatim provider fails without crashing', async () = // geocoder() catches and returns {}. What matters is that the process // survives to get here at all. const result = await geocoder(server.url, 'Denver', false, '{{city}}') - assert.deepEqual(result, {}) + assert.deepEqual(result, []) } finally { await server.close() } @@ -508,7 +508,7 @@ test('a Nominatim URL on the Photon provider fails without returning nothing sil '{{city}}', 'photon', ) - assert.deepEqual(result, {}) + assert.deepEqual(result, []) } finally { await server.close() } @@ -747,3 +747,64 @@ test('a correctly configured Photon webhook still reverse geocodes', async () => await server.close() } }) + +// Captured from a live index, q=West Town Chicago. A suburb searched by name +// comes back as its own result: the label is in `name` and the containing +// `district` field is absent, so without a district entry in the type map the +// one value being asked for is the one missing from the answer. +test('a district searched by name populates suburb', () => { + const got = formatPhotonFeature( + feature( + { + city: 'West Chicago Township', + country: 'United States', + countrycode: 'US', + county: 'Cook County', + name: 'West Town', + osm_key: 'place', + osm_value: 'suburb', + state: 'Illinois', + type: 'district', + }, + [-87.6796, 41.9088], + ), + ) + assert.equal(got.suburb, 'West Town') + assert.equal( + got.formattedAddress, + 'West Town, West Chicago Township, Cook County, Illinois, United States', + ) +}) + +// Query.geocoder is [Geocoder] and Gym.formatted is String. An object fallback +// makes GraphQL discard the field entirely rather than degrade to an empty +// answer, so a transient upstream failure has to keep the caller's shape. +test('a transient upstream failure keeps the shape the schema expects', async () => { + const failing = http.createServer((_, res) => { + res.writeHead(500, { 'Content-Type': 'application/json' }) + res.end(JSON.stringify({ message: 'overloaded' })) + }) + await new Promise((resolve) => { + failing.listen(0, '127.0.0.1', resolve) + }) + const url = `http://127.0.0.1:${failing.address().port}` + try { + const forward = await geocoder(url, 'Denver', false, '{{city}}', 'photon') + assert.ok(Array.isArray(forward), `forward returned ${typeof forward}`) + assert.deepEqual(forward, []) + + const reverse = await geocoder( + url, + { lat: 39.7392, lon: -104.9903 }, + true, + '{{city}}', + 'photon', + ) + assert.equal(typeof reverse, 'string', 'reverse must stay a String') + assert.equal(reverse, '') + } finally { + await new Promise((resolve) => { + failing.close(resolve) + }) + } +}) From c1e868e518dfda4c3ad25f59d53c5cb51c47e75f Mon Sep 17 00:00:00 2001 From: Rin <58572875+TurtIeSocks@users.noreply.github.com> Date: Mon, 24 Aug 2026 12:57:47 -0400 Subject: [PATCH 06/12] fix(geocoder): report a Photon URL left on the Nominatim provider 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. --- server/src/services/geocoder.js | 52 ++++++-- server/src/services/photonGeocoder.js | 5 + server/test/geocoder.test.js | 164 ++++++++++++++++++++++++-- 3 files changed, 204 insertions(+), 17 deletions(-) diff --git a/server/src/services/geocoder.js b/server/src/services/geocoder.js index 61fcdf608..c0ce0413e 100644 --- a/server/src/services/geocoder.js +++ b/server/src/services/geocoder.js @@ -12,7 +12,7 @@ const { photonGeocoder } = require('./photonGeocoder') function formatter(addressFormat, result) { return addressFormat .replace( - /{{(streetNumber|streetName|city|state|country|zipcode|latitude|longitude|countryCode|neighborhoods|suburb|town|village)}}/g, + /{{(streetNumber|streetName|city|state|country|zipcode|latitude|longitude|countryCode|neighborhoods|neighborhood|neighbourhood|suburb|town|village)}}/g, (a, b) => result[b] || '', ) .trim() @@ -38,9 +38,33 @@ function formatter(addressFormat, result) { */ function assertNominatimResponse(results, url) { const raw = results?.raw - if (raw && !Array.isArray(raw) && raw.type === 'FeatureCollection') { + if (!raw || Array.isArray(raw) || typeof raw !== 'object') return + + // Photon rarely gets far enough to answer with GeoJSON here, because + // node-geocoder asks for routes and parameters it does not serve. Verified + // against a live instance: + // + // GET /search?q=..&format=json&addressdetails=1 + // 404 {"title":"Endpoint GET /search not found","status":404,...} + // Photon serves /api, not /search. + // + // GET /reverse?lat=..&lon=..&format=json&addressdetails=1 + // 400 {"message":"Unknown query parameter 'format'. Allowed parameters + // are: [include, debug, dedupe, ...]"} + // Photon rejects anything outside its allow list, and format and + // addressdetails are forced onto every request by node-geocoder. + // + // node-geocoder ignores the status and parses the body regardless, so both + // arrive here as an object with no address and format into a blank result. + const isPhoton = + raw.type === 'FeatureCollection' || + (typeof raw.title === 'string' && typeof raw.status === 'number') || + (typeof raw.message === 'string' && + raw.message.includes('Unknown query parameter')) + + if (isPhoton) { throw new Error( - `${url} answered with GeoJSON, which is Photon's format rather than Nominatim's. Set "geocoderProvider": "photon" on this webhook, or point the URL at a Nominatim instance.`, + `${url} answered as a Photon instance rather than a Nominatim one. Set "geocoderProvider": "photon" on this webhook, or point the URL at a Nominatim instance.`, ) } } @@ -57,12 +81,20 @@ async function nominatimGeocoder(url, search, isReverse) { osmServer: url, timeout: 5000, }) - stockGeocoder._geocoder._formatResult = ((original) => (result) => ({ - ...original(result), - suburb: result.address?.suburb || '', - town: result.address?.town || '', - village: result.address?.village || '', - }))(stockGeocoder._geocoder._formatResult) + stockGeocoder._geocoder._formatResult = ((original) => (result) => { + const formatted = original(result) + return { + ...formatted, + suburb: result.address?.suburb || '', + town: result.address?.town || '', + village: result.address?.village || '', + // node-geocoder emits the British spelling. The GraphQL schema exposes + // `neighborhood` and formatter() templates on `neighborhoods`, so the + // value reached neither consumer. Carrying the alias is what makes it + // visible without changing what node-geocoder itself produces. + neighborhood: formatted.neighbourhood || '', + } + })(stockGeocoder._geocoder._formatResult) // Awaited rather than returned so the shape check runs here. A throw inside // _formatResult would not reach geocoder()'s catch at all: node-geocoder // resolves through bluebird's asCallback, so it surfaces as an uncaught @@ -117,4 +149,4 @@ async function geocoder(nominatimUrl, search, reverse, format, provider) { } } -module.exports = { geocoder, formatter } +module.exports = { geocoder, formatter, nominatimGeocoder } diff --git a/server/src/services/photonGeocoder.js b/server/src/services/photonGeocoder.js index 6a8d43a15..fb08db4fb 100644 --- a/server/src/services/photonGeocoder.js +++ b/server/src/services/photonGeocoder.js @@ -247,6 +247,11 @@ function formatPhotonFeature(feature) { // same building, Photon's locality and district carry the values Nominatim // returns as quarter and suburb. neighbourhood: properties.locality || '', + // The same value under the spelling the GraphQL schema and formatter use. + // Geocoder.neighborhood is American and formatter() templates on + // neighborhoods, while node-geocoder emits neighbourhood, so without the + // alias the mapped value reaches no consumer at all. + neighborhood: properties.locality || '', suburb: properties.district || '', town: town || '', village: village || '', diff --git a/server/test/geocoder.test.js b/server/test/geocoder.test.js index 2cbe0720e..f5bb64347 100644 --- a/server/test/geocoder.test.js +++ b/server/test/geocoder.test.js @@ -6,7 +6,11 @@ const NodeGeocoder = require('node-geocoder') const http = require('node:http') const { PoracleAPI } = require('../src/services/Poracle') -const { geocoder } = require('../src/services/geocoder') +const { + formatter, + geocoder, + nominatimGeocoder, +} = require('../src/services/geocoder') const { formatPhotonFeature, joinComponents, @@ -59,6 +63,7 @@ test('maps a Photon city onto the geocoder entry shape', () => { streetNumber: undefined, countryCode: 'US', neighbourhood: '', + neighborhood: '', suburb: '', town: '', village: '', @@ -374,12 +379,18 @@ test('the Photon path emits the same keys as the Nominatim path', () => { osmServer: 'http://127.0.0.1:0', timeout: 5000, }) - stockGeocoder._geocoder._formatResult = ((original) => (result) => ({ - ...original(result), - suburb: result.address.suburb || '', - town: result.address.town || '', - village: result.address.village || '', - }))(stockGeocoder._geocoder._formatResult.bind(stockGeocoder._geocoder)) + // Mirrors the patch in geocoder.js, alias included. The duplication is the + // point: if the two drift apart, this test stops proving parity. + stockGeocoder._geocoder._formatResult = ((original) => (result) => { + const formatted = original(result) + return { + ...formatted, + suburb: result.address?.suburb || '', + town: result.address?.town || '', + village: result.address?.village || '', + neighborhood: formatted.neighbourhood || '', + } + })(stockGeocoder._geocoder._formatResult.bind(stockGeocoder._geocoder)) // The same place as STREET_ADDRESS, in Nominatim's response shape. const fromNominatim = stockGeocoder._geocoder._formatResult({ @@ -808,3 +819,142 @@ test('a transient upstream failure keeps the shape the schema expects', async () }) } }) + +// Behaves like a real Photon instance rather than accepting anything: it serves +// /api and /reverse only, and rejects any parameter outside its allow list. +// node-geocoder forces format and addressdetails onto every request and calls +// /search, so a Photon URL left on the Nominatim provider never reaches a +// FeatureCollection at all. Captured from a live instance. +const servePhoton = async () => { + const ALLOWED = new Set([ + 'include', + 'debug', + 'dedupe', + 'query_string_filter', + 'lon', + 'layer', + 'limit', + 'osm_tag', + 'distance_sort', + 'geometry', + 'exclude', + 'lang', + 'radius', + 'lat', + 'q', + ]) + const server = http.createServer((req, res) => { + const url = new URL(req.url, 'http://127.0.0.1') + const json = (code, body) => { + res.writeHead(code, { 'Content-Type': 'application/json' }) + res.end(JSON.stringify(body)) + } + if (url.pathname !== '/api' && url.pathname !== '/reverse') { + json(404, { + title: `Endpoint ${req.method} ${url.pathname} not found`, + status: 404, + type: 'https://javalin.io/documentation#endpointnotfound', + details: {}, + }) + return + } + const bad = [...url.searchParams.keys()].find((k) => !ALLOWED.has(k)) + if (bad) { + json(400, { + message: `Unknown query parameter '${bad}'. Allowed parameters are: [${[...ALLOWED].join(', ')}]`, + }) + return + } + json(200, { type: 'FeatureCollection', features: [] }) + }) + await new Promise((resolve) => { + server.listen(0, '127.0.0.1', resolve) + }) + return { + url: `http://127.0.0.1:${server.address().port}`, + close: () => + new Promise((resolve) => { + server.close(resolve) + }), + } +} + +// The exact production misconfiguration: a Photon URL inherited from Poracle +// with no geocoderProvider set. It used to format into a single blank result. +test('a Photon URL on the Nominatim provider is reported on the forward path', async () => { + const server = await servePhoton() + try { + await assert.rejects( + () => nominatimGeocoder(server.url, 'Denver', false), + /answered as a Photon instance/, + ) + } finally { + await server.close() + } +}) + +test('a Photon URL on the Nominatim provider is reported on the reverse path', async () => { + const server = await servePhoton() + try { + await assert.rejects( + () => + nominatimGeocoder(server.url, { lat: 39.7392, lon: -104.9903 }, true), + /answered as a Photon instance/, + ) + } finally { + await server.close() + } +}) + +// The value has to reach the two consumers that actually exist: the GraphQL +// field is `neighborhood` and formatter() templates on `neighborhoods`, while +// node-geocoder produces `neighbourhood`. +test('locality reaches the public neighborhood contract', () => { + const got = formatPhotonFeature( + feature(WICKER_PARK_REVERSE, [-87.6796, 41.9088]), + ) + assert.equal(got.neighbourhood, 'Wicker Park') + assert.equal(got.neighborhood, 'Wicker Park') + assert.equal(formatter('{{neighborhood}}', got), 'Wicker Park') + assert.equal(formatter('{{neighbourhood}}', got), 'Wicker Park') +}) + +// The alias has to exist on the Nominatim path too. node-geocoder emits +// neighbourhood there, and the GraphQL field and formatter token both use the +// American spellings, so a Nominatim deployment had the same invisible value. +test('the Nominatim path also carries the neighborhood alias', async () => { + const server = http.createServer((_, res) => { + res.writeHead(200, { 'Content-Type': 'application/json' }) + res.end( + JSON.stringify([ + { + lat: '41.9088', + lon: '-87.6796', + display_name: 'Wicker Park, Chicago', + address: { + neighbourhood: 'Wicker Park', + city: 'Chicago', + country_code: 'us', + }, + }, + ]), + ) + }) + await new Promise((resolve) => { + server.listen(0, '127.0.0.1', resolve) + }) + try { + const [entry] = await nominatimGeocoder( + `http://127.0.0.1:${server.address().port}`, + 'Wicker Park', + false, + ) + assert.equal(entry.neighbourhood, 'Wicker Park') + assert.equal(entry.neighborhood, 'Wicker Park') + assert.equal(formatter('{{neighborhood}}', entry), 'Wicker Park') + } finally { + await new Promise((resolve) => { + server.close(resolve) + }) + } +}) From 02de8dc2586a67425681791bcef48cb8c4a7d175 Mon Sep 17 00:00:00 2001 From: Rin <58572875+TurtIeSocks@users.noreply.github.com> Date: Mon, 24 Aug 2026 13:10:51 -0400 Subject: [PATCH 07/12] fix(geocoder): do not query the upstream for an empty search 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. --- server/src/services/geocoder.js | 36 ++++++++++++---- server/test/geocoder.test.js | 73 +++++++++++++++++++++++++++++++++ 2 files changed, 102 insertions(+), 7 deletions(-) diff --git a/server/src/services/geocoder.js b/server/src/services/geocoder.js index c0ce0413e..8a56af62e 100644 --- a/server/src/services/geocoder.js +++ b/server/src/services/geocoder.js @@ -20,6 +20,18 @@ function formatter(addressFormat, result) { .trim() } +/** + * The empty answer for a caller, in the shape its schema declares. + * + * Query.geocoder is [Geocoder] and Gym.formatted is String, so an object here + * makes GraphQL discard the field with "Expected Iterable" or "String cannot + * represent value" rather than degrading to an empty result. + * @param {boolean} reverse + */ +function emptyResult(reverse) { + return reverse ? '' : [] +} + /** * Fails loudly when the configured URL answered with something that is not a * Nominatim response. @@ -124,7 +136,22 @@ async function geocoder(nominatimUrl, search, reverse, format, provider) { // A coordinate pair means a reverse lookup. `reverse` separately controls // whether a single formatted string comes back, so the two are not // interchangeable. - const isReverse = typeof search === 'object' + // + // The null check is load-bearing: typeof null is 'object', so a null search + // would otherwise be taken for a coordinate pair and sent to /reverse with + // no lat or lon at all. + const isReverse = typeof search === 'object' && search !== null + + // Clearing the search box sends an empty string, and there is nothing to + // look up. Photon rejects it outright with + // {"message":"q parameter is required when no include categories are + // specified"} and a 400, which now surfaces as an error and fills the log + // with failures caused by a user deleting their own input. Answering + // directly costs nothing and keeps a blank box quiet. + if (!isReverse && !String(search ?? '').trim()) { + return emptyResult(reverse) + } + const results = provider === 'photon' ? await photonGeocoder(nominatimUrl, search, isReverse) @@ -140,12 +167,7 @@ async function geocoder(nominatimUrl, search, reverse, format, provider) { : results } catch (e) { log.warn(TAGS.geocoder, 'Unable to geocode for', search, e) - // The fallback has to match the shape the caller's schema expects. - // 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. The failure is - // still reported: it is logged immediately above. - return reverse ? '' : [] + return emptyResult(reverse) } } diff --git a/server/test/geocoder.test.js b/server/test/geocoder.test.js index f5bb64347..d57616d29 100644 --- a/server/test/geocoder.test.js +++ b/server/test/geocoder.test.js @@ -958,3 +958,76 @@ test('the Nominatim path also carries the neighborhood alias', async () => { }) } }) + +// Clearing the search box sends an empty string. Photon rejects it with a 400, +// which surfaces as an error, so a user deleting their own input would fill the +// log with failures. Nothing should leave the process at all. +test('an empty search never reaches the upstream', async () => { + const seen = [] + const server = http.createServer((req, res) => { + seen.push(req.url) + res.writeHead(200, { 'Content-Type': 'application/json' }) + res.end(JSON.stringify({ type: 'FeatureCollection', features: [] })) + }) + await new Promise((resolve) => { + server.listen(0, '127.0.0.1', resolve) + }) + const url = `http://127.0.0.1:${server.address().port}` + try { + const blank = ['', ' ', '\t\n', undefined, null] + // eslint-disable-next-line no-restricted-syntax + await Promise.all( + blank.map(async (search) => { + const photon = await geocoder(url, search, false, '{{city}}', 'photon') + assert.deepEqual( + photon, + [], + `photon returned a result for ${JSON.stringify(search)}`, + ) + const nominatim = await geocoder(url, search, false, '{{city}}') + assert.deepEqual( + nominatim, + [], + `nominatim returned a result for ${JSON.stringify(search)}`, + ) + }), + ) + assert.deepEqual(seen, [], `requests were sent: ${seen.join(', ')}`) + + // A real query still goes out, so the guard is not swallowing everything. + await geocoder(url, 'Denver', false, '{{city}}', 'photon') + assert.equal(seen.length, 1) + } finally { + await new Promise((resolve) => { + server.close(resolve) + }) + } +}) + +// A reverse lookup at 0,0 is a real coordinate pair, not an empty search. +test('the empty guard does not swallow a zero coordinate', async () => { + const seen = [] + const server = http.createServer((req, res) => { + seen.push(req.url) + res.writeHead(200, { 'Content-Type': 'application/json' }) + res.end(JSON.stringify({ type: 'FeatureCollection', features: [] })) + }) + await new Promise((resolve) => { + server.listen(0, '127.0.0.1', resolve) + }) + try { + await geocoder( + `http://127.0.0.1:${server.address().port}`, + { lat: 0, lon: 0 }, + true, + '{{city}}', + 'photon', + ) + assert.equal(seen.length, 1, 'a reverse lookup at 0,0 must still be sent') + assert.match(seen[0], /lat=0/) + } finally { + await new Promise((resolve) => { + server.close(resolve) + }) + } +}) From 70eedcd2c506748d9e04c2b3b1015b8bc37280af Mon Sep 17 00:00:00 2001 From: Rin <58572875+TurtIeSocks@users.noreply.github.com> Date: Mon, 24 Aug 2026 18:13:11 -0400 Subject: [PATCH 08/12] fix(geocoder): narrow the Photon signature and carry neighborhood end 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. --- server/src/services/geocoder.js | 25 +++++-- server/test/geocoder.test.js | 128 ++++++++++++++++++++++++++++++-- 2 files changed, 142 insertions(+), 11 deletions(-) diff --git a/server/src/services/geocoder.js b/server/src/services/geocoder.js index 8a56af62e..28764402e 100644 --- a/server/src/services/geocoder.js +++ b/server/src/services/geocoder.js @@ -68,9 +68,15 @@ function assertNominatimResponse(results, url) { // // node-geocoder ignores the status and parses the body regardless, so both // arrive here as an object with no address and format into a blank result. + // The 404 body is matched on Javalin's own documentation URL rather than on + // the generic title/status pair, because that pair is RFC 7807's shape: a + // rate-limiting proxy in front of a healthy Nominatim answers + // {"title":"Too Many Requests","status":429}, and diagnosing that as "your + // provider is wrong" would turn a transient failure into advice to break a + // working configuration permanently. const isPhoton = raw.type === 'FeatureCollection' || - (typeof raw.title === 'string' && typeof raw.status === 'number') || + (typeof raw.type === 'string' && raw.type.includes('javalin.io')) || (typeof raw.message === 'string' && raw.message.includes('Unknown query parameter')) @@ -104,7 +110,13 @@ async function nominatimGeocoder(url, search, isReverse) { // `neighborhood` and formatter() templates on `neighborhoods`, so the // value reached neither consumer. Carrying the alias is what makes it // visible without changing what node-geocoder itself produces. - neighborhood: formatted.neighbourhood || '', + // + // quarter is the fallback because Nominatim reports neighbourhood-level + // data under that key for many places: 1521 N Hoyne Ave carries + // quarter "Wicker Park" and no neighbourhood at all, and node-geocoder + // only reads address.neighbourhood, leaving the pair empty. + neighbourhood: formatted.neighbourhood || result.address?.quarter || '', + neighborhood: formatted.neighbourhood || result.address?.quarter || '', } })(stockGeocoder._geocoder._formatResult) // Awaited rather than returned so the shape check runs here. A throw inside @@ -159,10 +171,13 @@ async function geocoder(nominatimUrl, search, reverse, format, provider) { return reverse ? formatter(format, results[0]) : format - ? results.map((result) => ({ + ? // The mapped fields ride along rather than being rebuilt, because the + // Geocoder GraphQL type resolves neighborhood, suburb and the rest + // straight off this object. Returning only the formatted triple made + // every other field null whenever an addressFormat was configured. + results.map((result) => ({ + ...result, formatted: formatter(format, result), - latitude: result.latitude, - longitude: result.longitude, })) : results } catch (e) { diff --git a/server/test/geocoder.test.js b/server/test/geocoder.test.js index d57616d29..67a7f5c8a 100644 --- a/server/test/geocoder.test.js +++ b/server/test/geocoder.test.js @@ -537,9 +537,13 @@ test('a correctly configured Photon webhook still geocodes', async () => { '{{city}}', 'photon', ) - assert.deepEqual(result, [ - { formatted: 'Denver', latitude: 39.7392, longitude: -104.9903 }, - ]) + assert.equal(result.length, 1) + assert.equal(result[0].formatted, 'Denver') + assert.equal(result[0].latitude, 39.7392) + assert.equal(result[0].longitude, -104.9903) + // The mapped fields ride along with formatted: Geocoder resolves + // neighborhood, suburb and the rest straight off this object. + assert.equal(result[0].city, 'Denver') } finally { await server.close() } @@ -549,9 +553,11 @@ test('a correctly configured Nominatim webhook still geocodes', async () => { const server = await serveOnce(NOMINATIM_BODY) try { const result = await geocoder(server.url, 'Denver', false, '{{city}}') - assert.deepEqual(result, [ - { formatted: 'Denver', latitude: 39.7392, longitude: -104.9903 }, - ]) + assert.equal(result.length, 1) + assert.equal(result[0].formatted, 'Denver') + assert.equal(result[0].latitude, 39.7392) + assert.equal(result[0].longitude, -104.9903) + assert.equal(result[0].city, 'Denver') } finally { await server.close() } @@ -1031,3 +1037,113 @@ test('the empty guard does not swallow a zero coordinate', async () => { }) } }) + +// An RFC 7807 problem body is not a Photon signature. 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. +test('an RFC 7807 error from a Nominatim proxy is not diagnosed as Photon', async () => { + const server = await serveOnce({ title: 'Too Many Requests', status: 429 }) + try { + // The generic path resolves with a blank entry, as it always did. What must + // not happen is the provider-mismatch throw, whose message tells the + // operator to change geocoderProvider. + const results = await nominatimGeocoder(server.url, 'Denver', false) + assert.equal(results.length, 1) + } finally { + await server.close() + } +}) + +// The Javalin signatures still fire, so narrowing did not disable the check. +test('the Javalin 404 body is still diagnosed as Photon', async () => { + const server = await serveOnce({ + title: 'Endpoint GET /search not found', + status: 404, + type: 'https://javalin.io/documentation#endpointnotfound', + }) + try { + await assert.rejects( + () => nominatimGeocoder(server.url, 'Denver', false), + /Photon instance/, + ) + } finally { + await server.close() + } +}) + +// Nominatim reports neighbourhood-level data under `quarter` for many places: +// 1521 N Hoyne Ave carries quarter "Wicker Park" and no neighbourhood key. +// node-geocoder only reads address.neighbourhood, so both spellings came out +// empty for exactly the responses the alias exists to surface. +test('a quarter-shaped Nominatim response populates both neighborhood spellings', async () => { + const server = await serveOnce([ + { + lat: '41.9088', + lon: '-87.6796', + display_name: + '1521, North Hoyne Avenue, Wicker Park, West Town, Chicago, Illinois, 60622, United States', + address: { + house_number: '1521', + road: 'North Hoyne Avenue', + quarter: 'Wicker Park', + suburb: 'West Town', + city: 'Chicago', + state: 'Illinois', + postcode: '60622', + country: 'United States', + country_code: 'us', + }, + }, + ]) + try { + const results = await nominatimGeocoder(server.url, 'Denver', false) + assert.equal(results[0].neighbourhood, 'Wicker Park') + assert.equal(results[0].neighborhood, 'Wicker Park') + } finally { + await server.close() + } +}) + +// When an addressFormat is configured, the resolver's list still resolves the +// Geocoder fields off these objects. Returning only the formatted triple made +// neighborhood, suburb and the rest null for every formatted call. +test('a formatted result keeps the mapped fields for the Geocoder type', async () => { + const server = await serveOnce({ + type: 'FeatureCollection', + features: [ + { + geometry: { type: 'Point', coordinates: [-87.6796, 41.9088] }, + properties: { + housenumber: '1521', + street: 'North Hoyne Avenue', + locality: 'Wicker Park', + district: 'West Town', + city: 'Chicago', + state: 'Illinois', + postcode: '60622', + country: 'United States', + countrycode: 'US', + osm_key: 'building', + osm_value: 'yes', + type: 'house', + }, + }, + ], + }) + try { + const results = await geocoder( + server.url, + '1521 N Hoyne', + false, + '{{city}}', + 'photon', + ) + assert.equal(results[0].formatted, 'Chicago') + assert.equal(results[0].neighborhood, 'Wicker Park') + assert.equal(results[0].suburb, 'West Town') + assert.equal(results[0].streetNumber, '1521') + } finally { + await server.close() + } +}) From bbd43dae161869a566dcf4d3da76d6ccdbaaf14a Mon Sep 17 00:00:00 2001 From: Rin <58572875+TurtIeSocks@users.noreply.github.com> Date: Sun, 23 Aug 2026 13:17:16 -0400 Subject: [PATCH 09/12] feat(geocoder): derive geocoderProvider from Poracle's remote config 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. --- packages/types/lib/config.d.ts | 5 ++++ server/src/services/Poracle.js | 37 +++++++++++++++++++++-- server/test/geocoder.test.js | 54 +++++++++++++++++++++++++++++++++- 3 files changed, 93 insertions(+), 3 deletions(-) diff --git a/packages/types/lib/config.d.ts b/packages/types/lib/config.d.ts index 0a2b80246..ee285fb2e 100644 --- a/packages/types/lib/config.d.ts +++ b/packages/types/lib/config.d.ts @@ -199,6 +199,11 @@ export interface Webhook { * Which geocoding backend `nominatimUrl` points at. Photon speaks GeoJSON * rather than Nominatim's JSON, so it needs its own request and response * handling. Defaults to `nominatim`. + * + * Optional when Poracle reports it. Poracle sends the backend behind its + * `providerURL` as `provider`, and ReactMap uses that when this is unset, + * the same way `nominatimUrl` falls back to Poracle's `providerURL`. Setting + * it here always wins. */ geocoderProvider?: 'nominatim' | 'photon' trialPeriodEligible?: boolean diff --git a/server/src/services/Poracle.js b/server/src/services/Poracle.js index 06a8c0ce6..452ce726f 100644 --- a/server/src/services/Poracle.js +++ b/server/src/services/Poracle.js @@ -41,6 +41,31 @@ const SUBCATEGORIES = /** @type {const} */ ({ gym: ['raid', 'egg', 'gym'], }) +/** + * Geocoding backends ReactMap can drive. Poracle also offers `google` and + * `none`, which have no equivalent here, so they are ignored rather than + * mapped onto something they are not. + */ +const REMOTE_GEOCODERS = new Set(['nominatim', 'photon']) + +/** + * Resolves which geocoding backend a webhook should use. + * + * Poracle reports the backend behind its `providerURL` as `provider`, so a + * deployment that gets its geocoder URL from Poracle no longer has to restate + * the backend type in ReactMap's own config. Local config still wins, matching + * how `providerURL` and `addressFormat` already behave. + * @param {string} [remote] The `provider` field from Poracle's remote config + * @param {string} [local] geocoderProvider from this webhook's own config + * @returns {'nominatim' | 'photon' | undefined} + */ +function resolveGeocoderProvider(remote, local) { + if (local) return /** @type {'nominatim' | 'photon'} */ (local) + return REMOTE_GEOCODERS.has(remote) + ? /** @type {'nominatim' | 'photon'} */ (remote) + : undefined +} + class PoracleAPI { /** @param {import("@rm/types").Config['webhooks'][number]} webhook */ constructor(webhook) { @@ -231,7 +256,11 @@ class PoracleAPI { `Poracle must be at least version 4.8.4, current version is ${this.version}`, ) } - const { providerURL, addressFormat, ...rest } = remoteConfig + // `provider` is pulled out of remoteConfig even when it is not usable. + // this.provider is the *webhook* provider ('poracle'); letting Poracle's + // geocoding provider through the spread below would silently overwrite it + // with an unrelated meaning. + const { providerURL, provider, addressFormat, ...rest } = remoteConfig Object.assign(this, rest) if (addressFormat && !this.addressFormat) { this.addressFormat = addressFormat @@ -239,6 +268,10 @@ class PoracleAPI { if (providerURL && !this.nominatimUrl) { this.nominatimUrl = providerURL } + this.geocoderProvider = resolveGeocoderProvider( + provider, + this.geocoderProvider, + ) this.leagues = [ { name: 'great', cp: 1500, min: remoteConfig.pvpFilterGreatMinCP }, { name: 'ultra', cp: 2500, min: remoteConfig.pvpFilterUltraMinCP }, @@ -1257,4 +1290,4 @@ class PoracleAPI { } } -module.exports = { PoracleAPI } +module.exports = { PoracleAPI, resolveGeocoderProvider } diff --git a/server/test/geocoder.test.js b/server/test/geocoder.test.js index 67a7f5c8a..7db114685 100644 --- a/server/test/geocoder.test.js +++ b/server/test/geocoder.test.js @@ -5,7 +5,10 @@ const NodeGeocoder = require('node-geocoder') const http = require('node:http') -const { PoracleAPI } = require('../src/services/Poracle') +const { + PoracleAPI, + resolveGeocoderProvider, +} = require('../src/services/Poracle') const { formatter, geocoder, @@ -1147,3 +1150,52 @@ test('a formatted result keeps the mapped fields for the Geocoder type', async ( await server.close() } }) + +// Poracle reports the backend behind its providerURL as `provider`, so a +// deployment whose geocoder URL comes from Poracle no longer has to restate the +// backend type in ReactMap's own config. Local config still wins, matching how +// providerURL and addressFormat already behave. +test('derives the geocoder provider from Poracle when it is not set locally', () => { + assert.equal(resolveGeocoderProvider('photon', undefined), 'photon') + assert.equal(resolveGeocoderProvider('nominatim', undefined), 'nominatim') +}) + +test('local configuration beats what Poracle reports', () => { + assert.equal(resolveGeocoderProvider('nominatim', 'photon'), 'photon') + assert.equal(resolveGeocoderProvider('photon', 'nominatim'), 'nominatim') +}) + +// Poracle supports google and none. Neither has a ReactMap equivalent, and +// mapping them onto nominatim would claim something untrue about the backend. +test('ignores Poracle backends ReactMap cannot drive', () => { + const unusable = ['google', 'none', '', undefined, 'PHOTON'] + unusable.forEach((remote) => { + assert.equal( + resolveGeocoderProvider(remote, undefined), + undefined, + `${remote} should not resolve to a provider`, + ) + }) +}) + +// The reason `provider` is destructured out of remoteConfig rather than left to +// the spread: this.provider is the webhook provider, an unrelated field that +// happens to share the name. +test('a Poracle geocoding provider never overwrites the webhook provider', () => { + const api = new PoracleAPI({ + name: 'test', + host: 'http://127.0.0.1', + port: 3030, + provider: 'poracle', + }) + assert.equal(api.provider, 'poracle') + + // Mirrors what #fetchConfig does with a remote payload. + const remoteConfig = { provider: 'photon', providerURL: 'http://photon:2322' } + const { provider, ...rest } = remoteConfig + Object.assign(api, rest) + api.geocoderProvider = resolveGeocoderProvider(provider, api.geocoderProvider) + + assert.equal(api.provider, 'poracle') + assert.equal(api.geocoderProvider, 'photon') +}) From b6471267dd66b2852abe3d68eb9d7c94d98a8458 Mon Sep 17 00:00:00 2001 From: Rin <58572875+TurtIeSocks@users.noreply.github.com> Date: Sun, 23 Aug 2026 16:23:22 -0400 Subject: [PATCH 10/12] fix(geocoder): only inherit Poracle's provider alongside its URL 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. --- packages/types/lib/config.d.ts | 7 +++-- server/src/services/Poracle.js | 29 +++++++++++++----- server/test/geocoder.test.js | 55 ++++++++++++++++++++++++++++++---- 3 files changed, 74 insertions(+), 17 deletions(-) diff --git a/packages/types/lib/config.d.ts b/packages/types/lib/config.d.ts index ee285fb2e..15bbf422a 100644 --- a/packages/types/lib/config.d.ts +++ b/packages/types/lib/config.d.ts @@ -201,9 +201,10 @@ export interface Webhook { * handling. Defaults to `nominatim`. * * Optional when Poracle reports it. Poracle sends the backend behind its - * `providerURL` as `provider`, and ReactMap uses that when this is unset, - * the same way `nominatimUrl` falls back to Poracle's `providerURL`. Setting - * it here always wins. + * `providerURL` as `provider`, and ReactMap uses that when this is unset and + * `nominatimUrl` was itself inherited from Poracle. A locally configured URL + * keeps its local backend, since the two are one setting in two fields. + * Setting this explicitly always wins. */ geocoderProvider?: 'nominatim' | 'photon' trialPeriodEligible?: boolean diff --git a/server/src/services/Poracle.js b/server/src/services/Poracle.js index 452ce726f..400487743 100644 --- a/server/src/services/Poracle.js +++ b/server/src/services/Poracle.js @@ -55,12 +55,21 @@ const REMOTE_GEOCODERS = new Set(['nominatim', 'photon']) * deployment that gets its geocoder URL from Poracle no longer has to restate * the backend type in ReactMap's own config. Local config still wins, matching * how `providerURL` and `addressFormat` already behave. - * @param {string} [remote] The `provider` field from Poracle's remote config - * @param {string} [local] geocoderProvider from this webhook's own config + * + * The backend type is only inherited when the URL was inherited with it. A URL + * and the protocol used to talk to it are one setting in two fields, and + * splitting them across two sources is how a working Nominatim deployment ends + * up being addressed as Photon: keep a local `nominatimUrl`, omit + * `geocoderProvider`, and let a Photon-backed Poracle supply the type. + * @param {object} args + * @param {string} [args.remote] The `provider` field from Poracle's remote config + * @param {string} [args.local] geocoderProvider from this webhook's own config + * @param {boolean} args.usingRemoteURL Whether nominatimUrl also came from Poracle * @returns {'nominatim' | 'photon' | undefined} */ -function resolveGeocoderProvider(remote, local) { +function resolveGeocoderProvider({ remote, local, usingRemoteURL }) { if (local) return /** @type {'nominatim' | 'photon'} */ (local) + if (!usingRemoteURL) return undefined return REMOTE_GEOCODERS.has(remote) ? /** @type {'nominatim' | 'photon'} */ (remote) : undefined @@ -265,13 +274,17 @@ class PoracleAPI { if (addressFormat && !this.addressFormat) { this.addressFormat = addressFormat } - if (providerURL && !this.nominatimUrl) { + // Evaluated before the assignment below, because it is the assignment that + // decides whether the backend type may be inherited too. + const usingRemoteURL = !!providerURL && !this.nominatimUrl + if (usingRemoteURL) { this.nominatimUrl = providerURL } - this.geocoderProvider = resolveGeocoderProvider( - provider, - this.geocoderProvider, - ) + this.geocoderProvider = resolveGeocoderProvider({ + remote: provider, + local: this.geocoderProvider, + usingRemoteURL, + }) this.leagues = [ { name: 'great', cp: 1500, min: remoteConfig.pvpFilterGreatMinCP }, { name: 'ultra', cp: 2500, min: remoteConfig.pvpFilterUltraMinCP }, diff --git a/server/test/geocoder.test.js b/server/test/geocoder.test.js index 7db114685..659dd6580 100644 --- a/server/test/geocoder.test.js +++ b/server/test/geocoder.test.js @@ -1156,13 +1156,48 @@ test('a formatted result keeps the mapped fields for the Geocoder type', async ( // backend type in ReactMap's own config. Local config still wins, matching how // providerURL and addressFormat already behave. test('derives the geocoder provider from Poracle when it is not set locally', () => { - assert.equal(resolveGeocoderProvider('photon', undefined), 'photon') - assert.equal(resolveGeocoderProvider('nominatim', undefined), 'nominatim') + const args = { local: undefined, usingRemoteURL: true } + assert.equal(resolveGeocoderProvider({ ...args, remote: 'photon' }), 'photon') + assert.equal( + resolveGeocoderProvider({ ...args, remote: 'nominatim' }), + 'nominatim', + ) }) test('local configuration beats what Poracle reports', () => { - assert.equal(resolveGeocoderProvider('nominatim', 'photon'), 'photon') - assert.equal(resolveGeocoderProvider('photon', 'nominatim'), 'nominatim') + assert.equal( + resolveGeocoderProvider({ + remote: 'nominatim', + local: 'photon', + usingRemoteURL: true, + }), + 'photon', + ) + // Explicit local config wins even when the URL is local too, which is the + // only way to run a backend Poracle does not know about. + assert.equal( + resolveGeocoderProvider({ + remote: 'nominatim', + local: 'photon', + usingRemoteURL: false, + }), + 'photon', + ) +}) + +// The URL and the protocol used to talk to it are one setting in two fields. +// Inheriting the type while keeping a local URL is how a working Nominatim +// deployment ends up being addressed as Photon: local nominatimUrl, no +// geocoderProvider, and a Photon-backed Poracle supplying the type. +test('never inherits the provider without the URL that goes with it', () => { + assert.equal( + resolveGeocoderProvider({ + remote: 'photon', + local: undefined, + usingRemoteURL: false, + }), + undefined, + ) }) // Poracle supports google and none. Neither has a ReactMap equivalent, and @@ -1171,7 +1206,11 @@ test('ignores Poracle backends ReactMap cannot drive', () => { const unusable = ['google', 'none', '', undefined, 'PHOTON'] unusable.forEach((remote) => { assert.equal( - resolveGeocoderProvider(remote, undefined), + resolveGeocoderProvider({ + remote, + local: undefined, + usingRemoteURL: true, + }), undefined, `${remote} should not resolve to a provider`, ) @@ -1194,7 +1233,11 @@ test('a Poracle geocoding provider never overwrites the webhook provider', () => const remoteConfig = { provider: 'photon', providerURL: 'http://photon:2322' } const { provider, ...rest } = remoteConfig Object.assign(api, rest) - api.geocoderProvider = resolveGeocoderProvider(provider, api.geocoderProvider) + api.geocoderProvider = resolveGeocoderProvider({ + remote: provider, + local: api.geocoderProvider, + usingRemoteURL: true, + }) assert.equal(api.provider, 'poracle') assert.equal(api.geocoderProvider, 'photon') From 6b37fd3230d7828d673ca51732fa8d28b4510872 Mon Sep 17 00:00:00 2001 From: Rin <58572875+TurtIeSocks@users.noreply.github.com> Date: Mon, 24 Aug 2026 21:08:07 -0400 Subject: [PATCH 11/12] fix(geocoder): reject non-Nominatim problem bodies instead of passing 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. --- server/src/services/geocoder.js | 21 +++++++++++++++++++++ server/test/geocoder.test.js | 19 ++++++++++++++----- 2 files changed, 35 insertions(+), 5 deletions(-) diff --git a/server/src/services/geocoder.js b/server/src/services/geocoder.js index 28764402e..27f7627bb 100644 --- a/server/src/services/geocoder.js +++ b/server/src/services/geocoder.js @@ -85,6 +85,27 @@ function assertNominatimResponse(results, url) { `${url} answered as a Photon instance rather than a Nominatim one. Set "geocoderProvider": "photon" on this webhook, or point the URL at a Nominatim instance.`, ) } + + // Anything else object-shaped that carries no Nominatim result fields is an + // upstream error body, such as RFC 7807 problem JSON from a proxy in front + // of Nominatim. node-geocoder has already mapped it onto one blank entry by + // the time it gets here, and letting that through hands the caller a result + // with null coordinates -- Location.jsx pans the map to it. Rejecting keeps + // geocoder()'s fallback shape ([] or ''), and the message stays generic + // because this is a failing upstream, not a misconfigured provider. + // {"error": ...} bodies never reach this: node-geocoder converts those to a + // thrown Error itself. A genuine /reverse result always carries lat, + // display_name and address (all 42 captured fixtures do). + const isNominatimResult = + 'lat' in raw || + 'display_name' in raw || + 'address' in raw || + 'place_id' in raw + if (!isNominatimResult) { + throw new Error( + `${url} answered with an error body instead of a geocoding result: ${JSON.stringify(raw).slice(0, 200)}`, + ) + } } /** diff --git a/server/test/geocoder.test.js b/server/test/geocoder.test.js index 659dd6580..41923f863 100644 --- a/server/test/geocoder.test.js +++ b/server/test/geocoder.test.js @@ -1048,11 +1048,20 @@ test('the empty guard does not swallow a zero coordinate', async () => { test('an RFC 7807 error from a Nominatim proxy is not diagnosed as Photon', async () => { const server = await serveOnce({ title: 'Too Many Requests', status: 429 }) try { - // The generic path resolves with a blank entry, as it always did. What must - // not happen is the provider-mismatch throw, whose message tells the - // operator to change geocoderProvider. - const results = await nominatimGeocoder(server.url, 'Denver', false) - assert.equal(results.length, 1) + // A problem body rejects with a generic upstream error. It must not carry + // the provider-mismatch advice, which would tell the operator to change + // geocoderProvider to cure a transient 429, and it must not fall through as + // a blank entry either: node-geocoder maps the object onto one result with + // undefined coordinates, and Location.jsx pans the map to [null, null]. + await assert.rejects( + () => nominatimGeocoder(server.url, 'Denver', false), + (err) => /error body/.test(err.message) && !/Photon/.test(err.message), + ) + + // Through the public entry point the caller gets the schema's empty shape, + // never a result with null coordinates. + const viaGeocoder = await geocoder(server.url, 'Denver', false, '{{city}}') + assert.deepEqual(viaGeocoder, []) } finally { await server.close() } From fb398d783e06c3e80418d5610c55ed06e15c5a8b Mon Sep 17 00:00:00 2001 From: Rin <58572875+TurtIeSocks@users.noreply.github.com> Date: Mon, 24 Aug 2026 21:41:30 -0400 Subject: [PATCH 12/12] fix(geocoder): validate coordinates and keep transient failures out of 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. --- server/src/services/geocoder.js | 17 ++++--- server/src/services/photonGeocoder.js | 13 ++++- server/test/geocoder.test.js | 69 ++++++++++++++++++++++++++- 3 files changed, 90 insertions(+), 9 deletions(-) diff --git a/server/src/services/geocoder.js b/server/src/services/geocoder.js index 27f7627bb..55aef9224 100644 --- a/server/src/services/geocoder.js +++ b/server/src/services/geocoder.js @@ -94,13 +94,18 @@ function assertNominatimResponse(results, url) { // geocoder()'s fallback shape ([] or ''), and the message stays generic // because this is a failing upstream, not a misconfigured provider. // {"error": ...} bodies never reach this: node-geocoder converts those to a - // thrown Error itself. A genuine /reverse result always carries lat, - // display_name and address (all 42 captured fixtures do). + // thrown Error itself. + // + // The test is on coordinate values, not key presence. RFC 7807 permits + // extension members, so {"title":"Too Many Requests","status":429, + // "address":null} would pass a presence check and still format into an entry + // with undefined coordinates. A genuine /reverse result always carries lat + // and lon as parseable strings (all 42 captured fixtures do), and a result + // node-geocoder cannot derive coordinates from is unusable to every caller + // regardless of what else it carries. const isNominatimResult = - 'lat' in raw || - 'display_name' in raw || - 'address' in raw || - 'place_id' in raw + Number.isFinite(Number.parseFloat(raw.lat)) && + Number.isFinite(Number.parseFloat(raw.lon)) if (!isNominatimResult) { throw new Error( `${url} answered with an error body instead of a geocoding result: ${JSON.stringify(raw).slice(0, 200)}`, diff --git a/server/src/services/photonGeocoder.js b/server/src/services/photonGeocoder.js index fb08db4fb..1e7420ee5 100644 --- a/server/src/services/photonGeocoder.js +++ b/server/src/services/photonGeocoder.js @@ -329,8 +329,19 @@ async function photonGeocoder(photonUrl, search, isReverse) { // and is reported here. if (response instanceof Response) { + // Only a 404 on Photon's own route is evidence about the provider: Photon + // serves /api, so a host without it is not a Photon instance. Any other + // status -- a 429 or 500 from a healthy but struggling Photon, a 401/403 + // from an authentication proxy in front of one -- is an operational + // failure, and advising the operator to remove geocoderProvider there + // prompts them to break a working configuration to cure a transient error. + 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.`, + ) + } throw new Error( - `${photonUrl} answered ${response.status} for Photon's ${response.status === 404 ? '/api endpoint, so it is not a Photon instance' : 'request'}. Check the URL, or remove "geocoderProvider": "photon" from this webhook.`, + `${photonUrl} answered ${response.status}; the upstream geocoder is failing, not misconfigured.`, ) } if (isNominatimResponse(response)) { diff --git a/server/test/geocoder.test.js b/server/test/geocoder.test.js index 41923f863..9ff39d809 100644 --- a/server/test/geocoder.test.js +++ b/server/test/geocoder.test.js @@ -464,9 +464,9 @@ test('PoracleAPI leaves the provider undefined when it is not configured', () => // node-geocoder resolves through bluebird's asCallback the throw never reached // geocoder()'s catch: it surfaced as an uncaught exception and killed the // process. -const serveOnce = async (body) => { +const serveOnce = async (body, status = 200) => { const server = http.createServer((_, res) => { - res.writeHead(200, { 'Content-Type': 'application/json' }) + res.writeHead(status, { 'Content-Type': 'application/json' }) res.end(typeof body === 'string' ? body : JSON.stringify(body)) }) await new Promise((resolve) => { @@ -1251,3 +1251,68 @@ test('a Poracle geocoding provider never overwrites the webhook provider', () => assert.equal(api.provider, 'poracle') assert.equal(api.geocoderProvider, 'photon') }) + +// Only a 404 on /api is evidence about the provider. Any other status is an +// operational failure from a correctly configured Photon (429, 500) or a proxy +// in front of one (401, 403), and advice to remove geocoderProvider there +// prompts the operator to break a working configuration. +test('a transient Photon failure is not blamed on configuration', async () => { + const statuses = [429, 500, 401, 403] + // eslint-disable-next-line no-restricted-syntax + for (const status of statuses) { + // eslint-disable-next-line no-await-in-loop + const server = await serveOnce('', status) + try { + // eslint-disable-next-line no-await-in-loop + await assert.rejects( + () => photonGeocoder(server.url, 'Denver', false), + (err) => + !/geocoderProvider/.test(err.message) && + new RegExp(String(status)).test(err.message), + `a ${status} must read as an upstream failure, not provider advice`, + ) + } finally { + // eslint-disable-next-line no-await-in-loop + await server.close() + } + } +}) + +// RFC 7807 permits extension members, so a presence check on address or lat is +// defeatable: this body carries an address key and still formats into an entry +// with undefined coordinates. The classifier validates coordinate values. +test('an extended problem body does not pass as a Nominatim result', async () => { + const server = await serveOnce({ + title: 'Too Many Requests', + status: 429, + address: null, + }) + try { + await assert.rejects( + () => nominatimGeocoder(server.url, 'Denver', false), + (err) => /error body/.test(err.message) && !/Photon/.test(err.message), + ) + const viaGeocoder = await geocoder(server.url, 'Denver', false, '{{city}}') + assert.deepEqual(viaGeocoder, []) + } finally { + await server.close() + } +}) + +// The coordinate validation must not reject what it exists to protect: a +// genuine reverse result, whose lat and lon are parseable strings. +test('a genuine Nominatim reverse result passes the coordinate validation', async () => { + const server = await serveOnce(NOMINATIM_REVERSE_BODY) + try { + const results = await nominatimGeocoder( + server.url, + { lat: 39.7392, lon: -104.9903 }, + true, + ) + assert.equal(results.length, 1) + assert.equal(results[0].latitude, 39.7392) + assert.equal(results[0].city, 'Denver') + } finally { + await server.close() + } +})