From 41e256bf0f84a7df22901c9bf44e0124ef795ea2 Mon Sep 17 00:00:00 2001 From: Maurice Date: Mon, 7 Sep 2026 14:20:37 +0200 Subject: [PATCH] feat(roadtrip): answer the corridor search from the place index Search along the route went to Overpass and only to Overpass, on the one path where that hurts most: one search fans out over sixteen boxes, and each of those races four public mirrors. The index answers the map's explore pills in milliseconds where those mirrors take seconds, and the corridor was the reason the index exists. Two independent things kept it out. The corridor sends its categories as a comma-separated list, and the index branch looked the whole string up as one key, so it missed every time. And the four kinds a drive needs, fuel, charging, rest areas and campsites, had no Overture terms mapped at all. They do now: gas_station and fueling_station, ev_charging_station, rest_stop, and campground with rv_park. The branch is list-aware the way searchOverpassPois already was, and it labels each hit with the category that produced it rather than with the list that was asked for, because the client colours and groups its markers by that field and "fuel,charging" is not a category. It answers all-or-nothing: a category the index has no terms for sends the whole query to Overpass, since answering the half it knows would silently drop the rest while the panel still reported a finished search. The caller's limit is passed through instead of a hardcoded 50, so a mixed search gets the same per-category budget the Overpass path spends. The other outbound path of this addon changes with it. Importing a shared Google route geocoded its waypoints by calling searchNominatim directly, which walked past geocodeQuery and its index lookup. Through geocodeQuery an index hit costs no throttle slot at all, and what the index does not know still falls through on the background lane. A place answered from the index also carries its chain and that chain's Wikidata item, so a branch gets its logo on the pin here exactly as it does on the Overpass path. Sockets stay an OSM field: a charging station answered from the index reports "not stated" rather than claiming it offers nothing. --- server/src/nest/maps/maps.service.ts | 44 ++++++++--- server/src/nest/maps/trek-places.client.ts | 8 ++ server/src/nest/places/places.service.ts | 17 +++-- server/tests/unit/nest/maps.pois.test.ts | 75 ++++++++++++++++--- .../tests/unit/nest/places.directions.test.ts | 45 +++++++---- 5 files changed, 149 insertions(+), 40 deletions(-) diff --git a/server/src/nest/maps/maps.service.ts b/server/src/nest/maps/maps.service.ts index ec9d00152..d2b1e3055 100644 --- a/server/src/nest/maps/maps.service.ts +++ b/server/src/nest/maps/maps.service.ts @@ -820,24 +820,45 @@ export class MapsService { category: string, bbox: { south: number; west: number; north: number; east: number }, lang?: string, + limit = 60, ): Promise { - const terms = POI_CATEGORY_TO_TREK[category]; - if (this.trekPlacesEnabled() && terms) { + // One or several categories, the way searchOverpassPois reads them. The + // corridor search sends a comma-separated list, and looking the whole string + // up as one key missed every time — so every corridor query fell through to + // Overpass, on the one path where that hurts most: a single search fans out + // over sixteen boxes, and each of those races four public mirrors. + const wanted = parsePoiCategories(category); + // Which category each Overture term belongs to, so a hit can be labelled + // with the category that actually produced it rather than with the whole + // list. The client colours and groups its markers by that field. + const categoryOfTerm = new Map(); + for (const key of wanted) { + for (const term of POI_CATEGORY_TO_TREK[key] ?? []) categoryOfTerm.set(term, key); + } + // All or nothing: a category the index has no terms for has to be answered + // by Overpass, and a mixed answer would silently drop it. + const indexKnowsAll = wanted.length > 0 && wanted.every(key => POI_CATEGORY_TO_TREK[key]?.length); + const terms = [...categoryOfTerm.keys()]; + if (this.trekPlacesEnabled() && indexKnowsAll) { try { const lat = (bbox.south + bbox.north) / 2; const lng = (bbox.west + bbox.east) / 2; // Half the diagonal, so the circle covers the viewport corners rather // than leaving the edges of the map empty. - const wanted = Math.round( + const reach = Math.round( Math.hypot( (bbox.north - bbox.south) * 111_320, (bbox.east - bbox.west) * 111_320 * Math.cos((lat * Math.PI) / 180), ) / 2, ); - const radius = Math.min(20000, Math.max(300, wanted)); + const radius = Math.min(20000, Math.max(300, reach)); + // The same budget the Overpass path spends: per category, capped, so a + // mixed search does not spend the whole allowance on whichever kind + // happens to be densest. + const cap = Math.min(limit * wanted.length, POI_RESULT_CAP); const found = await trekPlacesNearby(lat, lng, { radius, - limit: 50, + limit: cap, category: terms.join(','), }); if (found.length > 0) { @@ -859,8 +880,11 @@ export class MapsService { name: p.name, lat: p.lat, lng: p.lng, - category, - poi_type: p.category ?? category, + // The category that produced the hit, not the list that was + // asked for: a mixed search must not label a petrol station as + // "fuel,charging,restaurant". + category: categoryOfTerm.get(p.category ?? '') ?? wanted[0], + poi_type: p.category ?? wanted[0], address: p.address?.freeform ?? null, website: p.contact?.website ?? null, phone: p.contact?.phone ?? null, @@ -880,17 +904,17 @@ export class MapsService { source: 'trek-places' as const, })), source: 'trek-places' as const, - truncated: found.length >= 50, + truncated: found.length >= cap, // A wide viewport is narrowed here too, and the caller is told so // for the same reason the Overpass path tells it. - clamped: radius < wanted, + clamped: radius < reach, }; } } catch (err: unknown) { console.warn('TREK Places nearby failed, falling back:', (err as Error).message); } } - return this.searchOverpassPois(category, bbox, lang); + return this.searchOverpassPois(category, bbox, lang, limit); } // ── API key retrieval ────────────────────────────────────────────────────── diff --git a/server/src/nest/maps/trek-places.client.ts b/server/src/nest/maps/trek-places.client.ts index 83e173141..5fb95a47e 100644 --- a/server/src/nest/maps/trek-places.client.ts +++ b/server/src/nest/maps/trek-places.client.ts @@ -261,6 +261,14 @@ export const POI_CATEGORY_TO_TREK: Record = { activity: ['amusement_park', 'zoo', 'aquarium', 'water_park', 'theme_park'], shopping: ['shopping', 'shopping_center', 'department_store', 'market'], supermarket: ['grocery_store', 'food_and_beverage_store', 'convenience_store', 'supermarket'], + // What a drive needs rather than what a city visit does (#1797). The corridor + // search asks for these by name, and without them every corridor query fell + // through to Overpass — the one path where that hurts most, because a single + // search fans out over sixteen boxes. + fuel: ['gas_station', 'fueling_station'], + charging: ['ev_charging_station'], + rest_area: ['rest_stop'], + campsite: ['campground', 'rv_park'], }; export interface TrekNearbyPlace extends TrekPlace { diff --git a/server/src/nest/places/places.service.ts b/server/src/nest/places/places.service.ts index c0b04dcff..730c6342d 100644 --- a/server/src/nest/places/places.service.ts +++ b/server/src/nest/places/places.service.ts @@ -1167,16 +1167,17 @@ export class PlacesService { } if (!wp.name) continue; try { - // Background lane, which is what this method's own contract asks of a - // bulk caller: up to thirty sequential lookups, each taking the next - // slot on a 1.1 s process-wide throttle. On the interactive lane every - // other member of the instance typing in the place search box waits - // behind the whole import. - const hits = await this.maps.searchNominatim(wp.name, undefined, 'background'); - const hit = hits.find((h) => h.lat !== null && h.lng !== null); + // Through geocodeQuery, which asks the TREK index first and only falls + // through to Nominatim for what it does not know — and does so on the + // BACKGROUND lane. That matters here more than anywhere: this loop runs + // up to thirty times in one request, each Nominatim call taking the next + // slot on a 1.1 s process-wide throttle, so on the interactive lane one + // pasted link made everybody else's place search queue behind it for + // half a minute. An index hit costs no slot at all. + const hit = await this.maps.geocodeQuery(wp.name); // The name from the link, not the one the geocoder answers with: somebody who // typed a nickname into Google should not find a street address on their trip. - if (hit) places.push({ name: wp.name, lat: hit.lat!, lng: hit.lng!, notes: null, googleFtid: null }); + if (hit) places.push({ name: wp.name, lat: hit.lat, lng: hit.lng, notes: null, googleFtid: null }); else unplaceable++; } catch { // A geocoder that is down or rate-limited costs this one stop, not the import. diff --git a/server/tests/unit/nest/maps.pois.test.ts b/server/tests/unit/nest/maps.pois.test.ts index 02f0fdece..f7d731b9e 100644 --- a/server/tests/unit/nest/maps.pois.test.ts +++ b/server/tests/unit/nest/maps.pois.test.ts @@ -83,6 +83,9 @@ const OVERPASS_ANSWER = { website: null, phone: null, opening_hours: null, + brand: null, + brand_wikidata: null, + charging: null, cuisine: null, source: 'openstreetmap' as const, }, @@ -141,6 +144,9 @@ describe('MapsService.pois answered from the index', () => { website: 'https://cafecentral.wien/', phone: '+43 1 5333763', opening_hours: 'Mo-Sa 07:30-22:00', + brand: null, + brand_wikidata: null, + charging: null, cuisine: null, // Named as what it is. Overture is not OpenStreetMap — it carries OSM // among other sources under other licences — and this branch argues @@ -171,6 +177,11 @@ describe('MapsService.pois answered from the index', () => { website: null, phone: null, opening_hours: null, + brand: null, + brand_wikidata: null, + // Sockets are an OSM thing; a station answered from the index reports + // "not stated" rather than claiming it offers nothing. + charging: null, cuisine: null, source: 'trek-places', }); @@ -205,6 +216,11 @@ describe('MapsService.pois answered from the index', () => { website: null, phone: null, opening_hours: 'Mo-Sa 07:30-22:00', + brand: null, + brand_wikidata: null, + // Sockets are an OSM thing; a station answered from the index reports + // "not stated" rather than claiming it offers nothing. + charging: null, cuisine: null, source: 'trek-places', }); @@ -222,27 +238,68 @@ describe('MapsService.pois answered from the index', () => { // a place in a corner of the viewport is still inside the circle. expect(mockNearby).toHaveBeenCalledWith(0, 0, { radius: 7872, - limit: 50, + // The caller's budget, per category, the way the Overpass path spends it. + limit: 60, category: 'cafe,coffee_shop', }); expect(out.clamped).toBe(false); }); - it('MAPS-POIS-005: a full page of 50 is reported as truncated', async () => { - mockNearby.mockResolvedValue(rows(50)); + it('MAPS-POIS-013: a corridor search asks the index once for all its categories', async () => { + // The corridor sends a comma-separated list. Looking the whole string up as + // one key missed every time, so every corridor query fell through to + // Overpass — on the one path where that hurts most, because a single search + // fans out over sixteen boxes and each races four public mirrors. + mockNearby.mockResolvedValue([ + { ...FULL, gers: 'f-1', name: 'Aral', category: 'gas_station' }, + { ...FULL, gers: 'c-1', name: 'Ionity', category: 'ev_charging_station' }, + ]); + const svc = make(); + const overpass = stubOverpass(svc); + + const out = await svc.pois('fuel,charging', BOX); + + expect(overpass).not.toHaveBeenCalled(); + // One request carrying every term the two categories map to. + expect(mockNearby).toHaveBeenCalledWith(0, 0, expect.objectContaining({ + category: 'gas_station,fueling_station,ev_charging_station', + // Per category, so a mixed search does not spend the whole allowance on + // whichever kind happens to be densest. + limit: 120, + })); + // Each hit carries the category that produced it, not the list that was + // asked for: the client colours and groups its markers by that field, and + // "fuel,charging" is not a category. + expect(out.pois.map(p => p.category)).toEqual(['fuel', 'charging']); + }); + + it('MAPS-POIS-014: a category the index has no terms for sends the whole query to Overpass', async () => { + // All or nothing. Answering the half it knows would silently drop the rest, + // and the caller counts a finished search either way. + const svc = make(); + const overpass = stubOverpass(svc); + + await svc.pois('fuel,something-new', BOX, 'de'); + + expect(mockNearby).not.toHaveBeenCalled(); + expect(overpass).toHaveBeenCalledWith('fuel,something-new', BOX, 'de', 60); + }); + + it('MAPS-POIS-005: a full page is reported as truncated', async () => { + mockNearby.mockResolvedValue(rows(60)); const svc = make(); stubOverpass(svc); const out = await svc.pois('cafe', BOX); - expect(out.pois).toHaveLength(50); + expect(out.pois).toHaveLength(60); // The page asked for is the page compared against — a caller told nothing // was cut off will not offer to zoom in for the rest. expect(out.truncated).toBe(true); }); it('MAPS-POIS-006: a page short of the limit is the whole answer', async () => { - mockNearby.mockResolvedValue(rows(49)); + mockNearby.mockResolvedValue(rows(59)); const svc = make(); const overpass = stubOverpass(svc); @@ -251,7 +308,7 @@ describe('MapsService.pois answered from the index', () => { // The Overpass answer is untruncated as well, so the count and the untouched // spy are what tell an index answer from a fallback. expect(overpass).not.toHaveBeenCalled(); - expect(out.pois).toHaveLength(49); + expect(out.pois).toHaveLength(59); expect(out.truncated).toBe(false); }); @@ -301,7 +358,7 @@ describe('MapsService.pois falls back to Overpass', () => { expect(await svc.pois('cafe', BOX, 'de')).toEqual(OVERPASS_ANSWER); expect(mockNearby).not.toHaveBeenCalled(); - expect(overpass).toHaveBeenCalledWith('cafe', BOX, 'de'); + expect(overpass).toHaveBeenCalledWith('cafe', BOX, 'de', 60); }); it('MAPS-POIS-011: an empty index page is a miss, not an empty answer', async () => { @@ -310,7 +367,7 @@ describe('MapsService.pois falls back to Overpass', () => { const overpass = stubOverpass(svc); expect(await svc.pois('cafe', BOX, 'de')).toEqual(OVERPASS_ANSWER); - expect(overpass).toHaveBeenCalledWith('cafe', BOX, 'de'); + expect(overpass).toHaveBeenCalledWith('cafe', BOX, 'de', 60); }); it('MAPS-POIS-012: an index failure is logged and dropped through, never surfaced', async () => { @@ -319,7 +376,7 @@ describe('MapsService.pois falls back to Overpass', () => { const overpass = stubOverpass(svc); expect(await svc.pois('cafe', BOX, 'de')).toEqual(OVERPASS_ANSWER); - expect(overpass).toHaveBeenCalledWith('cafe', BOX, 'de'); + expect(overpass).toHaveBeenCalledWith('cafe', BOX, 'de', 60); expect(warn).toHaveBeenCalledWith('TREK Places nearby failed, falling back:', 'places api down'); }); }); diff --git a/server/tests/unit/nest/places.directions.test.ts b/server/tests/unit/nest/places.directions.test.ts index 9f3ba9dc0..aa5e9fd53 100644 --- a/server/tests/unit/nest/places.directions.test.ts +++ b/server/tests/unit/nest/places.directions.test.ts @@ -71,7 +71,19 @@ function svc(searchNominatim: MapsService['searchNominatim']): PlacesService { new RealtimeService(), // The address backfill runs fire-and-forget after every import, so the stub answers // it too — otherwise every passing test prints a rejected promise. - { searchNominatim, reverseGeocode: vi.fn(async () => null) } as unknown as MapsService, + // geocodeQuery is what the importer calls now: it asks the TREK index first + // and falls through to Nominatim on the background lane. Stubbed in terms of + // the searchNominatim each case already provides, so the cases keep driving + // one seam while the code under test uses the real one. + { + searchNominatim, + geocodeQuery: async (query: string) => { + const hit = (await searchNominatim(query, undefined, 'background')) + .find((h: { lat: number | null; lng: number | null }) => h.lat !== null && h.lng !== null); + return hit ? { lat: hit.lat as number, lng: hit.lng as number } : null; + }, + reverseGeocode: vi.fn(async () => null), + } as unknown as MapsService, new QueryHelpersService(dbs), new UnsplashService(dbs, new RuntimeEnvService(), storageFx.storage), photoCacheStub, @@ -202,19 +214,26 @@ describe('PlacesService.importGoogleDirections', () => { expect((result as { error: string }).error).toMatch(/Share button/); }); - it('PLACES-DIR-012: the bulk geocoding yields to whoever is typing', async () => { - // searchNominatim's own contract asks a bulk caller for the background - // lane, naming this exact shape: up to thirty sequential lookups, each - // taking the next slot on a 1.1 s process-wide throttle. On the interactive - // lane every other member of the instance typing in the place search box - // queues behind the whole import. - const search = vi.fn(async () => [{ lat: 52.52, lng: 13.405 }]) as unknown as MapsService['searchNominatim']; - await svc(search).importGoogleDirections(tripId, 'https://www.google.com/maps/dir/Berlin/Dresden'); + it('PLACES-DIR-012: the bulk geocoding goes through the index, not straight at Nominatim', async () => { + // geocodeQuery asks the TREK index first and only falls through to + // Nominatim for what it does not know, on the BACKGROUND lane. That matters + // more here than anywhere: this loop runs up to thirty times in one request, + // each Nominatim call taking the next slot on a 1.1 s process-wide throttle, + // so on the interactive lane one pasted link made everybody else's place + // search queue behind it for half a minute. An index hit costs no slot. + const nominatim = vi.fn(async () => [{ lat: 52.52, lng: 13.405 }]) as unknown as MapsService['searchNominatim']; + const geocode = vi.fn(async () => ({ lat: 52.52, lng: 13.405 })); + const service = svc(nominatim); + (service as unknown as { maps: Partial }).maps.geocodeQuery = + geocode as unknown as MapsService['geocodeQuery']; - expect(search).toHaveBeenCalled(); - for (const call of (search as unknown as { mock: { calls: unknown[][] } }).mock.calls) { - expect(call[2]).toBe('background'); - } + await service.importGoogleDirections(tripId, 'https://www.google.com/maps/dir/Berlin/Dresden'); + + expect(geocode).toHaveBeenCalledWith('Berlin'); + expect(geocode).toHaveBeenCalledWith('Dresden'); + // Not reached directly: whether Nominatim is asked at all, and on which + // lane, is geocodeQuery's business and is pinned in maps.geocode.test.ts. + expect(nominatim).not.toHaveBeenCalled(); }); it('PLACES-DIR-011: a route shared as a short link is imported as a route', async () => {