diff --git a/CHANGELOG.md b/CHANGELOG.md index 0e4281c..c85f71d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,29 @@ breaking is [written down in CONTRIBUTING.md](CONTRIBUTING.md#compatibility) — command names, flags, the two output modes and the exit codes are promises; the Mapbox APIs' own response bodies are not. +## Unreleased + +### Added + +- Paginated listings now say when there is more to fetch. A response the API + paged answers with one page, and the CLI prints the flags that fetch the + next one — "More results: add `--limit 2 --start …` for the next page" — + on stderr in both output modes. Before this the extra pages were + unreachable: the API signals them in a `Link` header, which nothing read, + so `-o text` and `-o json` both looked complete. `--id` on a paged listing + now also distinguishes "not on this page" from "does not exist", and says + how to look further. Following the pages is still the caller's job; there + is no `--all` yet. + +- Failures now carry the response's request id, which is what Mapbox support + needs to find one request in their logs. In practice that is CloudFront's + `x-amz-cf-id`, which every Mapbox response carries; a service sending its + own `x-request-id` is preferred when one does. Present on every failure + under `-o json` as `request_id`; printed under `-o text` for a 5xx only, + where the server is at fault and there is nothing the caller can do about + it. `mapbox agent-skills` is exempt — it fetches from GitHub, whose request + id Mapbox support cannot look up. + ## 0.1.8 - 2026-09-14 Initial beta release. The next release is `0.2.0`. diff --git a/docs/commands.md b/docs/commands.md index 7de64d7..a09eb2c 100644 --- a/docs/commands.md +++ b/docs/commands.md @@ -571,6 +571,55 @@ map.png: PNG image data, 600 x 400 +#### One page at a time + +Several listings are paginated by the API, which returns one page and a +`Link` header naming the next. **The CLI says so rather than leaving the +result looking complete**, and names the flags that fetch the next page: + +``` +$ mapbox accounts list-tokens --username user --limit 2 +ID NOTE CREATED USAGE +cmtoken00000000000000001a CI deploy key 2026-09-04 sk +cmtoken00000000000000002b Local dev 2026-09-03 pk + +Tips: + `-o json` for the response as the API sent it. + To see one row: add `--id cmtoken00000000000000001a` + More results: add `--limit 2 --start cmtoken00000000000000002b` for the next page. +``` + +Under `-o json` the same note is the only thing printed to stderr on a +success, and as the lone tip it takes the singular form: + +``` +$ mapbox accounts list-tokens --username user --limit 2 -o json > page1.json +Tip: More results: add `--limit 2 --start cmtoken00000000000000002b` for the next page. +``` + +The flags are derived from the response, not hardcoded: whatever the spec +calls an operation's paging parameters is what the line names. The access +token is never among them, even though the API echoes it back in that header. + +Two details worth knowing: + +- **The note goes to stderr in both modes**, including `-o json`. The result + is just as partial there, and the API's own document cannot carry the fact + without an envelope this CLI has promised not to add — so a `-o json` + consumer reading stdout alone is unaffected, and one watching stderr is + told. There is no `--all` yet; following the pages is the caller's job, + and [#117](https://github.com/mapbox/mapbox-cli-private/issues/117) tracks + changing that. +- **`--id` searches the page it was given.** On a paginated listing a miss + means "not on this page", which is not the same as "does not exist", so + the error says which and how to look further: + + ``` + Error: No row has the id `cmtoken00000000000000009z`. + Fix: This is one page of results, so the id may be on a later one. Add + `--start cmtoken00000000000000002b --limit 2` to search the next page. + ``` + --- ## Accounts @@ -593,8 +642,8 @@ Lists the access tokens for an account. Secret (`sk`) entries omit the | `--usage ` | Only tokens of that kind. | | `--default` | Only the account's default token. | -Results are paginated: a `Link` header with `rel="next"` signals more, and -its `start` value is what `--start` wants. +Results are paginated. When more exist the CLI prints the `--start` value to +continue from — see [One page at a time](#one-page-at-a-time). #### Examples @@ -3420,6 +3469,30 @@ answers "was this computed?". | `fix` | One line: why it failed, and what would make it work. | | `next_actions` | Commands to run, and nothing else — no prose to strip before running one. | | `docs` | The pages that bear on the failure: the command's own, plus the tokens page when it was the credential that was refused. | +| `request_id` | The response's request id, for quoting to Mapbox support. | + +`request_id` is the one field whose two renderings differ on purpose. Under +`-o json` it is there on **every** failure that carried one, whatever the +status, because a caller logging failures wants it on all of them and a field +costs nothing to ignore. Under `-o text` it is printed for a **5xx only**: + +``` +Error: Internal server error (HTTP 500) +Fix: The service failed rather than refusing the request. Retry, and check https://status.mapbox.com if it persists. +Request ID: 01JC8K3Q7V9XZ4M2 (quote this to Mapbox support) +``` + +That is the failure a person escalates, and the id is what lets support find +the request in their logs. A 404 on a mistyped id is the reader's own to fix, +so an id under it would be noise on the common case. + +The id is whatever identified the response: `x-request-id` from a service +that sends one, and otherwise `x-amz-cf-id`, the CloudFront id every Mapbox +response carries. Quote it as printed — support can trace either. + +`mapbox agent-skills` is the one command whose failures carry no +`request_id`, and deliberately: it fetches from GitHub, which identifies +requests with its own header that Mapbox support cannot look up. The advice is keyed on the HTTP status, with the command filling in what only it knows — and it is read off the parsed spec, so a suggestion can only name diff --git a/src/account_usage.rs b/src/account_usage.rs index d2d03dd..d5d308b 100644 --- a/src/account_usage.rs +++ b/src/account_usage.rs @@ -614,12 +614,16 @@ fn fetch( .map_err(|e| executor::transport_failure("Request failed", e))?; let status = response.status(); + // Before `text()` consumes the response: a 5xx here is worth escalating, + // and this is the only thing that lets support find the request. + let request_id = executor::request_id(response.headers()); let text = response .text() .map_err(|e| executor::transport_failure("Failed to read response", e))?; if !status.is_success() { return Err(CliError::http(status.as_u16(), &text) + .with_request_id(request_id) .with_remedy(remedy_for(status.as_u16())) .into()); } diff --git a/src/auth.rs b/src/auth.rs index c31e724..735e1f7 100644 --- a/src/auth.rs +++ b/src/auth.rs @@ -1032,12 +1032,16 @@ fn verify_token(token: &str, debug: bool, timeout: Option) -> Result match serde_json::from_str::(&text) { Ok(json) => match matches.get_one::(output::FILTER_ARG) { Some(wanted) => output::emit_value( mode, - &output::pick_row(&json, wanted)?, + &output::pick_row(&json, wanted) + .map_err(|err| with_page_context(err, next_page.as_ref()))?, None, Some(&op.service), + // The row asked for is in hand; where the *other* rows + // are is not advice about it. + None, + )?, + None => output::emit_value( + mode, + &json, + detail_hint(op).as_deref(), + Some(&op.service), + next_page.as_ref().map(NextPage::tip).as_deref(), )?, - None => { - output::emit_value(mode, &json, detail_hint(op).as_deref(), Some(&op.service))? - } }, Err(_) => output::emit_text_body(mode, &text)?, }, @@ -328,6 +344,201 @@ fn dispatch( Ok(()) } +/// The headers a Mapbox response may identify itself with, in the order they +/// are preferred. +/// +/// Measured rather than assumed, and the measurement is the reason there are +/// two. `x-request-id` is the name the convention would predict and is what +/// this looked for first — but no Mapbox endpoint reachable from here sends +/// it: styles, tokens, fonts and geocoding v6 all answer without one, on both +/// success and failure. What every one of them does carry is `x-amz-cf-id`, +/// the CloudFront request id, because the whole API is fronted by it — and +/// that is the id support traces a request with. +/// +/// `x-request-id` stays first because a service that does send one means it +/// more specifically than the CDN in front of it does, and it costs a lookup +/// in a map that is already in memory. +const REQUEST_ID_HEADERS: [&str; 2] = ["x-request-id", "x-amz-cf-id"]; + +/// The request id from a response, for a caller that reads the body itself. +/// +/// `text()` and `bytes()` both consume the response, so this has to be called +/// before the body is read — which is the whole reason it is a named function +/// rather than a line inlined at each of the three call sites. +/// +/// Mapbox-bound requests only. `agent_skills` talks to GitHub codeload, which +/// identifies requests with `x-github-request-id` and is not something Mapbox +/// support can look up, so it deliberately does not call this. +pub fn request_id(headers: &reqwest::header::HeaderMap) -> Option { + REQUEST_ID_HEADERS.iter().find_map(|name| { + headers + .get(*name) + .and_then(|value| value.to_str().ok()) + .map(str::trim) + .filter(|value| !value.is_empty()) + .map(String::from) + }) +} + +/// What a response says about itself, past its body. +/// +/// A struct rather than three reads at the call site because `bytes()` +/// consumes the response: whatever is not taken before it is unrecoverable. +/// Taking only `Content-Type` is what left paginated listings truncating +/// silently and left a 500 with nothing to quote to support +/// (mapbox/mapbox-cli-private#117). +struct ResponseHeaders { + /// Decides whether the body is read as text or written as bytes. + content_type: String, + /// The request id — what support needs to find this one request in their + /// logs. Carried into the error and never printed on success, because on + /// a response that worked it is noise. + request_id: Option, + /// The `rel="next"` target of a `Link` header, when this response is one + /// page of several. + next_page: Option, +} + +impl ResponseHeaders { + fn read(headers: &reqwest::header::HeaderMap) -> Self { + // A header present but empty says nothing, and an empty request id + // would print as `Request ID:` with a blank after it. + let text = |name: &str| { + headers + .get(name) + .and_then(|value| value.to_str().ok()) + .map(str::trim) + .filter(|value| !value.is_empty()) + }; + + ResponseHeaders { + content_type: text(reqwest::header::CONTENT_TYPE.as_str()) + .unwrap_or_default() + .to_string(), + request_id: request_id(headers), + next_page: text(reqwest::header::LINK.as_str()) + .and_then(link::next_url) + .map(String::from), + } + } +} + +/// A response that is one page of several, and how to ask for the next one. +/// +/// Holds the flags rather than the URL: following the `Link` target verbatim +/// would mean re-sending a URL the API built, token and all, while the flags +/// are something a caller can read, edit and run. +struct NextPage(Option); + +impl NextPage { + /// Derives the flags from the operation rather than hardcoding `--start`. + /// + /// The next URL's query is matched against the parameters this command + /// declares, so the tip names whatever the spec calls its paging + /// parameters, and a service that pages some other way needs no change + /// here. + /// + /// **The access token cannot appear in the result.** It rides in the + /// query string of every request, so the API echoes it back in this very + /// URL. Two things keep it out, and the second is why the first is not + /// enough: `dispatch` adds it directly rather than declaring it in + /// `op.query_params`, so matching against the declared parameters + /// excludes it today — but a spec is free to declare a parameter by that + /// name, and then "by construction" would quietly stop being true, so it + /// is also refused explicitly. `the_page_tip_never_names_the_access_token` + /// and `a_declared_parameter_named_access_token_is_still_withheld` hold + /// both halves. + fn of(declared: &[Parameter], next: &str) -> Self { + let flags: Vec = query_pairs(next) + .into_iter() + .filter(|(name, _)| name != ACCESS_TOKEN) + .filter_map(|(name, value)| { + declared + .iter() + .find(|param| param.name == name) + .map(|param| format!("--{} {}", param.arg_name, shell_value(&value))) + }) + .collect(); + + NextPage((!flags.is_empty()).then(|| flags.join(" "))) + } + + /// The line printed under a listing that has more pages. + fn tip(&self) -> String { + match &self.0 { + Some(flags) => format!("More results: add `{flags}` for the next page."), + // Reachable only if the API pages an operation whose spec + // declares no paging parameter — a spec gap, not a user error. + // Saying so beats saying nothing, because the result is + // incomplete either way and only this knows it. + None => "More results exist, but this command declares no parameter to reach them." + .to_string(), + } + } + + /// The `fix` on a `--id` that found nothing on this page. + fn fix(&self) -> String { + match &self.0 { + Some(flags) => format!( + "This is one page of results, so the id may be on a later one. \ + Add `{flags}` to search the next page." + ), + None => "This is one page of results, so the id may be on a later one.".to_string(), + } + } +} + +/// A URL's query, decoded. +/// +/// Percent-decoded on purpose: the values go into a tip meant to be copied +/// onto a command line, and the CLI re-encodes whatever it is given — so +/// handing back `%2B` would round-trip to `%252B` and ask for the wrong page. +/// An unparseable URL yields nothing rather than failing: a tip is not worth +/// turning a successful request into an error. +fn query_pairs(url: &str) -> Vec<(String, String)> { + match reqwest::Url::parse(url) { + Ok(parsed) => parsed + .query_pairs() + .map(|(name, value)| (name.into_owned(), value.into_owned())) + .collect(), + Err(_) => vec![], + } +} + +/// A query value as it would have to be typed into a shell. +/// +/// Paging cursors are opaque ids in practice, but the tip is advice a reader +/// pastes, and an unquoted value with a space in it would silently become +/// two arguments. +fn shell_value(value: &str) -> String { + let safe = |c: char| c.is_ascii_alphanumeric() || "-_.~:@+,".contains(c); + if !value.is_empty() && value.chars().all(safe) { + return value.to_string(); + } + format!("'{}'", value.replace('\'', r"'\''")) +} + +/// Adds "there are more pages" to a `--id` that matched nothing. +/// +/// `pick_row` searches the rows it was handed and says "No row has the id", +/// which is true of the page and may well be false of the listing. On a +/// paginated response that is the most misleading form of the truncation +/// this whole path exists to stop, so the error says which it means. +fn with_page_context(err: anyhow::Error, next_page: Option<&NextPage>) -> anyhow::Error { + let Some(next_page) = next_page else { + return err; + }; + match err.downcast::() { + // `not_a_list` is about the shape of the response, which another + // page would not change. + Ok(cli) if cli.code == "not_found" => cli + .with_remedy(Remedy::default().with_fix(&next_page.fix())) + .into(), + Ok(cli) => cli.into(), + Err(other) => other, + } +} + /// The request line a reader may safely see: URL, query, token replaced. /// /// The access token rides in the query string, so every rendering of this URL @@ -933,11 +1144,13 @@ fn write_binary(body: &[u8], content_type: &str) -> Result<()> { mod tests { use super::{ describe_body, empty_success_line, file_name_of, is_binary_content_type, part_media_type, - payload_of, redacted_url, resolve_body_source, substitute_path_param, BodySource, + payload_of, query_pairs, redacted_url, request_id, resolve_body_source, shell_value, + substitute_path_param, with_page_context, BodySource, NextPage, ResponseHeaders, + ACCESS_TOKEN, REQUEST_ID_HEADERS, }; use crate::http::Payload; use crate::output::CliError; - use crate::spec::RequestBody; + use crate::spec::{Parameter, RequestBody}; /// The `CliError` inside a refusal, so a test can name the code the /// caller would see rather than match on prose. @@ -1394,4 +1607,249 @@ mod tests { assert_eq!(payload_of(Some(&BodySource::Json("{}"))), Payload::Bounded); assert_eq!(payload_of(Some(&text)), Payload::Bounded); } + + /// A declared query parameter, with only the fields these tests read set + /// to anything meaningful. + fn param(name: &str) -> Parameter { + Parameter { + name: name.to_string(), + arg_name: name.to_string(), + required: false, + description: None, + enum_values: vec![], + is_boolean: false, + numeric: None, + } + } + + #[test] + fn the_page_tip_names_the_flags_the_spec_declares() { + let declared = [param("start"), param("limit")]; + let next = "https://api.mapbox.com/styles/v1/u?start=cjk2&limit=10"; + + let tip = NextPage::of(&declared, next).tip(); + assert!(tip.contains("--start cjk2"), "{tip}"); + assert!(tip.contains("--limit 10"), "{tip}"); + } + + /// A parameter the API sent back but this command does not declare has no + /// flag to name, so it is left out rather than invented. + #[test] + fn an_undeclared_query_parameter_is_not_named() { + let declared = [param("start")]; + let next = "https://api.mapbox.com/a?start=7&fresh=true"; + + let tip = NextPage::of(&declared, next).tip(); + assert!(tip.contains("--start 7"), "{tip}"); + assert!(!tip.contains("fresh"), "{tip}"); + } + + /// **The security property of this whole path.** + /// + /// The access token rides in the query string, so the URL the API echoes + /// back in `Link` contains a live token. It is excluded by construction — + /// `dispatch` adds it to the query directly rather than declaring it in + /// `op.query_params`, and only declared parameters become flags — but + /// "by construction" is worth a test, because the cost of being wrong is + /// printing a credential to a terminal and into whatever captured it. + #[test] + fn the_page_tip_never_names_the_access_token() { + let secret = "pk.eyJ1IjoibWFwYm94IiwiYSI6ImNqa2xpdmV0b2tlbiJ9.aaaaaaaaaaaaaaaaaaaaaa"; + let declared = [param("start"), param("limit")]; + let next = + format!("https://api.mapbox.com/styles/v1/u?access_token={secret}&start=cjk2&limit=10"); + + let page = NextPage::of(&declared, &next); + for rendered in [page.tip(), page.fix()] { + assert!(!rendered.contains(secret), "leaked the token: {rendered}"); + assert!(!rendered.contains("access_token"), "{rendered}"); + assert!(!rendered.contains("pk.ey"), "{rendered}"); + } + } + + /// Even if someone later declares a parameter by that name, which is the + /// way the guarantee above could be undone from a spec rather than from + /// this file. + #[test] + fn a_declared_parameter_named_access_token_is_still_withheld() { + let declared = [param(ACCESS_TOKEN), param("start")]; + let next = "https://api.mapbox.com/a?access_token=pk.secret&start=3"; + + let tip = NextPage::of(&declared, next).tip(); + assert!(!tip.contains("pk.secret"), "{tip}"); + assert!(tip.contains("--start 3"), "{tip}"); + } + + /// The values are decoded, because the CLI re-encodes whatever it is + /// given: handing back `%2B` would round-trip to `%252B` and fetch the + /// wrong page. + #[test] + fn the_page_tip_decodes_percent_escapes() { + let declared = [param("start")]; + let next = "https://api.mapbox.com/a?start=a%2Bb"; + + let tip = NextPage::of(&declared, next).tip(); + assert!(tip.contains("--start a+b"), "{tip}"); + } + + /// A value with a space in it would silently become two arguments if the + /// tip were pasted unquoted. + #[test] + fn a_value_needing_a_shell_quote_gets_one() { + assert_eq!(shell_value("cjk2ab"), "cjk2ab"); + assert_eq!(shell_value("2026-09-01"), "2026-09-01"); + assert_eq!(shell_value("a b"), "'a b'"); + assert_eq!(shell_value(""), "''"); + assert_eq!(shell_value("it's"), r"'it'\''s'"); + } + + /// An operation the API pages but whose spec declares no paging + /// parameter. The result is incomplete either way, so saying so beats + /// saying nothing — but it must not claim a flag that does not exist. + #[test] + fn no_declared_paging_parameter_still_says_the_result_is_partial() { + let tip = NextPage::of(&[param("unrelated")], "https://api.mapbox.com/a?start=3").tip(); + assert!(tip.contains("More results exist"), "{tip}"); + assert!(!tip.contains("--"), "{tip}"); + } + + /// An unparseable `Link` target costs nothing: the request succeeded, and + /// a tip is not worth turning that into a failure. + #[test] + fn an_unparseable_next_url_yields_no_flags() { + assert!(query_pairs("not a url").is_empty()); + assert!(query_pairs("").is_empty()); + } + + #[test] + fn response_headers_read_the_three_things_that_survive() { + let mut map = reqwest::header::HeaderMap::new(); + map.insert( + reqwest::header::CONTENT_TYPE, + "application/json".parse().unwrap(), + ); + map.insert(REQUEST_ID_HEADERS[0], "req-abc123".parse().unwrap()); + map.insert( + reqwest::header::LINK, + r#"; rel="next""#.parse().unwrap(), + ); + + let headers = ResponseHeaders::read(&map); + assert_eq!(headers.content_type, "application/json"); + assert_eq!(headers.request_id.as_deref(), Some("req-abc123")); + assert_eq!( + headers.next_page.as_deref(), + Some("https://api.mapbox.com/a?start=3") + ); + } + + /// The header that actually arrives in practice. + /// + /// No Mapbox endpoint reachable from here sends `x-request-id` — styles, + /// tokens, fonts and geocoding v6 were all checked, on success and on a + /// 404. Every one of them sends `x-amz-cf-id`, because the API is fronted + /// by CloudFront. Looking for the conventional name alone would have made + /// this feature inert, which is what this test exists to stop happening + /// again. + #[test] + fn the_cloudfront_id_is_read_when_there_is_no_request_id() { + let mut map = reqwest::header::HeaderMap::new(); + map.insert( + "x-amz-cf-id", + "E5Kat8az0mUkEYObB4Nvhm6Bi49kt50A".parse().unwrap(), + ); + + assert_eq!( + request_id(&map).as_deref(), + Some("E5Kat8az0mUkEYObB4Nvhm6Bi49kt50A") + ); + } + + /// A service that sends its own id means it more specifically than the + /// CDN in front of it does. + #[test] + fn an_explicit_request_id_outranks_the_cloudfront_one() { + let mut map = reqwest::header::HeaderMap::new(); + map.insert("x-amz-cf-id", "cloudfront".parse().unwrap()); + map.insert("x-request-id", "from-the-service".parse().unwrap()); + + assert_eq!(request_id(&map).as_deref(), Some("from-the-service")); + } + + /// A response with neither claims nothing. + #[test] + fn no_identifying_header_is_none() { + let mut map = reqwest::header::HeaderMap::new(); + map.insert( + reqwest::header::CONTENT_TYPE, + "application/json".parse().unwrap(), + ); + assert_eq!(request_id(&map), None); + } + + /// A header present but blank says nothing, and an empty request id would + /// print as `Request ID:` with nothing after it. + #[test] + fn a_blank_header_reads_as_absent() { + let mut map = reqwest::header::HeaderMap::new(); + map.insert(REQUEST_ID_HEADERS[0], " ".parse().unwrap()); + map.insert(reqwest::header::LINK, "".parse().unwrap()); + + let headers = ResponseHeaders::read(&map); + assert_eq!(headers.content_type, ""); + assert_eq!(headers.request_id, None); + assert_eq!(headers.next_page, None); + } + + /// The last page of a listing still carries a `Link`, naming the pages + /// behind it. "Has a header" must not read as "has more". + #[test] + fn a_link_without_a_next_relation_is_not_another_page() { + let mut map = reqwest::header::HeaderMap::new(); + map.insert( + reqwest::header::LINK, + r#"; rel="prev""#.parse().unwrap(), + ); + assert_eq!(ResponseHeaders::read(&map).next_page, None); + } + + /// `--id` searches the page it was handed. On a paginated response + /// "No row has the id" is true of the page and may be false of the + /// listing, which is the most misleading form of the truncation this + /// path exists to stop. + #[test] + fn an_id_miss_on_a_paginated_listing_says_the_row_may_be_later() { + let page = NextPage::of(&[param("start")], "https://api.mapbox.com/a?start=3"); + let err = with_page_context( + CliError::new("not_found", "No row has the id `x`.").into(), + Some(&page), + ); + + let cli = err.downcast_ref::().expect("still a CliError"); + let fix = cli.fix.as_deref().expect("a fix was added"); + assert!(fix.contains("one page of results"), "{fix}"); + assert!(fix.contains("--start 3"), "{fix}"); + } + + /// `not_a_list` is about the shape of the response, which another page + /// would not change — so it keeps its own advice. + #[test] + fn a_shape_error_is_not_given_paging_advice() { + let page = NextPage::of(&[param("start")], "https://api.mapbox.com/a?start=3"); + let err = with_page_context( + CliError::new("not_a_list", "`--id` only applies to a list.").into(), + Some(&page), + ); + assert_eq!(err.downcast_ref::().unwrap().fix, None); + } + + /// An unpaginated response leaves every error exactly as it was. + #[test] + fn without_another_page_an_error_passes_through_untouched() { + let err = with_page_context( + CliError::new("not_found", "No row has the id `x`.").into(), + None, + ); + assert_eq!(err.downcast_ref::().unwrap().fix, None); + } } diff --git a/src/link.rs b/src/link.rs new file mode 100644 index 0000000..7d4f0f7 --- /dev/null +++ b/src/link.rs @@ -0,0 +1,186 @@ +//! The `Link` response header (RFC 8288), which is how the Mapbox APIs say +//! a listing has another page. +//! +//! `styles list-styles`, `accounts list-tokens` and the other paginated +//! listings answer with one page and a header naming the next: +//! +//! ```text +//! Link: ; rel="next" +//! ``` +//! +//! Parsed rather than matched with a substring or a regex, because the two +//! things that would break a shortcut both occur in real headers: a URI may +//! contain a comma, which is also the delimiter between links, and `rel` may +//! carry a space-separated list of relation types rather than one word. +//! [`next_url`] is the only thing this module is for, so it does the least +//! parsing that gets that right. + +/// The `rel="next"` target in a `Link` header, if it has one. +/// +/// Returns a slice of the header, so the caller decides whether to own it. +/// `None` for an absent relation, a malformed header, or a link with no +/// `rel` — nothing here fails, because a header this CLI could not parse is +/// a reason to say nothing rather than a reason to stop. +pub fn next_url(header: &str) -> Option<&str> { + links(header).find_map(|link| link.is_next().then_some(link.uri)) +} + +/// One `; param=value; …` entry. +struct Link<'a> { + uri: &'a str, + params: &'a str, +} + +impl Link<'_> { + /// Whether this link's `rel` names `next`. + /// + /// RFC 8288 §3.3: the value may be a space-separated list, and relation + /// types are compared case-insensitively. `rel="prev next"` is therefore + /// a next link, and `rel=NEXT` — legal unquoted, since `next` needs no + /// quoting — is too. + fn is_next(&self) -> bool { + self.params.split(';').any(|param| { + let Some((name, value)) = param.split_once('=') else { + return false; + }; + name.trim().eq_ignore_ascii_case("rel") + && value + .trim() + .trim_matches('"') + .split_whitespace() + .any(|rel| rel.eq_ignore_ascii_case("next")) + }) + } +} + +/// Splits a `Link` header into its entries. +/// +/// The delimiter is a comma, and a URI may contain one +/// (`?bbox=1,2,3,4` is ordinary in this API), so entries are found by their +/// angle brackets instead of by splitting the header: everything between +/// `<` and the next `>` is a URI, and everything from there to the following +/// `<` is that link's parameters. +fn links(header: &str) -> impl Iterator> { + let mut rest = header; + std::iter::from_fn(move || { + let open = rest.find('<')?; + let after_open = &rest[open + 1..]; + let close = after_open.find('>')?; + let uri = &after_open[..close]; + + let tail = &after_open[close + 1..]; + // Up to the next link, which is where this link's parameters end. + // A comma inside the *parameters* would be inside a quoted string + // (`title="a, b"`), and stopping at `<` rather than `,` means one + // cannot end the entry early either. + let (params, next) = match tail.find('<') { + Some(at) => (&tail[..at], &tail[at..]), + None => (tail, ""), + }; + rest = next; + // The comma that separated this link from the one after it is still + // on the end, and it is the delimiter rather than part of the last + // parameter — left on, `rel="next",` does not equal `next`. + let params = params.trim().trim_end_matches(','); + Some(Link { uri, params }) + }) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn finds_the_next_link() { + let header = r#"; rel="next""#; + assert_eq!( + next_url(header), + Some("https://api.mapbox.com/styles/v1/u?start=cjk2") + ); + } + + #[test] + fn picks_next_out_of_several_relations() { + let header = concat!( + r#"; rel="prev", "#, + r#"; rel="next", "#, + r#"; rel="last""# + ); + assert_eq!(next_url(header), Some("https://api.mapbox.com/a?start=3")); + } + + /// The reason this is a parser and not `header.split(',')`. A bounding + /// box is four comma-separated numbers, and `geocoding` and `tilequery` + /// both take one — splitting on commas would cut this URI into pieces + /// and find no link at all. + #[test] + fn a_comma_inside_the_uri_does_not_end_the_link() { + let header = r#"; rel="next""#; + assert_eq!( + next_url(header), + Some("https://api.mapbox.com/a?bbox=-1,2,-3,4&start=7") + ); + } + + /// RFC 8288 §3.3: a space-separated list of relation types, compared + /// case-insensitively. Both halves of that matter here. + #[test] + fn a_relation_list_and_odd_casing_still_count() { + for header in [ + r#"; rel="prev next""#, + r#"; rel="NEXT""#, + r#"; rel=next"#, + r#"; REL="next""#, + ] { + assert_eq!( + next_url(header), + Some("https://api.mapbox.com/a"), + "{header}" + ); + } + } + + #[test] + fn other_parameters_alongside_rel_are_ignored() { + let header = r#"; type="application/json"; rel="next""#; + assert_eq!(next_url(header), Some("https://api.mapbox.com/a")); + } + + /// The last page of a listing. The API still sends a `Link`, naming the + /// pages behind rather than ahead — so "has a header" must not be read + /// as "has more", which is the whole reason this returns an `Option` + /// rather than a bool about the header's presence. + #[test] + fn no_next_relation_is_none() { + let header = r#"; rel="prev""#; + assert_eq!(next_url(header), None); + } + + /// A relation that merely *contains* "next" is not `next`. + #[test] + fn a_longer_relation_name_is_not_next() { + for header in [ + r#"; rel="nextpage""#, + r#"; rel="mynext""#, + ] { + assert_eq!(next_url(header), None, "{header}"); + } + } + + /// Nothing here panics or errors: a header this module cannot make sense + /// of means the CLI says nothing about pagination, which is what it did + /// before there was a parser at all. + #[test] + fn malformed_headers_are_none_rather_than_a_failure() { + for header in [ + "", + "not a link header", + "", + ";;;", + ] { + assert_eq!(next_url(header), None, "{header:?}"); + } + } +} diff --git a/src/main.rs b/src/main.rs index 9a9b4db..35c818c 100644 --- a/src/main.rs +++ b/src/main.rs @@ -23,6 +23,7 @@ mod executor; mod feature_flags; mod generate_skills; mod http; +mod link; mod output; mod remedy; mod schema; diff --git a/src/output.rs b/src/output.rs index eb9fdf3..07bd234 100644 --- a/src/output.rs +++ b/src/output.rs @@ -230,6 +230,14 @@ pub struct CliError { /// The documentation for what failed. Attached per service and per /// status by [`crate::remedy`], which is where the URLs live. pub docs: Vec, + /// The request id from the response that failed — see + /// `executor::REQUEST_ID_HEADERS` for which header it comes from. + /// + /// Always in the `json` rendering, where a field costs a reader nothing. + /// In `text` only for a 5xx, because that is the failure a person takes + /// to support — a 404 on a mistyped style id is theirs to fix, and an id + /// under it would be noise on the common case. + pub request_id: Option, } impl CliError { @@ -243,6 +251,7 @@ impl CliError { fix: None, next_actions: Vec::new(), docs: Vec::new(), + request_id: None, } } @@ -316,8 +325,31 @@ impl CliError { fix: None, next_actions: Vec::new(), docs: Vec::new(), + request_id: None, } } + + /// Records the response's request id for this failure. + /// + /// Takes the `Option` rather than a value so the caller hands over + /// whatever the response had without a branch of its own. + pub fn with_request_id(mut self, request_id: Option) -> Self { + self.request_id = request_id; + self + } + + /// The request id worth showing a person, as opposed to a program. + /// + /// A 5xx only. The server broke, nothing the reader typed will fix it, + /// and this is what lets support find the request. Under a 404 on a + /// mistyped id it would be a line of noise beneath an error the reader + /// can already act on — so the `json` rendering carries it always and + /// this decides the `text` one. + fn support_request_id(&self) -> Option<&str> { + self.request_id + .as_deref() + .filter(|_| self.status.is_some_and(|status| status >= 500)) + } } /// Caps a message taken from a response body. @@ -375,14 +407,27 @@ fn encode(value: &Value, pretty: bool) -> Result { /// `service` gates the exception — see [`list_rendering`]: `search`'s, /// `geocoder`'s and `tilequery`'s GeoJSON render as a list instead. Every /// other value takes the path it always has. +/// +/// `page` is the note that this response is one page of several. It goes to +/// stderr **in both modes**, unlike the other notes here: the result is just +/// as incomplete under `json`, and the API's own answer cannot carry the +/// fact without wrapping it in an envelope this CLI has promised not to add. pub fn emit_value( mode: Mode, value: &Value, footer: Option<&str>, service: Option<&str>, + page: Option<&str>, ) -> Result<()> { if let Mode::Json { pretty } = mode { - return write_stdout(&encode(value, pretty)?); + write_stdout(&encode(value, pretty)?)?; + // The only thing `json` prints to stderr on a success. A consumer + // reading stdout alone is unaffected; one that would otherwise + // believe it had the whole list is told. Through `print_tips` like + // every other note, so the one thing `json` says on stderr is not + // also the one thing shaped differently. + print_tips(page.map(String::from).as_slice()); + return Ok(()); } match list_rendering(value, service).or_else(|| render_human(value)) { @@ -411,6 +456,9 @@ pub fn emit_value( "`-o json` for the response as the API sent it.".to_string() }]; tips.extend(next); + // Last, because it is about the response as a whole rather than + // about the rendering above it. + tips.extend(page.map(String::from)); print_tips(&tips); Ok(()) } @@ -1386,6 +1434,46 @@ pub fn progress(message: &str) { eprintln!("{message}"); } +/// A `CliError` as the object `json` mode prints. +/// +/// Split out from [`emit_error`] because it is the machine-readable contract +/// — a consumer branches on these keys — and a function returning a value +/// can be tested, where one that writes to stderr cannot. +fn error_payload(e: &CliError) -> Value { + let mut obj = json!({ "code": e.code, "message": e.message }); + if let Some(status) = e.status { + obj["status"] = json!(status); + } + // Same rule the text rendering uses: a body whose only key is `message` + // has already been said, and repeating it makes a consumer wonder which + // of the two to read. + if let Some(body) = e.body.as_ref().filter(|b| adds_detail(b)) { + obj["body"] = body.clone(); + } + if let Some(text) = &e.body_text { + obj["body_text"] = json!(text); + } + if let Some(fix) = &e.fix { + obj["fix"] = json!(fix); + } + // Absent rather than empty. `[]` invites a consumer to wonder whether the + // list was computed and came out empty, which is the question a missing + // key already answers. + if !e.next_actions.is_empty() { + obj["next_actions"] = json!(e.next_actions); + } + if !e.docs.is_empty() { + obj["docs"] = json!(e.docs); + } + // Here whatever the status, unlike the `text` rendering: a field costs a + // consumer nothing to ignore, and a caller logging failures wants the id + // on all of them, not only the ones a person would escalate. + if let Some(request_id) = &e.request_id { + obj["request_id"] = json!(request_id); + } + obj +} + /// Renders a failure to stderr. /// /// Flat, not wrapped in an `{"error": …}` object. Under `json`, stderr never @@ -1398,34 +1486,7 @@ pub fn emit_error(mode: Mode, err: &anyhow::Error) { if mode.is_json() { let payload = match cli { - Some(e) => { - let mut obj = json!({ "code": e.code, "message": e.message }); - if let Some(status) = e.status { - obj["status"] = json!(status); - } - // Same rule the text rendering uses: a body whose only key - // is `message` has already been said, and repeating it makes - // a consumer wonder which of the two to read. - if let Some(body) = e.body.as_ref().filter(|b| adds_detail(b)) { - obj["body"] = body.clone(); - } - if let Some(text) = &e.body_text { - obj["body_text"] = json!(text); - } - if let Some(fix) = &e.fix { - obj["fix"] = json!(fix); - } - // Absent rather than empty. `[]` invites a consumer to - // wonder whether the list was computed and came out empty, - // which is the question a missing key already answers. - if !e.next_actions.is_empty() { - obj["next_actions"] = json!(e.next_actions); - } - if !e.docs.is_empty() { - obj["docs"] = json!(e.docs); - } - obj - } + Some(e) => error_payload(e), // `{:#}` flattens anyhow's context chain into one line, so a // wrapped error keeps the context that explains it. None => json!({ "code": GENERIC_CODE, "message": format!("{err:#}") }), @@ -1460,6 +1521,9 @@ pub fn emit_error(mode: Mode, err: &anyhow::Error) { if let Some(fix) = &e.fix { eprintln!("Fix: {fix}"); } + if let Some(request_id) = e.support_request_id() { + eprintln!("Request ID: {request_id} (quote this to Mapbox support)"); + } eprint_labelled("Next", &e.next_actions); eprint_labelled("Docs", &e.docs); } @@ -2932,4 +2996,60 @@ request-id: abc123 assert!(adds_detail(&json!({ "message": "nope", "code": 12 }))); assert!(adds_detail(&json!(["a"]))); } + + /// The `json` rendering carries the request id on every failure that had + /// one. A consumer logging errors wants it on all of them, and a field + /// costs nothing to ignore. + #[test] + fn the_json_error_carries_the_request_id_at_any_status() { + for status in [404u16, 429, 500, 503] { + let err = CliError::http(status, r#"{"message":"nope"}"#) + .with_request_id(Some("req-abc123".to_string())); + let payload = error_payload(&err); + assert_eq!( + payload["request_id"], + json!("req-abc123"), + "missing at {status}" + ); + } + } + + /// Absent rather than null, the same rule the other optional keys follow. + #[test] + fn a_failure_without_a_request_id_has_no_such_key() { + let payload = error_payload(&CliError::http(404, r#"{"message":"nope"}"#)); + assert!(payload.get("request_id").is_none(), "{payload}"); + } + + /// The `text` rendering shows it for a server fault and nothing else: + /// a 404 on a mistyped id is the reader's to fix, and an id under it + /// would be noise on the common case. + #[test] + fn the_text_error_shows_the_request_id_only_for_a_server_fault() { + let with_id = |status: u16| { + CliError::http(status, r#"{"message":"nope"}"#) + .with_request_id(Some("req-abc123".to_string())) + }; + + for quiet in [400u16, 401, 403, 404, 422, 429] { + assert_eq!(with_id(quiet).support_request_id(), None, "at {quiet}"); + } + for loud in [500u16, 502, 503, 504] { + assert_eq!( + with_id(loud).support_request_id(), + Some("req-abc123"), + "at {loud}" + ); + } + } + + /// A failure with no HTTP status at all — a local one, like an unreadable + /// `--file` — cannot have come with a request id, and must not claim one. + #[test] + fn a_local_failure_shows_no_request_id() { + let err = CliError::new("invalid_file", "no such file") + .with_request_id(Some("req-abc123".to_string())); + assert_eq!(err.status, None); + assert_eq!(err.support_request_id(), None); + } } diff --git a/src/schema.rs b/src/schema.rs index 520d47a..e3e3003 100644 --- a/src/schema.rs +++ b/src/schema.rs @@ -258,7 +258,15 @@ fn relaxed(cmd: Command) -> Command { pub fn emit(mode: Mode, app: &Command, specs: &[ServiceSpec], matches: &ArgMatches) -> Result<()> { let path = target_path(matches); let schema = build(app, specs, &path); - output::emit_value(json_mode(mode), &serde_json::to_value(schema)?, None, None) + output::emit_value( + json_mode(mode), + &serde_json::to_value(schema)?, + None, + None, + // `--schema` describes the binary, not an API response; there is no + // page after it. + None, + ) } /// `--schema` answers in JSON whatever `--output` says, because there is no diff --git a/tests/source_guards.rs b/tests/source_guards.rs index 993f6f6..4cc10c7 100644 --- a/tests/source_guards.rs +++ b/tests/source_guards.rs @@ -170,3 +170,73 @@ fn only_output_completion_and_binary_responses_write_to_stdout() { unexpected.join("\n ") ); } + +/// Modules that turn a Mapbox API failure into a `CliError::http`, and so +/// must carry the response's `X-Request-Id` into it. +const CARRIES_A_REQUEST_ID: &[&str] = &["account_usage.rs", "auth.rs", "executor.rs"]; + +/// Modules that raise `CliError::http` and deliberately carry no request id. +/// +/// `agent_skills.rs` talks to GitHub codeload, which identifies requests with +/// `x-github-request-id`. That is not something Mapbox support can look up, +/// so an id there would point at the wrong company — worse than none. +const NO_REQUEST_ID_TO_CARRY: &[&str] = &["agent_skills.rs"]; + +/// `output.rs` defines `CliError::http` rather than calling it over a wire. +const NOT_A_SEND_PATH: &[&str] = &["output.rs"]; + +/// A new path that reports an API failure has to decide about the request id. +/// +/// The failure this is aimed at is not a missing field — it is the shape of +/// the mistake that made #117 worth filing: `bytes()` and `text()` both +/// consume the response, so every header not read *before* the body is gone +/// for good. Someone adding a fifth send path will read the status and the +/// body, because those are what the code after it needs, and the id will be +/// unrecoverable by the time anyone wants it. Being on one of two lists is a +/// decision; being on neither is an oversight, which is what this catches. +#[test] +fn every_mapbox_failure_path_carries_the_request_id() { + for (name, body) in sources() { + let file = name.as_str(); + if NOT_A_SEND_PATH.contains(&file) { + continue; + } + let raises = body.contains("CliError::http("); + let carries = CARRIES_A_REQUEST_ID.contains(&file); + let exempt = NO_REQUEST_ID_TO_CARRY.contains(&file); + + assert!( + !(carries && exempt), + "{file} is on both lists; it cannot both carry an id and have none to carry" + ); + + if carries { + assert!( + raises, + "{file} is listed in CARRIES_A_REQUEST_ID but no longer raises \ + CliError::http — drop it from the list" + ); + assert!( + body.contains("with_request_id") || body.contains("request_id("), + "{file} reports an API failure without carrying its request id. \ + Read `executor::request_id(response.headers())` before the body, \ + because `text()`/`bytes()` consume the response and the header is \ + gone afterwards." + ); + } else if exempt { + assert!( + raises, + "{file} is listed in NO_REQUEST_ID_TO_CARRY but no longer raises \ + CliError::http — drop it from the list" + ); + } else { + assert!( + !raises, + "{file} raises CliError::http but is on neither request-id list. \ + Either carry the id (see `executor::request_id`) and add it to \ + CARRIES_A_REQUEST_ID, or add it to NO_REQUEST_ID_TO_CARRY with the \ + reason this endpoint has no id Mapbox support could look up." + ); + } + } +}