diff --git a/server/src/nest/maps/maps.helpers.ts b/server/src/nest/maps/maps.helpers.ts index d4b6f7bae..dffbf5427 100644 --- a/server/src/nest/maps/maps.helpers.ts +++ b/server/src/nest/maps/maps.helpers.ts @@ -129,17 +129,29 @@ export function namesOverlap(a: string, b: string): boolean { } const GOOGLE_FTID_RE = /^0x[0-9a-f]+:0x[0-9a-f]+$/i; +// The same id inside the `data=` pathname blob, where a resolved share link keeps it: +// /maps/place//data=!4m6!3m5!1s0x47e66e1f06e2b70f:0x40b82c3688c9460!8m2!3d48.85!4d2.29 +const GOOGLE_FTID_DATA_RE = /!1s(0x[0-9a-f]{1,20}:0x[0-9a-f]{1,20})/i; -// Extracts a Google Maps feature id (ftid, 0x..:0x..) from a URL's ?ftid= param. +// Extracts a Google Maps feature id (ftid, 0x..:0x..) from a URL. +// Two shapes carry it. The `?ftid=` query parameter is the old one. A share link that +// resolveGoogleMapsUrl has followed to its final hop carries it instead in the `data=` +// pathname blob, in the same `!1s…!3d…!4d…` run the coordinates come from (#1954) — so +// reading only the query parameter dropped the id for every pasted maps.app.goo.gl link. // The Places API (New) googleMapsUri is usually a cid-style URL (https://maps.google.com/?cid=NNN) -// with no ftid, so this returns null for most API responses — the precise query_place_id link is -// used instead. It does recover an ftid from a /place/?...&ftid= URL, e.g. a pasted share link -// resolved by resolveGoogleMapsUrl or a Google MyMaps list import. +// with neither, so this still returns null for most API responses — the precise +// query_place_id link is used instead. export function googleFtidFromMapsUrl(url?: string | null): string | null { if (!url) return null; try { - const ftid = new URL(url).searchParams.get('ftid')?.trim(); - return ftid && GOOGLE_FTID_RE.test(ftid) ? ftid.toLowerCase() : null; + const parsed = new URL(url); + const ftid = parsed.searchParams.get('ftid')?.trim(); + if (ftid && GOOGLE_FTID_RE.test(ftid)) return ftid.toLowerCase(); + // Only for a place URL. A /maps/dir/ route carries one !1s per waypoint and the + // first one is the origin, not the place the link is about. + if (!parsed.pathname.includes('/place/')) return null; + const fromPath = GOOGLE_FTID_DATA_RE.exec(parsed.pathname)?.[1]; + return fromPath ? fromPath.toLowerCase() : null; } catch { return null; } diff --git a/server/src/nest/maps/maps.service.ts b/server/src/nest/maps/maps.service.ts index 4657174f0..97a495576 100644 --- a/server/src/nest/maps/maps.service.ts +++ b/server/src/nest/maps/maps.service.ts @@ -1974,6 +1974,7 @@ export class MapsService { lat: string, lng: string, lang?: string, + opts?: { lane?: GeoLane; timeoutMs?: number }, ): Promise<{ name: string | null; address: string | null }> { const params = new URLSearchParams({ lat, @@ -1983,7 +1984,7 @@ export class MapsService { zoom: '18', 'accept-language': toApiLang(lang), }); - const response = await nominatimFetch('reverse', params); + const response = await nominatimFetch('reverse', params, opts); if (!response.ok) return { name: null, address: null }; const data = (await response.json()) as { name?: string; display_name?: string; address?: Record }; const addr = data.address || {}; diff --git a/server/src/nest/places/places.helpers.ts b/server/src/nest/places/places.helpers.ts index e55f66124..9168d233d 100644 --- a/server/src/nest/places/places.helpers.ts +++ b/server/src/nest/places/places.helpers.ts @@ -254,6 +254,12 @@ export const SEARCH_BIAS_RADIUS_METERS = 2000; /** Concurrent enrichment lookups — small, to stay friendly to the Maps quota. */ export const ENRICH_CONCURRENCY = 3; +// The free address backfill is one Nominatim request per place on the throttled +// background lane (~1/s), so a normal list costs seconds. Past this it stops being +// a backfill and starts being bulk geocoding, which Nominatim's usage policy asks +// people not to do — so it stops rather than queueing for an hour. +export const ADDRESS_BACKFILL_MAX_PLACES = 250; + /** * Pick the search result that is the same place as the import: it must be a diff --git a/server/src/nest/places/places.service.ts b/server/src/nest/places/places.service.ts index 552422c2e..f73b1ee2e 100644 --- a/server/src/nest/places/places.service.ts +++ b/server/src/nest/places/places.service.ts @@ -29,6 +29,7 @@ import { JourneyDomainService } from '../journey/journey-domain.service'; import { COORD_DEDUP_TOLERANCE, ENRICH_CONCURRENCY, + ADDRESS_BACKFILL_MAX_PLACES, escapeLikePattern, MAX_LIST_RESPONSE_BYTES, googleMapsFeatureIdFromItem, @@ -983,8 +984,8 @@ export class PlacesService { } }); - if (opts?.enrich && opts.userId && created.length) { - void this.enrichImportedPlaces(tripId, opts.userId, created as EnrichablePlace[], opts.lang); + if (created.length) { + void this.enrichImportedList(tripId, created as EnrichablePlace[], opts); } return { places: created, listName, skipped }; @@ -1126,8 +1127,8 @@ export class PlacesService { } }); - if (opts?.enrich && opts.userId && created.length) { - void this.enrichImportedPlaces(tripId, opts.userId, created as EnrichablePlace[], opts.lang); + if (created.length) { + void this.enrichImportedList(tripId, created as EnrichablePlace[], opts); } return { places: created, listName, skipped }; @@ -1226,6 +1227,63 @@ export class PlacesService { } } + /** + * Everything a just-imported list gets in the background, in one place so the + * Google and Naver call sites cannot drift apart. + * + * The Google pass stays exactly as opt-in and key-gated as before. The address + * backfill behind it needs neither — it is the same free Nominatim reverse + * lookup a single pasted link has always had, which is what made a list import + * come out thinner than the same place added one at a time (#1954). + */ + private async enrichImportedList(tripId: string, places: EnrichablePlace[], opts?: ListImportOptions): Promise { + if (opts?.enrich && opts.userId) { + await this.enrichImportedPlaces(tripId, opts.userId, places, opts.lang); + } + await this.backfillMissingAddresses(tripId, places, opts?.lang); + } + + /** + * Fill in the address of imported places that have none, from Nominatim. + * + * Runs on the background lane so it never queues in front of a user's own + * search, writes through COALESCE so it can only fill an empty column — the + * Google pass above may already have written a better one — and pushes each + * row over the websocket so the sidebar fills in without a reload. Never + * throws: one bad lookup must not take down a detached task. + */ + async backfillMissingAddresses(tripId: string, places: EnrichablePlace[], lang?: string): Promise { + try { + const pending = places.filter(p => !p.address && p.lat != null && p.lng != null); + if (!pending.length) return; + if (pending.length > ADDRESS_BACKFILL_MAX_PLACES) { + console.warn(`[Places] address backfill skipped for trip ${tripId}: ${pending.length} places exceeds the ${ADDRESS_BACKFILL_MAX_PLACES} cap`); + return; + } + // Serial on purpose: the background lane throttles to roughly one request a + // second anyway, so concurrency would only build a queue. + for (const place of pending) { + try { + const { address } = await this.maps.reverseGeocode(String(place.lat), String(place.lng), lang, { + lane: 'background', + timeoutMs: 10000, + }); + if (!address) continue; + this.dbs.run( + 'UPDATE places SET address = COALESCE(address, ?), updated_at = CURRENT_TIMESTAMP WHERE id = ? AND trip_id = ?', + address, place.id, tripId, + ); + const updated = this.dbs.getPlaceWithTags(place.id); + if (updated) this.realtime.broadcast(tripId, 'place:updated', { place: updated }, undefined); + } catch (err) { + console.error(`[Places] address backfill failed for place ${place.id}:`, err instanceof Error ? err.message : err); + } + } + } catch (err) { + console.error('[Places] address backfill pass failed:', err instanceof Error ? err.message : err); + } + } + // ------------------------------------------------------------------------- // Search place image (Unsplash) // ------------------------------------------------------------------------- diff --git a/server/tests/unit/nest/maps.service.test.ts b/server/tests/unit/nest/maps.service.test.ts index 2161148b9..93bfb7e02 100644 --- a/server/tests/unit/nest/maps.service.test.ts +++ b/server/tests/unit/nest/maps.service.test.ts @@ -2266,6 +2266,46 @@ describe('googleFtidFromMapsUrl', () => { expect(googleFtidFromMapsUrl('not a url')).toBeNull(); expect(googleFtidFromMapsUrl(null)).toBeNull(); }); + + // #1954 — a followed maps.app.goo.gl link keeps the id in the `data=` blob, not + // in a query parameter, which is why a pasted single link used to lose it. + it('MAPS-FTID-004: extracts the ftid from the data= path blob of a resolved share link', () => { + expect( + googleFtidFromMapsUrl( + 'https://www.google.com/maps/place/Notre-Dame/data=!4m6!3m5!1s0x47e671e23a09b3b1:0x40b82c3688b2f60!8m2!3d48.8529!4d2.3499', + ), + ).toBe('0x47e671e23a09b3b1:0x40b82c3688b2f60'); + }); + + it('MAPS-FTID-005: the query parameter still wins over the path blob', () => { + expect( + googleFtidFromMapsUrl( + 'https://www.google.com/maps/place/X/data=!3m5!1s0x1:0x2!8m2?ftid=0x882bf179e806d471:0x8591dde29c821a93', + ), + ).toBe('0x882bf179e806d471:0x8591dde29c821a93'); + }); + + it('MAPS-FTID-006: ignores the path blob outside a /place/ URL', () => { + // A directions link carries one !1s per waypoint and the first is the origin, + // so reading it would attach the wrong place. + expect( + googleFtidFromMapsUrl('https://www.google.com/maps/dir/A/B/data=!4m2!1s0x47e671e23a09b3b1:0x40b82c3688b2f60'), + ).toBeNull(); + }); + + it('MAPS-FTID-007: a place URL without either shape is still null, and case is normalised', () => { + expect(googleFtidFromMapsUrl('https://www.google.com/maps/place/Notre-Dame/@48.85,2.34,17z')).toBeNull(); + expect( + googleFtidFromMapsUrl('https://www.google.com/maps/place/X/data=!1s0x47E671E23A09B3B1:0x40B82C3688B2F60'), + ).toBe('0x47e671e23a09b3b1:0x40b82c3688b2f60'); + }); + + it('MAPS-FTID-008: a long hostile path blob resolves in linear time (ReDoS budget)', () => { + const hostile = `https://www.google.com/maps/place/X/data=${'!1s0x'.repeat(20000)}`; + const started = Date.now(); + expect(googleFtidFromMapsUrl(hostile)).toBeNull(); + expect(Date.now() - started).toBeLessThan(500); + }); }); // ── buildUserAgent (instance-specific UA, #1309) ────────────────────────────── diff --git a/server/tests/unit/nest/places.service.test.ts b/server/tests/unit/nest/places.service.test.ts index 615135716..4b5a50303 100644 --- a/server/tests/unit/nest/places.service.test.ts +++ b/server/tests/unit/nest/places.service.test.ts @@ -14,6 +14,7 @@ import type { PlacePhotoCacheService } from '../../../src/nest/place-photos/plac import { UnsplashService } from '../../../src/nest/unsplash/unsplash.service'; import { RuntimeEnvService } from '../../../src/nest/app-config/runtime-env.service'; import { TRACK_COLORS } from '@trek/shared'; +import { ADDRESS_BACKFILL_MAX_PLACES } from '../../../src/nest/places/places.helpers'; // ── DB setup ────────────────────────────────────────────────────────────────── @@ -1468,3 +1469,100 @@ describe('importNaverList provider payload', () => { expect(result.places[0].name).toBe('Gyeongbokgung'); }); }); + +// ── Free address backfill for list imports (#1954) ─────────────────────────── +// +// A place pasted as a single Google Maps link has always been reverse geocoded; +// the same place arriving through a list import was not, so it stayed without an +// address forever. These cover the backfill that closes that gap. + +describe('backfillMissingAddresses', () => { + function backfillSvc(reverseGeocode: MapsService['reverseGeocode']) { + return makePlacesService({ reverseGeocode } as unknown as MapsService); + } + + it('PLACE-SVC-078 — fills the address of a place that has none', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const place = createPlace(testDb, trip.id, { name: 'Bar', lat: 48.85, lng: 2.35 }) as any; + + const reverseGeocode = vi.fn(async () => ({ name: null, address: '1 Rue de Rivoli, Paris' })); + await backfillSvc(reverseGeocode).backfillMissingAddresses(String(trip.id), [ + { id: place.id, name: 'Bar', lat: 48.85, lng: 2.35 }, + ]); + + expect(reverseGeocode).toHaveBeenCalledWith('48.85', '2.35', undefined, { lane: 'background', timeoutMs: 10000 }); + const row = testDb.prepare('SELECT address FROM places WHERE id = ?').get(place.id) as { address: string }; + expect(row.address).toBe('1 Rue de Rivoli, Paris'); + }); + + it('PLACE-SVC-079 — never overwrites an address the import or the Google pass already wrote', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const place = createPlace(testDb, trip.id, { name: 'Bar', lat: 48.85, lng: 2.35 }) as any; + testDb.prepare('UPDATE places SET address = ? WHERE id = ?').run('Imported address', place.id); + + const reverseGeocode = vi.fn(async () => ({ name: null, address: 'Nominatim address' })); + await backfillSvc(reverseGeocode).backfillMissingAddresses(String(trip.id), [ + { id: place.id, name: 'Bar', lat: 48.85, lng: 2.35, address: 'Imported address' }, + ]); + + expect(reverseGeocode).not.toHaveBeenCalled(); + const row = testDb.prepare('SELECT address FROM places WHERE id = ?').get(place.id) as { address: string }; + expect(row.address).toBe('Imported address'); + }); + + it('PLACE-SVC-080 — a lookup that answers with nothing leaves the row untouched', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const place = createPlace(testDb, trip.id, { name: 'Bar', lat: 48.85, lng: 2.35 }) as any; + + await backfillSvc(vi.fn(async () => ({ name: null, address: null }))).backfillMissingAddresses(String(trip.id), [ + { id: place.id, name: 'Bar', lat: 48.85, lng: 2.35 }, + ]); + + const row = testDb.prepare('SELECT address FROM places WHERE id = ?').get(place.id) as { address: string | null }; + expect(row.address).toBeNull(); + }); + + it('PLACE-SVC-081 — one failing lookup does not take down the rest of the batch', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const first = createPlace(testDb, trip.id, { name: 'A', lat: 1, lng: 2 }) as any; + const second = createPlace(testDb, trip.id, { name: 'B', lat: 3, lng: 4 }) as any; + + const reverseGeocode = vi.fn() + .mockRejectedValueOnce(new Error('nominatim down')) + .mockResolvedValueOnce({ name: null, address: 'Second address' }); + + await expect( + backfillSvc(reverseGeocode as unknown as MapsService['reverseGeocode']).backfillMissingAddresses(String(trip.id), [ + { id: first.id, name: 'A', lat: 1, lng: 2 }, + { id: second.id, name: 'B', lat: 3, lng: 4 }, + ]), + ).resolves.toBeUndefined(); + + const rows = testDb.prepare('SELECT id, address FROM places WHERE trip_id = ? ORDER BY id').all(trip.id) as { + id: number; address: string | null; + }[]; + expect(rows[0].address).toBeNull(); + expect(rows[1].address).toBe('Second address'); + }); + + it('PLACE-SVC-082 — an oversized batch is refused rather than queued for an hour', async () => { + const reverseGeocode = vi.fn(); + const batch = Array.from({ length: ADDRESS_BACKFILL_MAX_PLACES + 1 }, (_, i) => ({ + id: i + 1, name: `P${i}`, lat: 1, lng: 2, + })); + await backfillSvc(reverseGeocode as unknown as MapsService['reverseGeocode']).backfillMissingAddresses('1', batch); + expect(reverseGeocode).not.toHaveBeenCalled(); + }); + + it('PLACE-SVC-083 — a place without coordinates is skipped', async () => { + const reverseGeocode = vi.fn(); + await backfillSvc(reverseGeocode as unknown as MapsService['reverseGeocode']).backfillMissingAddresses('1', [ + { id: 1, name: 'A', lat: null as unknown as number, lng: null as unknown as number }, + ]); + expect(reverseGeocode).not.toHaveBeenCalled(); + }); +});