Skip to content

Commit

Permalink
fix(http1): code review fixes
Browse files Browse the repository at this point in the history
- remove uncessary if statement
- prefer into_iter() instead of drain()
- prefer !vec.is_empty() to vec.len() > 0
- add another test for multiple trailer headers in single response
  • Loading branch information
hjr3 committed Nov 11, 2023
1 parent 401dfaf commit 9cc26a5
Show file tree
Hide file tree
Showing 3 changed files with 50 additions and 18 deletions.
9 changes: 2 additions & 7 deletions src/proto/h1/conn.rs
Original file line number Diff line number Diff line change
Expand Up @@ -265,17 +265,12 @@ where
self.state.reading = Reading::Body(Decoder::new(msg.decode));
}

if msg
self.state.allow_trailer_fields = msg
.head
.headers
.get(TE)
.map(|te_header| te_header == "trailers")
.unwrap_or(false)
{
self.state.allow_trailer_fields = true;
} else {
self.state.allow_trailer_fields = false;
}
.unwrap_or(false);

Poll::Ready(Some(Ok((msg.head, msg.decode, wants))))
}
Expand Down
57 changes: 47 additions & 10 deletions src/proto/h1/encode.rs
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,7 @@ impl Encoder {

pub(crate) fn encode_trailers<B>(
&self,
mut trailers: HeaderMap,
trailers: HeaderMap,
title_case_headers: bool,
) -> Option<EncodedBuf<B>> {
match &self.kind {
Expand All @@ -175,7 +175,7 @@ impl Encoder {
let mut cur_name = None;
let mut allowed_trailers = HeaderMap::new();

for (opt_name, value) in trailers.drain() {
for (opt_name, value) in trailers.into_iter() {
if let Some(n) = opt_name {
cur_name = Some(n);
}
Expand Down Expand Up @@ -530,14 +530,18 @@ mod tests {
let trailers = vec![HeaderValue::from_static("chunky-trailer")];
let encoder = encoder.into_chunked_with_trailing_fields(trailers);

let mut headers = HeaderMap::new();
headers.insert(
HeaderName::from_static("chunky-trailer"),
HeaderValue::from_static("header data"),
);
headers.insert(
HeaderName::from_static("should-not-be-included"),
HeaderValue::from_static("oops"),
let headers = HeaderMap::from_iter(
vec![
(
HeaderName::from_static("chunky-trailer"),
HeaderValue::from_static("header data"),
),
(
HeaderName::from_static("should-not-be-included"),
HeaderValue::from_static("oops"),
),
]
.into_iter(),
);

let buf1 = encoder.encode_trailers::<&[u8]>(headers, false).unwrap();
Expand All @@ -547,6 +551,39 @@ mod tests {
assert_eq!(dst, b"0\r\nchunky-trailer: header data\r\n\r\n");
}

#[test]
fn chunked_with_multiple_trailer_headers() {
let encoder = Encoder::chunked();
let trailers = vec![
HeaderValue::from_static("chunky-trailer"),
HeaderValue::from_static("chunky-trailer-2"),
];
let encoder = encoder.into_chunked_with_trailing_fields(trailers);

let headers = HeaderMap::from_iter(
vec![
(
HeaderName::from_static("chunky-trailer"),
HeaderValue::from_static("header data"),
),
(
HeaderName::from_static("chunky-trailer-2"),
HeaderValue::from_static("more header data"),
),
]
.into_iter(),
);

let buf1 = encoder.encode_trailers::<&[u8]>(headers, false).unwrap();

let mut dst = Vec::new();
dst.put(buf1);
assert_eq!(
dst,
b"0\r\nchunky-trailer: header data\r\nchunky-trailer-2: more header data\r\n\r\n"
);
}

#[test]
fn chunked_with_no_trailer_header() {
let encoder = Encoder::chunked();
Expand Down
2 changes: 1 addition & 1 deletion src/proto/h1/role.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1350,7 +1350,7 @@ impl Client {
let allowed_trailer_fields: Vec<HeaderValue> =
headers.get_all(header::TRAILER).iter().cloned().collect();

if allowed_trailer_fields.len() > 0 {
if !allowed_trailer_fields.is_empty() {
return enc.into_chunked_with_trailing_fields(allowed_trailer_fields);
}
}
Expand Down

0 comments on commit 9cc26a5

Please sign in to comment.