From 78b8b8f55f629bea7d1f308b49f2f1f23003a0b2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ullrich=20Sch=C3=A4fer?= Date: Sun, 10 May 2026 15:02:56 +0200 Subject: [PATCH] Atomic GPX save: validate + persist row + geometry in one transaction Eliminates the silent-failure pattern where a route/activity row could be committed with gpx IS NOT NULL but geom IS NULL if the PostGIS write failed after the row insert. - New gpx-save.server.ts owns all GPX validation (GpxValidationError, validateGpx) and PostGIS geometry writes (writeGeom, tx-aware) - createRoute, updateRoute, createActivity, createRouteFromActivity all wrapped in db.transaction() covering row + geom + version snapshot - demo-bot uses createRoute/createActivity instead of raw inserts; errors propagate loudly - Callback endpoint returns 400 for GpxValidationError instead of 401 - ADR-0006 documents the invariant for future explorers Co-Authored-By: Claude Sonnet 4.6 --- CONTEXT.md | 18 ++ apps/journal/app/lib/activities.server.ts | 97 ++++----- apps/journal/app/lib/demo-bot.server.ts | 60 ++---- apps/journal/app/lib/gpx-save.server.test.ts | 86 ++++++++ apps/journal/app/lib/gpx-save.server.ts | 66 ++++++ apps/journal/app/lib/routes.server.ts | 190 +++++++++--------- .../app/routes/api.routes.$id.callback.ts | 4 + docs/adr/0006-atomic-gpx-save.md | 28 +++ .../changes/atomic-gpx-save/.openspec.yaml | 2 + openspec/changes/atomic-gpx-save/design.md | 101 ++++++++++ openspec/changes/atomic-gpx-save/proposal.md | 32 +++ .../atomic-gpx-save/specs/gpx-save/spec.md | 83 ++++++++ openspec/changes/atomic-gpx-save/tasks.md | 39 ++++ 13 files changed, 624 insertions(+), 182 deletions(-) create mode 100644 apps/journal/app/lib/gpx-save.server.test.ts create mode 100644 apps/journal/app/lib/gpx-save.server.ts create mode 100644 docs/adr/0006-atomic-gpx-save.md create mode 100644 openspec/changes/atomic-gpx-save/.openspec.yaml create mode 100644 openspec/changes/atomic-gpx-save/design.md create mode 100644 openspec/changes/atomic-gpx-save/proposal.md create mode 100644 openspec/changes/atomic-gpx-save/specs/gpx-save/spec.md create mode 100644 openspec/changes/atomic-gpx-save/tasks.md diff --git a/CONTEXT.md b/CONTEXT.md index 007ea13..e48200e 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -9,6 +9,24 @@ first. If the term you need isn't here, propose it (don't invent a synonym). --- +## GPX Save + +The atomic unit of persisting spatial data in the Journal. Any write of a GPX track — whether a new route, an updated route, a new activity, or a route derived from an activity — goes through a single path that validates, writes the row, and writes the PostGIS geometry in one transaction. + +### gpx-save module +`apps/journal/app/lib/gpx-save.server.ts`. The sole owner of GPX validation and geometry persistence. Imported by `routes.server.ts`, `activities.server.ts`, and `demo-bot.server.ts`. Nothing else calls `setGeomFromGpx` directly. + +### GpxValidationError +Typed error thrown by `validateGpx` when the GPX string cannot produce a valid LineString. Conditions: fewer than 2 track points, or coordinates outside valid ranges (lat −90..90, lon −180..180). Callers catch this to return a user-facing 400. + +### validateGpx +`(gpx: string) → Promise`. Entry point of the gpx-save module. Parses the GPX string once and validates the result. Returns the `ParsedGpx` so callers can extract stats without re-parsing. Throws `GpxValidationError` on invalid input. Called at the start of `createRoute`, `updateRoute`, `createActivity`, and `createRouteFromActivity` — before any DB write. + +### atomic GPX save +The invariant: a route or activity row with a `gpx` column set **always** has a corresponding `geom` column set. Enforced by wrapping the row insert/update, the PostGIS geometry write, and the version snapshot in a single `db.transaction()`. A PostGIS failure rolls back the row write; partial state (row exists, geom NULL) is not possible through the normal save path. + +--- + ## Connected Services The user-facing surface for linking external accounts and devices to a Journal diff --git a/apps/journal/app/lib/activities.server.ts b/apps/journal/app/lib/activities.server.ts index 0dfd7f2..d41d6f2 100644 --- a/apps/journal/app/lib/activities.server.ts +++ b/apps/journal/app/lib/activities.server.ts @@ -3,8 +3,8 @@ import { eq, desc, and, sql } from "drizzle-orm"; import { getDb } from "./db.ts"; import { activities, routes, syncImports, users, follows } from "@trails-cool/db/schema/journal"; import type { Visibility } from "@trails-cool/db/schema/journal"; -import { parseGpxAsync } from "@trails-cool/gpx"; -import { setGeomFromGpx } from "./routes.server.ts"; +import { validateGpx, writeGeom } from "./gpx-save.server.ts"; +import type { GpxData } from "./gpx-save.server.ts"; export interface ActivityInput { name: string; @@ -15,6 +15,7 @@ export interface ActivityInput { duration?: number | null; startedAt?: Date | null; visibility?: Visibility; + synthetic?: boolean; } export async function updateActivityVisibility( @@ -46,44 +47,45 @@ export async function createActivity(ownerId: string, input: ActivityInput) { const db = getDb(); const id = randomUUID(); + let parsed: GpxData | null = null; let distance: number | null = input.distance ?? null; let elevationGain: number | null = null; let elevationLoss: number | null = null; let startedAt: Date | null = input.startedAt ?? null; const duration: number | null = input.duration ?? null; - if (input.gpx) { - try { - const gpxData = await parseGpxAsync(input.gpx); - distance = gpxData.distance || distance; - elevationGain = gpxData.elevation.gain; - elevationLoss = gpxData.elevation.loss; - if (!startedAt && gpxData.tracks[0]?.[0]?.time) { - startedAt = new Date(gpxData.tracks[0][0].time); - } - } catch { - // Continue without stats if GPX parsing fails + if (input.gpx) { + parsed = await validateGpx(input.gpx); + distance = parsed.distance || distance; + elevationGain = parsed.elevation.gain; + elevationLoss = parsed.elevation.loss; + if (!startedAt && parsed.tracks[0]?.[0]?.time) { + startedAt = new Date(parsed.tracks[0][0].time); } } - await db.insert(activities).values({ - id, - ownerId, - routeId: input.routeId ?? null, - name: input.name, - description: input.description ?? "", - gpx: input.gpx, - distance, - duration, - elevationGain, - elevationLoss, - startedAt, - ...(input.visibility ? { visibility: input.visibility } : {}), - }); + await db.transaction(async (tx) => { + await tx.insert(activities).values({ + id, + ownerId, + routeId: input.routeId ?? null, + name: input.name, + description: input.description ?? "", + gpx: input.gpx, + distance, + duration, + elevationGain, + elevationLoss, + startedAt, + ...(input.visibility ? { visibility: input.visibility } : {}), + ...(input.synthetic ? { synthetic: true } : {}), + }); - if (input.gpx) { - await setGeomFromGpx(id, "activities", input.gpx); - } + if (input.gpx && parsed) { + const coords = parsed.tracks.flat().map((p) => [p.lon, p.lat] as [number, number]); + await writeGeom(tx, id, "activities", coords); + } + }); // Public activities at creation also fan out (matches the // updateActivityVisibility path for the case where visibility is set @@ -236,26 +238,27 @@ export async function createRouteFromActivity(activityId: string, ownerId: strin const [activity] = await db.select().from(activities).where(eq(activities.id, activityId)); if (!activity?.gpx) return null; + const parsed = await validateGpx(activity.gpx); + const coords = parsed.tracks.flat().map((p) => [p.lon, p.lat] as [number, number]); const routeId = randomUUID(); - await db.insert(routes).values({ - id: routeId, - ownerId, - name: `Route from: ${activity.name}`, - description: `Created from activity "${activity.name}"`, - gpx: activity.gpx, - distance: activity.distance, - elevationGain: activity.elevationGain, - elevationLoss: activity.elevationLoss, + + await db.transaction(async (tx) => { + await tx.insert(routes).values({ + id: routeId, + ownerId, + name: `Route from: ${activity.name}`, + description: `Created from activity "${activity.name}"`, + gpx: activity.gpx, + distance: activity.distance, + elevationGain: activity.elevationGain, + elevationLoss: activity.elevationLoss, + }); + + await writeGeom(tx, routeId, "routes", coords); + + await tx.update(activities).set({ routeId }).where(eq(activities.id, activityId)); }); - await setGeomFromGpx(routeId, "routes", activity.gpx); - - // Link the activity to the new route - await db - .update(activities) - .set({ routeId }) - .where(eq(activities.id, activityId)); - return routeId; } diff --git a/apps/journal/app/lib/demo-bot.server.ts b/apps/journal/app/lib/demo-bot.server.ts index e2d3d0e..8ee5fd5 100644 --- a/apps/journal/app/lib/demo-bot.server.ts +++ b/apps/journal/app/lib/demo-bot.server.ts @@ -4,9 +4,10 @@ import { z } from "zod"; import { and, eq, lt, sql } from "drizzle-orm"; import { getDb } from "./db.ts"; import { activities, routes, users } from "@trails-cool/db/schema/journal"; -import { setGeomFromGpx } from "./routes.server.ts"; +import { createRoute } from "./routes.server.ts"; +import { createActivity } from "./activities.server.ts"; +import { validateGpx } from "./gpx-save.server.ts"; import { TERMS_VERSION } from "./legal.ts"; -import { parseGpxAsync } from "@trails-cool/gpx"; import { logger } from "./logger.server.ts"; import { demoBotSyntheticActivitiesTotal, @@ -648,59 +649,40 @@ export async function generateOneWalk( const name = templateName(now, locale, persona); const description = templateDescription(now, locale, persona); - let distance: number; - let elevationGain: number; - let elevationLoss: number; - try { - const parsed = await parseGpxAsync(result.gpx); - distance = parsed.distance; - elevationGain = parsed.elevation.gain; - elevationLoss = parsed.elevation.loss; - } catch { - return null; - } + const parsed = await validateGpx(result.gpx); + const distance = parsed.distance; + const elevationGain = parsed.elevation.gain; + const elevationLoss = parsed.elevation.loss; if (!distance || distance < 500) return null; - const db = getDb(); - const routeId = randomUUID(); - const activityId = randomUUID(); - - await db.insert(routes).values({ - id: routeId, - ownerId, - name, - description, - gpx: result.gpx, - routingProfile: "trekking", - distance, - elevationGain, - elevationLoss, - visibility: "public", - synthetic: true, - }); - await setGeomFromGpx(routeId, "routes", result.gpx); - const walkingMetersPerSecond = 4.5 * 1000 / 3600; // ~4.5 km/h const jitter = 0.85 + Math.random() * 0.3; // ±15 % const durationSeconds = Math.round((distance / walkingMetersPerSecond) * jitter); - await db.insert(activities).values({ - id: activityId, - ownerId, - routeId, + const routeId = await createRoute(ownerId, { name, description, gpx: result.gpx, - startedAt: now, - duration: durationSeconds, + routingProfile: "trekking", + visibility: "public", + synthetic: true, distance, elevationGain, elevationLoss, + }); + + await createActivity(ownerId, { + name, + description, + gpx: result.gpx, + routeId, + startedAt: now, + duration: durationSeconds, + distance, visibility: "public", synthetic: true, }); - await setGeomFromGpx(activityId, "activities", result.gpx); return routeId; } diff --git a/apps/journal/app/lib/gpx-save.server.test.ts b/apps/journal/app/lib/gpx-save.server.test.ts new file mode 100644 index 0000000..b1455c2 --- /dev/null +++ b/apps/journal/app/lib/gpx-save.server.test.ts @@ -0,0 +1,86 @@ +import { describe, it, expect } from "vitest"; +import { validateGpx, GpxValidationError } from "./gpx-save.server.ts"; + +const VALID_GPX = ` + + + + 500 + 520 + 510 + + +`; + +const ONE_POINT_GPX = ` + + + + 500 + + +`; + +const EMPTY_GPX = ` + + +`; + +const OUT_OF_RANGE_LAT_GPX = ` + + + + + + + +`; + +const OUT_OF_RANGE_LON_GPX = ` + + + + + + + +`; + +describe("validateGpx", () => { + it("returns parsed GpxData for valid GPX", async () => { + const result = await validateGpx(VALID_GPX); + expect(result.tracks.flat().length).toBe(3); + expect(result.tracks[0]![0]!.lat).toBeCloseTo(47.0); + }); + + it("throws GpxValidationError for GPX with fewer than 2 track points", async () => { + await expect(validateGpx(ONE_POINT_GPX)).rejects.toThrow(GpxValidationError); + await expect(validateGpx(ONE_POINT_GPX)).rejects.toThrow("at least 2 track points"); + }); + + it("throws GpxValidationError for GPX with zero track points", async () => { + await expect(validateGpx(EMPTY_GPX)).rejects.toThrow(GpxValidationError); + await expect(validateGpx(EMPTY_GPX)).rejects.toThrow("at least 2 track points"); + }); + + it("throws GpxValidationError for out-of-range latitude", async () => { + await expect(validateGpx(OUT_OF_RANGE_LAT_GPX)).rejects.toThrow(GpxValidationError); + await expect(validateGpx(OUT_OF_RANGE_LAT_GPX)).rejects.toThrow("out-of-range coordinates"); + }); + + it("throws GpxValidationError for out-of-range longitude", async () => { + await expect(validateGpx(OUT_OF_RANGE_LON_GPX)).rejects.toThrow(GpxValidationError); + await expect(validateGpx(OUT_OF_RANGE_LON_GPX)).rejects.toThrow("out-of-range coordinates"); + }); + + it("throws GpxValidationError for unparseable XML", async () => { + await expect(validateGpx("not xml at all")).rejects.toThrow(GpxValidationError); + await expect(validateGpx(" { + const err = await validateGpx(EMPTY_GPX).catch((e) => e); + expect(err).toBeInstanceOf(GpxValidationError); + expect(err.name).toBe("GpxValidationError"); + }); +}); diff --git a/apps/journal/app/lib/gpx-save.server.ts b/apps/journal/app/lib/gpx-save.server.ts new file mode 100644 index 0000000..b1411e9 --- /dev/null +++ b/apps/journal/app/lib/gpx-save.server.ts @@ -0,0 +1,66 @@ +import { sql } from "drizzle-orm"; +import type { Database } from "@trails-cool/db"; +import { parseGpxAsync } from "@trails-cool/gpx"; +import type { GpxData } from "@trails-cool/gpx"; + +// Re-export so callers only need one import. +export type { GpxData }; + +export class GpxValidationError extends Error { + constructor(message: string) { + super(message); + this.name = "GpxValidationError"; + } +} + +/** + * Parse and validate a GPX string. Returns the parsed data so callers + * don't need to parse again for stat extraction. Throws GpxValidationError + * when the GPX is malformed, has fewer than 2 track points, or contains + * out-of-range coordinates. + */ +export async function validateGpx(gpx: string): Promise { + let parsed: GpxData; + try { + parsed = await parseGpxAsync(gpx); + } catch (e) { + throw new GpxValidationError(`GPX parse failed: ${(e as Error).message}`); + } + + const points = parsed.tracks.flat(); + if (points.length < 2) { + throw new GpxValidationError( + `GPX must contain at least 2 track points (got ${points.length})`, + ); + } + + for (const p of points) { + if (p.lat < -90 || p.lat > 90 || p.lon < -180 || p.lon > 180) { + throw new GpxValidationError( + `GPX contains out-of-range coordinates: lat=${p.lat}, lon=${p.lon}`, + ); + } + } + + return parsed; +} + +type Tx = Parameters[0]>[0]; + +/** + * Write a PostGIS LineString geometry from already-validated track points. + * Must be called inside a db.transaction() — the tx client is required so + * the geometry write participates in the enclosing transaction. + * Throws on failure; callers must NOT swallow the error. + */ +export async function writeGeom( + tx: Tx, + id: string, + table: "routes" | "activities", + coords: Array<[number, number]>, +): Promise { + const geojson = JSON.stringify({ type: "LineString", coordinates: coords }); + await tx.execute( + sql`UPDATE ${sql.identifier("journal")}.${sql.identifier(table)} SET geom = ST_GeomFromGeoJSON(${geojson}) WHERE id = ${id}`, + ); +} diff --git a/apps/journal/app/lib/routes.server.ts b/apps/journal/app/lib/routes.server.ts index 1d6f13c..7c7d181 100644 --- a/apps/journal/app/lib/routes.server.ts +++ b/apps/journal/app/lib/routes.server.ts @@ -3,8 +3,9 @@ import { eq, desc, and } from "drizzle-orm"; import { getDb } from "./db.ts"; import { routes, routeVersions } from "@trails-cool/db/schema/journal"; import type { Visibility } from "@trails-cool/db/schema/journal"; -import { parseGpxAsync } from "@trails-cool/gpx"; import { sql } from "drizzle-orm"; +import { validateGpx, writeGeom } from "./gpx-save.server.ts"; +import type { GpxData } from "./gpx-save.server.ts"; export interface RouteInput { name: string; @@ -12,52 +13,65 @@ export interface RouteInput { gpx?: string; routingProfile?: string; visibility?: Visibility; + // Pre-computed stats — when provided, skip re-parsing (used by demo-bot) + distance?: number | null; + elevationGain?: number | null; + elevationLoss?: number | null; + dayBreaks?: number[]; + synthetic?: boolean; } export async function createRoute(ownerId: string, input: RouteInput) { const db = getDb(); const id = randomUUID(); - let distance: number | null = null; - let elevationGain: number | null = null; - let elevationLoss: number | null = null; - let dayBreaks: number[] = []; + let parsed: GpxData | null = null; + let distance: number | null = input.distance ?? null; + let elevationGain: number | null = input.elevationGain ?? null; + let elevationLoss: number | null = input.elevationLoss ?? null; + let dayBreaks: number[] = input.dayBreaks ?? []; + if (input.gpx) { - const stats = await computeRouteStats(input.gpx); - distance = stats.distance; - elevationGain = stats.elevationGain; - elevationLoss = stats.elevationLoss; - dayBreaks = stats.dayBreaks; + parsed = await validateGpx(input.gpx); + // Only compute stats from GPX if not pre-supplied by caller + if (input.distance === undefined) { + const stats = computeRouteStats(parsed); + distance = stats.distance; + elevationGain = stats.elevationGain; + elevationLoss = stats.elevationLoss; + dayBreaks = stats.dayBreaks; + } } - await db.insert(routes).values({ - id, - ownerId, - name: input.name, - description: input.description ?? "", - gpx: input.gpx, - routingProfile: input.routingProfile, - distance, - elevationGain, - elevationLoss, - dayBreaks, - }); - - if (input.gpx) { - await setGeomFromGpx(id, "routes", input.gpx); - } - - // Create initial version if GPX provided - if (input.gpx) { - await db.insert(routeVersions).values({ - id: randomUUID(), - routeId: id, - version: 1, + await db.transaction(async (tx) => { + await tx.insert(routes).values({ + id, + ownerId, + name: input.name, + description: input.description ?? "", gpx: input.gpx, - createdBy: ownerId, - changeDescription: "Initial version", + routingProfile: input.routingProfile, + distance, + elevationGain, + elevationLoss, + dayBreaks, + ...(input.synthetic ? { synthetic: true } : {}), }); - } + + if (input.gpx && parsed) { + const coords = parsed.tracks.flat().map((p) => [p.lon, p.lat] as [number, number]); + await writeGeom(tx, id, "routes", coords); + + await tx.insert(routeVersions).values({ + id: randomUUID(), + routeId: id, + version: 1, + gpx: input.gpx, + createdBy: ownerId, + changeDescription: "Initial version", + }); + } + }); return id; } @@ -92,7 +106,6 @@ export async function listRoutes(ownerId: string) { .where(eq(routes.ownerId, ownerId)) .orderBy(desc(routes.updatedAt)); - // Batch-fetch simplified GeoJSON for list thumbnails const ids = rows.map((r) => r.id); const geojsonMap = ids.length > 0 ? await getSimplifiedGeojsonBatch(ids) : new Map(); return rows.map((r) => ({ ...r, geojson: geojsonMap.get(r.id) ?? null })); @@ -122,14 +135,19 @@ export async function updateRoute( ) { const db = getDb(); + let parsed: GpxData | null = null; + if (input.gpx) { + parsed = await validateGpx(input.gpx); + } + const updateData: Record = { updatedAt: new Date() }; if (input.name !== undefined) updateData.name = input.name; if (input.description !== undefined) updateData.description = input.description; if (input.visibility !== undefined) updateData.visibility = input.visibility; - if (input.gpx) { + if (input.gpx && parsed) { + const stats = computeRouteStats(parsed); updateData.gpx = input.gpx; - const stats = await computeRouteStats(input.gpx); updateData.distance = stats.distance; updateData.elevationGain = stats.elevationGain; updateData.elevationLoss = stats.elevationLoss; @@ -137,33 +155,35 @@ export async function updateRoute( if (stats.description && input.description === undefined) { updateData.description = stats.description; } - - // Get next version number - const existingVersions = await db - .select() - .from(routeVersions) - .where(eq(routeVersions.routeId, id)) - .orderBy(desc(routeVersions.version)); - - const nextVersion = (existingVersions[0]?.version ?? 0) + 1; - - await db.insert(routeVersions).values({ - id: randomUUID(), - routeId: id, - version: nextVersion, - gpx: input.gpx, - createdBy: ownerId, - }); } - await db - .update(routes) - .set(updateData) - .where(and(eq(routes.id, id), eq(routes.ownerId, ownerId))); + await db.transaction(async (tx) => { + await tx + .update(routes) + .set(updateData) + .where(and(eq(routes.id, id), eq(routes.ownerId, ownerId))); - if (input.gpx) { - await setGeomFromGpx(id, "routes", input.gpx); - } + if (input.gpx && parsed) { + const coords = parsed.tracks.flat().map((p) => [p.lon, p.lat] as [number, number]); + await writeGeom(tx, id, "routes", coords); + + const existingVersions = await tx + .select() + .from(routeVersions) + .where(eq(routeVersions.routeId, id)) + .orderBy(desc(routeVersions.version)); + + const nextVersion = (existingVersions[0]?.version ?? 0) + 1; + + await tx.insert(routeVersions).values({ + id: randomUUID(), + routeId: id, + version: nextVersion, + gpx: input.gpx, + createdBy: ownerId, + }); + } + }); } export async function deleteRoute(id: string, ownerId: string) { @@ -175,41 +195,19 @@ export async function deleteRoute(id: string, ownerId: string) { return result.length > 0; } -async function computeRouteStats(gpxString: string) { - try { - const gpxData = await parseGpxAsync(gpxString); - const dayBreaks = gpxData.waypoints - .map((w, i) => (w.isDayBreak ? i : -1)) - .filter((i) => i >= 0); - return { - distance: gpxData.distance, - elevationGain: gpxData.elevation.gain, - elevationLoss: gpxData.elevation.loss, - dayBreaks, - description: gpxData.description, - }; - } catch { - return { distance: null, elevationGain: null, elevationLoss: null, dayBreaks: [] as number[], description: undefined }; - } +function computeRouteStats(gpxData: GpxData) { + const dayBreaks = gpxData.waypoints + .map((w, i) => (w.isDayBreak ? i : -1)) + .filter((i) => i >= 0); + return { + distance: gpxData.distance, + elevationGain: gpxData.elevation.gain, + elevationLoss: gpxData.elevation.loss, + dayBreaks, + description: gpxData.description, + }; } -async function setGeomFromGpx(id: string, table: "routes" | "activities", gpxString: string) { - try { - const gpxData = await parseGpxAsync(gpxString); - const coords = gpxData.tracks.flat().map((p) => [p.lon, p.lat] as [number, number]); - if (coords.length < 2) return; - const geojson = JSON.stringify({ type: "LineString", coordinates: coords }); - const db = getDb(); - await db.execute( - sql`UPDATE ${sql.identifier("journal")}.${sql.identifier(table)} SET geom = ST_GeomFromGeoJSON(${geojson}) WHERE id = ${id}`, - ); - } catch (e) { - console.error(`Failed to set geom for ${table}/${id}:`, e); - } -} - -export { setGeomFromGpx }; - async function getGeojson(table: "routes" | "activities", id: string): Promise { try { const db = getDb(); diff --git a/apps/journal/app/routes/api.routes.$id.callback.ts b/apps/journal/app/routes/api.routes.$id.callback.ts index 58a94ce..2bc4f8c 100644 --- a/apps/journal/app/routes/api.routes.$id.callback.ts +++ b/apps/journal/app/routes/api.routes.$id.callback.ts @@ -2,6 +2,7 @@ import { data } from "react-router"; import type { Route } from "./+types/api.routes.$id.callback"; import { verifyRouteToken } from "~/lib/jwt.server"; import { updateRoute, getRoute } from "~/lib/routes.server"; +import { GpxValidationError } from "~/lib/gpx-save.server"; const PLANNER_ORIGIN = process.env.PLANNER_URL ?? "http://localhost:3001"; @@ -65,6 +66,9 @@ export async function action({ params, request }: Route.ActionArgs) { return data({ success: true, routeId: params.id }, { headers: corsHeaders() }); } catch (e) { + if (e instanceof GpxValidationError) { + return data({ error: e.message }, { status: 400, headers: corsHeaders() }); + } return data({ error: (e as Error).message }, { status: 401, headers: corsHeaders() }); } } diff --git a/docs/adr/0006-atomic-gpx-save.md b/docs/adr/0006-atomic-gpx-save.md new file mode 100644 index 0000000..422226b --- /dev/null +++ b/docs/adr/0006-atomic-gpx-save.md @@ -0,0 +1,28 @@ +# ADR-0006: Atomic GPX Save via gpx-save Module + +**Status:** Accepted +**Date:** 2026-05-10 + +## Context + +Routes and activities store GPX text in a `gpx` column and a derived PostGIS LineString in a `geom` column. Originally, the row insert and the `ST_GeomFromGeoJSON` update were separate statements issued outside any transaction. If the geometry write failed (e.g. PostGIS unavailable, malformed coordinates), the row would be committed with `geom IS NULL` and no error surfaced. This silent failure made the geom column unreliable and was invisible to callers. + +Additionally, GPX validation (parse, minimum track points, coordinate range check) was either absent or scattered across callers, with some paths swallowing parse errors and continuing without geometry. + +## Decision + +1. **Single module owns geometry persistence.** `apps/journal/app/lib/gpx-save.server.ts` is the sole place that validates GPX and writes PostGIS geometry. No other module issues raw `UPDATE … SET geom = …` statements. + +2. **`validateGpx(gpx) → Promise`** parses the GPX string, asserts ≥ 2 track points, and checks all coordinates are within valid ranges (lat −90..90, lon −180..180). It throws `GpxValidationError` on any failure. It returns the parsed result so callers don't re-parse for stat extraction. + +3. **`writeGeom(tx, id, table, coords)`** accepts a Drizzle transaction client and writes the PostGIS geometry inside that transaction. It throws on failure — no try/catch anywhere in the call chain. + +4. **Every save that includes GPX is wrapped in `db.transaction()`** covering: the row insert/update, the `writeGeom` call, and any version snapshot insert. A PostGIS failure rolls back the entire transaction — a row with `gpx IS NOT NULL` and `geom IS NULL` cannot be produced through the normal save path. + +5. **Fail loudly everywhere.** The demo-bot and all callers surface errors rather than swallowing them. The demo-bot uses `createRoute` / `createActivity` rather than raw inserts. + +## Consequences + +- `GpxValidationError` surfaces to API callers. The Planner callback endpoint (`api.routes.$id.callback.ts`) catches it and returns 400 so the Planner receives a typed error. +- The `synthetic` flag is threaded through `RouteInput` and `ActivityInput` so the demo-bot can mark rows without bypassing the shared save functions. +- Any future module that needs to persist geometry **must** go through `gpx-save.server.ts`. Do not re-introduce inline `UPDATE … SET geom = …` statements elsewhere. diff --git a/openspec/changes/atomic-gpx-save/.openspec.yaml b/openspec/changes/atomic-gpx-save/.openspec.yaml new file mode 100644 index 0000000..ac20efa --- /dev/null +++ b/openspec/changes/atomic-gpx-save/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-05-10 diff --git a/openspec/changes/atomic-gpx-save/design.md b/openspec/changes/atomic-gpx-save/design.md new file mode 100644 index 0000000..bec26e4 --- /dev/null +++ b/openspec/changes/atomic-gpx-save/design.md @@ -0,0 +1,101 @@ +## Context + +Every write of a GPX track in the Journal follows a two-step pattern: insert/update the row, then call `setGeomFromGpx` to write the PostGIS `geom` column via a raw `UPDATE`. The two steps are not in a transaction. `setGeomFromGpx` wraps its error in a `console.error` and returns, leaving the row with `geom IS NULL`. This affects `createRoute`, `updateRoute`, `createActivity`, `createRouteFromActivity`, and `demo-bot.server.ts` (which bypasses the save functions entirely with raw inserts). + +`setGeomFromGpx` is currently exported from `routes.server.ts` and imported by `activities.server.ts` and `demo-bot.server.ts`, making it a de-facto shared utility that lives in the wrong module. + +## Goals / Non-Goals + +**Goals:** +- `geom IS NULL` on a row that has `gpx` set is impossible through the normal save path. +- GPX is validated (≥2 track points, valid coordinate ranges) before any DB write. +- All callers fail loudly on invalid or unpersistable GPX — no silent degradation. +- PostGIS write participates in the same `db.transaction()` as the row write and version snapshot. +- `setGeomFromGpx` is internal to the new `gpx-save.server.ts` module; no other file calls it. +- `demo-bot.server.ts` uses `createRoute` / `createActivity` and gets the invariant for free. + +**Non-Goals:** +- No API surface changes. +- No DB schema changes or migrations. +- No changes to the `@trails-cool/gpx` parser. +- No changes to how the Planner callback endpoint validates its authorization (JWT stays as-is); validation of the GPX payload is now handled inside `updateRoute`. +- No changes to `getGeojson` / `getSimplifiedGeojsonBatch` — read paths are out of scope. +- No fix for the O(N) `getSimplifiedGeojsonBatch` query pattern (separate concern). + +## Decisions + +### 1. New module: `gpx-save.server.ts` + +`setGeomFromGpx` belongs in neither `routes.server.ts` nor `activities.server.ts` — it is shared infrastructure. A dedicated `apps/journal/app/lib/gpx-save.server.ts` owns validation and geometry persistence. `routes.server.ts` and `activities.server.ts` both import from it. + +Exports: +- `validateGpx(gpx: string): Promise` — public +- `writeGeom(tx, id, table, coords): Promise` — internal (not exported) +- `GpxValidationError` — public (callers catch it to return 400) + +The existing `setGeomFromGpx` export from `routes.server.ts` is removed. Any file that currently imports it must migrate. + +### 2. Validation inside save functions, not at call sites + +`validateGpx` is called at the start of `createRoute`, `updateRoute`, `createActivity`, and `createRouteFromActivity` — before any transaction is opened. This means: + +- Every caller gets validation for free, including the mobile API (`api.v1.routes`) and the Planner callback. +- The callback endpoint does not need its own validation logic; it catches `GpxValidationError` and returns 400. +- Future callers can't bypass validation by accident. + +Alternative considered: validate only at the callback boundary. Rejected because it leaves mobile API and direct DB writes unprotected. + +### 3. `db.transaction()` wrapping row write + geom write + version snapshot + +Drizzle supports `db.transaction(async (tx) => { ... })`. The raw PostGIS `UPDATE` inside `writeGeom` receives `tx` as its first argument and executes within the same transaction. If `writeGeom` throws, Drizzle rolls back the entire transaction — the row insert is reversed. + +``` +db.transaction(async (tx) => { + // 1. validateGpx already ran (before tx opened) + await tx.insert(routes).values(...) // or update + await writeGeom(tx, id, "routes", coords) // throws → rollback + await tx.insert(routeVersions).values(...) +}) +``` + +The transaction is opened *after* `validateGpx` succeeds. This keeps the transaction as short as possible (no async GPX parsing inside a held transaction). + +### 4. `activities.server.ts` stat extraction uses `validateGpx` result + +Currently `createActivity` calls `parseGpxAsync` inline to extract `distance`, `elevationGain`, `elevationLoss`, `startedAt`. After this change, `validateGpx` returns the `ParsedGpx` object. `createActivity` uses that result directly — one parse, not two. + +The activity-specific fields (`startedAt` from `gpxData.tracks[0][0].time`) are still extracted by `createActivity`; `validateGpx` returns the full parsed object so callers have access to everything. + +### 5. `demo-bot.server.ts` migrates to `createRoute` / `createActivity` + +The demo-bot currently does: +```ts +await db.insert(routes).values({ ... distance, elevationGain, ... }) +await setGeomFromGpx(routeId, "routes", result.gpx) +await db.insert(activities).values({ ... }) +await setGeomFromGpx(activityId, "activities", result.gpx) +``` + +After migration it calls `createRoute` and `createActivity`, which own all of this. The demo-bot passes pre-computed stats via `RouteInput` / `ActivityInput` (already supported: the input types accept optional `distance`, `elevationGain`, etc.). `createRoute` will prefer input stats over re-parsed stats when provided, avoiding redundant parsing for the demo-bot's case where BRouter already computed them. + +Alternative considered: leave demo-bot as-is and just make `setGeomFromGpx` throw. Rejected because it keeps a second bypass path alive and requires demo-bot to handle its own transaction. + +### 6. ADR recorded + +ADR-0006 documents that PostGIS geometry writes are always transactional with row writes, so future code doesn't re-introduce the split pattern. + +## Risks / Trade-offs + +- **Transaction duration**: Opening a DB transaction and then doing a raw SQL UPDATE adds a short hold on the row. For typical GPX sizes this is negligible — `validateGpx` runs before the transaction opens, so the held time is just the two DB statements. +- **demo-bot stat re-computation**: `createRoute` will call `validateGpx` on GPX that the demo-bot already parsed. Minor redundancy; acceptable given demo-bot is not a hot path. +- **Existing NULL geom rows**: Rows already in production with `geom IS NULL` are not backfilled by this change. A separate data-repair script would be needed. Out of scope. +- **`GpxValidationError` surfaces to users**: Routes/activities that previously saved silently will now return errors. This is the intended behaviour (fail loudly), but any caller that didn't expect an exception from `updateRoute` must now handle `GpxValidationError`. All current call sites are audited in the tasks. + +## Migration Plan + +No DB migration required. The change is purely in application code. Deploy is a standard app rollout — no staged migration, no feature flag. + +Existing rows with `geom IS NULL` remain as-is (pre-existing data debt). A follow-up query can identify and optionally repair them: +```sql +SELECT id FROM journal.routes WHERE gpx IS NOT NULL AND geom IS NULL; +``` diff --git a/openspec/changes/atomic-gpx-save/proposal.md b/openspec/changes/atomic-gpx-save/proposal.md new file mode 100644 index 0000000..030047d --- /dev/null +++ b/openspec/changes/atomic-gpx-save/proposal.md @@ -0,0 +1,32 @@ +## Why + +Route and activity rows can currently be saved with `geom IS NULL` when the PostGIS geometry write fails after the row insert succeeds — the error is swallowed silently with a `console.error`. This means missing map thumbnails, broken route-push deduplication, and corrupted spatial queries with no signal to the user or operator. The Planner callback endpoint also accepts GPX from an external source with no validation before writing. + +## What Changes + +- **New `gpx-save.server.ts` module** owns GPX validation and atomic geometry persistence; `setGeomFromGpx` moves here and becomes internal. +- **`validateGpx(gpx)`** parses the GPX string, checks ≥2 track points and valid coordinate ranges, throws `GpxValidationError` on failure, and returns the parsed result so callers don't re-parse. +- **`createRoute`, `updateRoute`, `createActivity`, `createRouteFromActivity`** all wrap their DB writes (row insert/update + geom write + version snapshot) in `db.transaction()` — partial state (row exists, geom NULL) is no longer possible. +- **`setGeomFromGpx` stops swallowing errors** — it throws, causing the transaction to roll back. +- **`demo-bot.server.ts`** migrated from raw inserts + `setGeomFromGpx` to `createRoute` / `createActivity`, eliminating a bypass of the new invariant. +- **`activities.server.ts`** removes inline `parseGpxAsync` call; uses the `ParsedGpx` returned by `validateGpx` for stat extraction instead. +- **ADR recorded**: PostGIS geometry writes are always transactional with row writes. + +## Capabilities + +### New Capabilities + +- `gpx-save`: Atomic GPX validation and PostGIS geometry persistence for routes and activities. + +### Modified Capabilities + +*(none — no user-visible requirement changes; this is an implementation-layer correctness fix)* + +## Impact + +- `apps/journal/app/lib/routes.server.ts` — `setGeomFromGpx` export removed; callers updated. +- `apps/journal/app/lib/activities.server.ts` — inline `parseGpxAsync` removed; imports from `gpx-save`. +- `apps/journal/app/lib/demo-bot.server.ts` — raw insert pattern replaced with `createRoute` / `createActivity`. +- `apps/journal/app/routes/api.routes.$id.callback.ts` — no validation changes needed (validation now inside `updateRoute`); GPX errors surface as thrown exceptions → callers return 400. +- `@trails-cool/gpx` — no changes; `parseGpxAsync` is still the underlying parser. +- No API surface changes, no schema changes, no migration required. diff --git a/openspec/changes/atomic-gpx-save/specs/gpx-save/spec.md b/openspec/changes/atomic-gpx-save/specs/gpx-save/spec.md new file mode 100644 index 0000000..ea397d4 --- /dev/null +++ b/openspec/changes/atomic-gpx-save/specs/gpx-save/spec.md @@ -0,0 +1,83 @@ +# gpx-save Specification + +## Purpose + +Atomic GPX validation and PostGIS geometry persistence for routes and activities. Every write of a GPX track in the Journal SHALL validate the track and persist the row + geometry in a single transaction. Silent geometry failures are not permitted. + +## Requirements + +### Requirement: GPX validation before any DB write + +The Journal SHALL validate any GPX string before writing a route or activity row. Validation SHALL confirm the GPX parses successfully, contains at least 2 track points, and all track points have coordinates within valid ranges (latitude −90..90, longitude −180..180). Validation SHALL throw `GpxValidationError` on failure. + +#### Scenario: Valid GPX proceeds to save + +- **WHEN** a caller passes a GPX string with ≥2 track points and valid coordinates to `createRoute`, `updateRoute`, `createActivity`, or `createRouteFromActivity` +- **THEN** the save proceeds and no `GpxValidationError` is thrown + +#### Scenario: GPX with fewer than 2 track points is rejected + +- **WHEN** a caller passes a GPX string that produces fewer than 2 track points +- **THEN** `GpxValidationError` is thrown before any DB write occurs + +#### Scenario: GPX with out-of-range coordinates is rejected + +- **WHEN** a caller passes a GPX string containing coordinates outside valid ranges +- **THEN** `GpxValidationError` is thrown before any DB write occurs + +#### Scenario: Unparseable GPX is rejected + +- **WHEN** a caller passes a malformed GPX string that cannot be parsed +- **THEN** `GpxValidationError` is thrown before any DB write occurs + +### Requirement: Atomic row + geometry persistence + +Every route or activity save that includes a GPX string SHALL wrap the row write, PostGIS geometry write, and version snapshot in a single database transaction. If any step fails, the entire transaction SHALL be rolled back — a route or activity row with a non-null `gpx` column and a null `geom` column SHALL NOT be possible through the normal save path. + +#### Scenario: Successful save stores row and geometry atomically + +- **WHEN** `createRoute` is called with valid GPX +- **THEN** the `routes` row, the `geom` column update, and the `routeVersions` row are all committed in a single transaction + +#### Scenario: PostGIS failure rolls back the row insert + +- **WHEN** the PostGIS geometry write fails during a `createRoute` call +- **THEN** the transaction is rolled back and no `routes` row is persisted + +#### Scenario: Route update is atomic + +- **WHEN** `updateRoute` is called with valid GPX +- **THEN** the row update, geometry update, and new version snapshot are committed atomically + +#### Scenario: Activity save is atomic + +- **WHEN** `createActivity` is called with valid GPX +- **THEN** the `activities` row and `geom` column update are committed atomically + +### Requirement: Loud failure on geometry errors + +The Journal SHALL NOT swallow PostGIS geometry write errors. Any failure during the geometry write SHALL propagate as a thrown exception, causing the enclosing transaction to roll back and the error to surface to the caller. + +#### Scenario: Geometry write error surfaces to caller + +- **WHEN** the PostGIS `UPDATE geom` statement fails +- **THEN** an exception is thrown, the transaction is rolled back, and the caller receives the error + +#### Scenario: demo-bot route creation fails loudly + +- **WHEN** `createRoute` is called from the demo-bot and the PostGIS write fails +- **THEN** the exception propagates — no partial route row is committed + +### Requirement: Single gpx-save module owns geometry persistence + +The Journal SHALL have exactly one module responsible for GPX validation and PostGIS geometry writes: `apps/journal/app/lib/gpx-save.server.ts`. No other module SHALL call PostGIS geometry update statements directly or implement GPX validation independently. + +#### Scenario: activities.server.ts uses gpx-save for geometry + +- **WHEN** `createActivity` or `createRouteFromActivity` writes geometry +- **THEN** the write goes through the `gpx-save` module, not an inline raw SQL statement + +#### Scenario: demo-bot uses createRoute and createActivity + +- **WHEN** the demo-bot creates synthetic routes and activities +- **THEN** it calls `createRoute` and `createActivity` rather than issuing raw inserts and calling `setGeomFromGpx` directly diff --git a/openspec/changes/atomic-gpx-save/tasks.md b/openspec/changes/atomic-gpx-save/tasks.md new file mode 100644 index 0000000..ab34bb6 --- /dev/null +++ b/openspec/changes/atomic-gpx-save/tasks.md @@ -0,0 +1,39 @@ +## 1. New gpx-save module + +- [x] 1.1 Create `apps/journal/app/lib/gpx-save.server.ts` with `GpxValidationError` class and `validateGpx(gpx: string): Promise` — parses, checks ≥2 track points, checks coordinate ranges, throws on failure, returns parsed result +- [x] 1.2 Move `setGeomFromGpx` logic into `gpx-save.server.ts` as an internal `writeGeom(tx, id, table, coords)` function that accepts a Drizzle transaction client and throws on failure (no try/catch) +- [x] 1.3 Write unit tests in `gpx-save.server.test.ts` covering: valid GPX passes, <2 track points throws, out-of-range coordinates throw, unparseable GPX throws + +## 2. Migrate routes.server.ts + +- [x] 2.1 Import `validateGpx` and `writeGeom` from `gpx-save.server.ts`; remove local `setGeomFromGpx` implementation and its export +- [x] 2.2 Wrap `createRoute` in `db.transaction()`: validate GPX → insert row → `writeGeom` → insert `routeVersions`, all within the transaction client +- [x] 2.3 Wrap `updateRoute` in `db.transaction()`: validate GPX → update row → `writeGeom` → insert new `routeVersions`, all within the transaction client +- [x] 2.4 Verify `computeRouteStats` is called with the `ParsedGpx` returned by `validateGpx` (avoid re-parsing) + +## 3. Migrate activities.server.ts + +- [x] 3.1 Replace inline `parseGpxAsync` call in `createActivity` with `validateGpx`; extract stats from the returned `ParsedGpx` +- [x] 3.2 Wrap `createActivity` in `db.transaction()`: validate GPX → insert row → `writeGeom`, all within the transaction client +- [x] 3.3 Wrap `createRouteFromActivity` in `db.transaction()`: insert route row → `writeGeom` → update activity's `routeId`, all within the transaction client +- [x] 3.4 Remove `import { setGeomFromGpx } from "./routes.server.ts"` — no longer needed + +## 4. Migrate demo-bot.server.ts + +- [x] 4.1 Replace the raw `db.insert(routes).values(...)` + `setGeomFromGpx(routeId, ...)` block with a `createRoute(ownerId, input)` call; pass pre-computed stats via `RouteInput` fields so `createRoute` doesn't re-parse BRouter's output +- [x] 4.2 Replace the raw `db.insert(activities).values(...)` + `setGeomFromGpx(activityId, ...)` block with a `createActivity(ownerId, input)` call +- [x] 4.3 Remove `import { setGeomFromGpx } from "./routes.server.ts"` from `demo-bot.server.ts` + +## 5. Callback endpoint error handling + +- [x] 5.1 In `api.routes.$id.callback.ts`, catch `GpxValidationError` from `updateRoute` and return a 400 response with the validation message (currently the catch block returns 401 for all errors — narrow it) + +## 6. ADR + +- [x] 6.1 Write `docs/adr/0006-atomic-gpx-save.md` documenting that PostGIS geometry writes are always transactional with row writes and that `gpx-save.server.ts` is the sole owner of geometry persistence + +## 7. Verification + +- [x] 7.1 Run `pnpm typecheck` — no new type errors +- [x] 7.2 Run `pnpm test` — all existing tests pass; new `gpx-save.server.test.ts` tests pass +- [x] 7.3 Confirm `setGeomFromGpx` is no longer exported from any file (`grep -r "setGeomFromGpx" apps/journal/app` returns no results outside `gpx-save.server.ts`)