mirror of
https://github.com/mauriceboe/TREK.git
synced 2026-10-18 21:16:47 +00:00
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.
This commit is contained in:
@@ -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<unknown>,
|
||||
count: number,
|
||||
): Promise<string> => {
|
||||
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: '' } }))
|
||||
})
|
||||
})
|
||||
|
||||
@@ -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<string>()
|
||||
|
||||
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<string | null> {
|
||||
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))
|
||||
|
||||
|
||||
@@ -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];
|
||||
|
||||
@@ -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([]);
|
||||
|
||||
Reference in New Issue
Block a user