Repository: ducaale/xh (rust)

Issue:
Option to set filename for multipart uploads
See header "Pinning a Directory Example": https://pinata.cloud/documentation#PinFileToIPFS

Essentially, I need to be able to set the filename of a multipart form part to be, e.g. `./foo/bar.jpg` and not just `bar.jpg` if I upload `foo/bar.jpg`.

Looks like the request item parsing already supports overriding the `Content-Type` of the file part with `;type=`: https://github.com/ducaale/xh/blob/master/src/request_items.rs#L105

So maybe `file@./foo/bar.jpg;filename=./foo/bar.jpg`? It's a bit verbose but I can work with it.

Pull request:
Add ;filename= parameter for multipart file uploads
Resolves #162.

- [x] Handle for body from file
- [x] Integration tests
- [x] Documentation

Patch:
diff --git a/README.md b/README.md
index 9e1e814..66b3bb4 100644
--- a/README.md
+++ b/README.md
@@ -90,7 +90,7 @@ Run `xh help` for more detailed information.
 
 - `=`/`:=` for setting the request body's JSON or form fields (`=` for strings and `:=` for other JSON types).
 - `==` for adding query strings.
-- `@` for including files in multipart requests e.g `picture@hello.jpg` or `picture@hello.jpg;type=image/jpeg`.
+- `@` for including files in multipart requests e.g `picture@hello.jpg` or `picture@hello.jpg;type=image/jpeg;filename=goodbye.jpg`.
 - `:` for adding or removing headers e.g `connection:keep-alive` or `connection:`.
 - `;` for including headers with empty values e.g `header-without-value;`.
 - `=@`/`:=@` for setting the request body's JSON or form fields from a file (`=` for strings and `:=` for other JSON types).
diff --git a/doc/man-template.roff b/doc/man-template.roff
index 9b9bf40..1e93666 100644
--- a/doc/man-template.roff
+++ b/doc/man-template.roff
@@ -38,4 +38,8 @@ Optional key-value pairs to be included in the request.
 \fBheader;\fR to add a header with an empty value
 .IP
 A backslash can be used to escape special characters (e.g. weird\\:key=value).
+.IP
+File uploads can take ';type=' and ';filename=' parameters to set the mimetype and filename, respectively. For example:
+.IP
+  key@filename;type=text/html;filename=document.html
 .SS "Flags"
diff --git a/src/main.rs b/src/main.rs
index 5714bb7..7c0371c 100644
--- a/src/main.rs
+++ b/src/main.rs
@@ -1,3 +1,4 @@
+#![allow(clippy::bool_assert_comparison)]
 mod auth;
 mod buffer;
 mod cli;
@@ -199,6 +200,10 @@ fn main() -> Result<i32> {
             Body::File {
                 file_name,
                 file_type,
+                // We could turn this into a Content-Disposition header, but
+                // that has no effect, so just ignore it
+                // (Additional precedent: HTTPie ignores file_type here)
+                file_name_header: _,
             } => request_builder.body(File::open(file_name)?).header(
                 CONTENT_TYPE,
                 file_type.unwrap_or_else(|| HeaderValue::from_static(JSON_CONTENT_TYPE)),
diff --git a/src/request_items.rs b/src/request_items.rs
index b3ead61..4bf98f5 100644
--- a/src/request_items.rs
+++ b/src/request_items.rs
@@ -29,6 +29,7 @@ pub enum RequestItem {
         key: String,
         file_name: String,
         file_type: Option<String>,
+        file_name_header: Option<String>,
     },
 }
 
@@ -100,25 +101,17 @@ impl FromStr for RequestItem {
                     })?,
                 )),
                 "@" => {
-                    // Technically there are concerns about escaping but people
-                    // probably don't put ;type= in their filenames often
-                    let with_type: Vec<&str> = value.rsplitn(2, ";type=").collect();
-                    // rsplitn iterates from the right, so it's either
-                    if let Some(&typed_filename) = with_type.get(1) {
-                        // [mimetype, filename]
-                        Ok(RequestItem::FormFile {
-                            key,
-                            file_name: typed_filename.to_owned(),
-                            file_type: Some(with_type[0].to_owned()),
-                        })
-                    } else {
-                        // [filename]
-                        Ok(RequestItem::FormFile {
-                            key,
-                            file_name: value,
-                            file_type: None,
-                        })
-                    }
+                    let PartWithParams {
+                        value,
+                        file_type,
+                        file_name_header,
+                    } = parse_part_params(&value);
+                    Ok(RequestItem::FormFile {
+                        key,
+                        file_name: value,
+                        file_type,
+                        file_name_header,
+                    })
                 }
                 ":" if value.is_empty() => Ok(RequestItem::HttpHeaderToUnset(key)),
                 ":" => Ok(RequestItem::HttpHeader(key, value)),
@@ -141,6 +134,76 @@ impl FromStr for RequestItem {
     }
 }
 
