From c8c11fdb7a08c58ec45cce3756bb5bddee4cfe00 Mon Sep 17 00:00:00 2001 From: Maurice Date: Sat, 29 Aug 2026 11:53:02 +0200 Subject: [PATCH] fix(places): pass the mapped candidate to the SQL half, and state where the two still differ MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The importer's parsed item spells the provider id `googleFtid`; the match rule reads `google_ftid`. `isPlaceDuplicate` was handed a mapped object, but `findDuplicatePlace` got the raw one, so its id strategy was always empty. The name then decided the match, and a second place sharing that name could take the ftid belonging to the renamed one. One candidate now serves all three calls. Sharing the strategy list shares the ORDER, not the comparison, and two differences survive it. SQLite `lower()` is ASCII-only where JavaScript's is not, so a row stored as CAFÉ CENTRAL does not match the candidate Café Central in SQL while it does in memory. The coordinate fallback used to paper over that for a named candidate, sometimes with the wrong row. And `buildDedupSet` collects coordinates only for unnamed rows while the coordinate query considers every row, which is the answer `findMatchingPlaceId` wants and the opposite of what the in-memory half says. Both are now written down where someone will read them, and pinned by tests, rather than implied away. PLACE-SVC-028e covers the backfill landing on the right row. PLACES-SVC-014 to -017 cover the id beating a misleading name, both remaining divergences, and the tolerance as a number rather than as a reference to itself. --- server/src/nest/places/places.helpers.ts | 8 +- server/src/nest/places/places.service.ts | 29 +++++- server/tests/unit/nest/places.service.test.ts | 99 +++++++++++++++++++ 3 files changed, 131 insertions(+), 5 deletions(-) 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(); + }); });