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); + }); +});