diff --git a/server/src/nest/places/places.helpers.ts b/server/src/nest/places/places.helpers.ts index a2dce9617..3dd5c2884 100644 --- a/server/src/nest/places/places.helpers.ts +++ b/server/src/nest/places/places.helpers.ts @@ -86,15 +86,58 @@ export async function reclaimPhotoCache( export interface DedupSet { names: Set; - /** The names of the places with a line: a track to import matches only these (#2810). Absent where none is read. */ - trackNames?: Set; + /** + * The places with a line, keyed by name and course (`trackKey`): a track to import + * matches only these (#2810), and only one that runs the same way (#2870). Absent + * where none is read. + */ + tracks?: Set; coords: Array<{ lat: number; lng: number }>; /** Provider ids (google_place_id, google_ftid, osm_id, amap_poi_id) of the places already in the trip. */ externalIds: Set; } -/** A candidate for the dedup set; `track` when it is a line (a GPX track or route, a KML path). */ -export type DedupCandidate = PlaceMatchCandidate & { track?: boolean }; +/** + * A candidate for the dedup set; `track` when it is a line (a GPX track or route, a + * KML path), with that line in `routeGeometry`. + */ +export type DedupCandidate = PlaceMatchCandidate & { track?: boolean; routeGeometry?: string | null }; + +/** + * Where a line starts, ends and how far it reaches, to about a hundred metres: two + * hikes a watch names alike ("Afternoon Hike") differ here, while the same file read + * again gives the same answer however its points were rounded or thinned. Empty for + * a line that does not parse. + */ +function lineCourse(routeGeometry: string | null | undefined): string { + let points: unknown; + try { + points = JSON.parse(routeGeometry ?? ''); + } catch { + return ''; + } + if (!Array.isArray(points)) return ''; + const line = points.filter( + (p): p is [number, number] => Array.isArray(p) && Number.isFinite(p[0]) && Number.isFinite(p[1]), + ); + if (line.length === 0) return ''; + // A loop rather than Math.min(...): a recorded day holds more points than a call takes arguments. + let [south, west] = line[0]; + let [north, east] = line[0]; + for (const [lat, lng] of line) { + south = Math.min(south, lat); + north = Math.max(north, lat); + west = Math.min(west, lng); + east = Math.max(east, lng); + } + const [first, last] = [line[0], line[line.length - 1]]; + return [first[0], first[1], last[0], last[1], south, west, north, east].map((v) => v.toFixed(3)).join(','); +} + +/** A track's entry in `DedupSet.tracks`: its normalized name and its course. */ +function trackKey(name: string, routeGeometry: string | null | undefined): string { + return `${name}\u0000${lineCourse(routeGeometry)}`; +} /** * Returns true if a candidate place is already represented in the dedup set. @@ -114,9 +157,14 @@ export function isPlaceDuplicate(candidate: DedupCandidate, dedup: DedupSet): bo if (strategy.by === 'externalId') { if (dedup.externalIds.has(strategy.id)) return true; } else if (strategy.by === 'name') { - // A track is the same as a track of that name, never as a sight of that name: - // a watch names a hike after the place it went to (#2810). - if ((candidate.track ? dedup.trackNames : dedup.names)?.has(strategy.name)) return true; + // A track is the same as a track of that name that runs the same way, never as a + // sight of that name: a watch names a hike after the place it went to (#2810), and + // two hikes of one day alike (#2870). + if (candidate.track) { + if (dedup.tracks?.has(trackKey(strategy.name, candidate.routeGeometry))) return true; + } else if (dedup.names.has(strategy.name)) { + return true; + } } else if ( dedup.coords.some( (c) => @@ -143,19 +191,19 @@ export function dedupSetOf( >, ): DedupSet { const names = new Set(); - const trackNames = new Set(); + const tracks = new Set(); const coords: Array<{ lat: number; lng: number }> = []; const externalIds = new Set(); for (const row of rows) { for (const id of externalIdsOf(row)) externalIds.add(id); if (row.name) { names.add(row.name.trim().toLowerCase()); - if (row.route_geometry) trackNames.add(row.name.trim().toLowerCase()); + if (row.route_geometry) tracks.add(trackKey(row.name.trim().toLowerCase(), row.route_geometry)); } else if (row.lat != null && row.lng != null) { coords.push({ lat: row.lat, lng: row.lng }); } } - return { names, trackNames, coords, externalIds }; + return { names, tracks, coords, externalIds }; } /** Record a newly inserted place so subsequent candidates in the same batch are checked against it. */ @@ -164,7 +212,7 @@ export function trackInsertedInDedupSet(place: DedupCandidate, dedup: DedupSet): const normalizedName = normalizePlaceName(place.name); if (normalizedName) { dedup.names.add(normalizedName); - if (place.track) (dedup.trackNames ??= new Set()).add(normalizedName); + if (place.track) (dedup.tracks ??= new Set()).add(trackKey(normalizedName, place.routeGeometry)); } else if (place.lat != null && place.lng != null) { dedup.coords.push({ lat: place.lat, lng: place.lng }); } diff --git a/server/src/nest/places/places.service.ts b/server/src/nest/places/places.service.ts index 33e0b2027..b0514e2f1 100644 --- a/server/src/nest/places/places.service.ts +++ b/server/src/nest/places/places.service.ts @@ -758,7 +758,7 @@ export class PlacesService { async gpxDuplicates(tripId: string, rows: PreparedGpxPlace[]): Promise { const dedup = await this.buildDedupSet(tripId); return rows.map((wp) => { - const candidate = { name: wp.name, lat: wp.lat, lng: wp.lng, track: !!wp.routeGeometry }; + const candidate = { name: wp.name, lat: wp.lat, lng: wp.lng, track: !!wp.routeGeometry, routeGeometry: wp.routeGeometry }; if (isPlaceDuplicate(candidate, dedup)) return true; trackInsertedInDedupSet(candidate, dedup); return false; @@ -925,7 +925,7 @@ export class PlacesService { // `findWithTagsAndRatings` INSIDE the caller's transaction, so the read // sees the still-uncommitted row. for (const wp of waypoints) { - if (isPlaceDuplicate({ name: wp.name, lat: wp.lat, lng: wp.lng, track: !!wp.routeGeometry }, dedup)) { + if (isPlaceDuplicate({ name: wp.name, lat: wp.lat, lng: wp.lng, track: !!wp.routeGeometry, routeGeometry: wp.routeGeometry }, dedup)) { skipped++; continue; } @@ -960,7 +960,7 @@ export class PlacesService { }); const place = (await this.placesRepo.findWithTagsAndRatings(placeId))!; created.push(place); - trackInsertedInDedupSet({ name: wp.name, lat: wp.lat, lng: wp.lng, track: !!wp.routeGeometry }, dedup); + trackInsertedInDedupSet({ name: wp.name, lat: wp.lat, lng: wp.lng, track: !!wp.routeGeometry, routeGeometry: wp.routeGeometry }, dedup); } return { places: created, count: created.length, skipped }; @@ -1033,7 +1033,7 @@ export class PlacesService { const fallbackName = `Placemark ${fallbackIndex}`; const name = parsedPlacemark.name || fallbackName; - if (isPlaceDuplicate({ name, lat: parsedPlacemark.lat, lng: parsedPlacemark.lng, track: isPath }, dedup)) { + if (isPlaceDuplicate({ name, lat: parsedPlacemark.lat, lng: parsedPlacemark.lng, track: isPath, routeGeometry: parsedPlacemark.routeGeometry }, dedup)) { summary.skippedCount += 1; dupCount++; fallbackIndex += 1; @@ -1079,7 +1079,7 @@ export class PlacesService { const place = (await this.placesRepo.findWithTagsAndRatings(placeId))!; created.push(place); - trackInsertedInDedupSet({ name, lat: parsedPlacemark.lat, lng: parsedPlacemark.lng, track: isPath }, dedup); + trackInsertedInDedupSet({ name, lat: parsedPlacemark.lat, lng: parsedPlacemark.lng, track: isPath, routeGeometry: parsedPlacemark.routeGeometry }, dedup); summary.createdCount += 1; fallbackIndex += 1; } diff --git a/server/tests/e2e/tours.e2e.test.ts b/server/tests/e2e/tours.e2e.test.ts index 7088a5d6f..b1025573e 100644 --- a/server/tests/e2e/tours.e2e.test.ts +++ b/server/tests/e2e/tours.e2e.test.ts @@ -535,6 +535,22 @@ describe('Tours e2e (real guards + temp SQLite)', () => { expect(await countRows(orm, Tours)).toBe(1); }); + it('imports two different tracks of the same name, and still skips either one the second time (#2870)', async () => { + // A second hike the watch called "Alpine loop" too, somewhere else entirely. + const other = TRACK_GPX.replace(/lat="49(\.\d+)?" lon="12/g, (m) => m.replace('49', '47').replace('12', '11')); + expect((await importGpx(OWNER, TRACK_GPX)).body).toMatchObject({ skipped: 0 }); + expect((await previewGpx(OWNER, other)).body.tracks[0]).toMatchObject({ duplicate: false }); + const second = await importGpx(OWNER, other); + expect(second.status).toBe(201); + expect(second.body).toMatchObject({ skipped: 0 }); + expect(second.body.tours).toHaveLength(1); + expect(await countRows(orm, Tours)).toBe(2); + + expect((await importGpx(OWNER, TRACK_GPX)).body).toEqual({ tours: [], caution: false, skipped: 1 }); + expect((await importGpx(OWNER, other)).body).toEqual({ tours: [], caution: false, skipped: 1 }); + expect(await countRows(orm, Tours)).toBe(2); + }); + it('takes over a track from before Tours with the same name instead of skipping it (#2810)', async () => { // 4.x stored a GPX track as a plain place: a line, no tours row. const line = '[[49,12,100],[49.01,12.01,150],[49.02,12.02,120]]'; diff --git a/server/tests/unit/nest/places.helpers.test.ts b/server/tests/unit/nest/places.helpers.test.ts index 30d1cd2e6..62ea01342 100644 --- a/server/tests/unit/nest/places.helpers.test.ts +++ b/server/tests/unit/nest/places.helpers.test.ts @@ -33,7 +33,9 @@ describe('isPlaceDuplicate / trackInsertedInDedupSet', () => { { name: ' 008 ', lat: 1, lng: 2, route_geometry: '[[1,2],[3,4]]' }, ]); expect(isPlaceDuplicate({ name: 'hokitika gorge', lat: -42.9, lng: 171.0, track: true }, dedup)).toBe(false); - expect(isPlaceDuplicate({ name: '008', lat: 1, lng: 2, track: true }, dedup)).toBe(true); + expect(isPlaceDuplicate({ name: '008', lat: 1, lng: 2, track: true, routeGeometry: '[[1,2],[3,4]]' }, dedup)).toBe( + true, + ); // A pin still matches every place of its name, a track included. expect(isPlaceDuplicate({ name: 'Hokitika Gorge', lat: null, lng: null }, dedup)).toBe(true); expect(isPlaceDuplicate({ name: '008', lat: null, lng: null }, dedup)).toBe(true); @@ -46,7 +48,37 @@ describe('isPlaceDuplicate / trackInsertedInDedupSet', () => { trackInsertedInDedupSet({ name: 'Ridge', lat: 1, lng: 2 }, bare); expect(isPlaceDuplicate({ name: 'Ridge', lat: 1, lng: 2, track: true }, bare)).toBe(false); trackInsertedInDedupSet({ name: 'Lake', lat: 1, lng: 2, track: true }, bare); - expect([...(bare.trackNames ?? [])]).toEqual(['lake']); + expect([...(bare.tracks ?? [])]).toEqual(['lake\u0000']); + }); + + it('keeps two tracks of one name apart when they run differently (#2870)', () => { + // Two watch hikes, both called "Afternoon Hike": one up the valley, one along the coast. + const valley = '[[46.5,8.0,1200],[46.52,8.03,1500],[46.55,8.06,1900]]'; + const coast = '[[43.7,7.25],[43.72,7.3],[43.74,7.35]]'; + const dedup = dedupSetOf([{ name: 'Afternoon Hike', lat: 46.5, lng: 8.0, route_geometry: valley }]); + expect( + isPlaceDuplicate({ name: 'afternoon hike', lat: 43.7, lng: 7.25, track: true, routeGeometry: coast }, dedup), + ).toBe(false); + // The same file again is still the same track, its points rounded or thinned as they may be. + const valleyAgain = '[[46.50001,8.00002],[46.55,8.06]]'; + expect( + isPlaceDuplicate({ name: 'Afternoon Hike', lat: 46.5, lng: 8.0, track: true, routeGeometry: valleyAgain }, dedup), + ).toBe(true); + + // Within one file: the first of the two goes in, the second is not taken for it. + const batch = emptyDedup(); + const first = { name: 'Day 1', lat: 46.5, lng: 8.0, track: true, routeGeometry: valley }; + const second = { name: 'Day 1', lat: 43.7, lng: 7.25, track: true, routeGeometry: coast }; + expect(isPlaceDuplicate(first, batch)).toBe(false); + trackInsertedInDedupSet(first, batch); + expect(isPlaceDuplicate(second, batch)).toBe(false); + trackInsertedInDedupSet(second, batch); + expect(isPlaceDuplicate({ ...second }, batch)).toBe(true); + + // A line that does not parse falls back to the name alone, on both sides. + const broken = dedupSetOf([{ name: 'Loop', lat: 1, lng: 2, route_geometry: 'not json' }]); + expect(isPlaceDuplicate({ name: 'Loop', lat: 1, lng: 2, track: true, routeGeometry: '{}' }, broken)).toBe(true); + expect(isPlaceDuplicate({ name: 'Loop', lat: 1, lng: 2, track: true, routeGeometry: coast }, broken)).toBe(false); }); it('matches a named place case- and whitespace-insensitively', () => {