diff --git a/openspec/changes/gpx-parser-robustness/tasks.md b/openspec/changes/gpx-parser-robustness/tasks.md index e053b89..0eaaba4 100644 --- a/openspec/changes/gpx-parser-robustness/tasks.md +++ b/openspec/changes/gpx-parser-robustness/tasks.md @@ -1,13 +1,13 @@ ## 1. Parser lenience -- [ ] 1.1 Add a finite-number coordinate gate in `packages/gpx/src/parse.ts`: skip `trkpt`/`rtept` with missing or non-finite `lat`/`lon`; treat non-finite `` as undefined -- [ ] 1.2 Drop segments with fewer than 2 surviving points -- [ ] 1.3 Unit tests: missing-lat point skipped, garbage lat skipped with finite stats, garbage ele → undefined with finite gain/loss, single-point segment dropped, well-formed files byte-identical output to before +- [x] 1.1 Add a finite-number coordinate gate in `packages/gpx/src/parse.ts`: skip `trkpt`/`rtept` with missing or non-finite `lat`/`lon`; treat non-finite `` as undefined +- [x] 1.2 Drop segments with fewer than 2 surviving points +- [x] 1.3 Unit tests: missing-lat point skipped, garbage lat skipped with finite stats, garbage ele → undefined with finite gain/loss, single-point segment dropped, well-formed files byte-identical output to before ## 2. Route (``) support -- [ ] 2.1 Parse ``/`rtept` as track segments (appended after `` segments), reusing the same point parsing and lenience -- [ ] 2.2 Unit tests: route-only file yields one segment and passes `validateGpx`; mixed trk+rte ordering; rtept with ele/time preserved +- [x] 2.1 Parse ``/`rtept` as track segments (appended after `` segments), reusing the same point parsing and lenience +- [x] 2.2 Unit tests: route-only file yields one segment and passes `validateGpx`; mixed trk+rte ordering; rtept with ele/time preserved ## 3. Timestamp repair diff --git a/packages/gpx/src/geom.test.ts b/packages/gpx/src/geom.test.ts index a1521ed..149e88d 100644 --- a/packages/gpx/src/geom.test.ts +++ b/packages/gpx/src/geom.test.ts @@ -56,7 +56,10 @@ describe("GPX to geometry coordinates", () => { const gpxData = await parseGpxAsync(singlePointGpx); const coords = gpxData.tracks.flat().map((p) => [p.lon, p.lat] as [number, number]); - expect(coords).toHaveLength(1); - // Caller should check coords.length >= 2 before creating LineString + // The parser now drops segments left with fewer than 2 points + // (gpx-parser-robustness "Invalid point handling"), so a lone point + // yields no track segment at all — the "insufficient for LineString" + // guard is enforced at the parser boundary rather than left to callers. + expect(coords).toHaveLength(0); }); }); diff --git a/packages/gpx/src/parse.test.ts b/packages/gpx/src/parse.test.ts index 3a1c1b1..b4a2963 100644 --- a/packages/gpx/src/parse.test.ts +++ b/packages/gpx/src/parse.test.ts @@ -68,3 +68,132 @@ describe("parseGpxAsync", () => { await expect(parseGpxAsync("not xml at all <<<<")).rejects.toThrow(); }); }); + +describe("parseGpxAsync — invalid point handling", () => { + const gpx = (body: string) => + `\n${body}`; + + it("skips a point with a missing lat/lon instead of defaulting to Null Island", async () => { + const result = await parseGpxAsync( + gpx(` + 34 + 113 + 519 + `), + ); + expect(result.tracks[0]).toHaveLength(2); + // No 0,0 point leaked in. + expect(result.tracks[0]!.some((p) => p.lat === 0 && p.lon === 0)).toBe(false); + }); + + it("skips a point with garbage coords and keeps distance finite", async () => { + const result = await parseGpxAsync( + gpx(` + + + + `), + ); + expect(result.tracks[0]).toHaveLength(2); + expect(Number.isFinite(result.distance)).toBe(true); + expect(result.distance).toBeGreaterThan(0); + }); + + it("treats unparseable as undefined so gain/loss stay finite", async () => { + const result = await parseGpxAsync( + gpx(` + 34 + NaN + 519 + `), + ); + expect(result.tracks[0]![1]!.ele).toBeUndefined(); + expect(Number.isFinite(result.elevation.gain)).toBe(true); + expect(Number.isFinite(result.elevation.loss)).toBe(true); + }); + + it("tolerates trailing junk on a numeric value (parseFloat lenience)", async () => { + const result = await parseGpxAsync( + gpx(` + 471.0m + 519 + `), + ); + expect(result.tracks[0]![0]!.ele).toBe(471); + }); + + it("drops a segment left with fewer than 2 points", async () => { + const result = await parseGpxAsync( + gpx(` + + + + + + `), + ); + expect(result.tracks).toHaveLength(1); + expect(result.tracks[0]).toHaveLength(2); + }); + + it("leaves a well-formed file's output unchanged", async () => { + const result = await parseGpxAsync(sampleGpx); + expect(result.tracks).toEqual([ + [ + { lat: 52.52, lon: 13.405, ele: 34, time: undefined }, + { lat: 51.05, lon: 13.74, ele: 113, time: undefined }, + { lat: 48.137, lon: 11.576, ele: 519, time: undefined }, + ], + ]); + }); +}); + +describe("parseGpxAsync — route () support", () => { + const gpx = (body: string) => + `\n${body}`; + + it("parses a route-only file into one segment", async () => { + const result = await parseGpxAsync( + gpx(`My Course + 34 + 113 + 519 + `), + ); + expect(result.tracks).toHaveLength(1); + expect(result.tracks[0]).toHaveLength(3); + expect(result.distance).toBeGreaterThan(0); + }); + + it("preserves rtept ele and time", async () => { + const result = await parseGpxAsync( + gpx(` + 34 + 519 + `), + ); + expect(result.tracks[0]![0]).toEqual({ + lat: 52.52, + lon: 13.405, + ele: 34, + time: "2026-01-01T10:00:00Z", + }); + }); + + it("appends route segments after track segments", async () => { + const result = await parseGpxAsync( + gpx(` + + + + + + + `), + ); + expect(result.tracks).toHaveLength(2); + // Track first, route second. + expect(result.tracks[0]![0]!.lat).toBe(52.52); + expect(result.tracks[1]![0]!.lat).toBe(10.0); + }); +}); diff --git a/packages/gpx/src/parse.ts b/packages/gpx/src/parse.ts index 1ea7faa..18f21e6 100644 --- a/packages/gpx/src/parse.ts +++ b/packages/gpx/src/parse.ts @@ -69,28 +69,54 @@ function parseWaypoints(doc: Document): Waypoint[] { }); } -function parseTracks(doc: Document): TrackPoint[][] { - const tracks: TrackPoint[][] = []; - const trksegs = doc.querySelectorAll("trk > trkseg"); +/** + * Parse one `trkpt`/`rtept` element into a TrackPoint, or null if it is + * unusable. Parsing stays `parseFloat`-lenient (accepts leading `+`, + * tolerates trailing junk like `471.0m` that real exporters emit), but a + * point whose `lat`/`lon` is missing or does not parse to a finite number + * is skipped rather than defaulted to `0,0` (which would land on Null + * Island and pass range validation) — spec: gpx-parser-robustness + * "Invalid point handling". A non-finite `` becomes `undefined` (the + * existing "no elevation" representation) so it never poisons gain/loss + * totals with `NaN`. + */ +function parsePoint(pt: Element): TrackPoint | null { + const lat = parseFloat(pt.getAttribute("lat") ?? ""); + const lon = parseFloat(pt.getAttribute("lon") ?? ""); + if (!Number.isFinite(lat) || !Number.isFinite(lon)) return null; + const eleText = pt.querySelector("ele")?.textContent; + const ele = eleText != null ? parseFloat(eleText) : NaN; + const time = pt.querySelector("time")?.textContent ?? undefined; + return { lat, lon, ele: Number.isFinite(ele) ? ele : undefined, time }; +} - for (const seg of trksegs) { - const points: TrackPoint[] = []; - for (const pt of seg.querySelectorAll("trkpt")) { - const lat = parseFloat(pt.getAttribute("lat") ?? "0"); - const lon = parseFloat(pt.getAttribute("lon") ?? "0"); - const eleText = pt.querySelector("ele")?.textContent; - const time = pt.querySelector("time")?.textContent ?? undefined; - points.push({ - lat, - lon, - ele: eleText ? parseFloat(eleText) : undefined, - time, - }); - } - tracks.push(points); +function parseSegmentPoints(pts: ArrayLike): TrackPoint[] { + const points: TrackPoint[] = []; + for (const pt of Array.from(pts)) { + const parsed = parsePoint(pt); + if (parsed) points.push(parsed); + } + return points; +} + +function parseTracks(doc: Document): TrackPoint[][] { + const segments: TrackPoint[][] = []; + + // Standard tracks: . + for (const seg of Array.from(doc.querySelectorAll("trk > trkseg"))) { + segments.push(parseSegmentPoints(seg.querySelectorAll("trkpt"))); + } + // Routes: . Many exporters (Garmin Connect courses, + // gpx.studio, planner exports) emit only routes; each becomes one + // segment appended after the track segments, with rtept handled + // identically to trkpt — spec: gpx-parser-robustness "Route support". + for (const rte of Array.from(doc.querySelectorAll("rte"))) { + segments.push(parseSegmentPoints(rte.querySelectorAll("rtept"))); } - return tracks; + // Drop empty or single-point segments: they render nothing and break + // distance-math assumptions (spec: "Invalid point handling"). + return segments.filter((seg) => seg.length >= 2); } function parseNoGoAreas(doc: Document): NoGoArea[] {