+#[derive(Debug, PartialEq)]
+struct PartWithParams {
+    value: String,
+    file_type: Option<String>,
+    file_name_header: Option<String>,
+}
+
+/// HTTPie's syntax for this is imitating curl's.
+///
+/// curl's syntax is pretty hairy. At the most basic level it's just key-value
+/// pairs separated by semicolons, but:
+/// - Values may be quoted. This stops spaces from being stripped and allows
+///   you to put semicolons in values. (Between quotes, quotes and backslashes
+///   have to be backslash-escaped.)
+/// - If a key is not recognized then it's skipped with a warning.
+///   - Unless it comes right after a mimetype, in which case it's seen as part
+///     of the last value, because mimetypes can use the exact same syntax
+///     (e.g. `text/html; charset=UTF-8`).
+///     `;type=text/plain;filename=foobar` will send Content-Type `text/plain`
+///     and filename `foobar`, but `;type=text/plain;foo=bar` will send
+///     Content-Type `text/plain;foo=bar`.
+///
+/// We'll cut some corners and just split on ";type=" and ";filename=". That should
+/// be good enough for most purposes. (HTTPie only splits on ";type=".)
+fn parse_part_params(mut text: &str) -> PartWithParams {
+    const TYPE_SEP: &str = ";type=";
+    const FNAME_SEP: &str = ";filename=";
+
+    let mut file_type = None;
+    let mut file_name_header = None;
+
+    // Look for parameters starting from the right.
+    // Only look for a parameter as long as it hasn't been found yet.
+    // (There may be a cleaner way, this is the best I could come up with.)
+    let mut delims = vec![TYPE_SEP, FNAME_SEP];
+    while let Some((pre, delim, post)) = rsplit_once_any(text, &delims) {
+        match delim {
+            TYPE_SEP => file_type = Some(post.to_owned()),
+            FNAME_SEP => file_name_header = Some(post.to_owned()),
+            _ => unreachable!(),
+        }
+        delims.retain(|&x| x != delim);
+        text = pre;
+    }
+
+    PartWithParams {
+        value: text.to_owned(),
+        file_type,
+        file_name_header,
+    }
+}
+
+/// Find the rightmost match of any of the delimiters and do a split.
+fn rsplit_once_any<'a, 'b>(
+    text: &'a str,
+    delimiters: &'b [&'static str],
+) -> Option<(&'a str, &'static str, &'a str)> {
+    let mut res = None;
+    let mut best = 0;
+    for &delim in delimiters {
+        if let Some(pos) = text.rfind(delim) {
+            if pos >= best {
+                best = pos;
+                res = Some((&text[..pos], delim, &text[pos + delim.len()..]));
+            }
+        }
+    }
+    res
+}
+
 pub struct RequestItems(pub Vec<RequestItem>);
 
 pub enum Body {
@@ -151,6 +214,7 @@ pub enum Body {
     File {
         file_name: PathBuf,
         file_type: Option<HeaderValue>,
+        file_name_header: Option<String>,
     },
 }
 
@@ -292,11 +356,15 @@ impl RequestItems {
                     key,
                     file_name,
                     file_type,
+                    file_name_header,
                 } => {
                     let mut part = file_to_part(&file_name)?;
                     if let Some(file_type) = file_type {
                         part = part.mime_str(&file_type)?;
                     }
+                    if let Some(file_name_header) = file_name_header {
+                        part = part.file_name(file_name_header);
+                    }
                     form = form.part(key, part);
                 }
                 RequestItem::HttpHeader(..) => {}
@@ -332,6 +400,7 @@ impl RequestItems {
                     key,
                     file_name,
                     file_type,
+                    file_name_header,
                 } => {
                     assert!(key.is_empty());
                     if body.is_some() {
@@ -344,6 +413,7 @@ impl RequestItems {
                             .map(HeaderValue::from_str)
                             .transpose()?,
                         file_name: file_name.into(),
+                        file_name_header,
                     });
                 }
                 RequestItem::HttpHeader(..)
@@ -473,7 +543,8 @@ mod tests {
             FormFile {
                 key: "foo".into(),
                 file_name: "bar".into(),
-                file_type: None
+                file_type: None,
+                file_name_header: None,
             }
         );
         // Typed file
@@ -482,7 +553,8 @@ mod tests {
             FormFile {
                 key: "foo".into(),
                 file_name: "bar".into(),
-                file_type: Some("qux".into())
+                file_type: Some("qux".into()),
+                file_name_header: None,
             },
         );
         // Multi-typed file
@@ -491,7 +563,8 @@ mod tests {
             FormFile {
                 key: "foo".into(),
                 file_name: "bar;type=qux".into(),
-                file_type: Some("qux".into())
+                file_type: Some("qux".into()),
+                file_name_header: None,
             },
         );
         // Empty filename
@@ -501,7 +574,8 @@ mod tests {
             FormFile {
                 key: "foo".into(),
                 file_name: "".into(),
-                file_type: None
+                file_type: None,
+                file_name_header: None,
             }
         );
         // No separator
@@ -519,4 +593,56 @@ mod tests {
         // Empty
         assert_eq!(parse("="), DataField("".into(), "".into()));
     }
+
+    #[test]
+    fn param_parsing() {
+        assert_eq!(
+            parse_part_params("foo;type=bar;filename=baz"),
+            PartWithParams {
+                value: "foo".into(),
+                file_type: Some("bar".into()),
+                file_name_header: Some("baz".into()),
+            }
+        );
+        assert_eq!(
+            parse_part_params(";type=foo"),
+            PartWithParams {
+                value: "".into(),
+                file_type: Some("foo".into()),
+                file_name_header: None,
+            }
+        );
+        assert_eq!(
+            parse_part_params("foo;type=bar;type=baz;filename=qux"),
+            PartWithParams {
+                value: "foo;type=bar".into(),
+                file_type: Some("baz".into()),
+                file_name_header: Some("qux".into()),
+            }
+        );
+        assert_eq!(
+            parse_part_params("foo;type=bar;filename=qux;type=baz"),
+            PartWithParams {
+                value: "foo;type=bar".into(),
+                file_type: Some("baz".into()),
+                file_name_header: Some("qux".into()),
+            }
+        );
+        assert_eq!(
+            parse_part_params("foo;x=y"),
+            PartWithParams {
+                value: "foo;x=y".into(),
+                file_type: None,
+                file_name_header: None,
+            }
+        );
+        assert_eq!(
+            parse_part_params(""),
+            PartWithParams {
+                value: "".into(),
+                file_type: None,
+                file_name_header: None,
+            }
+        );
+    }
 }
