Fix: return all headers with the same name from http <method> (#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 <method>`, but other
headers can be set multiple times as well.

# User-Facing Changes
- Changes the return type of an `http <method>` `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
<!-- If your PR had any user-facing changes, update [the
documentation](https://github.com/nushell/nushell.github.io) after the
PR is merged, if necessary. This will help us keep the docs up to date.
-->
- [ ] 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
This commit is contained in:
Julian Aichholz 2023-07-03 08:20:39 -07:00 committed by GitHub
parent 227d1d9508
commit 9c7e37f728
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23

View File

@ -544,29 +544,34 @@ pub fn request_handle_response_headers_raw(
span: Span,
response: &Response,
) -> Result<PipelineData, ShellError> {
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(