From fc631d36b66d89ca5f8b01af8e68cfd2de1bbeab Mon Sep 17 00:00:00 2001 From: Maurice Date: Sun, 18 Oct 2026 12:27:12 +0200 Subject: [PATCH] fix(maps): category pills ask the index by Overture path, near misses are left out The index matches a term anywhere inside a place's category text, so the bare names behind the pills caught whatever was spelled like them: "park" brought car parks and an amusement park, "garden" furniture stores and beer gardens, "bar" barbers, "pub" publishers and public squares, "market" marketing agencies. In Vienna four of sixty "Nature & parks" pins were parks. Every pill now names its Overture categories by full path, a place counts only when its path is that path or lies under it, and a page of near misses only falls through to Overpass. Terms Overture no longer has (nightclub, nature_preserve, shopping_center ...) got their current names. Closes #2663 --- server/src/nest/maps/maps-index.helpers.ts | 40 +++++---- server/src/nest/maps/maps.service.ts | 5 +- server/src/nest/maps/trek-places.client.ts | 73 ++++++++++++---- server/tests/unit/nest/maps.pois.test.ts | 83 ++++++++++++++++--- .../unit/nest/trek-places.client.test.ts | 49 +++++++++++ 5 files changed, 205 insertions(+), 45 deletions(-) diff --git a/server/src/nest/maps/maps-index.helpers.ts b/server/src/nest/maps/maps-index.helpers.ts index 520b24b8d..ff79f689c 100644 --- a/server/src/nest/maps/maps-index.helpers.ts +++ b/server/src/nest/maps/maps-index.helpers.ts @@ -9,6 +9,7 @@ import { buildOsmDetails, parsePoiCategories, samePlace, toApiLang } from './map import { POI_RESULT_CAP, type PoiSearchResult } from './providers/osm.client'; import { POI_CATEGORY_TO_TREK, + indexRowTerm, isOsmHit, osmPlaceId, toPlaceRecord, @@ -55,22 +56,15 @@ export function indexPoiPlan(category: string, bbox: PoiBbox, limit: number): In // by Overpass, and a mixed answer would silently drop it. const indexKnowsAll = wanted.length > 0 && wanted.every((key) => POI_CATEGORY_TO_TREK[key]?.length); if (!indexKnowsAll) return null; - // The index matches a term as a SUBSTRING of `category` and `category_path` - // (see POI_CATEGORY_TO_TREK), so an exact lookup misses every leaf that is - // not literally a term: `italian_restaurant` is a hit for `restaurant` and - // finds nothing here. Falling through to wanted[0] then labelled it with - // whichever pill the user happened to tap first, and the corridor panel - // groups and colours on exactly that field, so a trattoria came back as a - // petrol station. Longest match wins, because `fast_food` must not lose to - // `food` when both are terms of different categories. + // A row belongs to the term whose path it lies under, not to one it is merely + // spelled like: the leaf alone is rarely a term (`italian_restaurant` sits + // under `food_and_drink>restaurant`), and falling through to wanted[0] then + // labelled it with whichever pill the user tapped first, so a trattoria came + // back as a petrol station. The most specific term wins, so a water park + // answers the activity pill rather than the park it sits under. const labelFor = (leaf: string | null, path: string | null): string | undefined => { - const haystack = `${leaf ?? ''} ${path ?? ''}`; - let best: string | undefined; - for (const term of categoryOfTerm.keys()) { - if (!haystack.includes(term)) continue; - if (best === undefined || term.length > best.length) best = term; - } - return best === undefined ? undefined : categoryOfTerm.get(best); + const term = indexRowTerm(categoryOfTerm.keys(), leaf, path); + return term === undefined ? undefined : categoryOfTerm.get(term); }; const lat = (bbox.south + bbox.north) / 2; const lng = (bbox.west + bbox.east) / 2; @@ -109,7 +103,7 @@ export function indexPoiAnswer( ): PoiSearchResult { const { wanted, labelFor, cap, radius, reach } = plan; return { - pois: found.map((p) => ({ + pois: belonging(found, plan).map((p) => ({ osm_id: `gers:${p.gers}`, name: p.name, lat: p.lat, @@ -138,6 +132,8 @@ export function indexPoiAnswer( source: 'trek-places' as const, })), source: 'trek-places' as const, + // The page the index sent, not what was left of it: a full page means more + // places, however many of them this one had to leave out. 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. @@ -145,6 +141,18 @@ export function indexPoiAnswer( }; } +/** + * The rows that belong to a pill the request asked for. The index matches its + * terms inside the category text, so a near miss can still come back, and it + * is left out here (#2663). A row the index holds without any category cannot + * be told either way and stays, as it always has. + */ +export function belonging(found: T[], plan: IndexPoiPlan): T[] { + return found.filter( + (p) => (!p.category && !p.categoryPath) || plan.labelFor(p.category ?? null, p.categoryPath ?? null) !== undefined, + ); +} + const regionNamesByLang = new Map(); /** diff --git a/server/src/nest/maps/maps.service.ts b/server/src/nest/maps/maps.service.ts index afc5942be..a0790154e 100644 --- a/server/src/nest/maps/maps.service.ts +++ b/server/src/nest/maps/maps.service.ts @@ -4,7 +4,7 @@ import type { AppSettingsRepository } from '../../db/repositories/AppSettings.re import type { GeoLane } from '../geo/nominatim.client'; import { PlacePhotoCacheService } from '../place-photos/place-photo-cache.service'; import type { ApiKeySource } from '../settings/instance-api-keys'; -import { indexPoiAnswer, indexPoiPlan, indexSuggestions } from './maps-index.helpers'; +import { belonging, indexPoiAnswer, indexPoiPlan, indexSuggestions } from './maps-index.helpers'; import { NEARBY_DEFAULT_LIMIT, NEARBY_DEFAULT_RADIUS_M, nearbyCacheKey, nearestFirst } from './maps-nearby.helpers'; import { MapsUrlResolver, type ResolvedMapsUrl } from './maps-url.resolver'; import { toApiLang, mergeSearchResults } from './maps.helpers'; @@ -214,7 +214,8 @@ export class MapsService { limit: plan.cap, category: plan.terms.join(','), }); - if (found.length > 0) return indexPoiAnswer(found, plan); + // A page of near misses only is a miss too, not an empty answer. + if (belonging(found, plan).length > 0) return indexPoiAnswer(found, plan); } catch (err: unknown) { console.warn('TREK Places nearby failed, falling back:', (err as Error).message); } diff --git a/server/src/nest/maps/trek-places.client.ts b/server/src/nest/maps/trek-places.client.ts index 94add9583..06c650587 100644 --- a/server/src/nest/maps/trek-places.client.ts +++ b/server/src/nest/maps/trek-places.client.ts @@ -345,31 +345,70 @@ export function indexHitsOnly(hits: TrekSearchHit[]): TrekPlace[] { * * Deliberately a hand-written map rather than a guess at runtime: the two * taxonomies do not line up, and a pill that quietly returns the wrong kind of - * place is worse than one that returns none. Each entry is a substring matched - * against `category` and `category_path`, so `eat_and_drink>cafe` answers the - * cafe pill without listing every leaf. + * place is worse than one that returns none. Each entry is a category path, and + * a place belongs to it when its own path is that path or lies under it, so + * `food_and_drink>restaurant` answers the italian restaurant two levels down. + * + * Paths, not bare leaf names: the index matches a term anywhere inside + * `category` and `category_path`, and a bare `park` brought car parks, `garden` + * furniture stores and `bar` barbers (#2663). A full path does not occur inside + * an unrelated one, and {@link indexRowTerm} drops whatever still slips through. + * Overture renames a branch now and then; a path that stops existing answers + * nothing, and the pill falls back to Overpass instead of answering wrong. */ export const POI_CATEGORY_TO_TREK: Record = { - restaurant: ['restaurant', 'casual_eatery', 'fast_food'], - cafe: ['cafe', 'coffee_shop'], - bar: ['bar', 'pub', 'nightclub', 'brewery'], - hotel: ['hotel', 'lodging', 'hostel', 'motel', 'bed_and_breakfast'], - sights: ['historic_site', 'landmark_and_historical_building', 'monument', 'castle', 'tourist_attraction'], - museum: ['museum', 'art_gallery', 'performing_arts', 'theater'], - nature: ['park', 'garden', 'beach', 'nature_preserve', 'mountain'], - 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'], + restaurant: ['food_and_drink>restaurant', 'food_and_drink>casual_eatery'], + cafe: ['food_and_drink>casual_eatery>cafe', 'food_and_drink>non_alcoholic_beverage_venue>coffee_shop'], + bar: ['food_and_drink>alcoholic_beverage_venue', 'arts_and_entertainment>nightlife_venue'], + hotel: ['lodging'], + sights: ['cultural_and_historic>historic_site', 'cultural_and_historic>memorial_site>monument'], + museum: [ + 'arts_and_entertainment>museum', + 'arts_and_entertainment>arts_and_crafts_space>art_gallery', + 'arts_and_entertainment>performing_arts_venue', + 'arts_and_entertainment>movie_theater', + ], + nature: [ + 'sports_and_recreation>park', + 'geographic_entities>built_feature>garden', + 'geographic_entities>land_feature>beach', + 'geographic_entities>land_feature>nature_reserve', + 'geographic_entities>land_feature>mountain', + ], + activity: [ + 'arts_and_entertainment>amusement_attraction>amusement_park', + 'arts_and_entertainment>animal_attraction>zoo', + 'arts_and_entertainment>animal_attraction>aquarium', + 'sports_and_recreation>park>water_park', + ], + shopping: ['shopping'], + supermarket: ['shopping>food_and_beverage_store', 'shopping>convenience_store'], // 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'], + fuel: ['travel_and_transportation>fueling_station>gas_station'], + charging: ['travel_and_transportation>fueling_station>ev_charging_station'], + rest_area: ['travel_and_transportation>ground_transport_facility_or_service>road_structures_and_service>rest_stop'], + campsite: ['lodging>campground', 'lodging>rv_park'], }; +/** + * The term of {@link POI_CATEGORY_TO_TREK} a row belongs to, the most specific + * one when several fit, or undefined when none does. A row is judged by its path + * where it has one and by its leaf where the index left the path empty. + */ +export function indexRowTerm(terms: Iterable, leaf: string | null, path: string | null): string | undefined { + let best: string | undefined; + for (const term of terms) { + const fits = path + ? path === term || path.startsWith(`${term}>`) + : !!leaf && leaf === term.slice(term.lastIndexOf('>') + 1); + if (fits && (best === undefined || term.length > best.length)) best = term; + } + return best; +} + export interface TrekNearbyPlace extends TrekPlace { distanceMetres: number; } diff --git a/server/tests/unit/nest/maps.pois.test.ts b/server/tests/unit/nest/maps.pois.test.ts index a7e53c010..5c281edce 100644 --- a/server/tests/unit/nest/maps.pois.test.ts +++ b/server/tests/unit/nest/maps.pois.test.ts @@ -49,7 +49,7 @@ const FULL = { lat: 48.2101, lng: 16.3657, category: 'coffee_shop', - categoryPath: 'eat_and_drink>cafe>coffee_shop', + categoryPath: 'food_and_drink>non_alcoholic_beverage_venue>coffee_shop', confidence: 0.94, address: { freeform: 'Herrengasse 14', locality: 'Wien', postcode: '1010', region: null, country: 'AT' }, contact: { website: 'https://cafecentral.wien/', phone: '+43 1 5333763', email: null, socials: null }, @@ -285,7 +285,7 @@ describe('MapsService.pois answered from the index', () => { radius: 7872, // The caller's budget, per category, the way the Overpass path spends it. limit: 60, - category: 'cafe,coffee_shop', + category: 'food_and_drink>casual_eatery>cafe,food_and_drink>non_alcoholic_beverage_venue>coffee_shop', }); expect(out.clamped).toBe(false); }); @@ -296,8 +296,20 @@ describe('MapsService.pois answered from the index', () => { // 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' }, + { + ...FULL, + gers: 'f-1', + name: 'Aral', + category: 'gas_station', + categoryPath: 'travel_and_transportation>fueling_station>gas_station', + }, + { + ...FULL, + gers: 'c-1', + name: 'Ionity', + category: 'ev_charging_station', + categoryPath: 'travel_and_transportation>fueling_station>ev_charging_station', + }, ]); const svc = make(); const overpass = stubOverpass(svc); @@ -310,7 +322,8 @@ describe('MapsService.pois answered from the index', () => { 0, 0, expect.objectContaining({ - category: 'gas_station,fueling_station,ev_charging_station', + category: + 'travel_and_transportation>fueling_station>gas_station,travel_and_transportation>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, @@ -333,9 +346,15 @@ describe('MapsService.pois answered from the index', () => { gers: 'r-1', name: 'Trattoria', category: 'italian_restaurant', - categoryPath: 'eat_and_drink>restaurant>italian_restaurant', + categoryPath: 'food_and_drink>restaurant>european_restaurant>italian_restaurant', + }, + { + ...FULL, + gers: 'f-1', + name: 'Aral', + category: 'gas_station', + categoryPath: 'travel_and_transportation>fueling_station>gas_station', }, - { ...FULL, gers: 'f-1', name: 'Aral', category: 'gas_station', categoryPath: 'automotive>gas_station' }, ]); const svc = make(); stubOverpass(svc); @@ -352,22 +371,66 @@ describe('MapsService.pois answered from the index', () => { expect(out.pois[0].poi_type).toBe('italian_restaurant'); }); - it('MAPS-POIS-013c: the longest matching term wins, so fast_food does not answer as a cafe', async () => { + it('MAPS-POIS-013c: the most specific term wins, so a cafe under casual_eatery answers the cafe pill', async () => { mockNearby.mockResolvedValue([ { ...FULL, gers: 'q-1', name: 'Imbiss', category: 'fast_food_restaurant', - categoryPath: 'eat_and_drink>fast_food>fast_food_restaurant', + categoryPath: 'food_and_drink>casual_eatery>fast_food_restaurant', }, + { ...FULL, gers: 'q-2', name: 'Kaffeehaus', category: 'cafe', categoryPath: 'food_and_drink>casual_eatery>cafe' }, ]); const svc = make(); stubOverpass(svc); const out = await svc.pois('cafe,restaurant', BOX); - expect(out.pois[0].category).toBe('restaurant'); + expect(out.pois.map((p) => [p.name, p.category])).toEqual([ + ['Imbiss', 'restaurant'], + ['Kaffeehaus', 'cafe'], + ]); + }); + + it('MAPS-POIS-013d: a place only spelled like the pill is left out, nature brings parks and no car parks (#2663)', async () => { + // What the index answered for Vienna: its terms match inside the category + // text, so `park` and `garden` brought car parks, furniture stores and a + // beer garden, and the parks were four in sixty. + const near = (gers: string, name: string, categoryPath: string) => ({ + ...FULL, + gers, + name, + category: categoryPath.slice(categoryPath.lastIndexOf('>') + 1), + categoryPath, + }); + mockNearby.mockResolvedValue([ + near('n-1', 'Stadtpark', 'sports_and_recreation>park'), + near('n-2', 'Garage am Ring', 'travel_and_transportation>parking>parking_garage'), + near('n-3', 'Möbelhaus', 'shopping>specialty_store>hardware_home_and_garden_store>furniture_store'), + near('n-4', 'Schweizerhaus', 'food_and_drink>alcoholic_beverage_venue>beer_garden'), + near('n-5', 'Prater', 'arts_and_entertainment>amusement_attraction>amusement_park'), + near('n-6', 'Botanischer Garten', 'geographic_entities>built_feature>garden>botanical_garden'), + near('n-7', 'Hundezone', 'sports_and_recreation>park>dog_park'), + ]); + const svc = make(); + const overpass = stubOverpass(svc); + + const out = await svc.pois('nature', BOX); + + expect(overpass).not.toHaveBeenCalled(); + expect(out.pois.map((p) => p.name)).toEqual(['Stadtpark', 'Botanischer Garten', 'Hundezone']); + // Seven came back, so the page was not full; leaving four out does not make it so. + expect(out.truncated).toBe(false); + }); + + it('MAPS-POIS-013e: a page holding near misses only is a miss, and Overpass answers', async () => { + mockNearby.mockResolvedValue([{ ...FULL, category: 'parking', categoryPath: 'travel_and_transportation>parking' }]); + const svc = make(); + const overpass = stubOverpass(svc); + + expect(await svc.pois('nature', BOX, 'de')).toEqual(OVERPASS_ANSWER); + expect(overpass).toHaveBeenCalledWith('nature', BOX, 'de', 60); }); it('MAPS-POIS-014: a category the index has no terms for sends the whole query to Overpass', async () => { diff --git a/server/tests/unit/nest/trek-places.client.test.ts b/server/tests/unit/nest/trek-places.client.test.ts index 949599996..eacaaccba 100644 --- a/server/tests/unit/nest/trek-places.client.test.ts +++ b/server/tests/unit/nest/trek-places.client.test.ts @@ -8,6 +8,7 @@ import { DEFAULT_TREK_PLACES_URL, POI_CATEGORY_TO_TREK, + indexRowTerm, toPlaceRecord, trekPlacesArea, trekPlacesBaseUrl, @@ -745,4 +746,52 @@ describe('POI_CATEGORY_TO_TREK', () => { expect(terms.length, pill).toBeGreaterThan(0); } }); + + it('names every category by its full path, so no bare word matches inside another (#2663)', () => { + // A bare `park` is inside `parking`, `bar` inside `barber`, `market` inside + // `marketing_agency`. Only the top-level `shopping` stands alone, and it is + // a whole branch on purpose. + for (const [pill, terms] of Object.entries(POI_CATEGORY_TO_TREK)) { + for (const term of terms) { + if (term === 'shopping' || term === 'lodging') continue; + expect(term, pill).toContain('>'); + } + } + }); +}); + +describe('indexRowTerm (#2663)', () => { + const nature = POI_CATEGORY_TO_TREK.nature; + + it('takes a place whose path is the term or lies under it', () => { + expect(indexRowTerm(nature, 'park', 'sports_and_recreation>park')).toBe('sports_and_recreation>park'); + expect(indexRowTerm(nature, 'national_park', 'sports_and_recreation>park>national_park')).toBe( + 'sports_and_recreation>park', + ); + }); + + it('leaves out a place whose path only contains the word', () => { + expect(indexRowTerm(nature, 'parking', 'travel_and_transportation>parking')).toBeUndefined(); + expect(indexRowTerm(nature, 'beer_garden', 'food_and_drink>alcoholic_beverage_venue>beer_garden')).toBeUndefined(); + expect( + indexRowTerm( + nature, + 'furniture_store', + 'shopping>specialty_store>hardware_home_and_garden_store>furniture_store', + ), + ).toBeUndefined(); + }); + + it('judges a place without a path by its leaf', () => { + expect(indexRowTerm(nature, 'beach', null)).toBe('geographic_entities>land_feature>beach'); + expect(indexRowTerm(nature, 'beach_bar', null)).toBeUndefined(); + expect(indexRowTerm(nature, null, null)).toBeUndefined(); + }); + + it('picks the most specific term when several fit', () => { + const terms = [...POI_CATEGORY_TO_TREK.nature, ...POI_CATEGORY_TO_TREK.activity]; + expect(indexRowTerm(terms, 'water_park', 'sports_and_recreation>park>water_park')).toBe( + 'sports_and_recreation>park>water_park', + ); + }); });