From 371b7da9dea281428db49124faf3943793878943 Mon Sep 17 00:00:00 2001 From: Maurice Date: Fri, 18 Sep 2026 13:17:43 +0200 Subject: [PATCH] fix(roadtrip): let a drive turn round at a stop it passes through A day is routed as one request over all of its stops, and OSRM's car profile forbids a u-turn at an intermediate waypoint unless asked. When it cannot obey it refuses the whole request rather than the one leg, so a single stop on a dead end took every leg of the day with it. Gouffre de Padirac snaps onto a cave access road that goes nowhere else: each of that day's pairs routes fine alone, all five together come back 400 NoRoute, and the same five with continue_straight=false come back with their legs. The permission goes on every request that has a stop in the middle, in the browser and in the server's planning path, which had the same defect. Two-point requests keep the url they had. OSRM refuses an unknown query parameter outright rather than ignoring it, so a router that has never heard of this one is recognised by its InvalidQuery, asked again without it, and remembered per origin. --- .../components/Map/RouteCalculator.test.ts | 74 ++++++++++++++++ client/src/components/Map/RouteCalculator.ts | 84 +++++++++++++++++-- .../nest/roadtrip/roadtrip-router.service.ts | 10 ++- .../tests/unit/nest/roadtrip-router.test.ts | 25 ++++++ 4 files changed, 187 insertions(+), 6 deletions(-) diff --git a/client/src/components/Map/RouteCalculator.test.ts b/client/src/components/Map/RouteCalculator.test.ts index e7348e037..dc92b877d 100644 --- a/client/src/components/Map/RouteCalculator.test.ts +++ b/client/src/components/Map/RouteCalculator.test.ts @@ -975,3 +975,77 @@ describe('calculateAlternatives', () => { .rejects.toMatchObject({ name: 'RoutingRefusedError', status: 429 }) }) }) + +// ── turning round at a stop ──────────────────────────────────────────────────── + +/** + * Gouffre de Padirac snaps onto `Route du Puits au Salvage`, which leads nowhere else. + * OSRM's car profile forbids a u-turn at an INTERMEDIATE waypoint unless asked, and it + * refuses the ENTIRE request when it cannot obey, so a road-trip day lost every leg it + * had over one cave car park, while each of its pairs routed perfectly on its own. + */ +describe('turning round at a stop', () => { + const askedFor = async ( + call: (points: { lat: number; lng: number }[]) => Promise, + count: number, + ): Promise => { + let asked = '' + server.use(http.get(`${FOSSGIS.driving}/:coords`, ({ request }) => { + asked = request.url + return HttpResponse.json(buildOsrmRouteResponse()) + })) + await call(freshWaypoints(count)) + return asked + } + + it('FE-COMP-ROUTECALCULATOR-055: a drive that stops on the way may turn round where it stopped', async () => { + expect(await askedFor(p => calculateRoute(p), 3)).toContain('continue_straight=false') + expect(await askedFor(p => calculateRouteWithLegs(p), 3)).toContain('continue_straight=false') + expect(await askedFor(p => calculateSegments(p), 3)).toContain('continue_straight=false') + }) + + it('FE-COMP-ROUTECALCULATOR-056: two points have no middle, so the request is the one it always was', async () => { + expect(await askedFor(p => calculateRoute(p), 2)).not.toContain('continue_straight') + expect(await askedFor(p => calculateRouteWithLegs(p), 2)).not.toContain('continue_straight') + expect(await askedFor(p => calculateSegments(p), 2)).not.toContain('continue_straight') + }) + + it('FE-COMP-ROUTECALCULATOR-057: a router that has never heard of the permission is asked without it, once and then always', async () => { + // OSRM refuses an unknown parameter outright instead of ignoring it, and says which + // it is refusing: `InvalidQuery` names the query string, `NoRoute` names the road. + // Without this a router that predates the parameter would refuse every drive with a + // stop in the middle, which is a far worse fault than the one it fixes. + useSettingsStore.setState(st => ({ settings: { ...st.settings, routing_base_url: 'https://old-osrm.example.org' } })) + const asked: string[] = [] + server.use(http.get('https://old-osrm.example.org/route/v1/driving/:coords', ({ request }) => { + asked.push(request.url) + return new URL(request.url).searchParams.has('continue_straight') + ? HttpResponse.json({ code: 'InvalidQuery', message: 'Query string malformed' }, { status: 400 }) + : HttpResponse.json(buildOsrmRouteResponse()) + })) + + const first = await calculateRoute(freshWaypoints(3)) + expect(first.coordinates).toHaveLength(2) + expect(asked).toHaveLength(2) + expect(asked[1]).not.toContain('continue_straight') + + // Remembered per host, so the second drive costs one request rather than two. + await calculateRoute(freshWaypoints(3)) + expect(asked).toHaveLength(3) + expect(asked[2]).not.toContain('continue_straight') + useSettingsStore.setState(st => ({ settings: { ...st.settings, routing_base_url: '' } })) + }) + + it('FE-COMP-ROUTECALCULATOR-058: a refusal about the road is still a refusal, not a second request', async () => { + useSettingsStore.setState(st => ({ settings: { ...st.settings, routing_base_url: 'https://noroute-osrm.example.org' } })) + let calls = 0 + server.use(http.get('https://noroute-osrm.example.org/route/v1/driving/:coords', () => { + calls += 1 + return HttpResponse.json({ code: 'NoRoute', message: 'No route found between points' }, { status: 400 }) + })) + + await expect(calculateRoute(freshWaypoints(3))).rejects.toThrow() + expect(calls).toBe(1) + useSettingsStore.setState(st => ({ settings: { ...st.settings, routing_base_url: '' } })) + }) +}) diff --git a/client/src/components/Map/RouteCalculator.ts b/client/src/components/Map/RouteCalculator.ts index 3638696c5..d201ee2de 100644 --- a/client/src/components/Map/RouteCalculator.ts +++ b/client/src/components/Map/RouteCalculator.ts @@ -53,6 +53,31 @@ function routeBaseFor(profile: 'driving' | 'walking' | 'cycling'): string { return `${withoutTrailingSlashes(configured)}/route/v1/${OSRM_PROFILE_PATH[profile]}` } +/** + * Permission to turn around at a stop, for requests that have stops to turn around at. + * + * OSRM's car profile forbids a u-turn at an INTERMEDIATE waypoint unless asked otherwise, + * and a refusal is not local: the whole request comes back `400 NoRoute`, every leg of it, + * however many were fine. A stop on a dead end is enough. Gouffre de Padirac snaps onto + * `Route du Puits au Salvage`, which goes nowhere else, so a day that visits it in the + * middle lost all its legs at once while each of its pairs routed perfectly on its own. + * That is also why the fault looked like it depended on how the stops were spread across + * days: a pair on a day boundary is asked for on its own, and a request with nothing in + * the middle has nothing to refuse. + * + * Correct as well as convenient here. TREK's waypoints are places somebody stops at, not + * shape hints, and the way out of a dead end IS the way back in, and the constraint was + * buying a detour even where it did not refuse outright. + * + * Empty for two waypoints, which have no middle, so the many two-point callers + * (alternatives, the day connectors, booking geometry) keep the URL they had. + */ +const U_TURN_PARAM = '&continue_straight=false' + +function uTurnParam(waypoints: readonly Waypoint[]): string { + return waypoints.length > 2 ? U_TURN_PARAM : '' +} + /** * `fetch`, with the request counted. * @@ -79,10 +104,16 @@ async function routedFetch( let km = 0 for (let i = 1; i < waypoints.length; i++) km += haversineKm(waypoints[i - 1], waypoints[i]) const sample = { profile, surface: kind, selfHosted, waypoints: waypoints.length, km } + const asked = withoutUTurnIfRefused(url) try { - const response = await fetch(url, { signal }) + const response = await fetch(asked, { signal }) countRoute({ ...sample, failed: !response.ok }) - return response + if (response.ok || !asked.includes(U_TURN_PARAM)) return response + const plain = await droppedUTurn(asked, response) + if (!plain) return response + const second = await fetch(plain, { signal }) + countRoute({ ...sample, failed: !second.ok }) + return second } catch (err) { if (!(err instanceof DOMException && err.name === 'AbortError')) { countRoute({ ...sample, failed: true }) @@ -91,6 +122,49 @@ async function routedFetch( } } +/** + * Hosts that answered `InvalidQuery` to `continue_straight`, by origin. + * + * OSRM refuses an unknown query parameter outright rather than ignoring it, so a router + * that has never heard of this one would refuse EVERY request with a stop in the middle, + * which is a far worse fault than the one it fixes. `osrm-routed` has understood it + * since 5.6, so this is about something else answering under `routing_base_url`, and the + * first real request is the probe: there is no separate one. + * + * By origin rather than per profile, because a router either knows the parameter or does + * not; and in a Set rather than a setting, so pointing the instance at a router that does + * know it starts asking again without a reload. Same shape and the same reasoning as + * `excludeUnsupported` below. + */ +const uTurnUnsupported = new Set() + +const originOf = (url: string): string => { + try { return new URL(url).origin } catch { return url } +} + +/** The url as it should go out, with the permission dropped for a host that refused it. */ +function withoutUTurnIfRefused(url: string): string { + if (!url.includes(U_TURN_PARAM)) return url + return uTurnUnsupported.has(originOf(url)) ? url.replace(U_TURN_PARAM, '') : url +} + +/** + * The same url without the u-turn permission, once it is clear the host objects to the + * PARAMETER and not to the route. Null when the refusal was about the route itself. + * + * OSRM says which it is: `InvalidQuery` names the query string, `NoRoute` names the road. + * Reading the body is safe here because this response is about to be thrown away either + * way, and it only happens on a 400 for a request that carried the parameter. + */ +async function droppedUTurn(url: string, response: Response): Promise { + if (response.status !== 400) return null + let code: unknown + try { code = (await response.clone().json())?.code } catch { return null } + if (code !== 'InvalidQuery') return null + uTurnUnsupported.add(originOf(url)) + return url.replace(U_TURN_PARAM, '') +} + /** * A routing request the host refused, as opposed to one it answered with "no route". * @@ -153,7 +227,7 @@ export async function calculateRoute( } const coords = waypoints.map((p) => `${p.lng},${p.lat}`).join(';') - const url = `${routeBaseFor(profile)}/${coords}?overview=full&geometries=geojson&steps=false` + const url = `${routeBaseFor(profile)}/${coords}?overview=full&geometries=geojson&steps=false${uTurnParam(waypoints)}` const response = await routedFetch(url, signal, 'route', profile, waypoints) if (!response.ok) { @@ -360,7 +434,7 @@ export async function calculateSegments( if (!waypoints || waypoints.length < 2) return [] const coords = waypoints.map((p) => `${p.lng},${p.lat}`).join(';') - const url = `${routeBaseFor('driving')}/${coords}?overview=false&geometries=geojson&steps=false&annotations=distance,duration` + const url = `${routeBaseFor('driving')}/${coords}?overview=false&geometries=geojson&steps=false&annotations=distance,duration${uTurnParam(waypoints)}` const response = await routedFetch(url, signal, 'segments', 'driving', waypoints) if (!response.ok) throw new RoutingRefusedError(response.status, retryAfterMs(response)) @@ -488,7 +562,7 @@ export async function calculateRouteWithLegs( // (plugins name their own modes), which no comparison narrows to the three OSRM knows. const osrmProfile: 'driving' | 'walking' | 'cycling' = profile === 'walking' ? 'walking' : profile === 'cycling' ? 'cycling' : 'driving' - const url = `${routeBaseFor(osrmProfile)}/${coords}?overview=full&geometries=geojson&annotations=distance,duration` + const url = `${routeBaseFor(osrmProfile)}/${coords}?overview=full&geometries=geojson&annotations=distance,duration${uTurnParam(waypoints)}` const response = await routedFetch(url, signal, 'legs', osrmProfile, waypoints) if (!response.ok) throw new RoutingRefusedError(response.status, retryAfterMs(response)) diff --git a/server/src/nest/roadtrip/roadtrip-router.service.ts b/server/src/nest/roadtrip/roadtrip-router.service.ts index 99945476b..b7e7b8a55 100644 --- a/server/src/nest/roadtrip/roadtrip-router.service.ts +++ b/server/src/nest/roadtrip/roadtrip-router.service.ts @@ -244,8 +244,16 @@ export class RoadtripRouterService { const origin = base || `https://routing.openstreetmap.de/routed-${mode === 'walking' ? 'foot' : mode === 'cycling' ? 'bike' : 'car'}`; + // `continue_straight=false` because a stop is somewhere the traveller stops, not a + // shape hint. OSRM's car profile otherwise forbids a u-turn at an INTERMEDIATE + // waypoint, and it refuses the WHOLE request rather than the one leg it cannot + // obey on: a single stop on a dead end (Gouffre de Padirac snaps onto a cave + // access road that leads nowhere else) turned a whole routed day into a 400. The + // browser sends the same thing, see `uTurnParam` in client RouteCalculator.ts. + // Only with something in the middle to turn round at. + const uTurn = points.length > 2 ? '&continue_straight=false' : ''; const payload = await this.request( - `${origin}/route/v1/${path}/${points.map((p) => `${p.lng},${p.lat}`).join(';')}?overview=full&geometries=geojson&steps=false`, + `${origin}/route/v1/${path}/${points.map((p) => `${p.lng},${p.lat}`).join(';')}?overview=full&geometries=geojson&steps=false${uTurn}`, ); const parsed = osrmSchema.parse(payload); const route = parsed.routes[0]; diff --git a/server/tests/unit/nest/roadtrip-router.test.ts b/server/tests/unit/nest/roadtrip-router.test.ts index 5b317f7e3..d15afcdf1 100644 --- a/server/tests/unit/nest/roadtrip-router.test.ts +++ b/server/tests/unit/nest/roadtrip-router.test.ts @@ -105,6 +105,31 @@ describe('server Roadtrip router', () => { expect((await pending).avoidMissed).toEqual(['ferry']); expect(safeFetchAdminConfigured).toHaveBeenCalledTimes(2); }); + it('lets a drive turn round at a stop it passes through, and asks nothing extra of a single leg', async () => { + // OSRM's car profile forbids a u-turn at an INTERMEDIATE waypoint unless asked, and + // refuses the WHOLE request when it cannot obey: one stop on a dead end used to + // cost a routed day all of its legs at once. + const { router } = setup(); + await router.route(1, 1, 1, points, 'driving', []); + expect(safeFetchAdminConfigured).toHaveBeenCalledWith( + expect.stringContaining('continue_straight=false'), + expect.anything(), + ); + + vi.mocked(safeFetchAdminConfigured).mockResolvedValueOnce( + new Response(JSON.stringify({ + code: 'Ok', + routes: [{ distance: 1000, duration: 100, geometry: { coordinates: [[10, 48], [11, 49]] }, legs: [{ distance: 1000, duration: 100 }] }], + })), + ); + const pair = router.route(2, 1, 1, points.slice(0, 2), 'driving', []); + await vi.runAllTimersAsync(); + await pair; + expect(safeFetchAdminConfigured).toHaveBeenLastCalledWith( + expect.not.stringContaining('continue_straight'), + expect.anything(), + ); + }); it('does not apply car avoidance to walking routes', async () => { const route = await setup().router.route(1, 1, 1, points, 'walking', ['toll']); expect(route.avoidMissed).toEqual([]);