diff --git a/src/to_curl.rs b/src/to_curl.rs
index 42f82d2..5f8caf6 100644
--- a/src/to_curl.rs
+++ b/src/to_curl.rs
@@ -270,13 +270,19 @@ pub fn translate(args: Cli) -> Result<Command> {
                     key,
                     file_name,
                     file_type,
+                    file_name_header,
                 } => {
                     cmd.flag("-F", "--form");
+                    let mut val = format!("{}=@{}", key, file_name);
                     if let Some(file_type) = file_type {
-                        cmd.push(format!("{}=@{};type={}", key, file_name, file_type));
-                    } else {
-                        cmd.push(format!("{}=@{}", key, file_name));
+                        val.push_str(";type=");
+                        val.push_str(&file_type);
                     }
+                    if let Some(file_name_header) = file_name_header {
+                        val.push_str(";filename=");
+                        val.push_str(&file_name_header);
+                    }
+                    cmd.push(val);
                 }
                 RequestItem::HttpHeader(..) => {}
                 RequestItem::HttpHeaderToUnset(..) => {}
@@ -320,6 +326,7 @@ pub fn translate(args: Cli) -> Result<Command> {
             Body::File {
                 file_name,
                 file_type,
+                file_name_header: _,
             } => {
                 if let Some(file_type) = file_type {
                     cmd.header("content-type", file_type.to_str()?);


Test patch:
diff --git a/tests/cli.rs b/tests/cli.rs
index e8a155b..db011c6 100644
--- a/tests/cli.rs
+++ b/tests/cli.rs
@@ -1041,6 +1041,51 @@ fn default_json_for_raw_body() {
     mock.assert();
 }
 
+#[test]
+fn multipart_file_upload() {
+    let server = MockServer::start();
+    let mock = server.mock(|when, _| {
+        // This test may be fragile, it's conceivable that the headers will become
+        // lowercase in the future
+        // (so if this breaks all of a sudden, check that first)
+        when.body_contains("Hello world")
+            .body_contains(concat!(
+                "Content-Disposition: form-data; name=\"x\"; filename=\"input.txt\"\r\n",
+                "\r\n",
+                "Hello world\n"
+            ))
+            .body_contains(concat!(
+                "Content-Disposition: form-data; name=\"y\"; filename=\"foobar.htm\"\r\n",
+                "Content-Type: text/html\r\n",
+                "\r\n",
+                "Hello world\n",
+            ));
+    });
+
+    let dir = tempfile::tempdir().unwrap();
+    let filename = dir.path().join("input.txt");
+    OpenOptions::new()
+        .create(true)
+        .write(true)
+        .open(&filename)
+        .unwrap()
+        .write_all(b"Hello world\n")
+        .unwrap();
+
+    get_command()
+        .arg("--form")
+        .arg(server.base_url())
+        .arg(format!("x@{}", filename.to_string_lossy()))
+        .arg(format!(
+            "y@{};type=text/html;filename=foobar.htm",
+            filename.to_string_lossy()
+        ))
+        .assert()
+        .success();
+
+    mock.assert();
+}
+
 #[test]
 fn body_from_file() {
     let server = MockServer::start();