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 () => {