diff --git a/server/src/nest/maps/maps.helpers.ts b/server/src/nest/maps/maps.helpers.ts index 035371cc4..1dfdafe7f 100644 --- a/server/src/nest/maps/maps.helpers.ts +++ b/server/src/nest/maps/maps.helpers.ts @@ -580,12 +580,18 @@ export function parseWikipediaTag(tag: string | undefined | null): { lang: strin // Ids that can never resolve against the Google Places API: coordinate pseudo-ids // (right-click places, in both the coords: and the bare "lat,lng" spelling the // collection views send), OSM ids the client sends when a place has no -// google_place_id, the raw photo URLs legacy rows keep in image_url, and photo -// cache keys of the form "~p3" that enrichment mints for the picker. +// google_place_id, GERS ids from the TREK index, the raw photo URLs legacy rows +// keep in image_url, and photo cache keys of the form "~p3" that +// enrichment mints for the picker. // Google answers those with a billable 400 INVALID_ARGUMENT, so every lookup // sorts them out before the call and uses the OSM/Wikimedia path instead. +// +// `gers:` belongs here for the same reason as `node:`, and it matters more: the +// index answers first for search and autocomplete, so a GERS id is what an +// ordinary new place now carries. Leaving it out billed three invalid lookups +// per place — photo refs, editorial summary, and the photo route. const NON_GOOGLE_PLACE_ID = - /^(?:coords|node|way|relation):|^https?:\/\/|^-?\d+(?:\.\d+)?,\s*-?\d+(?:\.\d+)?$|~p\d+$/i; + /^(?:coords|gers|node|way|relation):|^https?:\/\/|^-?\d+(?:\.\d+)?,\s*-?\d+(?:\.\d+)?$|~p\d+$/i; // The subset that still has a provider behind it — Overpass for details, // Wikimedia for photos. export const OSM_PLACE_ID = /^(?:node|way|relation):/i; diff --git a/server/src/nest/maps/maps.service.ts b/server/src/nest/maps/maps.service.ts index 216f08ccf..a9d0ec070 100644 --- a/server/src/nest/maps/maps.service.ts +++ b/server/src/nest/maps/maps.service.ts @@ -1625,6 +1625,7 @@ export class MapsService { query: string, lang?: string, locationBias?: { lat: number; lng: number; radius?: number }, + opts: { googleIdentityOnly?: boolean } = {}, ): Promise<{ places: Record[]; source: string }> { const { key: apiKey, source: keySource } = this.resolveMapsKey(userId); @@ -1636,7 +1637,17 @@ export class MapsService { // It never throws upward. A search that used to work must keep working when // the service is slow or down, so a failure drops through to exactly what // this method did before. - if (this.trekPlacesEnabled()) { + // + // `googleIdentityOnly` skips it. One caller does not want the best answer, + // it wants a Google id: list-import enrichment (#886) exists to attach a + // `google_place_id` to a bare imported pin, and `pickEnrichmentMatch` + // discards every candidate that has none. Index and OpenStreetMap records + // both carry `google_place_id: null`, so answering that caller from them + // returns matches it must throw away, and the import silently stays + // unenriched on an instance that pays for a key. It changes nothing for an + // instance without one: enrichment could never resolve anything there + // either, before this branch existed or after. + if (this.trekPlacesEnabled() && !(opts.googleIdentityOnly && apiKey)) { try { // Both at once. The index is a dataset of businesses and is very good // at those; OpenStreetMap is where the temples, bridges, riverside diff --git a/server/src/nest/maps/trek-places.client.ts b/server/src/nest/maps/trek-places.client.ts index 88b9a6fbf..6273df6ec 100644 --- a/server/src/nest/maps/trek-places.client.ts +++ b/server/src/nest/maps/trek-places.client.ts @@ -26,6 +26,22 @@ const TIMEOUT_MS = 3500; /** A search answer is a few kB. A megabyte means something is wrong upstream. */ const MAX_BYTES = 1_000_000; +/** + * The area download is the one call that is neither short nor interactive. + * + * It runs from the offline prefetch, asks for thousands of rows at once, and + * nobody is watching a spinner while it does. Holding it to the search budget + * capped a Rostock-sized city at the megabyte the prefetch was measured to need + * and threw, which the caller could only report as "index unavailable" — the + * trip then went offline with an empty place cache and nothing said so. + * + * Budgeted per row rather than as a flat number so the ceiling follows the + * limit the caller actually asked for, with a floor for small boxes. + */ +const AREA_TIMEOUT_MS = 20_000; +const AREA_BYTES_PER_ROW = 1_200; +const AREA_MIN_BYTES = 2_000_000; + export interface TrekPlace { gers: string; name: string; @@ -86,14 +102,18 @@ export function trekPlacesBaseUrl(): string { return (configured || DEFAULT_TREK_PLACES_URL).replace(/\/+$/, ''); } -async function getJson(path: string, params: Record): Promise { +async function getJson( + path: string, + params: Record, + opts: { maxBytes?: number; timeoutMs?: number } = {}, +): Promise { const url = new URL(trekPlacesBaseUrl() + path); for (const [k, v] of Object.entries(params)) { if (v !== undefined && v !== null && v !== '') url.searchParams.set(k, String(v)); } const controller = new AbortController(); - const timer = setTimeout(() => controller.abort(), TIMEOUT_MS); + const timer = setTimeout(() => controller.abort(), opts.timeoutMs ?? TIMEOUT_MS); try { const res = await fetch(url, { signal: controller.signal, @@ -109,7 +129,7 @@ async function getJson(path: string, params: Record { - const body = await getJson('/v1/bbox', { ...bbox, limit }); + const body = await getJson( + '/v1/bbox', + { ...bbox, limit }, + { + maxBytes: Math.max(AREA_MIN_BYTES, limit * AREA_BYTES_PER_ROW), + timeoutMs: AREA_TIMEOUT_MS, + }, + ); return { count: Number(body.count) || 0, truncated: !!body.truncated, diff --git a/server/src/nest/places/places.service.ts b/server/src/nest/places/places.service.ts index 6a3d23516..ba66db01d 100644 --- a/server/src/nest/places/places.service.ts +++ b/server/src/nest/places/places.service.ts @@ -1226,11 +1226,18 @@ export class PlacesService { if (place.google_place_id) return; if (typeof place.lat !== 'number' || typeof place.lng !== 'number') return; - const { places: results } = await this.maps.searchPlaces(userId, place.name, lang, { - lat: place.lat, - lng: place.lng, - radius: SEARCH_BIAS_RADIUS_METERS, - }); + // Asked for a Google identity rather than for the best answer: the whole + // point here is the `google_place_id` that `pickEnrichmentMatch` selects on, + // and the TREK index and OpenStreetMap have none to give. Without this the + // search returns index records, every candidate is discarded for a missing + // id, and the import quietly stays unenriched on an instance with a key. + const { places: results } = await this.maps.searchPlaces( + userId, + place.name, + lang, + { lat: place.lat, lng: place.lng, radius: SEARCH_BIAS_RADIUS_METERS }, + { googleIdentityOnly: true }, + ); const match = pickEnrichmentMatch(results, { lat: place.lat, lng: place.lng }); if (!match) return; diff --git a/server/tests/unit/nest/maps.autocomplete.test.ts b/server/tests/unit/nest/maps.autocomplete.test.ts new file mode 100644 index 000000000..c3974cb29 --- /dev/null +++ b/server/tests/unit/nest/maps.autocomplete.test.ts @@ -0,0 +1,142 @@ +/** + * MAPS-AUTO-001..008 — 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 + * whole TREK fleet shares one User-Agent there — so before the index, every + * install was one abusive neighbour away from being blocked. What matters here + * is that an index answer is shaped the way the details lookup can read back + * (`gers:` ids), and that nothing about a slow or missing index leaves somebody + * typing into a box that never answers. + */ +import { describe, it, expect, vi, beforeEach } from 'vitest'; + +const { mockSearch } = vi.hoisted(() => ({ + mockSearch: vi.fn( + async (_query: string, _opts?: { lat?: number; lng?: number; limit?: number }): Promise => [], + ), +})); +vi.mock('../../../src/nest/maps/trek-places.client', async (importOriginal) => ({ + ...(await importOriginal()), + trekPlacesSearch: mockSearch, +})); + +vi.mock('../../../src/config', () => ({ JWT_SECRET: 'test-secret', ENCRYPTION_KEY: '0'.repeat(64) })); + +import { MapsService } from '../../../src/nest/maps/maps.service'; +import type { DatabaseService } from '../../../src/nest/database/database.service'; +import type { PlacePhotoCacheService } from '../../../src/nest/place-photos/place-photo-cache.service'; + +const INPUT = 'Café Kröpel'; + +const hit = (over: Record = {}) => ({ + gers: 'abc-123', + name: 'Café Kröpeliner', + lat: 54.0879, + lng: 12.1408, + address: { locality: 'Rostock', country: 'DE' }, + ...over, +}); + +/** + * `enabled` drives the admin kill switch, which is read straight off + * app_settings; an unset row reads as on, because the switch is fail-open. + * + * Keyed on the statement rather than answering everything the same way: the + * same `get` also resolves the Google key, and a blanket answer would hand + * `'false'` to the key resolver and send the fallback at Google for real. + */ +function make(enabled = true) { + const database = { + get: vi.fn((sql: string) => + sql.includes('trek_places_enabled') && !enabled ? { value: 'false' } : undefined, + ), + } as unknown as DatabaseService; + return new MapsService(database, {} as PlacePhotoCacheService); +} + +beforeEach(() => { + mockSearch.mockReset(); + mockSearch.mockResolvedValue([]); + vi.spyOn(console, 'warn').mockImplementation(() => {}); +}); + +describe('MapsService.autocompletePlaces', () => { + it('MAPS-AUTO-001: index hits answer the box, tagged so the details lookup can read them back', async () => { + mockSearch.mockResolvedValue([hit(), hit({ gers: 'def-456', name: 'Café Central' })]); + + const result = await make().autocompletePlaces(1, INPUT); + + expect(result.source).toBe('trek-places'); + expect(result.suggestions).toHaveLength(2); + // The `gers:` prefix is the contract with getPlaceDetails, which branches on + // it. A suggestion without it lands in the generic colon branch and asks + // Overpass for an OSM element that does not exist, so the place comes back + // empty after the user has already picked it. + expect(result.suggestions[0].placeId).toBe('gers:abc-123'); + expect(result.suggestions[1].placeId).toBe('gers:def-456'); + expect(result.suggestions[0].mainText).toBe('Café Kröpeliner'); + expect(result.suggestions[0].secondaryText).toBe('Rostock, DE'); + }); + + it('MAPS-AUTO-002: a place with no locality gets an empty line, not the word "undefined"', async () => { + mockSearch.mockResolvedValue([hit({ address: null }), hit({ gers: 'x', address: { locality: 'Rostock' } })]); + + const { suggestions } = await make().autocompletePlaces(1, INPUT); + + expect(suggestions[0].secondaryText).toBe(''); + // One half present is still one half: no stray comma either. + expect(suggestions[1].secondaryText).toBe('Rostock'); + }); + + it('MAPS-AUTO-003: a location bias is sent as the centre of the box the client drew', async () => { + mockSearch.mockResolvedValue([hit()]); + + await make().autocompletePlaces(1, INPUT, undefined, { + low: { lat: 54, lng: 12 }, + high: { lat: 54.2, lng: 12.4 }, + }); + + // Eight, not ten: the suggestion list is what a person reads while typing. + expect(mockSearch).toHaveBeenCalledWith(INPUT, { lat: 54.1, lng: 12.2, limit: 8 }); + }); + + it('MAPS-AUTO-004: without a bias the index is asked without coordinates rather than with zeroes', async () => { + mockSearch.mockResolvedValue([hit()]); + + await make().autocompletePlaces(1, INPUT); + + // A 0/0 bias is a point in the Atlantic, and the index treats a bias as + // permission to relax the match — which is how the shops around a landmark + // start outranking the landmark. + expect(mockSearch).toHaveBeenCalledWith(INPUT, { lat: undefined, lng: undefined, limit: 8 }); + }); + + it('MAPS-AUTO-005: an index that found nothing falls through instead of answering empty', async () => { + mockSearch.mockResolvedValue([]); + + const result = await make().autocompletePlaces(1, INPUT); + + expect(mockSearch).toHaveBeenCalledTimes(1); + // Whatever the fallback answers, it must not claim to be the index. + expect(result.source).not.toBe('trek-places'); + }); + + it('MAPS-AUTO-006: an index failure is a warning and a fallback, not a dead search box', async () => { + mockSearch.mockRejectedValue(new Error('places api down')); + + const result = await make().autocompletePlaces(1, INPUT); + + expect(console.warn).toHaveBeenCalledWith('TREK Places autocomplete failed, falling back:', 'places api down'); + expect(result.source).not.toBe('trek-places'); + }); + + it('MAPS-AUTO-007: the keystroke never leaves for the index while the admin has it off', async () => { + mockSearch.mockResolvedValue([hit()]); + + const result = await make(false).autocompletePlaces(1, INPUT); + + expect(mockSearch).not.toHaveBeenCalled(); + expect(result.source).not.toBe('trek-places'); + }); +}); diff --git a/server/tests/unit/nest/maps.gers-details.test.ts b/server/tests/unit/nest/maps.gers-details.test.ts index a5f4670f8..dfa2e1277 100644 --- a/server/tests/unit/nest/maps.gers-details.test.ts +++ b/server/tests/unit/nest/maps.gers-details.test.ts @@ -97,4 +97,26 @@ describe('MapsService.getPlaceDetails for a gers: id', () => { mockById.mockRejectedValue(new Error('places api down')); expect(await make(null).getPlaceDetails(1, 'gers:abc-123')).toEqual({ place: null }); }); + + it('MAPS-GERS-008: Overpass being down costs the OSM half, not the whole place', async () => { + // The index has already answered by the time OSM is asked; the second lookup + // only adds what OSM knows about the same building. Letting its failure + // through would turn a working answer into an error for the one user whose + // details request happened to land while Overpass was unreachable. + const database = { get: vi.fn(() => undefined) } as unknown as DatabaseService; + const svc = new MapsService(database, {} as PlacePhotoCacheService); + vi.spyOn(svc, 'resolveOsmIdentity').mockRejectedValue(new Error('overpass down')); + mockById.mockResolvedValue({ ...PLACE, hours: { osm: 'Mo-Su 12:00-22:00' } }); + + const out = await svc.getPlaceDetails(1, 'gers:abc-123'); + + expect(out.place).toMatchObject({ + name: "L'Osteria", + osm_id: 'gers:abc-123', + source: 'trek-places', + website: 'https://losteria.net/', + }); + // The index's own hours survive: they came with the record, not from OSM. + expect(out.place?.opening_hours).toBeTruthy(); + }); }); diff --git a/server/tests/unit/nest/maps.service.test.ts b/server/tests/unit/nest/maps.service.test.ts index 394af1af4..4c43cd1e2 100644 --- a/server/tests/unit/nest/maps.service.test.ts +++ b/server/tests/unit/nest/maps.service.test.ts @@ -2348,6 +2348,22 @@ describe('isGooglePlaceId', () => { // The collection views send the bare coordinate pair when a place has no ids. expect(isGooglePlaceId('36.7617499,-3.8448432')).toBe(false); }); + + it('MAPS-045b: rejects the TREK index ids that search and autocomplete now hand out', () => { + // The index answers before Google, so `gers:` is what an ordinary new place + // carries. Sending one to Google costs a billable 400 INVALID_ARGUMENT and can + // never return anything, and three separate call sites gate on this function: + // photo refs, the editorial summary, and the place-photo route. + expect(isGooglePlaceId('gers:08f2ab12345678901234567890abcd')).toBe(false); + expect(isGooglePlaceId('gers:abc-123')).toBe(false); + // Case-insensitive, like every other prefix in the list. + expect(isGooglePlaceId('GERS:abc-123')).toBe(false); + // The photo-picker cache key keeps its suffix meaning on top of the prefix. + expect(isGooglePlaceId('gers:abc-123~p3')).toBe(false); + // Guard against over-matching: a Google id that merely starts with those + // letters is still a Google id, because the prefix only counts before a colon. + expect(isGooglePlaceId('gersChIJLU7jZClu5kcR')).toBe(true); + }); }); describe('googleFtidFromMapsUrl', () => { diff --git a/server/tests/unit/nest/trek-places.client.test.ts b/server/tests/unit/nest/trek-places.client.test.ts index dc466b8cc..32a992d35 100644 --- a/server/tests/unit/nest/trek-places.client.test.ts +++ b/server/tests/unit/nest/trek-places.client.test.ts @@ -213,6 +213,86 @@ describe('reading the body', () => { await expect(trekPlacesSearch('x')).rejects.toThrow('TREK Places API response too large'); expect(streamCancelled).toBe(true); }); + + it('gives the area download a budget that follows the rows it asked for', async () => { + // The search cap is a megabyte because a search answer is a few kB. The area + // download is the one call that is neither: it asks for thousands of rows at + // once for the offline cache, and a city-sized answer sits right on that + // megabyte. Holding it to the search budget threw, the caller could only + // report "index unavailable", and the trip went offline with nothing cached. + // A real answer, just a big one: 1200 rows of a padded name puts the body + // comfortably past the megabyte the search path would refuse. + const pad = 'x'.repeat(1000); + const rows = Array.from({ length: 1200 }, (_, i) => ({ ...PLACE, gers: `row-${i}`, name: pad })); + const payload = JSON.stringify({ count: rows.length, truncated: false, results: rows }); + expect(payload.length).toBeGreaterThan(1_000_000); + stubStreamingFetch([encoder.encode(payload)]); + const area = await trekPlacesArea(BOX, 3000); + expect(area.results).toHaveLength(1200); + expect(streamCancelled).toBe(false); + }); + + it('still caps the area download, at the budget its own limit buys', async () => { + // Not unbounded — a hostile or broken upstream must not be able to hand the + // instance an endless body just because the path is the bulk one. + stubStreamingFetch(Array.from({ length: 8 }, () => BIG)); + await expect(trekPlacesArea(BOX, 1)).rejects.toThrow('TREK Places API response too large'); + expect(streamCancelled).toBe(true); + }); +}); + +describe('getJson request shape', () => { + it('carries an abort signal on every call, and aborts when the answer never comes', async () => { + // The repo rule is that no outbound fetch runs without a timeout. Nothing + // asserted the signal before, so removing it would not have failed a test. + vi.useFakeTimers(); + try { + calls = []; + vi.stubGlobal('fetch', vi.fn((url: URL | string, init?: RequestInit) => { + calls.push({ url: String(url), init }); + // Answers only when the caller gives up. + return new Promise((_resolve, reject) => { + init?.signal?.addEventListener('abort', () => reject(new Error('aborted'))); + }) as Promise; + })); + + const pending = trekPlacesSearch('never answered'); + const failed = expect(pending).rejects.toThrow('aborted'); + expect(calls[0].init?.signal).toBeInstanceOf(AbortSignal); + expect(calls[0].init?.signal?.aborted).toBe(false); + await vi.advanceTimersByTimeAsync(3500); + expect(calls[0].init?.signal?.aborted).toBe(true); + await failed; + } finally { + vi.useRealTimers(); + } + }); + + it('lets the area download run longer than a keystroke would', async () => { + // The search timeout sits in front of a keystroke. The offline prefetch is a + // background bulk download that nobody is watching, and 3.5 s is not enough + // to move thousands of rows — it would abort mid-body and cache nothing. + vi.useFakeTimers(); + try { + calls = []; + vi.stubGlobal('fetch', vi.fn((url: URL | string, init?: RequestInit) => { + calls.push({ url: String(url), init }); + return new Promise((_resolve, reject) => { + init?.signal?.addEventListener('abort', () => reject(new Error('aborted'))); + }) as Promise; + })); + + const pending = trekPlacesArea(BOX, 3000); + const failed = expect(pending).rejects.toThrow('aborted'); + await vi.advanceTimersByTimeAsync(3500); + expect(calls[0].init?.signal?.aborted).toBe(false); + await vi.advanceTimersByTimeAsync(20_000); + expect(calls[0].init?.signal?.aborted).toBe(true); + await failed; + } finally { + vi.useRealTimers(); + } + }); }); describe('trekPlacesById', () => {