From 3caafe0548644574a878b07fd2017c00e9ad1d29 Mon Sep 17 00:00:00 2001 From: Maurice Date: Sun, 6 Sep 2026 01:27:17 +0200 Subject: [PATCH] fix(maps): show OpenStreetMap alongside the index, not instead of it Measured against 128 places out of a real trip to Japan, all saved through the old search: the index answered 51.6 percent inside its top five. For most of the rest it returned the shops AROUND the landmark rather than nothing, which is worse than nothing, because ten plausible wrong answers look like an answer. Overture Places is a dataset of businesses and is very good at those. It largely does not carry temples, bridges, riverside walks, viewpoints or observation decks, which is a fair share of what somebody planning a trip searches for. Two ways to detect "the index does not have this one" were measured and both failed. Its own score does not separate the cases: a 0.75 threshold catches 19 of 62 misses and throws away 6 of 66 hits. Name overlap does not either: 57 false positives, because "Hase Station" shares a word with "Hase-dera". So this does not classify. Explicit search asks both concurrently and merges, name matches first and the index ahead of OpenStreetMap within each group. Autocomplete is untouched, because Nominatim's policy names it as unacceptable use. Nominatim also gets the location bias it never had, without which "Hase-dera" returns the temple in Nara rather than the one in Kamakura the user is standing beside. Two booking-import cases still stubbed searchNominatim after geocoding moved to geocodeQuery, and stubbed it with the old array shape, which made "has coordinates" pass on an undefined lat. --- server/src/nest/maps/maps.helpers.ts | 61 ++++++++++++- server/src/nest/maps/maps.service.ts | 47 ++++++++-- .../booking-import.service.test.ts | 27 +++--- .../tests/unit/nest/maps.merge-search.test.ts | 86 +++++++++++++++++++ 4 files changed, 202 insertions(+), 19 deletions(-) create mode 100644 server/tests/unit/nest/maps.merge-search.test.ts diff --git a/server/src/nest/maps/maps.helpers.ts b/server/src/nest/maps/maps.helpers.ts index a32bdaa5f..8f2b90746 100644 --- a/server/src/nest/maps/maps.helpers.ts +++ b/server/src/nest/maps/maps.helpers.ts @@ -1,5 +1,6 @@ import { readEnv, getAppUrl } from '../../app-config'; import { stripHtmlTags } from '../common/stripHtmlTags'; +import { haversineMetres } from '../common/geo'; /** * Pure maps/geo helpers — no DB, no Nest, no side effects beyond reading env. @@ -75,7 +76,7 @@ export function toWikiLang(lang: string | undefined, fallback = 'en'): string { // Re-exported, not redefined: this was one of three copies. Kept as an export // here because callers and a test import it from maps.helpers. -export { haversineMetres } from '../common/geo'; +export { haversineMetres }; /** * Whether two place names plausibly refer to the same thing. @@ -91,6 +92,64 @@ export { haversineMetres } from '../common/geo'; * list for all of them is its own maintenance problem. Anything the pair rule * lets through has already survived the distance check. */ +/** + * Put the index's answer and OpenStreetMap's into one list. + * + * Overture Places is a dataset of businesses. It is very good at those and + * largely does not carry temples, bridges, riverside walks, viewpoints or + * observation decks, which is a fair share of what somebody planning a trip + * searches for. Measured against 128 places out of a real trip to Japan, saved + * through the old search: the index answered 51.6 percent inside its top five, + * and for most of the rest it returned the shops AROUND the landmark rather + * than nothing, which is worse than nothing because it looks like an answer. + * + * Two candidate signals for "the index does not have this one" were measured + * and both failed. The index's own score does not separate the cases (a + * threshold of 0.75 catches 19 of 62 misses and throws away 6 of 66 hits), and + * name overlap against the result set does not either (57 false positives, + * because "Hase Station" shares a word with "Hase-dera"). So this does not + * classify. It shows both, and lets the name decide the order: + * + * 1. results whose name plausibly matches what was typed, index first + * 2. everything else, index first + * + * A place both sources know is kept once, as the index's copy, because that is + * the one carrying a stable id, contact details and hours. + */ +export function mergeSearchResults( + fromIndex: Record[], + fromOsm: Record[], + query: string, + limit = 10, +): Record[] { + const sameThing = (a: Record, b: Record): boolean => { + const [aLat, aLng, bLat, bLng] = [a.lat, a.lng, b.lat, b.lng]; + if (![aLat, aLng, bLat, bLng].every((v) => typeof v === 'number')) return false; + // 60 m is close enough to be the same shop. It is NOT enough on its own: + // a temple and the coffee shop at its gate are fifty metres apart, and + // deduping on distance alone swallowed the temple, which is the exact + // result this function exists to surface. Two things in one spot with + // unrelated names are two things. + if (haversineMetres(aLat as number, aLng as number, bLat as number, bLng as number) >= 60) { + return false; + } + return ( + typeof a.name === 'string' && typeof b.name === 'string' && namesOverlap(a.name, b.name) + ); + }; + + const extra = fromOsm.filter((o) => !fromIndex.some((i) => sameThing(i, o))); + const matches = (p: Record): boolean => + typeof p.name === 'string' && namesOverlap(query, p.name); + + return [ + ...fromIndex.filter(matches), + ...extra.filter(matches), + ...fromIndex.filter((p) => !matches(p)), + ...extra.filter((p) => !matches(p)), + ].slice(0, limit); +} + export function namesOverlap(a: string, b: string): boolean { const words = (value: string): Set => new Set( diff --git a/server/src/nest/maps/maps.service.ts b/server/src/nest/maps/maps.service.ts index 1d85efb33..651c6c1d6 100644 --- a/server/src/nest/maps/maps.service.ts +++ b/server/src/nest/maps/maps.service.ts @@ -41,6 +41,7 @@ import { toWikiLang, haversineMetres, namesOverlap, + mergeSearchResults, type GoogleOpeningHours, type OverpassPoi, } from './maps.helpers'; @@ -764,7 +765,12 @@ export class MapsService { * whole import. Yielding does not make the import faster, it stops it from * being the only thing the process will do for half a minute. */ - async searchNominatim(query: string, lang?: string, lane: GeoLane = 'interactive') { + async searchNominatim( + query: string, + lang?: string, + lane: GeoLane = 'interactive', + bias?: { lat: number; lng: number }, + ) { const params = new URLSearchParams({ q: query, format: 'json', @@ -777,6 +783,15 @@ export class MapsService { limit: '10', 'accept-language': toApiLang(lang), }); + // Prefer the area the caller is looking at, never restrict to it + // (bounded=0). Without this, "Hase-dera" returns the temple of that name in + // Nara rather than the one in Kamakura the user is standing next to; with + // bounded=1 a search for somewhere genuinely far away would return nothing. + if (bias) { + const d = 0.5; + params.set('viewbox', [bias.lng - d, bias.lat - d, bias.lng + d, bias.lat + d].join(',')); + params.set('bounded', '0'); + } // Through the shared client: one throttle for the whole process. const response = await nominatimFetch('search', params, { lane }); if (!response.ok) { @@ -1624,13 +1639,29 @@ export class MapsService { // this method did before. if (this.trekPlacesEnabled()) { try { - const found = await trekPlacesSearch(query, { - lat: locationBias?.lat, - lng: locationBias?.lng, - limit: 10, - }); - if (found.length > 0) { - return { places: found.map(toPlaceRecord), source: 'trek-places' }; + // Both at once. The index is a dataset of businesses and is very good + // at those; OpenStreetMap is where the temples, bridges, riverside + // walks and viewpoints are, and a travel search asks for those + // constantly. Concurrently, so the pair costs the slower one rather + // than the sum. This is the explicit search, not the keystroke path + // Nominatim's policy rules out. + const [found, osm] = await Promise.all([ + trekPlacesSearch(query, { + lat: locationBias?.lat, + lng: locationBias?.lng, + limit: 10, + }), + this.searchNominatim(query, lang, 'interactive', locationBias).catch((err: unknown) => { + console.warn('OpenStreetMap search failed, index only:', (err as Error).message); + return [] as Record[]; + }), + ]); + const places = mergeSearchResults(found.map(toPlaceRecord), osm, query); + if (places.length > 0) { + return { + places, + source: osm.length > 0 ? 'trek-places+openstreetmap' : 'trek-places', + }; } } catch (err: unknown) { // Logged, not surfaced: the fallback below is a working answer. diff --git a/server/tests/unit/nest/booking-import/booking-import.service.test.ts b/server/tests/unit/nest/booking-import/booking-import.service.test.ts index 908295ae4..9e9e471d8 100644 --- a/server/tests/unit/nest/booking-import/booking-import.service.test.ts +++ b/server/tests/unit/nest/booking-import/booking-import.service.test.ts @@ -25,7 +25,10 @@ function make(opts: { kit?: boolean; ai?: boolean; extract?: any; parse?: any }) const reservations = { create: vi.fn() }; // budget/addons/realtime/maps ride the confirm() path only — the preview() // tests never reach them, so stubs beyond the positional slots aren't needed. - const maps = { searchNominatim: vi.fn() }; + // geocodeQuery, not searchNominatim: the import asks the TREK index first + // and falls through to OpenStreetMap inside that method, so the service no + // longer knows which of the two answered. + const maps = { geocodeQuery: vi.fn() }; // Places became a constructor dep with the place DI fold (was a path mock of // services/placeService); only confirm() reaches it, so a bare create stub does. const places = { create: vi.fn() }; @@ -116,11 +119,14 @@ describe('BookingImportService.preview endpoint geocoding (#1969)', () => { }, }; - const hit = (lat: number, lng: number) => [{ lat, lng, display_name: 'x' }]; + // geocodeQuery answers with one coordinate or null, not a provider result + // list. Stubbing the old array shape made 'has coordinates' pass on an + // undefined lat, which is exactly the bug the test is meant to catch. + const hit = (lat: number, lng: number) => ({ lat, lng }); it('gives a named station coordinates, so it survives the save', async () => { const { svc, maps } = make({ kit: true, extract: async () => [TRAIN_KI] }); - maps.searchNominatim.mockResolvedValue(hit(52.525, 13.369)); + maps.geocodeQuery.mockResolvedValue(hit(52.525, 13.369)); const res = await svc.preview([file()], 'no-ai', 1); const endpoints = (res.items[0] as { endpoints?: { lat: number | null }[] }).endpoints ?? []; @@ -134,12 +140,13 @@ describe('BookingImportService.preview endpoint geocoding (#1969)', () => { // Two legs out of and back into the same station. extract: async () => [TRAIN_KI, TRAIN_KI], }); - maps.searchNominatim.mockResolvedValue(hit(52.525, 13.369)); + maps.geocodeQuery.mockResolvedValue(hit(52.525, 13.369)); await svc.preview([file()], 'no-ai', 1); - // Two distinct names across four endpoints — Nominatim allows about one - // request a second on this lane, so repeats have to come from the cache. - expect(maps.searchNominatim).toHaveBeenCalledTimes(2); + // Two distinct names across four endpoints. The index is fast, but its + // fallback lane is Nominatim at about one request a second, so repeats + // still have to come from the cache. + expect(maps.geocodeQuery).toHaveBeenCalledTimes(2); }); /* @@ -149,7 +156,7 @@ describe('BookingImportService.preview endpoint geocoding (#1969)', () => { */ it('keeps an endpoint it cannot place, and says so', async () => { const { svc, maps } = make({ kit: true, extract: async () => [TRAIN_KI] }); - maps.searchNominatim.mockResolvedValue([]); + maps.geocodeQuery.mockResolvedValue(null); const res = await svc.preview([file()], 'no-ai', 1); const endpoints = (res.items[0] as { endpoints?: { name: string }[] }).endpoints ?? []; @@ -159,7 +166,7 @@ describe('BookingImportService.preview endpoint geocoding (#1969)', () => { it('survives a geocoder that throws, rather than failing the import', async () => { const { svc, maps } = make({ kit: true, extract: async () => [TRAIN_KI] }); - maps.searchNominatim.mockRejectedValue(new Error('nominatim down')); + maps.geocodeQuery.mockRejectedValue(new Error('nominatim down')); const res = await svc.preview([file()], 'no-ai', 1); expect(res.items).toHaveLength(1); @@ -177,6 +184,6 @@ describe('BookingImportService.preview endpoint geocoding (#1969)', () => { const { svc, maps } = make({ kit: true, extract: async () => [withGeo] }); await svc.preview([file()], 'no-ai', 1); - expect(maps.searchNominatim).not.toHaveBeenCalled(); + expect(maps.geocodeQuery).not.toHaveBeenCalled(); }); }); diff --git a/server/tests/unit/nest/maps.merge-search.test.ts b/server/tests/unit/nest/maps.merge-search.test.ts new file mode 100644 index 000000000..130427196 --- /dev/null +++ b/server/tests/unit/nest/maps.merge-search.test.ts @@ -0,0 +1,86 @@ +/** + * MAPS-MERGE-001..008 — showing the index's answer and OpenStreetMap's together. + * + * Why this exists, in numbers. 128 places out of a real trip to Japan, all + * saved through the old search, replayed against the index: 51.6 percent came + * back inside the top five. For most of the rest the index returned the shops + * AROUND the landmark instead of nothing, which is worse than nothing, because + * ten plausible wrong answers look like an answer and the user never learns the + * temple was one query away in OpenStreetMap. + */ +import { describe, it, expect } from 'vitest'; +import { mergeSearchResults } from '../../../src/nest/maps/maps.helpers'; + +const p = (name: string, lat: number, lng: number, source: string) => ({ + name, + lat, + lng, + source, + osm_id: `${source}:${name}`, +}); + +describe('mergeSearchResults', () => { + it('MAPS-MERGE-001: lifts the landmark above the shops that surround it', () => { + // The Hase-dera case. The index has the coffee shop next door and not the + // temple; OpenStreetMap has the temple. + const index = [p('Uni Coffee Roastery', 35.3128, 139.534, 'trek-places')]; + const osm = [p('Hase-dera', 35.3125, 139.5335, 'openstreetmap')]; + const out = mergeSearchResults(index, osm, 'Hase-dera'); + expect(out.map((r) => r.name)).toEqual(['Hase-dera', 'Uni Coffee Roastery']); + }); + + it('MAPS-MERGE-002: leaves a good index match at the top', () => { + const index = [p("L'Osteria Rostock", 54.0879, 12.1408, 'trek-places')]; + const osm = [p('Osteria Steinstrasse', 54.09, 12.15, 'openstreetmap')]; + const out = mergeSearchResults(index, osm, "L'Osteria"); + expect(out[0].name).toBe("L'Osteria Rostock"); + }); + + it('MAPS-MERGE-003: keeps one copy of a place both sources know', () => { + // The index's copy, because that is the one carrying a stable id, contact + // details and hours. + const index = [p("L'Osteria", 54.0879, 12.1408, 'trek-places')]; + const osm = [p("L'Osteria", 54.08792, 12.14082, 'openstreetmap')]; + const out = mergeSearchResults(index, osm, "L'Osteria"); + expect(out).toHaveLength(1); + expect(out[0].source).toBe('trek-places'); + }); + + it('MAPS-MERGE-004: two neighbouring shops both survive', () => { + // 60 m is the dedupe distance: close enough to be one shop, far enough + // apart that the bakery beside it is still its own result. + const index = [p('Bakery A', 54.0879, 12.1408, 'trek-places')]; + const osm = [p('Bakery B', 54.0889, 12.1408, 'openstreetmap')]; + expect(mergeSearchResults(index, osm, 'Bakery')).toHaveLength(2); + }); + + it('MAPS-MERGE-005: a source that answered nothing changes nothing', () => { + const index = [p("L'Osteria", 54.0879, 12.1408, 'trek-places')]; + expect(mergeSearchResults(index, [], "L'Osteria")).toEqual(index); + expect(mergeSearchResults([], index, "L'Osteria")).toEqual(index); + }); + + it('MAPS-MERGE-006: name matches come first, index before OpenStreetMap', () => { + const index = [p('Something Else', 1, 1, 'trek-places'), p('Kegon Falls', 1.5, 1.5, 'trek-places')]; + const osm = [p('Kegon Waterfall', 2, 2, 'openstreetmap'), p('Unrelated', 3, 3, 'openstreetmap')]; + expect(mergeSearchResults(index, osm, 'Kegon Falls').map((r) => r.name)).toEqual([ + 'Kegon Falls', + 'Kegon Waterfall', + 'Something Else', + 'Unrelated', + ]); + }); + + it('MAPS-MERGE-007: honours the cap', () => { + const many = Array.from({ length: 12 }, (_, i) => p(`Place ${i}`, 50 + i, 10 + i, 'trek-places')); + expect(mergeSearchResults(many, many, 'Place', 10)).toHaveLength(10); + }); + + it('MAPS-MERGE-008: a result without coordinates is kept, not deduped away', () => { + // Nominatim occasionally answers with an unparseable coordinate. Dropping + // the row would lose a result; treating it as "not near anything" keeps it. + const index = [p('A', 54, 12, 'trek-places')]; + const osm = [{ name: 'B', lat: null, lng: null, source: 'openstreetmap' }]; + expect(mergeSearchResults(index, osm, 'A')).toHaveLength(2); + }); +});