diff --git a/packages/types/lib/config.d.ts b/packages/types/lib/config.d.ts index 0a2b80246..15bbf422a 100644 --- a/packages/types/lib/config.d.ts +++ b/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 diff --git a/server/src/services/Poracle.js b/server/src/services/Poracle.js index 06a8c0ce6..400487743 100644 --- a/server/src/services/Poracle.js +++ b/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 } diff --git a/server/test/geocoder.test.js b/server/test/geocoder.test.js index 67a7f5c8a..659dd6580 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,95 @@ 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', () => { + 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({ + 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 +// 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, + local: undefined, + usingRemoteURL: true, + }), + 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({ + remote: provider, + local: api.geocoderProvider, + usingRemoteURL: true, + }) + + assert.equal(api.provider, 'poracle') + assert.equal(api.geocoderProvider, 'photon') +})