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

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
 }
 
 /**

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.

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()
+  }
+})