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

packages/types/lib/config.d.ts

@@ -199,6 +199,12 @@ 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 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

server/src/services/Poracle.js

@@ -41,6 +41,40 @@ 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.
+ *
+ * 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, usingRemoteURL }) {
+  if (local) return /** @type {'nominatim' | 'photon'} */ (local)
+  if (!usingRemoteURL) return undefined
+  return REMOTE_GEOCODERS.has(remote)
+    ? /** @type {'nominatim' | 'photon'} */ (remote)
+    : undefined
+}
+
 class PoracleAPI {
   /** @param {import("@rm/types").Config['webhooks'][number]} webhook */
   constructor(webhook) {
@@ -231,14 +265,26 @@ 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
     }
-    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({
+      remote: provider,
+      local: this.geocoderProvider,
+      usingRemoteURL,
+    })
     this.leagues = [
       { name: 'great', cp: 1500, min: remoteConfig.pvpFilterGreatMinCP },
       { name: 'ultra', cp: 2500, min: remoteConfig.pvpFilterUltraMinCP },
@@ -1257,4 +1303,4 @@ class PoracleAPI {
   }
 }
 
-module.exports = { PoracleAPI }
+module.exports = { PoracleAPI, resolveGeocoderProvider }

server/src/services/geocoder.js

@@ -12,14 +12,107 @@ 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()
     .replace(/^,|,$/g, '')
     .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.
+ *
+ * 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) || 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.
+  // 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.type === 'string' && raw.type.includes('javalin.io')) ||
+    (typeof raw.message === 'string' &&
+      raw.message.includes('Unknown query parameter'))
+
+  if (isPhoton) {
+    throw new Error(
+      `${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.
+  //
+  // 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 =
+    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)}`,
+    )
+  }
+}
+
 /**
  * Nominatim, via node-geocoder's `openstreetmap` provider.
  * @param {string} url
@@ -32,15 +125,36 @@ 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)
-  return isReverse && typeof search === 'object'
+  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.
+      //
+      // 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
+  // _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
 }
 
 /**
@@ -60,24 +174,42 @@ 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)
         : await nominatimGeocoder(nominatimUrl, search, isReverse)
     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) {
     log.warn(TAGS.geocoder, 'Unable to geocode for', search, e)
-    return {}
+    return emptyResult(reverse)
   }
 }
 
-module.exports = { geocoder, formatter }
+module.exports = { geocoder, formatter, nominatimGeocoder }