From 9c7e37f7282e399e058a8a2395f911bb443bcac6 Mon Sep 17 00:00:00 2001 From: Julian Aichholz <39018167+rusty-jules@users.noreply.github.com> Date: Mon, 3 Jul 2023 08:20:39 -0700 Subject: [PATCH] Fix: return all headers with the same name from `http ` (#9594) # Description Hi nushell! Thanks so much for [adding http headers][headers]! I've been waiting for this feature for a long time and it's great. However, I found that `Record` as a return type and using `ureq`'s `response.header` api results in missing header values when there are multiple with the same name, as can occur with `set-cookie` and others. This issue with http has been discussed at length [on stackoverflow][stackoverflow] and in [`ureq` itself][ureq]. It seems like concatenating header values with `,` is a common solution, but tricky especially with `set-cookie` which may contain `,` in the `Expires` field, as discussed in the former post. I propose changing the return type to a `List` of `Record` so we can get all of the header values without relying on ad-hoc mutation. This solution does not return the headers in the same order as they appear in the `Response` due to `ureq`'s `Response.all` API, but it's better than dropping values imo. This is a **breaking change**. I'm sure `ureq`'s [`CookieStore`][cookiestore] is a better long-term solution for returning cookies as a separate record on `http `, but other headers can be set multiple times as well. # User-Facing Changes - Changes the return type of an `http ` `header` field from `Record` to `List` (Table with columns `name` and `value`) - Returns all values of a header set multiple times instead of just the first one duplicated # Implementation Quick note that running `header_names.dedup()` does not resolve the necessity to iterate through the previously parsed headers since `dedup` only removes identical values when they are next to each other in the `Vec`. You could do a `sort` first, but header ordering can be important in some cases, so I tried to avoid messing with that more than is already the case with `Response.all`. Would love to see a better way of doing this though! # Tests + Formatting No tests broke implementing this change. Not sure what endpoint to hit or mock server to use to verify this in tests. I have some screenshots to illustrate what I'm talking about. Before: ![Screenshot 2023-07-03 at 12 50 17 AM](https://github.com/nushell/nushell/assets/39018167/41604bef-54c6-424b-91b2-6b89a020e4ff) > `set-cookie` has the same value for every record field. > Even if it did not I'm not sure how you would access the different values since they all have the same key. After: ![Screenshot 2023-07-03 at 12 49 45 AM](https://github.com/nushell/nushell/assets/39018167/4ee45e6e-3785-471f-aee7-5af185cd06c2) > Actual values from the response returned for the same name Make sure you've run and fixed any issues with these commands: - [x] `cargo fmt --all -- --check` to check standard code formatting (`cargo fmt --all` applies these changes) - [x] `cargo clippy --workspace -- -D warnings -D clippy::unwrap_used -A clippy::needless_collect -A clippy::result_large_err` to check that you're using the standard code style - [x] `cargo test --workspace` to check that all tests pass - [x] `cargo run -- crates/nu-std/tests/run.nu` to run the tests for the standard library <-- Note: I did not see a `crates/nu-std/test/run.nu` file so I ran the snippet below which returned without error ```nushell for $i in (ls crates/nu-std/tests/*.nu) { cargo run -- $i.name } ``` # Code of Conduct Apologies for not opening an issue first. Just did this fix for myself because it seemed simple enough before deciding to open this PR. # After Submitting - [ ] update docs [stackoverflow]: https://stackoverflow.com/questions/3241326/set-more-than-one-http-header-with-the-same-name [headers]: https://github.com/nushell/nushell/pull/8571 [ureq]: https://github.com/algesten/ureq/issues/95 [cookiestore]: https://docs.rs/cookie_store/latest/cookie_store/struct.CookieStore.html --- crates/nu-command/src/network/http/client.rs | 41 +++++++++++--------- 1 file changed, 23 insertions(+), 18 deletions(-) diff --git a/crates/nu-command/src/network/http/client.rs b/crates/nu-command/src/network/http/client.rs index d50c21a3df..f53da174d1 100644 --- a/crates/nu-command/src/network/http/client.rs +++ b/crates/nu-command/src/network/http/client.rs @@ -544,29 +544,34 @@ pub fn request_handle_response_headers_raw( span: Span, response: &Response, ) -> Result { - let cols = response.headers_names(); + let header_names = response.headers_names(); - let mut vals = Vec::with_capacity(cols.len()); - for key in &cols { - match response.header(key) { - // match value.to_str() { - Some(str_value) => vals.push(Value::String { - val: str_value.to_string(), - span, - }), - None => { - return Err(ShellError::GenericError( - "Failure when converting header value".to_string(), - "".to_string(), - None, - Some("Failure when converting header value".to_string()), - Vec::new(), - )) + let cols = vec!["name".to_string(), "value".to_string()]; + let mut vals = Vec::with_capacity(header_names.len()); + + for name in &header_names { + let is_duplicate = vals.iter().any(|val| { + if let Value::Record { vals, .. } = val { + if let Some(Value::String { + val: header_name, .. + }) = vals.get(0) + { + return name == header_name; + } + } + false + }); + if !is_duplicate { + // Use the ureq `Response.all` api to get all of the header values with a given name. + // This interface is why we needed to check if we've already parsed this header name. + for str_value in response.all(name) { + let header = vec![Value::string(name, span), Value::string(str_value, span)]; + vals.push(Value::record(cols.clone(), header, span)); } } } - Ok(Value::Record { cols, vals, span }.into_pipeline_data()) + Ok(Value::list(vals, span).into_pipeline_data()) } pub fn request_handle_response_headers(