mirror of
https://github.com/mauriceboe/TREK.git
synced 2026-10-18 21:16:47 +00:00
fix(collections): stop findDuplicateCollectionPlace merging places by coordinates alone
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
This commit is contained in:
@@ -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 };
|
||||
}
|
||||
|
||||
|
||||
@@ -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 ─────────────────────────────────────────
|
||||
|
||||
Reference in New Issue
Block a user