diff --git a/server/src/nest/maps/maps-index.helpers.ts b/server/src/nest/maps/maps-index.helpers.ts index 4b81ed0d1..62b907204 100644 --- a/server/src/nest/maps/maps-index.helpers.ts +++ b/server/src/nest/maps/maps-index.helpers.ts @@ -4,6 +4,7 @@ * need no branch per source. Pure: the calls themselves are trek-places.client's, * the decision to make them is MapsService's. */ +import { getCountryFromCoords } from '../atlas/atlas-geo'; import { buildOsmDetails, parsePoiCategories, samePlace, toApiLang } from './maps.helpers'; import { POI_RESULT_CAP, type PoiSearchResult } from './providers/osm.client'; import { @@ -96,11 +97,11 @@ export function indexPoiPlan(category: string, bbox: PoiBbox, limit: number): In * this branch argues elsewhere that naming a source is a licence obligation. The * wire contract keeps `source` an open string, so widening it costs nothing. * - * One thing the index cannot do is localise. The Overpass path picks - * `name:` and falls back to `int_name`; the service has no language - * parameter at all, so a German user exploring Tokyo gets the Japanese primary - * names here. Named rather than hidden: whoever adds localisation upstream - * should find this comment. + * One thing the index cannot do is translate a name. The Overpass path picks + * `name:` and falls back to `int_name`; the service takes a language now, + * but only puts localities into it, because Overture carries no translated + * names. A German user exploring Tokyo therefore still gets the Japanese + * primary names here, and the POI pins show no locality to translate. */ export function indexPoiAnswer( found: (TrekPlace & { categoryPath?: string | null })[], @@ -195,10 +196,18 @@ export function indexSuggestions(found: TrekSearchHit[], lang?: string): MapsAut // the failure would land after the user had picked it. placeId: osmPlaceId(p), mainText: p.name, - // The layer carries no address. The local name is what the - // place is called on the spot, which is more use under a - // translated label than an empty line. - secondaryText: p.local_name && p.local_name !== p.name ? p.local_name : '', + // The layer carries no address, but asked with a language it + // names the town the place is in, and that reads the way an + // index row does: "Sumida, Japan" (#2592). The country comes + // from the coordinate, which is all the layer has. Without a + // locality the local name is what the place is called on the + // spot, which is more use under a translated label than an + // empty line. + secondaryText: p.locality + ? [p.locality, countryName(getCountryFromCoords(p.lat, p.lng), lang)].filter(Boolean).join(', ') + : p.local_name && p.local_name !== p.name + ? p.local_name + : '', // Per row, because this list is two indexes interleaved. // The name above the list says `trek-places`, which is true // of the call and false of half the rows in it — the layer diff --git a/server/src/nest/maps/maps.service.ts b/server/src/nest/maps/maps.service.ts index 17140c616..afc5942be 100644 --- a/server/src/nest/maps/maps.service.ts +++ b/server/src/nest/maps/maps.service.ts @@ -355,6 +355,7 @@ export class MapsService { lat: locationBias?.lat, lng: locationBias?.lng, limit: 10, + lang, }) .then(indexHitsOnly) .catch((err: unknown) => { @@ -446,7 +447,7 @@ export class MapsService { const keyed = await this.keyedProvider(userId); if (this.trekPlacesEnabled() && !(await this.selector.googleOnly(keyed))) { // Never throws upward, like the search: a slow index drops to the next source. - const found = await trekPlacesNearby(origin.lat, origin.lng, { radius, limit }).catch((err: unknown) => { + const found = await trekPlacesNearby(origin.lat, origin.lng, { radius, limit, lang }).catch((err: unknown) => { console.warn('TREK Places nearby failed, falling back:', (err as Error).message); return []; }); @@ -519,6 +520,7 @@ export class MapsService { lng: centre?.lng, limit: 8, sources: 'index,osm', + lang, }); if (found.length > 0) return { suggestions: indexSuggestions(found, lang), source: 'trek-places' }; } catch (err: unknown) { diff --git a/server/src/nest/maps/place-details.resolver.ts b/server/src/nest/maps/place-details.resolver.ts index 2ec20e89a..83099eef7 100644 --- a/server/src/nest/maps/place-details.resolver.ts +++ b/server/src/nest/maps/place-details.resolver.ts @@ -58,7 +58,9 @@ export class PlaceDetailsResolver { // the index: a place saved while it was on is still opened from what the // trip holds, not looked up again. if (!indexEnabled) return { place: null }; - const found = await trekPlacesById(placeId.slice('gers:'.length)).catch(() => null); + // In the reader's language, so the locality the place is saved with is + // one they can read (#2592). The name stays what the index has. + const found = await trekPlacesById(placeId.slice('gers:'.length), { lang }).catch(() => null); if (!found) return { place: null }; // What the index lacks, OpenStreetMap often has for the same building diff --git a/server/src/nest/maps/trek-places.client.ts b/server/src/nest/maps/trek-places.client.ts index 3edf05c2d..94add9583 100644 --- a/server/src/nest/maps/trek-places.client.ts +++ b/server/src/nest/maps/trek-places.client.ts @@ -12,6 +12,7 @@ * and the service being down, both of which fall back to what TREK did before. */ import { readEnv, getAppUrl } from '../../app-config'; +import { toApiLang } from './maps.helpers'; import { normalizePlaceWebsite } from '@trek/shared'; /** The public instance. An operator may point at their own copy instead. */ @@ -219,6 +220,26 @@ async function getJson( } } +/** + * The reader's language in the form the service takes, or nothing. + * + * Optional on every call that returns places: without it the service answers + * exactly as it did before it knew languages. With it, localities come back in + * the reader's language where OpenStreetMap has one, and the OSM layer's rows + * are named in it too (#2592). TREK's own codes go through toApiLang first + * (`br` is Brazilian Portuguese here and Breton there). Anything that is still + * not a language tag is left off rather than sent: the service refuses it with + * a 422, and a refused keystroke would drop to the fallback for nothing. + * Regional Chinese (`zh-TW`) is passed as it is; the service maps it onto the + * script OpenStreetMap keys by. + */ +const SERVICE_LANG = /^[A-Za-z]{2,3}([_-][A-Za-z0-9]{2,8}){0,2}$/; +export function serviceLang(lang: string | undefined): string | undefined { + if (!lang?.trim()) return undefined; + const tag = toApiLang(lang); + return tag.length <= 20 && SERVICE_LANG.test(tag) ? tag : undefined; +} + async function readCapped(res: Response, max: number): Promise { const reader = res.body?.getReader(); if (!reader) return res.text(); @@ -249,7 +270,9 @@ async function readCapped(res: Response, max: number): Promise { * * It has no address block. OpenStreetMap does not carry one per node, and * inventing an empty one here would let a caller read `address.locality` and - * quietly get nothing. + * quietly get nothing. What it does carry, when the search was made with a + * language, is `locality`: the settlement the place is in, named in that + * language. Without a language the field is absent, as it always was. */ export interface TrekOsmPlace { /** Always `osm:/`, e.g. `osm:node/9712313`. */ @@ -262,6 +285,7 @@ export interface TrekOsmPlace { lng: number; category: string | null; source: 'openstreetmap'; + locality?: string | null; distance_km?: number; } @@ -281,9 +305,20 @@ export function osmPlaceId(hit: TrekOsmPlace): string { return `${hit.osm_type}:${hit.osm_id}`; } +/** + * The layer's `locality` as a name or null. It is the one field of that row + * TREK prints as it came, so a number, an object or a blank from a broken + * upstream must not reach the suggestion line. + */ +function withLocality(hit: TrekSearchHit): TrekSearchHit { + if (!isOsmHit(hit) || hit.locality === undefined) return hit; + const locality = typeof hit.locality === 'string' ? hit.locality.trim() : ''; + return { ...hit, locality: locality || null }; +} + export async function trekPlacesSearch( query: string, - opts: { lat?: number; lng?: number; limit?: number; sources?: string } = {}, + opts: { lat?: number; lng?: number; limit?: number; sources?: string; lang?: string } = {}, ): Promise { const body = await getJson<{ results?: TrekSearchHit[] }>('/v1/search', { q: query, @@ -295,8 +330,9 @@ export async function trekPlacesSearch( // wants: the explicit search path already asks Nominatim in parallel, and // adding the layer there would return the same OpenStreetMap places twice. sources: opts.sources, + lang: serviceLang(opts.lang), }); - return Array.isArray(body.results) ? body.results : []; + return Array.isArray(body.results) ? body.results.map(withLocality) : []; } /** The index-only rows of a mixed result, for callers that need Overture fields. */ @@ -346,7 +382,7 @@ export interface TrekNearbyPlace extends TrekPlace { export async function trekPlacesNearby( lat: number, lng: number, - opts: { radius?: number; limit?: number; category?: string } = {}, + opts: { radius?: number; limit?: number; category?: string; lang?: string } = {}, ): Promise { const body = await getJson<{ results: TrekNearbyPlace[] }>('/v1/nearby', { lat, @@ -354,6 +390,7 @@ export async function trekPlacesNearby( radius: Math.round(opts.radius ?? 1500), limit: opts.limit ?? 50, category: opts.category, + lang: serviceLang(opts.lang), }); return Array.isArray(body.results) ? body.results : []; } @@ -372,6 +409,9 @@ export interface TrekPlacesArea { * are its call, not ours, and a `truncated` answer is passed through honestly * rather than smoothed over. A client that believed it had the whole area would * search offline and quietly miss places. + * + * No language here, on purpose: the offline package is stored once per trip and + * read in whatever language the app is in later, and its contract carries none. */ export async function trekPlacesArea( bbox: { minLat: number; minLng: number; maxLat: number; maxLng: number }, @@ -392,9 +432,11 @@ export async function trekPlacesArea( }; } -export async function trekPlacesById(gers: string): Promise { +export async function trekPlacesById(gers: string, opts: { lang?: string } = {}): Promise { try { - const body = await getJson<{ place: TrekPlace }>(`/v1/place/${encodeURIComponent(gers)}`, {}); + const body = await getJson<{ place: TrekPlace }>(`/v1/place/${encodeURIComponent(gers)}`, { + lang: serviceLang(opts.lang), + }); return body.place ?? null; } catch (err) { if ((err as { status?: number }).status === 404) return null; diff --git a/server/tests/unit/mcp/tools-tags-maps-weather.test.ts b/server/tests/unit/mcp/tools-tags-maps-weather.test.ts index 8148cdfa7..3754dfbc4 100644 --- a/server/tests/unit/mcp/tools-tags-maps-weather.test.ts +++ b/server/tests/unit/mcp/tools-tags-maps-weather.test.ts @@ -445,7 +445,23 @@ describe('Tool: search_nearby_places', () => { expect(data.source).toBe('trek-places'); expect(data.places.map((p: any) => p.name)).toEqual(['Closer', 'Farther']); expect(data.places[0].distance_m).toBe(11); - expect(trekNearbyMock).toHaveBeenCalledWith(48.8606, 2.3376, { radius: 200, limit: 5 }); + expect(trekNearbyMock).toHaveBeenCalledWith(48.8606, 2.3376, { radius: 200, limit: 5, lang: 'en' }); + }); + }); + + it('asks the index in the language the tool was given, as the REST route does (#2592)', async () => { + const { user } = createUser(testDb); + trekNearbyMock.mockClear(); + trekNearbyMock.mockResolvedValueOnce([ + { gers: 'n-1', name: 'Closer', lat: 35.7104, lng: 139.8119, address: null, contact: null }, + ]); + + await withHarness(user.id, async (h) => { + await h.client.callTool({ + name: 'search_nearby_places', + arguments: { lat: 35.7103, lng: 139.8118, radius: 200, limit: 5, lang: 'de' }, + }); + expect(trekNearbyMock).toHaveBeenCalledWith(35.7103, 139.8118, { radius: 200, limit: 5, lang: 'de' }); }); }); diff --git a/server/tests/unit/nest/maps.autocomplete.test.ts b/server/tests/unit/nest/maps.autocomplete.test.ts index 44f2e6811..ec2b78160 100644 --- a/server/tests/unit/nest/maps.autocomplete.test.ts +++ b/server/tests/unit/nest/maps.autocomplete.test.ts @@ -383,4 +383,50 @@ describe('MapsService.autocompletePlaces', () => { 'gers:hasedera', ]); }); + it('MAPS-AUTO-017: the search language goes to the index, which names localities in it (#2592)', async () => { + mockSearch.mockResolvedValue([hit()]); + + await make().autocompletePlaces(1, INPUT, 'br'); + + expect(mockSearch).toHaveBeenCalledWith(INPUT, expect.objectContaining({ sources: 'index,osm', lang: 'br' })); + }); + + it('MAPS-AUTO-018: a layer row reads as its town and country, like an index row (#2592)', async () => { + // What the service answers for "kirby cafe" with a language: the two layer + // rows used to repeat カービィカフェ under "Kirby Cafe" with no place at all. + const tokyo = osmHit({ name: 'Kirby Cafe', local_name: 'カービィカフェ', lat: 35.7103685, lng: 139.8118977 }); + const rows = (locality: string) => [ + { ...tokyo, locality }, + hit({ gers: 'emmetsburg', name: "Kirby's Cafe", lat: 43.11215, lng: -94.69049 }), + ]; + + mockSearch.mockResolvedValue(rows('Sumida')); + expect((await make().autocompletePlaces(1, INPUT, 'en')).suggestions[0].secondaryText).toBe('Sumida, Japan'); + + mockSearch.mockResolvedValue(rows('Sumida')); + expect((await make().autocompletePlaces(1, INPUT, 'fr')).suggestions[0].secondaryText).toBe('Sumida, Japon'); + + mockSearch.mockResolvedValue(rows('Tokio')); + expect((await make().autocompletePlaces(1, INPUT, 'de')).suggestions[0].secondaryText).toBe('Tokio, Japan'); + }); + + it('MAPS-AUTO-019: without a locality the layer row keeps its local name, and never prints a blank', async () => { + mockSearch.mockResolvedValue([ + osmHit({ id: 'osm:node/1', osm_id: 1, locality: null }), + osmHit({ id: 'osm:node/2', osm_id: 2, name: 'Same', local_name: 'Same', lat: 1, lng: 1, locality: null }), + osmHit({ id: 'osm:node/3', osm_id: 3, name: 'Old service', local_name: '旧', lat: 2, lng: 2 }), + ]); + + const { suggestions } = await make().autocompletePlaces(1, INPUT, 'en'); + + expect(suggestions.map((s) => s.secondaryText)).toEqual(['東京駅丸の内駅舎', '', '旧']); + }); + + it('MAPS-AUTO-020: a locality out at sea, where no country is, stands alone', async () => { + mockSearch.mockResolvedValue([osmHit({ lat: 0.5, lng: -30, locality: 'Somewhere' })]); + + const { suggestions } = await make().autocompletePlaces(1, INPUT, 'en'); + + expect(suggestions[0].secondaryText).toBe('Somewhere'); + }); }); diff --git a/server/tests/unit/nest/maps.gers-details.test.ts b/server/tests/unit/nest/maps.gers-details.test.ts index 287f6cd12..fa1d11ff4 100644 --- a/server/tests/unit/nest/maps.gers-details.test.ts +++ b/server/tests/unit/nest/maps.gers-details.test.ts @@ -108,6 +108,11 @@ describe('MapsService.getPlaceDetails for a gers: id', () => { expect(out.place).toMatchObject({ name: "L'Osteria", osm_id: 'gers:abc-123', source: 'trek-places' }); }); + it('MAPS-GERS-011: the place is read in the language of the request, so its locality is too (#2592)', async () => { + await make(null).getPlaceDetails(1, 'gers:abc-123', 'ja'); + expect(mockById).toHaveBeenCalledWith('abc-123', { lang: 'ja' }); + }); + it('MAPS-GERS-006: an unknown id is null rather than a thrown request', async () => { mockById.mockResolvedValue(null); expect(await make(null).getPlaceDetails(1, 'gers:nope')).toEqual({ place: null }); diff --git a/server/tests/unit/nest/maps.nearby.test.ts b/server/tests/unit/nest/maps.nearby.test.ts index 74b7904b2..eb255c93b 100644 --- a/server/tests/unit/nest/maps.nearby.test.ts +++ b/server/tests/unit/nest/maps.nearby.test.ts @@ -108,7 +108,9 @@ describe('MapsService.nearbyPlaces', () => { const out = await make({ google: true }).nearbyPlaces(1, p.lat, p.lng, { radius: 800, limit: 5 }); - expect(mockNearby).toHaveBeenCalledWith(p.lat, p.lng, { radius: 800, limit: 5 }); + // The language of the answer, English when none was asked for, the same + // default Google and OpenStreetMap get below (#2592). + expect(mockNearby).toHaveBeenCalledWith(p.lat, p.lng, { radius: 800, limit: 5, lang: 'en' }); expect(out.source).toBe('trek-places'); expect(out.places.map((r) => r.name)).toEqual(['Near Cafe', 'Far Cafe']); expect(out.places[0].distance_m).toBe(56); @@ -171,6 +173,15 @@ describe('MapsService.nearbyPlaces', () => { ); }); + it('MAPS-NEARBY-011: the index names the localities in the language asked for (#2592)', async () => { + const p = nextPoint(); + mockNearby.mockResolvedValue([indexRow('near', 'Near Cafe', p.lat + 0.0005, p.lng)]); + + await make().nearbyPlaces(1, p.lat, p.lng, { lang: 'br' }); + + expect(mockNearby).toHaveBeenCalledWith(p.lat, p.lng, expect.objectContaining({ lang: 'pt-BR' })); + }); + it('MAPS-NEARBY-004: without a key OpenStreetMap answers, in the asked language, nearest first', async () => { const p = nextPoint(); const fetchMock = vi.fn().mockResolvedValue( diff --git a/server/tests/unit/nest/maps.search-merge.test.ts b/server/tests/unit/nest/maps.search-merge.test.ts index 9008d5594..23ddf32fd 100644 --- a/server/tests/unit/nest/maps.search-merge.test.ts +++ b/server/tests/unit/nest/maps.search-merge.test.ts @@ -129,6 +129,14 @@ describe('MapsService.searchPlaces — the merged path', () => { expect(out.places[1].name).toBe('長谷寺'); }); + it('MAPS-SEARCH-014: the index is asked in the language of the search, like OpenStreetMap is (#2592)', async () => { + mockSearch.mockResolvedValue([indexHit('Kirby Cafe Hakata')]); + + await make().searchPlaces(1, 'kirby cafe', 'de'); + + expect(mockSearch).toHaveBeenCalledWith('kirby cafe', expect.objectContaining({ lang: 'de' })); + }); + it('MAPS-SEARCH-002: an index answer with nothing from OSM is named as index only', async () => { mockSearch.mockResolvedValue([indexHit("L'Osteria")]); mockNominatim.mockResolvedValue(osmAnswer([])); @@ -311,7 +319,7 @@ describe('MapsService.searchPlaces — the merged path', () => { await make().searchPlaces(1, "L'Osteria", 'de', { lat: 54.0879, lng: 12.1408, radius: 5000 }); - expect(mockSearch).toHaveBeenCalledWith("L'Osteria", { lat: 54.0879, lng: 12.1408, limit: 10 }); + expect(mockSearch).toHaveBeenCalledWith("L'Osteria", { lat: 54.0879, lng: 12.1408, limit: 10, lang: 'de' }); // Without the viewbox, "Hase-dera" answers with the temple in Nara rather // than the one the user is standing beside in Kamakura. const params = mockNominatim.mock.calls[0][1] as URLSearchParams; diff --git a/server/tests/unit/nest/trek-places.client.test.ts b/server/tests/unit/nest/trek-places.client.test.ts index 2318d3bf6..949599996 100644 --- a/server/tests/unit/nest/trek-places.client.test.ts +++ b/server/tests/unit/nest/trek-places.client.test.ts @@ -15,6 +15,7 @@ import { trekPlacesNearby, trekPlacesSearch, resetTrekPlacesBreaker, + serviceLang, type TrekPlace, } from '../../../src/nest/maps/trek-places.client'; @@ -263,6 +264,68 @@ describe('trekPlacesSearch', () => { }); }); +describe('the reader language (#2592)', () => { + it('sends TREK codes as the tags the service reads, and nothing it would refuse', () => { + expect(serviceLang('de')).toBe('de'); + // Brazilian Portuguese and Greek have codes of their own in TREK. + expect(serviceLang('br')).toBe('pt-BR'); + expect(serviceLang('gr')).toBe('el'); + // Passed as it is: the service maps a region onto the script OSM keys by. + expect(serviceLang('zh-TW')).toBe('zh-TW'); + expect(serviceLang(undefined)).toBeUndefined(); + expect(serviceLang(' ')).toBeUndefined(); + expect(serviceLang('not a locale!')).toBeUndefined(); + }); + + it('asks search, nearby and the place lookup in it', async () => { + stubFetch({ query: 'x', results: [], place: PLACE }); + await trekPlacesSearch('x', { lang: 'br' }); + await trekPlacesNearby(54.1, 12.1, { lang: 'gr' }); + await trekPlacesById('abc-123', { lang: 'ja' }); + expect(calls.map((c) => new URL(c.url).searchParams.get('lang'))).toEqual(['pt-BR', 'el', 'ja']); + }); + + it('leaves the parameter off without a language, so the answer is the one it always was', async () => { + stubFetch({ query: 'x', results: [], place: PLACE, count: 0 }); + await trekPlacesSearch('x'); + await trekPlacesNearby(54.1, 12.1); + await trekPlacesById('abc-123'); + await trekPlacesSearch('x', { lang: 'not a locale!' }); + await trekPlacesArea(BOX); + expect(calls.every((c) => !new URL(c.url).searchParams.has('lang'))).toBe(true); + }); + + it('keeps a layer row locality only when it is a name', async () => { + const osm = (locality: unknown) => ({ + id: 'osm:node/1', + osm_type: 'node', + osm_id: 1, + name: 'Kirby Cafe', + local_name: 'カービィカフェ', + lat: 35.71, + lng: 139.81, + category: null, + source: 'openstreetmap', + locality, + }); + stubFetch({ query: 'x', results: [osm(' Sumida '), osm(''), osm(42), osm(null), { ...PLACE }] }); + + const found = await trekPlacesSearch('x', { lang: 'en', sources: 'index,osm' }); + + expect(found.map((h) => ('locality' in h ? h.locality : 'index row'))).toEqual([ + 'Sumida', + null, + null, + null, + 'index row', + ]); + // A row the service sent without the field, which is every row asked + // without a language, is left without it. + stubFetch({ query: 'x', results: [{ ...osm(null), locality: undefined }] }); + expect((await trekPlacesSearch('x'))[0]).not.toHaveProperty('locality'); + }); +}); + describe('reading the body', () => { // One buffer, handed out more than once: two of them are already over the cap. const BIG = new Uint8Array(600_000);