From 0c369b91219eb8bbf09593a570e3f62cc976db9e Mon Sep 17 00:00:00 2001 From: Matthew Podwysocki Date: Mon, 14 Sep 2026 13:10:57 -0400 Subject: [PATCH] Let --data read the body: @ from a file, @- from stdin MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `--data` could only carry a body typed on the command line, so sending a style from a file meant `--data "$(cat style.json)"`. That works on macOS and Linux and nowhere else this CLI ships to: there is no `cmd.exe` equivalent of `$(cat …)`, the body lands in the process table where `ps` shows it, and it breaks at a size that fails in the shell *before* `mapbox` runs — so no remedy string can explain it. `@` and `@-` are curl's spelling. Only the first character decides, so `--data '{"contact":"a@b.example"}'` is still the body it looks like; a body whose first character is a literal `@` cannot be passed this way, which costs nothing because all five operations that take `--data` send JSON and `@` is not valid JSON. Resolved before the dry-run branch, so `--dry-run` validates the file too. **The timeout had to move with it.** `Payload::Bounded`'s reasoning was "a `--data` body was typed on a command line, and every operating system caps how much one of those can hold" — true until this change, and `BodySource::Json` looks identical whether it was typed or read from a 200 MB file. A read body now gets the 900-second transfer budget, so `payload_of` takes the provenance rather than inferring it from a body that no longer carries the distinction. Three smaller decisions, each with a test: read as text, because a JSON body has to be UTF-8 and a lossy conversion would be rejected for reasons naming nothing the caller did; sent byte for byte, because trimming a supplied body would be the CLI editing what it was asked to send; and an empty source names itself, because `cat missing.json | mapbox …` otherwise fails as "EOF while parsing a value", which describes the parser rather than the pipe. Documented one interaction rather than changing it: of the five operations only `sprites delete-batch` is a DELETE, and a confirmation needs stdin to be a terminal — which a pipe is not. So `@-` sends it unasked, exactly as `< file` always did. `@` keeps the prompt. Verified live against the API through both forms, not only under `--dry-run`. Ported from mapbox/mapbox-cli-private#136, which cannot merge there now that `oss/` is a submodule (mapbox/mapbox-cli-private#132). --- CHANGELOG.md | 10 ++ docs/commands.md | 33 ++++- src/executor.rs | 311 +++++++++++++++++++++++++++++++++++++++++++---- src/http.rs | 5 +- src/main.rs | 11 +- 5 files changed, 343 insertions(+), 27 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c85f71d..eb1db80 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -33,6 +33,16 @@ the Mapbox APIs' own response bodies are not. it. `mapbox agent-skills` is exempt — it fetches from GitHub, whose request id Mapbox support cannot look up. +- `--data` can now read the request body instead of carrying it: `@` + reads a file and `@-` reads stdin, the spelling curl uses. Before this the + only way to send a style from a file was `--data "$(cat style.json)"`, + which has no `cmd.exe` equivalent on a platform this CLI ships installers + for, put the body in the process table where `ps` shows it, and broke at a + size that failed in the shell before `mapbox` ran — so no error message + could explain it. Five operations take `--data`. A body read this way also + gets the 900-second transfer budget rather than the 60-second one, since + nothing bounds a file the way a command line bounds what can be typed. + ## 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 a09eb2c..9ad9d50 100644 --- a/docs/commands.md +++ b/docs/commands.md @@ -462,6 +462,36 @@ and `--file ` when it is bytes: raw for `application/octet-stream` and operations declare both and reject having both passed. Each command's own **Parameters** below lists only what is specific to it. +`--data` does not have to carry the body itself. Following curl, +**`@` reads a file and `@-` reads stdin**: + +```sh +mapbox styles create --data @style.json +jq '.name = "Renamed"' style.json | mapbox styles update STYLE_ID --data @- +``` + +Only the first character decides, so `--data '{"contact":"a@b.example"}'` is +still the body it looks like. A body whose *first* character is a literal `@` +cannot be passed this way — curl has the same limitation, and it costs +nothing here because every operation that takes `--data` sends JSON, and `@` +is not valid JSON. + +Three things worth knowing about the read forms: + +- **The body is sent byte for byte.** The trailing newline a text editor + leaves is insignificant to a JSON parser and is not stripped, because + trimming a body the caller supplied would be the CLI editing what it was + asked to send. +- **The timeout changes with it.** A typed `--data` is capped by the command + line at a megabyte or so and gets the 60-second budget; `@` and `@-` + are unbounded and get the same 900 seconds `--file` does. +- **`@-` suppresses the confirmation on a delete.** Of the five operations + that take `--data`, only `mapbox sprites delete-batch` is a `DELETE`, and + a question needs stdin to be a terminal — which a pipe is not. So piping a + body into it sends it unasked, exactly as `< file` always did. Use + `@` rather than `@-` to keep the prompt, or pass `--yes` to say the + answer deliberately. + A command that changes something takes `--dry-run`, which prints the request it would send, on stdout, and sends nothing. Which commands those are is not a list anyone keeps: it is every `POST`, `PUT`, `PATCH` and `DELETE` — 12 of @@ -3513,7 +3543,8 @@ names the flag rather than offering a login that could not outrank it. | `request_failed` | Transport failure — proxy, DNS, TLS. Never carries the URL, because the access token rides in its query string. | | `request_timed_out` | The request ran out of its time budget. Its own code because it is the one transport failure worth retrying or raising `--timeout` for. | | `missing_path_parameters` | A `{username}`/`{owner}`/`{account}` placeholder went unresolved. | -| `invalid_data` | `--data` was not valid JSON. | +| `invalid_data` | `--data` was not valid JSON, or a `@`/`@-` body was empty. | +| `invalid_file` | A file could not be read: one named by `--file`, or one named by `--data @`. Also a `@` that is not valid UTF-8, which a JSON body has to be. | | `binary_response` | The response was bytes and stdout is a terminal. Redirect it to a file. | | `missing_subcommand` | A command group was named with no operation. | | `cancelled` | A delete was declined at the confirmation prompt. Nothing was sent. | diff --git a/src/executor.rs b/src/executor.rs index 700f4e8..fbee348 100644 --- a/src/executor.rs +++ b/src/executor.rs @@ -169,11 +169,16 @@ fn dispatch( // `--data` and `--file` are declared per-operation, so either may be // absent from this command entirely; `get_one` panics on an argument // that was never registered. - let data = matches - .try_get_one::("data") - .ok() - .flatten() - .map(String::as_str); + // Resolved before anything reads it, `--dry-run` included, so a `@path` + // that does not exist fails here rather than being described as a request + // and then failing at send time. `resolve_data` says why. + let data_argument = match matches.try_get_one::("data").ok().flatten() { + Some(value) => Some(resolve_data(value)?), + None => None, + }; + let data = data_argument + .as_ref() + .map(|argument| argument.body.as_ref()); let files: Vec<&str> = matches .try_get_many::("file") .ok() @@ -238,7 +243,15 @@ fn dispatch( // `reqwest` prefers the request's own over the client's — 0.12.28's // `execute_request` reads `req.timeout().copied().or(self.timeout.0)` — // so this is what actually applies to everything sent from here. - .timeout(http::budget(timeout, payload_of(body_source.as_ref()))); + .timeout(http::budget( + timeout, + payload_of( + body_source.as_ref(), + data_argument + .as_ref() + .is_some_and(|argument| argument.streamed), + ), + )); if let Some(source) = body_source { req = attach_body(req, source)?; @@ -754,6 +767,117 @@ enum BodySource<'a> { }, } +/// What `--data` was given, once a `@path` or `@-` has been read. +#[derive(Debug)] +struct DataArgument<'a> { + /// The body itself. Borrowed when it was typed, owned when it was read. + body: std::borrow::Cow<'a, str>, + /// Whether it came from a file or stdin rather than from argv. + /// + /// Carried because the timeout budget turns on it and on nothing else a + /// caller can see: argv caps what can be typed at roughly a megabyte, and + /// nothing caps a file. See [`payload_of`]. + streamed: bool, +} + +/// The `--data` prefix that means "read this rather than send it". +const DATA_FROM_PATH: char = '@'; + +/// The `@` path that means stdin, spelled as curl and every other tool spell +/// it. +const STDIN_PATH: &str = "-"; + +/// Resolves a `--data` argument that names a file instead of carrying a body. +/// +/// `@path` reads the file, `@-` reads stdin, and anything else is the body +/// itself — the spelling curl has used for long enough that it is what people +/// try first. +/// +/// The ambiguity this inherits is curl's: a body whose first character is a +/// literal `@` cannot be passed this way. It costs nothing here, because every +/// operation reachable with `--data` today sends JSON, and `@` is not valid +/// JSON. If a text body that could start with one is ever wired up, `--data-raw` +/// is the established escape hatch — see mapbox/mapbox-cli-private#118. +/// +/// Read here rather than at send time so that a `--dry-run` validates the file +/// too. A dry run that skipped this would describe a request that could not +/// actually be sent, which is the one thing it exists to rule out. +fn resolve_data(value: &str) -> Result> { + let Some(path) = value.strip_prefix(DATA_FROM_PATH) else { + return Ok(DataArgument { + body: std::borrow::Cow::Borrowed(value), + streamed: false, + }); + }; + + if path.is_empty() { + return Err(CliError::new( + "invalid_data", + "`--data @` names nothing to read. Use `@` for a file, or `@-` for stdin.", + ) + .into()); + } + + let (body, source) = if path == STDIN_PATH { + (read_stdin()?, "stdin".to_string()) + } else { + (read_data_file(path)?, format!("`{path}`")) + }; + + // An empty body reaches `attach_body` as invalid JSON and is reported as + // one — "EOF while parsing a value" — which describes the symptom and not + // the mistake. The mistake is almost always a pipe that produced nothing + // (`cat missing.json | mapbox …`, whose own error went to the same stderr + // and scrolled past), and naming the source is what points at it. + if body.trim().is_empty() { + return Err(CliError::new( + "invalid_data", + format!("{source} was empty, so there is no request body to send."), + ) + .into()); + } + + Ok(DataArgument { + body: std::borrow::Cow::Owned(body), + streamed: true, + }) +} + +/// A `--data @path` file, as text. +/// +/// Text rather than bytes, and that is a check rather than a convenience: a +/// JSON body has to be UTF-8, so a file that is not says so here instead of +/// being lossily converted into a body the API would reject for reasons that +/// name nothing the caller did. +fn read_data_file(path: &str) -> Result { + std::fs::read_to_string(path).map_err(|e| { + let message = if e.kind() == std::io::ErrorKind::InvalidData { + format!("`{path}` is not valid UTF-8, so it cannot be sent as a JSON body.") + } else { + format!("Cannot read `{path}`: {e}") + }; + CliError::new("invalid_file", message).into() + }) +} + +/// A `--data @-` body, from stdin. +fn read_stdin() -> Result { + use std::io::Read; + + let mut body = String::new(); + std::io::stdin() + .read_to_string(&mut body) + .map_err(|e| -> anyhow::Error { + let message = if e.kind() == std::io::ErrorKind::InvalidData { + "stdin is not valid UTF-8, so it cannot be sent as a JSON body.".to_string() + } else { + format!("Cannot read the request body from stdin: {e}") + }; + CliError::new("invalid_file", message).into() + })?; + Ok(body) +} + /// Picks between `--data` and `--file` for an operation that takes a body. /// /// Pure, and kept that way: every rejection here is a mistake the caller can @@ -822,19 +946,23 @@ fn resolve_body_source<'a>( /// How much the request is about to move, which is all the budget turns on. /// -/// Only `--file` counts as unbounded. A `--data` body was typed on a command -/// line, and every operating system caps how much one of those can hold — two -/// megabytes at the outside, which goes out inside the ordinary budget with -/// room to spare. `--file` names something on disk, and the sprite and upload -/// operations exist precisely for the cases where that is large. +/// `--file` is unbounded, and so is a `--data @path` or `--data @-`, which is +/// why this takes a second argument rather than reading the body alone. A +/// `--data` body *typed* on a command line is capped by argv at a megabyte or +/// so and goes out inside the ordinary budget with room to spare — that was +/// once true of every `--data` body, and the reasoning is the thing `@path` +/// broke: `BodySource::Json` looks identical whether it was typed or read from +/// a 200 MB file, and the second would have been given a sixty-second budget +/// it could not meet. /// /// The response is not consulted, because nothing here knows it yet: six of /// the twelve services answer with bytes, but a tile, a glyph range and a /// style ZIP all arrive well inside a minute, so the one shape worth /// separating out is the one this CLI is sending. -fn payload_of(body: Option<&BodySource<'_>>) -> http::Payload { +fn payload_of(body: Option<&BodySource<'_>>, data_was_read: bool) -> http::Payload { match body { Some(BodySource::Raw { .. } | BodySource::Multipart { .. }) => http::Payload::File, + _ if data_was_read => http::Payload::File, _ => http::Payload::Bounded, } } @@ -1144,9 +1272,9 @@ 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, 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, + 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, REQUEST_ID_HEADERS, }; use crate::http::Payload; use crate::output::CliError; @@ -1595,17 +1723,158 @@ mod tests { paths: vec!["a.svg", "b.svg"], field: "images", }; - assert_eq!(payload_of(Some(&raw)), Payload::File); - assert_eq!(payload_of(Some(&multipart)), Payload::File); + assert_eq!(payload_of(Some(&raw), false), Payload::File); + assert_eq!(payload_of(Some(&multipart), false), Payload::File); + + let text = BodySource::Text { + data: "true", + content_type: "text/plain", + }; + assert_eq!(payload_of(None, false), Payload::Bounded); + assert_eq!( + payload_of(Some(&BodySource::Empty), false), + Payload::Bounded + ); + assert_eq!( + payload_of(Some(&BodySource::Json("{}")), false), + Payload::Bounded + ); + assert_eq!(payload_of(Some(&text), false), Payload::Bounded); + } + /// A body read from `@path` or `@-` is indistinguishable from a typed one + /// by the time it reaches `BodySource::Json`, and nothing bounds its size. + /// Given the sixty-second budget, a large one would fail on a timeout that + /// described the network rather than the choice of flag. + #[test] + fn a_data_body_that_was_read_gets_the_transfer_budget() { + assert_eq!( + payload_of(Some(&BodySource::Json("{}")), true), + Payload::File + ); let text = BodySource::Text { data: "true", content_type: "text/plain", }; - assert_eq!(payload_of(None), Payload::Bounded); - assert_eq!(payload_of(Some(&BodySource::Empty)), Payload::Bounded); - assert_eq!(payload_of(Some(&BodySource::Json("{}"))), Payload::Bounded); - assert_eq!(payload_of(Some(&text)), Payload::Bounded); + assert_eq!(payload_of(Some(&text), true), Payload::File); + } + + #[test] + fn a_plain_data_argument_is_the_body_itself() { + let resolved = resolve_data(r#"{"version":8}"#).expect("a literal body"); + assert_eq!(resolved.body, r#"{"version":8}"#); + assert!(!resolved.streamed, "argv bounds it"); + } + + #[test] + fn an_at_path_is_read_from_disk() { + let dir = tempdir(); + let path = dir.join("style.json"); + std::fs::write(&path, "{\"version\":8}\n").expect("write the style"); + + let spec = format!("@{}", path.display()); + let resolved = resolve_data(&spec).expect("the file is read"); + assert_eq!(resolved.body, "{\"version\":8}\n"); + assert!(resolved.streamed, "nothing bounds a file"); + } + + /// The trailing newline a text editor leaves is *not* stripped. It is + /// insignificant to every JSON parser, and trimming a body the caller + /// supplied would be this CLI quietly editing what it was asked to send. + #[test] + fn a_read_body_is_sent_byte_for_byte() { + let dir = tempdir(); + let path = dir.join("body.json"); + std::fs::write(&path, " {\"a\": 1} \n\n").expect("write the body"); + + let spec = format!("@{}", path.display()); + let resolved = resolve_data(&spec).expect("read"); + assert_eq!(resolved.body, " {\"a\": 1} \n\n"); + } + + #[test] + fn a_missing_at_path_names_the_path_rather_than_crashing() { + let cli = refusal(resolve_data("@/no/such/style.json").unwrap_err()); + assert_eq!(cli.code, "invalid_file"); + assert!( + cli.message.contains("/no/such/style.json"), + "{}", + cli.message + ); + } + + /// A JSON body has to be UTF-8, so this is a check rather than a + /// convenience — the alternative is a lossy conversion the API rejects for + /// reasons that name nothing the caller did. + #[test] + fn a_non_utf8_file_says_so_rather_than_being_mangled() { + let dir = tempdir(); + let path = dir.join("bytes.json"); + std::fs::write(&path, [0x7b, 0xff, 0xfe, 0x7d]).expect("write the bytes"); + + let spec = format!("@{}", path.display()); + let cli = refusal(resolve_data(&spec).unwrap_err()); + assert_eq!(cli.code, "invalid_file"); + assert!(cli.message.contains("not valid UTF-8"), "{}", cli.message); + } + + /// The mistake is almost always a pipe that produced nothing, and the + /// symptom without this is "EOF while parsing a value", which names the + /// parser rather than the pipe. + #[test] + fn an_empty_file_says_which_source_was_empty() { + let dir = tempdir(); + let path = dir.join("empty.json"); + std::fs::write(&path, " \n").expect("write whitespace"); + + let spec = format!("@{}", path.display()); + let cli = refusal(resolve_data(&spec).unwrap_err()); + assert_eq!(cli.code, "invalid_data"); + assert!(cli.message.contains("was empty"), "{}", cli.message); + assert!(cli.message.contains("empty.json"), "{}", cli.message); + } + + #[test] + fn a_bare_at_sign_names_both_forms() { + let cli = refusal(resolve_data("@").unwrap_err()); + assert_eq!(cli.code, "invalid_data"); + assert!(cli.message.contains("@"), "{}", cli.message); + assert!(cli.message.contains("@-"), "{}", cli.message); + } + + /// A body that merely *contains* an `@` is not a path. Only the first + /// character decides, which is what makes `--data '{"a":"b@c"}'` safe. + #[test] + fn an_at_sign_inside_the_body_is_not_a_path() { + let resolved = resolve_data(r#"{"email":"a@b.example"}"#).expect("a literal body"); + assert!(!resolved.streamed); + assert_eq!(resolved.body, r#"{"email":"a@b.example"}"#); + } + + /// A scratch directory that cleans itself up, so these tests leave + /// nothing behind and cannot collide with each other. + fn tempdir() -> TempDir { + let base = std::env::temp_dir().join(format!( + "mapbox-cli-data-{}-{:?}", + std::process::id(), + std::thread::current().id() + )); + std::fs::create_dir_all(&base).expect("create a scratch directory"); + TempDir(base) + } + + struct TempDir(std::path::PathBuf); + + impl TempDir { + fn join(&self, name: &str) -> std::path::PathBuf { + self.0.join(name) + } + } + + impl Drop for TempDir { + fn drop(&mut self) { + let _ = std::fs::remove_dir_all(&self.0); + } } /// A declared query parameter, with only the fields these tests read set diff --git a/src/http.rs b/src/http.rs index f87766b..ea1a636 100644 --- a/src/http.rs +++ b/src/http.rs @@ -78,7 +78,10 @@ pub enum Payload { /// No body, or one typed into `--data` — argv bounds it — and a response /// no larger than a tile, a glyph range or a style. Bounded, - /// A file named by `--file`. Nothing bounds it. + /// A file: named by `--file`, or read by `--data @` / `--data @-`. + /// Nothing bounds it. `executor::payload_of` decides which of the two + /// `--data` cases a request is, since the body looks the same either way + /// by the time it is built. File, } diff --git a/src/main.rs b/src/main.rs index 35c818c..d758cbd 100644 --- a/src/main.rs +++ b/src/main.rs @@ -242,10 +242,13 @@ fn build_operation_command(op: &spec::Operation) -> Command { .long("data") .short('d') .help(match text_body { - Some(content_type) => { - format!("Request body, sent verbatim as {content_type}") - } - None => "Request body as JSON string".to_string(), + Some(content_type) => format!( + "Request body, sent verbatim as {content_type}. \ + `@` reads a file, `@-` reads stdin" + ), + None => "Request body as a JSON string, or `@` to read a file \ + and `@-` to read stdin" + .to_string(), }), ); }