From 8373085d70c89948bebba506bf6f8be8dae97608 Mon Sep 17 00:00:00 2001 From: Matthew Podwysocki Date: Wed, 23 Sep 2026 17:48:58 -0400 Subject: [PATCH 1/3] Add mapbox directions route MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Navigation API category (Directions, Isochrone, Map Matching, Matrix, Optimization, EV Charge Finder) has had no CLI coverage — this is the first of those, hand-authored into custom-openapi/ since openapi-specs publishes no spec for it yet. Excludes the ~30 electric-vehicle-routing parameters (engine=electric and everything under it): those describe a vehicle's charging curve down to the watt, which an integration passes in from a vehicle profile rather than something to hand-type as a CLI flag. The EV Charge Finder API is a better fit for that case. Also excludes POST, which this spec format has no way to express alongside GET for the same operation — a real gap for a very long coordinate list, not a design choice. Two path-parameter bugs surfaced while wiring this up, fixed generically rather than just for this command: - A spec parameter literally named `profile` (the routing profile, mapbox/driving etc.) silently collided with the global --profile credentials flag — clap has one namespace of ids per command, and the generated positional replaced the global one outright instead of erring. `mapbox directions route mapbox/driving …` failed with `Invalid profile name "mapbox/driving"` before this. Fixed with a small, documented arg-name override table (ARG_NAME_OVERRIDES), the same shape BODY_CONTENT_TYPE_OVERRIDES already uses. - An enum-constrained path parameter whose only valid values contain a literal `/` (again, mapbox/driving) was being percent-encoded to %2F by the same escaping that stops a free-text path parameter from smuggling in extra path segments or retargeting the request. Safe to skip once clap's PossibleValuesParser has already limited the value to one of the spec's own literal strings — there is nothing left for a caller to inject. Smoke-tested against production: real routes for driving and walking profiles, with --steps, --geometries geojson, --overview full, and --annotations all verified to return the documented shape. 621 tests, fmt and clippy clean. Co-Authored-By: Claude Sonnet 5 --- CHANGELOG.md | 19 + README.md | 6 +- .../directions/openapi/directions.yaml | 400 ++++++++++++++++++ docs/commands.md | 128 ++++++ src/executor.rs | 50 ++- src/main.rs | 11 +- src/remedy.rs | 4 + src/schema.rs | 6 + src/spec.rs | 109 ++++- tests/fixtures/api_command_surface.txt | 1 + 10 files changed, 719 insertions(+), 15 deletions(-) create mode 100644 custom-openapi/directions/openapi/directions.yaml diff --git a/CHANGELOG.md b/CHANGELOG.md index 273b384..54cb4d3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,25 @@ that may never merge. They are not releases and are not listed here. ## Unreleased +- `mapbox directions route`, routes between 2-25 waypoints for driving (with + or without live traffic), walking, or cycling. Hand-authored into + `custom-openapi/` rather than waiting on an upstream spec — the whole + Navigation API category had no CLI coverage before this; excludes the + ~30 electric-vehicle-routing parameters + (`engine=electric` and everything under it), which describe a vehicle's + charging curve down to the watt and are a poor fit for a hand-typed CLI + flag — the EV Charge Finder API is a better fit for that case and its own + follow-up. Two path-parameter bugs surfaced while wiring this up and are + fixed for every command, not just this one: a spec parameter literally + named `profile` (the routing profile, `mapbox/driving` etc.) silently + collided with the global `--profile` credentials flag, since clap has one + namespace of ids per command and a positional of the same name replaced it + outright; and an enum-constrained path parameter whose only valid values + contain a literal `/` (again, `mapbox/driving`) was being percent-encoded + to `%2F` by the same escaping that stops a free-text value from smuggling + in extra path segments — safe to skip once clap has already limited the + value to one of the spec's own literal strings. + - `MAPBOX_CLI_EXTRA_QUERY` appends raw query parameters to every request, in the same `k1=v1&k2=v2` shape as a URL's own query string — for an API parameter this CLI's specs don't declare a flag for. diff --git a/README.md b/README.md index f047655..1dd71af 100644 --- a/README.md +++ b/README.md @@ -168,15 +168,13 @@ Each API is a top-level subcommand, one sub-subcommand per operation: ```sh mapbox accounts * +mapbox directions * mapbox fonts * mapbox geocoder * -mapbox rasterarrays * mapbox search * mapbox sprites * -mapbox static-images * -mapbox static-tiles * +mapbox static * mapbox styles * -mapbox tilequery * mapbox tilesets * ``` diff --git a/custom-openapi/directions/openapi/directions.yaml b/custom-openapi/directions/openapi/directions.yaml new file mode 100644 index 0000000..75a665f --- /dev/null +++ b/custom-openapi/directions/openapi/directions.yaml @@ -0,0 +1,400 @@ +openapi: "3.0.0" +# `parse_spec` turns `info.description` below into this service's clap +# `long_about`, so it also reaches `mapbox directions --help`, `--schema` and +# `generate-skills` output. Keep it to API prose only — the provenance below +# is for whoever edits this file, not for a CLI user: +# +# Hand-authored down to the parameters documented at +# docs.mapbox.com/api/navigation/directions: routes between 2-25 waypoints +# for driving (with or without live traffic), walking, and cycling. Excludes +# the ~30 electric-vehicle-routing parameters (`engine=electric` and +# everything under it) — those describe a vehicle's charging curve down to +# the watt, which is data an integration passes in from a vehicle profile, +# not something to hand-type as CLI flags. The EV Charge Finder API is +# Mapbox's dedicated per-vehicle routing product and the better fit for that +# case; this file can grow the EV parameters later if that changes. Also +# excludes POST — every other +# operation this CLI wires up is one HTTP method per operation, and the spec +# format here has no way to say "GET or POST, caller's choice" for the same +# operationId. GET's 2-25 waypoint limit covers the CLI's own use (typed or +# scripted, not machine-generated route requests with hundreds of stops), so +# this is a real gap in the corner case, not a design flaw — noted for +# whoever picks up POST as a follow-up. See `custom-openapi/README.md` for +# how a file like this is wired in. +info: + title: "Mapbox Directions API" + description: >- + Turn-by-turn routes between 2-25 waypoints, for driving (with or without + live traffic), walking, or cycling. + version: "0.0.0" +servers: + - url: https://api.mapbox.com + description: Directions API +paths: + /directions/v5/{profile}/{coordinates}: + get: + operationId: route + summary: A route between 2-25 waypoints. + description: >- + Returns one or more routes (`alternatives=true` for more than one) + between the given waypoints, in the order given. Coordinates are + always `{longitude},{latitude}`, and the route follows them in the + order listed — this is a route through fixed stops, not a + traveling-salesman solve; see the Optimization API for that. + parameters: + - name: "profile" + in: path + required: true + description: >- + The routing profile. `mapbox/driving-traffic` accounts for live + traffic conditions; `mapbox/driving` does not. + schema: + type: string + enum: + [ + "mapbox/driving-traffic", + "mapbox/driving", + "mapbox/walking", + "mapbox/cycling", + ] + example: "mapbox/driving" + - name: "coordinates" + in: path + required: true + description: >- + 2-25 waypoints, semicolon-separated, each `{longitude},{latitude}` + — the route visits them in this order. + schema: + type: string + minLength: 1 + example: "-122.42,37.78;-122.45,37.91" + - name: "access_token" + in: query + required: true + description: "Mapbox API Access Token" + schema: + type: string + minLength: 1 + - name: "alternatives" + in: query + required: false + description: >- + Return up to 2 alternative routes alongside the primary one. + schema: + type: boolean + # Prose rather than an `enum`: this is a comma-separated list, and + # the command builder turns a spec `enum` into a clap + # `PossibleValuesParser`, which accepts one value and would refuse + # `distance,duration`. Same reasoning as `search.yaml`'s `types`. + - name: "annotations" + in: query + required: false + description: >- + Segment-level metadata to add to each leg, comma-separated. + Requires `overview=full`. Options are `distance`, `duration`, + `speed`, `congestion`, `congestion_numeric`, `maxspeed`, + `closure`, `state_of_charge`. + schema: + type: string + example: "duration,distance" + - name: "avoid_maneuver_radius" + in: query + required: false + description: >- + Radius in meters, 1-1000, around the start point within which to + avoid a significant maneuver. + schema: + type: number + minimum: 1 + maximum: 1000 + - name: "bearings" + in: query + required: false + description: >- + `{angle},{degrees}` per waypoint, semicolon-separated, filtering + the road segments considered by direction of travel. One entry + per coordinate, or empty (`;;`) to skip a waypoint. + schema: + type: string + - name: "layers" + in: query + required: false + description: >- + A signed integer per waypoint, semicolon-separated, selecting a + road layer (Z-order) at that point on a multi-level road. + schema: + type: string + - name: "continue_straight" + in: query + required: false + description: >- + Whether to keep going straight at an intermediate waypoint + rather than u-turning back to it, even if that is faster. + schema: + type: boolean + # Prose rather than an `enum`, for the reason given on `annotations` + # above — this also accepts freeform `point(lon lat)` values a fixed + # enum couldn't represent at all. + - name: "exclude" + in: query + required: false + description: >- + Road types or specific points to route around, comma-separated. + Options are `motorway`, `toll`, `ferry`, `unpaved`, + `cash_only_tolls`, `country_border`, `state_border`, `tunnel`, + or one or more `point(lon lat)` values. + schema: + type: string + example: "motorway,toll" + - name: "geometries" + in: query + required: false + description: "The route geometry's format. Defaults to `polyline`." + schema: + type: string + enum: ["geojson", "polyline", "polyline6"] + # Prose rather than an `enum`, for the reason given on `annotations` + # above. + - name: "include" + in: query + required: false + description: >- + Special road types to allow, comma-separated. Options are + `hov2`, `hov3`, `hot`. + schema: + type: string + - name: "overview" + in: query + required: false + description: >- + How much geometry detail the response carries. Defaults to + `simplified`. + schema: + type: string + enum: ["full", "simplified", "false"] + - name: "radiuses" + in: query + required: false + description: >- + Maximum distance in meters a coordinate may snap to the road + network, semicolon-separated, one per coordinate — a number or + `unlimited`. + schema: + type: string + - name: "approaches" + in: query + required: false + description: >- + Which side of the road each waypoint should be approached from, + semicolon-separated — `unrestricted` or `curb` per coordinate. + schema: + type: string + - name: "steps" + in: query + required: false + description: >- + Return turn-by-turn instructions. Several other parameters + (`banner_instructions`, `language`, `roundabout_exits`, + `voice_instructions`, `waypoints`, `waypoint_names`, + `waypoint_targets`) only take effect when this is set. + schema: + type: boolean + - name: "banner_instructions" + in: query + required: false + description: "Return banner objects for display. Requires `steps=true`." + schema: + type: boolean + - name: "language" + in: query + required: false + description: >- + The language turn-by-turn instructions are written in. Defaults + to `en`. Requires `steps=true`. + schema: + type: string + example: "en" + - name: "roundabout_exits" + in: query + required: false + description: >- + Emit a separate instruction for entering and exiting a + roundabout, rather than one instruction for the whole + maneuver. Requires `steps=true`. + schema: + type: boolean + - name: "voice_instructions" + in: query + required: false + description: >- + Return SSML-marked-up voice guidance text. Requires + `steps=true`. + schema: + type: boolean + - name: "voice_units" + in: query + required: false + description: >- + Units for voice instructions. Requires `steps=true` and + `voice_instructions=true`. + schema: + type: string + enum: ["imperial", "british_imperial", "metric"] + - name: "waypoints" + in: query + required: false + description: >- + Zero-based indices into `coordinates` marking which ones get + their own arrival instruction — must include `0` and the last + index. Requires `steps=true`. + schema: + type: string + example: "0,2" + - name: "waypoints_per_route" + in: query + required: false + description: >- + Nest each route's waypoints under that route object instead of + once at the top level. + schema: + type: boolean + - name: "waypoint_names" + in: query + required: false + description: >- + A name per waypoint, semicolon-separated, used in that + waypoint's arrival instruction instead of the road name. + Requires `steps=true`. + schema: + type: string + - name: "waypoint_targets" + in: query + required: false + description: >- + A `{longitude},{latitude}` drop-off point per waypoint, + semicolon-separated, when it differs from the routed-to + coordinate. Requires `steps=true`. + schema: + type: string + - name: "notifications" + in: query + required: false + description: "Whether to return route notification/warning metadata." + schema: + type: string + enum: ["all", "none"] + - name: "alley_bias" + in: query + required: false + description: >- + `mapbox/driving` only. -1 to 1: bias the route against (negative) + or toward (positive) alleys. + schema: + type: number + minimum: -1 + maximum: 1 + - name: "arrive_by" + in: query + required: false + description: >- + `mapbox/driving` only. Desired arrival time, ISO 8601, for + time-dependent routing. + schema: + type: string + - name: "depart_at" + in: query + required: false + description: >- + `mapbox/driving` and `mapbox/driving-traffic`. Departure time, + ISO 8601 — for `mapbox/driving` this drives time-dependent + routing; for `mapbox/driving-traffic` it selects which live + traffic conditions to route against. + schema: + type: string + - name: "max_height" + in: query + required: false + description: >- + `mapbox/driving` and `mapbox/driving-traffic`. Vehicle height in + meters, 0-10. Defaults to 1.6. + schema: + type: number + minimum: 0 + maximum: 10 + - name: "max_width" + in: query + required: false + description: >- + `mapbox/driving` and `mapbox/driving-traffic`. Vehicle width in + meters, 0-10. Defaults to 1.9. + schema: + type: number + minimum: 0 + maximum: 10 + - name: "max_weight" + in: query + required: false + description: >- + `mapbox/driving` and `mapbox/driving-traffic`. Vehicle weight in + metric tons, 0-100. Defaults to 2.5. + schema: + type: number + minimum: 0 + maximum: 100 + - name: "snapping_include_closures" + in: query + required: false + description: >- + `mapbox/driving-traffic` only. Allow snapping to roads carrying + a live-traffic closure. + schema: + type: boolean + - name: "snapping_include_static_closures" + in: query + required: false + description: >- + `mapbox/driving-traffic` only. Allow snapping to roads carrying + a long-term closure. + schema: + type: boolean + - name: "walking_speed" + in: query + required: false + description: >- + `mapbox/walking` only. Walking speed in meters/second, + 0.14-6.94. Defaults to 1.42. + schema: + type: number + minimum: 0.14 + maximum: 6.94 + - name: "walkway_bias" + in: query + required: false + description: >- + `mapbox/walking` only. -1 to 1: bias the route against + (negative) or toward (positive) walkways/footpaths. + schema: + type: number + minimum: -1 + maximum: 1 + responses: + "200": + description: >- + A JSON object with a `code`, a `routes` array (each with + `duration`, `distance`, `geometry`, `legs`, and — when + `steps=true` — turn-by-turn `steps`), and a `waypoints` array. + "400": + description: Bad Request + "401": + description: Unauthorized + "403": + description: Forbidden + "404": + description: Not Found + "422": + description: >- + Unprocessable Entity — a well-formed request `code` can still + report as `NoRoute`, `NoSegment`, `ProfileNotFound` or + `InvalidInput` in its JSON body rather than as an HTTP error; + this status covers a request rejected outright, e.g. more than + 25 coordinates. diff --git a/docs/commands.md b/docs/commands.md index 972f0c5..899fdfa 100644 --- a/docs/commands.md +++ b/docs/commands.md @@ -71,6 +71,9 @@ nests, and is typed `mapbox styles draft get`. [accounts.retrieve-token](#mapbox-accounts-retrieve-token) · [accounts.list-scopes](#mapbox-accounts-list-scopes) +**[Directions](#directions)** — +[directions.route](#mapbox-directions-route) + **[Fonts](#fonts)** — [fonts.list](#mapbox-fonts-list) · [fonts.upload](#mapbox-fonts-upload) · [fonts.delete](#mapbox-fonts-delete) @@ -802,6 +805,131 @@ styles:list List styles. What this lists is what the account is *allowed* to hold, which is not the same as what the current token holds — `retrieve-token` answers that. +--- +## Directions + +Turn-by-turn routes between 2-25 waypoints, for driving (with or without +live traffic), walking, or cycling. Curated by hand down to the parameters +documented at docs.mapbox.com/api/navigation/directions — see +`custom-openapi/README.md` for why this command group doesn't come from the +vendored specs the way most others do. The ~30 electric-vehicle-routing +parameters (`engine=electric` and everything under it) are deliberately not +here: those describe a vehicle's charging curve down to the watt, which is +data an integration passes in from a vehicle profile, not something to +hand-type as CLI flags. + +### `mapbox directions route` + +A route between 2-25 waypoints, in the order given — a route through fixed +stops, not a traveling-salesman solve (see the future Optimization API +command for that). + +#### Parameters + +`` and `` (both positional) are required. +`` is one of `mapbox/driving-traffic`, `mapbox/driving`, +`mapbox/walking`, `mapbox/cycling`. `` is 2-25 +`{longitude},{latitude}` pairs, semicolon-separated. + +| Parameter | Effect | +| --- | --- | +| `--alternatives` | Return up to 2 alternative routes alongside the primary one. | +| `--annotations ` | Segment-level metadata per leg, comma-separated (`distance`, `duration`, `speed`, `congestion`, `congestion_numeric`, `maxspeed`, `closure`, `state_of_charge`). Requires `--overview full`. | +| `--avoid-maneuver-radius <1-1000>` | Meters around the start to avoid a significant maneuver within. | +| `--bearings ` | Filter road segments by direction of travel, one entry per coordinate. | +| `--layers ` | A road layer (Z-order) per waypoint, for multi-level roads. | +| `--continue-straight` | Keep going straight at an intermediate waypoint rather than u-turning back to it. | +| `--exclude ` | Road types or `point(lon lat)` values to route around, comma-separated (`motorway`, `toll`, `ferry`, `unpaved`, `cash_only_tolls`, `country_border`, `state_border`, `tunnel`). | +| `--geometries ` | Route geometry format. Defaults to `polyline`. | +| `--include ` | Special road types to allow, comma-separated (`hov2`, `hov3`, `hot`). | +| `--overview ` | Geometry detail level. Defaults to `simplified`. | +| `--radiuses ` | Max snap distance to the road network, one per coordinate. | +| `--approaches ` | Which side of the road to approach each waypoint from. | +| `--steps` | Return turn-by-turn instructions. Several flags below only take effect with this set. | +| `--banner-instructions` | Return banner objects for display. Requires `--steps`. | +| `--language ` | Instruction language. Defaults to `en`. Requires `--steps`. | +| `--roundabout-exits` | Separate entry/exit instructions for a roundabout. Requires `--steps`. | +| `--voice-instructions` | Return SSML-marked voice guidance. Requires `--steps`. | +| `--voice-units ` | Requires `--steps` and `--voice-instructions`. | +| `--waypoints ` | Which coordinates get their own arrival instruction — must include `0` and the last index. Requires `--steps`. | +| `--waypoints-per-route` | Nest each route's waypoints under that route object instead of once at the top level. | +| `--waypoint-names ` | A name per waypoint for its arrival instruction. Requires `--steps`. | +| `--waypoint-targets ` | A drop-off point per waypoint, when it differs from the routed-to coordinate. Requires `--steps`. | +| `--notifications ` | Whether to return route notification/warning metadata. | +| `--alley-bias <-1..1>` | `mapbox/driving` only. | +| `--arrive-by ` | `mapbox/driving` only. | +| `--depart-at ` | `mapbox/driving` and `mapbox/driving-traffic`. | +| `--max-height <0-10>` / `--max-width <0-10>` / `--max-weight <0-100>` | `mapbox/driving` and `mapbox/driving-traffic`. Meters, meters, metric tons. | +| `--snapping-include-closures` / `--snapping-include-static-closures` | `mapbox/driving-traffic` only. | +| `--walking-speed <0.14-6.94>` / `--walkway-bias <-1..1>` | `mapbox/walking` only. | + +#### Examples + +```sh +mapbox directions route mapbox/driving "-122.42,37.78;-122.45,37.91" +mapbox directions route mapbox/walking "-122.42,37.78;-122.43,37.79" \ + --steps --geometries geojson --overview full --annotations distance,duration +``` + +The negative longitude is not treated as a flag here, even without `--` +before it — `coordinates` is one of the parameters this CLI recognizes a +leading `-` on and lets through, the same way `search`'s `--proximity` and +`--bbox` already do (see `HYPHEN_LEADING_VALUE_PARAMS` in `src/main.rs`). + +#### Outputs + +Captured live against `mapbox/driving` between two San Francisco points. +`routes`/`legs`/`waypoints` are nested arrays of objects, which +`output::emit`'s text mode has no bespoke summary for (unlike the GeoJSON +`FeatureCollection` responses `search` and `geocoder` return) — both modes +print the same JSON, `-o text` pretty-printed and `-o json` on one line: + + + + +
Terminal — -o textAgent — -o json
+ +```json +{ + "code": "Ok", + "routes": [ + { + "distance": 26966.74, + "duration": 2552.258, + "geometry": "{{qeFvdejVqdChZ…", + "legs": [ + { + "distance": 26966.74, + "duration": 2552.258, + "steps": [], + "summary": "US 101 North, Paradise Drive", + "weight": 3068.861 + } + ], + "weight": 3068.861, + "weight_name": "auto" + } + ], + "uuid": "…", + "waypoints": [ + { "distance": 11.034, "location": [-122.420122, 37.779978], "name": "US 101 North" }, + { "distance": 1820.457, "location": [-122.453429, 37.893872], "name": "" } + ] +} +``` + + + +```json +{"code":"Ok","routes":[{"distance":26966.74,"duration":2552.258,"geometry":"{{qeFvdejVqdChZ…","legs":[{"distance":26966.74,"duration":2552.258,"steps":[],"summary":"US 101 North, Paradise Drive","weight":3068.861}],"weight":3068.861,"weight_name":"auto"}],"uuid":"…","waypoints":[{"distance":11.034,"location":[-122.420122,37.779978],"name":"US 101 North"},{"distance":1820.457,"location":[-122.453429,37.893872],"name":""}]} +``` + +
+ +Both trimmed to one leg for length — the real response also carries +`admins` (administrative boundaries traversed) and `notifications` (three +tunnel alerts, on this particular route) per leg. + --- ## Fonts diff --git a/src/executor.rs b/src/executor.rs index 7bdadff..d130f7f 100644 --- a/src/executor.rs +++ b/src/executor.rs @@ -151,7 +151,7 @@ fn dispatch( for param in &op.path_params { if let Some(val) = matches.get_one::(¶m.arg_name) { - let safe = path_segment(¶m.name, val)?; + let safe = path_segment_for(param, val)?; path = substitute_path_param(&path, ¶m.name, &safe, param.required); } } @@ -1219,6 +1219,27 @@ fn path_segment<'a>(name: &str, value: &'a str) -> Result> { Ok(Cow::Owned(encoded)) } +/// [`path_segment`], skipped for a parameter clap has already constrained to +/// one of a fixed set of literal strings. +/// +/// `path_segment`'s escaping exists for a value nobody has constrained — see +/// its own doc comment. An enum-valued parameter is different: clap's +/// `PossibleValuesParser` has already limited it to one of the spec's own +/// literal strings before this runs, so there is nothing left for a caller +/// to smuggle in. That distinction matters here specifically: +/// `directions.yaml`'s `profile` enum is `mapbox/driving`, `mapbox/walking`, +/// etc. — one path parameter whose only valid values contain a literal `/`, +/// which the API's own routing depends on reaching it unescaped. Encoding it +/// to `%2F` is exactly the request-redirection fix `path_segment` exists +/// for, misapplied to a value that was never free text. +fn path_segment_for<'a>(param: &Parameter, value: &'a str) -> Result> { + if param.enum_values.is_empty() { + path_segment(¶m.name, value) + } else { + Ok(Cow::Borrowed(value)) + } +} + fn substitute_path_param(path: &str, name: &str, value: &str, required: bool) -> String { let placeholder = format!("{{{name}}}"); let segment = format!("/{placeholder}"); @@ -1402,8 +1423,8 @@ fn write_binary(body: &[u8], content_type: &str) -> Result<()> { mod tests { use super::{ describe_body, empty_success_line, extra_query_from_env, file_name_of, - is_binary_content_type, part_media_type, path_segment, payload_of, query_pairs, - redacted_url, request_id, resolve_body_source, resolve_data, shell_value, + is_binary_content_type, part_media_type, path_segment, path_segment_for, payload_of, + query_pairs, redacted_url, request_id, resolve_body_source, resolve_data, shell_value, substitute_path_param, with_page_context, BodySource, NextPage, ResponseHeaders, ACCESS_TOKEN, EXTRA_QUERY_ENV, REQUEST_ID_HEADERS, }; @@ -2527,4 +2548,27 @@ mod tests { "/styles/v1/{u}/{id}" ); } + + /// `directions.yaml`'s `profile` is exactly this shape: a path parameter + /// whose only valid values, `mapbox/driving` and friends, carry a + /// literal `/` the API's routing depends on. Plain `path_segment` would + /// encode it to `%2F` and 404 — this is why `path_segment_for` exists. + #[test] + fn an_enum_valued_path_parameter_keeps_its_slash() { + let mut profile = param("profile"); + profile.enum_values = vec!["mapbox/driving".to_string(), "mapbox/walking".to_string()]; + + let safe = path_segment_for(&profile, "mapbox/driving").expect("not refused"); + assert_eq!(safe, "mapbox/driving", "the slash must survive, unencoded"); + } + + /// The bypass is keyed on the parameter carrying an enum, not on its + /// name or its value's shape — a non-enum parameter goes through the + /// same escaping as ever, `/` included. + #[test] + fn a_non_enum_path_parameter_is_still_escaped() { + let style_id = param("style_id"); + let safe = path_segment_for(&style_id, "../../tokens/v2/victim").expect("encoded"); + assert_eq!(safe, "..%2F..%2Ftokens%2Fv2%2Fvictim"); + } } diff --git a/src/main.rs b/src/main.rs index a3a7961..b8227bc 100644 --- a/src/main.rs +++ b/src/main.rs @@ -116,9 +116,14 @@ fn help_text(param: &spec::Parameter) -> String { /// `allow_negative_numbers`, set on every generated command, only recognizes /// a value that is *itself* a number — `-74.0`, not `-121.9,37.4`. A /// comma-joined pair still reads as a cluster of short flags, which is what -/// made `--proximity -121.9,37.4` unusable west of Greenwich. These four are -/// every parameter across the twelve specs that takes one. -const HYPHEN_LEADING_VALUE_PARAMS: &[&str] = &["proximity", "bbox", "near", "origin"]; +/// made `--proximity -121.9,37.4` unusable west of Greenwich. These five are +/// every parameter across the specs that takes one — `coordinates` is a +/// positional rather than a flag (`directions.yaml`'s path parameter), but +/// the same shape and the same failure: `mapbox directions route +/// mapbox/driving -122.42,37.78` read `-122.42,37.78` as an unrecognized +/// flag before this was added. +const HYPHEN_LEADING_VALUE_PARAMS: &[&str] = + &["proximity", "bbox", "near", "origin", "coordinates"]; /// Whether clap should accept a `-`-leading value for this parameter. /// diff --git a/src/remedy.rs b/src/remedy.rs index b1d1b56..7520844 100644 --- a/src/remedy.rs +++ b/src/remedy.rs @@ -75,6 +75,10 @@ impl Remedy { // `every_documentation_page_belongs_to_a_service`. const SERVICE_DOCS: &[(&str, &str)] = &[ ("accounts", TOKENS_DOC), + ( + "directions", + "https://docs.mapbox.com/api/navigation/directions/", + ), ("fonts", "https://docs.mapbox.com/api/maps/fonts/"), ( "geocoder", diff --git a/src/schema.rs b/src/schema.rs index d49d655..05abdf1 100644 --- a/src/schema.rs +++ b/src/schema.rs @@ -420,6 +420,12 @@ fn api_command(app: &Command, svc: &ServiceSpec, op: &Operation) -> CommandEntry required: true, location: Some("path"), omit_with_empty: omits_with_empty(param, &op.path_template), + // Named as well as filled, the same reason `username_argument` + // is: `ARG_NAME_OVERRIDES` (`directions.yaml`'s `profile`, kept + // apart from the global `--profile`) means an argument's own + // name can differ from the `{placeholder}` it fills, and an + // agent matching one against the other would find nothing. + fills: (param.arg_name != param.name).then(|| format!("{{{}}}", param.name)), ..parameter(param, ArgKind::Positional) }) .collect(); diff --git a/src/spec.rs b/src/spec.rs index 8e4a413..4dab2ab 100644 --- a/src/spec.rs +++ b/src/spec.rs @@ -81,6 +81,42 @@ pub struct Operation { /// deleting a row. const BODY_CONTENT_TYPE_OVERRIDES: &[(&str, &str, &str)] = &[("styles", "starFile", "text/plain")]; +/// (service, parameter name, the `arg_name` to use instead) for a parameter +/// whose spec name is also a global argument's id — `--profile`, `--token`, +/// `--username`, `--id`, `--output`, `--schema`, `--dry-run`, `--yes`, +/// `--timeout`, `--use-login`, `--debug`. +/// +/// `directions.yaml`'s path parameter is genuinely named `profile` — that is +/// the API's own name for it, and the path template substitutes on +/// [`Parameter::name`], not `arg_name`, so the spec can't just rename it. +/// But every `clap::Arg` is built from `arg_name` +/// (`build_operation_command`), and clap has one namespace of ids per +/// command: a second `Arg::new("profile")` on the same command silently +/// replaces the global one instead of erring, so the routing profile this +/// parameter means and the credentials profile the global flag means become +/// one and the same id, whichever definition happened to be added last winning +/// the help text while the *other* one's reader (`main.rs`, reading +/// `matches.get_one::("profile")` to pick a credentials file) still +/// runs — `mapbox directions route mapbox/driving …` failed with +/// `Invalid profile name "mapbox/driving"` this way before this table +/// existed. `tests/source_guards.rs`'s `no_generated_flag_shadows_a_global` +/// catches the `--flag`/`-short` half of this; it can't catch a positional, +/// since a positional has neither. +/// +/// Kept as a table rather than a branch, for the same reason +/// [`BODY_CONTENT_TYPE_OVERRIDES`] is: the fix sits next to the operation +/// it's for, and outgrowing a global name later is just deleting a row. +const ARG_NAME_OVERRIDES: &[(&str, &str, &str)] = &[("directions", "profile", "routing-profile")]; + +/// The `arg_name` a parameter should present as, when its spec name collides +/// with a global argument's id. See [`ARG_NAME_OVERRIDES`]. +fn arg_name_override(service_name: &str, param_name: &str) -> Option<&'static str> { + ARG_NAME_OVERRIDES + .iter() + .find(|(svc, name, _)| *svc == service_name && *name == param_name) + .map(|(_, _, arg_name)| *arg_name) +} + /// The media types an operation's request body may be sent as. /// /// This used to be a plain `has_body: bool`, which forced the executor to @@ -608,10 +644,16 @@ pub const MAPBOX_SPEC_ENTRIES: &[SpecEntry] = &[ /// name here wins over the same name in [`MAPBOX_SPEC_ENTRIES`]. Delete the /// override once upstream ships the service — a drift check flags a name /// wired on both sides, for exactly this reason. -pub const CUSTOM_SPEC_ENTRIES: &[SpecEntry] = &[SpecEntry { - name: "search", - yaml: include_str!("../custom-openapi/search/openapi/search.yaml"), -}]; +pub const CUSTOM_SPEC_ENTRIES: &[SpecEntry] = &[ + SpecEntry { + name: "search", + yaml: include_str!("../custom-openapi/search/openapi/search.yaml"), + }, + SpecEntry { + name: "directions", + yaml: include_str!("../custom-openapi/directions/openapi/directions.yaml"), + }, +]; /// The list the CLI actually generates commands from: [`MAPBOX_SPEC_ENTRIES`], /// with each [`CUSTOM_SPEC_ENTRIES`] override swapped in and the @@ -905,12 +947,15 @@ pub fn parse_spec(service_name: &str, yaml: &str) -> Result { let mut path_params = vec![]; let mut query_params = vec![]; - for p in op_params { + for mut p in op_params { // Auto-filled from the global `--username`; see // ACCOUNT_PLACEHOLDERS. if ACCOUNT_PLACEHOLDERS.contains(&p.name.as_str()) { continue; } + if let Some(arg_name) = arg_name_override(service_name, &p.name) { + p.arg_name = arg_name.to_string(); + } if path_str.contains(&format!("{{{}}}", p.name)) { path_params.push(p); } else { @@ -1953,4 +1998,58 @@ paths: assert!(aliases.is_empty()); assert!(hidden.is_empty()); } + + /// `directions.yaml`'s `profile` path parameter is a real collision with + /// the global `--profile` (credentials profile) argument's id — clap has + /// one namespace of ids per command, and the generated positional would + /// otherwise silently replace the global one. This is the regression + /// test for `mapbox directions route mapbox/driving …` failing with + /// `Invalid profile name "mapbox/driving"` before [`ARG_NAME_OVERRIDES`] + /// existed: the parsed parameter's `arg_name` must differ from the + /// global's id, while `name` stays `profile` so the path template's + /// `{profile}` placeholder still resolves. + #[test] + fn the_directions_profile_parameter_does_not_collide_with_the_global_flag() { + let spec = parse_spec( + "directions", + include_str!("../custom-openapi/directions/openapi/directions.yaml"), + ) + .expect("directions.yaml parses"); + + let route = spec + .operations + .iter() + .find(|op| op.command_path == ["route"]) + .expect("the route operation exists"); + + let profile = route + .path_params + .iter() + .find(|p| p.name == "profile") + .expect("a path parameter named profile"); + + assert_ne!( + profile.arg_name, "profile", + "must not collide with the global --profile id" + ); + assert_eq!( + profile.enum_values, + [ + "mapbox/driving-traffic", + "mapbox/driving", + "mapbox/walking", + "mapbox/cycling" + ] + ); + } + + #[test] + fn arg_name_override_only_fires_for_the_row_it_names() { + assert_eq!( + arg_name_override("directions", "profile"), + Some("routing-profile") + ); + assert_eq!(arg_name_override("directions", "coordinates"), None); + assert_eq!(arg_name_override("styles", "profile"), None); + } } diff --git a/tests/fixtures/api_command_surface.txt b/tests/fixtures/api_command_surface.txt index 9dd4675..6885a38 100644 --- a/tests/fixtures/api_command_surface.txt +++ b/tests/fixtures/api_command_surface.txt @@ -1,6 +1,7 @@ mapbox accounts list-scopes | aliases: (none) mapbox accounts list-tokens | aliases: (none) mapbox accounts retrieve-token | aliases: (none) +mapbox directions route | aliases: (none) mapbox fonts delete | aliases: (none) mapbox fonts list | aliases: (none) mapbox fonts upload | aliases: (none) From dda57aac89a79256a0f1863d7475e35fbeb34244 Mon Sep 17 00:00:00 2001 From: Matthew Podwysocki Date: Wed, 23 Sep 2026 18:32:29 -0400 Subject: [PATCH 2/3] Reorder profile-scoped descriptions so --help doesn't cut them short MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit first_sentence() in src/main.rs cuts a --help line at the first '.', which is right after "`mapbox/driving` only" or "`mapbox/driving` and `mapbox/driving-traffic`" on eight parameters here — a real sentence on its own that was eating everything substantive that followed it in --help. --schema and docs/commands.md were never affected; they show the full text. Fixed by moving the profile-scope note to the end of each description instead of the front, so the useful part survives the cut. Co-Authored-By: Claude Sonnet 5 --- .../directions/openapi/directions.yaml | 49 ++++++++++--------- 1 file changed, 27 insertions(+), 22 deletions(-) diff --git a/custom-openapi/directions/openapi/directions.yaml b/custom-openapi/directions/openapi/directions.yaml index 75a665f..c95f2e3 100644 --- a/custom-openapi/directions/openapi/directions.yaml +++ b/custom-openapi/directions/openapi/directions.yaml @@ -286,9 +286,15 @@ paths: - name: "alley_bias" in: query required: false + # Content before the profile note, not after: `first_sentence` in + # `src/main.rs` cuts a `--help` line at the first `.`, and + # `` `mapbox/driving` only. `` — a real sentence on its own — was + # eating everything that followed it. `--schema` and + # `docs/commands.md` show the description in full either way — + # this ordering is only for the one-line case. description: >- - `mapbox/driving` only. -1 to 1: bias the route against (negative) - or toward (positive) alleys. + Bias the route against (negative) or toward (positive) alleys, + -1 to 1. `mapbox/driving` only. schema: type: number minimum: -1 @@ -297,26 +303,25 @@ paths: in: query required: false description: >- - `mapbox/driving` only. Desired arrival time, ISO 8601, for - time-dependent routing. + Desired arrival time, ISO 8601, for time-dependent routing. + `mapbox/driving` only. schema: type: string - name: "depart_at" in: query required: false description: >- - `mapbox/driving` and `mapbox/driving-traffic`. Departure time, - ISO 8601 — for `mapbox/driving` this drives time-dependent - routing; for `mapbox/driving-traffic` it selects which live - traffic conditions to route against. + Departure time, ISO 8601 — for `mapbox/driving` this drives + time-dependent routing; for `mapbox/driving-traffic` it selects + which live traffic conditions to route against. Both profiles. schema: type: string - name: "max_height" in: query required: false description: >- - `mapbox/driving` and `mapbox/driving-traffic`. Vehicle height in - meters, 0-10. Defaults to 1.6. + Vehicle height in meters, 0-10, defaulting to 1.6. + `mapbox/driving` and `mapbox/driving-traffic` only. schema: type: number minimum: 0 @@ -325,8 +330,8 @@ paths: in: query required: false description: >- - `mapbox/driving` and `mapbox/driving-traffic`. Vehicle width in - meters, 0-10. Defaults to 1.9. + Vehicle width in meters, 0-10, defaulting to 1.9. + `mapbox/driving` and `mapbox/driving-traffic` only. schema: type: number minimum: 0 @@ -335,8 +340,8 @@ paths: in: query required: false description: >- - `mapbox/driving` and `mapbox/driving-traffic`. Vehicle weight in - metric tons, 0-100. Defaults to 2.5. + Vehicle weight in metric tons, 0-100, defaulting to 2.5. + `mapbox/driving` and `mapbox/driving-traffic` only. schema: type: number minimum: 0 @@ -345,24 +350,24 @@ paths: in: query required: false description: >- - `mapbox/driving-traffic` only. Allow snapping to roads carrying - a live-traffic closure. + Allow snapping to roads carrying a live-traffic closure. + `mapbox/driving-traffic` only. schema: type: boolean - name: "snapping_include_static_closures" in: query required: false description: >- - `mapbox/driving-traffic` only. Allow snapping to roads carrying - a long-term closure. + Allow snapping to roads carrying a long-term closure. + `mapbox/driving-traffic` only. schema: type: boolean - name: "walking_speed" in: query required: false description: >- - `mapbox/walking` only. Walking speed in meters/second, - 0.14-6.94. Defaults to 1.42. + Walking speed in meters/second, 0.14-6.94, defaulting to 1.42. + `mapbox/walking` only. schema: type: number minimum: 0.14 @@ -371,8 +376,8 @@ paths: in: query required: false description: >- - `mapbox/walking` only. -1 to 1: bias the route against - (negative) or toward (positive) walkways/footpaths. + Bias the route against (negative) or toward (positive) + walkways/footpaths, -1 to 1. `mapbox/walking` only. schema: type: number minimum: -1 From 63629b307c7aec2ef1719e08ddc2d01f79ec4bdc Mon Sep 17 00:00:00 2001 From: Matthew Podwysocki Date: Fri, 25 Sep 2026 10:40:39 -0400 Subject: [PATCH 3/3] Address review: flatten to mapbox directions, fix the profile enum MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two pieces of review feedback landed on this PR (danpat, and product guidance relayed separately), addressed together since both touch the same lines: 1. Product guidance was to drop the verb for single-operation Nav commands: mapbox directions instead of mapbox directions route, matching the shape mapbox usage already has. Generalized rather than hand-coded per service: spec::FLATTENED_SERVICES names which services get this, build_service_command attaches the one operation's own args directly to the service-level command instead of nesting it, and run()'s dispatch skips the subcommand walk for one of these. Every reader of a command's identity (--schema, docs/commands.md, generate-skills, remedy's suggestions) goes through Operation::command(), which is now flattening-aware, so they all render correctly for free. 2. Real bug, not just wording: the routing profile's enum (the four documented values) rejected anything else client-side via clap's PossibleValuesParser. Some accounts — OEM agreements, mainly — have additional profiles never published to docs.mapbox.com, so the enum would have broken this command for exactly the customers who need a profile named beyond driving/walking/cycling. Reported in review. profile is now a free-form string, sent exactly as typed. Removing the enum broke the mechanism that kept profile's literal `/` from being percent-encoded — that fix was keyed on "does this parameter have an enum", which stopped being true. Replaced with a named table, spec::UNESCAPED_PATH_PARAMS, keyed on (service, parameter name) instead: the same outcome, but as an explicit decision rather than a side effect of a constraint that needed removing for an unrelated reason. Also corrected a factual error a reviewer caught in this PR's own explanatory text: EV Charge Finder is a location-search product (charging stations near a point), not related to the Directions API's own engine=electric vehicle-profile parameters that were excluded — the two are unrelated capabilities, not alternatives for the same thing. Fixed everywhere that sentence appeared (custom-openapi/directions.yaml, CHANGELOG.md). 487 tests, fmt and clippy clean. Two new regression tests (a_flattened_service_takes_no_subcommand, command_only_drops_the_path_for_a_flattened_service) plus the existing path-escaping tests rewritten for the table-based mechanism instead of the enum-based one. Co-Authored-By: Claude Sonnet 5 --- CHANGELOG.md | 39 +++-- README.md | 6 +- .../directions/openapi/directions.yaml | 38 +++-- docs/commands.md | 23 +-- src/api_command_surface.rs | 38 +++-- src/executor.rs | 75 +++++---- src/main.rs | 158 +++++++++++++----- src/schema.rs | 19 ++- src/spec.rs | 96 +++++++++-- tests/fixtures/api_command_surface.txt | 2 +- 10 files changed, 346 insertions(+), 148 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 54cb4d3..edd8e26 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,24 +19,33 @@ that may never merge. They are not releases and are not listed here. ## Unreleased -- `mapbox directions route`, routes between 2-25 waypoints for driving (with - or without live traffic), walking, or cycling. Hand-authored into - `custom-openapi/` rather than waiting on an upstream spec — the whole +- `mapbox directions`, routes between 2-25 waypoints for driving (with + or without live traffic), walking, or cycling. No subcommand: this API + has one operation, so — like `mapbox usage` — there's nothing a second + word would disambiguate; see `spec::FLATTENED_SERVICES`. Hand-authored + into `custom-openapi/` rather than waiting on an upstream spec — the whole Navigation API category had no CLI coverage before this; excludes the - ~30 electric-vehicle-routing parameters - (`engine=electric` and everything under it), which describe a vehicle's - charging curve down to the watt and are a poor fit for a hand-typed CLI - flag — the EV Charge Finder API is a better fit for that case and its own - follow-up. Two path-parameter bugs surfaced while wiring this up and are - fixed for every command, not just this one: a spec parameter literally - named `profile` (the routing profile, `mapbox/driving` etc.) silently + ~30 electric-vehicle-routing parameters (`engine=electric` and everything + under it), which describe one vehicle's charge/discharge curve down to + the watt and are a poor fit for a hand-typed CLI flag — left for a + follow-up. + + The routing profile (`mapbox/driving` etc.) is a free-form value, not a + fixed set of four: an early version rejected anything else client-side, + which would have broken this command for exactly the accounts that most + need it, since some (OEM agreements, mainly) have additional profiles of + their own never published to docs.mapbox.com. Reported in review before + this shipped anywhere. Two path-parameter bugs surfaced while wiring the + original four up and are fixed for every command, not just this one: a + spec parameter literally named `profile` (the routing profile) silently collided with the global `--profile` credentials flag, since clap has one namespace of ids per command and a positional of the same name replaced it - outright; and an enum-constrained path parameter whose only valid values - contain a literal `/` (again, `mapbox/driving`) was being percent-encoded - to `%2F` by the same escaping that stops a free-text value from smuggling - in extra path segments — safe to skip once clap has already limited the - value to one of the spec's own literal strings. + outright; and a path parameter whose every legitimate value contains a + literal `/` (`mapbox/driving`) was being percent-encoded to `%2F` by the + same escaping that stops a free-text value from smuggling in extra path + segments — safe to skip for a parameter named in a small table + (`UNESCAPED_PATH_PARAMS`) as one whose values are trusted to carry that + character on purpose. - `MAPBOX_CLI_EXTRA_QUERY` appends raw query parameters to every request, in the same `k1=v1&k2=v2` shape as a URL's own query string — for an API diff --git a/README.md b/README.md index 1dd71af..6735806 100644 --- a/README.md +++ b/README.md @@ -168,7 +168,6 @@ Each API is a top-level subcommand, one sub-subcommand per operation: ```sh mapbox accounts * -mapbox directions * mapbox fonts * mapbox geocoder * mapbox search * @@ -178,6 +177,11 @@ mapbox styles * mapbox tilesets * ``` +`mapbox directions` is the one exception: its API has a single operation, +so there's a bare command with no subcommand at all, the same shape +`mapbox usage` already has — see [docs/commands.md](./docs/commands.md) for +its own parameters. + A command group is not the same thing as a spec file: which one an operation belongs to is decided per operation. So `sprites` and `tilesets` are each assembled from operations declared by the Styles, Raster Tiles and Vector diff --git a/custom-openapi/directions/openapi/directions.yaml b/custom-openapi/directions/openapi/directions.yaml index c95f2e3..1a1af28 100644 --- a/custom-openapi/directions/openapi/directions.yaml +++ b/custom-openapi/directions/openapi/directions.yaml @@ -8,12 +8,13 @@ openapi: "3.0.0" # docs.mapbox.com/api/navigation/directions: routes between 2-25 waypoints # for driving (with or without live traffic), walking, and cycling. Excludes # the ~30 electric-vehicle-routing parameters (`engine=electric` and -# everything under it) — those describe a vehicle's charging curve down to -# the watt, which is data an integration passes in from a vehicle profile, -# not something to hand-type as CLI flags. The EV Charge Finder API is -# Mapbox's dedicated per-vehicle routing product and the better fit for that -# case; this file can grow the EV parameters later if that changes. Also -# excludes POST — every other +# everything under it) — those describe one vehicle's charge/discharge +# curve down to the watt so this same Directions API can plan charging +# stops along the route, which is data an integration passes in from its +# own vehicle profile, not something to hand-type as CLI flags on every +# call. Left for a follow-up rather than the EV Charge Finder API, a +# different, separate product (finding charging stations near a point) that +# doesn't cover this. Also excludes POST — every other # operation this CLI wires up is one HTTP method per operation, and the spec # format here has no way to say "GET or POST, caller's choice" for the same # operationId. GET's 2-25 waypoint limit covers the CLI's own use (typed or @@ -45,18 +46,23 @@ paths: - name: "profile" in: path required: true - description: >- - The routing profile. `mapbox/driving-traffic` accounts for live - traffic conditions; `mapbox/driving` does not. + # Not an `enum`: the four documented values are what's public, but + # not what's exhaustive — some customers (OEM agreements, mainly) + # have additional profiles never published to docs.mapbox.com. + # An `enum` here becomes a clap `PossibleValuesParser` that + # rejects anything else client-side, which would break this CLI + # for exactly the accounts that most need a routing profile + # named beyond `driving`/`walking`/`cycling`. Reported in review. + description: >- + The routing profile — `mapbox/driving-traffic` (accounts for + live traffic), `mapbox/driving`, `mapbox/walking`, or + `mapbox/cycling` are documented, but not necessarily + exhaustive: some accounts have additional profiles of their + own. Sent exactly as typed; the API is the authority on + whether a value is valid, not this description. schema: type: string - enum: - [ - "mapbox/driving-traffic", - "mapbox/driving", - "mapbox/walking", - "mapbox/cycling", - ] + minLength: 1 example: "mapbox/driving" - name: "coordinates" in: path diff --git a/docs/commands.md b/docs/commands.md index 899fdfa..83e55a8 100644 --- a/docs/commands.md +++ b/docs/commands.md @@ -71,8 +71,7 @@ nests, and is typed `mapbox styles draft get`. [accounts.retrieve-token](#mapbox-accounts-retrieve-token) · [accounts.list-scopes](#mapbox-accounts-list-scopes) -**[Directions](#directions)** — -[directions.route](#mapbox-directions-route) +**[Directions](#directions)** — [directions](#mapbox-directions) **[Fonts](#fonts)** — [fonts.list](#mapbox-fonts-list) · [fonts.upload](#mapbox-fonts-upload) · [fonts.delete](#mapbox-fonts-delete) @@ -818,18 +817,22 @@ here: those describe a vehicle's charging curve down to the watt, which is data an integration passes in from a vehicle profile, not something to hand-type as CLI flags. -### `mapbox directions route` +### `mapbox directions` A route between 2-25 waypoints, in the order given — a route through fixed -stops, not a traveling-salesman solve (see the future Optimization API -command for that). +stops, not a traveling-salesman solve. No subcommand: this API has one +operation, so there's nothing a second word would disambiguate — the same +reason `mapbox usage` has none either. #### Parameters `` and `` (both positional) are required. -`` is one of `mapbox/driving-traffic`, `mapbox/driving`, -`mapbox/walking`, `mapbox/cycling`. `` is 2-25 -`{longitude},{latitude}` pairs, semicolon-separated. +`` is sent exactly as typed, not checked against a fixed +list: `mapbox/driving-traffic`, `mapbox/driving`, `mapbox/walking` and +`mapbox/cycling` are documented, but some accounts (OEM agreements, mainly) +have additional profiles of their own that were never published — the API +is the authority on whether a value is valid, not this page. `` +is 2-25 `{longitude},{latitude}` pairs, semicolon-separated. | Parameter | Effect | | --- | --- | @@ -866,8 +869,8 @@ command for that). #### Examples ```sh -mapbox directions route mapbox/driving "-122.42,37.78;-122.45,37.91" -mapbox directions route mapbox/walking "-122.42,37.78;-122.43,37.79" \ +mapbox directions mapbox/driving "-122.42,37.78;-122.45,37.91" +mapbox directions mapbox/walking "-122.42,37.78;-122.43,37.79" \ --steps --geometries geojson --overview full --annotations distance,duration ``` diff --git a/src/api_command_surface.rs b/src/api_command_surface.rs index c4f51a1..07a1a27 100644 --- a/src/api_command_surface.rs +++ b/src/api_command_surface.rs @@ -41,7 +41,7 @@ use std::path::PathBuf; use clap::Command; -use crate::spec::{effective_services, ServiceSpec}; +use crate::spec::{effective_services, ServiceSpec, FLATTENED_SERVICES}; const FIXTURE: &str = "tests/fixtures/api_command_surface.txt"; @@ -64,22 +64,26 @@ fn surface_lines(app: &Command, specs: &[ServiceSpec]) -> Vec { for svc in specs { for op in svc.operations.iter().filter(|op| op.is_exposed()) { // Down the whole command path, since one can nest: `styles draft - // get` is three levels from the root. - let cmd = op - .command_path - .iter() - .try_fold( - app.find_subcommand(&svc.name).unwrap_or_else(|| { - panic!("`mapbox {}` has operations and is not a command", svc.name) - }), - |cmd, segment| cmd.find_subcommand(segment), - ) - .unwrap_or_else(|| { - panic!( - "`mapbox {}` is an exposed operation and not a command", - op.command() - ) - }); + // get` is three levels from the root. A `FLATTENED_SERVICES` + // service has no subcommand to walk down to — its one operation + // *is* the service-level command — so the path is skipped for + // one of those, the same way `schema::declared_command` skips it. + let root = app.find_subcommand(&svc.name).unwrap_or_else(|| { + panic!("`mapbox {}` has operations and is not a command", svc.name) + }); + let cmd = if FLATTENED_SERVICES.contains(&svc.name.as_str()) { + root + } else { + op.command_path + .iter() + .try_fold(root, |cmd, segment| cmd.find_subcommand(segment)) + .unwrap_or_else(|| { + panic!( + "`mapbox {}` is an exposed operation and not a command", + op.command() + ) + }) + }; // `(hidden)` on the ones nothing publishes, so flipping a // `COMMAND_ALIASES` row's `show_generated_name` — which swaps a diff --git a/src/executor.rs b/src/executor.rs index d130f7f..accc149 100644 --- a/src/executor.rs +++ b/src/executor.rs @@ -9,7 +9,9 @@ use crate::http; use crate::link; use crate::output::{self, CliError, Mode}; use crate::remedy::{self, Remedy}; -use crate::spec::{Operation, Parameter, RequestBody, ACCOUNT_PLACEHOLDERS, MULTIPART}; +use crate::spec::{ + Operation, Parameter, RequestBody, ACCOUNT_PLACEHOLDERS, MULTIPART, UNESCAPED_PATH_PARAMS, +}; /// The query parameter the access token travels in, and what stands in for /// it anywhere the URL is shown. Named once because getting this wrong @@ -151,7 +153,7 @@ fn dispatch( for param in &op.path_params { if let Some(val) = matches.get_one::(¶m.arg_name) { - let safe = path_segment_for(param, val)?; + let safe = path_segment_for(&op.service, param, val)?; path = substitute_path_param(&path, ¶m.name, &safe, param.required); } } @@ -1219,24 +1221,27 @@ fn path_segment<'a>(name: &str, value: &'a str) -> Result> { Ok(Cow::Owned(encoded)) } -/// [`path_segment`], skipped for a parameter clap has already constrained to -/// one of a fixed set of literal strings. +/// [`path_segment`], skipped for a parameter named in +/// [`UNESCAPED_PATH_PARAMS`]. /// -/// `path_segment`'s escaping exists for a value nobody has constrained — see -/// its own doc comment. An enum-valued parameter is different: clap's -/// `PossibleValuesParser` has already limited it to one of the spec's own -/// literal strings before this runs, so there is nothing left for a caller -/// to smuggle in. That distinction matters here specifically: -/// `directions.yaml`'s `profile` enum is `mapbox/driving`, `mapbox/walking`, -/// etc. — one path parameter whose only valid values contain a literal `/`, -/// which the API's own routing depends on reaching it unescaped. Encoding it +/// `path_segment`'s escaping exists for a value nobody has vouched for — see +/// its own doc comment. This is what vouching looks like: a named decision +/// that every legitimate value of this specific parameter already contains +/// a character the escaping would otherwise mangle. `directions.yaml`'s +/// `profile` is the reason this exists — `mapbox/driving`, `mapbox/cycling`, +/// or an OEM account's own undocumented profile name, all sharing a literal +/// `/` the API's own routing depends on reaching it unescaped. Encoding it /// to `%2F` is exactly the request-redirection fix `path_segment` exists /// for, misapplied to a value that was never free text. -fn path_segment_for<'a>(param: &Parameter, value: &'a str) -> Result> { - if param.enum_values.is_empty() { - path_segment(¶m.name, value) - } else { +/// +/// Used to be inferred from the parameter having an `enum` instead of a +/// named table — see `UNESCAPED_PATH_PARAMS`'s own doc comment for why an +/// `enum` stopped being the right signal. +fn path_segment_for<'a>(service: &str, param: &Parameter, value: &'a str) -> Result> { + if UNESCAPED_PATH_PARAMS.contains(&(service, param.name.as_str())) { Ok(Cow::Borrowed(value)) + } else { + path_segment(¶m.name, value) } } @@ -2550,25 +2555,35 @@ mod tests { } /// `directions.yaml`'s `profile` is exactly this shape: a path parameter - /// whose only valid values, `mapbox/driving` and friends, carry a - /// literal `/` the API's routing depends on. Plain `path_segment` would - /// encode it to `%2F` and 404 — this is why `path_segment_for` exists. - #[test] - fn an_enum_valued_path_parameter_keeps_its_slash() { - let mut profile = param("profile"); - profile.enum_values = vec!["mapbox/driving".to_string(), "mapbox/walking".to_string()]; - - let safe = path_segment_for(&profile, "mapbox/driving").expect("not refused"); + /// whose every legitimate value, `mapbox/driving` and friends (plus + /// whatever an OEM account's own undocumented profiles are named), + /// carries a literal `/` the API's routing depends on. Plain + /// `path_segment` would encode it to `%2F` and 404 — this is why + /// `path_segment_for` exists. + #[test] + fn a_named_unescaped_path_parameter_keeps_its_slash() { + let profile = param("profile"); + let safe = path_segment_for("directions", &profile, "mapbox/driving").expect("not refused"); assert_eq!(safe, "mapbox/driving", "the slash must survive, unencoded"); } - /// The bypass is keyed on the parameter carrying an enum, not on its - /// name or its value's shape — a non-enum parameter goes through the - /// same escaping as ever, `/` included. + /// The bypass is keyed on `(service, name)` being in + /// `UNESCAPED_PATH_PARAMS`, not on the parameter's name alone or its + /// value's shape — the same parameter name on a different, unlisted + /// service goes through the same escaping as ever, `/` included. + #[test] + fn an_unlisted_service_still_escapes_the_same_parameter_name() { + let profile = param("profile"); + let safe = + path_segment_for("some-other-service", &profile, "a/b").expect("encoded, not refused"); + assert_eq!(safe, "a%2Fb"); + } + #[test] - fn a_non_enum_path_parameter_is_still_escaped() { + fn a_path_parameter_not_in_the_table_is_still_escaped() { let style_id = param("style_id"); - let safe = path_segment_for(&style_id, "../../tokens/v2/victim").expect("encoded"); + let safe = + path_segment_for("styles", &style_id, "../../tokens/v2/victim").expect("encoded"); assert_eq!(safe, "..%2F..%2Ftokens%2Fv2%2Fvictim"); } } diff --git a/src/main.rs b/src/main.rs index b8227bc..1325e49 100644 --- a/src/main.rs +++ b/src/main.rs @@ -347,6 +347,31 @@ fn attach_operations<'a>( } fn build_service_command(svc: &ServiceSpec) -> Command { + // Operations needing a scope nobody can hold are left out of the command + // surface entirely, so they answer exactly as a mistyped name does. An + // operation that can never succeed is not a feature to advertise, and a + // dedicated "this is disabled" reply told a caller which scopes exist + // without doing anything for them. See issue #9. + let exposed: Vec<&spec::Operation> = + svc.operations.iter().filter(|op| op.is_exposed()).collect(); + + // See `spec::FLATTENED_SERVICES`'s own doc comment for why this exists + // and what it changes: the operation's own command — same args, same + // `--dry-run`, same everything `build_operation_command` gives it — is + // the service-level command itself, renamed from its own generated name + // (`route`, say) to the service's (`directions`), rather than attached + // under it as a subcommand a caller has to name too. + if spec::FLATTENED_SERVICES.contains(&svc.name.as_str()) { + assert_eq!( + exposed.len(), + 1, + "`{}` is in FLATTENED_SERVICES but has {} exposed operations, not exactly one", + svc.name, + exposed.len() + ); + return build_operation_command(exposed[0]).name(svc.name.clone()); + } + // `subcommand_required` alone, not paired with `arg_required_else_help`. // The pairing used to give a bare `mapbox styles` the full help text — // but only when nothing had populated the global `--token` arg. That arg @@ -366,13 +391,6 @@ fn build_service_command(svc: &ServiceSpec) -> Command { cmd = cmd.long_about(desc.clone()); } - // Operations needing a scope nobody can hold are left out of the command - // surface entirely, so they answer exactly as a mistyped name does. An - // operation that can never succeed is not a feature to advertise, and a - // dedicated "this is disabled" reply told a caller which scopes exist - // without doing anything for them. See issue #9. - let exposed: Vec<&spec::Operation> = - svc.operations.iter().filter(|op| op.is_exposed()).collect(); attach_operations(cmd, &svc.name, &exposed, 0) } @@ -1116,43 +1134,59 @@ fn run(app: &Command, specs: &[ServiceSpec], matches: &ArgMatches, mode: Mode) - .find(|s| s.name == svc_name) .expect("unknown service"); - // Down to the leaf, since a command path may be more than one - // segment long — `styles draft get` is three matches deep. Every - // intermediate group sets `subcommand_required(true)`, so the - // walk can only stop on an operation. - let mut command_path: Vec = vec![]; - let mut op_matches = svc_matches; - while let Some((name, sub)) = op_matches.subcommand() { - command_path.push(name.to_string()); - op_matches = sub; - } + // A `FLATTENED_SERVICES` service has no subcommand to walk down + // to: `svc_matches` already carries the one operation's own + // args, parsed directly onto the service-level command + // `build_service_command` built. See that function and + // `spec::FLATTENED_SERVICES`'s own doc comment. + let (op, op_matches) = if spec::FLATTENED_SERVICES.contains(&svc_name) { + let op = svc + .operations + .iter() + .find(|o| o.is_exposed()) + .expect("a flattened service has exactly one exposed operation"); + (op, svc_matches) + } else { + // Down to the leaf, since a command path may be more than one + // segment long — `styles draft get` is three matches deep. + // Every intermediate group sets `subcommand_required(true)`, + // so the walk can only stop on an operation. + let mut command_path: Vec = vec![]; + let mut op_matches = svc_matches; + while let Some((name, sub)) = op_matches.subcommand() { + command_path.push(name.to_string()); + op_matches = sub; + } - if command_path.is_empty() { - // `subcommand_required` should have caught this; saying so - // beats the silent exit 0 that a gap here used to produce. - return Err(CliError::new( - "missing_subcommand", - format!( - "`mapbox {svc_name}` needs an operation. Run `mapbox {svc_name} --help`." - ), - ) - // Same suggestion the clap path attaches, for the same - // code — see `help_for_missing_subcommand`. - .with_remedy( - Remedy::default().with_action(Some(format!("mapbox {svc_name} --help"))), - ) - .into()); - } + if command_path.is_empty() { + // `subcommand_required` should have caught this; saying so + // beats the silent exit 0 that a gap here used to produce. + return Err(CliError::new( + "missing_subcommand", + format!( + "`mapbox {svc_name}` needs an operation. Run `mapbox {svc_name} --help`." + ), + ) + // Same suggestion the clap path attaches, for the same + // code — see `help_for_missing_subcommand`. + .with_remedy( + Remedy::default().with_action(Some(format!("mapbox {svc_name} --help"))), + ) + .into()); + } - // Read before the credentials are touched, which is the whole - // reason the operation is resolved first: refreshing spends a - // single-use refresh token and rewrites the credentials file, and - // a command that promised to change nothing must not do that. - let op = svc - .operations - .iter() - .find(|o| o.command_path == command_path) - .expect("unknown operation"); + // Read before the credentials are touched, which is the whole + // reason the operation is resolved first: refreshing spends a + // single-use refresh token and rewrites the credentials file, + // and a command that promised to change nothing must not do + // that. + let op = svc + .operations + .iter() + .find(|o| o.command_path == command_path) + .expect("unknown operation"); + (op, op_matches) + }; // Ahead of the credentials for the same reason `dry_run` is read // ahead of them: `load_fresh_credentials` spends a single-use @@ -1882,6 +1916,39 @@ mod tests { spec::effective_services().expect("the bundled specs parse") } + /// A `FLATTENED_SERVICES` service takes its one operation's own args + /// directly, with no subcommand — and the old two-word form is gone, + /// not merely hidden: `route` there is read as this positional + /// spec's own value, which doesn't match `mapbox/driving-traffic`'s + /// enum, so it fails exactly the way an unrecognized profile would. + #[test] + fn a_flattened_service_takes_no_subcommand() { + let specs = bundled_specs(); + let app = build_app(&specs); + + let bare = app.clone().try_get_matches_from([ + "mapbox", + "directions", + "mapbox/driving", + "-122.42,37.78;-122.45,37.91", + "--schema", + ]); + assert!(bare.is_ok(), "the flattened form must parse: {bare:?}"); + + let with_the_old_subcommand = app.try_get_matches_from([ + "mapbox", + "directions", + "route", + "mapbox/driving", + "-122.42,37.78;-122.45,37.91", + ]); + assert!( + with_the_old_subcommand.is_err(), + "`route` is not a subcommand any more — it must fail to parse, \ + not silently resolve to something else" + ); + } + /// A spec that says `enum: [created, modified]` used to say so only in /// the help text, so a typo travelled to the API and came back as /// whatever that endpoint says about bad input — often a 404 blaming @@ -2144,7 +2211,12 @@ mod tests { op: &spec::Operation, ) -> ArgMatches { let mut argv = vec!["mapbox".to_string(), svc.name.clone()]; - argv.extend(op.command_path.iter().cloned()); + // A `FLATTENED_SERVICES` service has no subcommand to type — its one + // operation's args are parsed directly onto the service-level + // command, the same reason `run`'s own dispatch skips this walk. + if !spec::FLATTENED_SERVICES.contains(&svc.name.as_str()) { + argv.extend(op.command_path.iter().cloned()); + } argv.extend(op.path_params.iter().map(a_value_for)); // `--schema` so the relaxed tree answers. What matters here is the // positionals, not whether the rest of the line is complete. diff --git a/src/schema.rs b/src/schema.rs index 05abdf1..232db24 100644 --- a/src/schema.rs +++ b/src/schema.rs @@ -25,7 +25,10 @@ use clap::{Arg, ArgAction, ArgMatches, Command}; use crate::generate_skills; use crate::output::{self, Mode}; -use crate::spec::{Numeric, Operation, Parameter, RequestBody, ServiceSpec, ACCOUNT_PLACEHOLDERS}; +use crate::spec::{ + Numeric, Operation, Parameter, RequestBody, ServiceSpec, ACCOUNT_PLACEHOLDERS, + FLATTENED_SERVICES, +}; use crate::tilesets_cli; /// The `--schema` flag's arg id, and its long spelling. @@ -396,12 +399,18 @@ fn commands(app: &Command, specs: &[ServiceSpec], path: &[String]) -> Vec(app: &'a Command, service: &str, path: &[String]) -> Option<&'a Command> { + let root = app.find_subcommand(service)?; + if FLATTENED_SERVICES.contains(&service) { + return Some(root); + } path.iter() - .try_fold(app.find_subcommand(service)?, |cmd, segment| { - cmd.find_subcommand(segment) - }) + .try_fold(root, |cmd, segment| cmd.find_subcommand(segment)) } fn api_command(app: &Command, svc: &ServiceSpec, op: &Operation) -> CommandEntry { diff --git a/src/spec.rs b/src/spec.rs index 4dab2ab..b1ade70 100644 --- a/src/spec.rs +++ b/src/spec.rs @@ -117,6 +117,55 @@ fn arg_name_override(service_name: &str, param_name: &str) -> Option<&'static st .map(|(_, _, arg_name)| *arg_name) } +/// A service with exactly one operation, typed with no subcommand at all — +/// `mapbox directions `, not `mapbox directions route `. +/// +/// Every other multi-operation service needs ` ` to say +/// which of several things to do; a one-operation service has nothing to +/// disambiguate, and naming the single operation anyway is a word the +/// caller has to know and type for no information it carries. `mapbox +/// usage` already has this shape, hand-written outside the generic +/// pipeline because it predates this table; these reuse the mechanism the +/// generic pipeline builds every other command through instead of adding a +/// second hand-written command per service. +/// +/// Reached for deliberately, not inferred from "this service happens to +/// have one operation": a future service could have exactly one operation +/// and still read better with it named (a first operation before a second +/// is added, say). Listed here is a decision, the same way +/// `ARG_NAME_OVERRIDES` and `BODY_CONTENT_TYPE_OVERRIDES` are tables of +/// decisions rather than something inferred from shape alone. +/// +/// `build_service_command` and `run`'s dispatch in `main.rs` are the two +/// places this changes anything: attaching the operation directly onto the +/// service-level `Command` instead of as a subcommand, and skipping the +/// subcommand walk that would otherwise expect one. Every other reader of a +/// command's identity — `--schema`, `docs/commands.md`, +/// `generate-skills`, this file's own `command()` above — reads a +/// [`FLATTENED_SERVICES`] service correctly for free, because they all go +/// through `command()` rather than reconstructing the string themselves. +pub const FLATTENED_SERVICES: &[&str] = &["directions"]; + +/// (service, path parameter name) pairs whose value is trusted to reach the +/// URL unescaped, because every legitimate value already contains a +/// character [`crate::executor::path_segment`]'s escaping would otherwise +/// mangle. +/// +/// Used to be inferred from the parameter having an `enum` — clap's +/// `PossibleValuesParser` had already limited it to one of the spec's own +/// literal strings, so there was nothing left to smuggle in. That stopped +/// being true once `directions.yaml`'s own `profile` dropped its `enum`: +/// the four documented routing profiles (`mapbox/driving` etc.) aren't +/// exhaustive — some accounts have additional ones of their own that never +/// reached docs.mapbox.com, and an `enum` rejected those client-side. +/// Reported in review. So this is now a named decision instead of a +/// side effect of another one — every entry here is a path parameter whose +/// documented *and* undocumented values alike contain a literal `/` +/// (`mapbox/driving`, `mapbox/cycling`, an OEM's own profile name, …), which +/// the routing profile's own path segment depends on reaching the API +/// unescaped regardless of which spelling was typed. +pub const UNESCAPED_PATH_PARAMS: &[(&str, &str)] = &[("directions", "profile")]; + /// The media types an operation's request body may be sent as. /// /// This used to be a plain `has_body: bool`, which forced the executor to @@ -414,9 +463,15 @@ impl Operation { .expect("a command path is never empty") } - /// The command as it is typed after `mapbox`: `styles draft get`. + /// The command as it is typed after `mapbox`: `styles draft get` — or, + /// for a [`FLATTENED_SERVICES`] service, just the service name, since + /// that service has exactly one operation and no subcommand at all. pub fn command(&self) -> String { - format!("{} {}", self.service, self.command_path.join(" ")) + if FLATTENED_SERVICES.contains(&self.service.as_str()) { + self.service.clone() + } else { + format!("{} {}", self.service, self.command_path.join(" ")) + } } /// Whether this is a service's own health check, rather than something @@ -2032,15 +2087,36 @@ paths: profile.arg_name, "profile", "must not collide with the global --profile id" ); - assert_eq!( - profile.enum_values, - [ - "mapbox/driving-traffic", - "mapbox/driving", - "mapbox/walking", - "mapbox/cycling" - ] + // Deliberately not an `enum`: see `UNESCAPED_PATH_PARAMS`'s own doc + // comment for why a closed set was wrong here (OEM accounts have + // undocumented profiles of their own). + assert!( + profile.enum_values.is_empty(), + "profile must accept any value, not just the four documented ones" + ); + assert!( + UNESCAPED_PATH_PARAMS.contains(&("directions", "profile")), + "profile's literal `/` must still reach the URL unescaped, \ + now that it can't rely on being an enum to prove that" ); + + // `directions` has exactly one operation and is in + // `FLATTENED_SERVICES` — `command()` must say so, dropping + // `command_path` from the string entirely, even though + // `command_path` itself stays `["route"]` for internal lookups + // (`op.command_name()`, the `command_path == …` matches above and + // in `main.rs`'s dispatch). + assert_eq!(route.command(), "directions"); + } + + /// A non-flattened operation's `command()` is unaffected — this is the + /// regression test for `FLATTENED_SERVICES` breaking every other + /// service's rendering along with the one it's meant for. + #[test] + fn command_only_drops_the_path_for_a_flattened_service() { + let svc = service(PAIRED); + let op = operation(&svc, "list-styles"); + assert_eq!(op.command(), "svc list-styles"); } #[test] diff --git a/tests/fixtures/api_command_surface.txt b/tests/fixtures/api_command_surface.txt index 6885a38..21d1f9e 100644 --- a/tests/fixtures/api_command_surface.txt +++ b/tests/fixtures/api_command_surface.txt @@ -1,7 +1,7 @@ mapbox accounts list-scopes | aliases: (none) mapbox accounts list-tokens | aliases: (none) mapbox accounts retrieve-token | aliases: (none) -mapbox directions route | aliases: (none) +mapbox directions | aliases: (none) mapbox fonts delete | aliases: (none) mapbox fonts list | aliases: (none) mapbox fonts upload | aliases: (none)