diff --git a/server/src/nest/maps/maps-index.helpers.ts b/server/src/nest/maps/maps-index.helpers.ts index bf9ed09e3..9f0b1e327 100644 --- a/server/src/nest/maps/maps-index.helpers.ts +++ b/server/src/nest/maps/maps-index.helpers.ts @@ -5,7 +5,7 @@ * the decision to make them is MapsService's. */ import { getCountryFromCoords } from '../../utils/country-borders'; -import { buildOsmDetails, parsePoiCategories, samePlace, toApiLang } from './maps.helpers'; +import { buildOsmDetails, interleave, parsePoiCategories, samePlace, toApiLang } from './maps.helpers'; import { POI_RESULT_CAP, type PoiSearchResult } from './providers/osm.client'; import { INDEX_MAX_CATEGORY_CHARS, @@ -335,6 +335,22 @@ export function indexSuggestions(found: TrekSearchHit[], lang?: string): MapsAut ); } +type Suggestion = MapsAutocompleteResult['suggestions'][number]; + +/** + * The index's suggestions with Photon's interleaved, the way the search list + * merges the index with Nominatim (#2877). A Photon row the list already holds + * is dropped: the same OpenStreetMap element under the layer's copy of its id, + * or the same place by the rule the search list dedupes with. + */ +export function mergeSuggestions(fromIndex: Suggestion[], fromPhoton: Suggestion[], limit = 8): Suggestion[] { + const asPlace = (s: Suggestion) => ({ lat: s.lat, lng: s.lng, name: s.mainText }); + const extra = fromPhoton.filter( + (o) => !fromIndex.some((i) => i.placeId === o.placeId || samePlace(asPlace(i), asPlace(o))), + ); + return interleave(fromIndex, extra, limit); +} + /** * The details record of a place picked out of the index, with what * OpenStreetMap knows about the same building filled in. diff --git a/server/src/nest/maps/maps.helpers.ts b/server/src/nest/maps/maps.helpers.ts index 6afcaf1d1..8fb7a7ec7 100644 --- a/server/src/nest/maps/maps.helpers.ts +++ b/server/src/nest/maps/maps.helpers.ts @@ -115,11 +115,15 @@ export function mergeSearchResults( limit = 10, ): Record[] { const extra = fromOsm.filter((o) => !fromIndex.some((i) => samePlace(i, o))); + return interleave(fromIndex, extra, limit); +} - const out: Record[] = []; - for (let i = 0; i < Math.max(fromIndex.length, extra.length) && out.length < limit; i++) { - if (i < fromIndex.length) out.push(fromIndex[i]); - if (i < extra.length && out.length < limit) out.push(extra[i]); +/** One row of each list in turn, the first list leading, up to `limit` rows. */ +export function interleave(first: T[], second: T[], limit: number): T[] { + const out: T[] = []; + for (let i = 0; i < Math.max(first.length, second.length) && out.length < limit; i++) { + if (i < first.length) out.push(first[i]); + if (i < second.length && out.length < limit) out.push(second[i]); } return out; } diff --git a/server/src/nest/maps/maps.service.ts b/server/src/nest/maps/maps.service.ts index 1ad9237bc..e0e90b06f 100644 --- a/server/src/nest/maps/maps.service.ts +++ b/server/src/nest/maps/maps.service.ts @@ -11,6 +11,7 @@ import { indexRequests, indexSuggestions, mergeIndexPages, + mergeSuggestions, } 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'; @@ -542,29 +543,39 @@ export class MapsService { } if (this.trekPlacesEnabled() && !(await this.selector.googleOnly(keyed))) { - try { - const centre = boxCentre; - // Both layers here, unlike the explicit search above. That path asks - // Nominatim in parallel and would get the same OpenStreetMap places - // twice; this one asks nobody else, because Nominatim's usage policy - // names autocomplete as unacceptable use. So the layer is not a second - // opinion here, it is the only place the missing names live. - // - // Measured on the case that surfaced it: "Tokio station" typed from - // Tokyo returned a weigh station in Ritzville and a station in Mexico - // from the index alone, and "Tokio Hauptbahnhof" in second place with - // the layer. The index holds businesses; stations, temples and bridges - // are in OpenStreetMap, and so is every exonym a traveller types. - const found = await trekPlacesSearch(input, { - lat: centre?.lat, - lng: centre?.lng, + // Both layers here, unlike the explicit search above. That path asks + // Nominatim in parallel and would get the same OpenStreetMap places + // twice; this one never asks Nominatim, because its usage policy names + // autocomplete as unacceptable use. + // + // Measured on the case that surfaced the layer: "Tokio station" typed + // from Tokyo returned a weigh station in Ritzville and a station in + // Mexico from the index alone, and "Tokio Hauptbahnhof" in second place + // with the layer. The index holds businesses; stations, temples and + // bridges are in OpenStreetMap, and so is every exonym a traveller types. + // + // Photon beside them (#2877). The layer is a slice of OpenStreetMap, and + // a place missing from it showed up only once the search was sent. + // Photon is built for this keystroke and answers null when the services + // are off, so the pair costs the slower one and an install without + // Photon reads exactly as before. Each side swallows its own failure, so + // one that is down does not throw away what the other found. + const [found, photon] = await Promise.all([ + trekPlacesSearch(input, { + lat: boxCentre?.lat, + lng: boxCentre?.lng, limit: 8, sources: 'index,osm', lang, - }); - if (found.length > 0) return { suggestions: indexSuggestions(found, lang), source: 'trek-places' }; - } catch (err: unknown) { - console.warn('TREK Places autocomplete failed, falling back:', (err as Error).message); + }).catch((err: unknown) => { + console.warn('TREK Places autocomplete failed, falling back:', (err as Error).message); + return []; + }), + this.osm.autocompletePhoton(input, lang, boxCentre), + ]); + const suggestions = mergeSuggestions(indexSuggestions(found, lang), photon ?? []); + if (suggestions.length > 0) { + return { suggestions, source: found.length > 0 ? 'trek-places' : 'openstreetmap' }; } } diff --git a/server/src/nest/maps/providers/osm.client.ts b/server/src/nest/maps/providers/osm.client.ts index 2e8adfe15..b33fb4ec4 100644 --- a/server/src/nest/maps/providers/osm.client.ts +++ b/server/src/nest/maps/providers/osm.client.ts @@ -48,7 +48,7 @@ import { import { overpassSelector, tagsMatchFilter } from '../osm-filter.helpers'; import { OverpassBusy, hedged, lastAnsweredFirst, retryAfterMs, soonestRetryMs } from '../overpass-hedge.helpers'; import { PhotonClient } from './photon.client'; -import { asNominatimHit, asNominatimReverse } from './photon.mapping'; +import { asNominatimHit, asNominatimReverse, asSuggestion } from './photon.mapping'; import { readWikiIdentity } from './wiki-identity'; import { Injectable } from '@nestjs/common'; import { normalizePlaceWebsite } from '@trek/shared'; @@ -526,6 +526,16 @@ export class OsmClient { } } + /** + * Suggestions from Photon, which is built for search-as-you-type, or null when + * it is not in use or did not answer. Never Nominatim in its place: its usage + * policy rules out autocomplete. + */ + async autocompletePhoton(input: string, lang?: string, bias?: { lat: number; lng: number }) { + const found = await this.photon.search(input, { lang, bias }); + return found?.map(asSuggestion).filter((row) => row !== null) ?? null; + } + /** * Finds the OpenStreetMap record for a place we only know by name and * coordinate, and hands back its tags. diff --git a/server/src/nest/maps/providers/photon.mapping.ts b/server/src/nest/maps/providers/photon.mapping.ts index 36ba49a7a..411b86489 100644 --- a/server/src/nest/maps/providers/photon.mapping.ts +++ b/server/src/nest/maps/providers/photon.mapping.ts @@ -10,6 +10,7 @@ */ import { toApiLang } from '../maps.helpers'; import type { NominatimResult, NominatimReverse } from './osm.client'; +import type { MapsAutocompleteResult } from '@trek/shared'; import { z } from 'zod'; @@ -74,6 +75,7 @@ export const photonAnswerSchema = z.object({ features: z.array(photonFeatureSche export type PhotonFeature = z.infer; type PhotonProperties = PhotonFeature['properties']; +type Suggestion = MapsAutocompleteResult['suggestions'][number]; /** * The language Photon is asked in: TREK's code through toApiLang (`br` is @@ -133,6 +135,33 @@ export function asNominatimHit(feature: PhotonFeature): NominatimResult | null { }; } +/** + * A search hit as one row of the suggestion list, in the id form the details + * lookup reads (`node:123`, as the index's OpenStreetMap layer hands it over), + * or null for one without an id or a name to show. + */ +export function asSuggestion(feature: PhotonFeature): Suggestion | null { + const hit = asNominatimHit(feature); + const [lng, lat] = feature.geometry.coordinates; + if (!hit || !Number.isFinite(lat) || !Number.isFinite(lng)) return null; + // The address line starts with the name, so the rest of it reads as where the + // place is. Cut by the name rather than at its first comma, which a name can + // hold; a house without a name is titled by its number, as Nominatim does. + const name = feature.properties.name?.trim(); + const line = hit.display_name; + const [head = '', ...rest] = line.split(', '); + const mainText = name || head; + if (!mainText) return null; + return { + placeId: `${hit.osm_type}:${hit.osm_id}`, + mainText, + secondaryText: name ? line.slice(name.length).replace(/^, /, '') : rest.join(', '), + source: 'openstreetmap', + lat, + lng, + }; +} + /** * The point a reverse lookup found, in the fields Nominatim's reverse answer * has. A town itself is its own `city`, and a named place is filed under its diff --git a/server/tests/unit/nest/maps.autocomplete.test.ts b/server/tests/unit/nest/maps.autocomplete.test.ts index ec2b78160..fd7f83c78 100644 --- a/server/tests/unit/nest/maps.autocomplete.test.ts +++ b/server/tests/unit/nest/maps.autocomplete.test.ts @@ -1,5 +1,5 @@ /** - * MAPS-AUTO-001..016: the suggestion list behind the place search box. + * MAPS-AUTO-001..024: the suggestion list behind the place search box. * * This is the path the index was built for. Nominatim's usage policy names * autocomplete as unacceptable use in its own words, whatever the rate, and the @@ -13,7 +13,8 @@ import type { AppSettingsRepository } from '../../../src/db/repositories/AppSett import type { UsersRepository } from '../../../src/db/repositories/Users.repository'; import type { PlacePhotoCacheService } from '../../../src/nest/place-photos/place-photo-cache.service'; import { noGoogleQuota } from '../../helpers/google-quota'; -import { buildMapsService } from '../../helpers/maps-service'; +import { buildMapsParts, buildMapsService } from '../../helpers/maps-service'; +import type { MapsAutocompleteResult } from '@trek/shared'; import { describe, it, expect, vi, beforeEach } from 'vitest'; @@ -47,6 +48,8 @@ vi.mock('../../../src/app-config', async (importOriginal) => { }; }); +type Suggestion = MapsAutocompleteResult['suggestions'][number]; + const INPUT = 'Café Kröpel'; const hit = (over: Record = {}) => ({ @@ -88,12 +91,44 @@ const osmHit = (over: Record = {}) => ({ */ function make(enabled = true, rows: Record = {}) { trekPlaces.on = enabled; - const appSettings = { + return buildMapsService( + {} as PlacePhotoCacheService, + settings(rows), + noUsers, + {} as never, + {} as never, + noGoogleQuota, + ); +} + +function settings(rows: Record): AppSettingsRepository { + return { getValue: async (key: string) => (rows[key] !== undefined ? rows[key] : null), } as unknown as AppSettingsRepository; - return buildMapsService({} as PlacePhotoCacheService, appSettings, noUsers, {} as never, {} as never, noGoogleQuota); } +/** + * The service with Photon answering `rows` (null: the services are off). The + * client itself is pinned in osm.photon.test.ts; here only what the keystroke + * path does with its answer matters. + */ +function withPhoton(rows: Suggestion[] | null, settingRows: Record = {}) { + trekPlaces.on = true; + const parts = buildMapsParts({} as PlacePhotoCacheService, settings(settingRows), noUsers, {} as never, {} as never); + const photon = vi.spyOn(parts.osm, 'autocompletePhoton').mockResolvedValue(rows); + return { svc: parts.svc, photon }; +} + +const photonRow = (over: Partial = {}): Suggestion => ({ + placeId: 'way:518071791', + mainText: 'Kröpeliner Tor', + secondaryText: 'Kröpeliner Straße, Rostock, Deutschland', + source: 'openstreetmap', + lat: 54.0893, + lng: 12.1325, + ...over, +}); + /** Google's autocomplete envelope, which is not the shape its text search answers in. */ const googleSuggestions = (name: string) => ({ ok: true, @@ -429,4 +464,56 @@ describe('MapsService.autocompletePlaces', () => { expect(suggestions[0].secondaryText).toBe('Somewhere'); }); + + it('MAPS-AUTO-021: Photon is asked beside the index and its places join the list (#2877)', async () => { + mockSearch.mockResolvedValue([hit(), hit({ gers: 'def-456', name: 'Café Central', lat: 54.2, lng: 12.2 })]); + const { svc, photon } = withPhoton([ + photonRow(), + photonRow({ placeId: 'node:7', mainText: 'Kröpeliner Straße', lat: 54.09, lng: 12.135 }), + ]); + const bias = { low: { lat: 54, lng: 12 }, high: { lat: 54.2, lng: 12.2 } }; + + const result = await svc.autocompletePlaces(1, INPUT, 'de', bias); + + expect(photon).toHaveBeenCalledWith(INPUT, 'de', { lat: 54.1, lng: 12.1 }); + expect(result.source).toBe('trek-places'); + expect(result.suggestions.map((s) => s.placeId)).toEqual([ + 'gers:abc-123', + 'way:518071791', + 'gers:def-456', + 'node:7', + ]); + expect(result.suggestions[1]).toMatchObject({ source: 'openstreetmap', mainText: 'Kröpeliner Tor' }); + }); + + it('MAPS-AUTO-022: what only Photon found is the answer when the index has nothing, named as OpenStreetMap', async () => { + mockSearch.mockResolvedValue([]); + const { svc } = withPhoton([photonRow()]); + + const result = await svc.autocompletePlaces(1, INPUT); + + expect(result).toEqual({ suggestions: [photonRow()], source: 'openstreetmap' }); + }); + + it('MAPS-AUTO-023: an index failure keeps what Photon found instead of throwing it away', async () => { + mockSearch.mockRejectedValue(new Error('places api down')); + const { svc } = withPhoton([photonRow()]); + + const result = await svc.autocompletePlaces(1, INPUT); + + expect(console.warn).toHaveBeenCalledWith('TREK Places autocomplete failed, falling back:', 'places api down'); + expect(result.suggestions).toEqual([photonRow()]); + }); + + it('MAPS-AUTO-024: the Google-only switch skips Photon the way it skips the index', async () => { + const fetchMock = vi.fn().mockResolvedValue(googleSuggestions('Tokyo Station')); + vi.stubGlobal('fetch', fetchMock); + const { svc, photon } = withPhoton([photonRow()], { maps_api_key: 'key', places_google_only: 'true' }); + + const result = await svc.autocompletePlaces(1, INPUT); + + expect(photon).not.toHaveBeenCalled(); + expect(result.source).toBe('google'); + vi.unstubAllGlobals(); + }); }); diff --git a/server/tests/unit/nest/maps.suggestion-merge.test.ts b/server/tests/unit/nest/maps.suggestion-merge.test.ts new file mode 100644 index 000000000..95205cecb --- /dev/null +++ b/server/tests/unit/nest/maps.suggestion-merge.test.ts @@ -0,0 +1,61 @@ +/** + * MAPS-SUGMERGE-001..004: the index's suggestions and Photon's in one list (#2877). + * + * The keystroke path asks the index with its OpenStreetMap layer and Photon at + * once. Both carry OpenStreetMap places, so the same element can come back from + * each side, and one list must show it once while still showing what only + * Photon found. + */ +import { mergeSuggestions } from '../../../src/nest/maps/maps-index.helpers'; + +import { describe, it, expect } from 'vitest'; + +const row = (placeId: string, mainText: string, lat: number, lng: number, source: string) => ({ + placeId, + mainText, + secondaryText: '', + source, + lat, + lng, +}); + +describe('mergeSuggestions', () => { + it('MAPS-SUGMERGE-001: interleaves the two, the index leading', () => { + const index = [ + row('gers:a', 'Café Kranzler', 52.5, 13.33, 'trek-places'), + row('gers:b', 'Café Central', 52.6, 13.4, 'trek-places'), + ]; + const photon = [ + row('way:1', 'Kranzler Eck', 52.503, 13.333, 'openstreetmap'), + row('node:2', 'Kranzlersee', 53, 13, 'openstreetmap'), + ]; + + expect(mergeSuggestions(index, photon).map((s) => s.placeId)).toEqual(['gers:a', 'way:1', 'gers:b', 'node:2']); + }); + + it("MAPS-SUGMERGE-002: drops Photon's copy of an element the layer already listed", () => { + const index = [row('node:9712313', 'Tokio Hauptbahnhof', 35.6811816, 139.76598265, 'openstreetmap')]; + // Photon names it in a different language, so only the id says it is one place. + const photon = [row('node:9712313', 'Tokyo Station', 35.6812, 139.7671, 'openstreetmap')]; + + expect(mergeSuggestions(index, photon)).toEqual(index); + }); + + it('MAPS-SUGMERGE-003: drops a Photon row for a place the index lists by the search-list rule', () => { + const index = [row('gers:a', 'Brandenburger Tor', 52.5163, 13.3777, 'trek-places')]; + const photon = [ + row('way:518071791', 'Brandenburger Tor', 52.51628, 13.37771, 'openstreetmap'), + row('node:3', 'Hotel Adlon', 52.5164, 13.3779, 'openstreetmap'), + ]; + + // The hotel next to the gate shares the spot and nothing else, so it stays. + expect(mergeSuggestions(index, photon).map((s) => s.placeId)).toEqual(['gers:a', 'node:3']); + }); + + it('MAPS-SUGMERGE-004: stops at the limit, and answers from Photon alone when the index found nothing', () => { + const photon = Array.from({ length: 10 }, (_, i) => row(`node:${i}`, `Place ${i}`, i, i, 'openstreetmap')); + + expect(mergeSuggestions([], photon)).toHaveLength(8); + expect(mergeSuggestions([], photon, 3).map((s) => s.placeId)).toEqual(['node:0', 'node:1', 'node:2']); + }); +}); diff --git a/server/tests/unit/nest/osm.photon.test.ts b/server/tests/unit/nest/osm.photon.test.ts index be0f619b4..d033017b8 100644 --- a/server/tests/unit/nest/osm.photon.test.ts +++ b/server/tests/unit/nest/osm.photon.test.ts @@ -1,5 +1,5 @@ /** - * OSM-PHOTON-001..012: the OpenStreetMap search and reverse lookup ask Photon + * OSM-PHOTON-001..014: the OpenStreetMap search and reverse lookup ask Photon * first where the installation holds a key, answer in the shape Nominatim's * answers always had, and fall back to Nominatim whenever Photon gives none. * Lookups by id stay on Nominatim. The network is a stubbed global fetch @@ -136,6 +136,31 @@ describe('search', () => { }, ]); }); + + it('OSM-PHOTON-013: the keystroke beside the index asks Photon alone, with the coordinates it found (#2877)', async () => { + photonReply = () => photonAnswer([gateFeature(), gateFeature({ osm_id: undefined })]); + const suggestions = await osm.autocompletePhoton('Brandenb', 'de', { lat: 52.5, lng: 13.4 }); + expect(hosts).toEqual(['photon.liketrek.com']); + expect(suggestions).toEqual([ + { + placeId: 'way:518071791', + mainText: 'Brandenburger Tor', + secondaryText: 'Pariser Platz, Mitte, Berlin, 10117, Deutschland', + source: 'openstreetmap', + lat: 52.5163, + lng: 13.3777, + }, + ]); + }); + + it('OSM-PHOTON-014: answers null without Photon rather than asking Nominatim, whose policy rules out autocomplete', async () => { + identity.bearer = null; + expect(await osm.autocompletePhoton('Brandenb', 'de')).toBeNull(); + photonReply = () => photonAnswer([], 503); + identity.bearer = 'tk1.kid.payload.signature'; + expect(await osm.autocompletePhoton('Brandenb', 'de')).toBeNull(); + expect(hosts).toEqual(['photon.liketrek.com']); + }); }); describe('reverse', () => { diff --git a/server/tests/unit/nest/photon.mapping.test.ts b/server/tests/unit/nest/photon.mapping.test.ts index 6d9e27bed..4b9d3a468 100644 --- a/server/tests/unit/nest/photon.mapping.test.ts +++ b/server/tests/unit/nest/photon.mapping.test.ts @@ -1,10 +1,11 @@ /** - * PHOTON-MAP-001..013: Photon's answers read into the shape Nominatim gives, - * and the language Photon is asked in. + * PHOTON-MAP-001..016: Photon's answers read into the shape Nominatim gives, + * the rows of the suggestion list, and the language Photon is asked in. */ import { asNominatimHit, asNominatimReverse, + asSuggestion, photonAnswerSchema, photonDisplayName, photonLang, @@ -121,3 +122,36 @@ describe('asNominatimReverse', () => { expect(asNominatimReverse(bare.features[0])).toEqual({ display_name: undefined, name: undefined, address: {} }); }); }); + +describe('asSuggestion', () => { + it('PHOTON-MAP-014: titles the row by its name, with where it is below, in the id form the details lookup reads', () => { + expect(asSuggestion(feature())).toEqual({ + placeId: 'way:518071791', + mainText: 'Brandenburger Tor', + secondaryText: 'Pariser Platz, Mitte, Berlin, 10117, Deutschland', + source: 'openstreetmap', + lat: 52.5163, + lng: 13.3777, + }); + }); + + it('PHOTON-MAP-015: keeps a name that holds a comma whole, and titles a house without one by its number', () => { + const bar = asSuggestion(feature({ name: 'Bar, Café Einstein' })); + expect(bar?.mainText).toBe('Bar, Café Einstein'); + expect(bar?.secondaryText).toBe('Pariser Platz, Mitte, Berlin, 10117, Deutschland'); + + const house = asSuggestion( + feature({ name: undefined, housenumber: '77', street: 'Unter den Linden', postcode: undefined }), + ); + expect(house?.mainText).toBe('77'); + expect(house?.secondaryText).toBe('Unter den Linden, Mitte, Berlin, Deutschland'); + }); + + it('PHOTON-MAP-016: has no row for a hit without an id or anything to call it', () => { + expect(asSuggestion(feature({ osm_id: undefined }))).toBeNull(); + const bare = photonAnswerSchema.parse({ + features: [{ geometry: { coordinates: [13.4, 52.5] }, properties: { osm_type: 'N', osm_id: 1 } }], + }); + expect(asSuggestion(bare.features[0])).toBeNull(); + }); +});