diff --git a/server/src/nest/places/places.helpers.ts b/server/src/nest/places/places.helpers.ts index 532a4616d..d9b23e3c2 100644 --- a/server/src/nest/places/places.helpers.ts +++ b/server/src/nest/places/places.helpers.ts @@ -1,5 +1,11 @@ import { XMLParser } from 'fast-xml-parser'; import unzipper from 'unzipper'; +import { + externalIdsOf, + normalizePlaceName, + placeMatchStrategies, + type PlaceMatchCandidate, +} from '@trek/shared'; import type { Place } from '../../types'; import type { PlaceWithTags } from '../database/database.service'; import type { KmlImportSummary } from './kml-import.helpers'; @@ -35,7 +41,9 @@ export { gpxParser, kmlParser }; export const KMZ_DECOMPRESSED_SIZE_LIMIT = 50 * 1024 * 1024; // 50 MB -export const COORD_DEDUP_TOLERANCE = 0.0001; // ≈ 11 m +// Re-exported so the importers that already read these from here keep working, +// while the values themselves live in @trek/shared with the rule that uses them. +export { COORD_DEDUP_TOLERANCE, externalIdsOf } from '@trek/shared'; /** Cap on a provider list-import response body — the payload is attacker-influenced via the list id. */ export const MAX_LIST_RESPONSE_BYTES = 8 * 1024 * 1024; // 8 MB @@ -122,70 +130,38 @@ export interface DedupSet { externalIds: Set; } -/** The provider ids a candidate can be recognised by, ignoring blanks. */ -export function externalIdsOf(candidate: { - google_place_id?: string | null; - google_ftid?: string | null; - osm_id?: string | null; -}): string[] { - return [candidate.google_place_id, candidate.google_ftid, candidate.osm_id] - .filter((id): id is string => typeof id === 'string' && id.trim() !== '') - .map(id => id.trim()); -} - /** * Returns true if a candidate place is already represented in the dedup set. * - * The provider id comes first, because it is the only part of a place that - * survives the user editing it. Re-importing a Google Maps list used to duplicate - * every place someone had renamed (#1550): the name no longer matched, and the - * coordinate fallback never ran for a named candidate. The ids were already being - * stored, and even backfilled onto matched places, they were simply never read. - * - * Name and coordinates keep their previous roles below it, deliberately unchanged: - * widening the coordinate check to named places would merge the restaurant and the - * bar in the same building, which is a worse failure than the one being fixed. + * The in-memory half of the matching rule; `PlacesService.findMatchingPlaceId` is + * the SQL half. Both walk the same strategy list from @trek/shared, so neither can + * drift into recognising a place the other would not — which is exactly what had + * happened: the SQL copy reached for coordinates on a named candidate, and this + * one deliberately never does (see place-match.ts for why). */ -export function isPlaceDuplicate( - candidate: { - name: string | null | undefined; - lat: number | null; - lng: number | null; - google_place_id?: string | null; - google_ftid?: string | null; - osm_id?: string | null; - }, - dedup: DedupSet, -): boolean { - for (const id of externalIdsOf(candidate)) { - if (dedup.externalIds.has(id)) return true; - } - const normalizedName = candidate.name?.trim().toLowerCase(); - if (normalizedName) return dedup.names.has(normalizedName); - if (candidate.lat != null && candidate.lng != null) { - return dedup.coords.some( - (c) => - Math.abs(c.lat - candidate.lat!) <= COORD_DEDUP_TOLERANCE && - Math.abs(c.lng - candidate.lng!) <= COORD_DEDUP_TOLERANCE, - ); +export function isPlaceDuplicate(candidate: PlaceMatchCandidate, dedup: DedupSet): boolean { + for (const strategy of placeMatchStrategies(candidate)) { + if (strategy.by === 'externalId') { + if (dedup.externalIds.has(strategy.id)) return true; + } else if (strategy.by === 'name') { + if (dedup.names.has(strategy.name)) return true; + } else if ( + dedup.coords.some( + (c) => + Math.abs(c.lat - strategy.lat) <= strategy.tolerance && + Math.abs(c.lng - strategy.lng) <= strategy.tolerance, + ) + ) { + return true; + } } return false; } /** Record a newly inserted place so subsequent candidates in the same batch are checked against it. */ -export function trackInsertedInDedupSet( - place: { - name: string | null | undefined; - lat: number | null; - lng: number | null; - google_place_id?: string | null; - google_ftid?: string | null; - osm_id?: string | null; - }, - dedup: DedupSet, -): void { +export function trackInsertedInDedupSet(place: PlaceMatchCandidate, dedup: DedupSet): void { for (const id of externalIdsOf(place)) dedup.externalIds.add(id); - const normalizedName = place.name?.trim().toLowerCase(); + const normalizedName = normalizePlaceName(place.name); if (normalizedName) { dedup.names.add(normalizedName); } else if (place.lat != null && place.lng != null) { diff --git a/server/src/nest/places/places.service.ts b/server/src/nest/places/places.service.ts index eb8e3b25b..69c071310 100644 --- a/server/src/nest/places/places.service.ts +++ b/server/src/nest/places/places.service.ts @@ -1,6 +1,6 @@ import { Injectable } from '@nestjs/common'; import { XMLValidator } from 'fast-xml-parser'; -import { TRACK_COLORS } from '@trek/shared'; +import { TRACK_COLORS, placeMatchStrategies, type PlaceMatchCandidate } from '@trek/shared'; import type { TrekWsPayload, TrekWsTripEventName } from '@trek/shared'; import { RealtimeService } from '../realtime/realtime.service'; import { DatabaseService, type TripAccess } from '../database/database.service'; @@ -28,7 +28,6 @@ import { reclaimPlaceImage } from './place-image'; import { JourneyDomainService } from '../journey/journey-domain.service'; import { StorageService } from '../storage/storage.service'; import { - COORD_DEDUP_TOLERANCE, ENRICH_CONCURRENCY, ADDRESS_BACKFILL_MAX_PLACES, escapeLikePattern, @@ -515,36 +514,49 @@ export class PlacesService { return { names, coords, externalIds }; } + /** + * The id of the place on this trip that `candidate` already is, or null. + * + * The public door onto the matching rule, for an importer that has to LINK to + * the existing place rather than merely skip the candidate — the booking + * importer needs the id so a hotel booking points at the hotel it already + * created. `findDuplicatePlace` stays private behind it: it also hands back + * `google_ftid` for the bulk importer's backfill, which is a detail of that + * caller and not part of the question "which place is this?". + */ + findMatchingPlaceId(tripId: string, candidate: PlaceMatchCandidate): number | null { + return this.findDuplicatePlace(tripId, candidate)?.id ?? null; + } + + /** + * Walks the shared match strategies (`@trek/shared`, place-match.ts) against + * the trip's rows, in order, first hit wins. The strategy list is the rule — + * notably it offers coordinates only for an unnamed candidate, so this can no + * longer disagree with `isPlaceDuplicate` about the restaurant and the bar at + * the same address. + */ private findDuplicatePlace( tripId: string, - place: { - name: string | null | undefined; lat: number | null; lng: number | null; - google_place_id?: string | null; google_ftid?: string | null; osm_id?: string | null; - }, + place: PlaceMatchCandidate, ): { id: number; google_ftid: string | null } | null { - // Same order as isPlaceDuplicate: the provider id wins, because it is what - // survives a rename (#1550). - for (const id of externalIdsOf(place)) { - const byId = this.dbs.get<{ id: number; google_ftid: string | null }>(` + for (const strategy of placeMatchStrategies(place)) { + let hit: { id: number; google_ftid: string | null } | undefined; + if (strategy.by === 'externalId') { + hit = this.dbs.get<{ id: number; google_ftid: string | null }>(` SELECT id, google_ftid FROM places WHERE trip_id = ? AND (google_place_id = ? OR google_ftid = ? OR osm_id = ?) ORDER BY id ASC LIMIT 1 - `, tripId, id, id, id); - if (byId) return byId; - } - const normalizedName = place.name?.trim().toLowerCase(); - if (normalizedName) { - const duplicate = this.dbs.get<{ id: number; google_ftid: string | null }>(` + `, tripId, strategy.id, strategy.id, strategy.id); + } else if (strategy.by === 'name') { + hit = this.dbs.get<{ id: number; google_ftid: string | null }>(` SELECT id, google_ftid FROM places WHERE trip_id = ? AND lower(trim(name)) = ? ORDER BY id ASC LIMIT 1 - `, tripId, normalizedName); - if (duplicate) return duplicate; - } - if (place.lat != null && place.lng != null) { - return this.dbs.get<{ id: number; google_ftid: string | null }>(` + `, tripId, strategy.name); + } else { + hit = this.dbs.get<{ id: number; google_ftid: string | null }>(` SELECT id, google_ftid FROM places WHERE trip_id = ? AND lat IS NOT NULL AND lng IS NOT NULL @@ -552,7 +564,9 @@ export class PlacesService { AND abs(lng - ?) <= ? ORDER BY id ASC LIMIT 1 - `, tripId, place.lat, COORD_DEDUP_TOLERANCE, place.lng, COORD_DEDUP_TOLERANCE) || null; + `, tripId, strategy.lat, strategy.tolerance, strategy.lng, strategy.tolerance); + } + if (hit) return hit; } return null; } diff --git a/server/tests/unit/nest/places.service.test.ts b/server/tests/unit/nest/places.service.test.ts index 6663f6d1c..c6fecb25a 100644 --- a/server/tests/unit/nest/places.service.test.ts +++ b/server/tests/unit/nest/places.service.test.ts @@ -13,7 +13,7 @@ import { describe, it, expect, vi, beforeAll, beforeEach, afterEach, afterAll } import type { PlacePhotoCacheService } from '../../../src/nest/place-photos/place-photo-cache.service'; 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 { TRACK_COLORS, COORD_DEDUP_TOLERANCE } from '@trek/shared'; import { ADDRESS_BACKFILL_MAX_PLACES } from '../../../src/nest/places/places.helpers'; // ── DB setup ────────────────────────────────────────────────────────────────── @@ -1571,3 +1571,76 @@ describe('backfillMissingAddresses', () => { expect(reverseGeocode).not.toHaveBeenCalled(); }); }); + +// ── findMatchingPlaceId ─────────────────────────────────────────────────────── + +/** + * The public door onto the place-matching rule, for importers that need the + * matched row's id so they can link to it rather than merely knowing a duplicate + * exists. The rule itself lives in @trek/shared (place-match.ts); these cases pin + * that this service interprets it faithfully against real SQL. + */ +describe('findMatchingPlaceId', () => { + it('PLACES-SVC-008 — matches an existing place by name, ignoring case and surrounding space', () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const place = createPlace(testDb, trip.id, { name: 'Eiffel Tower' }); + + expect(svc.findMatchingPlaceId(String(trip.id), { name: ' eiffel tower ' })).toBe(place.id); + }); + + it('PLACES-SVC-009 — matches on a provider id even after the place was renamed (#1550)', () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const place = createPlace(testDb, trip.id, { name: 'Original Name' }); + testDb.prepare('UPDATE places SET google_place_id = ? WHERE id = ?').run('ChIJ_abc', place.id); + + expect( + svc.findMatchingPlaceId(String(trip.id), { name: 'Renamed By User', google_place_id: 'ChIJ_abc' }), + ).toBe(place.id); + }); + + it('PLACES-SVC-010 — does NOT match a NAMED candidate to a different place at the same coordinates', () => { + // The restaurant and the bar in the same building are two places. This is the + // rule isPlaceDuplicate has always applied; the SQL copy used to disagree. + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + createPlace(testDb, trip.id, { name: 'Ground Floor Diner', lat: 52.52, lng: 13.405 }); + + expect( + svc.findMatchingPlaceId(String(trip.id), { name: 'Rooftop Bar', lat: 52.52, lng: 13.405 }), + ).toBeNull(); + }); + + it('PLACES-SVC-011 — matches an UNNAMED candidate by coordinates within tolerance', () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const place = createPlace(testDb, trip.id, { name: 'Anything', lat: 48.85, lng: 2.35 }); + + expect( + svc.findMatchingPlaceId(String(trip.id), { + name: null, + lat: 48.85 + COORD_DEDUP_TOLERANCE / 2, + lng: 2.35, + }), + ).toBe(place.id); + }); + + it('PLACES-SVC-012 — returns null when nothing recognises the candidate', () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + createPlace(testDb, trip.id, { name: 'Somewhere Else' }); + + expect(svc.findMatchingPlaceId(String(trip.id), { name: 'Unseen Place' })).toBeNull(); + expect(svc.findMatchingPlaceId(String(trip.id), { name: null, lat: null, lng: null })).toBeNull(); + }); + + it('PLACES-SVC-013 — never matches a place belonging to another trip', () => { + const { user } = createUser(testDb); + const mine = createTrip(testDb, user.id); + const theirs = createTrip(testDb, user.id); + createPlace(testDb, theirs.id, { name: 'Shared Name' }); + + expect(svc.findMatchingPlaceId(String(mine.id), { name: 'Shared Name' })).toBeNull(); + }); +}); diff --git a/shared/src/index.ts b/shared/src/index.ts index 6ae29d239..249c864eb 100644 --- a/shared/src/index.ts +++ b/shared/src/index.ts @@ -35,6 +35,7 @@ export * from './day/day.schema'; export * from './day/note-colors'; export * from './assignment/assignment.schema'; export * from './place/place.schema'; +export * from './place/place-match'; export * from './place/track-colors'; export * from './collection/collection.schema'; export * from './trip/trip.schema'; diff --git a/shared/src/place/place-match.spec.ts b/shared/src/place/place-match.spec.ts new file mode 100644 index 000000000..6edb9a0f7 --- /dev/null +++ b/shared/src/place/place-match.spec.ts @@ -0,0 +1,74 @@ +import { COORD_DEDUP_TOLERANCE, externalIdsOf, normalizePlaceName, placeMatchStrategies } from './place-match'; + +import { describe, it, expect } from 'vitest'; + +describe('normalizePlaceName', () => { + it('trims and lowercases', () => { + expect(normalizePlaceName(' Eiffel Tower ')).toBe('eiffel tower'); + }); + + it('treats blank, whitespace, null and undefined alike as no name', () => { + expect(normalizePlaceName('')).toBeNull(); + expect(normalizePlaceName(' ')).toBeNull(); + expect(normalizePlaceName(null)).toBeNull(); + expect(normalizePlaceName(undefined)).toBeNull(); + }); +}); + +describe('externalIdsOf', () => { + it('collects the provider ids that carry a value, trimmed', () => { + expect(externalIdsOf({ google_place_id: ' ChIJ_a ', google_ftid: '0x1:0x2', osm_id: null })).toEqual([ + 'ChIJ_a', + '0x1:0x2', + ]); + }); + + it('ignores blank ids', () => { + expect(externalIdsOf({ google_ftid: ' ', osm_id: '' })).toEqual([]); + }); + + it('keeps provider order: place_id, ftid, osm', () => { + expect(externalIdsOf({ osm_id: 'node/42', google_ftid: 'f', google_place_id: 'p' })).toEqual(['p', 'f', 'node/42']); + }); +}); + +describe('placeMatchStrategies', () => { + it('puts every provider id first, one strategy each, in provider order', () => { + expect(placeMatchStrategies({ name: 'Trattoria', google_place_id: 'p', osm_id: 'node/42' })).toEqual([ + { by: 'externalId', id: 'p' }, + { by: 'externalId', id: 'node/42' }, + { by: 'name', name: 'trattoria' }, + ]); + }); + + it('never offers coordinates for a NAMED candidate, even when it has them', () => { + // The whole point of the rule: widening the coordinate check to named places + // would merge the restaurant and the bar in the same building. + const strategies = placeMatchStrategies({ name: 'Ground Floor Diner', lat: 52.52, lng: 13.405 }); + + expect(strategies).toEqual([{ by: 'name', name: 'ground floor diner' }]); + expect(strategies.some((s) => s.by === 'coords')).toBe(false); + }); + + it('falls back to coordinates only when there is no name', () => { + expect(placeMatchStrategies({ name: null, lat: 48.85, lng: 2.35 })).toEqual([ + { by: 'coords', lat: 48.85, lng: 2.35, tolerance: COORD_DEDUP_TOLERANCE }, + ]); + }); + + it('offers provider ids then coordinates for an unnamed candidate that has both', () => { + expect(placeMatchStrategies({ name: ' ', google_ftid: 'f', lat: 1, lng: 2 })).toEqual([ + { by: 'externalId', id: 'f' }, + { by: 'coords', lat: 1, lng: 2, tolerance: COORD_DEDUP_TOLERANCE }, + ]); + }); + + it('offers nothing for a candidate with no name, no ids and no coordinates', () => { + expect(placeMatchStrategies({ name: null, lat: null, lng: null })).toEqual([]); + }); + + it('needs both halves of a coordinate pair', () => { + expect(placeMatchStrategies({ name: null, lat: 48.85, lng: null })).toEqual([]); + expect(placeMatchStrategies({ name: null, lat: null, lng: 2.35 })).toEqual([]); + }); +}); diff --git a/shared/src/place/place-match.ts b/shared/src/place/place-match.ts new file mode 100644 index 000000000..e81086513 --- /dev/null +++ b/shared/src/place/place-match.ts @@ -0,0 +1,84 @@ +/** + * When two places are the same place — the single source of the rule. + * + * There are two consumers with nothing in common but this decision: the bulk + * place importers ask it of an in-memory set they already built for the trip + * (`isPlaceDuplicate`), and the booking importer asks it of the database and + * needs the matched row's id so it can link to it (`findMatchingPlaceId`). Both + * used to spell the rule out themselves, in the same order, and the SQL copy had + * quietly drifted: it reached for coordinates on a NAMED candidate, which the + * in-memory copy deliberately never does. + * + * So the order lives here as data, and each consumer is a mechanical interpreter + * of it. Adding a way to recognise a place means adding a strategy here once. + * + * The order, and why: + * + * 1. Provider id — the only part of a place that survives the user editing it. + * Re-importing a Google Maps list used to duplicate every place someone had + * renamed (#1550). One strategy per id rather than one for all of them, so + * an earlier provider still wins when two ids point at different rows. + * 2. Name — exact after trim + lowercase. + * 3. Coordinates — **only when there is no name.** Widening the coordinate + * check to named places would merge the restaurant and the bar at the same + * address, which is a worse failure than the duplicate it would prevent. + */ + +/** ≈ 11 m. Two coordinate-only places inside this box are treated as one place. */ +export const COORD_DEDUP_TOLERANCE = 0.0001; + +/** The fields a place can be recognised by. Both a parsed candidate and a stored row fit. */ +export interface PlaceMatchCandidate { + name?: string | null; + lat?: number | null; + lng?: number | null; + google_place_id?: string | null; + google_ftid?: string | null; + osm_id?: string | null; +} + +/** One way to look for an existing place, to be tried in order. */ +export type PlaceMatchStrategy = + | { by: 'externalId'; id: string } + | { by: 'name'; name: string } + | { by: 'coords'; lat: number; lng: number; tolerance: number }; + +/** Trim + lowercase, or null when there is no usable name. */ +export function normalizePlaceName(name: string | null | undefined): string | null { + const trimmed = typeof name === 'string' ? name.trim() : ''; + return trimmed === '' ? null : trimmed.toLowerCase(); +} + +/** The provider ids a candidate carries, trimmed, in provider order, blanks dropped. */ +export function externalIdsOf(candidate: PlaceMatchCandidate): string[] { + return [candidate.google_place_id, candidate.google_ftid, candidate.osm_id] + .filter((id): id is string => typeof id === 'string' && id.trim() !== '') + .map((id) => id.trim()); +} + +/** + * How to look for `candidate`, most reliable first. An empty list means the + * candidate carries nothing to recognise it by, so it cannot match anything. + */ +export function placeMatchStrategies(candidate: PlaceMatchCandidate): PlaceMatchStrategy[] { + const strategies: PlaceMatchStrategy[] = externalIdsOf(candidate).map((id) => ({ + by: 'externalId' as const, + id, + })); + + const name = normalizePlaceName(candidate.name); + if (name) { + strategies.push({ by: 'name', name }); + return strategies; + } + + if (candidate.lat != null && candidate.lng != null) { + strategies.push({ + by: 'coords', + lat: candidate.lat, + lng: candidate.lng, + tolerance: COORD_DEDUP_TOLERANCE, + }); + } + return strategies; +}