feat(client): add support for title case header names at the socket level #1497
Conversation
src/proto/h1/conn.rs
Outdated
| @@ -649,6 +655,7 @@ impl fmt::Debug for State { | |||
| .field("keep_alive", &self.keep_alive) | |||
| .field("error", &self.error) | |||
| //.field("method", &self.method) | |||
| .field("title_case_headers", &self.title_case_headers) | |||
There was a problem hiding this comment.
I'm thinking that perhaps this field shouldn't be logged, since the State gets dumped somewhat frequently to help debug the current state, and seeing this longer field being the same constant forever will just be noisy.
There was a problem hiding this comment.
I agree with that reasoning. Should it be commented like the method field to indicate it isn't logged or just get rid of the line entirely?
| write_headers_title_case(&head.headers, dst); | ||
| } else { | ||
| write_headers(&head.headers, dst); | ||
| } |
There was a problem hiding this comment.
Since there's no way to set this for the server yet (and I'm tempted to leave it out until someone asks for it), the bool check is kind of a waste. Whatcha think of removing it from the ServerTransaction version here?
There was a problem hiding this comment.
Since ServerTransaction implements Http1Transaction, I cannot just remove it from the method signature as that would deviate from the trait definition, but I can prefix it with an underscore to signify that the boolean is unused.
| // Write header names as title case. The header name is assumed to be ASCII, | ||
| // therefore it is trivial to convert an ASCII character from lowercase to | ||
| // uppercase. It is as simple as XORing the lowercase character byte with | ||
| // space. |
There was a problem hiding this comment.
I believe this will get a couple characters changed completely. Specifically, | and ~ will become \\ and ^, respectively. Probably not super common, but it'd be a semantic change...
Also, it'd be nice to include some tests of title_case, perhaps at the bottom of the file.
There was a problem hiding this comment.
Oh, I forgot to think of those. A check to see if the byte falls between 97 (a) and 172 (z) should suffice.
There was a problem hiding this comment.
The z byte is 122, not 172 :D
Actually, to make it clearer, you can just write b'a' and b'z', which the compiler sees as the meaning the ASCII byte of the character.
There was a problem hiding this comment.
I read the octal field by accident from the man page. I fixed it to a >= b'a' and <= b'z'.
|
I noticed that the encode methods are not tested at the bottom of the file, but they are heavily tested in the integration tests of |
| @@ -857,6 +901,21 @@ mod tests { | |||
| Client::decoder(&head, method).unwrap_err(); | |||
| } | |||
|
|
|||
| #[test] | |||
| fn test_client_request_encode_title_case() { | |||
This introduces support for the HTTP/1 Client to write header names as title case when encoding the request. Closes #1492
|
I modified the client integration test macros to accept a If you feel that this change is unnecessary because of the unit test, then I can revert that file back to its original state. |
|
Hmm. The tests passed on my computer, but on Travis they flat out fail or crash. Let me know if they crash on your computer. |
|
The tests in CI failing are unrelated. I'm battling a race condition in the futures mpsc channel implementation, and still haven't gotten it working always, it seems. |
|
It looks good to me, thanks! |
|
Thank you very much! |
ghost
mentioned this pull request
This introduces support for the HTTP/1 Client to write header names as title case when encoding
the request.
Need to work on tests.
Closes #1492