fix(places): pass the mapped candidate to the SQL half, and state where the two still differ

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.
This commit is contained in:
Maurice
2026-08-29 11:53:02 +02:00
committed by Maurice
parent 84b51a8af5
commit c8c11fdb7a
3 changed files with 131 additions and 5 deletions
+6 -2
View File
@@ -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)) {
+26 -3
View File
@@ -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);
}
});