diff --git a/server/src/nest/maps/maps.service.ts b/server/src/nest/maps/maps.service.ts index 084acffd3..a5eaa085a 100644 --- a/server/src/nest/maps/maps.service.ts +++ b/server/src/nest/maps/maps.service.ts @@ -18,6 +18,9 @@ import { DatabaseService } from '../database/database.service'; import { nominatimFetch, type GeoLane } from '../geo/nominatim.client'; import { trekPlacesSearch, + indexHitsOnly, + isOsmHit, + osmPlaceId, trekPlacesById, trekPlacesArea, trekPlacesNearby, @@ -1915,7 +1918,7 @@ export class MapsService { lat: locationBias?.lat, lng: locationBias?.lng, limit: 10, - }).catch((err: unknown) => { + }).then(indexHitsOnly).catch((err: unknown) => { console.warn('TREK Places search failed, falling back:', (err as Error).message); return [] as TrekPlace[]; }), @@ -2036,14 +2039,45 @@ export class MapsService { lng: (locationBias.low.lng + locationBias.high.lng) / 2, } : undefined; - const found = await trekPlacesSearch(input, { lat: centre?.lat, lng: centre?.lng, limit: 8 }); + // Both layers here, unlike the explicit search above. That path asks + // Nominatim in parallel and would get the same OpenStreetMap places + // twice; this one asks nobody else, because Nominatim's usage policy + // names autocomplete as unacceptable use. So the layer is not a second + // opinion here, it is the only place the missing names live. + // + // Measured on the case that surfaced it: "Tokio station" typed from + // Tokyo returned a weigh station in Ritzville and a station in Mexico + // from the index alone, and "Tokio Hauptbahnhof" in second place with + // the layer. The index holds businesses; stations, temples and bridges + // are in OpenStreetMap, and so is every exonym a traveller types. + const found = await trekPlacesSearch(input, { + lat: centre?.lat, + lng: centre?.lng, + limit: 8, + sources: 'index,osm', + }); if (found.length > 0) { return { - suggestions: found.map(p => ({ - placeId: `gers:${p.gers}`, - mainText: p.name, - secondaryText: [p.address?.locality, p.address?.country].filter(Boolean).join(', '), - })), + suggestions: found.map(p => + isOsmHit(p) + ? { + // The service's own id form, translated into the one this + // file already resolves. Leaving it as `osm:node/123` would + // hand the client an id getPlaceDetails does not know, and + // the failure would land after the user had picked it. + placeId: osmPlaceId(p), + mainText: p.name, + // The layer carries no address. The local name is what the + // place is called on the spot, which is more use under a + // translated label than an empty line. + secondaryText: p.local_name && p.local_name !== p.name ? p.local_name : '', + } + : { + placeId: `gers:${p.gers}`, + mainText: p.name, + secondaryText: [p.address?.locality, p.address?.country].filter(Boolean).join(', '), + }, + ), source: 'trek-places', }; } diff --git a/server/src/nest/maps/trek-places.client.ts b/server/src/nest/maps/trek-places.client.ts index 5fb95a47e..5881aea09 100644 --- a/server/src/nest/maps/trek-places.client.ts +++ b/server/src/nest/maps/trek-places.client.ts @@ -228,19 +228,74 @@ async function readCapped(res: Response, max: number): Promise { return Buffer.concat(chunks).toString('utf8'); } +/** + * A row from the service's OpenStreetMap layer, which answers alongside the + * index when `sources` asks for it. + * + * A different shape from TrekPlace, and that is the point rather than an + * oversight: the layer carries every name form a place is known under, which is + * what the index does not have. `name` is the one that matched the query and + * `local_name` the one written on the building, so a German searching "Tokio + * Hauptbahnhof" gets that as the label and 東京駅丸の内駅舎 underneath. + * + * It has no address block. OpenStreetMap does not carry one per node, and + * inventing an empty one here would let a caller read `address.locality` and + * quietly get nothing. + */ +export interface TrekOsmPlace { + /** Always `osm:/`, e.g. `osm:node/9712313`. */ + id: string; + osm_type: 'node' | 'way' | 'relation'; + osm_id: number; + name: string; + local_name: string | null; + lat: number; + lng: number; + category: string | null; + source: 'openstreetmap'; + distance_km?: number; +} + +export type TrekSearchHit = TrekPlace | TrekOsmPlace; + +export const isOsmHit = (hit: TrekSearchHit): hit is TrekOsmPlace => + hit.source === 'openstreetmap'; + +/** + * Turn the layer's id into the form the rest of this service already speaks. + * + * `osm:node/9712313` becomes `node:9712313`, which is what OSM_PLACE_ID matches + * and what getPlaceDetails resolves through the existing OpenStreetMap path. + * Without this the suggestion would carry an id nothing downstream recognises, + * and picking it would fail after the user had already chosen it. + */ +export function osmPlaceId(hit: TrekOsmPlace): string { + return `${hit.osm_type}:${hit.osm_id}`; +} + export async function trekPlacesSearch( query: string, - opts: { lat?: number; lng?: number; limit?: number } = {}, -): Promise { - const body = await getJson('/v1/search', { + opts: { lat?: number; lng?: number; limit?: number; sources?: string } = {}, +): Promise { + const body = await getJson<{ results?: TrekSearchHit[] }>('/v1/search', { q: query, lat: opts.lat, lng: opts.lng, limit: opts.limit ?? 10, + // Left off unless asked for, so the service applies its own default. That + // default is the index alone, which is what every existing caller here + // wants: the explicit search path already asks Nominatim in parallel, and + // adding the layer there would return the same OpenStreetMap places twice. + sources: opts.sources, }); return Array.isArray(body.results) ? body.results : []; } +/** The index-only rows of a mixed result, for callers that need Overture fields. */ +export function indexHitsOnly(hits: TrekSearchHit[]): TrekPlace[] { + return hits.filter((h): h is TrekPlace => !isOsmHit(h)); +} + /** * The planner's category pills, mapped onto Overture categories. * diff --git a/server/tests/unit/nest/maps.autocomplete.test.ts b/server/tests/unit/nest/maps.autocomplete.test.ts index 719d2282c..5f4fcd71c 100644 --- a/server/tests/unit/nest/maps.autocomplete.test.ts +++ b/server/tests/unit/nest/maps.autocomplete.test.ts @@ -54,6 +54,24 @@ const hit = (over: Record = {}) => ({ ...over, }); +/** + * A row from the service's OpenStreetMap layer. A different shape from the + * index rows above, which is the whole point of the two tests below: no `gers`, + * no address block, and an id in the service's own `osm:/` form. + */ +const osmHit = (over: Record = {}) => ({ + id: 'osm:node/9712313', + osm_type: 'node', + osm_id: 9712313, + name: 'Tokio Hauptbahnhof', + local_name: '東京駅丸の内駅舎', + lat: 35.6811816, + lng: 139.76598265, + category: 'sehenswuerdigkeit', + source: 'openstreetmap', + ...over, +}); + /** * `enabled` drives the admin kill switch, which is read straight off * app_settings; an unset row reads as on, because the switch is fail-open. @@ -111,7 +129,8 @@ describe('MapsService.autocompletePlaces', () => { }); // Eight, not ten: the suggestion list is what a person reads while typing. - expect(mockSearch).toHaveBeenCalledWith(INPUT, { lat: 54.1, lng: 12.2, limit: 8 }); + expect(mockSearch).toHaveBeenCalledWith(INPUT, + { lat: 54.1, lng: 12.2, limit: 8, sources: 'index,osm' }); }); it('MAPS-AUTO-004: without a bias the index is asked without coordinates rather than with zeroes', async () => { @@ -122,7 +141,8 @@ describe('MapsService.autocompletePlaces', () => { // A 0/0 bias is a point in the Atlantic, and the index treats a bias as // permission to relax the match — which is how the shops around a landmark // start outranking the landmark. - expect(mockSearch).toHaveBeenCalledWith(INPUT, { lat: undefined, lng: undefined, limit: 8 }); + expect(mockSearch).toHaveBeenCalledWith(INPUT, + { lat: undefined, lng: undefined, limit: 8, sources: 'index,osm' }); }); it('MAPS-AUTO-005: an index that found nothing falls through instead of answering empty', async () => { @@ -144,6 +164,38 @@ describe('MapsService.autocompletePlaces', () => { expect(result.source).not.toBe('trek-places'); }); + it('MAPS-AUTO-008: a hit from the OpenStreetMap layer carries an id the details lookup can read', async () => { + mockSearch.mockResolvedValue([osmHit()]); + + const { suggestions } = await make().autocompletePlaces(1, INPUT); + + // `osm:node/9712313` is the service's form; this file resolves `node:123`. + // Passing the service's form through would hand the client an id nothing + // downstream recognises, and it would fail after the user had picked it. + expect(suggestions[0].placeId).toBe('node:9712313'); + expect(suggestions[0].mainText).toBe('Tokio Hauptbahnhof'); + // The layer has no address. The name written on the building is more use + // under a translated label than an empty second line. + expect(suggestions[0].secondaryText).toBe('東京駅丸の内駅舎'); + }); + + it('MAPS-AUTO-009: index and layer hits keep their own id form in one list', async () => { + mockSearch.mockResolvedValue([hit(), osmHit(), hit({ gers: 'def-456' })]); + + const { suggestions } = await make().autocompletePlaces(1, INPUT); + + expect(suggestions.map(s => s.placeId)).toEqual([ + 'gers:abc-123', + 'node:9712313', + 'gers:def-456', + ]); + // A local name equal to the label would be a repeated line, not a hint. + const same = osmHit({ local_name: 'Tokio Hauptbahnhof' }); + mockSearch.mockResolvedValue([same]); + const second = await make().autocompletePlaces(1, INPUT); + expect(second.suggestions[0].secondaryText).toBe(''); + }); + it('MAPS-AUTO-007: the keystroke never leaves for the index while the admin has it off', async () => { mockSearch.mockResolvedValue([hit()]);