fix(maps): keep index ids away from Google, and let the area download be big

Four things the index changed and nothing followed:

`gers:` was missing from NON_GOOGLE_PLACE_ID, so isGooglePlaceId said true
for every id the index hands out. Five call sites gate on that function to
keep unresolvable ids away from Google, which answers them with a billable
400 INVALID_ARGUMENT: photo refs, the editorial summary, the place-photo
route, the expanded details lookup and the enrichment photo pass. Since the
index answers first for search and autocomplete, a GERS id is what an
ordinary new place now carries, so this was the normal case rather than an
edge one — three billed lookups per added place, none of which could ever
return anything.

The area download shared the search path's megabyte cap and its 3.5 s
timeout. Both are right for a keystroke and wrong for the offline prefetch,
which asks for 3000 rows at once: a city-sized answer sits on that megabyte,
readCapped threw, and the caller could only report "index unavailable" — the
trip then went offline with an empty place cache and nothing said so. The cap
now follows the row count the caller asked for, with a floor, and the bulk
call gets a budget that fits a bulk download. It is still capped, so a broken
upstream cannot hand us an endless body.

searchPlaces returned before the Google branch as soon as the index or
OpenStreetMap had anything, and both set google_place_id to null. That is
right for a search and wrong for the one caller that wants an identity rather
than an answer: list-import enrichment (#886) exists to attach a
google_place_id to a bare imported pin, and pickEnrichmentMatch discards every
candidate without one. So the import silently stayed unenriched on an instance
that pays for a key. That caller now asks for the Google identity explicitly.
Nothing changes for an instance without a key — enrichment could never resolve
anything there, before or after.

Tests cover the three functions this branch had left untested, which is what
put the maps ratchet under its functions threshold: the autocomplete index
path (a new file, MAPS-AUTO-001..007), the gers details branch when Overpass
is down (MAPS-GERS-008), and the client's abort timeout — including that the
signal exists at all, which nothing asserted before.
This commit is contained in:
Maurice
2026-09-07 10:51:15 +02:00
committed by Maurice
parent 2b7af6267b
commit 273bac8917
8 changed files with 324 additions and 13 deletions
+9 -3
View File
@@ -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 "<placeId>~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 "<placeId>~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;
+12 -1
View File
@@ -1625,6 +1625,7 @@ export class MapsService {
query: string,
lang?: string,
locationBias?: { lat: number; lng: number; radius?: number },
opts: { googleIdentityOnly?: boolean } = {},
): Promise<{ places: Record<string, unknown>[]; 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
+31 -4
View File
@@ -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<T>(path: string, params: Record<string, string | number | undefined>): Promise<T> {
async function getJson<T>(
path: string,
params: Record<string, string | number | undefined>,
opts: { maxBytes?: number; timeoutMs?: number } = {},
): Promise<T> {
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<T>(path: string, params: Record<string, string | number |
}
// Read with a cap rather than res.json(): a hostile or broken upstream
// should not be able to hand us an unbounded body to buffer.
const text = await readCapped(res, MAX_BYTES);
const text = await readCapped(res, opts.maxBytes ?? MAX_BYTES);
return JSON.parse(text) as T;
} finally {
clearTimeout(timer);
@@ -212,7 +232,14 @@ export async function trekPlacesArea(
bbox: { minLat: number; minLng: number; maxLat: number; maxLng: number },
limit = 2000,
): Promise<TrekPlacesArea> {
const body = await getJson<TrekPlacesArea>('/v1/bbox', { ...bbox, limit });
const body = await getJson<TrekPlacesArea>(
'/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,
+12 -5
View File
@@ -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;
@@ -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<unknown> => [],
),
}));
vi.mock('../../../src/nest/maps/trek-places.client', async (importOriginal) => ({
...(await importOriginal<typeof import('../../../src/nest/maps/trek-places.client')>()),
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<string, unknown> = {}) => ({
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');
});
});
@@ -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();
});
});
@@ -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', () => {
@@ -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<Response>;
}));
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<Response>;
}));
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', () => {