diff --git a/server/src/nest/places/places.helpers.ts b/server/src/nest/places/places.helpers.ts index d9b23e3c2..0a3043f00 100644 --- a/server/src/nest/places/places.helpers.ts +++ b/server/src/nest/places/places.helpers.ts @@ -135,9 +135,13 @@ export interface DedupSet { * * 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 + * reach for a KIND of match the other would not — which is exactly what had + * happened: the SQL copy fell back to coordinates on a named candidate, and this * one deliberately never does (see place-match.ts for why). + * + * Shared order, not shared comparison: SQLite `lower()` is ASCII-only where + * JavaScript's is not, and only unnamed rows contribute coordinates to a dedup + * set. `findDuplicatePlace` spells out where the two still answer differently. */ export function isPlaceDuplicate(candidate: PlaceMatchCandidate, dedup: DedupSet): boolean { for (const strategy of placeMatchStrategies(candidate)) { diff --git a/server/src/nest/places/places.service.ts b/server/src/nest/places/places.service.ts index 69c071310..6a3d23516 100644 --- a/server/src/nest/places/places.service.ts +++ b/server/src/nest/places/places.service.ts @@ -534,6 +534,23 @@ export class PlacesService { * 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. + * + * What is shared is the ORDER, not the comparison. Two differences remain, both + * of them older than this method and neither worth widening its scope for: + * + * - The name is matched with SQLite `lower()`, which is ASCII-only, against a + * parameter `normalizePlaceName` lowercased in JavaScript, which is not. A + * row stored as `CAFÉ CENTRAL` therefore does not match the candidate + * `Café Central` here, while `isPlaceDuplicate` does match it in memory. + * Before the strategies, the coordinate fallback quietly covered that gap + * for a named candidate — sometimes with the wrong row, since it matched on + * position alone. The cost today is a `google_ftid` backfill that does not + * happen; the benefit is that it can no longer happen to a different place. + * - `buildDedupSet` collects coordinates only for UNNAMED rows, while the + * coordinate query below considers every row. An unnamed candidate can + * therefore match a named row here and not there. That is the behaviour + * `findMatchingPlaceId` wants — a booking with no place name should link to + * the hotel that has one — so it is stated rather than removed. */ private findDuplicatePlace( tripId: string, @@ -1013,8 +1030,14 @@ export class PlacesService { let skipped = 0; this.dbs.transaction(() => { for (const p of places) { - if (isPlaceDuplicate({ name: p.name, lat: p.lat, lng: p.lng, google_ftid: p.googleFtid }, dedup)) { - const duplicate = this.findDuplicatePlace(tripId, p); + // One candidate for both halves. Passing the raw parser object to the SQL + // half used to mean its provider id never arrived — the field is + // `googleFtid` there and `google_ftid` here — so the id strategy was + // always empty and the name could match a different row, which then took + // this candidate's ftid on the backfill below. + const candidate = { name: p.name, lat: p.lat, lng: p.lng, google_ftid: p.googleFtid }; + if (isPlaceDuplicate(candidate, dedup)) { + const duplicate = this.findDuplicatePlace(tripId, candidate); if (duplicate && !duplicate.google_ftid && p.googleFtid) { updateGoogleFtidStmt.run(p.googleFtid, duplicate.id); } @@ -1024,7 +1047,7 @@ export class PlacesService { const result = insertStmt.run(tripId, p.name, p.lat, p.lng, p.notes, p.googleFtid); const place = this.dbs.getPlaceWithTags(Number(result.lastInsertRowid))!; created.push(place); - trackInsertedInDedupSet({ name: p.name, lat: p.lat, lng: p.lng, google_ftid: p.googleFtid }, dedup); + trackInsertedInDedupSet(candidate, dedup); } }); diff --git a/server/tests/unit/nest/places.service.test.ts b/server/tests/unit/nest/places.service.test.ts index c6fecb25a..b2d04c61d 100644 --- a/server/tests/unit/nest/places.service.test.ts +++ b/server/tests/unit/nest/places.service.test.ts @@ -741,6 +741,43 @@ describe('importGoogleList', () => { expect(row.google_ftid).toBe('0x882bf179e806d471:0x8591dde29c821a93'); }); + it('PLACE-SVC-028e — the backfill lands on the row the provider id names, not on a namesake', async () => { + // The importer's parsed item spells the id `googleFtid`, the match rule reads + // `google_ftid`, so the raw object used to reach findDuplicatePlace with no id + // at all. The name then decided, and a second place sharing the name took the + // ftid that belonged to the renamed one. + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const renamed = createPlace(testDb, trip.id, { + name: 'Saturday market run', + lat: 43.5118527, + lng: -80.5542617, + }) as any; + testDb.prepare('UPDATE places SET google_ftid = ? WHERE id = ?') + .run('0x882bf179e806d471:0x8591dde29c821a93', renamed.id); + const namesake = createPlace(testDb, trip.id, { + name: "St. Jacobs Farmers' Market", + lat: 40.0, + lng: -80.0, + }) as any; + + const listPayload = [ + [null, null, null, null, 'My Test List', null, null, null, [ + [null, [null, null, null, null, '878 Weber St N', [null, null, 43.5118527, -80.5542617], ['-8634542354666695567', '-8822026229683971437']], "St. Jacobs Farmers' Market"], + ]], + ]; + vi.stubGlobal('fetch', vi.fn().mockResolvedValue({ + ok: true, + text: async () => 'prefix\n' + JSON.stringify(listPayload), + })); + + const result = await svc.importGoogleList(String(trip.id), 'https://www.google.com/maps/placelists/list/ABC123DEF456') as any; + const other = testDb.prepare('SELECT google_ftid FROM places WHERE id = ?').get(namesake.id) as any; + + expect(result.skipped).toBe(1); + expect(other.google_ftid).toBeNull(); + }); + it('PLACE-SVC-028d — a renamed place is not re-imported as a twin (#1550)', async () => { const { user } = createUser(testDb); const trip = createTrip(testDb, user.id); @@ -1643,4 +1680,66 @@ describe('findMatchingPlaceId', () => { expect(svc.findMatchingPlaceId(String(mine.id), { name: 'Shared Name' })).toBeNull(); }); + + it('PLACES-SVC-014 — a provider id still wins over a name that points elsewhere', () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + // The place the user renamed, carrying the id the importer knows it by. + const renamed = createPlace(testDb, trip.id, { name: 'Dinner Tuesday', lat: 41.88, lng: 12.47 }); + testDb.prepare('UPDATE places SET google_ftid = ? WHERE id = ?').run('0x1:0x2', renamed.id); + // A different place that happens to carry the name the list still uses. + createPlace(testDb, trip.id, { name: 'Trattoria da Enzo', lat: 41.9, lng: 12.5 }); + + expect( + svc.findMatchingPlaceId(String(trip.id), { name: 'Trattoria da Enzo', google_ftid: '0x1:0x2' }), + ).toBe(renamed.id); + }); + + it('PLACES-SVC-015 — the name comparison is ASCII-only, so an accented capital does not match', () => { + // Not a wish, a boundary: `lower(trim(name))` is SQLite's ASCII lowercase, + // while normalizePlaceName uses JavaScript's Unicode one. isPlaceDuplicate + // does match this pair in memory. Before the shared strategies, the + // coordinate fallback covered the gap for a named candidate — sometimes with + // the wrong row. This pins where the two halves still answer differently. + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + createPlace(testDb, trip.id, { name: 'CAFÉ CENTRAL', lat: 48.85, lng: 2.35 }); + + expect(svc.findMatchingPlaceId(String(trip.id), { name: 'Café Central', lat: 48.85, lng: 2.35 })).toBeNull(); + // The all-ASCII spelling of the same shape does match. + createPlace(testDb, trip.id, { name: 'CAFE CENTRAL', lat: 48.86, lng: 2.36 }); + expect(svc.findMatchingPlaceId(String(trip.id), { name: 'Cafe Central' })).not.toBeNull(); + }); + + it('PLACES-SVC-016 — an unnamed candidate can match a NAMED row on coordinates', () => { + // The other place the two halves differ: buildDedupSet collects coordinates + // only for unnamed rows, so isPlaceDuplicate would say no here. This is the + // answer findMatchingPlaceId wants — a booking with no place name should + // link to the hotel that has one — so it is pinned rather than removed. + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const hotel = createPlace(testDb, trip.id, { name: 'Hotel Lutetia', lat: 48.851, lng: 2.326 }); + + expect(svc.findMatchingPlaceId(String(trip.id), { name: null, lat: 48.851, lng: 2.326 })).toBe(hotel.id); + }); + + it('PLACES-SVC-017 — the coordinate tolerance is a box roughly 11 m wide, and it closes', () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const place = createPlace(testDb, trip.id, { name: null, lat: 48.85, lng: 2.35 }); + + // Pinned as a number, not against the constant: every other reference is + // relative to it, so widening it from 11 m to 111 m would keep them all green. + expect(COORD_DEDUP_TOLERANCE).toBe(0.0001); + + const inside = { name: null, lat: 48.85 + COORD_DEDUP_TOLERANCE * 0.9, lng: 2.35 }; + const outside = { name: null, lat: 48.85 + COORD_DEDUP_TOLERANCE * 1.5, lng: 2.35 }; + expect(svc.findMatchingPlaceId(String(trip.id), inside)).toBe(place.id); + expect(svc.findMatchingPlaceId(String(trip.id), outside)).toBeNull(); + // Exactly one tolerance away is NOT a match: 48.85 + 0.0001 lands on + // 48.850100000000004 in binary floating point, a hair over the bound. The + // edge is fuzzy by design of the arithmetic, so nothing should lean on it. + const edge = { name: null, lat: 48.85 + COORD_DEDUP_TOLERANCE, lng: 2.35 }; + expect(svc.findMatchingPlaceId(String(trip.id), edge)).toBeNull(); + }); });