Correctly include port in host header #23
Conversation
|
I think the third commit ("Remove unused export") should be squashed into "Remove use of deprecated chrono functions". "Therefore" is just one word. |
|
Oops, closed in error. Sorry! |
The correct form of the Host header is ``<host>:<port>`. If no port is included, the default port for the service requested is implied (e.g., 443 for an HTTPS URL, and 80 for an HTTP URL). reqwest's `host_str()` does not return a port, even if a non-default one is set, therefore we need to add it.
Also, remove unneeded export.
robalar
force-pushed
the
rh/fix-reqwest-host
branch
from
8882365 to
30c4b66
Compare
| fn it_works_blocking() { | ||
| let config = SigningConfig::new_default("test_key", "abcdefgh".as_bytes()); | ||
|
|
||
| let client = reqwest::blocking::Client::new(); |
There was a problem hiding this comment.
It took me a few minutes playing spot-the-difference to see that this line is the only difference between this test and the it_works test: I resorted to pasting the two of them into kdiff3 to display the difference.
Is it worth factoring out the duplicated code?
Something like
#[test]
fn it_works_blocking() {
it_works(reqwest::blocking::Client::new())
}
#[test]
fn it_works_async() {
it_works(reqwest::Client::new())
}or using a parameterisation library such as test-case.
It's the same for the other pair of tests: the ones specifying a non-standard port.
There was a problem hiding this comment.
they are completely seperate types, and don't implement a shared trait for the builder (as far as i can tell) so I dont think its possible to construct a function that takes both as an argument?
The correct form of the Host header is ``:`. If no port is included, the default port for the service requested is implied (e.g., 443 for an HTTPS URL, and 80 for an HTTP URL).
reqwest's
host_str()does not return a port, even if a non-default one is set, there for we need to add it.