From 8a0932df728b6f9be4f484af88c1bd1db828f0e7 Mon Sep 17 00:00:00 2001 From: michael-bohr <263459669+michael-bohr@users.noreply.github.com> Date: Sat, 29 Aug 2026 09:00:42 -0400 Subject: [PATCH] fix(collections): stop findDuplicateCollectionPlace merging places by coordinates alone MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three places answered "is this a place the trip already has?", not two: PlacesService.findDuplicatePlace and isPlaceDuplicate got the shared strategy list in #2130; CollectionsService.findDuplicateCollectionPlace was missed and kept its own hand-rolled copy of the pre-#2130 order. Unlike findDuplicatePlace, this one had no isPlaceDuplicate() guard in front of it — savePlace calls it directly — so the coordinate fallback for a NAMED candidate was live, not latent: two distinct places sharing an address (a restaurant and the bar upstairs) would report each other as duplicates the moment their coordinates round to the same ~11 m box, regardless of name. It also never read google_place_id/google_ftid/osm_id at all, even though every collection_places row stores them, so a place renamed since it was saved could be saved again instead of being recognised by its provider id. Now walks placeMatchStrategies() from @trek/shared, same as findMatchingPlaceId: provider id, then name, then coordinates only when there is no name. savePlace passes the provider id fields through since they are already on the request body; the two bulk call sites (importablePlaces, saveFromTripPlaces) are unchanged, since their source rows don't carry provider ids to pass. Closes #2137 --- .../nest/collections/collections.service.ts | 48 ++++++++++++++----- .../unit/nest/collections.service.test.ts | 41 ++++++++++++++++ 2 files changed, 78 insertions(+), 11 deletions(-) diff --git a/server/src/nest/collections/collections.service.ts b/server/src/nest/collections/collections.service.ts index 51f3a0349..200ed40d0 100644 --- a/server/src/nest/collections/collections.service.ts +++ b/server/src/nest/collections/collections.service.ts @@ -12,6 +12,7 @@ import { trackInsertedInDedupSet, type DedupSet, } from '../places/places.helpers'; +import { placeMatchStrategies, type PlaceMatchCandidate } from '@trek/shared'; import type { Collection, CollectionDetailResponse, @@ -418,26 +419,48 @@ export class CollectionsService { // Dedup (collection-scoped ports of placeService helpers) // ------------------------------------------------------------------------- + /** + * A third hand-written copy of the place-matching order (alongside + * PlacesService.findDuplicatePlace and isPlaceDuplicate), and the one that had + * actually drifted live rather than latently: unlike findDuplicatePlace's one + * caller, savePlace calls this directly with no isPlaceDuplicate guard in + * front, so a named candidate that matched nothing by name fell through to a + * coordinate match unconditionally — merging the restaurant and the bar at one + * address. It also never read google_place_id/google_ftid/osm_id at all, even + * though every collection_places row stores them, so a renamed place with no + * matching name or coordinates could be saved again under its old id. + * + * Now walks the shared strategy list from @trek/shared, same as + * PlacesService.findMatchingPlaceId: provider id, then name, then coordinates + * and only when there is no name. + */ private findDuplicateCollectionPlace( collectionId: number, - candidate: { name: string | null | undefined; lat: number | null | undefined; lng: number | null | undefined }, + candidate: PlaceMatchCandidate, ): { id: number; name: string } | null { - const normalizedName = candidate.name?.trim().toLowerCase(); - if (normalizedName) { - const dup = this.db.get<{ id: number; name: string }>(` + for (const strategy of placeMatchStrategies(candidate)) { + let hit: { id: number; name: string } | undefined; + if (strategy.by === 'externalId') { + hit = this.db.get<{ id: number; name: string }>(` + SELECT id, name FROM collection_places + WHERE collection_id = ? AND (google_place_id = ? OR google_ftid = ? OR osm_id = ?) + ORDER BY id ASC LIMIT 1 + `, collectionId, strategy.id, strategy.id, strategy.id); + } else if (strategy.by === 'name') { + hit = this.db.get<{ id: number; name: string }>(` SELECT id, name FROM collection_places WHERE collection_id = ? AND lower(trim(name)) = ? ORDER BY id ASC LIMIT 1 - `, collectionId, normalizedName); - if (dup) return dup; - } - if (candidate.lat != null && candidate.lng != null) { - return this.db.get<{ id: number; name: string }>(` + `, collectionId, strategy.name); + } else { + hit = this.db.get<{ id: number; name: string }>(` SELECT id, name FROM collection_places WHERE collection_id = ? AND lat IS NOT NULL AND lng IS NOT NULL AND abs(lat - ?) <= ? AND abs(lng - ?) <= ? ORDER BY id ASC LIMIT 1 - `, collectionId, candidate.lat, COORD_DEDUP_TOLERANCE, candidate.lng, COORD_DEDUP_TOLERANCE) || null; + `, collectionId, strategy.lat, strategy.tolerance, strategy.lng, strategy.tolerance); + } + if (hit) return hit; } return null; } @@ -491,7 +514,10 @@ export class CollectionsService { this.assertCanEdit(userId, body.collection_id); if (!body.force) { - const dup = this.findDuplicateCollectionPlace(body.collection_id, { name: body.name, lat: body.lat, lng: body.lng }); + const dup = this.findDuplicateCollectionPlace(body.collection_id, { + name: body.name, lat: body.lat, lng: body.lng, + google_place_id: body.google_place_id, google_ftid: body.google_ftid, osm_id: body.osm_id, + }); if (dup) return { duplicate: true, duplicateOf: dup }; } diff --git a/server/tests/unit/nest/collections.service.test.ts b/server/tests/unit/nest/collections.service.test.ts index ba0abe91c..60309ee88 100644 --- a/server/tests/unit/nest/collections.service.test.ts +++ b/server/tests/unit/nest/collections.service.test.ts @@ -179,6 +179,47 @@ describe('saved places + dedup', () => { const col = svc.createCollection(a.id, { name: 'Locked' }); expect(() => svc.savePlace(b.id, { collection_id: col.id, name: 'X' })).toThrow(); }); + + it('COLLECTIONS-SVC-100: a NAMED candidate does not merge into a different place at the same coordinates', () => { + // The wrong-city hazard: findDuplicateCollectionPlace used to fall through to + // a coordinate match for a named candidate whose name did not match anything, + // which would report two distinct places at one address (the restaurant and + // the bar) as duplicates of each other. + const u = createUser(testDb).user; + const col = svc.createCollection(u.id, { name: 'Berlin' }); + svc.savePlace(u.id, { collection_id: col.id, name: 'Ground Floor Diner', lat: 52.52, lng: 13.405 }); + + const result = svc.savePlace(u.id, { collection_id: col.id, name: 'Rooftop Bar', lat: 52.52, lng: 13.405 }); + + expect(result.duplicate).toBeFalsy(); + expect(result.place).toBeDefined(); + }); + + it('COLLECTIONS-SVC-101: a provider id still recognises a renamed place a name/coords search would miss', () => { + // google_place_id/google_ftid/osm_id are stored on every collection_places row + // but were never read back for dedup, so a renamed place with no matching + // name or coordinates could be saved again under its old provider id. + const u = createUser(testDb).user; + const col = svc.createCollection(u.id, { name: 'Renames' }); + svc.savePlace(u.id, { + collection_id: col.id, + name: 'Original Name', + lat: 1, + lng: 1, + google_place_id: 'ChIJ_abc', + }); + + const result = svc.savePlace(u.id, { + collection_id: col.id, + name: 'Renamed By User', + lat: 2, + lng: 2, + google_place_id: 'ChIJ_abc', + }); + + expect(result.duplicate).toBe(true); + expect(result.duplicateOf?.name).toBe('Original Name'); + }); }); // ── save-from-trip provenance + IDOR ─────────────────────────────────────────