From fab3d2bdf8b3a456b9aa20d026f379764f8095f6 Mon Sep 17 00:00:00 2001 From: Maurice Date: Wed, 9 Sep 2026 15:59:06 +0200 Subject: [PATCH] fix(maps): ask the OpenStreetMap layer on the autocomplete path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Typing "Tokio station" from Tokyo suggested a weigh station in Ritzville and a station in Tuxtla Gutiérrez, and typing "Tokyo Station" put Japan in seventh place behind Markham, Arlington, Calgary, Milano and Melbourne. The station itself was never in the list. The service has had the answer all along. Its OpenStreetMap layer carries 14.6M travel places with every name form each is known under, including the exonyms a traveller actually types, and it is not the same data as the index: Overture holds businesses, so a search for a station finds the shops inside it. The layer returns "Tokio Hauptbahnhof" for the German spelling and 東京駅 for the Japanese. It was never asked. `sources` defaults to the index alone and trekPlacesSearch never sent the parameter, so no caller here has ever seen a layer row. Sent on the autocomplete path only. The explicit search asks Nominatim in parallel and would get the same OpenStreetMap places twice; this path 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 those names live. A layer row has a different shape, and that shape is why this is more than one parameter. It has no `gers` and no address block, and its id reads `osm:node/9712313` where this file resolves `node:9712313`. Passing it through untranslated would hand the client an id getPlaceDetails does not know, and the failure would land after the user had already picked the suggestion. The secondary line falls back to the local name, which is what is written on the building and more use under a translated label than an empty line. Measured against the service before the change, same query, same coordinates: "Tokio station" index only -> Tokio Weigh Station, Ritzville US with layer -> Tokio Hauptbahnhof in second place "東京駅" index only -> six restaurants inside the station with layer -> 東京駅 itself in the list --- server/src/nest/maps/maps.service.ts | 48 ++++++++++++--- server/src/nest/maps/trek-places.client.ts | 61 ++++++++++++++++++- .../tests/unit/nest/maps.autocomplete.test.ts | 56 ++++++++++++++++- 3 files changed, 153 insertions(+), 12 deletions(-) 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()]);